[PATCH 2/2] clk: Move init fields from clk to clk_hw

[skannan@codeaurora.org (Saravana Kannan)]

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.