[alsa-devel] Question about devm_get_clk_from_child()
Hi Stephen
I noticed that this commit breaks my ALSA Sound playback
4472287a3b2f52f4aa53f294ccb74392dde4e07d ("clk: Introduce of_clk_get_hw_from_clkspec()")
I debuged this issue, and noticed that devm_get_clk_from_child() can't get clk my pseudo DT code is like this
deviceA: devA { ... device = <&devB> };
deviceB: devB { clocks = <xxxx> };
I could get clock on deviceB driver
// dev = deviceB clk = clk_get(dev, NULL);
clk is "clocks = <xxx>"
But, I couldn't get clock from deviceA driver
// node = device = devB node = ...
// dev = deviceA clk = devm_get_clk_from_child(dev, node, NULL);
My understanding is these can get same clock. But, am I wrong ??
Best regards --- Kuninori Morimoto
Hi Stephen, again
I noticed that this commit breaks my ALSA Sound playback
4472287a3b2f52f4aa53f294ccb74392dde4e07d ("clk: Introduce of_clk_get_hw_from_clkspec()")
I could solve this issue, but I'm not sure this is correct.
For ALSA SoC At least if you are using "simple-card" or "audio-graph" driver, you might got same issue.
------------------ From ea035a378a24479aea8d00fa8463bc4ff0bc5926 Mon Sep 17 00:00:00 2001 From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Wed, 6 Mar 2019 15:54:59 +0900 Subject: [PATCH] clk: fixup default index for of_clk_get_by_name()
of_clk_get_by_name() is using -1 for __of_clk_get() index. It will goes to of_parse_clkspec(), and be used for of_parse_phandle_with_args(). Here, if user doesn't specified clock name (= of_clk_get_by_name(np, NULL)), this index is still -1, and of_parse_phandle_with_args() will return -EINVAL (This index will be updated if if it had clock name). clk_get_by_name(np, NULL) should work, then, default index should be 0 instead of -1. This patch fixup it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- drivers/clk/clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 14cbf23..96053a9 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -4213,7 +4213,7 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name) if (!np) return ERR_PTR(-ENOENT);
- return __of_clk_get(np, -1, np->full_name, name); + return __of_clk_get(np, 0, np->full_name, name); } EXPORT_SYMBOL(of_clk_get_by_name);
Quoting Kuninori Morimoto (2019-03-05 23:18:28)
of_clk_get_by_name() is using -1 for __of_clk_get() index. It will goes to of_parse_clkspec(), and be used for of_parse_phandle_with_args(). Here, if user doesn't specified clock name (= of_clk_get_by_name(np, NULL)), this index is still -1, and of_parse_phandle_with_args() will return -EINVAL (This index will be updated if if it had clock name). clk_get_by_name(np, NULL) should work, then, default index should be 0 instead of -1. This patch fixup it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
drivers/clk/clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 14cbf23..96053a9 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -4213,7 +4213,7 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name) if (!np) return ERR_PTR(-ENOENT);
return __of_clk_get(np, -1, np->full_name, name);
return __of_clk_get(np, 0, np->full_name, name);}
Yes this is correct. Thanks for debugging and fixing my thinko here. I was thinking that nobody would call of_clk_get_by_name() unless they wanted to find some clk that had a matching name, but it seems that we also allow NULL to be passed as the name to mean the typical "wildcard match" thing that clkdev has done for years. I'll throw this patch on top of the merge commit so the breakage window is small as I'd rather not rewrite the series just for this. Thanks.
Hi Stephen
index 14cbf23..96053a9 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -4213,7 +4213,7 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name) if (!np) return ERR_PTR(-ENOENT);
return __of_clk_get(np, -1, np->full_name, name);
return __of_clk_get(np, 0, np->full_name, name);}
Yes this is correct. Thanks for debugging and fixing my thinko here. I was thinking that nobody would call of_clk_get_by_name() unless they wanted to find some clk that had a matching name, but it seems that we also allow NULL to be passed as the name to mean the typical "wildcard match" thing that clkdev has done for years. I'll throw this patch on top of the merge commit so the breakage window is small as I'd rather not rewrite the series just for this. Thanks.
Ahh, OK. So, how about this ? it is including both opinion :) I'm not sure which one should be applied, but both can solve my issue.
-------------------- From e75755351963c9b70ff3484f3088c4ed76d9d196 Mon Sep 17 00:00:00 2001 From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Date: Thu, 7 Mar 2019 09:05:51 +0900 Subject: [PATCH] clk: fixup default index for of_clk_get_by_name()
of_clk_get_by_name() is using -1 for __of_clk_get() index. It will goes to of_parse_clkspec(), and be used for of_parse_phandle_with_args(). Here, if user doesn't specified clock name (= of_clk_get_by_name(np, NULL)), this index is still -1, and of_parse_phandle_with_args() will return -EINVAL (This index will be updated if if it had clock name).
OTAH, nobody would call of_clk_get_by_name() unless they wanted to find some clk that had a matching name. But we also allow NULL to be passed as the name to mean the typical "wildcard match" thing.
This means default index should be 0 instead of -1 if name was NULL. This patch fixup it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- drivers/clk/clk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 14cbf23..66c71e9 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -4210,10 +4210,12 @@ EXPORT_SYMBOL(of_clk_get); */ struct clk *of_clk_get_by_name(struct device_node *np, const char *name) { + int index = name ? -1 : 0; + if (!np) return ERR_PTR(-ENOENT);
- return __of_clk_get(np, -1, np->full_name, name); + return __of_clk_get(np, index, np->full_name, name); } EXPORT_SYMBOL(of_clk_get_by_name);
Quoting Kuninori Morimoto (2019-03-06 16:20:40)
Hi Stephen
index 14cbf23..96053a9 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -4213,7 +4213,7 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name) if (!np) return ERR_PTR(-ENOENT);
return __of_clk_get(np, -1, np->full_name, name);
return __of_clk_get(np, 0, np->full_name, name);}
Yes this is correct. Thanks for debugging and fixing my thinko here. I was thinking that nobody would call of_clk_get_by_name() unless they wanted to find some clk that had a matching name, but it seems that we also allow NULL to be passed as the name to mean the typical "wildcard match" thing that clkdev has done for years. I'll throw this patch on top of the merge commit so the breakage window is small as I'd rather not rewrite the series just for this. Thanks.
Ahh, OK. So, how about this ? it is including both opinion :) I'm not sure which one should be applied, but both can solve my issue.
No worries. I applied your original patch.
Hi Stephen
Yes this is correct. Thanks for debugging and fixing my thinko here. I was thinking that nobody would call of_clk_get_by_name() unless they wanted to find some clk that had a matching name, but it seems that we also allow NULL to be passed as the name to mean the typical "wildcard match" thing that clkdev has done for years. I'll throw this patch on top of the merge commit so the breakage window is small as I'd rather not rewrite the series just for this. Thanks.
Ahh, OK. So, how about this ? it is including both opinion :) I'm not sure which one should be applied, but both can solve my issue.
No worries. I applied your original patch.
OK. Thanks a lot !!
Best regards --- Kuninori Morimoto
participants (2)
-
Kuninori Morimoto -
Stephen Boyd