From: Stephen Boyd <sboyd@kernel.org>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Michael Turquette <mturquette@baylibre.com>,
Phil Edworthy <phil.edworthy@renesas.com>,
Russell King <linux@armlinux.org.uk>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>,
Simon Horman <horms@verge.net.au>,
"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>
Subject: RE: [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function
Date: Tue, 13 Nov 2018 11:44:50 -0800 [thread overview]
Message-ID: <154213829039.88331.14286655280073748584@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <TY1PR01MB1769B2978D73DBC2EADC6B8DF50C0@TY1PR01MB1769.jpnprd01.prod.outlook.com>
Quoting Phil Edworthy (2018-09-03 06:21:02)
> Hi Stephen,
>
> On 03 September 2018 10:33 Phil Edworthy wrote:
> > On 01 September 2018 03:46, Stephen Boyd wrote:
> > > Quoting Phil Edworthy (2018-08-31 07:07:22)
> > > > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index
> > > > 9ab3db8..4adb99e 100644
> > > > --- a/drivers/clk/clkdev.c
> > > > +++ b/drivers/clk/clkdev.c
> > > > @@ -54,30 +54,29 @@ EXPORT_SYMBOL(of_clk_get);
> > > >
> > > > static struct clk *__of_clk_get_by_name(struct device_node *np,
> > > > const char *dev_id,
> > > > - const char *name)
> > > > + const char *name,
> > > > + bool optional)
> > > > {
> > > > struct clk *clk = ERR_PTR(-ENOENT);
> > > > + struct device_node *child = np;
> > > > + int index = 0;
> > > >
> > > > /* Walk up the tree of devices looking for a clock that matches */
> > > > while (np) {
> > > > - int index = 0;
> > > >
> > > > /*
> > > > * For named clocks, first look up the name in the
> > > > * "clock-names" property. If it cannot be found, then
> > > > - * index will be an error code, and of_clk_get() will fail.
> > > > + * index will be an error code.
> > > > */
> > > > if (name)
> > > > index = of_property_match_string(np, "clock-names",
> > name);
> > > > - clk = __of_clk_get(np, index, dev_id, name);
> > > > - if (!IS_ERR(clk)) {
> > > > - break;
> > > > - } else if (name && index >= 0) {
> > > > - if (PTR_ERR(clk) != -EPROBE_DEFER)
> > > > - pr_err("ERROR: could not get clock %pOF:%s(%i)\n",
> > > > - np, name ? name : "", index);
> > > > + if (index >= 0)
> > > > + clk = __of_clk_get(np, index, dev_id, name);
> > > > + if (!IS_ERR(clk))
> > >
> > > Was this change necessary? It looks like we can leave it all alone and keep
> > > passing a negative number to __of_clk_get() and have that return an error
> > > pointer which we then return immediately as an error. But, if the clock is
> > > optional and we've passed a name here, shouldn't we treat an error from
> > > of_property_match_string() as success too? This is all looking pretty fragile
> > so
> > > maybe it can be better commented and also more explicit instead of relying
> > > on the reader to jump through all the function calls to figure out what the
> > > return value is in some cases.
> > If we call __of_clk_get, with index < 0, we will not be able to differentiate
> > between clock provider not present and other errors with the passed data,
> > as it will just return -EINVAL.
> >
> > of_property_match_string() will return -EINVAL if the "clock-names"
> > property
> > is missing, or -ENODATA if the specified clock name in the "clock-names"
> > property is missing. That is why I have changed the code to conditionally
> > call __of_clk_get, so the code will correctly treat optional clocks that are not
> > present.
> When getting named optional clocks, if the node has a "clock-names" property,
> but no clock matching the name we want, I think the function should stop there
> and *not* walk up the tree of devices looking for a matching clock. In this case,
> the code determines that the optional clock is not present.
>
> If there isn’t a "clock-names" property in the current node, the function should
> walk up the tree of devices looking for a matching optional clock. If there are no
> parent nodes left and we haven't found a matching optional clock, we determine
> that the clock isn’t there.
>
> Is that how this should work?
>
Yes that sounds right.
next prev parent reply other threads:[~2018-11-13 19:44 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-31 14:07 [PATCH v5 0/2] clk: Add functions to get optional clocks Phil Edworthy
2018-08-31 14:07 ` [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function Phil Edworthy
2018-09-01 2:45 ` Stephen Boyd
2018-09-03 9:32 ` Phil Edworthy
2018-09-03 13:21 ` Phil Edworthy
2018-11-13 19:44 ` Stephen Boyd [this message]
2018-08-31 14:07 ` [PATCH v5 2/2] clk: Add functions to get optional clocks Phil Edworthy
2018-09-01 11:43 ` kbuild test robot
2018-09-01 16:01 ` kbuild test robot
2018-08-31 18:23 ` [PATCH v5 0/2] " Andy Shevchenko
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=154213829039.88331.14286655280073748584@swboyd.mtv.corp.google.com \
--to=sboyd@kernel.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=geert@linux-m68k.org \
--cc=horms@verge.net.au \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=mturquette@baylibre.com \
--cc=phil.edworthy@renesas.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).