[PATCH] clk: Allocate and add clock lookups from clk_register()

[PATCH] clk: Allocate and add clock lookups from clk_register()

[Viresh Kumar]

clock lookups are required for few clocks for which we do clk_get() or
clk_get_sys(). Adding clock lookups for them with new clock framework, would
mean a lot of code for each platform. Simplify this by doing this allocation and
addition to global list in clk_register() only.

This is done on request by the caller, i.e. only if user passes valid pointers
in "dev_id" OR "con_id" fields.

A lot of changes at other places/files/comments are also required, that i would
do separately. This is the initial version to get initial feedback.

Pointer to original discussion:
http://www.spinics.net/lists/arm-kernel/msg169133.html

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/clk/clk.c            |   33 ++++++++++++++++++++++++++-------
 include/linux/clk-provider.h |    4 +++-
 2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 2bcce5a..16ce136 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -9,6 +9,7 @@
  * Standard functionality for the common clock API.  See Documentation/clk.txt
  */
 
+#include <linux/clkdev.h>
 #include <linux/clk-private.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -1341,7 +1342,8 @@ out:
  */
 struct clk *clk_register(struct device *dev, const char *name,
 		const struct clk_ops *ops, struct clk_hw *hw,
-		const char **parent_names, u8 num_parents, unsigned long flags)
+		const char **parent_names, u8 num_parents, unsigned long flags,
+		struct clk_lookup **cl, const char *dev_id, const char *con_id)
 {
 	int i, ret = -ENOMEM;
 	struct clk *clk;
@@ -1365,7 +1367,7 @@ struct clk *clk_register(struct device *dev, const char *name,
 
 	if (!clk->parent_names) {
 		pr_err("%s: could not allocate clk->parent_names\n", __func__);
-		goto fail_parent_names;
+		goto free_clk;
 	}
 
 	/* copy each string name in case parent_names is __initdata */
@@ -1373,19 +1375,36 @@ struct clk *clk_register(struct device *dev, const char *name,
 		clk->parent_names[i] = kstrdup(parent_names[i], GFP_KERNEL);
 		if (!clk->parent_names[i]) {
 			pr_err("%s: could not copy parent_names\n", __func__);
-			goto fail_parent_names_copy;
+			goto free_parents;
 		}
 	}
 
 	ret = __clk_init(dev, clk);
-	if (!ret)
-		return clk;
+	if (ret)
+		goto free_parents;
+
+	if (dev_id || con_id) {
+		struct clk_lookup *lookup;
+
+		lookup = clkdev_alloc(clk, dev_id, con_id);
+		if (!lookup) {
+			ret = ENODEV;
+			goto free_parents;
+		}
+
+		clkdev_add(lookup);
+
+		if (cl)
+			*cl = lookup;
+	}
+
+	return clk;
 
-fail_parent_names_copy:
+free_parents:
 	while (--i >= 0)
 		kfree(clk->parent_names[i]);
 	kfree(clk->parent_names);
-fail_parent_names:
+free_clk:
 	kfree(clk);
 fail_out:
 	return ERR_PTR(ret);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 8bb93c4..a50119d 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -12,6 +12,7 @@
 #define __LINUX_CLK_PROVIDER_H
 
 #include <linux/clk.h>
+#include <linux/clkdev.h>
 
 #ifdef CONFIG_COMMON_CLK
 
@@ -281,7 +282,8 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
  */
 struct clk *clk_register(struct device *dev, const char *name,
 		const struct clk_ops *ops, struct clk_hw *hw,
-		const char **parent_names, u8 num_parents, unsigned long flags);
+		const char **parent_names, u8 num_parents, unsigned long flags,
+		struct clk_lookup **cl, const char *dev_id, const char *con_id);
 
 /* helper functions */
 const char *__clk_get_name(struct clk *clk);
-- 
1.7.9

[PATCH] clk: Allocate and add clock lookups from clk_register()

[Richard Zhao]

>  struct clk *clk_register(struct device *dev, const char *name,
>  		const struct clk_ops *ops, struct clk_hw *hw,
> -		const char **parent_names, u8 num_parents, unsigned long flags)
> +		const char **parent_names, u8 num_parents, unsigned long flags,
> +		struct clk_lookup **cl, const char *dev_id, const char *con_id)
10 arguments. It worths to use a struct ? 

Thanks
Richard

[PATCH] clk: Allocate and add clock lookups from clk_register()

[Viresh Kumar]

[Added you question from both threads here :) ]

On 4/13/2012 2:47 PM, Richard Zhao wrote:
> On Fri, Apr 13, 2012 at 02:40:12PM +0530, Viresh Kumar wrote:
>> On 4/13/2012 2:29 PM, Mark Brown wrote:
>>> Or we need to dump the arguments into a struct.
>>
>> Then we would end up creating too many structures in our mach/clock.c
>> files. That will look bad. May perform better. :)
> or pass struct clk_lookup* ? For non-leaf clks, it's NULL.

Non leaf nodes may also need to have clk_lookups, if you want to do
clk_get*() on them, to set their rate or parents.

On 4/13/2012 2:26 PM, Richard Zhao wrote:
>> >  struct clk *clk_register(struct device *dev, const char *name,
>> >  		const struct clk_ops *ops, struct clk_hw *hw,
>> > -		const char **parent_names, u8 num_parents, unsigned long flags)
>> > +		const char **parent_names, u8 num_parents, unsigned long flags,
>> > +		struct clk_lookup **cl, const char *dev_id, const char *con_id)
> 10 arguments. It worths to use a struct ? 

I am not saying no here.

But, with that we pass structs all clk_register_*() routines too. So, we will
create a lot of structures in our mach clock.c file.

Because in most cases this is one time stuff, only at boot, i vote for the
current implementation. :)

I am not an expert but, with my logic i feel with more args system will not
be slower. Performance wise, both should be same. As all args would always be
read from memory and not registers. In case of struct, only address of struct
will be read from cpu registers.

-- 
viresh

[PATCH] clk: Allocate and add clock lookups from clk_register()

[Sascha Hauer]

On Fri, Apr 13, 2012 at 12:04:57PM +0530, Viresh Kumar wrote:
> clock lookups are required for few clocks for which we do clk_get() or
> clk_get_sys(). Adding clock lookups for them with new clock framework, would
> mean a lot of code for each platform. Simplify this by doing this allocation and
> addition to global list in clk_register() only.
> 
> This is done on request by the caller, i.e. only if user passes valid pointers
> in "dev_id" OR "con_id" fields.
> 
> A lot of changes at other places/files/comments are also required, that i would
> do separately. This is the initial version to get initial feedback.
> 
> Pointer to original discussion:
> http://www.spinics.net/lists/arm-kernel/msg169133.html

For reasons mentioned in this thread (long argument list, line wraps in
clk_register calls, only leaf nodes need lookups) I don't think this is
a good idea.

>   */
>  struct clk *clk_register(struct device *dev, const char *name,
>  		const struct clk_ops *ops, struct clk_hw *hw,
> -		const char **parent_names, u8 num_parents, unsigned long flags)
> +		const char **parent_names, u8 num_parents, unsigned long flags,
> +		struct clk_lookup **cl, const char *dev_id, const char *con_id)
>  {

Also this doesn't handle the case when multiple lookups are associated
with a single clock (which is quite a common case on some SoCs). Yes,
we could still use the returned clk to register the additional lookups,
but that would make the advantage of this patch even smaller.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] clk: Allocate and add clock lookups from clk_register()

[Viresh Kumar]

On 4/13/2012 3:31 PM, Sascha Hauer wrote:
>> > Pointer to original discussion:
>> > http://www.spinics.net/lists/arm-kernel/msg169133.html
> For reasons mentioned in this thread (long argument list, line wraps in
> clk_register calls, only leaf nodes need lookups) I don't think this is
> a good idea.

For that we can pass struct as argument.

>> >   */
>> >  struct clk *clk_register(struct device *dev, const char *name,
>> >  		const struct clk_ops *ops, struct clk_hw *hw,
>> > -		const char **parent_names, u8 num_parents, unsigned long flags)
>> > +		const char **parent_names, u8 num_parents, unsigned long flags,
>> > +		struct clk_lookup **cl, const char *dev_id, const char *con_id)
>> >  {
> Also this doesn't handle the case when multiple lookups are associated
> with a single clock (which is quite a common case on some SoCs). Yes,
> we could still use the returned clk to register the additional lookups,
> but that would make the advantage of this patch even smaller.

If there is a better way of managing this within the clock framework,
without platforms having to do clkdev management, we can choose that.
My main idea was to make platform code lighter.

We can do one more thing here to support multiple lookups for same clocks:
We can pass array of struct having these three args as its fields.

-- 
viresh

[PATCH] clk: Allocate and add clock lookups from clk_register()

[Russell King - ARM Linux]

On Fri, Apr 13, 2012 at 03:40:44PM +0530, Viresh Kumar wrote:
> On 4/13/2012 3:31 PM, Sascha Hauer wrote:
> >> > Pointer to original discussion:
> >> > http://www.spinics.net/lists/arm-kernel/msg169133.html
> > For reasons mentioned in this thread (long argument list, line wraps in
> > clk_register calls, only leaf nodes need lookups) I don't think this is
> > a good idea.
> 
> For that we can pass struct as argument.
> 
> >> >   */
> >> >  struct clk *clk_register(struct device *dev, const char *name,
> >> >  		const struct clk_ops *ops, struct clk_hw *hw,
> >> > -		const char **parent_names, u8 num_parents, unsigned long flags)
> >> > +		const char **parent_names, u8 num_parents, unsigned long flags,
> >> > +		struct clk_lookup **cl, const char *dev_id, const char *con_id)
> >> >  {
> > Also this doesn't handle the case when multiple lookups are associated
> > with a single clock (which is quite a common case on some SoCs). Yes,
> > we could still use the returned clk to register the additional lookups,
> > but that would make the advantage of this patch even smaller.
> 
> If there is a better way of managing this within the clock framework,
> without platforms having to do clkdev management, we can choose that.
> My main idea was to make platform code lighter.
> 
> We can do one more thing here to support multiple lookups for same clocks:
> We can pass array of struct having these three args as its fields.

You can do something else too:

int clk_register_clkdev(struct clk *clk, struct clk_lookup *cl, size_t num)
{
	unsigned i;

	if (!clk)
		return -ENOMEM;

	for (i = 0; i < num; i++, cl++) {
		cl->clk = clk;
		clkdev_add(cl);
	}
	return 0;
}

and:

int clk_register_single_clkdev(struct clk *clk, const char *devname, const char *conname)
{
	struct clk_lookup *cl;

	if (!clk)
		return -ENOMEM;

	cl = clkdev_alloc(clk, conname, "%s", devname);
	if (!cl)
		return -ENOMEM;
	clkdev_add(cl);
	return 0;
}

and do:

static struct clk_lookup clk_foo_lookups[] = {
	{
		.dev_name = foo,
		.con_name = bar,
	}, {
		...
	},
};

	clk = clk_register(dev, NULL, &ops, &hw, NULL, 0, 0);
	ret = clk_register_clkdev(clk, clk_foo_lookups, ARRAY_SIZE(clk_foo_lookups));
	if (ret)
		...

and:

	clk = clk_register(dev, NULL, &ops, &hw, NULL, 0, 0);
	ret = clk_register_simple_clkdev(clk, "devname", "conname");
	if (ret)
		...

which are both much easier to use than a function taking some 11
arguments, and doesn't require each and every clk_register*() function
to be modified to take the clkdev stuff.

[PATCH] clk: Allocate and add clock lookups from clk_register()

[Domenico Andreoli]

On Fri, Apr 13, 2012 at 12:04:57PM +0530, Viresh Kumar wrote:
> clock lookups are required for few clocks for which we do clk_get() or
> clk_get_sys(). Adding clock lookups for them with new clock framework, would
> mean a lot of code for each platform. Simplify this by doing this allocation and
> addition to global list in clk_register() only.
> 
> This is done on request by the caller, i.e. only if user passes valid pointers
> in "dev_id" OR "con_id" fields.
> 
> A lot of changes at other places/files/comments are also required, that i would
> do separately. This is the initial version to get initial feedback.
> 
> Pointer to original discussion:
> http://www.spinics.net/lists/arm-kernel/msg169133.html
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
>  drivers/clk/clk.c            |   33 ++++++++++++++++++++++++++-------
>  include/linux/clk-provider.h |    4 +++-
>  2 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 2bcce5a..16ce136 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1373,19 +1375,36 @@ struct clk *clk_register(struct device *dev, const char *name,
>  		}
>  	}
>  
>  	ret = __clk_init(dev, clk);
> -	if (!ret)
> -		return clk;
> +	if (ret)
> +		goto free_parents;
> +
> +	if (dev_id || con_id) {
> +		struct clk_lookup *lookup;
> +
> +		lookup = clkdev_alloc(clk, dev_id, con_id);
> +		if (!lookup) {
> +			ret = ENODEV;
> +			goto free_parents;
> +		}
> +
> +		clkdev_add(lookup);
> +
> +		if (cl)
> +			*cl = lookup;
> +	}
> +
> +	return clk;

Basically this is the amount of code you want to save all around, right?

Why can't be this moved in a different function? So only who wants to
register the lookup will invoke it after clk_register().

cheers,
Domenico

[PATCH] clk: Allocate and add clock lookups from clk_register()

[Viresh Kumar]

On 4/13/2012 4:46 PM, Domenico Andreoli wrote:
> Basically this is the amount of code you want to save all around, right?
> 
> Why can't be this moved in a different function? So only who wants to
> register the lookup will invoke it after clk_register().

Yes, this is discussed at the end of following thread too. :)

http://www.spinics.net/lists/arm-kernel/msg169133.html

-- 
viresh