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

[Stephen Boyd <sboyd@kernel.org>]

Quoting Jerome Brunet (2019-08-30 07:06:29)
> 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.

Ok. I see that this is how the meson clk driver does clk_regmap structs.
I agree that if the same data exists for many clks then having a pointer
to that data instead of duplicating it is good for saving space, but
that sort of detail can still be put into a void *data member of the
wrapper structure(s) and then passed to some function that knows the
type of the data that's the same. It may require writing a few more
functions to unwrap the data member out of the wrapper struct though so
having a data member of the clk_hw or clk_core pointer may be useful to
make "polymorphic" code that just knows that the data member is some
particular type and can operate on that generically without having to
unwrap the container for each different type.

TL;DR is I'm half convinced that this is a good approach to solving that
sort of problem.