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

Paul Walmsley
2008-04-25T09:36:53Z

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.

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.

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.

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.

regards,

- Paul