[PATCH] clk: Add functions to get optional clocks
From: Phil Edworthy <hidden>
Date: 2018-07-27 15:38:26
Also in:
linux-clk, lkml
Subsystem:
arm/clkdev support, clk api, the rest · Maintainers:
Russell King, Linus Torvalds
Hi Stephen, On 25 July 2018 23:37, Stephen Boyd wrote:
Quoting Phil Edworthy (2018-07-18 06:56:26)quoted
On 18 July 2018 14:19, Geert Uytterhoeven wrote:quoted
On Wed, Jul 18, 2018 at 3:02 PM Russell King - ARM Linux wrote:quoted
On Wed, Jul 18, 2018 at 01:57:38PM +0100, Phil Edworthy wrote:quoted
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.How does this work with non-DT systems, where looking a clock up which isn't yet registered with clkdev returns -ENOENT ? (clkdev doesn't know when all clocks are registered with it.)Good question. I guess all drivers trying to handle optional clocks this way are already broken on non-DT systems where clocks may be registered late...So how do non-DT systems that look a clock up which isn't yet registered with clkdev, determine that an optional clock is there or not?Short answer is they don't. I'd still prefer we have this API though. Can you rework this patch to be a little more invasive into the clk_get() path, perhaps by reworking __of_clk_get_by_name() a little to take an 'optional' argument, so that it only returns NULL when the clk is looked up from DT? The fallback path in clkdev where we have a DT based system looking up a clk through clkdev lookups doesn't seem to be a real scenario that we should worry about here. I think sometimes people use clkdev lookups when they're migrating to DT systems and things aren't wired up properly in DT, but that isn't the norm.
Do you mean something like this:
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 7f2cd1e..42a7d4e 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c@@ -57,7 +57,8 @@ EXPORT_SYMBOL(of_clk_get); static struct clk *__of_clk_get_by_name(struct device_node *np, const char *dev_id, - const char *name) + const char *name, + bool optional) { struct clk *clk = ERR_PTR(-ENOENT);
@@ -79,6 +80,8 @@ static struct clk *__of_clk_get_by_name(struct device_node *np, if (PTR_ERR(clk) != -EPROBE_DEFER) pr_err("ERROR: could not get clock %pOF:%s(%i)\n", np, name ? name : "", index); + if (optional && PTR_ERR(clk) == -ENOENT) + clk = NULL; return clk; }
@@ -109,15 +112,38 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name) if (!np) return ERR_PTR(-ENOENT); - return __of_clk_get_by_name(np, np->full_name, name); + return __of_clk_get_by_name(np, np->full_name, name, false); } EXPORT_SYMBOL(of_clk_get_by_name); +/** + * of_clk_get_by_name_optional() - Parse and lookup an optional clock referenced + * by a device node + * @np: pointer to clock consumer node + * @name: name of consumer's clock input, or NULL for the first clock reference + * + * This function parses the clocks and clock-names properties, + * and uses them to look up the struct clk from the registered list of clock + * providers. + * It behaves the same as of_clk_get_by_name(), except when no clock is found. + * In this case, instead of returning -ENOENT, it returns NULL. + */ +struct clk *of_clk_get_by_name_optional(struct device_node *np, + const char *name) +{ + if (!np) + return ERR_PTR(-ENOENT); + + return __of_clk_get_by_name(np, np->full_name, name, true); +} +EXPORT_SYMBOL(of_clk_get_by_name_optional); + #else /* defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) */ static struct clk *__of_clk_get_by_name(struct device_node *np, const char *dev_id, - const char *name) + const char *name, + bool optional) { return ERR_PTR(-ENOENT); }
@@ -200,7 +226,7 @@ struct clk *clk_get(struct device *dev, const char *con_id) struct clk *clk; if (dev) { - 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 907202b..830209a 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h@@ -771,6 +771,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)
--- A lot of drivers use devm_clk_get() so I think a devm_clk_get_optional() version would be useful. That would probably need an additional clk_get_optional() function. Thanks Phil