Re: [patch 2.6.23-rc2 1/2] define clk_must_disable()

[Russell King <rmk@arm.linux.org.uk>]

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@arm.linux.org.uk> 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: