[PATCH] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[PATCH] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[Viresh Kumar]

From: Russell King <rmk+kernel@arm.linux.org.uk>

With common clock framework, clks are allocated at runtime. Some of them require
clkdevs to be allocated and added in global clkdev list.

This patch introduces helper routines to:

 - allocate and add single clkdev for a single clk structure.
 - add multiple clkdevs for a single clk structure.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/clk/clkdev.c   |   33 +++++++++++++++++++++++++++++++++
 include/linux/clkdev.h |    3 +++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 6db161f..6a98177 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -173,3 +173,36 @@ void clkdev_drop(struct clk_lookup *cl)
 	kfree(cl);
 }
 EXPORT_SYMBOL(clkdev_drop);
+
+int clk_register_single_clkdev(struct clk *clk, const char *dev_id,
+		const char *con_id)
+{
+	struct clk_lookup *cl;
+
+	if (!clk || (!dev_id && !con_id))
+		return -ENOMEM;
+
+	cl = clkdev_alloc(clk, con_id, "%s", dev_id);
+	if (!cl)
+		return -ENOMEM;
+
+	clkdev_add(cl);
+	return 0;
+}
+EXPORT_SYMBOL(clk_register_single_clkdev);
+
+int clk_register_clkdevs(struct clk *clk, struct clk_lookup *cl, size_t num)
+{
+	unsigned i;
+
+	if (!clk || !cl || !num)
+		return -ENOMEM;
+
+	for (i = 0; i < num; i++, cl++) {
+		cl->clk = clk;
+		clkdev_add(cl);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(clk_register_clkdevs);
diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h
index d9a4fd0..3579e0a 100644
--- a/include/linux/clkdev.h
+++ b/include/linux/clkdev.h
@@ -39,5 +39,8 @@ void clkdev_drop(struct clk_lookup *cl);
 
 void clkdev_add_table(struct clk_lookup *, size_t);
 int clk_add_alias(const char *, const char *, char *, struct device *);
+int clk_register_single_clkdev(struct clk *clk, const char *dev_id,
+		const char *con_id);
+int clk_register_clkdevs(struct clk *clk, struct clk_lookup *cl, size_t num);
 
 #endif
-- 
1.7.9

[PATCH] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[Domenico Andreoli]

On Mon, Apr 16, 2012 at 10:49:37AM +0530, Viresh Kumar wrote:
> From: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> With common clock framework, clks are allocated at runtime. Some of them require
> clkdevs to be allocated and added in global clkdev list.
> 
> This patch introduces helper routines to:
> 
>  - allocate and add single clkdev for a single clk structure.
>  - add multiple clkdevs for a single clk structure.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
>  drivers/clk/clkdev.c   |   33 +++++++++++++++++++++++++++++++++
>  include/linux/clkdev.h |    3 +++
>  2 files changed, 36 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 6db161f..6a98177 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -173,3 +173,36 @@ void clkdev_drop(struct clk_lookup *cl)
>  	kfree(cl);
>  }
>  EXPORT_SYMBOL(clkdev_drop);
> +
> +int clk_register_single_clkdev(struct clk *clk, const char *dev_id,
> +		const char *con_id)
> +{
> +	struct clk_lookup *cl;
> +
> +	if (!clk || (!dev_id && !con_id))
> +		return -ENOMEM;

I would return -EINVAL here.

> +
> +	cl = clkdev_alloc(clk, con_id, "%s", dev_id);

clkdev_alloc() allows you to specify dev_fmt and possibly other arguments
to build the dev_id on the fly, could clk_register_single_clkdev()
preserve this ability?

> +	if (!cl)
> +		return -ENOMEM;
> +
> +	clkdev_add(cl);
> +	return 0;
> +}
> +EXPORT_SYMBOL(clk_register_single_clkdev);
> +
> +int clk_register_clkdevs(struct clk *clk, struct clk_lookup *cl, size_t num)
> +{
> +	unsigned i;
> +
> +	if (!clk || !cl || !num)
> +		return -ENOMEM;

I would return -EINVAL here as well.

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

cheers,
Domenico

[PATCH] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[Viresh Kumar]

On 4/16/2012 3:55 PM, Domenico Andreoli wrote:
> On Mon, Apr 16, 2012 at 10:49:37AM +0530, Viresh Kumar wrote:
>> From: Russell King <rmk+kernel@arm.linux.org.uk>
>> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
>> +int clk_register_single_clkdev(struct clk *clk, const char *dev_id,
>> +		const char *con_id)
>> +{
>> +	struct clk_lookup *cl;
>> +
>> +	if (!clk || (!dev_id && !con_id))
>> +		return -ENOMEM;
> 
> I would return -EINVAL here.

Will fix it.

>> +
>> +	cl = clkdev_alloc(clk, con_id, "%s", dev_id);
> 
> clkdev_alloc() allows you to specify dev_fmt and possibly other arguments
> to build the dev_id on the fly, could clk_register_single_clkdev()
> preserve this ability?

Can be done. Maybe we can create two versions here: with and without dev_fmt.

>> +	if (!cl)
>> +		return -ENOMEM;
>> +
>> +	clkdev_add(cl);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(clk_register_single_clkdev);
>> +
>> +int clk_register_clkdevs(struct clk *clk, struct clk_lookup *cl, size_t num)
>> +{
>> +	unsigned i;
>> +
>> +	if (!clk || !cl || !num)
>> +		return -ENOMEM;
> 
> I would return -EINVAL here as well.

Will fix this too.

-- 
viresh

[PATCH] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[Russell King - ARM Linux]

On Mon, Apr 16, 2012 at 04:00:53PM +0530, Viresh Kumar wrote:
> On 4/16/2012 3:55 PM, Domenico Andreoli wrote:
> > On Mon, Apr 16, 2012 at 10:49:37AM +0530, Viresh Kumar wrote:
> >> From: Russell King <rmk+kernel@arm.linux.org.uk>
> >> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> >> +int clk_register_single_clkdev(struct clk *clk, const char *dev_id,
> >> +		const char *con_id)
> >> +{
> >> +	struct clk_lookup *cl;
> >> +
> >> +	if (!clk || (!dev_id && !con_id))
> >> +		return -ENOMEM;
> > 
> > I would return -EINVAL here.
> 
> Will fix it.

Do we actually need this kind of check?

> >> +int clk_register_clkdevs(struct clk *clk, struct clk_lookup *cl, size_t num)
> >> +{
> >> +	unsigned i;
> >> +
> >> +	if (!clk || !cl || !num)
> >> +		return -ENOMEM;
> > 
> > I would return -EINVAL here as well.
> 
> Will fix this too.

Ditto.

I don't think these checks actually help anyone, especially if the user
forgets to check the return value (which makes them silent errors.)

If you're going to abuse the interface by passing a NULL clk_lookup or
num=0 then you deserve to get a big fat oops to tell you that you messed
up.  Same for NULL dev_id and con_id above.

Checking for NULL clk (or IS_ERR(clk)) and returning -ENOMEM does make
sense as I mentioned in my original proposal (it allows you to pass the
returned value from clk_register() directly to this function without
further checking, and you get the right error code.

[PATCH] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[Viresh Kumar]

On 4/16/2012 4:08 PM, Russell King - ARM Linux wrote:
> I don't think these checks actually help anyone, especially if the user
> forgets to check the return value (which makes them silent errors.)
> 
> If you're going to abuse the interface by passing a NULL clk_lookup or
> num=0 then you deserve to get a big fat oops to tell you that you messed
> up.  Same for NULL dev_id and con_id above.
> 
> Checking for NULL clk (or IS_ERR(clk)) and returning -ENOMEM does make
> sense as I mentioned in my original proposal (it allows you to pass the
> returned value from clk_register() directly to this function without
> further checking, and you get the right error code.

Got it.

-- 
viresh

[PATCH] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[Domenico Andreoli]

On Mon, Apr 16, 2012 at 11:38:22AM +0100, Russell King - ARM Linux wrote:
> On Mon, Apr 16, 2012 at 04:00:53PM +0530, Viresh Kumar wrote:
> > On 4/16/2012 3:55 PM, Domenico Andreoli wrote:
> > > On Mon, Apr 16, 2012 at 10:49:37AM +0530, Viresh Kumar wrote:
> > >> From: Russell King <rmk+kernel@arm.linux.org.uk>
> > >> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> > >> +int clk_register_single_clkdev(struct clk *clk, const char *dev_id,
> > >> +		const char *con_id)
> > >> +{
> > >> +	struct clk_lookup *cl;
> > >> +
> > >> +	if (!clk || (!dev_id && !con_id))
> > >> +		return -ENOMEM;
> > > 
> > > I would return -EINVAL here.
> > 
> > Will fix it.
> 
> Do we actually need this kind of check?
> 
> > >> +int clk_register_clkdevs(struct clk *clk, struct clk_lookup *cl, size_t num)
> > >> +{
> > >> +	unsigned i;
> > >> +
> > >> +	if (!clk || !cl || !num)
> > >> +		return -ENOMEM;
> > > 
> > > I would return -EINVAL here as well.
> > 
> > Will fix this too.
> 
> Ditto.
> 
> I don't think these checks actually help anyone, especially if the user
> forgets to check the return value (which makes them silent errors.)
> 
> If you're going to abuse the interface by passing a NULL clk_lookup or
> num=0 then you deserve to get a big fat oops to tell you that you messed
> up.  Same for NULL dev_id and con_id above.

I hope for a BUG_ON then.

cheers,
Domenico

[PATCH] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[Viresh Kumar]

On 4/16/2012 4:26 PM, Domenico Andreoli wrote:
>> > If you're going to abuse the interface by passing a NULL clk_lookup or
>> > num=0 then you deserve to get a big fat oops to tell you that you messed
>> > up.  Same for NULL dev_id and con_id above.
> I hope for a BUG_ON then.

Would be better if we place that in clkdev_alloc(), instead of this one.

-- 
viresh

[PATCH] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[Russell King - ARM Linux]

On Mon, Apr 16, 2012 at 12:56:48PM +0200, Domenico Andreoli wrote:
> > I don't think these checks actually help anyone, especially if the user
> > forgets to check the return value (which makes them silent errors.)
> > 
> > If you're going to abuse the interface by passing a NULL clk_lookup or
> > num=0 then you deserve to get a big fat oops to tell you that you messed
> > up.  Same for NULL dev_id and con_id above.
> 
> I hope for a BUG_ON then.

Would you also like all the standard functions such as strcpy() to also
check for NULL pointers just in case you want it to copy your string to
the NULL pointer as well?

This is utterly rediculous.  If your programming ability is soo poor that
you need this kind of help (because you think you might abuse interfaces
by doing really stupid stuff like passing NULL clk_lookup pointers to
functions which can only ever take a valid pointer) then you shouldn't be
touching the kernel.

[PATCH] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[Domenico Andreoli]

On Mon, Apr 16, 2012 at 1:02 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Apr 16, 2012 at 12:56:48PM +0200, Domenico Andreoli wrote:
>> > I don't think these checks actually help anyone, especially if the user
>> > forgets to check the return value (which makes them silent errors.)
>> >
>> > If you're going to abuse the interface by passing a NULL clk_lookup or
>> > num=0 then you deserve to get a big fat oops to tell you that you messed
>> > up. ?Same for NULL dev_id and con_id above.
>>
>> I hope for a BUG_ON then.
>
> Would you also like all the standard functions such as strcpy() to also
> check for NULL pointers just in case you want it to copy your string to
> the NULL pointer as well?
>
> This is utterly rediculous. ?If your programming ability is soo poor that
> you need this kind of help (because you think you might abuse interfaces
> by doing really stupid stuff like passing NULL clk_lookup pointers to
> functions which can only ever take a valid pointer) then you shouldn't be
> touching the kernel.

ok, you are for the total removal. got it now.

thanks,
Domenico

[PATCH] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[viresh kumar]

On 4/16/12, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> Checking for NULL clk (or IS_ERR(clk)) and returning -ENOMEM does make
> sense as I mentioned in my original proposal (it allows you to pass the
> returned value from clk_register() directly to this function without
> further checking, and you get the right error code.

V2:

From: Russell King <rmk+kernel@arm.linux.org.uk>
CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

With common clock framework, clks are allocated at runtime. Some of them require
clkdevs to be allocated and added in global clkdev list.

This patch introduces helper routines to:

 - allocate and add single clkdev for a single clk structure.
 - add multiple clkdevs for a single clk structure.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/clk/clkdev.c   |   64 +++++++++++++++++++++++++++++++++++++++++++----
 include/linux/clkdev.h |    3 ++
 2 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 6db161f..0a28374 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -116,8 +116,9 @@ struct clk_lookup_alloc {
 	char	con_id[MAX_CON_ID];
 };

-struct clk_lookup * __init_refok
-clkdev_alloc(struct clk *clk, const char *con_id, const char *dev_fmt, ...)
+static struct clk_lookup * __init_refok
+clkdev_alloc_valist(struct clk *clk, const char *con_id, const char *dev_fmt,
+		va_list ap)
 {
 	struct clk_lookup_alloc *cla;

@@ -132,16 +133,29 @@ clkdev_alloc(struct clk *clk, const char
*con_id, const char *dev_fmt, ...)
 	}

 	if (dev_fmt) {
-		va_list ap;
-
-		va_start(ap, dev_fmt);
 		vscnprintf(cla->dev_id, sizeof(cla->dev_id), dev_fmt, ap);
 		cla->cl.dev_id = cla->dev_id;
-		va_end(ap);
 	}

 	return &cla->cl;
 }
+
+struct clk_lookup * __init_refok
+clkdev_alloc(struct clk *clk, const char *con_id, const char *dev_fmt, ...)
+{
+	struct clk_lookup * cl;
+	va_list ap = NULL;
+
+	if (dev_fmt)
+		va_start(ap, dev_fmt);
+
+	cl = clkdev_alloc_valist(clk, con_id, dev_fmt, ap);
+
+	if (dev_fmt)
+		va_end(ap);
+
+	return cl;
+}
 EXPORT_SYMBOL(clkdev_alloc);

 int clk_add_alias(const char *alias, const char *alias_dev_name, char *id,
@@ -173,3 +187,41 @@ void clkdev_drop(struct clk_lookup *cl)
 	kfree(cl);
 }
 EXPORT_SYMBOL(clkdev_drop);
+
+int clk_register_single_clkdev(struct clk *clk, const char *con_id,
+		const char *dev_fmt, ...)
+{
+	struct clk_lookup * cl;
+	va_list ap = NULL;
+
+	if (dev_fmt)
+		va_start(ap, dev_fmt);
+
+	cl = clkdev_alloc_valist(clk, con_id, dev_fmt, ap);
+
+	if (dev_fmt)
+		va_end(ap);
+
+	if (!cl)
+		return -ENOMEM;
+
+	clkdev_add(cl);
+	return 0;
+}
+EXPORT_SYMBOL(clk_register_single_clkdev);
+
+int clk_register_clkdevs(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;
+}
+EXPORT_SYMBOL(clk_register_clkdevs);
diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h
index d9a4fd0..49f8bd4 100644
--- a/include/linux/clkdev.h
+++ b/include/linux/clkdev.h
@@ -39,5 +39,8 @@ void clkdev_drop(struct clk_lookup *cl);

 void clkdev_add_table(struct clk_lookup *, size_t);
 int clk_add_alias(const char *, const char *, char *, struct device *);
+int clk_register_single_clkdev(struct clk *clk, const char *con_id,
+		const char *dev_fmt, ...);
+int clk_register_clkdevs(struct clk *clk, struct clk_lookup *cl, size_t num);

 #endif
-- 
1.7.9

[PATCH] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[Domenico Andreoli]

On Mon, Apr 16, 2012 at 07:09:32PM +0530, viresh kumar wrote:
> V2:
> 
> From: Russell King <rmk+kernel@arm.linux.org.uk>
> CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *
> 
> With common clock framework, clks are allocated at runtime. Some of them require
> clkdevs to be allocated and added in global clkdev list.
> 
> This patch introduces helper routines to:
> 
>  - allocate and add single clkdev for a single clk structure.
>  - add multiple clkdevs for a single clk structure.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
>  drivers/clk/clkdev.c   |   64 +++++++++++++++++++++++++++++++++++++++++++----
>  include/linux/clkdev.h |    3 ++
>  2 files changed, 61 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 6db161f..0a28374 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -116,8 +116,9 @@ struct clk_lookup_alloc {
>  	char	con_id[MAX_CON_ID];
>  };
> 
> -struct clk_lookup * __init_refok
> -clkdev_alloc(struct clk *clk, const char *con_id, const char *dev_fmt, ...)
> +static struct clk_lookup * __init_refok
> +clkdev_alloc_valist(struct clk *clk, const char *con_id, const char *dev_fmt,
> +		va_list ap)
>  {
>  	struct clk_lookup_alloc *cla;
> 
> @@ -132,16 +133,29 @@ clkdev_alloc(struct clk *clk, const char
> *con_id, const char *dev_fmt, ...)
>  	}
> 
>  	if (dev_fmt) {
> -		va_list ap;
> -
> -		va_start(ap, dev_fmt);
>  		vscnprintf(cla->dev_id, sizeof(cla->dev_id), dev_fmt, ap);
>  		cla->cl.dev_id = cla->dev_id;
> -		va_end(ap);
>  	}
> 
>  	return &cla->cl;
>  }
> +
> +struct clk_lookup * __init_refok
> +clkdev_alloc(struct clk *clk, const char *con_id, const char *dev_fmt, ...)
> +{
> +	struct clk_lookup * cl;
> +	va_list ap = NULL;

gcc complains because of this initialization:

drivers/clk/clkdev.c: In function 'clkdev_alloc':
drivers/clk/clkdev.c:149:2: error: invalid initializer
drivers/clk/clkdev.c: In function 'clk_register_single_clkdev':
drivers/clk/clkdev.c:197:2: error: invalid initializer
make[2]: *** [drivers/clk/clkdev.o] Error 1
make[1]: *** [drivers/clk] Error 2


so I finally tested it with this modification:

	if (dev_fmt) {
		va_start(ap, dev_fmt);
		cl = clkdev_alloc_valist(clk, con_id, dev_fmt, ap);
		va_end(ap);
	} else
		cl = clkdev_alloc(clk, con_id, NULL);

> +
> +	return cl;
> +}
>  EXPORT_SYMBOL(clkdev_alloc);
> 
>  int clk_add_alias(const char *alias, const char *alias_dev_name, char *id,
> @@ -173,3 +187,41 @@ void clkdev_drop(struct clk_lookup *cl)
>  	kfree(cl);
>  }
>  EXPORT_SYMBOL(clkdev_drop);
> +
> +int clk_register_single_clkdev(struct clk *clk, const char *con_id,
> +		const char *dev_fmt, ...)
> +{
> +	struct clk_lookup * cl;
> +	va_list ap = NULL;
> +
> +	if (dev_fmt)
> +		va_start(ap, dev_fmt);
> +
> +	cl = clkdev_alloc_valist(clk, con_id, dev_fmt, ap);
> +
> +	if (dev_fmt)
> +		va_end(ap);
> +
> +	if (!cl)
> +		return -ENOMEM;

same as above

I'm not a C legal, why this error? Thanks.

cheers,
Domenico

[PATCH] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[s.hauer at pengutronix.de]

On Mon, Apr 16, 2012 at 10:46:09PM +0200, Domenico Andreoli wrote:
> On Mon, Apr 16, 2012 at 07:09:32PM +0530, viresh kumar wrote:
> > V2:
> > 
> > From: Russell King <rmk+kernel@arm.linux.org.uk>
> > CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *
> > 
> > With common clock framework, clks are allocated at runtime. Some of them require
> > clkdevs to be allocated and added in global clkdev list.
> > 
> > This patch introduces helper routines to:
> > 
> >  - allocate and add single clkdev for a single clk structure.
> >  - add multiple clkdevs for a single clk structure.
> > 
> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> > ---
> > +struct clk_lookup * __init_refok
> > +clkdev_alloc(struct clk *clk, const char *con_id, const char *dev_fmt, ...)
> > +{
> > +	struct clk_lookup * cl;
> > +	va_list ap = NULL;
> 
> gcc complains because of this initialization:
> 
> drivers/clk/clkdev.c: In function 'clkdev_alloc':
> drivers/clk/clkdev.c:149:2: error: invalid initializer
> drivers/clk/clkdev.c: In function 'clk_register_single_clkdev':
> drivers/clk/clkdev.c:197:2: error: invalid initializer
> make[2]: *** [drivers/clk/clkdev.o] Error 1
> make[1]: *** [drivers/clk] Error 2
> 
> 
> so I finally tested it with this modification:
> 
> 	if (dev_fmt) {
> 		va_start(ap, dev_fmt);
> 		cl = clkdev_alloc_valist(clk, con_id, dev_fmt, ap);
> 		va_end(ap);
> 	} else
> 		cl = clkdev_alloc(clk, con_id, NULL);

Same here. The correct fix is simply not to initialize ap.

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] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[s.hauer at pengutronix.de]

On Mon, Apr 16, 2012 at 07:09:32PM +0530, viresh kumar wrote:
> On 4/16/12, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > Checking for NULL clk (or IS_ERR(clk)) and returning -ENOMEM does make
> > sense as I mentioned in my original proposal (it allows you to pass the
> > returned value from clk_register() directly to this function without
> > further checking, and you get the right error code.
> 
> V2:
> 
> From: Russell King <rmk+kernel@arm.linux.org.uk>
> CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *
> 
> With common clock framework, clks are allocated at runtime. Some of them require
> clkdevs to be allocated and added in global clkdev list.
> 
> This patch introduces helper routines to:
> 
>  - allocate and add single clkdev for a single clk structure.
>  - add multiple clkdevs for a single clk structure.
> 
> +
> +int clk_register_single_clkdev(struct clk *clk, const char *con_id,
> +		const char *dev_fmt, ...)

Can we drop the 'single' in the name? Otherwise it's quite a long
function name. I think name clk_register_clkdev makes it clear already
that we only register a single clkdev.

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] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[Domenico Andreoli]

On Mon, Apr 16, 2012 at 10:56:55PM +0200, s.hauer at pengutronix.de wrote:
> On Mon, Apr 16, 2012 at 07:09:32PM +0530, viresh kumar wrote:
> > On 4/16/12, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > > Checking for NULL clk (or IS_ERR(clk)) and returning -ENOMEM does make
> > > sense as I mentioned in my original proposal (it allows you to pass the
> > > returned value from clk_register() directly to this function without
> > > further checking, and you get the right error code.
> > 
> > V2:
> > 
> > From: Russell King <rmk+kernel@arm.linux.org.uk>
> > CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *
> > 
> > With common clock framework, clks are allocated at runtime. Some of them require
> > clkdevs to be allocated and added in global clkdev list.
> > 
> > This patch introduces helper routines to:
> > 
> >  - allocate and add single clkdev for a single clk structure.
> >  - add multiple clkdevs for a single clk structure.
> > 
> > +
> > +int clk_register_single_clkdev(struct clk *clk, const char *con_id,
> > +		const char *dev_fmt, ...)
> 
> Can we drop the 'single' in the name? Otherwise it's quite a long
> function name. I think name clk_register_clkdev makes it clear already
> that we only register a single clkdev.

clkdev_register?

Dome

[PATCH] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[s.hauer at pengutronix.de]

On Mon, Apr 16, 2012 at 11:06:18PM +0200, Domenico Andreoli wrote:
> > >  - allocate and add single clkdev for a single clk structure.
> > >  - add multiple clkdevs for a single clk structure.
> > > 
> > > +
> > > +int clk_register_single_clkdev(struct clk *clk, const char *con_id,
> > > +		const char *dev_fmt, ...)
> > 
> > Can we drop the 'single' in the name? Otherwise it's quite a long
> > function name. I think name clk_register_clkdev makes it clear already
> > that we only register a single clkdev.
> 
> clkdev_register?

Maybe even this. I just though that it might be too easy to confuse with
clkdev_add (In fact I think clkdev_register would be a good name for
what clkdev_add currently does, but it's probably not worth the churn
changing this).

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] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[Viresh Kumar]

On 4/17/2012 2:16 AM, Domenico Andreoli wrote:
>> > +struct clk_lookup * __init_refok
>> > +clkdev_alloc(struct clk *clk, const char *con_id, const char *dev_fmt, ...)
>> > +{
>> > +	struct clk_lookup * cl;
>> > +	va_list ap = NULL;

> so I finally tested it with this modification:
> 
> 	if (dev_fmt) {
> 		va_start(ap, dev_fmt);
> 		cl = clkdev_alloc_valist(clk, con_id, dev_fmt, ap);
> 		va_end(ap);
> 	} else
> 		cl = clkdev_alloc(clk, con_id, NULL);

Isn't this a infinite loop now, when dev_fmt is passed as NULL?


-- 
viresh

[PATCH] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[Viresh Kumar]

On 4/17/2012 2:21 AM, s.hauer at pengutronix.de wrote:
>>> > > +struct clk_lookup * __init_refok
>>> > > +clkdev_alloc(struct clk *clk, const char *con_id, const char *dev_fmt, ...)
>>> > > +{
>>> > > +	struct clk_lookup * cl;
>>> > > +	va_list ap = NULL;

>> > so I finally tested it with this modification:
>> > 
>> > 	if (dev_fmt) {
>> > 		va_start(ap, dev_fmt);
>> > 		cl = clkdev_alloc_valist(clk, con_id, dev_fmt, ap);
>> > 		va_end(ap);
>> > 	} else
>> > 		cl = clkdev_alloc(clk, con_id, NULL);

> Same here. The correct fix is simply not to initialize ap.

As this will turn into an infinite loop, we can simple pass ap instead
of NULL in the earlier implementation. As clkdev_alloc_valist() always
checks dev_fmt before accessing ap, it will work fine.

-- 
viresh

[PATCH] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[Viresh Kumar]

On 4/17/2012 2:42 AM, s.hauer at pengutronix.de wrote:
>>> > > Can we drop the 'single' in the name? Otherwise it's quite a long
>>> > > function name. I think name clk_register_clkdev makes it clear already
>>> > > that we only register a single clkdev.
>> > 
>> > clkdev_register?
> Maybe even this. I just though that it might be too easy to confuse with
> clkdev_add (In fact I think clkdev_register would be a good name for
> what clkdev_add currently does, but it's probably not worth the churn
> changing this).

I will keep clk_register_clkdev() and clk_register_clkdevs().

-- 
viresh

[PATCH V3] CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

[Viresh Kumar]

From: Russell King <rmk+kernel@arm.linux.org.uk>

With common clock framework, clks are allocated at runtime. Some of them require
clkdevs to be allocated and added in global clkdev list.

This patch introduces helper routines to:

 - allocate and add single clkdev for a single clk structure.
 - add multiple clkdevs for a single clk structure.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
Arnd,

If this patch is acceptable to everybody, can you push it for v3.5?

V2->V3:
- Don't initialize ap to NULL, as this gives warnings on some compilers
- Create macro clkdev_alloc_valist() to avoid duplicate code.

 drivers/clk/clkdev.c   |   64 +++++++++++++++++++++++++++++++++++++++++++----
 include/linux/clkdev.h |    3 ++
 2 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 6db161f..9d93d27 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -116,8 +116,9 @@ struct clk_lookup_alloc {
 	char	con_id[MAX_CON_ID];
 };
 
-struct clk_lookup * __init_refok
-clkdev_alloc(struct clk *clk, const char *con_id, const char *dev_fmt, ...)
+static struct clk_lookup * __init_refok
+__clkdev_alloc_valist(struct clk *clk, const char *con_id, const char *dev_fmt,
+		va_list ap)
 {
 	struct clk_lookup_alloc *cla;
 
@@ -132,16 +133,35 @@ clkdev_alloc(struct clk *clk, const char *con_id, const char *dev_fmt, ...)
 	}
 
 	if (dev_fmt) {
-		va_list ap;
-
-		va_start(ap, dev_fmt);
 		vscnprintf(cla->dev_id, sizeof(cla->dev_id), dev_fmt, ap);
 		cla->cl.dev_id = cla->dev_id;
-		va_end(ap);
 	}
 
 	return &cla->cl;
 }
+
+#define clkdev_alloc_valist(_cl, _clk, _con_id, _dev_fmt, _ap)		\
+	do {								\
+		if (_dev_fmt)						\
+			va_start(_ap, _dev_fmt);			\
+									\
+		_cl = __clkdev_alloc_valist(_clk, _con_id, _dev_fmt,	\
+				_ap);					\
+									\
+		if (_dev_fmt)						\
+			va_end(ap);					\
+	} while (0);
+
+struct clk_lookup * __init_refok
+clkdev_alloc(struct clk *clk, const char *con_id, const char *dev_fmt, ...)
+{
+	struct clk_lookup *cl;
+	va_list ap;
+
+	clkdev_alloc_valist(cl, clk, con_id, dev_fmt, ap);
+
+	return cl;
+}
 EXPORT_SYMBOL(clkdev_alloc);
 
 int clk_add_alias(const char *alias, const char *alias_dev_name, char *id,
@@ -173,3 +193,35 @@ void clkdev_drop(struct clk_lookup *cl)
 	kfree(cl);
 }
 EXPORT_SYMBOL(clkdev_drop);
+
+int clk_register_clkdev(struct clk *clk, const char *con_id,
+		const char *dev_fmt, ...)
+{
+	struct clk_lookup *cl;
+	va_list ap;
+
+	clkdev_alloc_valist(cl, clk, con_id, dev_fmt, ap);
+
+	if (!cl)
+		return -ENOMEM;
+
+	clkdev_add(cl);
+	return 0;
+}
+EXPORT_SYMBOL(clk_register_clkdev);
+
+int clk_register_clkdevs(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;
+}
+EXPORT_SYMBOL(clk_register_clkdevs);
diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h
index d9a4fd0..01d8ff6 100644
--- a/include/linux/clkdev.h
+++ b/include/linux/clkdev.h
@@ -39,5 +39,8 @@ void clkdev_drop(struct clk_lookup *cl);
 
 void clkdev_add_table(struct clk_lookup *, size_t);
 int clk_add_alias(const char *, const char *, char *, struct device *);
+int clk_register_clkdev(struct clk *clk, const char *con_id,
+		const char *dev_fmt, ...);
+int clk_register_clkdevs(struct clk *clk, struct clk_lookup *cl, size_t num);
 
 #endif
-- 
1.7.9