| Message ID | 87wpl2yyuw.wl%kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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);
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.
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.
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) {
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(+)