| Message ID | 1364553095-25110-4-git-send-email-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote: > The driver is initialized several times. This is wrong and if the > return code of the function was checked, it will return -EINVAL. > > Move this initialization out of the loop. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- Fix for this is already and v2 of the patch is here [1] Regards, Santosh [1] https://patchwork.kernel.org/patch/2329861/
On 03/29/2013 11:38 AM, Santosh Shilimkar wrote: > On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote: >> The driver is initialized several times. This is wrong and if the >> return code of the function was checked, it will return -EINVAL. >> >> Move this initialization out of the loop. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- > Fix for this is already and v2 of the patch is here [1] Ah, ok. Thanks for reviewing the patch. Can we find a solution to have a single entry point to sumbit patches for all the cpuidle drivers ? Otherwise, consolidating them is a pain: a patch for the samsung tree, another one for the at91 tree, etc ... and wait for all the trees to sync before continuing to consolidate the code. Wouldn't be worth to move these drivers under the PM umbrella instead of the SoC specific code ? Any idea to simplify the cpuidle consolidation and maintenance ? Thanks -- Daniel
On Friday 29 March 2013 04:15 PM, Daniel Lezcano wrote: > On 03/29/2013 11:38 AM, Santosh Shilimkar wrote: >> On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote: >>> The driver is initialized several times. This is wrong and if the >>> return code of the function was checked, it will return -EINVAL. >>> >>> Move this initialization out of the loop. >>> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>> --- >> Fix for this is already and v2 of the patch is here [1] > > Ah, ok. Thanks for reviewing the patch. > > Can we find a solution to have a single entry point to sumbit patches > for all the cpuidle drivers ? > > Otherwise, consolidating them is a pain: a patch for the samsung tree, > another one for the at91 tree, etc ... and wait for all the trees to > sync before continuing to consolidate the code. > > Wouldn't be worth to move these drivers under the PM umbrella instead of > the SoC specific code ? > > Any idea to simplify the cpuidle consolidation and maintenance ? > Fisrtly patches get posted to right mailing list based on where the code resides. So one must keep a watch on LAKML for the patches. Talking specific to OMAP idle code, there is plan to move to drivers/idle/* but for that to happen there are some PRM/CM dependency for which also driver movement is planned. Once that happen, OMAP idle will find its way in drivers/idle/*
On 03/29/2013 11:53 AM, Santosh Shilimkar wrote: > On Friday 29 March 2013 04:15 PM, Daniel Lezcano wrote: >> On 03/29/2013 11:38 AM, Santosh Shilimkar wrote: >>> On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote: >>>> The driver is initialized several times. This is wrong and if the >>>> return code of the function was checked, it will return -EINVAL. >>>> >>>> Move this initialization out of the loop. >>>> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> --- >>> Fix for this is already and v2 of the patch is here [1] >> >> Ah, ok. Thanks for reviewing the patch. >> >> Can we find a solution to have a single entry point to sumbit patches >> for all the cpuidle drivers ? >> >> Otherwise, consolidating them is a pain: a patch for the samsung tree, >> another one for the at91 tree, etc ... and wait for all the trees to >> sync before continuing to consolidate the code. >> >> Wouldn't be worth to move these drivers under the PM umbrella instead of >> the SoC specific code ? >> >> Any idea to simplify the cpuidle consolidation and maintenance ? >> > Fisrtly patches get posted to right mailing list based on where the > code resides. So one must keep a watch on LAKML for the patches. Yes, I agree. The main issue is the multiple tree for the different drivers making hard to track, modify and improve the drivers in one shot. It is not the first time, a modification of the cpuidle framework implied to modify all the drivers. When Rob introduced the first code consolidation, that took months to add a simple flag in the drivers because we had to wait for the merge before the changes in drivers/cpuidle/cpuidle.c were visible. > Talking specific to OMAP idle code, there is plan to move > to drivers/idle/* but for that to happen there are some PRM/CM > dependency for which also driver movement is planned. Once > that happen, OMAP idle will find its way in drivers/idle/* That would be *really* great. If we can do that for all the drivers, that will solve the multi-location / multi-tree problem. The u8500 driver will be moved soon to this directory also. I did some modifications around the at91 some months ago to encapsulate the code more, maybe it could be also a good candidate. Nicolas ? For OMAP3 that could be a bit more difficult. Who is maintaining the driver now ? I Cc'ed the different maintainers for the other boards, may be they can react ? Thanks -- Daniel
On Friday 29 March 2013 04:53 PM, Daniel Lezcano wrote: > On 03/29/2013 11:53 AM, Santosh Shilimkar wrote: >> On Friday 29 March 2013 04:15 PM, Daniel Lezcano wrote: [..] > For OMAP3 that could be a bit more difficult. Who is maintaining the > driver now ? > The driver is still maintained by Kevin Hilman and quite a few us keep that driver upto date. OMAP3 idle is movable as well. A while back I have assessed move of OMAP3 and OMAP4 idle drivers, and the only issue was the other PRM/CM dependency. Regards, Santosh
On Fri, Mar 29, 2013 at 4:15 PM, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > On 03/29/2013 11:38 AM, Santosh Shilimkar wrote: >> On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote: >>> The driver is initialized several times. This is wrong and if the >>> return code of the function was checked, it will return -EINVAL. >>> >>> Move this initialization out of the loop. >>> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>> --- >> Fix for this is already and v2 of the patch is here [1] > > Ah, ok. Thanks for reviewing the patch. > > Can we find a solution to have a single entry point to sumbit patches > for all the cpuidle drivers ? > > Otherwise, consolidating them is a pain: a patch for the samsung tree, > another one for the at91 tree, etc ... and wait for all the trees to > sync before continuing to consolidate the code. > > Wouldn't be worth to move these drivers under the PM umbrella instead of > the SoC specific code ? > > Any idea to simplify the cpuidle consolidation and maintenance ? Adding Arnd and Olof to this discussion since atleast the ARM drivers go through their arm-soc tree. Given the work you're putting in to consolidate the drivers, perhaps they can insist that idle drivers get acked by you? /Amit
On Friday 29 March 2013 05:26 PM, Amit Kucheria wrote: > On Fri, Mar 29, 2013 at 4:15 PM, Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> On 03/29/2013 11:38 AM, Santosh Shilimkar wrote: >>> On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote: >>>> The driver is initialized several times. This is wrong and if the >>>> return code of the function was checked, it will return -EINVAL. >>>> >>>> Move this initialization out of the loop. >>>> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> --- >>> Fix for this is already and v2 of the patch is here [1] >> >> Ah, ok. Thanks for reviewing the patch. >> >> Can we find a solution to have a single entry point to sumbit patches >> for all the cpuidle drivers ? >> >> Otherwise, consolidating them is a pain: a patch for the samsung tree, >> another one for the at91 tree, etc ... and wait for all the trees to >> sync before continuing to consolidate the code. >> >> Wouldn't be worth to move these drivers under the PM umbrella instead of >> the SoC specific code ? >> >> Any idea to simplify the cpuidle consolidation and maintenance ? > > Adding Arnd and Olof to this discussion since atleast the ARM drivers > go through their arm-soc tree. > > Given the work you're putting in to consolidate the drivers, perhaps > they can insist that idle drivers get acked by you? > Not to create controversy but as a general rule there is nothing like *insisting* ack on patches for merge apart from the official maintainers(gate keepers). Having said that, its always good to get more reviews and acks so that better code gets merged. This just my personal opinion. Regards, Santosh
On Fri, Mar 29, 2013 at 5:50 PM, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > On Friday 29 March 2013 05:26 PM, Amit Kucheria wrote: >> On Fri, Mar 29, 2013 at 4:15 PM, Daniel Lezcano >> <daniel.lezcano@linaro.org> wrote: >>> On 03/29/2013 11:38 AM, Santosh Shilimkar wrote: >>>> On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote: >>>>> The driver is initialized several times. This is wrong and if the >>>>> return code of the function was checked, it will return -EINVAL. >>>>> >>>>> Move this initialization out of the loop. >>>>> >>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>>> --- >>>> Fix for this is already and v2 of the patch is here [1] >>> >>> Ah, ok. Thanks for reviewing the patch. >>> >>> Can we find a solution to have a single entry point to sumbit patches >>> for all the cpuidle drivers ? >>> >>> Otherwise, consolidating them is a pain: a patch for the samsung tree, >>> another one for the at91 tree, etc ... and wait for all the trees to >>> sync before continuing to consolidate the code. >>> >>> Wouldn't be worth to move these drivers under the PM umbrella instead of >>> the SoC specific code ? >>> >>> Any idea to simplify the cpuidle consolidation and maintenance ? >> >> Adding Arnd and Olof to this discussion since atleast the ARM drivers >> go through their arm-soc tree. >> >> Given the work you're putting in to consolidate the drivers, perhaps >> they can insist that idle drivers get acked by you? >> > Not to create controversy but as a general rule there is nothing > like *insisting* ack on patches for merge apart from the official > maintainers(gate keepers). > > Having said that, its always good to get more reviews and acks so > that better code gets merged. > > This just my personal opinion. I'm not asking for special treatment here. :) I'm requesting one set of maintainers (arm-soc maintainers) to push back on changes that don't get platform idle drivers in sync with the consolidation work that is currently ongoing. This will speed up the process since it is hard to track every SoC-specific list for these changes. Some platform maintainers might not even be aware of it (those that Daniel hasn't modified yet). A similar approach seems to have worked for common clock, DT, pinmux, etc. /Amit
On Friday 29 March 2013 06:20 PM, Amit Kucheria wrote: > On Fri, Mar 29, 2013 at 5:50 PM, Santosh Shilimkar > <santosh.shilimkar@ti.com> wrote: >> On Friday 29 March 2013 05:26 PM, Amit Kucheria wrote: >>> On Fri, Mar 29, 2013 at 4:15 PM, Daniel Lezcano >>> <daniel.lezcano@linaro.org> wrote: >>>> On 03/29/2013 11:38 AM, Santosh Shilimkar wrote: >>>>> On Friday 29 March 2013 04:01 PM, Daniel Lezcano wrote: >>>>>> The driver is initialized several times. This is wrong and if the >>>>>> return code of the function was checked, it will return -EINVAL. >>>>>> >>>>>> Move this initialization out of the loop. >>>>>> >>>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>>>> --- >>>>> Fix for this is already and v2 of the patch is here [1] >>>> >>>> Ah, ok. Thanks for reviewing the patch. >>>> >>>> Can we find a solution to have a single entry point to sumbit patches >>>> for all the cpuidle drivers ? >>>> >>>> Otherwise, consolidating them is a pain: a patch for the samsung tree, >>>> another one for the at91 tree, etc ... and wait for all the trees to >>>> sync before continuing to consolidate the code. >>>> >>>> Wouldn't be worth to move these drivers under the PM umbrella instead of >>>> the SoC specific code ? >>>> >>>> Any idea to simplify the cpuidle consolidation and maintenance ? >>> >>> Adding Arnd and Olof to this discussion since atleast the ARM drivers >>> go through their arm-soc tree. >>> >>> Given the work you're putting in to consolidate the drivers, perhaps >>> they can insist that idle drivers get acked by you? >>> >> Not to create controversy but as a general rule there is nothing >> like *insisting* ack on patches for merge apart from the official >> maintainers(gate keepers). >> >> Having said that, its always good to get more reviews and acks so >> that better code gets merged. >> >> This just my personal opinion. > > I'm not asking for special treatment here. :) I'm requesting one set > of maintainers (arm-soc maintainers) to push back on changes that > don't get platform idle drivers in sync with the consolidation work > that is currently ongoing. > > This will speed up the process since it is hard to track every > SoC-specific list for these changes. Some platform maintainers might > not even be aware of it (those that Daniel hasn't modified yet). A > similar approach seems to have worked for common clock, DT, pinmux, > etc. > Every patch gets pulled into arm-soc/arm-core has to be posted on LAKML. So as long as everybody follows that rule, there is no need to track every SoC lists. And what I have seen so far every this rule has been followed well. Regards, Santosh
diff --git a/arch/arm/mach-omap2/cpuidle44xx.c b/arch/arm/mach-omap2/cpuidle44xx.c index f4b1b23..3d33b8a 100644 --- a/arch/arm/mach-omap2/cpuidle44xx.c +++ b/arch/arm/mach-omap2/cpuidle44xx.c @@ -210,6 +210,7 @@ int __init omap4_idle_init(void) { struct cpuidle_device *dev; unsigned int cpu_id = 0; + int ret; mpu_pd = pwrdm_lookup("mpu_pwrdm"); cpu_pd[0] = pwrdm_lookup("cpu0_pwrdm"); @@ -222,14 +223,18 @@ int __init omap4_idle_init(void) if (!cpu_clkdm[0] || !cpu_clkdm[1]) return -ENODEV; + ret = cpuidle_register_driver(&omap4_idle_driver); + if (ret) { + printk(KERN_ERR "failed to register the idle driver\n"); + return ret; + } + for_each_cpu(cpu_id, cpu_online_mask) { dev = &per_cpu(omap4_idle_dev, cpu_id); dev->cpu = cpu_id; #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED dev->coupled_cpus = *cpu_online_mask; #endif - cpuidle_register_driver(&omap4_idle_driver); - if (cpuidle_register_device(dev)) { pr_err("%s: CPUidle register failed\n", __func__); return -EIO;
The driver is initialized several times. This is wrong and if the return code of the function was checked, it will return -EINVAL. Move this initialization out of the loop. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- arch/arm/mach-omap2/cpuidle44xx.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)