ASoC: kirkwood: clk: probe defer when clock not yet ready
Subsystems (2)
PATCH v2 1/2 ASoC: kirkwood: clk: probe defer when clock not yet ready
At probe time, a clock device may not be ready when some other device wants to use it.
This patch lets the functions clk_get/devm_clk_get return a probe defer when the clock is defined in the DT but not yet available.
--- drivers/clk/clk.c | 2 +- drivers/clk/clkdev.c | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-)
drivers/clk/clk.c +1 -1
@@ -2181,7 +2181,7 @@ EXPORT_SYMBOL_GPL(of_clk_del_provider);struct clk *of_clk_get_from_provider (struct of_phandle_args *clkspec) {struct of_clk_provider *provider; -struct clk *clk =ERR_PTR (-ENOENT ); +struct clk *clk =ERR_PTR (-EPROBE_DEFER );/* Check if we have such a provider in our array */ mutex_lock (&of_clk_lock);
drivers/clk/clkdev.c +7 -2
@@ -157,8 +157,13 @@ struct clk *clk_get(struct device *dev, const char *con_id)if (dev) { clk =of_clk_get_by_name (dev->of_node, con_id); -if (!IS_ERR (clk) &&__clk_get (clk)) -return clk; +if (!IS_ERR (clk)) { +if (__clk_get (clk)) +return clk; + }else { +if (PTR_ERR (clk) == -EPROBE_DEFER ) +return clk; + } }return clk_get_sys (dev_id, con_id);
Signature · 6 lines
Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 7 lines · Jean-Francois Moine
drivers/clk/clkdev.c +0
-0@@ -157,8 +157,13 @@ struct clk *clk_get(struct device *dev, const char *con_id)if (dev) {
drivers/clk/clkdev.c struct clk *clk_get(struct device *dev, const char *con_id) · line 166
Quoted 10 lines
drivers/clk/clkdev.c +7
-2clk =of_clk_get_by_name (dev->of_node, con_id); -if (!IS_ERR (clk) &&__clk_get (clk)) -return clk; +if (!IS_ERR (clk)) { +if (__clk_get (clk)) +return clk; + }else { +if (PTR_ERR (clk) == -EPROBE_DEFER ) +return clk; + }
Thanks for patching this file and bringing this code to my attention. The original is fscked. Calling __clk_get() outside of any lock is a serious bug and leads to races.
Also, a failing __clk_get() probably should not drop through to clk_get_sys(), but should instead do this:
if (!IS_ERR(clk)) {
if (!__clk_get(clk))
clk = ERR_PTR(-ENOENT);
return clk;
}since a _found_ clock but one which can't be used (due to the module going away) is not a reason to drop through to clk_get_sys().
Which means your change should look like this:
if (!IS_ERR(clk)) {
if (!__clk_get(clk))
clk = ERR_PTR(-ENOENT);
return clk;
}
if (PTR_ERR(clk) == -EPROBE_DEFER)
return clk;--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 20 lines · Jean-Francois Moine
drivers/clk/clk.c +1
-1@@ -2181,7 +2181,7 @@ EXPORT_SYMBOL_GPL(of_clk_del_provider);struct clk *of_clk_get_from_provider (struct of_phandle_args *clkspec) {struct of_clk_provider *provider; -struct clk *clk =ERR_PTR (-ENOENT ); +struct clk *clk =ERR_PTR (-EPROBE_DEFER );/* Check if we have such a provider in our array */ mutex_lock (&of_clk_lock);drivers/clk/clkdev.c +0
-0@@ -157,8 +157,13 @@ struct clk *clk_get(struct device *dev, const char *con_id)if (dev) {
drivers/clk/clkdev.c struct clk *clk_get(struct device *dev, const char *con_id) · line 166
Quoted 10 lines
drivers/clk/clkdev.c +7
-2clk =of_clk_get_by_name (dev->of_node, con_id); -if (!IS_ERR (clk) &&__clk_get (clk)) -return clk; +if (!IS_ERR (clk)) { +if (__clk_get (clk)) +return clk; + }else { +if (PTR_ERR (clk) == -EPROBE_DEFER ) +return clk; + }
Another comment. This is broken - it will never fall through if a 'dev' is given, because of_clk_get_by_name() will always return an -EPROBE_DEFER error pointer in that case.
So this approach is unworkable.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 20130921104734.GJ12758-l+eeeJia6m9vn6HldHNs0ANdhmdF6hFW@public.gmane.org
Quoted 5 lines · Russell King - ARM Linux
Another comment. This is broken - it will never fall through if a 'dev' is given, because
of_clk_get_by_name()will always return an -EPROBE_DEFER error pointer in that case.So this approach is unworkable.
And another comment:
This whole approach is broken - while Mark is correct that we should find a way to return -EPROBE_DEFER if a clock is specified, the solution here does not allow that - it will return -EPROBE_DEFER whenever the clock is not found _or_ is not specified.
This has the effect of making the external clock *non*optional for the kirkwood-i2s driver, which is also completely broken.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 40 lines · On Sat, 21 Sep 2013 11:47:34 +0100 Russell King - ARM Linux…
Quoted 20 lines · Jean-Francois Moine
drivers/clk/clk.c +1
-1@@ -2181,7 +2181,7 @@ EXPORT_SYMBOL_GPL(of_clk_del_provider);struct clk *of_clk_get_from_provider (struct of_phandle_args *clkspec) {struct of_clk_provider *provider; -struct clk *clk =ERR_PTR (-ENOENT ); +struct clk *clk =ERR_PTR (-EPROBE_DEFER );/* Check if we have such a provider in our array */ mutex_lock (&of_clk_lock);drivers/clk/clkdev.c +0
-0@@ -157,8 +157,13 @@ struct clk *clk_get(struct device *dev, const char *con_id)if (dev) {drivers/clk/clkdev.c struct clk *clk_get(struct device *dev, const char *con_id) · line 166
Quoted 10 lines
drivers/clk/clkdev.c +7
-2clk =of_clk_get_by_name (dev->of_node, con_id); -if (!IS_ERR (clk) &&__clk_get (clk)) -return clk; +if (!IS_ERR (clk)) { +if (__clk_get (clk)) +return clk; + }else { +if (PTR_ERR (clk) == -EPROBE_DEFER ) +return clk; + }Another comment. This is broken - it will never fall through if a 'dev' is given, because
of_clk_get_by_name()will always return an -EPROBE_DEFER error pointer in that case.So this approach is unworkable.
And another comment:
Quoted 7 lines
This whole approach is broken - while Mark is correct that we should find a way to return -EPROBE_DEFER if a clock is specified, the solution here does not allow that - it will return -EPROBE_DEFER whenever the clock is not found _or_ is not specified.
This has the effect of making the external clock *non*optional for the kirkwood-i2s driver, which is also completely broken.
It works fine for me (I tried both cases, with and without the external clock).
The scheme is as follows:
- kirkwood-i2s kirkwood_i2s_dev_probe() calls:
priv->extclk = &pdev->dev, "extclk");
- devm_clk_get() calls:
clk = clk_get(dev, id);
- clk_get() does:
if (dev) // this is the case clk = of_clk_get_by_name(dev->of_node, con_id);
- of_clk_get_by_name() does:
if (name) // this is the case index = of_property_match_string(np, "clock-names", name);
Here, if the external clock is declared in the DT, index is >= 0. If there is no externel clock in the DT, index is < 0.
clk = of_clk_get(np, index);
if (!IS_ERR(clk))
break;
else if (name && index >= 0) {
pr_err("ERROR: could not get clock %s:%s(%i)\n", np->full_name, name ? name : "", index);
return clk;
}
- If the index is < 0, of_clk_get() immediately returns -EINVAL.
So, kirkwood-i2s knows there is no external clock. DONE.
- otherwise, if the index is >= 0 (the external clock is declared in the DT), of_clk_get() gets the phandle (of_parse_phandle_with_args()) and gets the clock (of_clk_get_from_provider()).
This last function returns either the clock (device initialized), a -EPROBE_DEFER error (clock not yet initialized) or some other error (bad DT or clock error).
So, kirkwood-i2s knows there is a declared external clock and it acts according to the return code (accept, defer or ignore). DONE.
Did I miss something?
Signature · 6 lines
Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html