Date:	Sat, 26 Apr 2008 12:47:40 +0400
From:	Dmitry <dbaryshkov@...il.com>
To:	"Paul Walmsley" <paul@...an.com>
Cc:	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
	"Haavard Skinnemoen" <haavard.skinnemoen@...el.com>,
	"Russell King" <rmk+lkml@....linux.org.uk>,
	"Paul Mundt" <lethal@...ux-sh.org>,
	"pHilipp Zabel" <philipp.zabel@...il.com>,
	"Pavel Machek" <pavel@....cz>, tony@...mide.com,
	"David Brownell" <david-b@...bell.net>, hiroshi.DOYU@...ia.com
Subject: Re: [PATCH 0/5] Clocklib: generic clocks framework

Hi,

2008/4/25, Paul Walmsley <paul@...an.com>:
> Hello Dmitry,
>
>
>  On Mon, 21 Apr 2008, Dmitry wrote:
>
>  > 2008/4/21, Paul Walmsley <paul@...an.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists