Messages in this thread | | | From | Phil Edworthy <> | Subject | RE: [PATCH v8] clk: Add (devm_)clk_get_optional() functions | Date | Fri, 30 Nov 2018 10:25:37 +0000 |
| |
Hi Stephen,
On 30 November 2018 09:09 Stephen Boyd wrote: > Quoting Phil Edworthy (2018-11-20 06:14:45) > > This adds clk_get_optional() and devm_clk_get_optional() functions to > > get optional clocks. > > They behave 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. > > Ok. I guess that works by virtue of how -ENOENT is returned by various > functions that are called deeper in the clk_get() path? I'm cautiously > optimistic. So cautious, we should probably add a comment to these optional > functions that indicate they rely on the functions they call to return -ENOENT > under the various conditions that make a clk optional. Yes, it does indeed rely on how clk_get() is implemented. Specifically, that if __of_clk_get_by_name() returns -EINVAL, the error is superseded by clk_get_sys() returning -ENOENT. As you say, a comment may help here.
> > > > diff --git a/include/linux/clk.h b/include/linux/clk.h index > > a7773b5c0b9f..3ea3c78f62dd 100644 > > --- a/include/linux/clk.h > > +++ b/include/linux/clk.h > > @@ -383,6 +383,17 @@ int __must_check devm_clk_bulk_get_all(struct > device *dev, > > */ > > 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. > > + * @dev: device for clock "consumer" > > + * @id: clock consumer ID > > + * > > + * Behaves the same as devm_clk_get except where there is no clock > > +producer. In > > Please add () around devm_clk_get() so we know it's a function. Will do.
> > + * 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. > > @@ -718,6 +729,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) { @@ -862,6 +879,16 @@ static inline void > > clk_bulk_disable_unprepare(int num_clks, > > clk_bulk_unprepare(num_clks, clks); } > > > > +static inline struct clk *clk_get_optional(struct device *dev, const > > +char *id) > > Any kernel doc for this function? I took my cue from the surrounding functions, let me know if I have to add it.
Thanks Phil
> > +{ > > + struct clk *clk = clk_get(dev, id); > > + > > + if (clk == ERR_PTR(-ENOENT)) > > + clk = NULL; > > + > > + return clk; > > +} > > +
| |