| Message ID | 1535724443-21150-2-git-send-email-phil.edworthy@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clk: Add functions to get optional clocks | expand |
Quoting Phil Edworthy (2018-08-31 07:07:22) > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c > index 9ab3db8..4adb99e 100644 > --- a/drivers/clk/clkdev.c > +++ b/drivers/clk/clkdev.c > @@ -54,30 +54,29 @@ 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); > + struct device_node *child = np; > + int index = 0; > > /* Walk up the tree of devices looking for a clock that matches */ > while (np) { > - int index = 0; > > /* > * For named clocks, first look up the name in the > * "clock-names" property. If it cannot be found, then > - * index will be an error code, and of_clk_get() will fail. > + * index will be an error code. > */ > if (name) > index = of_property_match_string(np, "clock-names", name); > - clk = __of_clk_get(np, index, dev_id, name); > - if (!IS_ERR(clk)) { > - break; > - } else if (name && index >= 0) { > - if (PTR_ERR(clk) != -EPROBE_DEFER) > - pr_err("ERROR: could not get clock %pOF:%s(%i)\n", > - np, name ? name : "", index); > + if (index >= 0) > + clk = __of_clk_get(np, index, dev_id, name); > + if (!IS_ERR(clk)) Was this change necessary? It looks like we can leave it all alone and keep passing a negative number to __of_clk_get() and have that return an error pointer which we then return immediately as an error. But, if the clock is optional and we've passed a name here, shouldn't we treat an error from of_property_match_string() as success too? This is all looking pretty fragile so maybe it can be better commented and also more explicit instead of relying on the reader to jump through all the function calls to figure out what the return value is in some cases. > return clk; > - } > + if (name && index >= 0) > + break; And this causes us to duplicate logic down below because we have to check it again if it's not optional or some other error condition? > > /* > * No matching clock found on this node. If the parent node > @@ -89,6 +88,16 @@ static struct clk *__of_clk_get_by_name(struct device_node *np, > break; > } > > + /* The clock is not valid, but it could be optional or deferred */ > + if (optional && PTR_ERR(clk) == -ENOENT) { > + clk = NULL; > + pr_info("no optional clock %pOF:%s\n", child, > + name ? name : ""); Is this intentionally pr_info? > + } else if (name && index >= 0 && PTR_ERR(clk) != -EPROBE_DEFER) { > + pr_err("ERROR: could not get clock %pOF:%s(%i)\n", > + child, name, index); > + } > + > return clk; > } >
Hi Stephen, On 01 September 2018 03:46, Stephen Boyd wrote: > Quoting Phil Edworthy (2018-08-31 07:07:22) > > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index > > 9ab3db8..4adb99e 100644 > > --- a/drivers/clk/clkdev.c > > +++ b/drivers/clk/clkdev.c > > @@ -54,30 +54,29 @@ 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); > > + struct device_node *child = np; > > + int index = 0; > > > > /* Walk up the tree of devices looking for a clock that matches */ > > while (np) { > > - int index = 0; > > > > /* > > * For named clocks, first look up the name in the > > * "clock-names" property. If it cannot be found, then > > - * index will be an error code, and of_clk_get() will fail. > > + * index will be an error code. > > */ > > if (name) > > index = of_property_match_string(np, "clock-names", name); > > - clk = __of_clk_get(np, index, dev_id, name); > > - if (!IS_ERR(clk)) { > > - break; > > - } else if (name && index >= 0) { > > - if (PTR_ERR(clk) != -EPROBE_DEFER) > > - pr_err("ERROR: could not get clock %pOF:%s(%i)\n", > > - np, name ? name : "", index); > > + if (index >= 0) > > + clk = __of_clk_get(np, index, dev_id, name); > > + if (!IS_ERR(clk)) > > Was this change necessary? It looks like we can leave it all alone and keep > passing a negative number to __of_clk_get() and have that return an error > pointer which we then return immediately as an error. But, if the clock is > optional and we've passed a name here, shouldn't we treat an error from > of_property_match_string() as success too? This is all looking pretty fragile so > maybe it can be better commented and also more explicit instead of relying > on the reader to jump through all the function calls to figure out what the > return value is in some cases. If we call __of_clk_get, with index < 0, we will not be able to differentiate between clock provider not present and other errors with the passed data, as it will just return -EINVAL. of_property_match_string() will return -EINVAL if the "clock-names" property is missing, or -ENODATA if the specified clock name in the "clock-names" property is missing. That is why I have changed the code to conditionally call __of_clk_get, so the code will correctly treat optional clocks that are not present. > > return clk; > > - } > > + if (name && index >= 0) > > + break; > > And this causes us to duplicate logic down below because we have to check it > again if it's not optional or some other error condition? Yes, the error handling is messy, though I have tried to make this simple. I'll have a think about some other way to make this cleaner. > > > > /* > > * No matching clock found on this node. If the > > parent node @@ -89,6 +88,16 @@ static struct clk > *__of_clk_get_by_name(struct device_node *np, > > break; > > } > > > > + /* The clock is not valid, but it could be optional or deferred */ > > + if (optional && PTR_ERR(clk) == -ENOENT) { > > + clk = NULL; > > + pr_info("no optional clock %pOF:%s\n", child, > > + name ? name : ""); > > Is this intentionally pr_info? Yes, it's not an error if an optional clock isn’t there. Would pr_debug be more appropriate? > > + } else if (name && index >= 0 && PTR_ERR(clk) != -EPROBE_DEFER) { > > + pr_err("ERROR: could not get clock %pOF:%s(%i)\n", > > + child, name, index); > > + } > > + > > return clk; > > } > > Thanks Phil
Hi Stephen, On 03 September 2018 10:33 Phil Edworthy wrote: > On 01 September 2018 03:46, Stephen Boyd wrote: > > Quoting Phil Edworthy (2018-08-31 07:07:22) > > > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index > > > 9ab3db8..4adb99e 100644 > > > --- a/drivers/clk/clkdev.c > > > +++ b/drivers/clk/clkdev.c > > > @@ -54,30 +54,29 @@ 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); > > > + struct device_node *child = np; > > > + int index = 0; > > > > > > /* Walk up the tree of devices looking for a clock that matches */ > > > while (np) { > > > - int index = 0; > > > > > > /* > > > * For named clocks, first look up the name in the > > > * "clock-names" property. If it cannot be found, then > > > - * index will be an error code, and of_clk_get() will fail. > > > + * index will be an error code. > > > */ > > > if (name) > > > index = of_property_match_string(np, "clock-names", > name); > > > - clk = __of_clk_get(np, index, dev_id, name); > > > - if (!IS_ERR(clk)) { > > > - break; > > > - } else if (name && index >= 0) { > > > - if (PTR_ERR(clk) != -EPROBE_DEFER) > > > - pr_err("ERROR: could not get clock %pOF:%s(%i)\n", > > > - np, name ? name : "", index); > > > + if (index >= 0) > > > + clk = __of_clk_get(np, index, dev_id, name); > > > + if (!IS_ERR(clk)) > > > > Was this change necessary? It looks like we can leave it all alone and keep > > passing a negative number to __of_clk_get() and have that return an error > > pointer which we then return immediately as an error. But, if the clock is > > optional and we've passed a name here, shouldn't we treat an error from > > of_property_match_string() as success too? This is all looking pretty fragile > so > > maybe it can be better commented and also more explicit instead of relying > > on the reader to jump through all the function calls to figure out what the > > return value is in some cases. > If we call __of_clk_get, with index < 0, we will not be able to differentiate > between clock provider not present and other errors with the passed data, > as it will just return -EINVAL. > > of_property_match_string() will return -EINVAL if the "clock-names" > property > is missing, or -ENODATA if the specified clock name in the "clock-names" > property is missing. That is why I have changed the code to conditionally > call __of_clk_get, so the code will correctly treat optional clocks that are not > present. When getting named optional clocks, if the node has a "clock-names" property, but no clock matching the name we want, I think the function should stop there and *not* walk up the tree of devices looking for a matching clock. In this case, the code determines that the optional clock is not present. If there isn’t a "clock-names" property in the current node, the function should walk up the tree of devices looking for a matching optional clock. If there are no parent nodes left and we haven't found a matching optional clock, we determine that the clock isn’t there. Is that how this should work? Thanks Phil > > > return clk; > > > - } > > > + if (name && index >= 0) > > > + break; > > > > And this causes us to duplicate logic down below because we have to check > it > > again if it's not optional or some other error condition? > Yes, the error handling is messy, though I have tried to make this simple. > I'll have a think about some other way to make this cleaner. > > > > > > > > /* > > > * No matching clock found on this node. If the > > > parent node @@ -89,6 +88,16 @@ static struct clk > > *__of_clk_get_by_name(struct device_node *np, > > > break; > > > } > > > > > > + /* The clock is not valid, but it could be optional or deferred */ > > > + if (optional && PTR_ERR(clk) == -ENOENT) { > > > + clk = NULL; > > > + pr_info("no optional clock %pOF:%s\n", child, > > > + name ? name : ""); > > > > Is this intentionally pr_info? > Yes, it's not an error if an optional clock isn’t there. > Would pr_debug be more appropriate? > > > > > + } else if (name && index >= 0 && PTR_ERR(clk) != -EPROBE_DEFER) { > > > + pr_err("ERROR: could not get clock %pOF:%s(%i)\n", > > > + child, name, index); > > > + } > > > + > > > return clk; > > > } > > > > > Thanks > Phil
Quoting Phil Edworthy (2018-09-03 06:21:02) > Hi Stephen, > > On 03 September 2018 10:33 Phil Edworthy wrote: > > On 01 September 2018 03:46, Stephen Boyd wrote: > > > Quoting Phil Edworthy (2018-08-31 07:07:22) > > > > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index > > > > 9ab3db8..4adb99e 100644 > > > > --- a/drivers/clk/clkdev.c > > > > +++ b/drivers/clk/clkdev.c > > > > @@ -54,30 +54,29 @@ 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); > > > > + struct device_node *child = np; > > > > + int index = 0; > > > > > > > > /* Walk up the tree of devices looking for a clock that matches */ > > > > while (np) { > > > > - int index = 0; > > > > > > > > /* > > > > * For named clocks, first look up the name in the > > > > * "clock-names" property. If it cannot be found, then > > > > - * index will be an error code, and of_clk_get() will fail. > > > > + * index will be an error code. > > > > */ > > > > if (name) > > > > index = of_property_match_string(np, "clock-names", > > name); > > > > - clk = __of_clk_get(np, index, dev_id, name); > > > > - if (!IS_ERR(clk)) { > > > > - break; > > > > - } else if (name && index >= 0) { > > > > - if (PTR_ERR(clk) != -EPROBE_DEFER) > > > > - pr_err("ERROR: could not get clock %pOF:%s(%i)\n", > > > > - np, name ? name : "", index); > > > > + if (index >= 0) > > > > + clk = __of_clk_get(np, index, dev_id, name); > > > > + if (!IS_ERR(clk)) > > > > > > Was this change necessary? It looks like we can leave it all alone and keep > > > passing a negative number to __of_clk_get() and have that return an error > > > pointer which we then return immediately as an error. But, if the clock is > > > optional and we've passed a name here, shouldn't we treat an error from > > > of_property_match_string() as success too? This is all looking pretty fragile > > so > > > maybe it can be better commented and also more explicit instead of relying > > > on the reader to jump through all the function calls to figure out what the > > > return value is in some cases. > > If we call __of_clk_get, with index < 0, we will not be able to differentiate > > between clock provider not present and other errors with the passed data, > > as it will just return -EINVAL. > > > > of_property_match_string() will return -EINVAL if the "clock-names" > > property > > is missing, or -ENODATA if the specified clock name in the "clock-names" > > property is missing. That is why I have changed the code to conditionally > > call __of_clk_get, so the code will correctly treat optional clocks that are not > > present. > When getting named optional clocks, if the node has a "clock-names" property, > but no clock matching the name we want, I think the function should stop there > and *not* walk up the tree of devices looking for a matching clock. In this case, > the code determines that the optional clock is not present. > > If there isn’t a "clock-names" property in the current node, the function should > walk up the tree of devices looking for a matching optional clock. If there are no > parent nodes left and we haven't found a matching optional clock, we determine > that the clock isn’t there. > > Is that how this should work? > Yes that sounds right.
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index 9ab3db8..4adb99e 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -54,30 +54,29 @@ 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); + struct device_node *child = np; + int index = 0; /* Walk up the tree of devices looking for a clock that matches */ while (np) { - int index = 0; /* * For named clocks, first look up the name in the * "clock-names" property. If it cannot be found, then - * index will be an error code, and of_clk_get() will fail. + * index will be an error code. */ if (name) index = of_property_match_string(np, "clock-names", name); - clk = __of_clk_get(np, index, dev_id, name); - if (!IS_ERR(clk)) { - break; - } else if (name && index >= 0) { - if (PTR_ERR(clk) != -EPROBE_DEFER) - pr_err("ERROR: could not get clock %pOF:%s(%i)\n", - np, name ? name : "", index); + if (index >= 0) + clk = __of_clk_get(np, index, dev_id, name); + if (!IS_ERR(clk)) return clk; - } + if (name && index >= 0) + break; /* * No matching clock found on this node. If the parent node @@ -89,6 +88,16 @@ static struct clk *__of_clk_get_by_name(struct device_node *np, break; } + /* The clock is not valid, but it could be optional or deferred */ + if (optional && PTR_ERR(clk) == -ENOENT) { + clk = NULL; + pr_info("no optional clock %pOF:%s\n", child, + name ? name : ""); + } else if (name && index >= 0 && PTR_ERR(clk) != -EPROBE_DEFER) { + pr_err("ERROR: could not get clock %pOF:%s(%i)\n", + child, name, index); + } + return clk; } @@ -106,15 +115,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 +228,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);
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. Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> --- v5: - Simplified the code by handling all the error conditions on exit v4: - For optional named clocks of_property_match_string() will return -EINVAL if the "clock-names" property is missing, or -ENODATA if the specified clock name in the "clock-names" property is missing. If we then call __of_clk_get() with these errors as the index, we get clk = -EINVAL. This is then filtered later on so users don't see it. However, if the clock is not named, __of_clk_get() will return -ENOENT is the clock provide is not there. So for optional named clocks, use index to determine if the clock provider is there or not, and for unnamed clocks, simply check if __of_clk_get() returns -ENOENT. 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 | 59 +++++++++++++++++++++++++++++++++++++++------------- include/linux/clk.h | 6 ++++++ 2 files changed, 51 insertions(+), 14 deletions(-)