Quite a few drivers get an optional clock, e.g. a clock required
to access peripheral's registers that is always enabled on some
devices.
Phil Edworthy (2):
clk: Add of_clk_get_by_name_optional() function
clk: Add functions to get optional clocks
drivers/clk/clk-devres.c | 18 ++++++++++++++++--
drivers/clk/clkdev.c | 49 +++++++++++++++++++++++++++++++++++++++++++-----
include/linux/clk.h | 30 +++++++++++++++++++++++++++++
3 files changed, 90 insertions(+), 7 deletions(-)
--
2.7.4
Quite a few drivers get an optional clock, e.g. a clock required
to access peripheral's registers that is always enabled on some
devices.
This function behaves the same as of_clk_get_by_name() except that
it will return NULL instead of -ENOENT. This makes error checking
for users easier and allows clk_prepare_enable, etc to be called
on the returned reference without additional checks.
Signed-off-by: Phil Edworthy <[email protected]>
---
drivers/clk/clkdev.c | 34 ++++++++++++++++++++++++++++++----
include/linux/clk.h | 1 +
2 files changed, 31 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 9ab3db8..c1b7262 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -54,7 +54,8 @@ 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);
@@ -73,6 +74,8 @@ static struct clk *__of_clk_get_by_name(struct device_node *np,
if (!IS_ERR(clk)) {
break;
} else if (name && index >= 0) {
+ if (optional && PTR_ERR(clk) == -ENOENT)
+ clk = NULL;
if (PTR_ERR(clk) != -EPROBE_DEFER)
pr_err("ERROR: could not get clock %pOF:%s(%i)\n",
np, name ? name : "", index);
@@ -106,15 +109,38 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
if (!np)
return ERR_PTR(-ENOENT);
- return __of_clk_get_by_name(np, np->full_name, name);
+ return __of_clk_get_by_name(np, np->full_name, name, false);
}
EXPORT_SYMBOL(of_clk_get_by_name);
+/**
+ * of_clk_get_by_name_optional() - Parse and lookup an optional clock referenced
+ * by a device node
+ * @np: pointer to clock consumer node
+ * @name: name of consumer's clock input, or NULL for the first clock reference
+ *
+ * This function parses the clocks and clock-names properties,
+ * and uses them to look up the struct clk from the registered list of clock
+ * providers.
+ * It behaves the same as of_clk_get_by_name(), except when no clock is found.
+ * In this case, instead of returning -ENOENT, it returns NULL.
+ */
+struct clk *of_clk_get_by_name_optional(struct device_node *np,
+ const char *name)
+{
+ if (!np)
+ return ERR_PTR(-ENOENT);
+
+ return __of_clk_get_by_name(np, np->full_name, name, true);
+}
+EXPORT_SYMBOL(of_clk_get_by_name_optional);
+
#else /* defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) */
static struct clk *__of_clk_get_by_name(struct device_node *np,
const char *dev_id,
- const char *name)
+ const char *name,
+ bool optional)
{
return ERR_PTR(-ENOENT);
}
@@ -197,7 +223,7 @@ struct clk *clk_get(struct device *dev, const char *con_id)
struct clk *clk;
if (dev && dev->of_node) {
- clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id);
+ clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id, false);
if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
return clk;
}
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 4f750c4..8cb5455 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -777,6 +777,7 @@ static inline void clk_bulk_disable_unprepare(int num_clks,
#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);
+struct clk *of_clk_get_by_name_optional(struct device_node *np, const char *name);
struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
#else
static inline struct clk *of_clk_get(struct device_node *np, int index)
--
2.7.4
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. 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]>
---
drivers/clk/clk-devres.c | 18 ++++++++++++++++--
drivers/clk/clkdev.c | 17 +++++++++++++++--
include/linux/clk.h | 29 +++++++++++++++++++++++++++++
3 files changed, 60 insertions(+), 4 deletions(-)
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index d854e26..a2bb01a 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -14,7 +14,7 @@ static void devm_clk_release(struct device *dev, void *res)
clk_put(*(struct clk **)res);
}
-struct clk *devm_clk_get(struct device *dev, const char *id)
+static struct clk *__devm_clk_get(struct device *dev, const char *id, bool optional)
{
struct clk **ptr, *clk;
@@ -22,7 +22,10 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
if (!ptr)
return ERR_PTR(-ENOMEM);
- clk = clk_get(dev, id);
+ if (!optional)
+ clk = clk_get(dev, id);
+ else
+ clk = clk_get_optional(dev, id);
if (!IS_ERR(clk)) {
*ptr = clk;
devres_add(dev, ptr);
@@ -32,8 +35,19 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
return clk;
}
+
+struct clk *devm_clk_get(struct device *dev, const char *id)
+{
+ return __devm_clk_get(dev, id, false);
+}
EXPORT_SYMBOL(devm_clk_get);
+struct clk *devm_clk_get_optional(struct device *dev, const char *id)
+{
+ return __devm_clk_get(dev, id, true);
+}
+EXPORT_SYMBOL(devm_clk_get_optional);
+
struct clk_bulk_devres {
struct clk_bulk_data *clks;
int num_clks;
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index c1b7262..484a444 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -217,21 +217,34 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id)
}
EXPORT_SYMBOL(clk_get_sys);
-struct clk *clk_get(struct device *dev, const char *con_id)
+static struct clk *internal_clk_get(struct device *dev, const char *con_id,
+ bool optional)
{
const char *dev_id = dev ? dev_name(dev) : NULL;
struct clk *clk;
if (dev && dev->of_node) {
- clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id, false);
+ clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id,
+ optional);
if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
return clk;
}
return clk_get_sys(dev_id, con_id);
}
+
+struct clk *clk_get(struct device *dev, const char *con_id)
+{
+ return internal_clk_get(dev, con_id, false);
+}
EXPORT_SYMBOL(clk_get);
+struct clk *clk_get_optional(struct device *dev, const char *con_id)
+{
+ return internal_clk_get(dev, con_id, true);
+}
+EXPORT_SYMBOL(clk_get_optional);
+
void clk_put(struct clk *clk)
{
__clk_put(clk);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 8cb5455..22040ea 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -291,6 +291,16 @@ static inline void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)
struct clk *clk_get(struct device *dev, const char *id);
/**
+ * clk_get_optional - lookup and obtain a reference to optional clock producer.
+ * @dev: device for clock "consumer"
+ * @id: clock consumer ID
+ *
+ * Behaves the same as clk_get except where there is no clock producer. In this
+ * case, instead of returning -ENOENT, the function returns NULL.
+ */
+struct clk *clk_get_optional(struct device *dev, const char *id);
+
+/**
* clk_bulk_get - lookup and obtain a number of references to clock producer.
* @dev: device for clock "consumer"
* @num_clks: the number of clk_bulk_data
@@ -349,6 +359,14 @@ int __must_check devm_clk_bulk_get(struct device *dev, int num_clks,
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.
+ * 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.
* @dev: device for clock "consumer"
@@ -636,6 +654,11 @@ static inline struct clk *clk_get(struct device *dev, const char *id)
return NULL;
}
+static inline struct clk *clk_get_optional(struct device *dev, const char *id)
+{
+ return NULL;
+}
+
static inline int __must_check clk_bulk_get(struct device *dev, int num_clks,
struct clk_bulk_data *clks)
{
@@ -647,6 +670,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)
{
--
2.7.4
Hi Phil,
On Mon, Jul 30, 2018 at 10:36 AM Phil Edworthy
<[email protected]> wrote:
> Quite a few drivers get an optional clock, e.g. a clock required
> to access peripheral's registers that is always enabled on some
> devices.
>
> This function behaves the same as of_clk_get_by_name() except that
> it will return NULL instead of -ENOENT. This makes error checking
> for users easier and allows clk_prepare_enable, etc to be called
> on the returned reference without additional checks.
>
> Signed-off-by: Phil Edworthy <[email protected]>
Thanks for your patch!
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -54,7 +54,8 @@ 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);
>
> @@ -73,6 +74,8 @@ static struct clk *__of_clk_get_by_name(struct device_node *np,
> if (!IS_ERR(clk)) {
> break;
> } else if (name && index >= 0) {
> + if (optional && PTR_ERR(clk) == -ENOENT)
> + clk = NULL;
This only handles the "name && index >= 0" case.
If that condition is never true, the loop will end, eventually, and the last
value of clk will be returned. Hence there should be a similar check at the
end of the function.
> if (PTR_ERR(clk) != -EPROBE_DEFER)
> pr_err("ERROR: could not get clock %pOF:%s(%i)\n",
> np, name ? name : "", index);
Hence if not found, this will always print an error, even if the clock is
optional?
> @@ -106,15 +109,38 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
> if (!np)
> return ERR_PTR(-ENOENT);
>
> - return __of_clk_get_by_name(np, np->full_name, name);
> + return __of_clk_get_by_name(np, np->full_name, name, false);
> }
> EXPORT_SYMBOL(of_clk_get_by_name);
>
> +/**
> + * of_clk_get_by_name_optional() - Parse and lookup an optional clock referenced
> + * by a device node
> + * @np: pointer to clock consumer node
> + * @name: name of consumer's clock input, or NULL for the first clock reference
> + *
> + * This function parses the clocks and clock-names properties,
> + * and uses them to look up the struct clk from the registered list of clock
> + * providers.
> + * It behaves the same as of_clk_get_by_name(), except when no clock is found.
> + * In this case, instead of returning -ENOENT, it returns NULL.
> + */
> +struct clk *of_clk_get_by_name_optional(struct device_node *np,
> + const char *name)
> +{
> + if (!np)
> + return ERR_PTR(-ENOENT);
Shouldn't this return NULL?
Or let __of_clk_get_by_name() handle that (cfr. above)?
Hmm, of_clk_get_by_name() has a similar check, while the current
__of_clk_get_by_name() already handle np == NULL, too.
> +
> + return __of_clk_get_by_name(np, np->full_name, name, true);
> +}
> +EXPORT_SYMBOL(of_clk_get_by_name_optional);
> +
> #else /* defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) */
>
> static struct clk *__of_clk_get_by_name(struct device_node *np,
> const char *dev_id,
> - const char *name)
> + const char *name,
> + bool optional)
> {
> return ERR_PTR(-ENOENT);
> }
> @@ -197,7 +223,7 @@ struct clk *clk_get(struct device *dev, const char *con_id)
> struct clk *clk;
>
> if (dev && dev->of_node) {
> - clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id);
> + clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id, false);
> if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
> return clk;
> }
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 4f750c4..8cb5455 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -777,6 +777,7 @@ static inline void clk_bulk_disable_unprepare(int num_clks,
> #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);
> +struct clk *of_clk_get_by_name_optional(struct device_node *np, const char *name);
> struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
> #else
> static inline struct clk *of_clk_get(struct device_node *np, int index)
No dummy version of of_clk_get_by_name_optional() for the !CLK || !COMMON_CLK
case?
It seems of_clk_get_by_name() and of_clk_get_by_name_optional() can just
be simple wrappers around __of_clk_get_by_name(), differing only in the
value of the "optional" parameter. Hence I think it makes sense to move them
to the header file, and make them static inline.
Then only __of_clk_get_by_name() needs a (static inline) dummy for the !OF ||
!COMMON_CLK case.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
On 30 July 2018 09:56, Geert Uytterhoeven wrote:
> On Mon, Jul 30, 2018 at 10:36 AM Phil Edworthy wrote:
> > Quite a few drivers get an optional clock, e.g. a clock required to
> > access peripheral's registers that is always enabled on some devices.
> >
> > This function behaves the same as of_clk_get_by_name() except that it
> > will return NULL instead of -ENOENT. This makes error checking for
> > users easier and allows clk_prepare_enable, etc to be called on the
> > returned reference without additional checks.
> >
> > Signed-off-by: Phil Edworthy <[email protected]>
>
> Thanks for your patch!
>
> > --- a/drivers/clk/clkdev.c
> > +++ b/drivers/clk/clkdev.c
> > @@ -54,7 +54,8 @@ 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);
> >
> > @@ -73,6 +74,8 @@ static struct clk *__of_clk_get_by_name(struct
> device_node *np,
> > if (!IS_ERR(clk)) {
> > break;
> > } else if (name && index >= 0) {
> > + if (optional && PTR_ERR(clk) == -ENOENT)
> > + clk = NULL;
>
> This only handles the "name && index >= 0" case.
> If that condition is never true, the loop will end, eventually, and the last value
> of clk will be returned. Hence there should be a similar check at the end of
> the function.
Oops, I was only considering named optional clocks, I'll fix this.
> > if (PTR_ERR(clk) != -EPROBE_DEFER)
> > pr_err("ERROR: could not get clock %pOF:%s(%i)\n",
> > np, name ? name : "", index);
>
> Hence if not found, this will always print an error, even if the clock is
> optional?
Right.
> > @@ -106,15 +109,38 @@ struct clk *of_clk_get_by_name(struct
> device_node *np, const char *name)
> > if (!np)
> > return ERR_PTR(-ENOENT);
> >
> > - return __of_clk_get_by_name(np, np->full_name, name);
> > + return __of_clk_get_by_name(np, np->full_name, name, false);
> > }
> > EXPORT_SYMBOL(of_clk_get_by_name);
> >
> > +/**
> > + * of_clk_get_by_name_optional() - Parse and lookup an optional clock
> > +referenced
> > + * by a device node
> > + * @np: pointer to clock consumer node
> > + * @name: name of consumer's clock input, or NULL for the first clock
> > +reference
> > + *
> > + * This function parses the clocks and clock-names properties,
> > + * and uses them to look up the struct clk from the registered list
> > +of clock
> > + * providers.
> > + * It behaves the same as of_clk_get_by_name(), except when no clock is
> found.
> > + * In this case, instead of returning -ENOENT, it returns NULL.
> > + */
> > +struct clk *of_clk_get_by_name_optional(struct device_node *np,
> > + const char *name) {
> > + if (!np)
> > + return ERR_PTR(-ENOENT);
>
> Shouldn't this return NULL?
I wasn’t sure on this, I thought the node should be valid, but I guess the node
is also optional. I'll fix.
> Or let __of_clk_get_by_name() handle that (cfr. above)?
> Hmm, of_clk_get_by_name() has a similar check, while the current
> __of_clk_get_by_name() already handle np == NULL, too.
Yes I see, I'll clean that up.
> > +
> > + return __of_clk_get_by_name(np, np->full_name, name, true); }
> > +EXPORT_SYMBOL(of_clk_get_by_name_optional);
> > +
> > #else /* defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) */
> >
> > static struct clk *__of_clk_get_by_name(struct device_node *np,
> > const char *dev_id,
> > - const char *name)
> > + const char *name,
> > + bool optional)
> > {
> > return ERR_PTR(-ENOENT);
> > }
> > @@ -197,7 +223,7 @@ struct clk *clk_get(struct device *dev, const char
> *con_id)
> > struct clk *clk;
> >
> > if (dev && dev->of_node) {
> > - clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id);
> > + clk = __of_clk_get_by_name(dev->of_node, dev_id,
> > + con_id, false);
> > if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
> > return clk;
> > }
> > diff --git a/include/linux/clk.h b/include/linux/clk.h index
> > 4f750c4..8cb5455 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -777,6 +777,7 @@ static inline void clk_bulk_disable_unprepare(int
> > num_clks, #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);
> > +struct clk *of_clk_get_by_name_optional(struct device_node *np, const
> > +char *name);
> > struct clk *of_clk_get_from_provider(struct of_phandle_args
> > *clkspec); #else static inline struct clk *of_clk_get(struct
> > device_node *np, int index)
>
> No dummy version of of_clk_get_by_name_optional() for the !CLK ||
> !COMMON_CLK case?
>
> It seems of_clk_get_by_name() and of_clk_get_by_name_optional() can
> just be simple wrappers around __of_clk_get_by_name(), differing only in
> the value of the "optional" parameter. Hence I think it makes sense to move
> them to the header file, and make them static inline.
> Then only __of_clk_get_by_name() needs a (static inline) dummy for the
> !OF || !COMMON_CLK case.
Makes sense.
Thanks!
Phil
On Mon, 2018-07-30 at 10:55 +0200, Geert Uytterhoeven wrote:
> On Mon, Jul 30, 2018 at 10:36 AM Phil Edworthy
> <[email protected]> wrote:
> > +struct clk *of_clk_get_by_name_optional(struct device_node *np,
> > + const char *name)
> > +{
> > + if (!np)
> > + return ERR_PTR(-ENOENT);
>
> Shouldn't this return NULL?
> Or let __of_clk_get_by_name() handle that (cfr. above)?
>
> Hmm, of_clk_get_by_name() has a similar check, while the current
> __of_clk_get_by_name() already handle np == NULL, too.
This check is needed to prevent NULL pointer dereference below.
And I agree that absence of device node should be considered as okay
case for optional clock (!CONFIG_OF case, for example).
> > +
> > + return __of_clk_get_by_name(np, np->full_name, name, true);
> > +}
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
Hi Andy,
On Mon, Jul 30, 2018 at 12:57 PM Andy Shevchenko
<[email protected]> wrote:
> On Mon, 2018-07-30 at 10:55 +0200, Geert Uytterhoeven wrote:
> > On Mon, Jul 30, 2018 at 10:36 AM Phil Edworthy
> > <[email protected]> wrote:
>
> > > +struct clk *of_clk_get_by_name_optional(struct device_node *np,
> > > + const char *name)
> > > +{
> > > + if (!np)
> > > + return ERR_PTR(-ENOENT);
> >
> > Shouldn't this return NULL?
> > Or let __of_clk_get_by_name() handle that (cfr. above)?
> >
> > Hmm, of_clk_get_by_name() has a similar check, while the current
> > __of_clk_get_by_name() already handle np == NULL, too.
>
> This check is needed to prevent NULL pointer dereference below.
Thank, I had missed that unexpected detail.
> > > +
> > > + return __of_clk_get_by_name(np, np->full_name, name, true);
> > > +}
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds