Re: [PATCH 0/5] Clocklib: generic clocks framework

[David Brownell <david-b@pacbell.net>]

On Monday 21 April 2008, Dmitry wrote:
> >  - I don't think that I understand the clk_functions part of your code.
> >   Is this a shorthand to construct aliases to other struct clks?
>
> Yes, that's one of usages for it. E.g. current AT91 code has same
> functionality named at91_clock_associate.

As in:

	at91_clock_associate("usart0_clk", &pdev0->dev, "usart");
	at91_clock_associate("usart1_clk", &pdev1->dev, "usart");
	at91_clock_associate("usart2_clk", &pdev2->dev, "usart");

That essentially maps a device and logical clock name to a specific
clock (those "usartX_clk" identifiers are global), so that drivers
can clk_get(dev, "usart") and get the right clock.  It decouples
clock and device declarations, which should help when integrating
external clocks too.

The assumption that clk_get(NULL, "usartX_clk") works is mostly
just a convenience, although it's probably a fair assumption for
many other SOC platforms.  Conceptually that first parameter is
a clock, not a name.


> > +struct clk_function {
> > +       const char *parent;
> > +       void *priv;
> > +       struct clk clk;
> > +};

That doesn't really seem attuned to the task of associating
logical clock names to devices.  Wouldn't it be better to
have something directly addressing that core mechanism?


> Also once we get to multiple chips providers/users, we'll see,
> that the clock simply can't have just one record in the clocks tree.

I don't follow.  Why not?  If a clock has multiple records, I'd
expect its refcounts would easily get borked.  I think I don't
like your notion of a "wrapper clock".


> It's provided by some
> pin (provider_name) and then consumed by several devices (several
> consumer_name + consumer_device pairs). That is it.

There would still be just one clock though.  It shouldn't matter
how clk_get(this_dev, "c1") and clk_get(that_dev, "c2") find it.
Hashtable, linked list, divination, ... so long as it works.

Those other records should just hold {dev, name} ==> clock mappings;
they wouldn't be records for the clock itself.  But those "functions"
do not seem to record just mappings.

- Dave