linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
To: Stephen Boyd <sboyd@kernel.org>
Cc: mazziesaccount@gmail.com, Jonathan Corbet <corbet@lwn.net>,
	Michael Turquette <mturquette@baylibre.com>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Russell King <linux@armlinux.org.uk>,
	Andy Gross <andy.gross@linaro.org>,
	David Brown <david.brown@linaro.org>,
	Andrey Smirnov <andrew.smirnov@gmail.com>,
	Guenter Roeck <linux@roeck-us.net>, Rob Herring <robh@kernel.org>,
	Sebastian Reichel <sre@kernel.org>,
	Lee Jones <lee.jones@linaro.org>,
	Huang Shijie <sjhuang@iluvatar.ai>,
	Daniel Kurtz <djkurtz@chromium.org>,
	Akshu Agrawal <akshu.agrawal@amd.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org
Subject: Re: [PATCH v4 1/8] clk: clkdev/of_clk - add managed lookup and provider registrations
Date: Fri, 30 Nov 2018 12:50:22 +0200	[thread overview]
Message-ID: <20181130105022.GA15388@localhost.localdomain> (raw)
In-Reply-To: <154356805035.88331.16867826308376667832@swboyd.mtv.corp.google.com>

Hello Stephen,

Thanks a bunch for taking the time and reviewing this!

On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-11-13 03:55:58)
> > With MFD devices the clk properties may be contained in MFD (parent) DT
> > node. Current devm_of_clk_add_hw_provider assumes the clk is bound to MFD
> > subdevice not to MFD device (parent). Add
> > devm_of_clk_add_hw_provider_parent to tackle this issue.
> > 
> > Also clkdev registration lacks of managed registration functions and it
> > seems few drivers do not drop clkdev lookups at exit. Add
> > devm_clk_hw_register_clkdev and devm_clk_release_clkdev to ease lookup
> > releasing at exit.
> 
> Please split this into clkdev and non-clkdev devm functionality.
Allright. I'll split this to two patches.

> > --- a/Documentation/driver-model/devres.txt
> > +++ b/Documentation/driver-model/devres.txt
> > @@ -238,6 +238,9 @@ CLOCK
> >    devm_clk_put()
> >    devm_clk_hw_register()
> >    devm_of_clk_add_hw_provider()
> > +  devm_of_clk_add_parent_hw_provider()
> > +  devm_clk_hw_register_clkdev()
> > +  devm_clk_release_clkdev()
> 
> The 'release' or non-common functions shouldn't be documented here.
So I will drop the line mentioning devm_clk_release_clkdev()

> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index af011974d4ec..9bb921eb90f6 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3893,12 +3893,12 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
> >         of_clk_del_provider(*(struct device_node **)res);
> >  }
> >  
> > -int devm_of_clk_add_hw_provider(struct device *dev,
> > +static int __devm_of_clk_add_hw_provider(struct device *dev,
> >                         struct clk_hw *(*get)(struct of_phandle_args *clkspec,
> >                                               void *data),
> > -                       void *data)
> > +                       struct device_node *of_node, void *data)
> >  {
> > -       struct device_node **ptr, *np;
> > +       struct device_node **ptr;
> >         int ret;
> >  
> >         ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr),
> > @@ -3906,10 +3906,9 @@ int devm_of_clk_add_hw_provider(struct device *dev,
> >         if (!ptr)
> >                 return -ENOMEM;
> >  
> > -       np = dev->of_node;
> > -       ret = of_clk_add_hw_provider(np, get, data);
> > +       *ptr = of_node;
> > +       ret = of_clk_add_hw_provider(of_node, get, data);
> >         if (!ret) {
> > -               *ptr = np;
> 
> Why is this moved outside of the if condition?
I completely removed the local variable np and just unconditionally set
the allocated devres to point at the node (if allocation succeeded). We
could of course only do this if the provider registration succeeded and
save one assignment - but I guess I intended to remove the curly braces
and thus decided to go for one liner after if. But apparently I didn't
remove the braces O_o. Well, I can put the assignment inside the
condition if you prefer that.

> In fact, why isn't just
> the first line in this hunk deleted and passed to this function as
> struct device_node *np?

I am sorry but I don't quite follow your suggestion here. Do you mean we
could just pass the struct device_node *np in devres_add()? I thought
the pointer passed to devress_add() should be allocated using
devres_alloc. Can you please elaborate what you mean?

> 
> >                 devres_add(dev, ptr);
> >         } else {
> >                 devres_free(ptr);
> > @@ -3917,8 +3916,25 @@ int devm_of_clk_add_hw_provider(struct device *dev,
> >  
> >         return ret;
> >  }
> 
> Nitpick: Add a newline here.

Will do.

> 
> > +int devm_of_clk_add_hw_provider(struct device *dev,
> > +                       struct clk_hw *(*get)(struct of_phandle_args *clkspec,
> > +                                             void *data),
> > +                       void *data)
> > +{
> > +       return __devm_of_clk_add_hw_provider(dev, get, dev->of_node, data);
> > +}
> >  EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider);
> >  
> > +int devm_of_clk_add_parent_hw_provider(struct device *dev,
> > +                       struct clk_hw *(*get)(struct of_phandle_args *clkspec,
> > +                                             void *data),
> > +                       void *data)
> > +{
> > +       return __devm_of_clk_add_hw_provider(dev, get, dev->parent->of_node,
> 
> I'm wondering if we can somehow auto-detect this in
> devm_of_clk_add_hw_provider() by looking for #clock-cells in the node.
> If it isn't there, then we go to the parent node and look for a
> #clock-cells property there in the DT node for that device. Does that
> make sense? Then there isn't any new API and we can attach the lifetime
> of the devm registration to the presence of the property indicating this
> is a clk controller or not.

Huh. I don't know why but building this kind of logic in core is a bit
scary to me. I guess I can try implementing something like this - but I
am not really a fan of this. (Accidentally) omit the #clock-cells from
node and we go to parent node - I am a novice on this area but this
sounds like a potential hazard to me. I believe the driver should know
if it's properties should be in own or parent node - and if they are
not, then there should be no guessing but error. The lifetime is topic
where I would like to get information from you who know the kernel
better than I do =) But I guess the parent node is there at least as
long as the child device is alive. So for me the life time of
get-callback is more crucial - but as I said, I don't understand the
kernel in details so you probably know it better than me. But please let
me know your final take on this and I will follow the guidance =)

> 
> > +                                            data);
> > +}
> > +EXPORT_SYMBOL_GPL(devm_of_clk_add_parent_hw_provider);
> 
> Can we get some kernel doc on these functions?
Sure. I will add the doc. Reason why I didn't do it is that the current
devm_of_clk_add_hw_provider() did not have doc. But I'll add that in
next patch.

> > +       rval = devres_release(dev, devm_clkdev_release,
> > +                             &devm_clk_match_clkdev, cl);
> 
> Nitpick: Drop & on functions taken as pointers.
Ok. Will do.

-- 
Matti Vaittinen
ROHM Semiconductors

~~~ "I don't think so," said Rene Descartes.  Just then, he vanished ~~~

  reply	other threads:[~2018-11-30 10:50 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-13 11:54 [PATCH v4 0/8] clk: clkdev: managed clk " Matti Vaittinen
2018-11-13 11:55 ` [PATCH v4 1/8] clk: clkdev/of_clk - add managed " Matti Vaittinen
2018-11-30  8:54   ` Stephen Boyd
2018-11-30 10:50     ` Matti Vaittinen [this message]
2018-11-30 11:06       ` Russell King - ARM Linux
2018-11-30 12:59         ` Matti Vaittinen
2018-12-03 12:16       ` Matti Vaittinen
2018-12-03 23:35       ` Stephen Boyd
2018-12-04  7:13         ` Matti Vaittinen
2018-12-04 18:21           ` Stephen Boyd
2018-12-04 18:59             ` Vaittinen, Matti
2018-11-13 11:57 ` [PATCH v4 2/8] clk: clk-max77686: Clean clkdev lookup leak and use devm Matti Vaittinen
2018-11-13 11:59 ` [PATCH v4 3/8] clk: clk-st: avoid clkdev lookup leak at remove Matti Vaittinen
2018-11-13 12:00 ` [PATCH v4 4/8] clk: clk-hi655x: Free of_provider " Matti Vaittinen
2018-11-13 12:00 ` [PATCH v4 5/8] clk: rk808: use managed version of of_provider registration Matti Vaittinen
2018-11-13 12:01 ` [PATCH v4 6/8] clk: clk-twl6040: Free of_provider at remove Matti Vaittinen
2018-11-13 12:02 ` [PATCH v4 7/8] clk: apcs-msm8916: simplify probe cleanup by using devm Matti Vaittinen
2018-11-13 12:03 ` [PATCH v4 8/8] clk: bd718x7: Initial support for ROHM bd71837/bd71847 PMIC clock Matti Vaittinen

Reply instructions:

You may reply publicly 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=20181130105022.GA15388@localhost.localdomain \
    --to=matti.vaittinen@fi.rohmeurope.com \
    --cc=akshu.agrawal@amd.com \
    --cc=andrew.smirnov@gmail.com \
    --cc=andy.gross@linaro.org \
    --cc=b.zolnierkie@samsung.com \
    --cc=corbet@lwn.net \
    --cc=cw00.choi@samsung.com \
    --cc=david.brown@linaro.org \
    --cc=djkurtz@chromium.org \
    --cc=krzk@kernel.org \
    --cc=lee.jones@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-soc@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=linux@roeck-us.net \
    --cc=mazziesaccount@gmail.com \
    --cc=mturquette@baylibre.com \
    --cc=rafael.j.wysocki@intel.com \
    --cc=robh@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=sjhuang@iluvatar.ai \
    --cc=sre@kernel.org \
    --subject='Re: [PATCH v4 1/8] clk: clkdev/of_clk - add managed lookup and provider registrations' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).