On Wed, 2009-08-26 at 13:27 +0200, Peter Zijlstra wrote: > On Wed, 2009-08-26 at 16:40 +0530, Arun R Bharadwaj wrote: > > +void (*pm_idle)(void); > > +EXPORT_SYMBOL_GPL(pm_idle); > > Seriously.. this caused plenty problems over on x86 and you're doing the > exact same dumb thing?
I already said I didn't want this export. First thing first: We already have a ppc_md.power_save() callback, filled by the platform. which implements the "low level" part of idle on powerpc (ie, the actual putting of the CPU into some kind of wait state) and is called by our idle loop. We -also- have a higher level ppc_md.idle_loop() which allows the platform to completely override the idle loop, though we rarely do it (I think only iSeries does it nowadays). So I see no need to -add- another callback here. I'm not entirely sure what the cpuidle framework does, it's no obvious from Arun commit messages I must say, but it doesn't look like the right approach for integration. In fact, pSeries already have a choice between different powersave models depending on what kind of hypervisor is there. So Arun, please try to fit nicely within the existing interfaces, or if you want to add a new one, please justify very precisely what design decisions lead you to that. Finally, there's also a problem with your first patch 1/2: You are setting a Kconfig flag unconditionally indicating that the arch supports some idle wait function, but you only implement it somewhere in arch/powerpc/platform/pseries, so you'll break the build of any other platform. You also prevent another platform to implement a different one and be built in the same kernel. For such generic callbacks, if it's justified (and only if it is), you can add a ppc_md. hook for the platform to fill, and you need to cater for platforms that don't. Cheers, Ben. _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
