* [PATCH v5 0/2] clk: Add functions to get optional clocks
@ 2018-08-31 14:07 ` Phil Edworthy
0 siblings, 0 replies; 23+ messages in thread
From: Phil Edworthy @ 2018-08-31 14:07 UTC (permalink / raw)
To: Andy Shevchenko, Michael Turquette, Stephen Boyd, Russell King
Cc: Geert Uytterhoeven, Simon Horman, linux-clk, linux-kernel,
linux-arm-kernel, 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 0/2] clk: Add functions to get optional clocks
@ 2018-08-31 14:07 ` Phil Edworthy
0 siblings, 0 replies; 23+ messages in thread
From: Phil Edworthy @ 2018-08-31 14:07 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function
2018-08-31 14:07 ` Phil Edworthy
@ 2018-08-31 14:07 ` Phil Edworthy
-1 siblings, 0 replies; 23+ messages in thread
From: Phil Edworthy @ 2018-08-31 14:07 UTC (permalink / raw)
To: Andy Shevchenko, Michael Turquette, Stephen Boyd, Russell King
Cc: Geert Uytterhoeven, Simon Horman, linux-clk, linux-kernel,
linux-arm-kernel, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function
@ 2018-08-31 14:07 ` Phil Edworthy
0 siblings, 0 replies; 23+ messages in thread
From: Phil Edworthy @ 2018-08-31 14:07 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v5 2/2] clk: Add functions to get optional clocks
2018-08-31 14:07 ` Phil Edworthy
@ 2018-08-31 14:07 ` Phil Edworthy
-1 siblings, 0 replies; 23+ messages in thread
From: Phil Edworthy @ 2018-08-31 14:07 UTC (permalink / raw)
To: Andy Shevchenko, Michael Turquette, Stephen Boyd, Russell King
Cc: Geert Uytterhoeven, Simon Horman, linux-clk, linux-kernel,
linux-arm-kernel, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v5 2/2] clk: Add functions to get optional clocks
@ 2018-08-31 14:07 ` Phil Edworthy
0 siblings, 0 replies; 23+ messages in thread
From: Phil Edworthy @ 2018-08-31 14:07 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v5 0/2] clk: Add functions to get optional clocks
2018-08-31 14:07 ` Phil Edworthy
@ 2018-08-31 18:23 ` Andy Shevchenko
-1 siblings, 0 replies; 23+ messages in thread
From: Andy Shevchenko @ 2018-08-31 18:23 UTC (permalink / raw)
To: Phil Edworthy
Cc: Michael Turquette, Stephen Boyd, Russell King,
Geert Uytterhoeven, Simon Horman, linux-clk, linux-kernel,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 0/2] clk: Add functions to get optional clocks
@ 2018-08-31 18:23 ` Andy Shevchenko
0 siblings, 0 replies; 23+ messages in thread
From: Andy Shevchenko @ 2018-08-31 18:23 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function
2018-08-31 14:07 ` Phil Edworthy
(?)
@ 2018-09-01 2:45 ` Stephen Boyd
-1 siblings, 0 replies; 23+ messages in thread
From: Stephen Boyd @ 2018-09-01 2:45 UTC (permalink / raw)
To: Andy Shevchenko, Michael Turquette, Phil Edworthy, Russell King
Cc: Geert Uytterhoeven, Simon Horman, linux-clk, linux-kernel,
linux-arm-kernel, Phil Edworthy
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;
> }
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function
@ 2018-09-01 2:45 ` Stephen Boyd
0 siblings, 0 replies; 23+ messages in thread
From: Stephen Boyd @ 2018-09-01 2:45 UTC (permalink / raw)
To: Andy Shevchenko, Michael Turquette, Phil Edworthy, Russell King
Cc: Geert Uytterhoeven, Simon Horman, linux-clk, linux-kernel,
linux-arm-kernel, Phil Edworthy
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 =3D ERR_PTR(-ENOENT);
> + struct device_node *child =3D np;
> + int index =3D 0;
> =
> /* Walk up the tree of devices looking for a clock that matches */
> while (np) {
> - int index =3D 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 fai=
l.
> + * index will be an error code.
> */
> if (name)
> index =3D of_property_match_string(np, "clock-nam=
es", name);
> - clk =3D __of_clk_get(np, index, dev_id, name);
> - if (!IS_ERR(clk)) {
> - break;
> - } else if (name && index >=3D 0) {
> - if (PTR_ERR(clk) !=3D -EPROBE_DEFER)
> - pr_err("ERROR: could not get clock %pOF:%=
s(%i)\n",
> - np, name ? name : "", index);
> + if (index >=3D 0)
> + clk =3D __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 >=3D 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 n=
ode
> @@ -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) =3D=3D -ENOENT) {
> + clk =3D NULL;
> + pr_info("no optional clock %pOF:%s\n", child,
> + name ? name : "");
Is this intentionally pr_info?
> + } else if (name && index >=3D 0 && PTR_ERR(clk) !=3D -EPROBE_DEFE=
R) {
> + pr_err("ERROR: could not get clock %pOF:%s(%i)\n",
> + child, name, index);
> + }
> +
> return clk;
> }
> =20
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function
@ 2018-09-01 2:45 ` Stephen Boyd
0 siblings, 0 replies; 23+ messages in thread
From: Stephen Boyd @ 2018-09-01 2:45 UTC (permalink / raw)
To: linux-arm-kernel
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;
> }
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 2/2] clk: Add functions to get optional clocks
2018-08-31 14:07 ` Phil Edworthy
@ 2018-09-01 11:43 ` kbuild test robot
-1 siblings, 0 replies; 23+ messages in thread
From: kbuild test robot @ 2018-09-01 11:43 UTC (permalink / raw)
To: Phil Edworthy
Cc: kbuild-all, Andy Shevchenko, Michael Turquette, Stephen Boyd,
Russell King, Geert Uytterhoeven, Simon Horman, linux-clk,
linux-kernel, linux-arm-kernel, Phil Edworthy
[-- 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 --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 2/2] clk: Add functions to get optional clocks
@ 2018-09-01 11:43 ` kbuild test robot
0 siblings, 0 replies; 23+ messages in thread
From: kbuild test robot @ 2018-09-01 11:43 UTC (permalink / raw)
To: linux-arm-kernel
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 6371 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180901/6e5bc2a0/attachment.gz>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 2/2] clk: Add functions to get optional clocks
2018-08-31 14:07 ` Phil Edworthy
@ 2018-09-01 16:01 ` kbuild test robot
-1 siblings, 0 replies; 23+ messages in thread
From: kbuild test robot @ 2018-09-01 16:01 UTC (permalink / raw)
To: Phil Edworthy
Cc: kbuild-all, Andy Shevchenko, Michael Turquette, Stephen Boyd,
Russell King, Geert Uytterhoeven, Simon Horman, linux-clk,
linux-kernel, linux-arm-kernel, Phil Edworthy
[-- 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 --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 2/2] clk: Add functions to get optional clocks
@ 2018-09-01 16:01 ` kbuild test robot
0 siblings, 0 replies; 23+ messages in thread
From: kbuild test robot @ 2018-09-01 16:01 UTC (permalink / raw)
To: linux-arm-kernel
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 6587 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180902/62b89b0f/attachment-0001.gz>
^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function
2018-09-01 2:45 ` Stephen Boyd
(?)
@ 2018-09-03 9:32 ` Phil Edworthy
-1 siblings, 0 replies; 23+ messages in thread
From: Phil Edworthy @ 2018-09-03 9:32 UTC (permalink / raw)
To: Stephen Boyd, Andy Shevchenko, Michael Turquette, Russell King
Cc: Geert Uytterhoeven, Simon Horman, linux-clk, linux-kernel,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function
@ 2018-09-03 9:32 ` Phil Edworthy
0 siblings, 0 replies; 23+ messages in thread
From: Phil Edworthy @ 2018-09-03 9:32 UTC (permalink / raw)
To: Stephen Boyd, Andy Shevchenko, Michael Turquette, Russell King
Cc: Geert Uytterhoeven, Simon Horman, linux-clk, linux-kernel,
linux-arm-kernel
SGkgU3RlcGhlbiwNCg0KT24gMDEgU2VwdGVtYmVyIDIwMTggMDM6NDYsIFN0ZXBoZW4gQm95ZCB3
cm90ZToNCj4gUXVvdGluZyBQaGlsIEVkd29ydGh5ICgyMDE4LTA4LTMxIDA3OjA3OjIyKQ0KPiA+
IGRpZmYgLS1naXQgYS9kcml2ZXJzL2Nsay9jbGtkZXYuYyBiL2RyaXZlcnMvY2xrL2Nsa2Rldi5j
IGluZGV4DQo+ID4gOWFiM2RiOC4uNGFkYjk5ZSAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL2Ns
ay9jbGtkZXYuYw0KPiA+ICsrKyBiL2RyaXZlcnMvY2xrL2Nsa2Rldi5jDQo+ID4gQEAgLTU0LDMw
ICs1NCwyOSBAQCBFWFBPUlRfU1lNQk9MKG9mX2Nsa19nZXQpOw0KPiA+DQo+ID4gIHN0YXRpYyBz
dHJ1Y3QgY2xrICpfX29mX2Nsa19nZXRfYnlfbmFtZShzdHJ1Y3QgZGV2aWNlX25vZGUgKm5wLA0K
PiA+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBjb25zdCBjaGFyICpk
ZXZfaWQsDQo+ID4gLSAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGNvbnN0
IGNoYXIgKm5hbWUpDQo+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
IGNvbnN0IGNoYXIgKm5hbWUsDQo+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgIGJvb2wgb3B0aW9uYWwpDQo+ID4gIHsNCj4gPiAgICAgICAgIHN0cnVjdCBjbGsgKmNs
ayA9IEVSUl9QVFIoLUVOT0VOVCk7DQo+ID4gKyAgICAgICBzdHJ1Y3QgZGV2aWNlX25vZGUgKmNo
aWxkID0gbnA7DQo+ID4gKyAgICAgICBpbnQgaW5kZXggPSAwOw0KPiA+DQo+ID4gICAgICAgICAv
KiBXYWxrIHVwIHRoZSB0cmVlIG9mIGRldmljZXMgbG9va2luZyBmb3IgYSBjbG9jayB0aGF0IG1h
dGNoZXMgKi8NCj4gPiAgICAgICAgIHdoaWxlIChucCkgew0KPiA+IC0gICAgICAgICAgICAgICBp
bnQgaW5kZXggPSAwOw0KPiA+DQo+ID4gICAgICAgICAgICAgICAgIC8qDQo+ID4gICAgICAgICAg
ICAgICAgICAqIEZvciBuYW1lZCBjbG9ja3MsIGZpcnN0IGxvb2sgdXAgdGhlIG5hbWUgaW4gdGhl
DQo+ID4gICAgICAgICAgICAgICAgICAqICJjbG9jay1uYW1lcyIgcHJvcGVydHkuICBJZiBpdCBj
YW5ub3QgYmUgZm91bmQsIHRoZW4NCj4gPiAtICAgICAgICAgICAgICAgICogaW5kZXggd2lsbCBi
ZSBhbiBlcnJvciBjb2RlLCBhbmQgb2ZfY2xrX2dldCgpIHdpbGwgZmFpbC4NCj4gPiArICAgICAg
ICAgICAgICAgICogaW5kZXggd2lsbCBiZSBhbiBlcnJvciBjb2RlLg0KPiA+ICAgICAgICAgICAg
ICAgICAgKi8NCj4gPiAgICAgICAgICAgICAgICAgaWYgKG5hbWUpDQo+ID4gICAgICAgICAgICAg
ICAgICAgICAgICAgaW5kZXggPSBvZl9wcm9wZXJ0eV9tYXRjaF9zdHJpbmcobnAsICJjbG9jay1u
YW1lcyIsIG5hbWUpOw0KPiA+IC0gICAgICAgICAgICAgICBjbGsgPSBfX29mX2Nsa19nZXQobnAs
IGluZGV4LCBkZXZfaWQsIG5hbWUpOw0KPiA+IC0gICAgICAgICAgICAgICBpZiAoIUlTX0VSUihj
bGspKSB7DQo+ID4gLSAgICAgICAgICAgICAgICAgICAgICAgYnJlYWs7DQo+ID4gLSAgICAgICAg
ICAgICAgIH0gZWxzZSBpZiAobmFtZSAmJiBpbmRleCA+PSAwKSB7DQo+ID4gLSAgICAgICAgICAg
ICAgICAgICAgICAgaWYgKFBUUl9FUlIoY2xrKSAhPSAtRVBST0JFX0RFRkVSKQ0KPiA+IC0gICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgcHJfZXJyKCJFUlJPUjogY291bGQgbm90IGdldCBj
bG9jayAlcE9GOiVzKCVpKVxuIiwNCj4gPiAtICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgbnAsIG5hbWUgPyBuYW1lIDogIiIsIGluZGV4KTsNCj4gPiArICAgICAgICAgICAg
ICAgaWYgKGluZGV4ID49IDApDQo+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgY2xrID0gX19v
Zl9jbGtfZ2V0KG5wLCBpbmRleCwgZGV2X2lkLCBuYW1lKTsNCj4gPiArICAgICAgICAgICAgICAg
aWYgKCFJU19FUlIoY2xrKSkNCj4gDQo+IFdhcyB0aGlzIGNoYW5nZSBuZWNlc3Nhcnk/IEl0IGxv
b2tzIGxpa2Ugd2UgY2FuIGxlYXZlIGl0IGFsbCBhbG9uZSBhbmQga2VlcA0KPiBwYXNzaW5nIGEg
bmVnYXRpdmUgbnVtYmVyIHRvIF9fb2ZfY2xrX2dldCgpIGFuZCBoYXZlIHRoYXQgcmV0dXJuIGFu
IGVycm9yDQo+IHBvaW50ZXIgd2hpY2ggd2UgdGhlbiByZXR1cm4gaW1tZWRpYXRlbHkgYXMgYW4g
ZXJyb3IuIEJ1dCwgaWYgdGhlIGNsb2NrIGlzDQo+IG9wdGlvbmFsIGFuZCB3ZSd2ZSBwYXNzZWQg
YSBuYW1lIGhlcmUsIHNob3VsZG4ndCB3ZSB0cmVhdCBhbiBlcnJvciBmcm9tDQo+IG9mX3Byb3Bl
cnR5X21hdGNoX3N0cmluZygpIGFzIHN1Y2Nlc3MgdG9vPyBUaGlzIGlzIGFsbCBsb29raW5nIHBy
ZXR0eSBmcmFnaWxlIHNvDQo+IG1heWJlIGl0IGNhbiBiZSBiZXR0ZXIgY29tbWVudGVkIGFuZCBh
bHNvIG1vcmUgZXhwbGljaXQgaW5zdGVhZCBvZiByZWx5aW5nDQo+IG9uIHRoZSByZWFkZXIgdG8g
anVtcCB0aHJvdWdoIGFsbCB0aGUgZnVuY3Rpb24gY2FsbHMgdG8gZmlndXJlIG91dCB3aGF0IHRo
ZQ0KPiByZXR1cm4gdmFsdWUgaXMgaW4gc29tZSBjYXNlcy4NCklmIHdlIGNhbGwgX19vZl9jbGtf
Z2V0LCB3aXRoIGluZGV4IDwgMCwgd2Ugd2lsbCBub3QgYmUgYWJsZSB0byBkaWZmZXJlbnRpYXRl
DQpiZXR3ZWVuIGNsb2NrIHByb3ZpZGVyIG5vdCBwcmVzZW50IGFuZCBvdGhlciBlcnJvcnMgd2l0
aCB0aGUgcGFzc2VkIGRhdGEsDQphcyBpdCB3aWxsIGp1c3QgcmV0dXJuIC1FSU5WQUwuDQoNCm9m
X3Byb3BlcnR5X21hdGNoX3N0cmluZygpIHdpbGwgcmV0dXJuIC1FSU5WQUwgaWYgdGhlICJjbG9j
ay1uYW1lcyIgcHJvcGVydHkNCmlzIG1pc3NpbmcsIG9yIC1FTk9EQVRBIGlmIHRoZSBzcGVjaWZp
ZWQgY2xvY2sgbmFtZSBpbiB0aGUgImNsb2NrLW5hbWVzIg0KcHJvcGVydHkgaXMgbWlzc2luZy4g
VGhhdCBpcyB3aHkgSSBoYXZlIGNoYW5nZWQgdGhlIGNvZGUgdG8gY29uZGl0aW9uYWxseQ0KY2Fs
bCBfX29mX2Nsa19nZXQsIHNvIHRoZSBjb2RlIHdpbGwgY29ycmVjdGx5IHRyZWF0IG9wdGlvbmFs
IGNsb2NrcyB0aGF0IGFyZSBub3QNCnByZXNlbnQuDQoNCg0KPiA+ICAgICAgICAgICAgICAgICAg
ICAgICAgIHJldHVybiBjbGs7DQo+ID4gLSAgICAgICAgICAgICAgIH0NCj4gPiArICAgICAgICAg
ICAgICAgaWYgKG5hbWUgJiYgaW5kZXggPj0gMCkNCj4gPiArICAgICAgICAgICAgICAgICAgICAg
ICBicmVhazsNCj4gDQo+IEFuZCB0aGlzIGNhdXNlcyB1cyB0byBkdXBsaWNhdGUgbG9naWMgZG93
biBiZWxvdyBiZWNhdXNlIHdlIGhhdmUgdG8gY2hlY2sgaXQNCj4gYWdhaW4gaWYgaXQncyBub3Qg
b3B0aW9uYWwgb3Igc29tZSBvdGhlciBlcnJvciBjb25kaXRpb24/DQpZZXMsIHRoZSBlcnJvciBo
YW5kbGluZyBpcyBtZXNzeSwgdGhvdWdoIEkgaGF2ZSB0cmllZCB0byBtYWtlIHRoaXMgc2ltcGxl
Lg0KSSdsbCBoYXZlIGEgdGhpbmsgYWJvdXQgc29tZSBvdGhlciB3YXkgdG8gbWFrZSB0aGlzIGNs
ZWFuZXIuDQoNCg0KPiA+DQo+ID4gICAgICAgICAgICAgICAgIC8qDQo+ID4gICAgICAgICAgICAg
ICAgICAqIE5vIG1hdGNoaW5nIGNsb2NrIGZvdW5kIG9uIHRoaXMgbm9kZS4gIElmIHRoZQ0KPiA+
IHBhcmVudCBub2RlIEBAIC04OSw2ICs4OCwxNiBAQCBzdGF0aWMgc3RydWN0IGNsaw0KPiAqX19v
Zl9jbGtfZ2V0X2J5X25hbWUoc3RydWN0IGRldmljZV9ub2RlICpucCwNCj4gPiAgICAgICAgICAg
ICAgICAgICAgICAgICBicmVhazsNCj4gPiAgICAgICAgIH0NCj4gPg0KPiA+ICsgICAgICAgLyog
VGhlIGNsb2NrIGlzIG5vdCB2YWxpZCwgYnV0IGl0IGNvdWxkIGJlIG9wdGlvbmFsIG9yIGRlZmVy
cmVkICovDQo+ID4gKyAgICAgICBpZiAob3B0aW9uYWwgJiYgUFRSX0VSUihjbGspID09IC1FTk9F
TlQpIHsNCj4gPiArICAgICAgICAgICAgICAgY2xrID0gTlVMTDsNCj4gPiArICAgICAgICAgICAg
ICAgcHJfaW5mbygibm8gb3B0aW9uYWwgY2xvY2sgJXBPRjolc1xuIiwgY2hpbGQsDQo+ID4gKyAg
ICAgICAgICAgICAgICAgICAgICAgbmFtZSA/IG5hbWUgOiAiIik7DQo+IA0KPiBJcyB0aGlzIGlu
dGVudGlvbmFsbHkgcHJfaW5mbz8NClllcywgaXQncyBub3QgYW4gZXJyb3IgaWYgYW4gb3B0aW9u
YWwgY2xvY2sgaXNu4oCZdCB0aGVyZS4NCldvdWxkIHByX2RlYnVnIGJlIG1vcmUgYXBwcm9wcmlh
dGU/DQoNCg0KPiA+ICsgICAgICAgfSBlbHNlIGlmIChuYW1lICYmIGluZGV4ID49IDAgJiYgUFRS
X0VSUihjbGspICE9IC1FUFJPQkVfREVGRVIpIHsNCj4gPiArICAgICAgICAgICAgICAgcHJfZXJy
KCJFUlJPUjogY291bGQgbm90IGdldCBjbG9jayAlcE9GOiVzKCVpKVxuIiwNCj4gPiArICAgICAg
ICAgICAgICAgICAgICAgICBjaGlsZCwgbmFtZSwgaW5kZXgpOw0KPiA+ICsgICAgICAgfQ0KPiA+
ICsNCj4gPiAgICAgICAgIHJldHVybiBjbGs7DQo+ID4gIH0NCj4gPg0KDQpUaGFua3MNClBoaWwN
Cg==
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function
@ 2018-09-03 9:32 ` Phil Edworthy
0 siblings, 0 replies; 23+ messages in thread
From: Phil Edworthy @ 2018-09-03 9:32 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function
2018-09-03 9:32 ` Phil Edworthy
(?)
@ 2018-09-03 13:21 ` Phil Edworthy
-1 siblings, 0 replies; 23+ messages in thread
From: Phil Edworthy @ 2018-09-03 13:21 UTC (permalink / raw)
To: Stephen Boyd, Andy Shevchenko, Michael Turquette, Russell King
Cc: Geert Uytterhoeven, Simon Horman, linux-clk, linux-kernel,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function
@ 2018-09-03 13:21 ` Phil Edworthy
0 siblings, 0 replies; 23+ messages in thread
From: Phil Edworthy @ 2018-09-03 13:21 UTC (permalink / raw)
To: Stephen Boyd, Andy Shevchenko, Michael Turquette, Russell King
Cc: Geert Uytterhoeven, Simon Horman, linux-clk, linux-kernel,
linux-arm-kernel
SGkgU3RlcGhlbiwNCg0KT24gMDMgU2VwdGVtYmVyIDIwMTggMTA6MzMgUGhpbCBFZHdvcnRoeSB3
cm90ZToNCj4gT24gMDEgU2VwdGVtYmVyIDIwMTggMDM6NDYsIFN0ZXBoZW4gQm95ZCB3cm90ZToN
Cj4gPiBRdW90aW5nIFBoaWwgRWR3b3J0aHkgKDIwMTgtMDgtMzEgMDc6MDc6MjIpDQo+ID4gPiBk
aWZmIC0tZ2l0IGEvZHJpdmVycy9jbGsvY2xrZGV2LmMgYi9kcml2ZXJzL2Nsay9jbGtkZXYuYyBp
bmRleA0KPiA+ID4gOWFiM2RiOC4uNGFkYjk5ZSAxMDA2NDQNCj4gPiA+IC0tLSBhL2RyaXZlcnMv
Y2xrL2Nsa2Rldi5jDQo+ID4gPiArKysgYi9kcml2ZXJzL2Nsay9jbGtkZXYuYw0KPiA+ID4gQEAg
LTU0LDMwICs1NCwyOSBAQCBFWFBPUlRfU1lNQk9MKG9mX2Nsa19nZXQpOw0KPiA+ID4NCj4gPiA+
ICBzdGF0aWMgc3RydWN0IGNsayAqX19vZl9jbGtfZ2V0X2J5X25hbWUoc3RydWN0IGRldmljZV9u
b2RlICpucCwNCj4gPiA+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBj
b25zdCBjaGFyICpkZXZfaWQsDQo+ID4gPiAtICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgY29uc3QgY2hhciAqbmFtZSkNCj4gPiA+ICsgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICBjb25zdCBjaGFyICpuYW1lLA0KPiA+ID4gKyAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgIGJvb2wgb3B0aW9uYWwpDQo+ID4gPiAgew0KPiA+ID4g
ICAgICAgICBzdHJ1Y3QgY2xrICpjbGsgPSBFUlJfUFRSKC1FTk9FTlQpOw0KPiA+ID4gKyAgICAg
ICBzdHJ1Y3QgZGV2aWNlX25vZGUgKmNoaWxkID0gbnA7DQo+ID4gPiArICAgICAgIGludCBpbmRl
eCA9IDA7DQo+ID4gPg0KPiA+ID4gICAgICAgICAvKiBXYWxrIHVwIHRoZSB0cmVlIG9mIGRldmlj
ZXMgbG9va2luZyBmb3IgYSBjbG9jayB0aGF0IG1hdGNoZXMgKi8NCj4gPiA+ICAgICAgICAgd2hp
bGUgKG5wKSB7DQo+ID4gPiAtICAgICAgICAgICAgICAgaW50IGluZGV4ID0gMDsNCj4gPiA+DQo+
ID4gPiAgICAgICAgICAgICAgICAgLyoNCj4gPiA+ICAgICAgICAgICAgICAgICAgKiBGb3IgbmFt
ZWQgY2xvY2tzLCBmaXJzdCBsb29rIHVwIHRoZSBuYW1lIGluIHRoZQ0KPiA+ID4gICAgICAgICAg
ICAgICAgICAqICJjbG9jay1uYW1lcyIgcHJvcGVydHkuICBJZiBpdCBjYW5ub3QgYmUgZm91bmQs
IHRoZW4NCj4gPiA+IC0gICAgICAgICAgICAgICAgKiBpbmRleCB3aWxsIGJlIGFuIGVycm9yIGNv
ZGUsIGFuZCBvZl9jbGtfZ2V0KCkgd2lsbCBmYWlsLg0KPiA+ID4gKyAgICAgICAgICAgICAgICAq
IGluZGV4IHdpbGwgYmUgYW4gZXJyb3IgY29kZS4NCj4gPiA+ICAgICAgICAgICAgICAgICAgKi8N
Cj4gPiA+ICAgICAgICAgICAgICAgICBpZiAobmFtZSkNCj4gPiA+ICAgICAgICAgICAgICAgICAg
ICAgICAgIGluZGV4ID0gb2ZfcHJvcGVydHlfbWF0Y2hfc3RyaW5nKG5wLCAiY2xvY2stbmFtZXMi
LA0KPiBuYW1lKTsNCj4gPiA+IC0gICAgICAgICAgICAgICBjbGsgPSBfX29mX2Nsa19nZXQobnAs
IGluZGV4LCBkZXZfaWQsIG5hbWUpOw0KPiA+ID4gLSAgICAgICAgICAgICAgIGlmICghSVNfRVJS
KGNsaykpIHsNCj4gPiA+IC0gICAgICAgICAgICAgICAgICAgICAgIGJyZWFrOw0KPiA+ID4gLSAg
ICAgICAgICAgICAgIH0gZWxzZSBpZiAobmFtZSAmJiBpbmRleCA+PSAwKSB7DQo+ID4gPiAtICAg
ICAgICAgICAgICAgICAgICAgICBpZiAoUFRSX0VSUihjbGspICE9IC1FUFJPQkVfREVGRVIpDQo+
ID4gPiAtICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHByX2VycigiRVJST1I6IGNvdWxk
IG5vdCBnZXQgY2xvY2sgJXBPRjolcyglaSlcbiIsDQo+ID4gPiAtICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgbnAsIG5hbWUgPyBuYW1lIDogIiIsIGluZGV4KTsNCj4gPiA+
ICsgICAgICAgICAgICAgICBpZiAoaW5kZXggPj0gMCkNCj4gPiA+ICsgICAgICAgICAgICAgICAg
ICAgICAgIGNsayA9IF9fb2ZfY2xrX2dldChucCwgaW5kZXgsIGRldl9pZCwgbmFtZSk7DQo+ID4g
PiArICAgICAgICAgICAgICAgaWYgKCFJU19FUlIoY2xrKSkNCj4gPg0KPiA+IFdhcyB0aGlzIGNo
YW5nZSBuZWNlc3Nhcnk/IEl0IGxvb2tzIGxpa2Ugd2UgY2FuIGxlYXZlIGl0IGFsbCBhbG9uZSBh
bmQga2VlcA0KPiA+IHBhc3NpbmcgYSBuZWdhdGl2ZSBudW1iZXIgdG8gX19vZl9jbGtfZ2V0KCkg
YW5kIGhhdmUgdGhhdCByZXR1cm4gYW4gZXJyb3INCj4gPiBwb2ludGVyIHdoaWNoIHdlIHRoZW4g
cmV0dXJuIGltbWVkaWF0ZWx5IGFzIGFuIGVycm9yLiBCdXQsIGlmIHRoZSBjbG9jayBpcw0KPiA+
IG9wdGlvbmFsIGFuZCB3ZSd2ZSBwYXNzZWQgYSBuYW1lIGhlcmUsIHNob3VsZG4ndCB3ZSB0cmVh
dCBhbiBlcnJvciBmcm9tDQo+ID4gb2ZfcHJvcGVydHlfbWF0Y2hfc3RyaW5nKCkgYXMgc3VjY2Vz
cyB0b28/IFRoaXMgaXMgYWxsIGxvb2tpbmcgcHJldHR5IGZyYWdpbGUNCj4gc28NCj4gPiBtYXli
ZSBpdCBjYW4gYmUgYmV0dGVyIGNvbW1lbnRlZCBhbmQgYWxzbyBtb3JlIGV4cGxpY2l0IGluc3Rl
YWQgb2YgcmVseWluZw0KPiA+IG9uIHRoZSByZWFkZXIgdG8ganVtcCB0aHJvdWdoIGFsbCB0aGUg
ZnVuY3Rpb24gY2FsbHMgdG8gZmlndXJlIG91dCB3aGF0IHRoZQ0KPiA+IHJldHVybiB2YWx1ZSBp
cyBpbiBzb21lIGNhc2VzLg0KPiBJZiB3ZSBjYWxsIF9fb2ZfY2xrX2dldCwgd2l0aCBpbmRleCA8
IDAsIHdlIHdpbGwgbm90IGJlIGFibGUgdG8gZGlmZmVyZW50aWF0ZQ0KPiBiZXR3ZWVuIGNsb2Nr
IHByb3ZpZGVyIG5vdCBwcmVzZW50IGFuZCBvdGhlciBlcnJvcnMgd2l0aCB0aGUgcGFzc2VkIGRh
dGEsDQo+IGFzIGl0IHdpbGwganVzdCByZXR1cm4gLUVJTlZBTC4NCj4gDQo+IG9mX3Byb3BlcnR5
X21hdGNoX3N0cmluZygpIHdpbGwgcmV0dXJuIC1FSU5WQUwgaWYgdGhlICJjbG9jay1uYW1lcyIN
Cj4gcHJvcGVydHkNCj4gaXMgbWlzc2luZywgb3IgLUVOT0RBVEEgaWYgdGhlIHNwZWNpZmllZCBj
bG9jayBuYW1lIGluIHRoZSAiY2xvY2stbmFtZXMiDQo+IHByb3BlcnR5IGlzIG1pc3NpbmcuIFRo
YXQgaXMgd2h5IEkgaGF2ZSBjaGFuZ2VkIHRoZSBjb2RlIHRvIGNvbmRpdGlvbmFsbHkNCj4gY2Fs
bCBfX29mX2Nsa19nZXQsIHNvIHRoZSBjb2RlIHdpbGwgY29ycmVjdGx5IHRyZWF0IG9wdGlvbmFs
IGNsb2NrcyB0aGF0IGFyZSBub3QNCj4gcHJlc2VudC4NCldoZW4gZ2V0dGluZyBuYW1lZCBvcHRp
b25hbCBjbG9ja3MsIGlmIHRoZSBub2RlIGhhcyBhICJjbG9jay1uYW1lcyIgcHJvcGVydHksDQpi
dXQgbm8gY2xvY2sgbWF0Y2hpbmcgdGhlIG5hbWUgd2Ugd2FudCwgSSB0aGluayB0aGUgZnVuY3Rp
b24gc2hvdWxkIHN0b3AgdGhlcmUNCmFuZCAqbm90KiB3YWxrIHVwIHRoZSB0cmVlIG9mIGRldmlj
ZXMgbG9va2luZyBmb3IgYSBtYXRjaGluZyBjbG9jay4gSW4gdGhpcyBjYXNlLA0KdGhlIGNvZGUg
ZGV0ZXJtaW5lcyB0aGF0IHRoZSBvcHRpb25hbCBjbG9jayBpcyBub3QgcHJlc2VudC4NCg0KSWYg
dGhlcmUgaXNu4oCZdCBhICJjbG9jay1uYW1lcyIgcHJvcGVydHkgaW4gdGhlIGN1cnJlbnQgbm9k
ZSwgdGhlIGZ1bmN0aW9uIHNob3VsZA0Kd2FsayB1cCB0aGUgdHJlZSBvZiBkZXZpY2VzIGxvb2tp
bmcgZm9yIGEgbWF0Y2hpbmcgb3B0aW9uYWwgY2xvY2suIElmIHRoZXJlIGFyZSBubw0KcGFyZW50
IG5vZGVzIGxlZnQgYW5kIHdlIGhhdmVuJ3QgZm91bmQgYSBtYXRjaGluZyBvcHRpb25hbCBjbG9j
aywgd2UgZGV0ZXJtaW5lDQp0aGF0IHRoZSBjbG9jayBpc27igJl0IHRoZXJlLg0KDQpJcyB0aGF0
IGhvdyB0aGlzIHNob3VsZCB3b3JrPw0KDQpUaGFua3MNClBoaWwNCg0KDQo+ID4gPiAgICAgICAg
ICAgICAgICAgICAgICAgICByZXR1cm4gY2xrOw0KPiA+ID4gLSAgICAgICAgICAgICAgIH0NCj4g
PiA+ICsgICAgICAgICAgICAgICBpZiAobmFtZSAmJiBpbmRleCA+PSAwKQ0KPiA+ID4gKyAgICAg
ICAgICAgICAgICAgICAgICAgYnJlYWs7DQo+ID4NCj4gPiBBbmQgdGhpcyBjYXVzZXMgdXMgdG8g
ZHVwbGljYXRlIGxvZ2ljIGRvd24gYmVsb3cgYmVjYXVzZSB3ZSBoYXZlIHRvIGNoZWNrDQo+IGl0
DQo+ID4gYWdhaW4gaWYgaXQncyBub3Qgb3B0aW9uYWwgb3Igc29tZSBvdGhlciBlcnJvciBjb25k
aXRpb24/DQo+IFllcywgdGhlIGVycm9yIGhhbmRsaW5nIGlzIG1lc3N5LCB0aG91Z2ggSSBoYXZl
IHRyaWVkIHRvIG1ha2UgdGhpcyBzaW1wbGUuDQo+IEknbGwgaGF2ZSBhIHRoaW5rIGFib3V0IHNv
bWUgb3RoZXIgd2F5IHRvIG1ha2UgdGhpcyBjbGVhbmVyLg0KPiANCj4gDQo+ID4gPg0KPiA+ID4g
ICAgICAgICAgICAgICAgIC8qDQo+ID4gPiAgICAgICAgICAgICAgICAgICogTm8gbWF0Y2hpbmcg
Y2xvY2sgZm91bmQgb24gdGhpcyBub2RlLiAgSWYgdGhlDQo+ID4gPiBwYXJlbnQgbm9kZSBAQCAt
ODksNiArODgsMTYgQEAgc3RhdGljIHN0cnVjdCBjbGsNCj4gPiAqX19vZl9jbGtfZ2V0X2J5X25h
bWUoc3RydWN0IGRldmljZV9ub2RlICpucCwNCj4gPiA+ICAgICAgICAgICAgICAgICAgICAgICAg
IGJyZWFrOw0KPiA+ID4gICAgICAgICB9DQo+ID4gPg0KPiA+ID4gKyAgICAgICAvKiBUaGUgY2xv
Y2sgaXMgbm90IHZhbGlkLCBidXQgaXQgY291bGQgYmUgb3B0aW9uYWwgb3IgZGVmZXJyZWQgKi8N
Cj4gPiA+ICsgICAgICAgaWYgKG9wdGlvbmFsICYmIFBUUl9FUlIoY2xrKSA9PSAtRU5PRU5UKSB7
DQo+ID4gPiArICAgICAgICAgICAgICAgY2xrID0gTlVMTDsNCj4gPiA+ICsgICAgICAgICAgICAg
ICBwcl9pbmZvKCJubyBvcHRpb25hbCBjbG9jayAlcE9GOiVzXG4iLCBjaGlsZCwNCj4gPiA+ICsg
ICAgICAgICAgICAgICAgICAgICAgIG5hbWUgPyBuYW1lIDogIiIpOw0KPiA+DQo+ID4gSXMgdGhp
cyBpbnRlbnRpb25hbGx5IHByX2luZm8/DQo+IFllcywgaXQncyBub3QgYW4gZXJyb3IgaWYgYW4g
b3B0aW9uYWwgY2xvY2sgaXNu4oCZdCB0aGVyZS4NCj4gV291bGQgcHJfZGVidWcgYmUgbW9yZSBh
cHByb3ByaWF0ZT8NCj4gDQo+IA0KPiA+ID4gKyAgICAgICB9IGVsc2UgaWYgKG5hbWUgJiYgaW5k
ZXggPj0gMCAmJiBQVFJfRVJSKGNsaykgIT0gLUVQUk9CRV9ERUZFUikgew0KPiA+ID4gKyAgICAg
ICAgICAgICAgIHByX2VycigiRVJST1I6IGNvdWxkIG5vdCBnZXQgY2xvY2sgJXBPRjolcyglaSlc
biIsDQo+ID4gPiArICAgICAgICAgICAgICAgICAgICAgICBjaGlsZCwgbmFtZSwgaW5kZXgpOw0K
PiA+ID4gKyAgICAgICB9DQo+ID4gPiArDQo+ID4gPiAgICAgICAgIHJldHVybiBjbGs7DQo+ID4g
PiAgfQ0KPiA+ID4NCj4gDQo+IFRoYW5rcw0KPiBQaGlsDQo=
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function
@ 2018-09-03 13:21 ` Phil Edworthy
0 siblings, 0 replies; 23+ messages in thread
From: Phil Edworthy @ 2018-09-03 13:21 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function
2018-09-03 13:21 ` Phil Edworthy
@ 2018-11-13 19:44 ` Stephen Boyd
-1 siblings, 0 replies; 23+ messages in thread
From: Stephen Boyd @ 2018-11-13 19:44 UTC (permalink / raw)
To: Andy Shevchenko, Michael Turquette, Phil Edworthy, Russell King
Cc: Geert Uytterhoeven, Simon Horman, linux-clk, linux-kernel,
linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function
@ 2018-11-13 19:44 ` Stephen Boyd
0 siblings, 0 replies; 23+ messages in thread
From: Stephen Boyd @ 2018-11-13 19:44 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2018-11-13 19:44 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-31 14:07 [PATCH v5 0/2] clk: Add functions to get optional clocks Phil Edworthy
2018-08-31 14:07 ` Phil Edworthy
2018-08-31 14:07 ` [PATCH v5 1/2] clk: Add of_clk_get_by_name_optional() function Phil Edworthy
2018-08-31 14:07 ` Phil Edworthy
2018-09-01 2:45 ` Stephen Boyd
2018-09-01 2:45 ` Stephen Boyd
2018-09-01 2:45 ` Stephen Boyd
2018-09-03 9:32 ` Phil Edworthy
2018-09-03 9:32 ` Phil Edworthy
2018-09-03 9:32 ` Phil Edworthy
2018-09-03 13:21 ` Phil Edworthy
2018-09-03 13:21 ` Phil Edworthy
2018-09-03 13:21 ` Phil Edworthy
2018-11-13 19:44 ` Stephen Boyd
2018-11-13 19:44 ` Stephen Boyd
2018-08-31 14:07 ` [PATCH v5 2/2] clk: Add functions to get optional clocks Phil Edworthy
2018-08-31 14:07 ` Phil Edworthy
2018-09-01 11:43 ` kbuild test robot
2018-09-01 11:43 ` kbuild test robot
2018-09-01 16:01 ` kbuild test robot
2018-09-01 16:01 ` kbuild test robot
2018-08-31 18:23 ` [PATCH v5 0/2] " Andy Shevchenko
2018-08-31 18:23 ` Andy Shevchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.