RE: [PATCH] clk: Add functions to get optional clocks

[Phil Edworthy]

Hi Stephen,

On 25 July 2018 23:37, Stephen Boyd wrote:
> Quoting Phil Edworthy (2018-07-18 06:56:26)
> > On 18 July 2018 14:19, Geert Uytterhoeven wrote:
> > > On Wed, Jul 18, 2018 at 3:02 PM Russell King - ARM Linux  wrote:
> > > > On Wed, Jul 18, 2018 at 01:57:38PM +0100, Phil Edworthy wrote:
> > > > > 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.
> > > >
> > > > How does this work with non-DT systems, where looking a clock up
> > > > which isn't yet registered with clkdev returns -ENOENT ?
> > > >
> > > > (clkdev doesn't know when all clocks are registered with it.)
> > >
> > > Good question.
> > >
> > > I guess all drivers trying to handle optional clocks this way are
> > > already broken on non-DT systems where clocks may be registered late...
> >
> > So how do non-DT systems that look a clock up which isn't yet
> > registered with clkdev, determine that an optional clock is there or
> > not?
> >
> 
> Short answer is they don't. I'd still prefer we have this API though.
> 
> Can you rework this patch to be a little more invasive into the
> clk_get() path, perhaps by reworking __of_clk_get_by_name() a little to
> take an 'optional' argument, so that it only returns NULL when the clk is
> looked up from DT? The fallback path in clkdev where we have a DT based
> system looking up a clk through clkdev lookups doesn't seem to be a real
> scenario that we should worry about here. I think sometimes people use
> clkdev lookups when they're migrating to DT systems and things aren't wired
> up properly in DT, but that isn't the norm.
Do you mean something like this:

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 7f2cd1e..42a7d4e 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -57,7 +57,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);
 
@@ -79,6 +80,8 @@ static struct clk *__of_clk_get_by_name(struct device_node *np,
                        if (PTR_ERR(clk) != -EPROBE_DEFER)
                                pr_err("ERROR: could not get clock %pOF:%s(%i)\n",
                                        np, name ? name : "", index);
+                       if (optional && PTR_ERR(clk) == -ENOENT)
+                               clk = NULL;
                        return clk;
                }
 
@@ -109,15 +112,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);
 }
@@ -200,7 +226,7 @@ struct clk *clk_get(struct device *dev, const char *con_id)
        struct clk *clk;
 
        if (dev) {
-               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 907202b..830209a 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -771,6 +771,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)

---
A lot of drivers use devm_clk_get() so I think a devm_clk_get_optional()
version would be useful. That would probably need an additional
clk_get_optional() function.

Thanks
Phil