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

Dmitry
2008-04-26T08:47:53Z

Hi,

2008/4/25, Paul Walmsley <paul@pwsan.com>:
> Hello Dmitry,
>
>
> On Mon, 21 Apr 2008, Dmitry wrote:
>
> > 2008/4/21, Paul Walmsley <paul@pwsan.com>:
> > >
>
> > > If the latter, your patchset will presumably
> > > need a higher standard of review, since once it is integrated, any
> > > changes will affect several architectures, rather than simply one.
>
> > [...]
>
> > I've reviewed the code for most linux/clk.h implementations in the
> > kernel. The OMAP code was... a bit scary for me. I don't have any deep
> > knowledge of this platform, and there were lots of structures, lots of
> > structs embedding struct clk, etc.
> > Tell me please, what do you need, that can't be done with this framework?
>
>
> I wouldn't pretend to have a comprehensive list at this point. But from a
> brief look, your clk_round_rate() and clk_set_rate() implementations will
> not work for the present OMAP clock tree. In OMAP, many parent clocks do
> not have the same rate as their children.

You can easily override any calculations in your clk->set_rate/clk->round_rate/
clk->get_rate functions.

>
> But that is not really the main issue. Ultimately, as long as your
> implementation remains completely optional, and any public interfaces
> (like debugfs/sysfs interfaces) are coordinated with other clock interface
> implementors, I personally have no issues with your code going into the
> tree somewhere. With the possible exception of the clk_functions code,
> clocklib looks pretty clean to me, and a good exemplar of a clock
> interface implementation.

My first goals are:
1) to have an infrastructure to plug in my clocks in a platform-independant way
2) to drop lots of copies of nearly the same code.

I certainly do not plan to force all platforms to use this framework. However,
I think that would be good if most of them can be converted to clklib.

> However: if the ultimate goal is to make your implementation normative,
> then I concur with Russell, and I would recommend against merging.
> Assumptions that you make in clocklib may not work well for future chips.
> Changing clocklib will affect many architectures. For example, perhaps
> someone may wish to implement clocks as an actual in-memory tree rather
> than a list. Or perhaps someone may need to handle clock usecounting
> differently, for situations when multiple clocks might share the same
> enable/disable code, but with different parents.

Sorry, but WTF? Do you prefer to keep a lot of code and disallow
merging a generification just because of some-that-may-even-not-exist
cases? That sounds
pretty... strange.
And your examples are really strange.

>
> I am concerned that having any implementation as an implicit standard in
> the tree would increase the risk that, over time, internal implementation
> assumptions will be considered normative. This could easily cause more
> pain in the future for maintainers than it would be worth.
>
>
> > This was already discussed. It was suggested to use struct embedding and
> > container_of, instead of pointers. If you do really need a pointer, you
> > can writes
> > struct my_clk {
> > void *data;
> > struct clk clk;
> > };
>
>
> OK.
>
>
> > > - 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. Also onece we get to multiple
> > chips providers/users, we'll see, that the clock simply can't have just
> > one record in the clocks tree. It's provided by some pin (provider_name)
> > and then consumed by several devices (several consumer_name +
> > consumer_device pairs). That is it.
>
>
> Perhaps you might consider renaming these functions, perhaps "dynamic"
> clocks or "alias" clocks or something similar? The word "function" has
> some other strong associations which I found confusing when I read the
> code.

I'll think about this.