[PATCH v5 0/2] clk: Add functions to get optional clocks

[PATCH v5 0/2] clk: Add functions to get optional clocks

[Phil Edworthy]

Quite a few drivers get an optional clock, e.g. a bus clock required to 
access peripheral's registers that is always enabled on some devices.

Phil Edworthy (2):
  clk: Add of_clk_get_by_name_optional() function
  clk: Add functions to get optional clocks

 drivers/clk/clk-devres.c | 18 ++++++++++--
 drivers/clk/clkdev.c     | 74 ++++++++++++++++++++++++++++++++++++++----------
 include/linux/clk.h      | 35 +++++++++++++++++++++++
 3 files changed, 110 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function

[Phil Edworthy]

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

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

[PATCH v5 2/2] clk: Add functions to get optional clocks

[Phil Edworthy]

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. 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 <phil.edworthy@renesas.com>
---
v5:
 - No changes.
v4:
 - No changes.
v3:
 - No changes.
---
 drivers/clk/clk-devres.c | 18 ++++++++++++++++--
 drivers/clk/clkdev.c     | 17 +++++++++++++++--
 include/linux/clk.h      | 29 +++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index d854e26..a2bb01a 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -14,7 +14,7 @@ static void devm_clk_release(struct device *dev, void *res)
 	clk_put(*(struct clk **)res);
 }
 
-struct clk *devm_clk_get(struct device *dev, const char *id)
+static struct clk *__devm_clk_get(struct device *dev, const char *id, bool optional)
 {
 	struct clk **ptr, *clk;
 
@@ -22,7 +22,10 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
 	if (!ptr)
 		return ERR_PTR(-ENOMEM);
 
-	clk = clk_get(dev, id);
+	if (!optional)
+		clk = clk_get(dev, id);
+	else
+		clk = clk_get_optional(dev, id);
 	if (!IS_ERR(clk)) {
 		*ptr = clk;
 		devres_add(dev, ptr);
@@ -32,8 +35,19 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
 
 	return clk;
 }
+
+struct clk *devm_clk_get(struct device *dev, const char *id)
+{
+	return __devm_clk_get(dev, id, false);
+}
 EXPORT_SYMBOL(devm_clk_get);
 
+struct clk *devm_clk_get_optional(struct device *dev, const char *id)
+{
+	return __devm_clk_get(dev, id, true);
+}
+EXPORT_SYMBOL(devm_clk_get_optional);
+
 struct clk_bulk_devres {
 	struct clk_bulk_data *clks;
 	int num_clks;
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 4adb99e..6355573 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -222,21 +222,34 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id)
 }
 EXPORT_SYMBOL(clk_get_sys);
 
-struct clk *clk_get(struct device *dev, const char *con_id)
+static struct clk *internal_clk_get(struct device *dev, const char *con_id,
+				    bool optional)
 {
 	const char *dev_id = dev ? dev_name(dev) : NULL;
 	struct clk *clk;
 
 	if (dev && dev->of_node) {
-		clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id, false);
+		clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id,
+					   optional);
 		if (!IS_ERR(clk) || PTR_ERR(clk) == -EPROBE_DEFER)
 			return clk;
 	}
 
 	return clk_get_sys(dev_id, con_id);
 }
+
+struct clk *clk_get(struct device *dev, const char *con_id)
+{
+	return internal_clk_get(dev, con_id, false);
+}
 EXPORT_SYMBOL(clk_get);
 
+struct clk *clk_get_optional(struct device *dev, const char *con_id)
+{
+	return internal_clk_get(dev, con_id, true);
+}
+EXPORT_SYMBOL(clk_get_optional);
+
 void clk_put(struct clk *clk)
 {
 	__clk_put(clk);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index de0e5e0..58ec7bc 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -291,6 +291,16 @@ static inline void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)
 struct clk *clk_get(struct device *dev, const char *id);
 
 /**
+ * clk_get_optional - lookup and obtain a reference to optional clock producer.
+ * @dev: device for clock "consumer"
+ * @id: clock consumer ID
+ *
+ * Behaves the same as clk_get except where there is no clock producer. In this
+ * case, instead of returning -ENOENT, the function returns NULL.
+ */
+struct clk *clk_get_optional(struct device *dev, const char *id);
+
+/**
  * clk_bulk_get - lookup and obtain a number of references to clock producer.
  * @dev: device for clock "consumer"
  * @num_clks: the number of clk_bulk_data
@@ -349,6 +359,14 @@ int __must_check devm_clk_bulk_get(struct device *dev, int num_clks,
 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.
+ * 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.
  * @dev: device for clock "consumer"
@@ -636,6 +654,11 @@ static inline struct clk *clk_get(struct device *dev, const char *id)
 	return NULL;
 }
 
+static inline struct clk *clk_get_optional(struct device *dev, const char *id)
+{
+	return NULL;
+}
+
 static inline int __must_check clk_bulk_get(struct device *dev, int num_clks,
 					    struct clk_bulk_data *clks)
 {
@@ -647,6 +670,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)
 {
-- 
2.7.4

Re: [PATCH v5 0/2] clk: Add functions to get optional clocks

[Andy Shevchenko]

On Fri, Aug 31, 2018 at 03:07:21PM +0100, Phil Edworthy wrote:
> Quite a few drivers get an optional clock, e.g. a bus clock required to 
> access peripheral's registers that is always enabled on some devices.
> 

Looks good to me

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Perhaps, you may convert 8250_dw to this one later on as an example...

> Phil Edworthy (2):
>   clk: Add of_clk_get_by_name_optional() function
>   clk: Add functions to get optional clocks
> 
>  drivers/clk/clk-devres.c | 18 ++++++++++--
>  drivers/clk/clkdev.c     | 74 ++++++++++++++++++++++++++++++++++++++----------
>  include/linux/clk.h      | 35 +++++++++++++++++++++++
>  3 files changed, 110 insertions(+), 17 deletions(-)
> 
> -- 
> 2.7.4
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function

[Stephen Boyd]

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

Re: [PATCH v5 2/2] clk: Add functions to get optional clocks

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

Hi Phil,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on clk/clk-next]
[also build test ERROR on v4.19-rc1 next-20180831]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Phil-Edworthy/clk-Add-functions-to-get-optional-clocks/20180901-154009
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: m68k-m5475evb_defconfig (attached as .config)
compiler: m68k-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=m68k 

All errors (new ones prefixed by >>):

   drivers/clk/clk-devres.o: In function `__devm_clk_get':
>> clk-devres.c:(.text+0xb6): undefined reference to `clk_get_optional'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6371 bytes --]

Re: [PATCH v5 2/2] clk: Add functions to get optional clocks

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 12930 bytes --]

Hi Phil,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on clk/clk-next]
[also build test WARNING on v4.19-rc1 next-20180831]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Phil-Edworthy/clk-Add-functions-to-get-optional-clocks/20180901-154009
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org)
>> include/linux/clk.h:368: warning: Function parameter or member 'dev' not described in 'devm_clk_get_optional'
>> include/linux/clk.h:368: warning: Function parameter or member 'id' not described in 'devm_clk_get_optional'
   include/linux/srcu.h:175: warning: Function parameter or member 'p' not described in 'srcu_dereference_notrace'
   include/linux/srcu.h:175: warning: Function parameter or member 'sp' not described in 'srcu_dereference_notrace'
   include/linux/gfp.h:1: warning: no structured comments found
   include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.ibss' not described in 'wireless_dev'
   include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.connect' not described in 'wireless_dev'
   include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.keys' not described in 'wireless_dev'
   include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.ie' not described in 'wireless_dev'
   include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.ie_len' not described in 'wireless_dev'
   include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.bssid' not described in 'wireless_dev'
   include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.ssid' not described in 'wireless_dev'
   include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.default_key' not described in 'wireless_dev'
   include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.default_mgmt_key' not described in 'wireless_dev'
   include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.prev_bssid_valid' not described in 'wireless_dev'
   include/net/mac80211.h:2328: warning: Function parameter or member 'radiotap_timestamp.units_pos' not described in 'ieee80211_hw'
   include/net/mac80211.h:2328: warning: Function parameter or member 'radiotap_timestamp.accuracy' not described in 'ieee80211_hw'
   include/net/mac80211.h:977: warning: Function parameter or member 'control.rates' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'control.rts_cts_rate_idx' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'control.use_rts' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'control.use_cts_prot' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'control.short_preamble' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'control.skip_table' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'control.jiffies' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'control.vif' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'control.hw_key' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'control.flags' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'control.enqueue_time' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'ack' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'ack.cookie' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'status.rates' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'status.ack_signal' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'status.ampdu_ack_len' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'status.ampdu_len' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'status.antenna' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'status.tx_time' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'status.is_valid_ack_signal' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'status.status_driver_data' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'driver_rates' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'pad' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'rate_driver_data' not described in 'ieee80211_tx_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.signal' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.chain_signal' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.filtered' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_failed' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_count' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.lost_packets' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_tdls_pkt_time' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_retries' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_failed' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack_signal' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.ack_signal_filled' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.avg_ack_signal' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.packets' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.bytes' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.last_rate' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.msdu' not described in 'sta_info'
   include/linux/mod_devicetable.h:763: warning: Function parameter or member 'driver_data' not described in 'typec_device_id'
   kernel/sched/fair.c:3371: warning: Function parameter or member 'flags' not described in 'attach_entity_load_avg'
   arch/x86/include/asm/atomic.h:84: warning: Excess function parameter 'i' description in 'arch_atomic_sub_and_test'
   arch/x86/include/asm/atomic.h:84: warning: Excess function parameter 'v' description in 'arch_atomic_sub_and_test'
   arch/x86/include/asm/atomic.h:96: warning: Excess function parameter 'v' description in 'arch_atomic_inc'
   arch/x86/include/asm/atomic.h:109: warning: Excess function parameter 'v' description in 'arch_atomic_dec'
   arch/x86/include/asm/atomic.h:124: warning: Excess function parameter 'v' description in 'arch_atomic_dec_and_test'
   arch/x86/include/asm/atomic.h:138: warning: Excess function parameter 'v' description in 'arch_atomic_inc_and_test'
   arch/x86/include/asm/atomic.h:153: warning: Excess function parameter 'i' description in 'arch_atomic_add_negative'
   arch/x86/include/asm/atomic.h:153: warning: Excess function parameter 'v' description in 'arch_atomic_add_negative'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.cb' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.poll' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.active' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.cb' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.poll' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.active' not described in 'dma_buf'
   include/linux/dma-fence-array.h:54: warning: Function parameter or member 'work' not described in 'dma_fence_array'
   include/linux/gpio/driver.h:142: warning: Function parameter or member 'request_key' not described in 'gpio_irq_chip'
   include/linux/iio/hw-consumer.h:1: warning: no structured comments found
   include/linux/input/sparse-keymap.h:46: warning: Function parameter or member 'sw' not described in 'key_entry'
   drivers/pci/pci.c:218: warning: Excess function parameter 'p' description in 'pci_dev_str_match_path'
   include/linux/regulator/driver.h:227: warning: Function parameter or member 'resume' not described in 'regulator_ops'
   drivers/regulator/core.c:4479: warning: Excess function parameter 'state' description in 'regulator_suspend'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw0' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw1' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw2' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw3' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.eadm' not described in 'irb'
   drivers/slimbus/stream.c:1: warning: no structured comments found
   drivers/target/target_core_device.c:1: warning: no structured comments found
   drivers/usb/dwc3/gadget.c:510: warning: Excess function parameter 'dwc' description in 'dwc3_gadget_start_config'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'

vim +368 include/linux/clk.h

 > 368	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6587 bytes --]

RE: [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function

[Phil Edworthy]

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

RE: [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function

[Phil Edworthy]

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

RE: [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function

[Stephen Boyd]

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.