Messages in this thread | | | Date | Mon, 26 Mar 2012 21:35:35 -0700 | From | Saravana Kannan <> | Subject | Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw |
| |
Mike,
(*nudge*) (*nudge*)
-Saravana
On 03/20/2012 08:01 PM, Saravana Kannan wrote: > On 03/20/2012 06:47 PM, Turquette, Mike wrote: >> On Tue, Mar 20, 2012 at 4:12 PM, Sascha Hauer<s.hauer@pengutronix.de> >> wrote: >>> On Tue, Mar 20, 2012 at 01:06:34PM -0700, Saravana Kannan wrote: >>>> On 03/20/2012 11:10 AM, Sascha Hauer wrote: >>>>> On Tue, Mar 20, 2012 at 10:18:14PM +0800, Shawn Guo wrote: >>>>>> On Tue, Mar 20, 2012 at 10:40:31AM +0100, Sascha Hauer wrote: >>>>>> ... >>>>>>> I am using these functions and don't need a static array, I just >>>>>>> call >>>>>>> the functions with the desired parameters. >>>>>>> >>>>>> With this patch getting in, you do not have to use them then. I feel >>>>>> a static array will be much more readable than a long list of >>>>>> function >>>>>> calls with a long list of hardcoded arguments to each. >>>>> >>>>> I'm also not a fan of long argument lists; a divider like defined >>>>> in my >>>>> tree takes 5 arguments, a gate 4 and a mux 6. While 6 is already at >>>>> the >>>>> border I think it's still acceptable. >>>>> >>>>> What I like in terms of readability is one line per clock which makes >>>>> for quite short clock files. >>>> >>>> It certainly makes for short clock files, but it's definitely less >>>> readable that the expanded struct. For the original author the "one >>>> line per clock" looks readable since they wrote it. But for someone >>>> looking at the code to modify it, the expanded one would be much >>>> easier to read. Also, you can always declare your own macro if you >>>> really want to follow the one line approach. >>>> >>>>> So when we use structs to initialize the clocks we'll probably have >>>>> something like this: >>>>> >>>>> static struct clk_divider somediv = { >>>>> .reg = CCM_BASE + 0x14, >>>>> .width = 3, >>>>> .shift = 17, >>>>> .lock =&ccm_lock, >>>>> .hw.parent = "someotherdiv", >>>>> .hw.flags = CLK_SET_RATE_PARENT, >>>>> }; >>>> >>>> Taken from your patches: >>>> >>>> imx_clk_mux("spll_sel", CCM_CSCR, 17, 1, spll_sel_clks, >>>> ARRAY_SIZE(spll_sel_clks)); >>>> >>>> Compare the struct to the one line call. Now tell me, what does "1" >>>> represent? No clue. But in the struct, I can immediately tell what >>>> each one of the parameters are. >>>> >>>> Anyway, my patch certainly isn't forcing you to use multiple lines. >>>> So, hopefully this won't be a point of contention. >>>> >>>>> This will make a 4000 line file out of a 500 line file. Now when for >>>>> some reason struct clk_divider changes we end with big patches. If the >>>>> clk core gets a new fancy CLK_ flag which we want to have then again >>>>> we end up with big patches. Then there's also the possibility that >>>>> someone finds out that .lock and .hw.flags are common to all dividers >>>>> and comes up with a #define DEFINE_CLK_DIVIDER again to share common >>>>> fields. >>>> >>>> This patch won't prevent you from doing any of that. You can still >>>> create macros for that (there are already one for that). Also, what >>>> you are pointing out is a bigger problem for the current >>>> clk_register() function since you might have to change the no. of >>>> params of all the callers even if a new field is optional. >>>> >>>>> All this can be solved when we introduce a small wrapper function and >>>>> use it in the clock files: >>>>> >>>>> static inline struct clk *imx_clk_divider(const char *name, const >>>>> char *parent, >>>>> void __iomem *reg, u8 shift, u8 width) >>>>> { >>>>> return clk_register_divider(NULL, name, parent, CLK_SET_RATE_PARENT, >>>>> reg, shift, width, 0,&imx_ccm_lock); >>>>> } >>>>> >>>>> It decouples us from the structs used by the clock framework, we can >>>>> add our preferred flags and still can share common fields like the >>>>> lock. >>>>> >>>>> While this was not the intention when I first converted from struct >>>>> initializers to function initializers I am confident that it will make >>>>> a good job. >>>> >>>> Now I'm confused -- it's not clear if you are leaning towards my >>>> patch or away from it? >>> >>> There was a tendency to get rid of static initializers and I like the >>> idea of not exposing any of the internally used members outside the >>> clock framework. >> >> I'm with Sascha on this. I feel that dividing the interface strictly >> into two halves is the best way. > > I addressed this concern in my earlier comments. We can make a copy or > we can agree the fields I moved to clk_hw aren't really useful wrt > writing hacky code and call it a day. Can you please clarify why neither > of these options are acceptable? > >> I have an uneasy feeling about >> exposing this stuff into struct clk_hw (or clk_initializer or >> whatever). This stretches the data out across three structures and >> just doesn't feel right to me. > > Wrt this discussion, there are three distinct classes of data: > 1) Those specific to the platform driver that the common code shouldn't > care about. > 2) Those specific to the common code that the platform driver shouldn't > care about. > 3) Stuff that's shared/passed between common code and the platform driver. > > When we have three classes of data, I don't what's wrong with having > three struct types to contain them. If anything, it's better than the > current approach of exposing the common clock code specific data (struct > clk) to code outside of common clock code just because we want to allow > static initialization. The end goal should be to move struct clk inside > clk.c. > > I think this patch just takes us one step close to that since IMX and > MSM won't have to include clk-private.h in any of our platform specific > files while also allowing OMAP to include it for the near term. > >>> Now people try their best to make themselves comfortable with the >>> static initializers and you even discussed the possibility of removing >>> the clk_register_* functions (which make it possible for users not >>> to use any of the internal struct members). That's what I don't like >>> about your patches. But if we go for static initializers anyway then >>> your >>> patches probably change things for the better. >> >> Static initialization is something I have fought for; in fact the >> original patches provided no way to do it, so clearly what we have >> today is some progress for the folks desiring static init. > > I too desire static init. Sorry if I was unclear and gave people the > misconception that I wanted to remove static init. > >> The patch >> above doesn't actually prevent allocation from happening as it still >> must call into clk_register and kmalloc struct clk, > > Correct. > >> so besides >> readability, I'm not sure what these patches buy us. > > I think readability is very important and if this buys us nothing but > readability, we should still take this patch. But there are other > benefits too -- I mentioned them in the commit text. > >> Assuming that C99 designated initializers (for the sole purpose of >> readability) is the main draw of the above patch, please let me know >> what you think about modifying the existing static init macros so that >> your clock data looks like this: >> >> DEFINE_CLK_DIVIDER(dpll_iva_m5x2_ck,&dpll_iva_x2_ck, "dpll_iva_x2_ck", >> .flags = 0x0, >> .reg = OMAP4430_CM_DIV_M5_DPLL_IVA, >> .shift = OMAP4430_HSDIVIDER_CLKOUT2_DIV_SHIFT, >> .width = OMAP4430_HSDIVIDER_CLKOUT2_DIV_WIDTH, >> .flags = CLK_DIVIDER_ONE_BASED, >> .lock = NULL >> ); >> >> Note that the first argument is the name of this clock (and will be >> properly stringified for .name = "whatever") and that the second and >> third arguments are both the parent clock, used for initializing the >> parent pointer and .parent_names, respectively. If that aspect of the >> macro is too ugly then those can even be broken out into a separate >> macro since they are independent data structure (struct clk **parents, >> and char **parent_names). Or you could just open code those data >> structures and only use a macro for struct clk and struct clk_foo. >> >> This gives you the readability of C99 designated initializers while >> keeping struct clk's members totally hidden from the rest of the >> world. > > But it still leaves the struct clk exposed to people who do static init > of the clock tree. I think agreeing that the name, parent names, flags > and ops are not used to hack with or just making a copy of all of them > (and mark the originals as __init if that's doable). is a better > solution than trying to go with macros and leave struct clk exposed to > everyone who want to do static init of the clock tree. > > At a later point when we are ready to move struct clk inside clk.c, with > this patch applied right now, IMX and MSM won't have to churn their code. > > Thanks, > Saravana >
-- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
| |