From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752285AbbCYWCH (ORCPT ); Wed, 25 Mar 2015 18:02:07 -0400 Received: from mail-wi0-f173.google.com ([209.85.212.173]:33889 "EHLO mail-wi0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751602AbbCYWCE (ORCPT ); Wed, 25 Mar 2015 18:02:04 -0400 Message-ID: <55133058.6020603@linaro.org> Date: Wed, 25 Mar 2015 23:02:00 +0100 From: Daniel Lezcano User-Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Paul Bolle CC: rjw@rjwysocki.net, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-pm@vger.kernel.org, lina.iyer@linaro.org, Lorenzo.Pieralisi@arm.com Subject: Re: [PATCH 6/8] ARM: cpuidle: Enable the ARM64 driver for both ARM32/ARM64 References: <55127A53.8050206@linaro.org> <1427274436-21916-1-git-send-email-daniel.lezcano@linaro.org> <1427274436-21916-6-git-send-email-daniel.lezcano@linaro.org> <1427316850.10958.58.camel@x220> In-Reply-To: <1427316850.10958.58.camel@x220> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/25/2015 09:54 PM, Paul Bolle wrote: > A few nits are all I spotted. > > On Wed, 2015-03-25 at 10:07 +0100, Daniel Lezcano wrote: >> --- a/drivers/cpuidle/Kconfig.arm >> +++ b/drivers/cpuidle/Kconfig.arm > >> +config ARM_CPUIDLE >> + bool "Generic ARM/ARM64 CPU idle Driver" >> + select DT_IDLE_STATES >> + help >> + Select this to enable generic cpuidle driver for ARM. >> + It provides a generic idle driver whose idle states are configured >> + at run-time through DT nodes. The CPUidle suspend backend is >> + initialized by calling the CPU operations init idle hook >> + provided by architecture code. > > Start with a tab instead of 8 spaces, please. Ok, I will send a fix for that on top of this patch. >> --- /dev/null >> +++ b/drivers/cpuidle/cpuidle-arm.c > >> +#include >> +#include >> +#include >> +#include >> +#include > > Is this include actually needed? I haven't tried building this without > that include, but I spotted nothing obviously module related in this > file. > >> +#include >> + >> +#include >> + >> +#include "dt_idle_states.h" > >> +static struct cpuidle_driver arm_idle_driver = { >> + .name = "arm_idle", >> + .owner = THIS_MODULE, > > Since this can only be built in, THIS_MODULE will (basically) be > equivalent to NULL according to include/linux/export.h. So I suppose > this line can be dropped. Yeah, THIS_MODULE is present in all drivers. Very likely we can safely remove it. >> + /* >> + * State at index 0 is standby wfi and considered standard >> + * on all ARM platforms. If in some platforms simple wfi >> + * can't be used as "state 0", DT bindings must be implemented >> + * to work around this issue and allow installing a special >> + * handler for idle state index 0. >> + */ >> + .states[0] = { >> + .enter = arm_enter_idle_state, >> + .exit_latency = 1, >> + .target_residency = 1, >> + .power_usage = UINT_MAX, >> + .name = "WFI", >> + .desc = "ARM WFI", >> + } >> +}; > > I did notice that these two nits are already present in > drivers/cpuidle/cpuidle-arm64.c. But since this patch is not just moving > that file's content around, you might as well look into those two nits. We can address the module code removal in a separate patchset as this is the case for all the drivers. -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog From mboxrd@z Thu Jan 1 00:00:00 1970 From: daniel.lezcano@linaro.org (Daniel Lezcano) Date: Wed, 25 Mar 2015 23:02:00 +0100 Subject: [PATCH 6/8] ARM: cpuidle: Enable the ARM64 driver for both ARM32/ARM64 In-Reply-To: <1427316850.10958.58.camel@x220> References: <55127A53.8050206@linaro.org> <1427274436-21916-1-git-send-email-daniel.lezcano@linaro.org> <1427274436-21916-6-git-send-email-daniel.lezcano@linaro.org> <1427316850.10958.58.camel@x220> Message-ID: <55133058.6020603@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 03/25/2015 09:54 PM, Paul Bolle wrote: > A few nits are all I spotted. > > On Wed, 2015-03-25 at 10:07 +0100, Daniel Lezcano wrote: >> --- a/drivers/cpuidle/Kconfig.arm >> +++ b/drivers/cpuidle/Kconfig.arm > >> +config ARM_CPUIDLE >> + bool "Generic ARM/ARM64 CPU idle Driver" >> + select DT_IDLE_STATES >> + help >> + Select this to enable generic cpuidle driver for ARM. >> + It provides a generic idle driver whose idle states are configured >> + at run-time through DT nodes. The CPUidle suspend backend is >> + initialized by calling the CPU operations init idle hook >> + provided by architecture code. > > Start with a tab instead of 8 spaces, please. Ok, I will send a fix for that on top of this patch. >> --- /dev/null >> +++ b/drivers/cpuidle/cpuidle-arm.c > >> +#include >> +#include >> +#include >> +#include >> +#include > > Is this include actually needed? I haven't tried building this without > that include, but I spotted nothing obviously module related in this > file. > >> +#include >> + >> +#include >> + >> +#include "dt_idle_states.h" > >> +static struct cpuidle_driver arm_idle_driver = { >> + .name = "arm_idle", >> + .owner = THIS_MODULE, > > Since this can only be built in, THIS_MODULE will (basically) be > equivalent to NULL according to include/linux/export.h. So I suppose > this line can be dropped. Yeah, THIS_MODULE is present in all drivers. Very likely we can safely remove it. >> + /* >> + * State at index 0 is standby wfi and considered standard >> + * on all ARM platforms. If in some platforms simple wfi >> + * can't be used as "state 0", DT bindings must be implemented >> + * to work around this issue and allow installing a special >> + * handler for idle state index 0. >> + */ >> + .states[0] = { >> + .enter = arm_enter_idle_state, >> + .exit_latency = 1, >> + .target_residency = 1, >> + .power_usage = UINT_MAX, >> + .name = "WFI", >> + .desc = "ARM WFI", >> + } >> +}; > > I did notice that these two nits are already present in > drivers/cpuidle/cpuidle-arm64.c. But since this patch is not just moving > that file's content around, you might as well look into those two nits. We can address the module code removal in a separate patchset as this is the case for all the drivers. -- Linaro.org ? Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog