On Tue, Mar 20, 2012 at 4:12 PM, Sascha Hauer [off-list ref] wrote:

On Tue, Mar 20, 2012 at 01:06:34PM -0700, Saravana Kannan wrote:

quoted

On 03/20/2012 11:10 AM, Sascha Hauer wrote:

quoted

On Tue, Mar 20, 2012 at 10:18:14PM +0800, Shawn Guo wrote:

quoted

On Tue, Mar 20, 2012 at 10:40:31AM +0100, Sascha Hauer wrote:
...

quoted

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.

quoted

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.

quoted

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.

quoted

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

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.  The patch
above doesn't actually prevent allocation from happening as it still
must call into clk_register and kmalloc struct clk, so besides
readability, I'm not sure what these patches buy us.

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.

Regards,
Mike