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>,
	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: Tue, 4 Dec 2018 09:13:15 +0200	[thread overview]
Message-ID: <20181204071315.GA31204@localhost.localdomain> (raw)
In-Reply-To: <154388011043.88331.6461709079501929390@swboyd.mtv.corp.google.com>

Hello Again Stephen,

I did already send v5 prior to your reply but I will create v6 today
based on this discussion.

On Mon, Dec 03, 2018 at 03:35:10PM -0800, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-11-30 02:50:22)
> > 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)
> > > > -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?
> 
> I'm just trying to reduce the diff in the patch.

Oh, right. I will see how renaming the argument to np would impact to
patch size. iActually, I never consider the patch size at all - I have
only been concentrating on how the resulting file looks like. It didn't
ever cross my mind that patch size matters. But I guess the size of
chanes is really meaningfull when the amount of changes is large.

> > > >                 devres_add(dev, ptr);
> > > >         } else {
> > > >                 devres_free(ptr);
> [..]
> > > 
> > > > +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 =)
> 
> Please do the magic instead of adding another API. It makes things
> simpler and will work for this case without having to change anything
> besides of_clk_add_provider().

All right. Let's go on this direction then.

> If the DT doesn't have the #clock-cells property in the node being
> registered then calling clk_get() will fail for any consumer devices
> that point to the node with a phandle and clock specifier. I don't
> expect us to get very far into development if that's the case.

Makes sense. So only potential thing to break is if someone out there
has broken DT/driver - where they currently see this failure. Eg. they
use node w/o #clock-cells as provider and where they try and fail
controlling this clock - but ignore the error (and system just "works"
with HW defaults). After this change they may actually succeed in
controlling - but do control wrong clock.

Not likely scenario (sure happens somewhere) - and it involves already
broken design. So I agree with you. Besides, you are the maintainer for
clk framework and thus get the most of the rain if **** hits the fan =D

> Of course, we don't fail in of_clk_add_provider() if there isn't a
> #clock-cells property in the node, we just happily add the node to the
> provider list and carry on. I doubt anyone is failing to specify the DT
> property, but maybe they are, in which case we could keep not failing
> and just add the node of whatever we're called with originally if
> neither the parent or the passed node have the #clock-cells property. I
> wouldn't try to go any higher than one node above the current node and
> look for a #clock-cells though.

I think we should use parent device's node, not the paren node in DT,
right? But I agree, we should only look "one level up in the chain".

> 
> If this all still seems scary then don't worry about it, I'll implement
> it myself.

It still is somewhat "scary" - but I really would like to use the devm
based provider registration in the bd718x7 driver so I will implement it
in this series. The engineer version of the "living on the edge", you
know =)

Br,
	Matti Vaittinen

-- 
Matti Vaittinen
ROHM Semiconductors

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

  reply	other threads:[~2018-12-04  7:13 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
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 [this message]
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=20181204071315.GA31204@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=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).