Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
From: Stephen Boyd <sboyd@kernel.org>
To: Jerome Brunet <jbrunet@baylibre.com>,
	Michael Turquette <mturquette@baylibre.com>
Cc: Kevin Hilman <khilman@baylibre.com>, linux-clk@vger.kernel.org
Subject: Re: [PATCH RFC 4/5] clk: add placeholder for clock internal data
Date: Wed, 04 Sep 2019 15:52:03 -0700
Message-ID: <20190904225204.7289420674@mail.kernel.org> (raw)
In-Reply-To: <1jsgpivjbu.fsf@starbuckisacylon.baylibre.com>

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.


  reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-28 10:20 [PATCH RFC 0/5] clk: let clock claim resources Jerome Brunet
2019-08-28 10:20 ` [PATCH RFC 1/5] clk: actually call the clock init before any other callback of the clock Jerome Brunet
2019-08-28 10:20 ` [PATCH RFC 2/5] clk: let init callback return an error code Jerome Brunet
2019-08-28 10:20 ` [PATCH RFC 3/5] clk: add terminate callback to clk_ops Jerome Brunet
2019-08-28 10:20 ` [PATCH RFC 4/5] clk: add placeholder for clock internal data Jerome Brunet
2019-08-28 22:15   ` Stephen Boyd
2019-08-29  7:20     ` Jerome Brunet
2019-08-29 17:17       ` Stephen Boyd
2019-08-30 14:06         ` Jerome Brunet
2019-09-04 22:52           ` Stephen Boyd [this message]
2019-08-28 10:20 ` [PATCH RFC 5/5] clk: meson: sclk-div: use runtime data Jerome Brunet

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190904225204.7289420674@mail.kernel.org \
    --to=sboyd@kernel.org \
    --cc=jbrunet@baylibre.com \
    --cc=khilman@baylibre.com \
    --cc=linux-clk@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org linux-clk@archiver.kernel.org
	public-inbox-index linux-clk

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/ public-inbox