[alsa-devel] [PATCH v2] clkdev: add devm_of_clk_get()
This is based on devm_clk_get() Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> --- v1 -> v2 - added "static" and "inline" on header drivers/clk/clkdev.c | 26 ++++++++++++++++++++++++++ include/linux/clk.h | 7 +++++++ 2 files changed, 33 insertions(+) diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c index 89cc700..93a613b 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -55,6 +55,32 @@ struct clk *of_clk_get(struct device_node *np, int index) } EXPORT_SYMBOL(of_clk_get); +static void devm_of_clk_release(struct device *dev, void *res) +{ + clk_put(*(struct clk **)res); +} + +struct clk *devm_of_clk_get(struct device *dev, + struct device_node *np, int index) +{ + struct clk **ptr, *clk; + + ptr = devres_alloc(devm_of_clk_release, sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return ERR_PTR(-ENOMEM); + + clk = of_clk_get(np, index); + if (!IS_ERR(clk)) { + *ptr = clk; + devres_add(dev, ptr); + } else { + devres_free(ptr); + } + + return clk; +} +EXPORT_SYMBOL(devm_of_clk_get); + static struct clk *__of_clk_get_by_name(struct device_node *np, const char *dev_id, const char *name) diff --git a/include/linux/clk.h b/include/linux/clk.h index a89ba4e..33cd540 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -502,6 +502,8 @@ struct of_phandle_args; #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) struct clk *of_clk_get(struct device_node *np, int index); +struct clk *devm_of_clk_get(struct device *dev, + 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_from_provider(struct of_phandle_args *clkspec); #else @@ -509,6 +511,11 @@ static inline struct clk *of_clk_get(struct device_node *np, int index) { return ERR_PTR(-ENOENT); } +static inline struct clk *devm_of_clk_get(struct device *dev, + struct device_node *np, int index) +{ + return ERR_PTR(-ENOENT); +} static inline struct clk *of_clk_get_by_name(struct device_node *np, const char *name) { -- 1.9.1
Quoting Kuninori Morimoto (2016-07-03 18:36:50)
+struct clk *devm_of_clk_get(struct device *dev, + struct device_node *np, int index)
Any reason not to use devm_clk_get? Why do we need this helper? Thanks, Mike
+{ + struct clk **ptr, *clk; + + ptr = devres_alloc(devm_of_clk_release, sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return ERR_PTR(-ENOMEM); + + clk = of_clk_get(np, index); + if (!IS_ERR(clk)) { + *ptr = clk; + devres_add(dev, ptr); + } else { + devres_free(ptr); + } + + return clk; +} +EXPORT_SYMBOL(devm_of_clk_get); + static struct clk *__of_clk_get_by_name(struct device_node *np, const char *dev_id, const char *name) diff --git a/include/linux/clk.h b/include/linux/clk.h index a89ba4e..33cd540 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -502,6 +502,8 @@ struct of_phandle_args;
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) struct clk *of_clk_get(struct device_node *np, int index); +struct clk *devm_of_clk_get(struct device *dev, + 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_from_provider(struct of_phandle_args *clkspec); #else @@ -509,6 +511,11 @@ static inline struct clk *of_clk_get(struct device_node *np, int index) { return ERR_PTR(-ENOENT); } +static inline struct clk *devm_of_clk_get(struct device *dev, + struct device_node *np, int index) +{ + return ERR_PTR(-ENOENT); +} static inline struct clk *of_clk_get_by_name(struct device_node *np, const char *name) { -- 1.9.1
Hi Michael
+struct clk *devm_of_clk_get(struct device *dev, + struct device_node *np, int index)
Any reason not to use devm_clk_get? Why do we need this helper?
Because of_clk_get() can parse "clocks", "#clock-cells" on DT. And it can manage of_clk_provider. But devm_clk_get() can't ?
On Thu, Jul 07, 2016 at 09:54:03AM +0000, Kuninori Morimoto wrote:
Hi Michael
+struct clk *devm_of_clk_get(struct device *dev, + struct device_node *np, int index)
Any reason not to use devm_clk_get? Why do we need this helper?
Because of_clk_get() can parse "clocks", "#clock-cells" on DT.
clk_get() should also work just fine. clk_get() uses __of_clk_get_by_name() internally, which uses "clock-names" to locate the index if a connection id is given. of_clk_get() allows lookup of a clock by index only, omitting the name, which means you need to coordinate the order of clocks in DT with the order that the driver wants... which sounds error prone to me. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
Hi Russell
+struct clk *devm_of_clk_get(struct device *dev, + struct device_node *np, int index)
Any reason not to use devm_clk_get? Why do we need this helper?
Because of_clk_get() can parse "clocks", "#clock-cells" on DT.
clk_get() should also work just fine. clk_get() uses __of_clk_get_by_name() internally, which uses "clock-names" to locate the index if a connection id is given. of_clk_get() allows lookup of a clock by index only, omitting the name, which means you need to coordinate the order of clocks in DT with the order that the driver wants... which sounds error prone to me.
Thanks. But, I have 1 issue on [devm_]clk_get(). It can't select device_node on [devm_]clk_get(), it uses dev->of_node directly. struct clk *clk_get(struct device *dev, const char *con_id) { ... if (dev) { clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id); ~~~~~~~~~~~~ ... } } I would like to select specific device_node. sound_soc { ... cpu { ... => clocks = <&xxx>; }; codec { ... => clocks = <&xxx>; }; }; But, of_clk_get_by_name() / of_clk_get() doesn't have devm_xxx version
Quoting Kuninori Morimoto (2016-07-07 17:03:00)
Hi Russell
+struct clk *devm_of_clk_get(struct device *dev, + struct device_node *np, int index)
Any reason not to use devm_clk_get? Why do we need this helper?
Because of_clk_get() can parse "clocks", "#clock-cells" on DT.
clk_get() should also work just fine. clk_get() uses __of_clk_get_by_name() internally, which uses "clock-names" to locate the index if a connection id is given. of_clk_get() allows lookup of a clock by index only, omitting the name, which means you need to coordinate the order of clocks in DT with the order that the driver wants... which sounds error prone to me.
Thanks.
But, I have 1 issue on [devm_]clk_get(). It can't select device_node on [devm_]clk_get(), it uses dev->of_node directly.
struct clk *clk_get(struct device *dev, const char *con_id) { ... if (dev) { clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id); ~~~~~~~~~~~~ ... } }
I would like to select specific device_node.
Do you have access to the struct device that you want to target? Can you pass that device into either clk_get or devm_clk_get? Regards, Mike
sound_soc { ... cpu { ... => clocks = <&xxx>; };
codec { ... => clocks = <&xxx>; }; };
But, of_clk_get_by_name() / of_clk_get() doesn't have devm_xxx version
Hi Michael Thank you for your feedback
struct clk *clk_get(struct device *dev, const char *con_id) { ... if (dev) { clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id); ~~~~~~~~~~~~ ... } }
I would like to select specific device_node.
Do you have access to the struct device that you want to target? Can you pass that device into either clk_get or devm_clk_get?
If my understanding was correct, I think I can't. In below case, "sound_soc" has its *dev, but "cpu" and "codec" doesn't have *dev, it has node only. Thus, we are using of_clk_get() for these now. clk = of_clk_get(cpu, xxx); clk = of_clk_get(codec, xxx); sound_soc { ... cpu { ... => clocks = <&xxx>; }; codec { ... => clocks = <&xxx>; }; };
Hi Rob, Michael, Russell Cc Rob What is the conclusion of this patch ? We shouldn't add devm_of_clk_get() ? or I can continue ?
Thank you for your feedback
struct clk *clk_get(struct device *dev, const char *con_id) { ... if (dev) { clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id); ~~~~~~~~~~~~ ... } }
I would like to select specific device_node.
Do you have access to the struct device that you want to target? Can you pass that device into either clk_get or devm_clk_get?
If my understanding was correct, I think I can't. In below case, "sound_soc" has its *dev, but "cpu" and "codec" doesn't have *dev, it has node only. Thus, we are using of_clk_get() for these now.
clk = of_clk_get(cpu, xxx); clk = of_clk_get(codec, xxx);
sound_soc { ... cpu { ... => clocks = <&xxx>; }; codec { ... => clocks = <&xxx>; }; }; _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hi Rob, Michael, Russell Cc Rob What is the conclusion of this patch ? We shouldn't add devm_of_clk_get() ? or I can continue ?
Thank you for your feedback
struct clk *clk_get(struct device *dev, const char *con_id) { ... if (dev) { clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id); ~~~~~~~~~~~~ ... } }
I would like to select specific device_node.
Do you have access to the struct device that you want to target? Can you pass that device into either clk_get or devm_clk_get?
If my understanding was correct, I think I can't. In below case, "sound_soc" has its *dev, but "cpu" and "codec" doesn't have *dev, it has node only. Thus, we are using of_clk_get() for these now.
clk = of_clk_get(cpu, xxx); clk = of_clk_get(codec, xxx);
sound_soc { ... cpu { ... => clocks = <&xxx>; }; codec { ... => clocks = <&xxx>; }; }; _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hi Rob, Michael, Russell What is the conclusion of this patch ? We shouldn't add devm_of_clk_get() ? or can I continue ? The problem of current [devm_]clk_get() handles *dev only, but I need to get clocks from DT node, not dev sound_soc { ... cpu { ... => clocks = <&xxx>; }; codec { ... => clocks = <&xxx>; }; };
Thank you for your feedback
struct clk *clk_get(struct device *dev, const char *con_id) { ... if (dev) { clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id); ~~~~~~~~~~~~ ... } }
I would like to select specific device_node.
Do you have access to the struct device that you want to target? Can you pass that device into either clk_get or devm_clk_get?
If my understanding was correct, I think I can't. In below case, "sound_soc" has its *dev, but "cpu" and "codec" doesn't have *dev, it has node only. Thus, we are using of_clk_get() for these now.
clk = of_clk_get(cpu, xxx); clk = of_clk_get(codec, xxx);
sound_soc { ... cpu { ... => clocks = <&xxx>; }; codec { ... => clocks = <&xxx>; }; };
Best regards --- Kuninori Morimoto
On 11/16, Kuninori Morimoto wrote:
Hi Rob, Michael, Russell
What is the conclusion of this patch ? We shouldn't add devm_of_clk_get() ? or can I continue ?
The problem of current [devm_]clk_get() handles *dev only, but I need to get clocks from DT node, not dev
sound_soc { ... cpu { ... => clocks = <&xxx>; }; codec { ... => clocks = <&xxx>; }; };
I've seen bindings that have the 'clocks' property at the top level and the appropriate 'clock-names' property to relate the clocks to a subnode. sound_soc { clocks = <&xxx>, <&xxx>; clock-names = "cpu", "codec"; ... cpu { ... }; codec { ... }; }; Then the subnodes call clk_get() with the top level device and the name of their node and things match up. I suppose this binding is finalized though, so we can't really do that? I see that the gpio framework has a similar design called devm_get_gpiod_from_child(), so how about we add a devm_get_clk_from_child() API? That would more closely match the intent here, which is to restrict the clk_get() operation to child nodes of the device passed as the first argument. struct clk *devm_get_clk_from_child(struct device *dev, const char *con_id, struct device_node *child); -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Hi Stephen Thank you for your feedback
I've seen bindings that have the 'clocks' property at the top level and the appropriate 'clock-names' property to relate the clocks to a subnode.
sound_soc { clocks = <&xxx>, <&xxx>; clock-names = "cpu", "codec"; ... cpu { ... }; codec { ... }; };
Then the subnodes call clk_get() with the top level device and the name of their node and things match up. I suppose this binding is finalized though, so we can't really do that?
I see that the gpio framework has a similar design called devm_get_gpiod_from_child(), so how about we add a devm_get_clk_from_child() API? That would more closely match the intent here, which is to restrict the clk_get() operation to child nodes of the device passed as the first argument.
struct clk *devm_get_clk_from_child(struct device *dev, const char *con_id, struct device_node *child);
Thanks. I will check above 2 ideas. Best regards --- Kuninori Morimoto
Hi Stephen, again
I've seen bindings that have the 'clocks' property at the top level and the appropriate 'clock-names' property to relate the clocks to a subnode.
sound_soc { clocks = <&xxx>, <&xxx>; clock-names = "cpu", "codec"; ... cpu { ... }; codec { ... }; };
Then the subnodes call clk_get() with the top level device and the name of their node and things match up. I suppose this binding is finalized though, so we can't really do that?
I see that the gpio framework has a similar design called devm_get_gpiod_from_child(), so how about we add a devm_get_clk_from_child() API? That would more closely match the intent here, which is to restrict the clk_get() operation to child nodes of the device passed as the first argument.
struct clk *devm_get_clk_from_child(struct device *dev, const char *con_id, struct device_node *child);
Thanks, but, my point is that Linux already have "of_clk_get()", but we don't have its devm_ version. The point is that of_clk_get() can get clock from "device_node". Why having devm_ version become so problem ? Best regards --- Kuninori Morimoto
On 11/24, Kuninori Morimoto wrote:
Hi Stephen, again
I've seen bindings that have the 'clocks' property at the top level and the appropriate 'clock-names' property to relate the clocks to a subnode.
sound_soc { clocks = <&xxx>, <&xxx>; clock-names = "cpu", "codec"; ... cpu { ... }; codec { ... }; };
Then the subnodes call clk_get() with the top level device and the name of their node and things match up. I suppose this binding is finalized though, so we can't really do that?
I see that the gpio framework has a similar design called devm_get_gpiod_from_child(), so how about we add a devm_get_clk_from_child() API? That would more closely match the intent here, which is to restrict the clk_get() operation to child nodes of the device passed as the first argument.
struct clk *devm_get_clk_from_child(struct device *dev, const char *con_id, struct device_node *child);
Thanks, but, my point is that Linux already have "of_clk_get()", but we don't have its devm_ version. The point is that of_clk_get() can get clock from "device_node". Why having devm_ version become so problem ?
The problem is that it encourages the use of of_clk_get() when clk_get() is more desirable. Ideally of_clk_get() is never used when a device exists. In this case, it seems like we need to support it though, hence the suggestion of having a devm_get_clk_from_child() API, that explicitly reads as "get a clock from a child node of this device". The distinction is important, because of_clk_get() should rarely be used. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Hi Stephen Thank you for your feedback.
sound_soc { clocks = <&xxx>, <&xxx>; clock-names = "cpu", "codec"; ... cpu { ... }; codec { ... }; }; (snip) The problem is that it encourages the use of of_clk_get() when clk_get() is more desirable. Ideally of_clk_get() is never used when a device exists. In this case, it seems like we need to support it though, hence the suggestion of having a devm_get_clk_from_child() API, that explicitly reads as "get a clock from a child node of this device". The distinction is important, because of_clk_get() should rarely be used.
I understand your point, but I think devm_get_clk_from_child() needs new DT setings, and it can't keep compatibility, or it makes driver complex. I think it is nice to have. but, I want to keep current style. Thus, I will try to use current of_clk_get() as-is, and call clk_free() somehow in this driver.
Hi Stephen, again Can I confirm ?? Was I misunderstanding ??
I understand your point, but I think devm_get_clk_from_child() needs new DT setings, and it can't keep compatibility, or it makes driver complex. I think it is nice to have. but, I want to keep current style. Thus, I will try to use current of_clk_get() as-is, and call clk_free() somehow in this driver.
------ Pattern1 ----------- sound_soc { clocks = <&xxx>, <&xxx>; clock-names = "cpu", "codec"; ... cpu { /* of_cpu_node */ ... }; codec { /* of_codec_node */ ... }; }; ---------------------------- Do you mean, this case we can use devm_get_clk_from_child(dev, of_cpu_node, "cpu"); devm_get_clk_from_child(dev, of_codec_node, "codec"); ------ Pattern2 ----------- sound_soc { ... cpu { /* of_cpu_node */ clocks = <&xxx>; ... }; codec { /* of_codec_node */ clocks = <&xxx>; ... }; }; ---------------------------- And, this case, we can use devm_get_clk_from_child(dev, of_cpu_node, NULL); devm_get_clk_from_child(dev, of_codec_node, NULL); If so, I can use it without DT change.
participants (4)
-
Kuninori Morimoto -
Michael Turquette -
Russell King - ARM Linux -
Stephen Boyd