Skip to contentSkip to conversation

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

PATCH New Viresh Kumar 5 participants 19 messages Apr 16, 2012 – Apr 17, 2012 Arm/clkdev support

lore
Share & tools
lore

Viresh Kumar

author
#0 lore raw

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

From: Russell King <[email protected]>

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 <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/clk/clkdev.c   |   33 +++++++++++++++++++++++++++++++++
 include/linux/clkdev.h |    3 +++
 2 files changed, 36 insertions(+), 0 deletions(-)
drivers/clk/clkdev.c +33 -0
index 6db161f..6a98177 100644
@@ -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);
include/linux/clkdev.h +3 -0
index d9a4fd0..3579e0a 100644
@@ -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
Signature · 1 line
1.7.9
#1 lore raw

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

Quoted 23 lines · Viresh Kumar

From: Russell King <[email protected]>

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 <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/clk/clkdev.c   |   33 +++++++++++++++++++++++++++++++++
 include/linux/clkdev.h |    3 +++
 2 files changed, 36 insertions(+), 0 deletions(-)
drivers/clk/clkdev.c +0 -0
index 6db161f..6a98177 100644
@@ -173,3 +173,36 @@ void clkdev_drop(struct clk_lookup *cl)
 	kfree(cl);

drivers/clk/clkdev.c void clkdev_drop(struct clk_lookup *cl) · line 183

Quoted 10 lines
drivers/clk/clkdev.c +8 -0
 }
 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.

drivers/clk/clkdev.c void clkdev_drop(struct clk_lookup *cl) · line 185

Quoted 2 lines
drivers/clk/clkdev.c +2 -0
+
+	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?

Quoted 4 lines
drivers/clk/clkdev.c +4 -0
+	if (!cl)
+		return -ENOMEM;
+
+	clkdev_add(cl);

drivers/clk/clkdev.c void clkdev_drop(struct clk_lookup *cl) · line 199

Quoted 10 lines
drivers/clk/clkdev.c +10 -0
+	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.

drivers/clk/clkdev.c void clkdev_drop(struct clk_lookup *cl) · line 208

Quoted 9 lines
drivers/clk/clkdev.c +9 -0
+
+	for (i = 0; i < num; i++, cl++) {
+		cl->clk = clk;
+		clkdev_add(cl);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(clk_register_clkdevs);

cheers,
Domenico

author
#2 lore raw

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

Quoted 12 lines · Domenico Andreoli

drivers/clk/clkdev.c void clkdev_drop(struct clk_lookup *cl) · line 183

Quoted 9 lines · Viresh Kumar

From: Russell King <[email protected]>

drivers/clk/clkdev.c +7 -0
+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.

Quoted 6 lines

drivers/clk/clkdev.c

Quoted 2 lines
drivers/clk/clkdev.c +2 -0
+
+	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.

Quoted 8 lines
Quoted 8 lines
drivers/clk/clkdev.c +8 -0
+	if (!cl)
+		return -ENOMEM;
+
+	clkdev_add(cl);
+	return 0;
+}
+EXPORT_SYMBOL(clk_register_single_clkdev);
+
Quoted 8 lines

drivers/clk/clkdev.c void clkdev_drop(struct clk_lookup *cl) · line 199

Quoted 6 lines
drivers/clk/clkdev.c +6 -0
+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.

Signature · 1 line
viresh
#3 lore raw

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

Quoted 14 lines · Viresh Kumar
Quoted 12 lines · Domenico Andreoli

drivers/clk/clkdev.c void clkdev_drop(struct clk_lookup *cl) · line 183

Quoted 9 lines · Viresh Kumar

From: Russell King <[email protected]>

drivers/clk/clkdev.c +7 -0
+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.

Quoted 1 line

Will fix it.

Do we actually need this kind of check?

Quoted 10 lines
Quoted 8 lines

drivers/clk/clkdev.c void clkdev_drop(struct clk_lookup *cl) · line 199

Quoted 6 lines
drivers/clk/clkdev.c +6 -0
+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.

author
#4 lore raw

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

Quoted 11 lines · Russell King - ARM Linux

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.

Signature · 1 line
viresh
#5 lore raw

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

In reply to Russell King - ARM Linux
Quoted 30 lines · Russell King - ARM Linux
Quoted 14 lines · Viresh Kumar
Quoted 12 lines · Domenico Andreoli

drivers/clk/clkdev.c void clkdev_drop(struct clk_lookup *cl) · line 183

Quoted 9 lines · Viresh Kumar

From: Russell King <[email protected]>

drivers/clk/clkdev.c +7 -0
+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.

Quoted 1 line

Will fix it.

Do we actually need this kind of check?

Quoted 10 lines
Quoted 8 lines

drivers/clk/clkdev.c void clkdev_drop(struct clk_lookup *cl) · line 199

Quoted 6 lines
drivers/clk/clkdev.c +6 -0
+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.

Quoted 8 lines

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

author
#6 lore raw

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

Quoted 4 lines · Domenico Andreoli
Quoted 3 lines
Quoted 3 lines

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.

Signature · 1 line
viresh
#7 lore raw

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

In reply to Domenico Andreoli
Quoted 8 lines · Domenico Andreoli
Quoted 6 lines

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.

#8 lore raw

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

Quoted 14 lines · Russell King - ARM Linux
Quoted 8 lines · Domenico Andreoli
Quoted 6 lines

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?

Quoted 5 lines

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

#9 lore raw

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

In reply to Russell King - ARM Linux
Quoted 4 lines · Russell King - ARM Linux

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 <[email protected]>
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 <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/clk/clkdev.c   |   64 +++++++++++++++++++++++++++++++++++++++++++----
 include/linux/clkdev.h |    3 ++
 2 files changed, 61 insertions(+), 6 deletions(-)
drivers/clk/clkdev.c +3 -2
index 6db161f..0a28374 100644
@@ -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);

include/linux/clkdev.h +3 -0
index d9a4fd0..49f8bd4 100644
@@ -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
Signature · 1 line
1.7.9
#10 lore raw

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

Quoted 47 lines · viresh kumar

V2:

From: Russell King <[email protected]>
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 <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/clk/clkdev.c   |   64 +++++++++++++++++++++++++++++++++++++++++++----
 include/linux/clkdev.h |    3 ++
 2 files changed, 61 insertions(+), 6 deletions(-)
drivers/clk/clkdev.c +3 -6
index 6db161f..0a28374 100644
@@ -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);

drivers/clk/clkdev.c clkdev_alloc(struct clk *clk, const char · line 148

Quoted 10 lines
drivers/clk/clkdev.c +6 -0
 	}
 
 	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);
Quoted 17 lines
drivers/clk/clkdev.c +10 -0
+
+	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;
+

drivers/clk/clkdev.c void clkdev_drop(struct clk_lookup *cl) · line 206

Quoted 10 lines
drivers/clk/clkdev.c +10 -0
+	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

#11 lore raw

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

Quoted 33 lines · Domenico Andreoli
Quoted 16 lines · viresh kumar

V2:

From: Russell King <[email protected]>
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 <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>

---

Review

Quoted 5 lines
(quoted diff) +5 -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 = 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

Quoted 8 lines

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

Signature · 4 lines
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 |
author
#12 lore raw

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

Quoted 8 lines · s.hauer at pengutronix.de
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines

Review

Quoted 5 lines
(quoted diff) +5 -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 = NULL;

so I finally tested it with this modification:

Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
	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.

Signature · 1 line
viresh
author
#13 lore raw

PATCH v3 1/1 CLKDEV: Add helper routines to allocate and add clkdevs for given struct clk *

In reply to s.hauer at pengutronix.de

From: Russell King <[email protected]>

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 <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>

---
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(-)
drivers/clk/clkdev.c +58 -6
index 6db161f..9d93d27 100644
@@ -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);
include/linux/clkdev.h +3 -0
index d9a4fd0..01d8ff6 100644
@@ -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
Signature · 1 line
1.7.9
author
#14 lore raw

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

In reply to Domenico Andreoli
Quoted 8 lines · Domenico Andreoli
Quoted 5 lines

Review

Quoted 5 lines
(quoted diff) +5 -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 = NULL;

so I finally tested it with this modification:

Quoted 6 lines
	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?

Signature · 1 line
viresh
#15 lore raw

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

In reply to viresh kumar
Quoted 19 lines · viresh kumar
Quoted 4 lines · Russell King - ARM Linux

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 <[email protected]>
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.

Review

Quoted 3 lines
(quoted diff) +3 -0
+
+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

Signature · 4 lines
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 |
#16 lore raw

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

Quoted 20 lines · s.hauer at pengutronix.de
Quoted 19 lines · viresh kumar
Quoted 4 lines · Russell King - ARM Linux

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 <[email protected]>
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.

Quoted 7 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+
+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

#17 lore raw

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

Quoted 11 lines · Domenico Andreoli
Quoted 10 lines

Review

Quoted 6 lines

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

(quoted diff) +3 -0
+
+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.

Quoted 1 line

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

Signature · 4 lines
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 |
author
#18 lore raw

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

Quoted 9 lines · s.hauer at pengutronix.de
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Quoted 3 lines

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

Signature · 1 line
viresh