| Message ID | 20181203111309.3709-2-phil.edworthy@renesas.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | clk: Add functions to get optional clocks | expand |
On Mon, Dec 03, 2018 at 11:13:08AM +0000, Phil Edworthy wrote: > It's not immediately obvious from the code that failure to get a > clock provider can return either -ENOENT or -EINVAL. Therefore, add > a comment to highlight this. > +/* > + * Beware the return values when np is valid, but no clock provider is found. > + * If name = NULL, the function returns -ENOENT. > + * If name != NULL, the function returns -EINVAL. This is because __of_clk_get() I would start new sentence from new line (this will emphasize the possible variants) * This is ... Otherwise looks good to me: Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > + * is called even if of_property_match_string() returns an error. > + */ > static struct clk *__of_clk_get_by_name(struct device_node *np, > const char *dev_id, > const char *name) > -- > 2.17.1 >
Hi Andy, On 03 December 2018 13:31 Andy Shevchenko wrote: > On Mon, Dec 03, 2018 at 11:13:08AM +0000, Phil Edworthy wrote: > > It's not immediately obvious from the code that failure to get a clock > > provider can return either -ENOENT or -EINVAL. Therefore, add a > > comment to highlight this. > > > +/* > > + * Beware the return values when np is valid, but no clock provider is > found. > > + * If name = NULL, the function returns -ENOENT. > > + * If name != NULL, the function returns -EINVAL. This is because > > +__of_clk_get() > > I would start new sentence from new line (this will emphasize the possible > variants) > > * This is ... I disagree, the explanation is specifically related to the case where the function returns -EINVAL. Though this is a nit, so I'm not really bothered either way. Thanks for the review! Phil > Otherwise looks good to me: > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > + * is called even if of_property_match_string() returns an error. > > + */ > > static struct clk *__of_clk_get_by_name(struct device_node *np, > > const char *dev_id, > > const char *name) > > -- > > 2.17.1 > > > > -- > With Best Regards, > Andy Shevchenko >
Hi, Any other comments on this patch and patch 2/2 (https://lkml.org/lkml/2018/12/3/326)? Thanks Phil > -----Original Message----- > From: Phil Edworthy > Sent: 06 December 2018 12:31 > To: 'Andy Shevchenko' <andriy.shevchenko@linux.intel.com> > Cc: Michael Turquette <mturquette@baylibre.com>; Stephen Boyd > <sboyd@kernel.org>; Russell King <linux@armlinux.org.uk>; Geert > Uytterhoeven <geert@linux-m68k.org>; Uwe Kleine-König <u.kleine- > koenig@pengutronix.de>; linux-clk@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: RE: [PATCH v9 1/2] clk: Add comment about > __of_clk_get_by_name() error values > > Hi Andy, > > On 03 December 2018 13:31 Andy Shevchenko wrote: > > On Mon, Dec 03, 2018 at 11:13:08AM +0000, Phil Edworthy wrote: > > > It's not immediately obvious from the code that failure to get a > > > clock provider can return either -ENOENT or -EINVAL. Therefore, add > > > a comment to highlight this. > > > > > +/* > > > + * Beware the return values when np is valid, but no clock provider > > > +is > > found. > > > + * If name = NULL, the function returns -ENOENT. > > > + * If name != NULL, the function returns -EINVAL. This is because > > > +__of_clk_get() > > > > I would start new sentence from new line (this will emphasize the > > possible > > variants) > > > > * This is ... > I disagree, the explanation is specifically related to the case where the > function returns -EINVAL. Though this is a nit, so I'm not really bothered > either way. > > Thanks for the review! > Phil > > > Otherwise looks good to me: > > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > + * is called even if of_property_match_string() returns an error. > > > + */ > > > static struct clk *__of_clk_get_by_name(struct device_node *np, > > > const char *dev_id, > > > const char *name) > > > -- > > > 2.17.1 > > > > > > > -- > > With Best Regards, > > Andy Shevchenko > >
On Wed, Jan 16, 2019 at 03:18:42PM +0000, Phil Edworthy wrote: > Hi, > > Any other comments on this patch and patch 2/2 (https://lkml.org/lkml/2018/12/3/326)? Was on vacations, sorry. > > > I would start new sentence from new line (this will emphasize the > > > possible > > > variants) > > > > > > * This is ... > > I disagree, the explanation is specifically related to the case where the > > function returns -EINVAL. Though this is a nit, so I'm not really bothered > > either way. Ah, okay. You may bear my tags on.
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index 9ab3db8b3988..cc5df3970cd3 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -52,6 +52,12 @@ struct clk *of_clk_get(struct device_node *np, int index) } EXPORT_SYMBOL(of_clk_get); +/* + * Beware the return values when np is valid, but no clock provider is found. + * If name = NULL, the function returns -ENOENT. + * If name != NULL, the function returns -EINVAL. This is because __of_clk_get() + * is called even if of_property_match_string() returns an error. + */ static struct clk *__of_clk_get_by_name(struct device_node *np, const char *dev_id, const char *name)
It's not immediately obvious from the code that failure to get a clock provider can return either -ENOENT or -EINVAL. Therefore, add a comment to highlight this. Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> --- drivers/clk/clkdev.c | 6 ++++++ 1 file changed, 6 insertions(+)