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; }
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");
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;
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?
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
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
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
linaro-kernel@lists.linaro.org
-
Joe Perches -
Linus Walleij -
Mark Brown