[v8] clk: Add (devm_)clk_get_optional() functions

Commit Message

Phil Edworthy Nov. 20, 2018, 2:14 p.m. UTC

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.

Signed-off-by: Phil Edworthy <[e-mail]>
Reviewed-by: Andy Shevchenko <[e-mail]>
---
v8:
 - Remove else clause
v7:
 - Instead of messing with the core functions, simply wrap them for the
   _optional() versions. By putting clk_get_optional() inline in the header
   file, we can get rid of the arch specific patches as well.
v6:
 - Add doxygen style comment for devm_clk_get_optional() args
v5:
 - No changes.
v4:
 - No changes.
v3:
 - No changes.
---
 drivers/clk/clk-devres.c | 11 +++++++++++
 include/linux/clk.h      | 27 +++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

Comments

Stephen Boyd Nov. 30, 2018, 9:09 a.m. UTC | #1

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;
> +}
> +

Phil Edworthy Nov. 30, 2018, 10:25 a.m. UTC | #2

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;
> > +}
> > +

Russell King (Oracle) Nov. 30, 2018, 11:04 a.m. UTC | #3

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.

Stephen Boyd Nov. 30, 2018, 6:55 p.m. UTC | #4

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.

Patch

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);