Messages in this thread | | | Date | Wed, 25 Mar 2015 23:02:00 +0100 | From | Daniel Lezcano <> | Subject | Re: [PATCH 6/8] ARM: cpuidle: Enable the ARM64 driver for both ARM32/ARM64 |
| |
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 <linux/cpuidle.h> >> +#include <linux/cpumask.h> >> +#include <linux/cpu_pm.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> > > 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 <linux/of.h> >> + >> +#include <asm/cpuidle.h> >> + >> +#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.
-- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
| |