Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

[PATCH] clk: Use a separate struct for holding init data.

 

 

First page Previous page 1 2 Next page Last page  View All Linux kernel RSS feed   Index | Next | Previous | View Threaded


arnd.bergmann at linaro

May 2, 2012, 6:32 AM

Post #26 of 32 (110 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

On Wednesday 02 May 2012, Mike Turquette wrote:
> I was happy to push my changes to Linus directly (as discussed in
> previous mails) but I'm starting to think that maybe having Arnd absorb
> the clk-next branch as part of arm-soc would be the fastest way to
> assist platforms that are porting over.
>
> Do the platform folks agree? Is this suggestion sane?

I guess that makes sense while there are still nontrivial
interdependencies between work going the clk-next branch and into
arm-soc.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


broonie at opensource

May 2, 2012, 8:28 AM

Post #27 of 32 (108 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

On Tue, May 01, 2012 at 06:56:50PM -0700, Mike Turquette wrote:
> On 20120501-19:19, Mark Brown wrote:

> > To be honest it doesn't look like your patch is a particular issue here
> > - there's wider process problems, for example we've managed to go
> > through most of the release cycle and so far the only changes showing up
> > in -next are:

> I think that "wider process problems" is probably a euphemism, and I'll
> take responsibility for that. This has been a learning process for me
> and I underestimated the percentage of my time that would be consumed by
> common clk maintenance. I'm trying to rectify that problem now.

It's not really a euphamism - it really does seem like we've got all the
technical stuff proceeding reasonably well but we're just struggling
with the mechanics of actually getting the code into -next and on its
way to mainline.

> I was happy to push my changes to Linus directly (as discussed in
> previous mails) but I'm starting to think that maybe having Arnd absorb
> the clk-next branch as part of arm-soc would be the fastest way to
> assist platforms that are porting over.

> Do the platform folks agree? Is this suggestion sane?

Seems to make sense to me; if there's some bits that are less clear you
could always keep them on a branch separate to the one that the
platforms use. It's probably also worth getting things into -next
directly, that way integration testing of bleeding edge stuff can happen
before it's been merged into other trees and it's hard to change.

What I've tried do with regmap when it's been possible (it's not this
time around because the stride changes touch everything) is to have
topic branches so that people can pull in only the specific bits they
need. I still end upn sending a pull request to Linus even if chunks of
it have also gone via other trees.
Attachments: signature.asc (0.82 KB)


mturquette at ti

May 2, 2012, 12:07 PM

Post #28 of 32 (110 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

On 20120501-21:42, Saravana Kannan wrote:
> On 05/01/2012 07:04 PM, Mike Turquette wrote:
> >1) I'm surprised that you abandoned the approach of exposing the
> >less-private members of struct clk via struct clk_hw. Your original
> >patch did just that, but did not account for static initialization.
> >This patch seems to have gone in the opposite direction and only
> >accounts for static initialization.
>
> I think there might be some misunderstanding on what can/can't be
> done with my patch. Or may be I'm not understanding your question.
>

I believe I am reading the code correctly. I'll try reiterating below.

> I used to expose the "shared" info through clk_hw. I just put them
> in a struct and make clk_hw point to it. This would allow for easily
> marking this shared info as __init data.

So platforms must choose between marking clk->hw->init as __initdata, OR
they can keep it around and reference it later from clock code that
includes clk-provder.h. Is this a fair statement?

> It would have a been a pain
> to do (or not even possible) if I had put the fields directly into
> clk_hw.
>

Not true. Combining Sascha's original static initializer idea with your
struct clk_hw patch would be easy. A separate struct for static init
would be marked as __initdata. When passed into a registration function
that data would be copied to fields in struct clk_hw. You've done this
exact same thing, with the exception of copying the data to struct clk
instead.

It is not a big deal, and I'm fine with the direction this patch has
taken, but I wanted to point it out. If you look at Russell's and
Sascha's replies in this thread you'll see that they regard
clk->hw->init as purely __initdata, going so far as to mark it NULL in
clk_register. This sort of change at the framework level would
eliminate the ability for your clock code to reference these members
directly.

> I'm not sure why you say my patch only accounts for static
> initialization. The entire clk specific struct (say, struct
> fixed_clk), the clk_init_data can be dynamically allocated and
> registered using clk_register.
>

This was a miscommunication on my part and can be disregarded. Of
course your patch allows for dynamic registration.

My point was that of the two previous approaches on this list (Sascha's
static initializers and your own struct clk_hw modifications), this
patch only represents the functionality of the former. I should have
been more clear.

> >2) I did make a modification to your patch where I kept the
> >DEFINE_CLK_* macros and continued to declare __clk_init in
> >clk-private.h. I do want to get rid of both of these in the future but
> >currently my platform relies on static initialization before the
> >allocator is available. Please let me know if this causes a problem for
> >you.
>
> I definitely had your requirements in mind too when I made the changes.
>
> You really shouldn't need __clk_init. That's why I added
> __clk_register.

I completely missed __clk_register. I'm not sure I see the point of it
however. struct clk is still exposed to folks that are using this new
function; compared to simply calling __clk_init it has added a few loads
& stores. Regardless these interfaces will hopefully die off completely
once OMAP's clock code uses an initcall.

> >Platform folks should rebase on top of this if needed. This should be
> >the last change to the driver/platform-facing API before 3.5.
>
> I really wish we discussed your changes before it was made, pulled
> in and announced since clk_init isn't really needed. But since you
> just added more APIs and didn't remove the ones I added, I guess
> it's not very bad.
>

I probably announced it too soon, but everything you added to the API is
still there, just with some extra stuff that should go away in the
future.

> Since people were already frustrated with the API change I made at
> this point, can we recommend people to not use __clk_init() when
> sending patches for your clk-next? And make it static after the next
> kernel release?

Yes, I completely agree. It would be good to get rid of __clk_register
and __clk_init down the road. Marking the interfaces as deprecated is
one solution; however I agree with your suggestion and just catching it
in review is probably the best route.

Thanks,
Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mturquette at ti

May 2, 2012, 12:19 PM

Post #29 of 32 (109 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

On 20120502-07:16, Andrew Lunn wrote:
> > I could use some suggestions on the best way to resolve the merge issues
> > we have currently. It appears that we have three bases that platforms
> > need to port over the common clk framework:
> >
> > Russell's clkdev
> > Arnd's arm-soc
> > My clk-next branch
>
> Hi Mike
>
> The Orion code only depends on clk-next. I've been more conservative
> with the changes, knowing that once they are merged i can add more
> patches to make use of devm_get_clk() etc.
>
> So for my, as well as going in via arm-soc, i could also imaging
> giving you a pull request and becoming part of clk-next. However, i
> don't care what route they take.
>

Hi Andrew,

The choice is yours.

Regards,
Mike

> Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


cavokz at gmail

May 3, 2012, 4:03 PM

Post #30 of 32 (110 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

On Wed, Apr 25, 2012 at 10:58:56PM -0700, Saravana Kannan wrote:
>
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index 90627e4..8ea11b4 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -167,6 +167,7 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
> {
> struct clk_divider *div;
> struct clk *clk;
> + struct clk_init_data init;
>
> /* allocate the divider */
> div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
> @@ -175,19 +176,22 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
> return ERR_PTR(-ENOMEM);
> }
>
> + init.name = name;
> + init.ops = &clk_divider_ops;
> + init.flags = flags;
> + init.parent_names = (parent_name ? &parent_name: NULL);
> + init.num_parents = (parent_name ? 1 : 0);
> +
> /* struct clk_divider assignments */
> div->reg = reg;
> div->shift = shift;
> div->width = width;
> div->flags = clk_divider_flags;
> div->lock = lock;
> + div->hw.init = &init;
>
> /* register the clock */
> - clk = clk_register(dev, name,
> - &clk_divider_ops, &div->hw,
> - (parent_name ? &parent_name: NULL),
> - (parent_name ? 1 : 0),
> - flags);
> + clk = clk_register(dev, &div->hw);
>
> if (IS_ERR(clk))
> kfree(div);

I would prefer to rip the parent _settings_ configuration out of
clk_register(). It's optional right? And passing a single parent is a
common case.

Three cases:

1) one parent:
__clk_register_parent(clk, parent_name);
clk_register(dev, name, &ops, flags);

2) many parents:
__clk_register_parents(clk, parent_names, num_parents);
clk_register(dev, name, &ops, flags);

3) no parents:
clk_register(dev, name, &ops, flags);

You may also want to move the whole parent initialization into
__clk_register_parents() and call it after clk_register(), it would
simplify some error paths.

This pattern could be used also with other common clocks registration
functions (fixed rate, divider, mux, etc) that may have complex
initializations and/or optional parameters that cannot go all on the
same function call.

cheers,
Domenico
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


skannan at codeaurora

May 3, 2012, 6:11 PM

Post #31 of 32 (108 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

On 05/03/2012 04:03 PM, Domenico Andreoli wrote:
> On Wed, Apr 25, 2012 at 10:58:56PM -0700, Saravana Kannan wrote:
>>
>> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
>> index 90627e4..8ea11b4 100644
>> --- a/drivers/clk/clk-divider.c
>> +++ b/drivers/clk/clk-divider.c
>> @@ -167,6 +167,7 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
>> {
>> struct clk_divider *div;
>> struct clk *clk;
>> + struct clk_init_data init;
>>
>> /* allocate the divider */
>> div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
>> @@ -175,19 +176,22 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
>> return ERR_PTR(-ENOMEM);
>> }
>>
>> + init.name = name;
>> + init.ops =&clk_divider_ops;
>> + init.flags = flags;
>> + init.parent_names = (parent_name ?&parent_name: NULL);
>> + init.num_parents = (parent_name ? 1 : 0);
>> +
>> /* struct clk_divider assignments */
>> div->reg = reg;
>> div->shift = shift;
>> div->width = width;
>> div->flags = clk_divider_flags;
>> div->lock = lock;
>> + div->hw.init =&init;
>>
>> /* register the clock */
>> - clk = clk_register(dev, name,
>> - &clk_divider_ops,&div->hw,
>> - (parent_name ?&parent_name: NULL),
>> - (parent_name ? 1 : 0),
>> - flags);
>> + clk = clk_register(dev,&div->hw);
>>
>> if (IS_ERR(clk))
>> kfree(div);
>
> I would prefer to rip the parent _settings_ configuration out of
> clk_register(). It's optional right? And passing a single parent is a
> common case.
>
> Three cases:
>
> 1) one parent:
> __clk_register_parent(clk, parent_name);
> clk_register(dev, name,&ops, flags);
>
> 2) many parents:
> __clk_register_parents(clk, parent_names, num_parents);
> clk_register(dev, name,&ops, flags);
>
> 3) no parents:
> clk_register(dev, name,&ops, flags);
>
> You may also want to move the whole parent initialization into
> __clk_register_parents() and call it after clk_register(), it would
> simplify some error paths.
>
> This pattern could be used also with other common clocks registration
> functions (fixed rate, divider, mux, etc) that may have complex
> initializations and/or optional parameters that cannot go all on the
> same function call.

Please no. If anything, make those other register functions go in the
direction of clk_register(). Have a long list of params to a function
and then having it fill up a structure just makes the code less
readable. Why would that be any better than having the whole structure
statically declared or the whole structure dynamically populated (by
device tree) and then calling clk_register()?

Take about 50 clocks with 3 parents each and try to register them in the
way you suggested and in a way how clk_register() in this patch will
need you to declare them statically. Compare the two and see which would
be more readable.

-Saravana

--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


cavokz at gmail

May 3, 2012, 11:50 PM

Post #32 of 32 (106 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

On Thu, May 03, 2012 at 06:11:56PM -0700, Saravana Kannan wrote:
> On 05/03/2012 04:03 PM, Domenico Andreoli wrote:
> >On Wed, Apr 25, 2012 at 10:58:56PM -0700, Saravana Kannan wrote:
> >>
> >>diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> >>index 90627e4..8ea11b4 100644
> >>--- a/drivers/clk/clk-divider.c
> >>+++ b/drivers/clk/clk-divider.c
> >>@@ -167,6 +167,7 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
> >> {
> >> struct clk_divider *div;
> >> struct clk *clk;
> >>+ struct clk_init_data init;
> >>
> >> /* allocate the divider */
> >> div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
> >>@@ -175,19 +176,22 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
> >> return ERR_PTR(-ENOMEM);
> >> }
> >>
> >>+ init.name = name;
> >>+ init.ops =&clk_divider_ops;
> >>+ init.flags = flags;
> >>+ init.parent_names = (parent_name ?&parent_name: NULL);
> >>+ init.num_parents = (parent_name ? 1 : 0);
> >>+
> >> /* struct clk_divider assignments */
> >> div->reg = reg;
> >> div->shift = shift;
> >> div->width = width;
> >> div->flags = clk_divider_flags;
> >> div->lock = lock;
> >>+ div->hw.init =&init;
> >>
> >> /* register the clock */
> >>- clk = clk_register(dev, name,
> >>- &clk_divider_ops,&div->hw,
> >>- (parent_name ?&parent_name: NULL),
> >>- (parent_name ? 1 : 0),
> >>- flags);
> >>+ clk = clk_register(dev,&div->hw);
> >>
> >> if (IS_ERR(clk))
> >> kfree(div);
> >
> >I would prefer to rip the parent _settings_ configuration out of
> >clk_register(). It's optional right? And passing a single parent is a
> >common case.
> >
> >Three cases:
> >
> > 1) one parent:
> > __clk_register_parent(clk, parent_name);
> > clk_register(dev, name,&ops, flags);
> >
> > 2) many parents:
> > __clk_register_parents(clk, parent_names, num_parents);
> > clk_register(dev, name,&ops, flags);
> >
> > 3) no parents:
> > clk_register(dev, name,&ops, flags);
> >
> >You may also want to move the whole parent initialization into
> >__clk_register_parents() and call it after clk_register(), it would
> >simplify some error paths.
> >
> >This pattern could be used also with other common clocks registration
> >functions (fixed rate, divider, mux, etc) that may have complex
> >initializations and/or optional parameters that cannot go all on the
> >same function call.
>
> Please no. If anything, make those other register functions go in
> the direction of clk_register(). Have a long list of params to a
> function and then having it fill up a structure just makes the code
> less readable. Why would that be any better than having the whole
> structure statically declared or the whole structure dynamically
> populated (by device tree) and then calling clk_register()?
>
> Take about 50 clocks with 3 parents each and try to register them in
> the way you suggested and in a way how clk_register() in this patch
> will need you to declare them statically. Compare the two and see
> which would be more readable.

I was not thinking at the static initialization at all (but I was
forgetting that clk does not yet exist before the invocation of
clk_register).

For a few hours I was convinced that moving the parent initialization
stuff in a separate function would have allowed also to ditch the (IMHO)
horrible whole name caching (whose purpose is... allowing to register
clock with a parent not yet known to the clock subsystem?)

Unfortunately the whole idea is quite invasive and the benefits
debatable, it's simply too late to speak.

Thanks anyway.

Domenico
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

First page Previous page 1 2 Next page Last page  View All Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.