Messages in this thread |  | | Date | Sat, 26 Apr 2008 12:47:40 +0400 | From | Dmitry <> | Subject | Re: [PATCH 0/5] Clocklib: generic clocks framework |
| |
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.
-- With best wishes Dmitry
|  |