| Message ID | 20181120141445.21378-1-phil.edworthy@renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v8] clk: Add (devm_)clk_get_optional() functions | expand |
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. > > 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. > + * 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? > +{ > + struct clk *clk = clk_get(dev, id); > + > + if (clk == ERR_PTR(-ENOENT)) > + clk = NULL; > + > + return clk; > +} > +
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; > > +} > > +
On Fri, Nov 30, 2018 at 10:25:37AM +0000, Phil Edworthy wrote: > 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. Each time the question of the optional clk_get() stuff comes up, we go around the same discussions time and time again. So far, each time has ended up flopping. Yes, clk_get() can only ever return -ENOENT if it falls back to the non-DT methods, because it assumes that the clk tables are complete (it can do nothing else.) I don't think it needs a comment because it's obvious from the code and also from the implementation point of view. > > > +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. I don't see you need to - this is an internal function by way of the "static inline" you have before it. It's not an API function.
Quoting Russell King - ARM Linux (2018-11-30 03:04:46) > On Fri, Nov 30, 2018 at 10:25:37AM +0000, Phil Edworthy wrote: > > 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. > > Each time the question of the optional clk_get() stuff comes up, we go > around the same discussions time and time again. So far, each time > has ended up flopping. > > Yes, clk_get() can only ever return -ENOENT if it falls back to the > non-DT methods, because it assumes that the clk tables are complete > (it can do nothing else.) > > I don't think it needs a comment because it's obvious from the code > and also from the implementation point of view. Ok. I would still suggest a comment on of_clk_get_by_name() that indicates what types of return values happen in there. Otherwise it's obvious from the code after reading 3 or 4 functions deep (__of_clk_get_by_name -> __of_clk_get -> of_parse_phandle_with_args and __of_clk_get_from_provider) that this is what happens. > > > > > +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. > > I don't see you need to - this is an internal function by way of the > "static inline" you have before it. It's not an API function. It's static inline in a header file. That is an API function as far as I can tell.
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index 12c87457eca1..c6f4a1a92c5f 100644 --- a/drivers/clk/clk-devres.c +++ b/drivers/clk/clk-devres.c @@ -34,6 +34,17 @@ struct clk *devm_clk_get(struct device *dev, const char *id) } EXPORT_SYMBOL(devm_clk_get); +struct clk *devm_clk_get_optional(struct device *dev, const char *id) +{ + struct clk *clk = devm_clk_get(dev, id); + + if (clk == ERR_PTR(-ENOENT)) + clk = NULL; + + return clk; +} +EXPORT_SYMBOL(devm_clk_get_optional); + struct clk_bulk_devres { struct clk_bulk_data *clks; int num_clks; 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 + * 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) +{ + struct clk *clk = clk_get(dev, id); + + if (clk == ERR_PTR(-ENOENT)) + clk = NULL; + + return clk; +} + #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);