On Tue, Aug 07, 2007 at 10:21:06AM -0700, Andrew Morton wrote: > On Tue, 7 Aug 2007 13:50:54 +0100 Russell King <rmk@xxxxxxxxxxxxxxxx> wrote: > > > I do not think that the clk_must_disable() API is well enough thought out > > for the following reasons: > > > > 1. the name sucks - it tells you nothing about it's purpose, which as > > the name currently stands can be interpreted in as many ways as there > > are species of animals on this planet. > > > > While the comments around the prototype help interpret its semantics, > > it is no subsitute for having a good name for the function. > > > > 2. it's unclear how this function obtains information about the "upcoming > > system state" and therefore decides whether the particular clock may > > be available. > > > > 3. due to the negative semantics, code such as the following is difficult > > to interpret and work out whether it's correct due to the double > > negative: > > > > + if (device_may_wakeup(&pdev->dev) > > + && !clk_must_disable(atmel_port->clk)) > > enable_irq_wake(port->irq); > > > > 4. the description of the function implies that this function may be > > called when we are not suspending: > > > > + * On platforms that support reduced functionality operating states, the > > + * constraint may also need to be tested during resume() and probe() calls. > > > > With SoCs with multiple power states affecting which clocks are > > available, and the need in point (2) for the architecture code to > > record which PM mode we're entering via the pm_ops set_target method, > > calling clk_must_disable() outside of the suspend methods results in > > this function essentially returning undefined values at driver probe > > time. Note: there is no locking between driver probing and the > > set_target method. > > > > Moreover, if used in a driver probe() path, the return value could > > well depend on the _last_ system suspend state entered, and would be > > undefined for a system which hasn't been suspended from boot. > > > > Changing the function name to "clk_available_in_suspend()" addresses at > > least two of these points. The other two points are addressed by > > providing a way for the method to be passed the desired system suspend > > state, which may be resolved by expanding pm_message_t to contain that > > information. > > I see, thanks. clk_available_in_suspend() sure is a better name. > > > Finally, concerning merging this during the -rc phase, I'd much rather > > see the one liner simple build fix of adding the missing function > > prototype going into the -rc kernels, and then a similar patch to this > > going in during the next merge window. > > Here's where confusion sets in. I have this: See 2/2. Both patches need to be looked at together. -- Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
