Re: [RFC,PATCH 1/3] Add a common struct clk

[Jeremy Kerr <jeremy.kerr@canonical.com>]

Hi Ryan,

> > +int clk_prepare(struct clk *clk)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!clk->ops->prepare)
> > +		return 0;
> 
> If there is no ops->prepare function then we never increment
> prepare_count, which means that driver writers can get sloppy if they
> know that ops->prepare is no-op on their platform since they will not
> get warned for omitting clk_prepare.

Yeah, as discussed in other replies, it's probably best that we do the 
counting unconditionally. I've removed these optimisations - I think we'd best 
enforce the checking here, at least at the introduction of this API.
 
> Also, why are the warnings added in a separate patch rather than being
> rolled into this patch?

Just splitting things up; the warnings were the most discussed issue 
previously, so I wanted to separate that discussion from the API side.

> Again, you should still increment enable_count even if ops->enabled is a
> no-op since it provides valuable warnings when clk_enable/disable calls
> are not matched correctly.

Yep, as above.

> > +unsigned long clk_get_rate(struct clk *clk)
> > +{
> > +	if (clk->ops->get_rate)
> > +		return clk->ops->get_rate(clk);
> 
> Possibly we should shadow the clock rate if ops->get_rate is no-op? So
> clock initialisation and clk_set_rate store the rate in the shadow
> field, and then do:
> 
> 	if (clk->ops->get_rate)
> 		return clk->ops->get_rate(clk);
> 	return clk->shadow_rate;
> 
> Because the API is generic, driver writers should reasonably expect that
> clk_get_rate will return something valid without having to know the
> platform implementation details. It may also be worth having a warning
> to let the user know that the returned rate may be approximate.

I'd prefer to require that get_rate is implemented as an op, rather than 
allowing two methods for retrieving the rate of the clock.

> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(clk_get_rate);
> > +
> > +int __clk_get(struct clk *clk)
> > +{
> > +	if (clk->ops->get)
> > +		return clk->ops->get(clk);
> > +	return 1;
> > +}
> > +EXPORT_SYMBOL_GPL(__clk_get);
> > +
> > +void clk_put(struct clk *clk)
> > +{
> > +	if (clk->ops->put)
> > +		clk->ops->put(clk);
> > +}
> > +EXPORT_SYMBOL_GPL(clk_put);
> 
> This has probably been covered, and I have probably missed it, but why
> don't the generic clk_get/put functions do ref-counting? Drivers must
> have matched clk_get/put calls so it should work like enable/prepare
> counting right?

clk_get is used to find a clock; most implementations will not use this for 
refcounting.

However, for the case where clocks are dynamically allocated, we need clk_put 
to do any possible freeing. There's an existing API for this type of reference 
counting (kref), so for the cases where this matters, the clock 
implementations can use that.

> > + * The choice of atomic or non-atomic clock depends on how the clock is
> > enabled. + * Typically, you'll want to use a non-atomic clock. For
> > clocks that need to be + * enabled/disabled in interrupt context, use
> > CLK_ATOMIC. Note that atomic + * clocks with parents will typically
> > cascade enable/disable operations to + * their parent, so the parent of
> > an atomic clock *must* be atomic too.
> 
> This comment seems out of date now that we have the prepare/enable
> semantics?

Yep, will update.

> > + * @unprepare:	Release the clock from its prepared state. This will
> > typically + *		undo any work done in the @prepare callback. Called 
> > with + *		clk->prepare_lock held.
> 
> I think you need to make it more clear the prepare/unprepare must be
> called from a sleepable context.

The documentation on clk_ops is intended for the clock implementor, so it's 
not really the right place to descibe the caller's requirements.

Indeed, the documentation for clk_prepare & clk_unprepare describe the 
caller's requirements for these (and contain the words "This function may 
sleep").

> > + * Typically, drivers will call clk_prepare when a clock may be needed
> > later + * (eg. when a device is opened), and clk_enable when the clock
> > is actually + * required (eg. from an interrupt).
> 
> Drivers _must_ call clk_prepare before clk_enable (not typically)?

This 'typically' is about the actual placement of the clk_prepare and 
clk_enable calls in the driver code, but I will clarify.

> > +/**
> > + * __clk_get - update clock-specific refcounter
> > + *
> > + * @clk: The clock to refcount
> > + *
> > + * Before a clock is returned from clk_get, this function should be
> > called + * to update any clock-specific refcounting.
> > + *
> > + * Returns non-zero on success, zero on failure.
> > + *
> > + * Drivers should not need this function; it is only needed by the
> > + * arch-specific clk_get() implementations.
> > + */
> > +int __clk_get(struct clk *clk);
> 
> I don't understand this. Are architectures supposed to provide a
> function called clk_get? Doesn't this break the whole idea of having a
> common struct clk?

clk_get() is now provided in drivers/clk/clkdev.c; the arch-specific part of 
this comment is old (I'll remove it).

Thanks for taking the time to review, I appreciate it.

Cheers,


Jeremy