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 <[email protected]>
---
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
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
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
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 <[email protected]>
> > > + 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
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/ |
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