[PATCH v2] clkdev: add devm_of_clk_get()

[PATCH v2] clkdev: add devm_of_clk_get()

[Kuninori Morimoto]

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

Re: [PATCH v2] clkdev: add devm_of_clk_get()

[Michael Turquette]

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
> 

Re: [PATCH v2] clkdev: add devm_of_clk_get()

[Kuninori Morimoto]

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 ?

Re: [PATCH v2] clkdev: add devm_of_clk_get()

[Russell King - ARM Linux]

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.

Re: [PATCH v2] clkdev: add devm_of_clk_get()

[Kuninori Morimoto]

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

Re: [PATCH v2] clkdev: add devm_of_clk_get()

[Michael Turquette]

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

Re: [PATCH v2] clkdev: add devm_of_clk_get()

[Kuninori Morimoto]

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>;
		};
	};

Re: [alsa-devel] [PATCH v2] clkdev: add devm_of_clk_get()

[Kuninori Morimoto]

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

Re: [alsa-devel] [PATCH v2] clkdev: add devm_of_clk_get()

[Kuninori Morimoto]

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

Re: [alsa-devel] [PATCH v2] clkdev: add devm_of_clk_get()

[Kuninori Morimoto]

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

Re: [alsa-devel] [PATCH v2] clkdev: add devm_of_clk_get()

[Stephen Boyd]

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

Re: [alsa-devel] [PATCH v2] clkdev: add devm_of_clk_get()

[Kuninori Morimoto]

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

Re: [alsa-devel] [PATCH v2] clkdev: add devm_of_clk_get()

[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

Re: [alsa-devel] [PATCH v2] clkdev: add devm_of_clk_get()

[Stephen Boyd]

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

Re: [alsa-devel] [PATCH v2] clkdev: add devm_of_clk_get()

[Kuninori Morimoto]

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.

Re: [alsa-devel] [PATCH v2] clkdev: add devm_of_clk_get()

[Kuninori Morimoto]

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.