[PATCH v2 0/2] Add functions to get optional clocks

[PATCH v2 0/2] Add functions to get optional clocks

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

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     | 49 +++++++++++++++++++++++++++++++++++++++++++-----
 include/linux/clk.h      | 30 +++++++++++++++++++++++++++++
 3 files changed, 90 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH v2 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. This makes error checking
for users easier 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>
---
 drivers/clk/clkdev.c | 34 ++++++++++++++++++++++++++++++----
 include/linux/clk.h  |  1 +
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 9ab3db8..c1b7262 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);
 
@@ -73,6 +74,8 @@ static struct clk *__of_clk_get_by_name(struct device_node *np,
 		if (!IS_ERR(clk)) {
 			break;
 		} else if (name && index >= 0) {
+			if (optional && PTR_ERR(clk) == -ENOENT)
+				clk = NULL;
 			if (PTR_ERR(clk) != -EPROBE_DEFER)
 				pr_err("ERROR: could not get clock %pOF:%s(%i)\n",
 					np, name ? name : "", index);
@@ -106,15 +109,38 @@ 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 no clock is found.
+ * In this case, instead of returning -ENOENT, it returns NULL.
+ */
+struct clk *of_clk_get_by_name_optional(struct device_node *np,
+					const char *name)
+{
+	if (!np)
+		return ERR_PTR(-ENOENT);
+
+	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 +223,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..8cb5455 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)
-- 
2.7.4

[PATCH v2 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>
---
 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 c1b7262..484a444 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -217,21 +217,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 8cb5455..22040ea 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 v2 1/2] clk: Add of_clk_get_by_name_optional() function

[Geert Uytterhoeven]

Hi Phil,

On Mon, Jul 30, 2018 at 10:36 AM Phil Edworthy
<phil.edworthy@renesas.com> 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 -ENOENT. This makes error checking
> for users easier 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>

Thanks for your patch!

> --- 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);
>
> @@ -73,6 +74,8 @@ static struct clk *__of_clk_get_by_name(struct device_node *np,
>                 if (!IS_ERR(clk)) {
>                         break;
>                 } else if (name && index >= 0) {
> +                       if (optional && PTR_ERR(clk) == -ENOENT)
> +                               clk = NULL;

This only handles the "name && index >= 0" case.
If that condition is never true, the loop will end, eventually, and the last
value of clk will be returned. Hence there should be a similar check at the
end of the function.

>                         if (PTR_ERR(clk) != -EPROBE_DEFER)
>                                 pr_err("ERROR: could not get clock %pOF:%s(%i)\n",
>                                         np, name ? name : "", index);

Hence if not found, this will always print an error, even if the clock is
optional?

> @@ -106,15 +109,38 @@ 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 no clock is found.
> + * In this case, instead of returning -ENOENT, it returns NULL.
> + */
> +struct clk *of_clk_get_by_name_optional(struct device_node *np,
> +                                       const char *name)
> +{
> +       if (!np)
> +               return ERR_PTR(-ENOENT);

Shouldn't this return NULL?
Or let __of_clk_get_by_name() handle that (cfr. above)?

Hmm, of_clk_get_by_name() has a similar check, while the current
__of_clk_get_by_name() already handle np == NULL, too.

> +
> +       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 +223,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..8cb5455 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)

No dummy version of of_clk_get_by_name_optional() for the !CLK || !COMMON_CLK
case?

It seems of_clk_get_by_name() and of_clk_get_by_name_optional() can just
be simple wrappers around __of_clk_get_by_name(), differing only in the
value of the "optional" parameter. Hence I think it makes sense to move them
to the header file, and make them static inline.
Then only __of_clk_get_by_name() needs a (static inline) dummy for the !OF ||
!COMMON_CLK case.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Phil Edworthy]

SGkgR2VlcnQsDQoNCk9uIDMwIEp1bHkgMjAxOCAwOTo1NiwgR2VlcnQgVXl0dGVyaG9ldmVuIHdy
b3RlOg0KPiBPbiBNb24sIEp1bCAzMCwgMjAxOCBhdCAxMDozNiBBTSBQaGlsIEVkd29ydGh5IHdy
b3RlOg0KPiA+IFF1aXRlIGEgZmV3IGRyaXZlcnMgZ2V0IGFuIG9wdGlvbmFsIGNsb2NrLCBlLmcu
IGEgY2xvY2sgcmVxdWlyZWQgdG8NCj4gPiBhY2Nlc3MgcGVyaXBoZXJhbCdzIHJlZ2lzdGVycyB0
aGF0IGlzIGFsd2F5cyBlbmFibGVkIG9uIHNvbWUgZGV2aWNlcy4NCj4gPg0KPiA+IFRoaXMgZnVu
Y3Rpb24gYmVoYXZlcyB0aGUgc2FtZSBhcyBvZl9jbGtfZ2V0X2J5X25hbWUoKSBleGNlcHQgdGhh
dCBpdA0KPiA+IHdpbGwgcmV0dXJuIE5VTEwgaW5zdGVhZCBvZiAtRU5PRU5ULiBUaGlzIG1ha2Vz
IGVycm9yIGNoZWNraW5nIGZvcg0KPiA+IHVzZXJzIGVhc2llciBhbmQgYWxsb3dzIGNsa19wcmVw
YXJlX2VuYWJsZSwgZXRjIHRvIGJlIGNhbGxlZCBvbiB0aGUNCj4gPiByZXR1cm5lZCByZWZlcmVu
Y2Ugd2l0aG91dCBhZGRpdGlvbmFsIGNoZWNrcy4NCj4gPg0KPiA+IFNpZ25lZC1vZmYtYnk6IFBo
aWwgRWR3b3J0aHkgPHBoaWwuZWR3b3J0aHlAcmVuZXNhcy5jb20+DQo+IA0KPiBUaGFua3MgZm9y
IHlvdXIgcGF0Y2ghDQo+IA0KPiA+IC0tLSBhL2RyaXZlcnMvY2xrL2Nsa2Rldi5jDQo+ID4gKysr
IGIvZHJpdmVycy9jbGsvY2xrZGV2LmMNCj4gPiBAQCAtNTQsNyArNTQsOCBAQCBFWFBPUlRfU1lN
Qk9MKG9mX2Nsa19nZXQpOw0KPiA+DQo+ID4gIHN0YXRpYyBzdHJ1Y3QgY2xrICpfX29mX2Nsa19n
ZXRfYnlfbmFtZShzdHJ1Y3QgZGV2aWNlX25vZGUgKm5wLA0KPiA+ICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICBjb25zdCBjaGFyICpkZXZfaWQsDQo+ID4gLSAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGNvbnN0IGNoYXIgKm5hbWUpDQo+ID4gKyAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGNvbnN0IGNoYXIgKm5hbWUsDQo+
ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGJvb2wgb3B0aW9uYWwp
DQo+ID4gIHsNCj4gPiAgICAgICAgIHN0cnVjdCBjbGsgKmNsayA9IEVSUl9QVFIoLUVOT0VOVCk7
DQo+ID4NCj4gPiBAQCAtNzMsNiArNzQsOCBAQCBzdGF0aWMgc3RydWN0IGNsayAqX19vZl9jbGtf
Z2V0X2J5X25hbWUoc3RydWN0DQo+IGRldmljZV9ub2RlICpucCwNCj4gPiAgICAgICAgICAgICAg
ICAgaWYgKCFJU19FUlIoY2xrKSkgew0KPiA+ICAgICAgICAgICAgICAgICAgICAgICAgIGJyZWFr
Ow0KPiA+ICAgICAgICAgICAgICAgICB9IGVsc2UgaWYgKG5hbWUgJiYgaW5kZXggPj0gMCkgew0K
PiA+ICsgICAgICAgICAgICAgICAgICAgICAgIGlmIChvcHRpb25hbCAmJiBQVFJfRVJSKGNsaykg
PT0gLUVOT0VOVCkNCj4gPiArICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGNsayA9IE5V
TEw7DQo+IA0KPiBUaGlzIG9ubHkgaGFuZGxlcyB0aGUgIm5hbWUgJiYgaW5kZXggPj0gMCIgY2Fz
ZS4NCj4gSWYgdGhhdCBjb25kaXRpb24gaXMgbmV2ZXIgdHJ1ZSwgdGhlIGxvb3Agd2lsbCBlbmQs
IGV2ZW50dWFsbHksIGFuZCB0aGUgbGFzdCB2YWx1ZQ0KPiBvZiBjbGsgd2lsbCBiZSByZXR1cm5l
ZC4gSGVuY2UgdGhlcmUgc2hvdWxkIGJlIGEgc2ltaWxhciBjaGVjayBhdCB0aGUgZW5kIG9mDQo+
IHRoZSBmdW5jdGlvbi4NCk9vcHMsIEkgd2FzIG9ubHkgY29uc2lkZXJpbmcgbmFtZWQgb3B0aW9u
YWwgY2xvY2tzLCBJJ2xsIGZpeCB0aGlzLg0KDQo+ID4gICAgICAgICAgICAgICAgICAgICAgICAg
aWYgKFBUUl9FUlIoY2xrKSAhPSAtRVBST0JFX0RFRkVSKQ0KPiA+ICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgcHJfZXJyKCJFUlJPUjogY291bGQgbm90IGdldCBjbG9jayAlcE9GOiVz
KCVpKVxuIiwNCj4gPiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgbnAs
IG5hbWUgPyBuYW1lIDogIiIsIGluZGV4KTsNCj4gDQo+IEhlbmNlIGlmIG5vdCBmb3VuZCwgdGhp
cyB3aWxsIGFsd2F5cyBwcmludCBhbiBlcnJvciwgZXZlbiBpZiB0aGUgY2xvY2sgaXMNCj4gb3B0
aW9uYWw/DQpSaWdodC4NCg0KPiA+IEBAIC0xMDYsMTUgKzEwOSwzOCBAQCBzdHJ1Y3QgY2xrICpv
Zl9jbGtfZ2V0X2J5X25hbWUoc3RydWN0DQo+IGRldmljZV9ub2RlICpucCwgY29uc3QgY2hhciAq
bmFtZSkNCj4gPiAgICAgICAgIGlmICghbnApDQo+ID4gICAgICAgICAgICAgICAgIHJldHVybiBF
UlJfUFRSKC1FTk9FTlQpOw0KPiA+DQo+ID4gLSAgICAgICByZXR1cm4gX19vZl9jbGtfZ2V0X2J5
X25hbWUobnAsIG5wLT5mdWxsX25hbWUsIG5hbWUpOw0KPiA+ICsgICAgICAgcmV0dXJuIF9fb2Zf
Y2xrX2dldF9ieV9uYW1lKG5wLCBucC0+ZnVsbF9uYW1lLCBuYW1lLCBmYWxzZSk7DQo+ID4gIH0N
Cj4gPiAgRVhQT1JUX1NZTUJPTChvZl9jbGtfZ2V0X2J5X25hbWUpOw0KPiA+DQo+ID4gKy8qKg0K
PiA+ICsgKiBvZl9jbGtfZ2V0X2J5X25hbWVfb3B0aW9uYWwoKSAtIFBhcnNlIGFuZCBsb29rdXAg
YW4gb3B0aW9uYWwgY2xvY2sNCj4gPiArcmVmZXJlbmNlZA0KPiA+ICsgKiBieSBhIGRldmljZSBu
b2RlDQo+ID4gKyAqIEBucDogcG9pbnRlciB0byBjbG9jayBjb25zdW1lciBub2RlDQo+ID4gKyAq
IEBuYW1lOiBuYW1lIG9mIGNvbnN1bWVyJ3MgY2xvY2sgaW5wdXQsIG9yIE5VTEwgZm9yIHRoZSBm
aXJzdCBjbG9jaw0KPiA+ICtyZWZlcmVuY2UNCj4gPiArICoNCj4gPiArICogVGhpcyBmdW5jdGlv
biBwYXJzZXMgdGhlIGNsb2NrcyBhbmQgY2xvY2stbmFtZXMgcHJvcGVydGllcywNCj4gPiArICog
YW5kIHVzZXMgdGhlbSB0byBsb29rIHVwIHRoZSBzdHJ1Y3QgY2xrIGZyb20gdGhlIHJlZ2lzdGVy
ZWQgbGlzdA0KPiA+ICtvZiBjbG9jaw0KPiA+ICsgKiBwcm92aWRlcnMuDQo+ID4gKyAqIEl0IGJl
aGF2ZXMgdGhlIHNhbWUgYXMgb2ZfY2xrX2dldF9ieV9uYW1lKCksIGV4Y2VwdCB3aGVuIG5vIGNs
b2NrIGlzDQo+IGZvdW5kLg0KPiA+ICsgKiBJbiB0aGlzIGNhc2UsIGluc3RlYWQgb2YgcmV0dXJu
aW5nIC1FTk9FTlQsIGl0IHJldHVybnMgTlVMTC4NCj4gPiArICovDQo+ID4gK3N0cnVjdCBjbGsg
Km9mX2Nsa19nZXRfYnlfbmFtZV9vcHRpb25hbChzdHJ1Y3QgZGV2aWNlX25vZGUgKm5wLA0KPiA+
ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBjb25zdCBjaGFyICpuYW1l
KSB7DQo+ID4gKyAgICAgICBpZiAoIW5wKQ0KPiA+ICsgICAgICAgICAgICAgICByZXR1cm4gRVJS
X1BUUigtRU5PRU5UKTsNCj4gDQo+IFNob3VsZG4ndCB0aGlzIHJldHVybiBOVUxMPw0KSSB3YXNu
4oCZdCBzdXJlIG9uIHRoaXMsIEkgdGhvdWdodCB0aGUgbm9kZSBzaG91bGQgYmUgdmFsaWQsIGJ1
dCBJIGd1ZXNzIHRoZSBub2RlDQppcyBhbHNvIG9wdGlvbmFsLiBJJ2xsIGZpeC4NCg0KPiBPciBs
ZXQgX19vZl9jbGtfZ2V0X2J5X25hbWUoKSBoYW5kbGUgdGhhdCAoY2ZyLiBhYm92ZSk/DQoNCj4g
SG1tLCBvZl9jbGtfZ2V0X2J5X25hbWUoKSBoYXMgYSBzaW1pbGFyIGNoZWNrLCB3aGlsZSB0aGUg
Y3VycmVudA0KPiBfX29mX2Nsa19nZXRfYnlfbmFtZSgpIGFscmVhZHkgaGFuZGxlIG5wID09IE5V
TEwsIHRvby4NClllcyBJIHNlZSwgSSdsbCBjbGVhbiB0aGF0IHVwLg0KDQogDQo+ID4gKw0KPiA+
ICsgICAgICAgcmV0dXJuIF9fb2ZfY2xrX2dldF9ieV9uYW1lKG5wLCBucC0+ZnVsbF9uYW1lLCBu
YW1lLCB0cnVlKTsgfQ0KPiA+ICtFWFBPUlRfU1lNQk9MKG9mX2Nsa19nZXRfYnlfbmFtZV9vcHRp
b25hbCk7DQo+ID4gKw0KPiA+ICAjZWxzZSAvKiBkZWZpbmVkKENPTkZJR19PRikgJiYgZGVmaW5l
ZChDT05GSUdfQ09NTU9OX0NMSykgKi8NCj4gPg0KPiA+ICBzdGF0aWMgc3RydWN0IGNsayAqX19v
Zl9jbGtfZ2V0X2J5X25hbWUoc3RydWN0IGRldmljZV9ub2RlICpucCwNCj4gPiAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgY29uc3QgY2hhciAqZGV2X2lkLA0KPiA+IC0g
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBjb25zdCBjaGFyICpuYW1lKQ0K
PiA+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBjb25zdCBjaGFyICpu
YW1lLA0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBib29sIG9w
dGlvbmFsKQ0KPiA+ICB7DQo+ID4gICAgICAgICByZXR1cm4gRVJSX1BUUigtRU5PRU5UKTsNCj4g
PiAgfQ0KPiA+IEBAIC0xOTcsNyArMjIzLDcgQEAgc3RydWN0IGNsayAqY2xrX2dldChzdHJ1Y3Qg
ZGV2aWNlICpkZXYsIGNvbnN0IGNoYXINCj4gKmNvbl9pZCkNCj4gPiAgICAgICAgIHN0cnVjdCBj
bGsgKmNsazsNCj4gPg0KPiA+ICAgICAgICAgaWYgKGRldiAmJiBkZXYtPm9mX25vZGUpIHsNCj4g
PiAtICAgICAgICAgICAgICAgY2xrID0gX19vZl9jbGtfZ2V0X2J5X25hbWUoZGV2LT5vZl9ub2Rl
LCBkZXZfaWQsIGNvbl9pZCk7DQo+ID4gKyAgICAgICAgICAgICAgIGNsayA9IF9fb2ZfY2xrX2dl
dF9ieV9uYW1lKGRldi0+b2Zfbm9kZSwgZGV2X2lkLA0KPiA+ICsgY29uX2lkLCBmYWxzZSk7DQo+
ID4gICAgICAgICAgICAgICAgIGlmICghSVNfRVJSKGNsaykgfHwgUFRSX0VSUihjbGspID09IC1F
UFJPQkVfREVGRVIpDQo+ID4gICAgICAgICAgICAgICAgICAgICAgICAgcmV0dXJuIGNsazsNCj4g
PiAgICAgICAgIH0NCj4gPiBkaWZmIC0tZ2l0IGEvaW5jbHVkZS9saW51eC9jbGsuaCBiL2luY2x1
ZGUvbGludXgvY2xrLmggaW5kZXgNCj4gPiA0Zjc1MGM0Li44Y2I1NDU1IDEwMDY0NA0KPiA+IC0t
LSBhL2luY2x1ZGUvbGludXgvY2xrLmgNCj4gPiArKysgYi9pbmNsdWRlL2xpbnV4L2Nsay5oDQo+
ID4gQEAgLTc3Nyw2ICs3NzcsNyBAQCBzdGF0aWMgaW5saW5lIHZvaWQgY2xrX2J1bGtfZGlzYWJs
ZV91bnByZXBhcmUoaW50DQo+ID4gbnVtX2Nsa3MsICAjaWYgZGVmaW5lZChDT05GSUdfT0YpICYm
IGRlZmluZWQoQ09ORklHX0NPTU1PTl9DTEspDQo+ID4gc3RydWN0IGNsayAqb2ZfY2xrX2dldChz
dHJ1Y3QgZGV2aWNlX25vZGUgKm5wLCBpbnQgaW5kZXgpOyAgc3RydWN0IGNsaw0KPiA+ICpvZl9j
bGtfZ2V0X2J5X25hbWUoc3RydWN0IGRldmljZV9ub2RlICpucCwgY29uc3QgY2hhciAqbmFtZSk7
DQo+ID4gK3N0cnVjdCBjbGsgKm9mX2Nsa19nZXRfYnlfbmFtZV9vcHRpb25hbChzdHJ1Y3QgZGV2
aWNlX25vZGUgKm5wLCBjb25zdA0KPiA+ICtjaGFyICpuYW1lKTsNCj4gPiAgc3RydWN0IGNsayAq
b2ZfY2xrX2dldF9mcm9tX3Byb3ZpZGVyKHN0cnVjdCBvZl9waGFuZGxlX2FyZ3MNCj4gPiAqY2xr
c3BlYyk7ICAjZWxzZSAgc3RhdGljIGlubGluZSBzdHJ1Y3QgY2xrICpvZl9jbGtfZ2V0KHN0cnVj
dA0KPiA+IGRldmljZV9ub2RlICpucCwgaW50IGluZGV4KQ0KPiANCj4gTm8gZHVtbXkgdmVyc2lv
biBvZiBvZl9jbGtfZ2V0X2J5X25hbWVfb3B0aW9uYWwoKSBmb3IgdGhlICFDTEsgfHwNCj4gIUNP
TU1PTl9DTEsgY2FzZT8NCj4gDQo+IEl0IHNlZW1zIG9mX2Nsa19nZXRfYnlfbmFtZSgpIGFuZCBv
Zl9jbGtfZ2V0X2J5X25hbWVfb3B0aW9uYWwoKSBjYW4NCj4ganVzdCBiZSBzaW1wbGUgd3JhcHBl
cnMgYXJvdW5kIF9fb2ZfY2xrX2dldF9ieV9uYW1lKCksIGRpZmZlcmluZyBvbmx5IGluDQo+IHRo
ZSB2YWx1ZSBvZiB0aGUgIm9wdGlvbmFsIiBwYXJhbWV0ZXIuIEhlbmNlIEkgdGhpbmsgaXQgbWFr
ZXMgc2Vuc2UgdG8gbW92ZQ0KPiB0aGVtIHRvIHRoZSBoZWFkZXIgZmlsZSwgYW5kIG1ha2UgdGhl
bSBzdGF0aWMgaW5saW5lLg0KPiBUaGVuIG9ubHkgX19vZl9jbGtfZ2V0X2J5X25hbWUoKSBuZWVk
cyBhIChzdGF0aWMgaW5saW5lKSBkdW1teSBmb3IgdGhlDQo+ICFPRiB8fCAhQ09NTU9OX0NMSyBj
YXNlLg0KTWFrZXMgc2Vuc2UuDQoNClRoYW5rcyENClBoaWwNCg0K

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

[Andy Shevchenko]

On Mon, 2018-07-30 at 10:55 +0200, Geert Uytterhoeven wrote:
> On Mon, Jul 30, 2018 at 10:36 AM Phil Edworthy
> <phil.edworthy@renesas.com> wrote:

> > +struct clk *of_clk_get_by_name_optional(struct device_node *np,
> > +                                       const char *name)
> > +{
> > +       if (!np)
> > +               return ERR_PTR(-ENOENT);
> 
> Shouldn't this return NULL?
> Or let __of_clk_get_by_name() handle that (cfr. above)?
> 
> Hmm, of_clk_get_by_name() has a similar check, while the current
> __of_clk_get_by_name() already handle np == NULL, too.

This check is needed to prevent NULL pointer dereference below.

And I agree that absence of device node should be considered as okay
case for optional clock (!CONFIG_OF case, for example).

> > +
> > +       return __of_clk_get_by_name(np, np->full_name, name, true);
> > +}

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

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

[Geert Uytterhoeven]

Hi Andy,

On Mon, Jul 30, 2018 at 12:57 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, 2018-07-30 at 10:55 +0200, Geert Uytterhoeven wrote:
> > On Mon, Jul 30, 2018 at 10:36 AM Phil Edworthy
> > <phil.edworthy@renesas.com> wrote:
>
> > > +struct clk *of_clk_get_by_name_optional(struct device_node *np,
> > > +                                       const char *name)
> > > +{
> > > +       if (!np)
> > > +               return ERR_PTR(-ENOENT);
> >
> > Shouldn't this return NULL?
> > Or let __of_clk_get_by_name() handle that (cfr. above)?
> >
> > Hmm, of_clk_get_by_name() has a similar check, while the current
> > __of_clk_get_by_name() already handle np == NULL, too.
>
> This check is needed to prevent NULL pointer dereference below.

Thank, I had missed that unexpected detail.

> > > +
> > > +       return __of_clk_get_by_name(np, np->full_name, name, true);
> > > +}

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds