[PATCH 1/2] Add a common struct clk

[Uwe Kleine-König]

On Fri, Dec 10, 2010 at 09:58:31AM +0800, Jeremy Kerr wrote:
> Hi Uwe
> 
> > > +/**
> > > + * clk_ops: Callback operations for clocks; these are to be provided by
> > > the + * clock implementation, and will be called by drivers through the
> > > clk_* API. + *
> > > + * @enable:	Enable the clock. This must not return until the clock is
> > > + *		generating a valid clock signal, usable by consumer devices.
> > > + *		Called with clk->lock held.
> > > + *
> > > + * @disable:	Disable the clock. Called with clk->lock held.
> > > + *
> > > + * @get	/ @put:	Called by the core clock code to notify the driver 
> about
> > 
> > I wonder if this is valid kerneldoc.  The tab before / looks (IMHO)
> > ugly.
> 
> Not valid kernel doc, so I'll fix that up. The tab was unintentional.
> 
> > Maybe specify "driver" a bit more to distinguish from "drivers"
> > above.  "clk_ops driver"?
> 
> This is actually for refcounting for uses by device drivers (ie, not the clock 
> provider), I've updated the comment:
> 
> /**
>  * struct clk_ops -  Callback operations for clocks; these are to be provided
>  * by the clock implementation, and will be called by drivers through the
>  * clk_* API.
>  *
>  * @enable:	Enable the clock. This must not return until the clock is
>  *		generating a valid clock signal, usable by consumer devices.
>  *		Called with clk->lock held.
>  *
>  * @disable:	Disable the clock. Called with clk->lock held.
>  *
>  * @get:	     Called by the core clock code to increment the clock's
>  *		     refount as clk is passed to device drivers. Optional.
s/refount/refcount/ (once more below)
>  *
>  * @put:	     Called by the core clock code to decrement the clocks's
>  *		     refounts as clk is released from device drivers. Optional.
again inconsistent tabbing.

IMHO the wording above is better.  This makes me unsure if the callback
has to fiddle with the refcount.

>  *
>  * For other callbacks, see the corresponding clk_* functions. Parameters and
>  * return values are passed directly from/to these API functions, or
>  * -ENOSYS is returned if the callback is NULL, see kernel/clk.c for
This is not true for clk_get_rate.  This returns 0 if the callback isn't
set.

>  * implementation details. All are optional.
>  */
> 
> > > +/**
> > > + * __clk_get - update clock-specific refcounter
> > > + *
> > > + * @clk: The clock to refcount
> > 
> > "The clock to update the refcount for"?
> 
> I'm using refcount as a verb here; if this isn't clear I can come up with 
> something else. Your solution splits the 'clock' and the 'for' which may be 
> difficult to parse too. Let me know if you have any other suggestions :)
hmm, don't know.  Keeping it as is is OK for me, too, if you don't
consider my suggestion to be better.
 
> > I wonder if it's worth to handle parents here, e.g.
> > 
> > 	if (!clk->enable_count) {
> > 		struct clk *parent = clk_get_parent(clk);
> > 		if (parent) {
> > 			ret = clk_enable(parent);
> > 			if (ret)
> > 				return ret;
> > 		}
> > 
> > 		ret = clk->ops->enable(clk);
> > 		if (likely(!ret))
> > 			clk->enable_count++;
> > 		else if (parent)
> > 			clk_disable(parent);
> > 	}
> > 
> > as they are quite common.
> 
> I'm not convinced we should do the parent handling in the core clock code. 
> It's fairly easy to do the parent enable/disable in platform code, which 
> should have explicit knowledge about whether or not the clock has a parent, 
> and the semantics of how the parent/child clocks interact.
> 
> However, happy to discuss this further if needs be.
As already said in #armlinux, in the meantime I agree.

> > > +void clk_disable(struct clk *clk)
> > > +{
> > > +	if (!clk->ops->disable)
> > > +		return;
> > 
> > 	WARN_ON(!clk->enable_count) ?
> 
> Yep, good idea. I'll do this check with the lock acquired.
fine.

> Thanks for the comments, I've updated my tree accordingly (along with some 
> other kerneldoc fixups). I'll wait to see if there is any other feedback and 
> re-post next week.
fine, too.

Thanks for your efforts
Uwe

PS: I don't know who looks at your git tree, but you might want to
update the arch branches to base on your current work.

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |