Message-ID: <alpine.DEB.1.00.0804250221040.14755@utopia.booyaka.com>
Date:	Fri, 25 Apr 2008 03:36:38 -0600 (MDT)
From:	Paul Walmsley <paul@...an.com>
To:	Dmitry <dbaryshkov@...il.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

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.

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
--
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