[PATCH v2] clkdev: add devm_of_clk_get()

[PATCH v2] clkdev: add devm_of_clk_get()

[Kuninori Morimoto]

Current Linux has of_clk_get(), but doesn't have devm_of_clk_get().
This patch adds it. This is based on devm_clk_get()

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

 - update git log

 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()

[Russell King - ARM Linux]

On Mon, Nov 28, 2016 at 06:56:52AM +0000, Kuninori Morimoto wrote:
> Current Linux has of_clk_get(), but doesn't have devm_of_clk_get().
> This patch adds it. This is based on devm_clk_get()
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Please put this in drivers/clk/clk-devres.c, where you'll find that
we have devm_clk_release() which is identical to your
devm_of_clk_release().  It'll also not need the dummy definition of
devm_of_clk_get().

Thanks.

> ---
> v1 -> v2
> 
>  - update git log
> 
>  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
> 

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

> > Current Linux has of_clk_get(), but doesn't have devm_of_clk_get().
> > This patch adds it. This is based on devm_clk_get()
> > 
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> 
> Please put this in drivers/clk/clk-devres.c, where you'll find that
> we have devm_clk_release() which is identical to your
> devm_of_clk_release().  It'll also not need the dummy definition of
> devm_of_clk_get().

OK, will do

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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