[PATCH] pinctrl: Quiet logging about missing DT nodes when not using DT

[PATCH] pinctrl: Quiet logging about missing DT nodes when not using DT

[Mark Brown]

From: Mark Brown <broonie@linaro.org>

On systems which were not booted using DT it is entirely unsurprising that
device nodes don't have any DT information and this is going to happen for
every single device in the system. Make pinctrl be less chatty about this
situation by only logging in the case where we have DT.

Signed-off-by: Mark Brown <broonie@linaro.org>
---
 drivers/pinctrl/devicetree.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index 340fb4e6c600..eda13de2e7c0 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -186,7 +186,9 @@ int pinctrl_dt_to_map(struct pinctrl *p)
 
 	/* CONFIG_OF enabled, p->dev not instantiated from DT */
 	if (!np) {
-		dev_dbg(p->dev, "no of_node; not parsing pinctrl DT\n");
+		if (of_have_populated_dt())
+			dev_dbg(p->dev,
+				"no of_node; not parsing pinctrl DT\n");
 		return 0;
 	}
 
-- 
1.9.rc1

Re: [PATCH] pinctrl: Quiet logging about missing DT nodes when not using DT

[Joe Perches]

On Thu, 2014-01-30 at 18:57 +0000, Mark Brown wrote:
> From: Mark Brown <broonie@linaro.org>
> 
> On systems which were not booted using DT it is entirely unsurprising that
> device nodes don't have any DT information and this is going to happen for
> every single device in the system. Make pinctrl be less chatty about this
> situation by only logging in the case where we have DT.

trivia:

> diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
[]
> @@ -186,7 +186,9 @@ int pinctrl_dt_to_map(struct pinctrl *p)
>  
>  	/* CONFIG_OF enabled, p->dev not instantiated from DT */
>  	if (!np) {
> -		dev_dbg(p->dev, "no of_node; not parsing pinctrl DT\n");
> +		if (of_have_populated_dt())
> +			dev_dbg(p->dev,
> +				"no of_node; not parsing pinctrl DT\n");

There are 80 columns.  It's OK to use them all.

			dev_dbg(p->dev, "no of_node; not parsing pinctrl DT\n");

[PATCH] pinctrl: Avoid kasprintf/kfree

[Joe Perches]

Avoid an unnecessary allocation/free by using stack instead.

Signed-off-by; Joe Perches <joe@perches.com>
---

This allocation seems unnecessary and the kasprintf isn't
checked for non-null, so maybe this is better.

 drivers/pinctrl/devicetree.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index 340fb4e..ffb59c8 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -176,13 +176,13 @@ int pinctrl_dt_to_map(struct pinctrl *p)
 {
 	struct device_node *np = p->dev->of_node;
 	int state, ret;
-	char *propname;
 	struct property *prop;
 	const char *statename;
 	const __be32 *list;
 	int size, config;
 	phandle phandle;
 	struct device_node *np_config;
+	char propname[9 + sizeof(int) * 2 + 4] = "pinctrl-";
 
 	/* CONFIG_OF enabled, p->dev not instantiated from DT */
 	if (!np) {
@@ -196,9 +196,8 @@ int pinctrl_dt_to_map(struct pinctrl *p)
 	/* For each defined state ID */
 	for (state = 0; ; state++) {
 		/* Retrieve the pinctrl-* property */
-		propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state);
+		snprintf(&propname[8], ARRAY_SIZE(propname) - 9, "%d", state);
 		prop = of_find_property(np, propname, &size);
-		kfree(propname);
 		if (!prop)
 			break;
 		list = prop->value;

Re: [PATCH] pinctrl: Avoid kasprintf/kfree

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]

On Thu, Jan 30, 2014 at 12:37:36PM -0800, Joe Perches wrote:

> +	char propname[9 + sizeof(int) * 2 + 4] = "pinctrl-";

I'm not sure this is a triumph of legibility - there's several magic
numbers there and it's not blindlingly obvious where the caclculation
came from.  Is this really a hot enough code path to benefit from an
optimisation like this?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] pinctrl: Quiet logging about missing DT nodes when not using DT

[Linus Walleij]

On Thu, Jan 30, 2014 at 7:57 PM, Mark Brown <broonie@kernel.org> wrote:

> From: Mark Brown <broonie@linaro.org>
>
> On systems which were not booted using DT it is entirely unsurprising that
> device nodes don't have any DT information and this is going to happen for
> every single device in the system. Make pinctrl be less chatty about this
> situation by only logging in the case where we have DT.
>
> Signed-off-by: Mark Brown <broonie@linaro.org>

Makes perfect sense. Patch applied.

Sorry for leaving it in limbo for so long!

Yours,
Linus Walleij

Re: [PATCH] pinctrl: Avoid kasprintf/kfree

[Linus Walleij]

On Thu, Jan 30, 2014 at 9:37 PM, Joe Perches <joe@perches.com> wrote:

> Avoid an unnecessary allocation/free by using stack instead.
>
> Signed-off-by; Joe Perches <joe@perches.com>

Hm.

> +       char propname[9 + sizeof(int) * 2 + 4] = "pinctrl-";

If you absolutely want to do this you have to

#define PINCTRL_PREFIX "pinctrl-"

char propname[strlen(PINCTRL_PREFIX) + sizeof(int)*2 + 4] = PINCTRL_PREFIX;

Then add a comment above stating why you add 4.

> This allocation seems unnecessary and the kasprintf isn't
> checked for non-null, so maybe this is better.

Another option would be to just add a NULL-check? This is
definately not on a performance-critical path.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: Avoid kasprintf/kfree

[Joe Perches]

On Mon, 2014-02-24 at 10:49 +0100, Linus Walleij wrote:
> On Thu, Jan 30, 2014 at 9:37 PM, Joe Perches <joe@perches.com> wrote:
> > Avoid an unnecessary allocation/free by using stack instead.
> >
> > Signed-off-by; Joe Perches <joe@perches.com>
> 
> Hm.
> 
> > +       char propname[9 + sizeof(int) * 2 + 4] = "pinctrl-";
> 
> If you absolutely want to do this you have to
> 
> #define PINCTRL_PREFIX "pinctrl-"
> 
> char propname[strlen(PINCTRL_PREFIX) + sizeof(int)*2 + 4] = PINCTRL_PREFIX;
> 
> Then add a comment above stating why you add 4.

No, I don't really.

> > This allocation seems unnecessary and the kasprintf isn't
> > checked for non-null, so maybe this is better.
> 
> Another option would be to just add a NULL-check? This is
> definately not on a performance-critical path.

It's a defect not to check the kasprintf return before
writing through it.

I submitted what I think appropriate.

Fix it as you choose.  Or not.

cheers, Joe