linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] clk: Add (devm_)clk_get_optional() functions
@ 2018-11-19 14:12 Phil Edworthy
  2018-11-20 10:38 ` Andy Shevchenko
  0 siblings, 1 reply; 6+ messages in thread
From: Phil Edworthy @ 2018-11-19 14:12 UTC (permalink / raw)
  To: Stephen Boyd, Michael Turquette, Russell King
  Cc: linux-clk, linux-kernel, linux-renesas-soc, Phil Edworthy,
	Greg Ungerer, Andy Shevchenko, Geert Uytterhoeven, Ralf Baechle,
	Paul Burton, James Hogan, Jiaxun Yang, Huacai Chen, Guan Xuetao,
	Uwe Kleine-König, linux-m68k, linux-mips, linux-arm-kernel

This adds clk_get_optional() and devm_clk_get_optional() functions to get
optional clocks.
They behave the same as (devm_)clk_get except where there is no clock
producer. In this case, instead of returning -ENOENT, the function
returns NULL. This makes error checking simpler and allows
clk_prepare_enable, etc to be called on the returned reference
without additional checks.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
v7:
 - Instead of messing with the core functions, simply wrap them for the
   _optional() versions. By putting clk_get_optional() inline in the header
   file, we can get rid of the arch specific patches as well.
v6:
 - Add doxygen style comment for devm_clk_get_optional() args
v5:
 - No changes.
v4:
 - No changes.
v3:
 - No changes.
---
 drivers/clk/clk-devres.c | 11 +++++++++++
 include/linux/clk.h      | 27 +++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 12c87457eca1..f0033d937c39 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -34,6 +34,17 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
 }
 EXPORT_SYMBOL(devm_clk_get);
 
+struct clk *devm_clk_get_optional(struct device *dev, const char *id)
+{
+	struct clk *clk = devm_clk_get(dev, id);
+
+	if (clk == ERR_PTR(-ENOENT))
+		return NULL;
+	else
+		return clk;
+}
+EXPORT_SYMBOL(devm_clk_get_optional);
+
 struct clk_bulk_devres {
 	struct clk_bulk_data *clks;
 	int num_clks;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index a7773b5c0b9f..c7bbb0678057 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -383,6 +383,17 @@ int __must_check devm_clk_bulk_get_all(struct device *dev,
  */
 struct clk *devm_clk_get(struct device *dev, const char *id);
 
+/**
+ * devm_clk_get_optional - lookup and obtain a managed reference to an optional
+ *			   clock producer.
+ * @dev: device for clock "consumer"
+ * @id: clock consumer ID
+ *
+ * Behaves the same as devm_clk_get except where there is no clock producer. In
+ * this case, instead of returning -ENOENT, the function returns NULL.
+ */
+struct clk *devm_clk_get_optional(struct device *dev, const char *id);
+
 /**
  * devm_get_clk_from_child - lookup and obtain a managed reference to a
  *			     clock producer from child node.
@@ -718,6 +729,12 @@ static inline struct clk *devm_clk_get(struct device *dev, const char *id)
 	return NULL;
 }
 
+static inline struct clk *devm_clk_get_optional(struct device *dev,
+						const char *id)
+{
+	return NULL;
+}
+
 static inline int __must_check devm_clk_bulk_get(struct device *dev, int num_clks,
 						 struct clk_bulk_data *clks)
 {
@@ -862,6 +879,16 @@ static inline void clk_bulk_disable_unprepare(int num_clks,
 	clk_bulk_unprepare(num_clks, clks);
 }
 
+static inline struct clk *clk_get_optional(struct device *dev, const char *id)
+{
+	struct clk *clk = clk_get(dev, id);
+
+	if (clk == ERR_PTR(-ENOENT))
+		return NULL;
+	else
+		return clk;
+}
+
 #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
 struct clk *of_clk_get(struct device_node *np, int index);
 struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] clk: Add (devm_)clk_get_optional() functions
  2018-11-19 14:12 [PATCH] clk: Add (devm_)clk_get_optional() functions Phil Edworthy
@ 2018-11-20 10:38 ` Andy Shevchenko
  2018-11-20 10:53   ` Phil Edworthy
  2018-11-20 12:56   ` Uwe Kleine-König
  0 siblings, 2 replies; 6+ messages in thread
From: Andy Shevchenko @ 2018-11-20 10:38 UTC (permalink / raw)
  To: Phil Edworthy
  Cc: Stephen Boyd, Michael Turquette, Russell King, linux-clk,
	linux-kernel, linux-renesas-soc, Greg Ungerer,
	Geert Uytterhoeven, Ralf Baechle, Paul Burton, James Hogan,
	Jiaxun Yang, Huacai Chen, Guan Xuetao, Uwe Kleine-König,
	linux-m68k, linux-mips, linux-arm-kernel

On Mon, Nov 19, 2018 at 02:12:59PM +0000, Phil Edworthy wrote:
> This adds clk_get_optional() and devm_clk_get_optional() functions to get
> optional clocks.
> They behave the same as (devm_)clk_get except where there is no clock
> producer. In this case, instead of returning -ENOENT, the function
> returns NULL. This makes error checking simpler and allows
> clk_prepare_enable, etc to be called on the returned reference
> without additional checks.

>  - Instead of messing with the core functions, simply wrap them for the
>    _optional() versions. By putting clk_get_optional() inline in the header
>    file, we can get rid of the arch specific patches as well.

Fine if it would have no surprises with error handling.

> +	if (ERR_PTR(-ENOENT))
> +		return NULL;
> +	else
> +		return clk;

return clk == ERR_PTR(-ENOENT) ? NULL : clk;

?

> +	if (clk == ERR_PTR(-ENOENT))
> +		return NULL;
> +	else
> +		return clk;

Ditto.


-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] clk: Add (devm_)clk_get_optional() functions
  2018-11-20 10:38 ` Andy Shevchenko
@ 2018-11-20 10:53   ` Phil Edworthy
  2018-11-20 11:12     ` Andy Shevchenko
  2018-11-20 12:56   ` Uwe Kleine-König
  1 sibling, 1 reply; 6+ messages in thread
From: Phil Edworthy @ 2018-11-20 10:53 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Stephen Boyd, Michael Turquette, Russell King, linux-clk,
	linux-kernel, linux-renesas-soc, Greg Ungerer,
	Geert Uytterhoeven, Ralf Baechle, Paul Burton, James Hogan,
	Jiaxun Yang, Huacai Chen, Guan Xuetao, Uwe Kleine-König,
	linux-m68k, linux-mips, linux-arm-kernel

Hi Andy,

On 20 November 2018 10:39 Andy Shevchenko wrote:
> On Mon, Nov 19, 2018 at 02:12:59PM +0000, Phil Edworthy wrote:
> > This adds clk_get_optional() and devm_clk_get_optional() functions to
> > get optional clocks.
> > They behave the same as (devm_)clk_get except where there is no clock
> > producer. In this case, instead of returning -ENOENT, the function
> > returns NULL. This makes error checking simpler and allows
> > clk_prepare_enable, etc to be called on the returned reference without
> > additional checks.
> 
> >  - Instead of messing with the core functions, simply wrap them for the
> >    _optional() versions. By putting clk_get_optional() inline in the header
> >    file, we can get rid of the arch specific patches as well.
> 
> Fine if it would have no surprises with error handling.
There shouldn't be any surprises. My earlier attempts at implementing this
were hampered by the fact that of_clk_get_by_name() can return -EINVAL
in some circumstances. By directly wrapping the (devm_)clk_get() functions
that problem goes away.

> > +	if (ERR_PTR(-ENOENT))
Huh? That wasn't the code I sent...

> > +		return NULL;
> > +	else
> > +		return clk;
> 
> return clk == ERR_PTR(-ENOENT) ? NULL : clk;
> 
> ?
> 
> > +	if (clk == ERR_PTR(-ENOENT))
> > +		return NULL;
> > +	else
> > +		return clk;
> 
> Ditto.
Sure, will fix both.

Thanks
Phil

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] clk: Add (devm_)clk_get_optional() functions
  2018-11-20 10:53   ` Phil Edworthy
@ 2018-11-20 11:12     ` Andy Shevchenko
  0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2018-11-20 11:12 UTC (permalink / raw)
  To: Phil Edworthy
  Cc: Stephen Boyd, Michael Turquette, Russell King, linux-clk,
	linux-kernel, linux-renesas-soc, Greg Ungerer,
	Geert Uytterhoeven, Ralf Baechle, Paul Burton, James Hogan,
	Jiaxun Yang, Huacai Chen, Guan Xuetao, Uwe Kleine-König,
	linux-m68k, linux-mips, linux-arm-kernel

On Tue, Nov 20, 2018 at 10:53:33AM +0000, Phil Edworthy wrote:
> On 20 November 2018 10:39 Andy Shevchenko wrote:
> > On Mon, Nov 19, 2018 at 02:12:59PM +0000, Phil Edworthy wrote:
> > > This adds clk_get_optional() and devm_clk_get_optional() functions to
> > > get optional clocks.
> > > They behave the same as (devm_)clk_get except where there is no clock
> > > producer. In this case, instead of returning -ENOENT, the function
> > > returns NULL. This makes error checking simpler and allows
> > > clk_prepare_enable, etc to be called on the returned reference without
> > > additional checks.

> > >  - Instead of messing with the core functions, simply wrap them for the
> > >    _optional() versions. By putting clk_get_optional() inline in the header
> > >    file, we can get rid of the arch specific patches as well.

> > Fine if it would have no surprises with error handling.
> There shouldn't be any surprises. My earlier attempts at implementing this
> were hampered by the fact that of_clk_get_by_name() can return -EINVAL
> in some circumstances. By directly wrapping the (devm_)clk_get() functions
> that problem goes away.

Good!

After my comments being addressed,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


> > > +	if (ERR_PTR(-ENOENT))
> Huh? That wasn't the code I sent...

Yup, it's my wrong editing flow. Anyway, you got the idea.

> > > +		return NULL;
> > > +	else
> > > +		return clk;

> > return clk == ERR_PTR(-ENOENT) ? NULL : clk;

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] clk: Add (devm_)clk_get_optional() functions
  2018-11-20 10:38 ` Andy Shevchenko
  2018-11-20 10:53   ` Phil Edworthy
@ 2018-11-20 12:56   ` Uwe Kleine-König
  2018-11-20 13:26     ` Andy Shevchenko
  1 sibling, 1 reply; 6+ messages in thread
From: Uwe Kleine-König @ 2018-11-20 12:56 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Phil Edworthy, Stephen Boyd, Michael Turquette, Russell King,
	linux-clk, linux-kernel, linux-renesas-soc, Greg Ungerer,
	Geert Uytterhoeven, Ralf Baechle, Paul Burton, James Hogan,
	Jiaxun Yang, Huacai Chen, Guan Xuetao, linux-m68k, linux-mips,
	linux-arm-kernel

Hello,

On Tue, Nov 20, 2018 at 12:38:33PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 19, 2018 at 02:12:59PM +0000, Phil Edworthy wrote:
> > +	if (clk == ERR_PTR(-ENOENT))
> > +		return NULL;
> > +	else
> > +		return clk;
> 
> return clk == ERR_PTR(-ENOENT) ? NULL : clk;
> 
> ?

Not sure this adds to the readability of the expression. Personally I
prefer the explicit if. Maybe even:

	clk = clk_get(...);

	if (clk == ERR_PTR(-ENOENT))
		clk = NULL;

	return clk;

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] clk: Add (devm_)clk_get_optional() functions
  2018-11-20 12:56   ` Uwe Kleine-König
@ 2018-11-20 13:26     ` Andy Shevchenko
  0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2018-11-20 13:26 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Phil Edworthy, Stephen Boyd, Michael Turquette, Russell King,
	linux-clk, linux-kernel, linux-renesas-soc, Greg Ungerer,
	Geert Uytterhoeven, Ralf Baechle, Paul Burton, James Hogan,
	Jiaxun Yang, Huacai Chen, Guan Xuetao, linux-m68k, linux-mips,
	linux-arm-kernel

On Tue, Nov 20, 2018 at 01:56:52PM +0100, Uwe Kleine-König wrote:
> On Tue, Nov 20, 2018 at 12:38:33PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 19, 2018 at 02:12:59PM +0000, Phil Edworthy wrote:
> > > +	if (clk == ERR_PTR(-ENOENT))
> > > +		return NULL;
> > > +	else
> > > +		return clk;
> > 
> > return clk == ERR_PTR(-ENOENT) ? NULL : clk;
> > 
> > ?
> 
> Not sure this adds to the readability of the expression. Personally I
> prefer the explicit if. Maybe even:
> 
> 	clk = clk_get(...);
> 
> 	if (clk == ERR_PTR(-ENOENT))
> 		clk = NULL;
> 
> 	return clk;

So, it almost repeats the initial variant.
I'm fine with no 'else' in initial code, like

if (...)
	return NULL;

return clk;


-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-11-20 13:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-19 14:12 [PATCH] clk: Add (devm_)clk_get_optional() functions Phil Edworthy
2018-11-20 10:38 ` Andy Shevchenko
2018-11-20 10:53   ` Phil Edworthy
2018-11-20 11:12     ` Andy Shevchenko
2018-11-20 12:56   ` Uwe Kleine-König
2018-11-20 13:26     ` Andy Shevchenko

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).