[v3,1/2] clk: Add of_clk_get_by_name_optional() function

Commit Message

Phil Edworthy July 30, 2018, 1:31 p.m. UTC

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

Signed-off-by: Phil Edworthy <[e-mail]>
---
v3:
 - Fix check for clock not present. __of_clk_get() returns -EINVAL
   if it's not there. Cover case of when there is no clock name.
 - of_clk_get_by_name_optional() should return NULL if !np.
 - Add dummy version of of_clk_get_by_name_optional() for the
   !OF || !COMMON_CLK case.
---
 drivers/clk/clkdev.c | 36 ++++++++++++++++++++++++++++++++----
 include/linux/clk.h  |  6 ++++++
 2 files changed, 38 insertions(+), 4 deletions(-)

Comments

Andy Shevchenko July 30, 2018, 4:03 p.m. UTC | #1

On Mon, 2018-07-30 at 14:31 +0100, 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 -EINVAL.

I'm puzzled a bit.

__of_clk_get() may return few error codes, and to me ENOENT sounds
correct when clock is not found. Other error codes should be passed to
the caller even for optional clocks.

If above is not true, we need to understand what circumstances for each
possible returned code are, and fix / act accordingly.

P.S. Possible way like regulator framework does is to return -ENODEV.

So, basically what I'm asking here is to be sure that single error code
(for now supposed -EINVAL) in this case is _the_ error code for absent /
can't be found clock.

>  - Fix check for clock not present. __of_clk_get() returns -EINVAL
>    if it's not there. Cover case of when there is no clock name.

Phil Edworthy July 30, 2018, 4:38 p.m. UTC | #2

Hi Andy,

On 30 July 2018 17:04, Andy Shevchenko wrote:
> On Mon, 2018-07-30 at 14:31 +0100, 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 -EINVAL.
> 
> I'm puzzled a bit.
> 
> __of_clk_get() may return few error codes, and to me ENOENT sounds
> correct when clock is not found. Other error codes should be passed to the
> caller even for optional clocks.
> 
> If above is not true, we need to understand what circumstances for each
> possible returned code are, and fix / act accordingly.
>
> P.S. Possible way like regulator framework does is to return -ENODEV.
> 
> So, basically what I'm asking here is to be sure that single error code (for now
> supposed -EINVAL) in this case is _the_ error code for absent / can't be
> found clock.
of_property_match_string() can return different return values for when:
 the "clock-names" property is missing (-EINVAL),
 the specified clock name in the "clock-names" property is missing  (-ENODATA),
 or internal errors, (e.g. -EILSEQ).
However, if you then call __of_clk_get() with the failed index, you just get
the -EINVAL error.

Looking at it further, __of_clk_get() should return -ENOENT if passed a valid
index (e.g. 0 when name=NULL) and the clock is not there. However, I think
it will take a while to check all possible return values as it's not immediately
clear what they are (to me).

I think the best thing is to test the return value of of_property_match_string()
for -ENODATA, and deal with it then. Then deal with the case of name=NULL
separately.

Thanks
Phil

 >  - Fix check for clock not present. __of_clk_get() returns -EINVAL
> >    if it's not there. Cover case of when there is no clock name.
> 
> --
> Andy Shevchenko <[e-mail]>
> Intel Finland Oy

Patch

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 9ab3db8..a710333 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);
 
@@ -70,6 +71,11 @@  static struct clk *__of_clk_get_by_name(struct device_node *np,
 		if (name)
 			index = of_property_match_string(np, "clock-names", name);
 		clk = __of_clk_get(np, index, dev_id, name);
+		if (optional && PTR_ERR(clk) == -EINVAL) {
+			clk = NULL;
+			pr_info("optional clock is not present %pOF:%s\n", np,
+				name ? name : "");
+		}
 		if (!IS_ERR(clk)) {
 			break;
 		} else if (name && index >= 0) {
@@ -106,15 +112,37 @@  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 np is NULL or no
+ * clock provider is found, when it then returns NULL.
+ */
+struct clk *of_clk_get_by_name_optional(struct device_node *np,
+					const char *name)
+{
+	if (!np)
+		return NULL;
+
+	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 +225,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..de0e5e0 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)
@@ -788,6 +789,11 @@  static inline struct clk *of_clk_get_by_name(struct device_node *np,
 {
 	return ERR_PTR(-ENOENT);
 }
+static inline struct clk *of_clk_get_by_name_optional(struct device_node *np,
+						      const char *name)
+{
+	return NULL;
+}
 static inline struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec)
 {
 	return ERR_PTR(-ENOENT);