Messages in this thread | | | From | Phil Edworthy <> | Subject | RE: [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function | Date | Mon, 3 Sep 2018 13:21:02 +0000 |
| |
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
| |