Allow drivers to declare their clk_init_data const, the framework really
shouldn't be modifying the data.
Signed-off-by: Mark Brown <[email protected]>
---
include/linux/clk-provider.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index c1c23b9..fc43ea6 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -143,7 +143,7 @@ struct clk_init_data {
*/
struct clk_hw {
struct clk *clk;
- struct clk_init_data *init;
+ const struct clk_init_data *init;
};
/*
--
1.7.10
On Mon, May 14, 2012 at 7:12 AM, Mark Brown <[email protected]> wrote:
> Allow drivers to declare their clk_init_data const, the framework really
> shouldn't be modifying the data.
>
> Signed-off-by: Mark Brown <[email protected]>
+interested parties
Mark, I like this change but it's reminded me of a few things I meant to
bring up on the list in the past. Certainly some folks will mark their
struct clk_hw_init data as __initconst. Currently none of the
documentation mentions that fact and I'm a bit worried about clk code
which assumes that hw->init will always be around and freely accesses
it.
I think that, as a rule, hw->init cannot be assumed to be valid after
clk_register returns. Would anyone else like to weigh in on it? If so
then I'll cook up a follow-up patch to reflect this in the kerneldoc.
Thanks,
Mike
> ---
> ?include/linux/clk-provider.h | ? ?2 +-
> ?1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index c1c23b9..fc43ea6 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -143,7 +143,7 @@ struct clk_init_data {
> ?*/
> ?struct clk_hw {
> ? ? ? ?struct clk *clk;
> - ? ? ? struct clk_init_data *init;
> + ? ? ? const struct clk_init_data *init;
> ?};
>
> ?/*
> --
> 1.7.10
>
On 05/14/2012 02:53 PM, Turquette, Mike wrote:
> On Mon, May 14, 2012 at 7:12 AM, Mark Brown<[email protected]> wrote:
>> Allow drivers to declare their clk_init_data const, the framework really
>> shouldn't be modifying the data.
>>
>> Signed-off-by: Mark Brown<[email protected]>
>
> +interested parties
>
> Mark, I like this change but it's reminded me of a few things I meant to
> bring up on the list in the past. Certainly some folks will mark their
> struct clk_hw_init data as __initconst. Currently none of the
> documentation mentions that fact and I'm a bit worried about clk code
> which assumes that hw->init will always be around and freely accesses
> it.
>
> I think that, as a rule, hw->init cannot be assumed to be valid after
> clk_register returns. Would anyone else like to weigh in on it? If so
> then I'll cook up a follow-up patch to reflect this in the kerneldoc.
If you mean hw->init pointer can't be assumed to be point to anything
valid after clk_register or __clk_register, then yes, I agree with that.
But the actual data that hw->init pointed to might still be around and
referenced by clk if __clk_register is used.
Btw, I didn't follow up on the other thread we were having, but can you
remind me again what was the reason that you thought that only
__clk_init() would work for your static init code and __clk_register()
won't work? Or did I just misunderstand your comment that was trying to
say that you didn't want to switch to __clk_register close to kernel
release?
There are some older threads where I think we are a bit out of sync on
the init data (I thought I did what you said you preferred, but you
asked me why I did that). I will try to restart them.
Thanks,
Saravana
>
> Thanks,
> Mike
>
>> ---
>> include/linux/clk-provider.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>> index c1c23b9..fc43ea6 100644
>> --- a/include/linux/clk-provider.h
>> +++ b/include/linux/clk-provider.h
>> @@ -143,7 +143,7 @@ struct clk_init_data {
>> */
>> struct clk_hw {
>> struct clk *clk;
>> - struct clk_init_data *init;
>> + const struct clk_init_data *init;
>> };
>>
>> /*
>> --
>> 1.7.10
>>
>
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On 05/14/2012 02:53 PM, Turquette, Mike wrote:
> On Mon, May 14, 2012 at 7:12 AM, Mark Brown<[email protected]> wrote:
>> Allow drivers to declare their clk_init_data const, the framework really
>> shouldn't be modifying the data.
>>
>> Signed-off-by: Mark Brown<[email protected]>
>
> +interested parties
>
> Mark, I like this change but it's reminded me of a few things I meant to
> bring up on the list in the past. Certainly some folks will mark their
> struct clk_hw_init data as __initconst. Currently none of the
> documentation mentions that fact and I'm a bit worried about clk code
> which assumes that hw->init will always be around and freely accesses
> it.
>
> I think that, as a rule, hw->init cannot be assumed to be valid after
> clk_register returns. Would anyone else like to weigh in on it? If so
> then I'll cook up a follow-up patch to reflect this in the kerneldoc.
>
> Thanks,
> Mike
>
>> ---
>> include/linux/clk-provider.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>> index c1c23b9..fc43ea6 100644
>> --- a/include/linux/clk-provider.h
>> +++ b/include/linux/clk-provider.h
>> @@ -143,7 +143,7 @@ struct clk_init_data {
>> */
>> struct clk_hw {
>> struct clk *clk;
>> - struct clk_init_data *init;
>> + const struct clk_init_data *init;
Oh, wait. This won't work for the case where the clock registration is
completely dynamic. Say, created from device tree or thru some PCI/USB
device probe, etc. That's why I didn't add it before.
-Saravana
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Hi Saravana,
On Tuesday 15 May 2012 06:38 AM, Saravana Kannan wrote:
> Btw, I didn't follow up on the other thread we were having, but can you
> remind me again what was the reason that you thought that only
> __clk_init() would work for your static init code and __clk_register()
> won't work?
One of the main reason has been the platform implementation we have to
handle some complex mux/divider combo clocks in OMAP2/3 which rely on
'struct clk' pointers. Maybe we can do away with the existing
implementation and redo it so we don't have any such limitation, but the
quantum of change moving to common clk has been so much that we are
trying to minimize on the platform code changes for now. So while
we move to common clk it would still be useful to have __clk_init()
around for a while till we figure out how to get rid of it for OMAP.
regards,
Rajendra
On Mon, May 14, 2012 at 10:13 PM, Rajendra Nayak <[email protected]> wrote:
> Hi Saravana,
>
>
> On Tuesday 15 May 2012 06:38 AM, Saravana Kannan wrote:
>>
>> Btw, I didn't follow up on the other thread we were having, but can you
>> remind me again what was the reason that you thought that only
>> __clk_init() would work for your static init code and __clk_register()
>> won't work?
>
>
> One of the main reason has been the platform implementation we have to
> handle some complex mux/divider combo clocks in OMAP2/3 which rely on
> 'struct clk' pointers. Maybe we can do away with the existing
> implementation and redo it so we don't have any such limitation, but the
> quantum of change moving to common clk has been so much that we are
> trying to minimize on the platform code changes for now. So while
> we move to common clk it would still be useful to have __clk_init()
> around for a while till we figure out how to get rid of it for OMAP.
>
Just to add to what Rajendra has stated for OMAP: after OMAP's
conversion is finally made initcall-able then I'll get rid of lots of
the gross macros and before-the-memory-allocator-is-alive stuff. I'll
make sure that no other platforms are using those bits of course, but
I plan on removing some of this stuff from the core once my platform
is up to speed.
Regards,
Mike
> regards,
> Rajendra
On Mon, May 14, 2012 at 06:19:16PM -0700, Saravana Kannan wrote:
> On 05/14/2012 02:53 PM, Turquette, Mike wrote:
> >On Mon, May 14, 2012 at 7:12 AM, Mark Brown<[email protected]> wrote:
> >>Allow drivers to declare their clk_init_data const, the framework really
> >>shouldn't be modifying the data.
> >>
> >>Signed-off-by: Mark Brown<[email protected]>
> >
> >+interested parties
> >
> >>
> >>diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> >>index c1c23b9..fc43ea6 100644
> >>--- a/include/linux/clk-provider.h
> >>+++ b/include/linux/clk-provider.h
> >>@@ -143,7 +143,7 @@ struct clk_init_data {
> >> */
> >> struct clk_hw {
> >> struct clk *clk;
> >>- struct clk_init_data *init;
> >>+ const struct clk_init_data *init;
>
> Oh, wait. This won't work for the case where the clock registration
> is completely dynamic. Say, created from device tree or thru some
> PCI/USB device probe, etc. That's why I didn't add it before.
Why not? In this case clk_init_data is also only used in clk_register.
Given that Mark has posted the patch I assume it actually works.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 05/15/2012 12:00 AM, Sascha Hauer wrote:
> On Mon, May 14, 2012 at 06:19:16PM -0700, Saravana Kannan wrote:
>> On 05/14/2012 02:53 PM, Turquette, Mike wrote:
>>> On Mon, May 14, 2012 at 7:12 AM, Mark Brown<[email protected]> wrote:
>>>> Allow drivers to declare their clk_init_data const, the framework really
>>>> shouldn't be modifying the data.
>>>>
>>>> Signed-off-by: Mark Brown<[email protected]>
>>>
>>> +interested parties
>>>
>>>>
>>>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>>>> index c1c23b9..fc43ea6 100644
>>>> --- a/include/linux/clk-provider.h
>>>> +++ b/include/linux/clk-provider.h
>>>> @@ -143,7 +143,7 @@ struct clk_init_data {
>>>> */
>>>> struct clk_hw {
>>>> struct clk *clk;
>>>> - struct clk_init_data *init;
>>>> + const struct clk_init_data *init;
>>
>> Oh, wait. This won't work for the case where the clock registration
>> is completely dynamic. Say, created from device tree or thru some
>> PCI/USB device probe, etc. That's why I didn't add it before.
>
> Why not? In this case clk_init_data is also only used in clk_register.
> Given that Mark has posted the patch I assume it actually works.
>
> Sascha
>
It's used in __clk_register() though. Which I added as a "as close as
clk_register() but allows static init" function.
-Saravana
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
On 05/15/2012 09:42 AM, Saravana Kannan wrote:
> On 05/15/2012 12:00 AM, Sascha Hauer wrote:
>> On Mon, May 14, 2012 at 06:19:16PM -0700, Saravana Kannan wrote:
>>> On 05/14/2012 02:53 PM, Turquette, Mike wrote:
>>>> On Mon, May 14, 2012 at 7:12 AM, Mark
>>>> Brown<[email protected]> wrote:
>>>>> Allow drivers to declare their clk_init_data const, the framework
>>>>> really
>>>>> shouldn't be modifying the data.
>>>>>
>>>>> Signed-off-by: Mark Brown<[email protected]>
>>>>
>>>> +interested parties
>>>>
>>>>>
>>>>> diff --git a/include/linux/clk-provider.h
>>>>> b/include/linux/clk-provider.h
>>>>> index c1c23b9..fc43ea6 100644
>>>>> --- a/include/linux/clk-provider.h
>>>>> +++ b/include/linux/clk-provider.h
>>>>> @@ -143,7 +143,7 @@ struct clk_init_data {
>>>>> */
>>>>> struct clk_hw {
>>>>> struct clk *clk;
>>>>> - struct clk_init_data *init;
>>>>> + const struct clk_init_data *init;
>>>
>>> Oh, wait. This won't work for the case where the clock registration
>>> is completely dynamic. Say, created from device tree or thru some
>>> PCI/USB device probe, etc. That's why I didn't add it before.
>>
>> Why not? In this case clk_init_data is also only used in clk_register.
>> Given that Mark has posted the patch I assume it actually works.
>>
>> Sascha
>>
>
> It's used in __clk_register() though. Which I added as a "as close as
> clk_register() but allows static init" function.
>
Sorry about my rushed responses earlier. I don't think the const will
work even when purely using clk_register().
Say I use device tree (you can extend this to HW probing/detection) to
know that there is a fixed factor clock. Now, if I want to dynamically
create and register it, I will have to allocate struct clk_fixed_factor
and struct clk_init_data. Then populate the fields of those structs and
finally point clk_fixed_factor.hw.init to the allocated clk_init_data.
Then call clk_register on clk_fixed_factor.hw. I'm not sure the
assigning of the init field above will be possible if we mark it as const.
Really though, not marking it as const shouldn't really have a huge
impact. What matters is that the common clock framework shouldn't try to
use it after clk_register() and we can do that by following what Russell
suggested (set hw.init to NULL at the end of clk_register).
Does that make sense?
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.
On 20120514-15:12, Mark Brown wrote:
> Allow drivers to declare their clk_init_data const, the framework really
> shouldn't be modifying the data.
>
> Signed-off-by: Mark Brown <[email protected]>
Thanks for the patch Mark. I've pulled into clk-next for testing.
Regards,
Mike