Re: [PATCH RFC 4/5] clk: add placeholder for clock internal data

[Jerome Brunet <jbrunet@baylibre.com>]

On Thu 29 Aug 2019 at 10:17, Stephen Boyd <sboyd@kernel.org> wrote:

> Quoting Jerome Brunet (2019-08-29 00:20:38)
>> On Wed 28 Aug 2019 at 15:15, Stephen Boyd <sboyd@kernel.org> wrote:
>> 
>> > Quoting Jerome Brunet (2019-08-28 03:20:11)
>> >> Add placeholder in clock core to save per clock data.
>> >> Such placeholder could use for clock doing memory allocation in .init().
>> >> It may also be useful for the save/restore_context() callbacks.
>> >> 
>> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> >> ---
>> >>  drivers/clk/clk.c            | 13 +++++++++++++
>> >>  include/linux/clk-provider.h |  2 ++
>> >>  2 files changed, 15 insertions(+)
>> >> 
>> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> >> index c703aa35ca37..aa77a2a98ea4 100644
>> >> --- a/drivers/clk/clk.c
>> >> +++ b/drivers/clk/clk.c
>> >> @@ -83,6 +83,7 @@ struct clk_core {
>> >>         struct hlist_node       debug_node;
>> >>  #endif
>> >>         struct kref             ref;
>> >> +       void                    *priv;
>> >
>> > Why? We have container structures around clk_hw that can be used to
>> > store data and clk_ops that should know to deref said clk_hw pointer in
>> > some way to access that data.
>> 
>> This simple addition give a placeholder to each clock instance that is
>> hosted under the clock core so we know it can only be accesed from this
>> particular instance. Seems like a better fit for runtime data, such as
>> saved context.
>> 
>> It gives a way to avoid mixing the clock description and its runtime
>> data. In the end, It would be nice if the clock description part could
>> be made const.
>> 
>> This is a (fairly usual) way to do it 
>
> Maybe you can provide an example or usage? Is that the last patch in
> this series?

Yes, I thought that was in my cover letter

> I still don't quite understand why we need this.
>
> I imagine that if you wanted to have a const description part you could
> have a container structure around the clk_hw struct
>
> 	struct my_clk_foo {
> 		const struct clk_description *desc;
> 		<runtime modifiable members>;
> 		struct clk_hw hw;
> 	};
>
> Did I miss something?

Not really. As with all C code, there are many way to do things.
Your way could work, mine too.

As explained in the previous mail, what I propose guaranteed to specific
to each clock instance. It's a fairly usual and simple way to give a
placeholder to the instance of something.

Things gets complicated with your way if the clock declaration uses
static data and there there multiple instance of it.

In the end that was merely a suggestion, so let's spend to much time on
this. If don't want it, that's fine. I'll just drop it.