From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759966AbYDZIrx (ORCPT ); Sat, 26 Apr 2008 04:47:53 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753171AbYDZIrn (ORCPT ); Sat, 26 Apr 2008 04:47:43 -0400 Received: from rv-out-0708.google.com ([209.85.198.246]:9399 "EHLO rv-out-0506.google.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753412AbYDZIrk (ORCPT ); Sat, 26 Apr 2008 04:47:40 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references; b=K3C58v49l+hx9CxbtEQ5nEdnJUdR5ZV4/bKgILQ0Gn4AHuNZhxIrrex0f3551JMP9fQELlaUL2Cned5NuOGBm1HMMsCQ0ozJh3I0tBJndTifDTNxuwEs9ANuRPc+dSisPoc80XPTLGOa3d0UEMCI+5Yb8UzUi4+0PKHZh7oSz60= Message-ID: Date: Sat, 26 Apr 2008 12:47:40 +0400 From: Dmitry To: "Paul Walmsley" Subject: Re: [PATCH 0/5] Clocklib: generic clocks framework Cc: linux-kernel@vger.kernel.org, akpm@linux-foundation.org, "Haavard Skinnemoen" , "Russell King" , "Paul Mundt" , "pHilipp Zabel" , "Pavel Machek" , tony@atomide.com, "David Brownell" , hiroshi.DOYU@nokia.com In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <20080420082925.GA32739@doriath.ww600.siemens.net> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, 2008/4/25, Paul Walmsley : > Hello Dmitry, > > > On Mon, 21 Apr 2008, Dmitry wrote: > > > 2008/4/21, Paul Walmsley : > > > > > > > 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