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

[PATCH v3 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     | 51 +++++++++++++++++++++++++++++++++++++++++++-----
 include/linux/clk.h      | 35 +++++++++++++++++++++++++++++++++
 3 files changed, 97 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH v3 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     | 51 +++++++++++++++++++++++++++++++++++++++++++-----
 include/linux/clk.h      | 35 +++++++++++++++++++++++++++++++++
 3 files changed, 97 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH v3 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 -EINVAL.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
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 | 36 ++++++++++++++++++++++++++++++++----
 include/linux/clk.h  |  6 ++++++
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 9ab3db8..a710333 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -54,7 +54,8 @@ 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);
 
@@ -70,6 +71,11 @@ static struct clk *__of_clk_get_by_name(struct device_node *np,
 		if (name)
 			index = of_property_match_string(np, "clock-names", name);
 		clk = __of_clk_get(np, index, dev_id, name);
+		if (optional && PTR_ERR(clk) == -EINVAL) {
+			clk = NULL;
+			pr_info("optional clock is not present %pOF:%s\n", np,
+				name ? name : "");
+		}
 		if (!IS_ERR(clk)) {
 			break;
 		} else if (name && index >= 0) {
@@ -106,15 +112,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 +225,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 v3 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 -EINVAL.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
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 | 36 ++++++++++++++++++++++++++++++++----
 include/linux/clk.h  |  6 ++++++
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 9ab3db8..a710333 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -54,7 +54,8 @@ 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);
 
@@ -70,6 +71,11 @@ static struct clk *__of_clk_get_by_name(struct device_node *np,
 		if (name)
 			index = of_property_match_string(np, "clock-names", name);
 		clk = __of_clk_get(np, index, dev_id, name);
+		if (optional && PTR_ERR(clk) == -EINVAL) {
+			clk = NULL;
+			pr_info("optional clock is not present %pOF:%s\n", np,
+				name ? name : "");
+		}
 		if (!IS_ERR(clk)) {
 			break;
 		} else if (name && index >= 0) {
@@ -106,15 +112,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 +225,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 v3 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>
---
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 a710333..2245ba6 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -219,21 +219,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

[PATCH v3 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>
---
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 a710333..2245ba6 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -219,21 +219,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 v3 1/2] clk: Add of_clk_get_by_name_optional() function

[Andy Shevchenko]

On Mon, 2018-07-30 at 14:31 +0100, Phil Edworthy wrote:
> 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 -EINVAL.

I'm puzzled a bit.

__of_clk_get() may return few error codes, and to me ENOENT sounds
correct when clock is not found. Other error codes should be passed to
the caller even for optional clocks.

If above is not true, we need to understand what circumstances for each
possible returned code are, and fix / act accordingly.

P.S. Possible way like regulator framework does is to return -ENODEV.

So, basically what I'm asking here is to be sure that single error code
(for now supposed -EINVAL) in this case is _the_ error code for absent /
can't be found clock.

>  - 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.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Andy Shevchenko]

On Mon, 2018-07-30 at 14:31 +0100, Phil Edworthy wrote:
> 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 -EINVAL.

I'm puzzled a bit.

__of_clk_get() may return few error codes, and to me ENOENT sounds
correct when clock is not found. Other error codes should be passed to
the caller even for optional clocks.

If above is not true, we need to understand what circumstances for each
possible returned code are, and fix / act accordingly.

P.S. Possible way like regulator framework does is to return -ENODEV.

So, basically what I'm asking here is to be sure that single error code
(for now supposed -EINVAL) in this case is _the_ error code for absent /
can't be found clock.

>  - 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.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Andy Shevchenko]

On Mon, 2018-07-30 at 14:31 +0100, Phil Edworthy wrote:
> 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 -EINVAL.

I'm puzzled a bit.

__of_clk_get() may return few error codes, and to me ENOENT sounds
correct when clock is not found. Other error codes should be passed to
the caller even for optional clocks.

If above is not true, we need to understand what circumstances for each
possible returned code are, and fix / act accordingly.

P.S. Possible way like regulator framework does is to return -ENODEV.

So, basically what I'm asking here is to be sure that single error code
(for now supposed -EINVAL) in this case is _the_ error code for absent /
can't be found clock.

>  - 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.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

[Phil Edworthy]

Hi Andy,

On 30 July 2018 17:04, Andy Shevchenko wrote:
> On Mon, 2018-07-30 at 14:31 +0100, Phil Edworthy wrote:
> > 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 -EINVAL.
> 
> I'm puzzled a bit.
> 
> __of_clk_get() may return few error codes, and to me ENOENT sounds
> correct when clock is not found. Other error codes should be passed to the
> caller even for optional clocks.
> 
> If above is not true, we need to understand what circumstances for each
> possible returned code are, and fix / act accordingly.
>
> P.S. Possible way like regulator framework does is to return -ENODEV.
> 
> So, basically what I'm asking here is to be sure that single error code (for now
> supposed -EINVAL) in this case is _the_ error code for absent / can't be
> found clock.
of_property_match_string() can return different return values for when:
 the "clock-names" property is missing (-EINVAL),
 the specified clock name in the "clock-names" property is missing  (-ENODATA),
 or internal errors, (e.g. -EILSEQ).
However, if you then call __of_clk_get() with the failed index, you just get
the -EINVAL error.

Looking at it further, __of_clk_get() should return -ENOENT if passed a valid
index (e.g. 0 when name=NULL) and the clock is not there. However, I think
it will take a while to check all possible return values as it's not immediately
clear what they are (to me).

I think the best thing is to test the return value of of_property_match_string()
for -ENODATA, and deal with it then. Then deal with the case of name=NULL
separately.

Thanks
Phil

 >  - 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.
> 
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy

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

[Phil Edworthy]

SGkgQW5keSwNCg0KT24gMzAgSnVseSAyMDE4IDE3OjA0LCBBbmR5IFNoZXZjaGVua28gd3JvdGU6
DQo+IE9uIE1vbiwgMjAxOC0wNy0zMCBhdCAxNDozMSArMDEwMCwgUGhpbCBFZHdvcnRoeSB3cm90
ZToNCj4gPiBRdWl0ZSBhIGZldyBkcml2ZXJzIGdldCBhbiBvcHRpb25hbCBjbG9jaywgZS5nLiBh
IGNsb2NrIHJlcXVpcmVkIHRvDQo+ID4gYWNjZXNzIHBlcmlwaGVyYWwncyByZWdpc3RlcnMgdGhh
dCBpcyBhbHdheXMgZW5hYmxlZCBvbiBzb21lIGRldmljZXMuDQo+ID4NCj4gPiBUaGlzIGZ1bmN0
aW9uIGJlaGF2ZXMgdGhlIHNhbWUgYXMgb2ZfY2xrX2dldF9ieV9uYW1lKCkgZXhjZXB0IHRoYXQg
aXQNCj4gPiB3aWxsIHJldHVybiBOVUxMIGluc3RlYWQgb2YgLUVJTlZBTC4NCj4gDQo+IEknbSBw
dXp6bGVkIGEgYml0Lg0KPiANCj4gX19vZl9jbGtfZ2V0KCkgbWF5IHJldHVybiBmZXcgZXJyb3Ig
Y29kZXMsIGFuZCB0byBtZSBFTk9FTlQgc291bmRzDQo+IGNvcnJlY3Qgd2hlbiBjbG9jayBpcyBu
b3QgZm91bmQuIE90aGVyIGVycm9yIGNvZGVzIHNob3VsZCBiZSBwYXNzZWQgdG8gdGhlDQo+IGNh
bGxlciBldmVuIGZvciBvcHRpb25hbCBjbG9ja3MuDQo+IA0KPiBJZiBhYm92ZSBpcyBub3QgdHJ1
ZSwgd2UgbmVlZCB0byB1bmRlcnN0YW5kIHdoYXQgY2lyY3Vtc3RhbmNlcyBmb3IgZWFjaA0KPiBw
b3NzaWJsZSByZXR1cm5lZCBjb2RlIGFyZSwgYW5kIGZpeCAvIGFjdCBhY2NvcmRpbmdseS4NCj4N
Cj4gUC5TLiBQb3NzaWJsZSB3YXkgbGlrZSByZWd1bGF0b3IgZnJhbWV3b3JrIGRvZXMgaXMgdG8g
cmV0dXJuIC1FTk9ERVYuDQo+IA0KPiBTbywgYmFzaWNhbGx5IHdoYXQgSSdtIGFza2luZyBoZXJl
IGlzIHRvIGJlIHN1cmUgdGhhdCBzaW5nbGUgZXJyb3IgY29kZSAoZm9yIG5vdw0KPiBzdXBwb3Nl
ZCAtRUlOVkFMKSBpbiB0aGlzIGNhc2UgaXMgX3RoZV8gZXJyb3IgY29kZSBmb3IgYWJzZW50IC8g
Y2FuJ3QgYmUNCj4gZm91bmQgY2xvY2suDQpvZl9wcm9wZXJ0eV9tYXRjaF9zdHJpbmcoKSBjYW4g
cmV0dXJuIGRpZmZlcmVudCByZXR1cm4gdmFsdWVzIGZvciB3aGVuOg0KIHRoZSAiY2xvY2stbmFt
ZXMiIHByb3BlcnR5IGlzIG1pc3NpbmcgKC1FSU5WQUwpLA0KIHRoZSBzcGVjaWZpZWQgY2xvY2sg
bmFtZSBpbiB0aGUgImNsb2NrLW5hbWVzIiBwcm9wZXJ0eSBpcyBtaXNzaW5nICAoLUVOT0RBVEEp
LA0KIG9yIGludGVybmFsIGVycm9ycywgKGUuZy4gLUVJTFNFUSkuDQpIb3dldmVyLCBpZiB5b3Ug
dGhlbiBjYWxsIF9fb2ZfY2xrX2dldCgpIHdpdGggdGhlIGZhaWxlZCBpbmRleCwgeW91IGp1c3Qg
Z2V0DQp0aGUgLUVJTlZBTCBlcnJvci4NCg0KTG9va2luZyBhdCBpdCBmdXJ0aGVyLCBfX29mX2Ns
a19nZXQoKSBzaG91bGQgcmV0dXJuIC1FTk9FTlQgaWYgcGFzc2VkIGEgdmFsaWQNCmluZGV4IChl
LmcuIDAgd2hlbiBuYW1lPU5VTEwpIGFuZCB0aGUgY2xvY2sgaXMgbm90IHRoZXJlLiBIb3dldmVy
LCBJIHRoaW5rDQppdCB3aWxsIHRha2UgYSB3aGlsZSB0byBjaGVjayBhbGwgcG9zc2libGUgcmV0
dXJuIHZhbHVlcyBhcyBpdCdzIG5vdCBpbW1lZGlhdGVseQ0KY2xlYXIgd2hhdCB0aGV5IGFyZSAo
dG8gbWUpLg0KDQpJIHRoaW5rIHRoZSBiZXN0IHRoaW5nIGlzIHRvIHRlc3QgdGhlIHJldHVybiB2
YWx1ZSBvZiBvZl9wcm9wZXJ0eV9tYXRjaF9zdHJpbmcoKQ0KZm9yIC1FTk9EQVRBLCBhbmQgZGVh
bCB3aXRoIGl0IHRoZW4uIFRoZW4gZGVhbCB3aXRoIHRoZSBjYXNlIG9mIG5hbWU9TlVMTA0Kc2Vw
YXJhdGVseS4NCg0KVGhhbmtzDQpQaGlsDQoNCiA+ICAtIEZpeCBjaGVjayBmb3IgY2xvY2sgbm90
IHByZXNlbnQuIF9fb2ZfY2xrX2dldCgpIHJldHVybnMgLUVJTlZBTA0KPiA+ICAgIGlmIGl0J3Mg
bm90IHRoZXJlLiBDb3ZlciBjYXNlIG9mIHdoZW4gdGhlcmUgaXMgbm8gY2xvY2sgbmFtZS4NCj4g
DQo+IC0tDQo+IEFuZHkgU2hldmNoZW5rbyA8YW5kcml5LnNoZXZjaGVua29AbGludXguaW50ZWwu
Y29tPg0KPiBJbnRlbCBGaW5sYW5kIE95DQo=

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

[Phil Edworthy]

Hi Andy,

On 30 July 2018 17:04, Andy Shevchenko wrote:
> On Mon, 2018-07-30 at 14:31 +0100, Phil Edworthy wrote:
> > 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 -EINVAL.
> 
> I'm puzzled a bit.
> 
> __of_clk_get() may return few error codes, and to me ENOENT sounds
> correct when clock is not found. Other error codes should be passed to the
> caller even for optional clocks.
> 
> If above is not true, we need to understand what circumstances for each
> possible returned code are, and fix / act accordingly.
>
> P.S. Possible way like regulator framework does is to return -ENODEV.
> 
> So, basically what I'm asking here is to be sure that single error code (for now
> supposed -EINVAL) in this case is _the_ error code for absent / can't be
> found clock.
of_property_match_string() can return different return values for when:
 the "clock-names" property is missing (-EINVAL),
 the specified clock name in the "clock-names" property is missing  (-ENODATA),
 or internal errors, (e.g. -EILSEQ).
However, if you then call __of_clk_get() with the failed index, you just get
the -EINVAL error.

Looking at it further, __of_clk_get() should return -ENOENT if passed a valid
index (e.g. 0 when name=NULL) and the clock is not there. However, I think
it will take a while to check all possible return values as it's not immediately
clear what they are (to me).

I think the best thing is to test the return value of of_property_match_string()
for -ENODATA, and deal with it then. Then deal with the case of name=NULL
separately.

Thanks
Phil

 >  - 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.
> 
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy