[RFC PATCH 00/13] of: Make drivers/of/resolver.c more readable

[RFC PATCH 00/13] of: Make drivers/of/resolver.c more readable

[frowand.list]

From: Frank Rowand <frank.rowand@am.sony.com>

drivers/of/resolve.c is a bit difficult to read.  Clean it up so
that review of future overlay related patches will be easier.

Most of the patches are intended to be reformatting, with no functional
change.  Patches that are expected to have a functional change are:

  Remove comments that state the obvious, to reduce clutter
  Remove excessive printks to reduce clutter.
  Update structure of code to be clearer, also remove BUG_ON()
    Any functional change would reflect undefined behavior on bad overlay.
    Some error message text modified.
    BUG_ON() removed.
  Add back an error message, restructured

The patches are grouped into sets of changes that are intended
to be easy to verify correctness through simple inspection.

Some of the individual patches have checkpatch warnings or errors.
But after all patches are applied, the number of errors and
warnings from running checkpatch against the entire file are
reduced to two line size warnings.

These patches are only tested via the unit tests. I do not have
expansion boards to test with real hardware.


Frank Rowand (13):
  Remove comments that state the obvious, to reduce clutter
  Remove excessive printks to reduce clutter.
  Remove braces around single line blocks.
  Convert comparisons to zero or NULL to simplify logical expressions
  Rename functions to more accurately reflect what they do
  Remove prefix "__of_" and prefix "__" from local function names
  Rename variables to better reflect purpose or follow convention
  Update structure of code to be clearer, also remove BUG_ON()
  Remove redundant size check
  Update comments to reflect changes and increase clarity
  Add back an error message, restructured
  Move setting of pointer to beside test for non-null
  Remove unused variable overlay_symbols

 drivers/of/resolver.c | 349 ++++++++++++++++++++------------------------------
 1 file changed, 141 insertions(+), 208 deletions(-)

-- 
1.9.1

[RFC PATCH 01/13] of: Remove comments that state the obvious

[frowand.list]

From: Frank Rowand <frank.rowand@am.sony.com>

Remove comments that state the obvious, to reduce clutter

Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
---
 drivers/of/resolver.c | 31 ++-----------------------------
 1 file changed, 2 insertions(+), 29 deletions(-)

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 46325d6394cf..4ff0220d7aa2 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -36,7 +36,6 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
 	if (node == NULL)
 		return NULL;
 
-	/* check */
 	if (of_node_cmp(node->full_name, full_name) == 0)
 		return of_node_get(node);
 
@@ -60,7 +59,6 @@ static phandle of_get_tree_max_phandle(void)
 	phandle phandle;
 	unsigned long flags;
 
-	/* now search recursively */
 	raw_spin_lock_irqsave(&devtree_lock, flags);
 	phandle = 0;
 	for_each_of_allnodes(node) {
@@ -75,8 +73,6 @@ static phandle of_get_tree_max_phandle(void)
 
 /*
  * Adjust a subtree's phandle values by a given delta.
- * Makes sure not to just adjust the device node's phandle value,
- * but modify the phandle properties values as well.
  */
 static void __of_adjust_tree_phandles(struct device_node *node,
 		int phandle_delta)
@@ -85,32 +81,25 @@ static void __of_adjust_tree_phandles(struct device_node *node,
 	struct property *prop;
 	phandle phandle;
 
-	/* first adjust the node's phandle direct value */
 	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL)
 		node->phandle += phandle_delta;
 
-	/* now adjust phandle & linux,phandle values */
 	for_each_property_of_node(node, prop) {
 
-		/* only look for these two */
 		if (of_prop_cmp(prop->name, "phandle") != 0 &&
 		    of_prop_cmp(prop->name, "linux,phandle") != 0)
 			continue;
 
-		/* must be big enough */
 		if (prop->length < 4)
 			continue;
 
-		/* read phandle value */
 		phandle = be32_to_cpup(prop->value);
-		if (phandle == OF_PHANDLE_ILLEGAL)	/* unresolved */
+		if (phandle == OF_PHANDLE_ILLEGAL)
 			continue;
 
-		/* adjust */
 		*(uint32_t *)prop->value = cpu_to_be32(node->phandle);
 	}
 
-	/* now do the children recursively */
 	for_each_child_of_node(node, child)
 		__of_adjust_tree_phandles(child, phandle_delta);
 }
@@ -125,7 +114,6 @@ static int __of_adjust_phandle_ref(struct device_node *node,
 	int offset, propcurlen;
 	int err = 0;
 
-	/* make a copy */
 	propval = kmalloc(rprop->length, GFP_KERNEL);
 	if (!propval) {
 		pr_err("%s: Could not copy value of '%s'\n",
@@ -165,7 +153,6 @@ static int __of_adjust_phandle_ref(struct device_node *node,
 			goto err_fail;
 		}
 
-		/* look into the resolve node for the full path */
 		refnode = __of_find_node_by_full_name(node, nodestr);
 		if (!refnode) {
 			pr_warn("%s: Could not find refnode '%s'\n",
@@ -173,7 +160,6 @@ static int __of_adjust_phandle_ref(struct device_node *node,
 			continue;
 		}
 
-		/* now find the property */
 		for_each_property_of_node(refnode, sprop) {
 			if (of_prop_cmp(sprop->name, propstr) == 0)
 				break;
@@ -240,7 +226,6 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
 		}
 		count = rprop->length / sizeof(__be32);
 
-		/* now find the target property */
 		for_each_property_of_node(target, sprop) {
 			if (of_prop_cmp(sprop->name, rprop->name) == 0)
 				break;
@@ -254,7 +239,6 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
 
 		for (i = 0; i < count; i++) {
 			off = be32_to_cpu(((__be32 *)rprop->value)[i]);
-			/* make sure the offset doesn't overstep (even wrap) */
 			if (off >= sprop->length ||
 					(off + 4) > sprop->length) {
 				pr_err("%s: Illegal property '%s' @%s\n",
@@ -264,7 +248,6 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
 			}
 
 			if (phandle_delta) {
-				/* adjust */
 				phandle = be32_to_cpu(*(__be32 *)(sprop->value + off));
 				phandle += phandle_delta;
 				*(__be32 *)(sprop->value + off) = cpu_to_be32(phandle);
@@ -320,22 +303,18 @@ int of_resolve_phandles(struct device_node *resolve)
 	if (resolve && !of_node_check_flag(resolve, OF_DETACHED))
 		pr_err("%s: node %s not detached\n", __func__,
 			 resolve->full_name);
-	/* the resolve node must exist, and be detached */
 	if (!resolve || !of_node_check_flag(resolve, OF_DETACHED))
 		return -EINVAL;
 
-	/* first we need to adjust the phandles */
 	phandle_delta = of_get_tree_max_phandle() + 1;
 	__of_adjust_tree_phandles(resolve, phandle_delta);
 
-	/* locate the local fixups */
 	childroot = NULL;
 	for_each_child_of_node(resolve, childroot)
 		if (of_node_cmp(childroot->name, "__local_fixups__") == 0)
 			break;
 
 	if (childroot != NULL) {
-		/* resolve root is guaranteed to be the '/' */
 		err = __of_adjust_tree_phandle_references(childroot,
 				resolve, 0);
 		if (err != 0)
@@ -349,10 +328,8 @@ int of_resolve_phandles(struct device_node *resolve)
 	resolve_sym = NULL;
 	resolve_fix = NULL;
 
-	/* this may fail (if no fixups are required) */
 	root_sym = of_find_node_by_path("/__symbols__");
 
-	/* locate the symbols & fixups nodes on resolve */
 	for_each_child_of_node(resolve, child) {
 
 		if (!resolve_sym &&
@@ -363,18 +340,15 @@ int of_resolve_phandles(struct device_node *resolve)
 				of_node_cmp(child->name, "__fixups__") == 0)
 			resolve_fix = child;
 
-		/* both found, don't bother anymore */
 		if (resolve_sym && resolve_fix)
 			break;
 	}
 
-	/* we do allow for the case where no fixups are needed */
 	if (!resolve_fix) {
-		err = 0;	/* no error */
+		err = 0;
 		goto out;
 	}
 
-	/* we need to fixup, but no root symbols... */
 	if (!root_sym) {
 		pr_err("%s: no symbols in root of device tree.\n", __func__);
 		err = -EINVAL;
@@ -415,7 +389,6 @@ int of_resolve_phandles(struct device_node *resolve)
 	}
 
 out:
-	/* NULL is handled by of_node_put as NOP */
 	of_node_put(root_sym);
 
 	return err;
-- 
1.9.1

Re: [RFC PATCH 01/13] of: Remove comments that state the obvious

[Joe Perches]

On Tue, 2016-10-25 at 13:58 -0700, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@am.sony.com>
> 
> Remove comments that state the obvious, to reduce clutter

Some of these removals might be overly aggressive.

> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
[]
> @@ -125,7 +114,6 @@ static int __of_adjust_phandle_ref(struct device_node *node,
>  	int offset, propcurlen;
>  	int err = 0;
>  
> -	/* make a copy */
>  	propval = kmalloc(rprop->length, GFP_KERNEL);
>  	if (!propval) {
>  		pr_err("%s: Could not copy value of '%s'\n",ld

This kmalloc/memcpy could use kmemdup instead.

It doesn't really need the pr_err either as kmalloc and/or
kmemdup get a generic OOM message.

Re: [RFC PATCH 01/13] of: Remove comments that state the obvious

[Rob Herring]

On Tue, Oct 25, 2016 at 3:58 PM,  <frowand.list@gmail.com> wrote:
> From: Frank Rowand <frank.rowand@am.sony.com>
>
> Remove comments that state the obvious, to reduce clutter

I'm probably not the best reviewer, have you ever seen a comment in my code. :)

>
> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
> ---
>  drivers/of/resolver.c | 31 ++-----------------------------
>  1 file changed, 2 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> index 46325d6394cf..4ff0220d7aa2 100644
> --- a/drivers/of/resolver.c
> +++ b/drivers/of/resolver.c

> @@ -75,8 +73,6 @@ static phandle of_get_tree_max_phandle(void)
>
>  /*
>   * Adjust a subtree's phandle values by a given delta.
> - * Makes sure not to just adjust the device node's phandle value,
> - * but modify the phandle properties values as well.

What's missing here is the why? Why do we adjust phandle values?

>   */
>  static void __of_adjust_tree_phandles(struct device_node *node,
>                 int phandle_delta)
> @@ -85,32 +81,25 @@ static void __of_adjust_tree_phandles(struct device_node *node,
>         struct property *prop;
>         phandle phandle;
>
> -       /* first adjust the node's phandle direct value */

Seems somewhat useful.

>         if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL)
>                 node->phandle += phandle_delta;
>
> -       /* now adjust phandle & linux,phandle values */

Seems somewhat useful.

>         for_each_property_of_node(node, prop) {
>
> -               /* only look for these two */
>                 if (of_prop_cmp(prop->name, "phandle") != 0 &&
>                     of_prop_cmp(prop->name, "linux,phandle") != 0)
>                         continue;
>
> -               /* must be big enough */
>                 if (prop->length < 4)
>                         continue;
>
> -               /* read phandle value */
>                 phandle = be32_to_cpup(prop->value);
> -               if (phandle == OF_PHANDLE_ILLEGAL)      /* unresolved */
> +               if (phandle == OF_PHANDLE_ILLEGAL)
>                         continue;
>
> -               /* adjust */
>                 *(uint32_t *)prop->value = cpu_to_be32(node->phandle);
>         }
>
> -       /* now do the children recursively */
>         for_each_child_of_node(node, child)
>                 __of_adjust_tree_phandles(child, phandle_delta);
>  }

> @@ -254,7 +239,6 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
>
>                 for (i = 0; i < count; i++) {
>                         off = be32_to_cpu(((__be32 *)rprop->value)[i]);
> -                       /* make sure the offset doesn't overstep (even wrap) */

Seems somewhat useful.

>                         if (off >= sprop->length ||
>                                         (off + 4) > sprop->length) {
>                                 pr_err("%s: Illegal property '%s' @%s\n",

> @@ -349,10 +328,8 @@ int of_resolve_phandles(struct device_node *resolve)
>         resolve_sym = NULL;
>         resolve_fix = NULL;
>
> -       /* this may fail (if no fixups are required) */

Seem somewhat useful.

>         root_sym = of_find_node_by_path("/__symbols__");
>
> -       /* locate the symbols & fixups nodes on resolve */
>         for_each_child_of_node(resolve, child) {
>
>                 if (!resolve_sym &&

Re: [RFC PATCH 01/13] of: Remove comments that state the obvious

[Frank Rowand]

On 10/27/16 05:18, Rob Herring wrote:
> On Tue, Oct 25, 2016 at 3:58 PM,  <frowand.list@gmail.com> wrote:
>> From: Frank Rowand <frank.rowand@am.sony.com>
>>
>> Remove comments that state the obvious, to reduce clutter
> 
> I'm probably not the best reviewer, have you ever seen a comment in my code. :)
> 
>>
>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
>> ---
>>  drivers/of/resolver.c | 31 ++-----------------------------
>>  1 file changed, 2 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>> index 46325d6394cf..4ff0220d7aa2 100644
>> --- a/drivers/of/resolver.c
>> +++ b/drivers/of/resolver.c
> 
>> @@ -75,8 +73,6 @@ static phandle of_get_tree_max_phandle(void)
>>
>>  /*
>>   * Adjust a subtree's phandle values by a given delta.
>> - * Makes sure not to just adjust the device node's phandle value,
>> - * but modify the phandle properties values as well.
> 
> What's missing here is the why? Why do we adjust phandle values?

Yes!  I will add that.

> 
>>   */
>>  static void __of_adjust_tree_phandles(struct device_node *node,
>>                 int phandle_delta)
>> @@ -85,32 +81,25 @@ static void __of_adjust_tree_phandles(struct device_node *node,
>>         struct property *prop;
>>         phandle phandle;
>>
>> -       /* first adjust the node's phandle direct value */
> 
> Seems somewhat useful.

ok

> 
>>         if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL)
>>                 node->phandle += phandle_delta;
>>
>> -       /* now adjust phandle & linux,phandle values */
> 
> Seems somewhat useful.

ok

> 
>>         for_each_property_of_node(node, prop) {
>>
>> -               /* only look for these two */
>>                 if (of_prop_cmp(prop->name, "phandle") != 0 &&
>>                     of_prop_cmp(prop->name, "linux,phandle") != 0)
>>                         continue;
>>
>> -               /* must be big enough */
>>                 if (prop->length < 4)
>>                         continue;
>>
>> -               /* read phandle value */
>>                 phandle = be32_to_cpup(prop->value);
>> -               if (phandle == OF_PHANDLE_ILLEGAL)      /* unresolved */
>> +               if (phandle == OF_PHANDLE_ILLEGAL)
>>                         continue;
>>
>> -               /* adjust */
>>                 *(uint32_t *)prop->value = cpu_to_be32(node->phandle);
>>         }
>>
>> -       /* now do the children recursively */
>>         for_each_child_of_node(node, child)
>>                 __of_adjust_tree_phandles(child, phandle_delta);
>>  }
> 
>> @@ -254,7 +239,6 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
>>
>>                 for (i = 0; i < count; i++) {
>>                         off = be32_to_cpu(((__be32 *)rprop->value)[i]);
>> -                       /* make sure the offset doesn't overstep (even wrap) */
> 
> Seems somewhat useful.

Seems obvious to me, but if others disagree I will leave it.  (I was somewhat
aggressive in removing comments).

> 
>>                         if (off >= sprop->length ||
>>                                         (off + 4) > sprop->length) {
>>                                 pr_err("%s: Illegal property '%s' @%s\n",
> 
>> @@ -349,10 +328,8 @@ int of_resolve_phandles(struct device_node *resolve)
>>         resolve_sym = NULL;
>>         resolve_fix = NULL;
>>
>> -       /* this may fail (if no fixups are required) */
> 
> Seem somewhat useful.

A later patch moves the "root_sym = ..." to just above the use of
root_sym.  At that location a failed of_find_node_by_path() is a
real failure.

> 
>>         root_sym = of_find_node_by_path("/__symbols__");
>>
>> -       /* locate the symbols & fixups nodes on resolve */
>>         for_each_child_of_node(resolve, child) {
>>
>>                 if (!resolve_sym &&
> 

[RFC PATCH 02/13] of: Remove excessive printks to reduce clutter

[frowand.list]

From: Frank Rowand <frank.rowand@am.sony.com>

Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
---
 drivers/of/resolver.c | 28 ----------------------------
 1 file changed, 28 deletions(-)

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 4ff0220d7aa2..93a7ca0bf98c 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -116,8 +116,6 @@ static int __of_adjust_phandle_ref(struct device_node *node,
 
 	propval = kmalloc(rprop->length, GFP_KERNEL);
 	if (!propval) {
-		pr_err("%s: Could not copy value of '%s'\n",
-				__func__, rprop->name);
 		return -ENOMEM;
 	}
 	memcpy(propval, rprop->value, rprop->length);
@@ -129,8 +127,6 @@ static int __of_adjust_phandle_ref(struct device_node *node,
 		nodestr = propcur;
 		s = strchr(propcur, ':');
 		if (!s) {
-			pr_err("%s: Illegal symbol entry '%s' (1)\n",
-				__func__, propcur);
 			err = -EINVAL;
 			goto err_fail;
 		}
@@ -139,8 +135,6 @@ static int __of_adjust_phandle_ref(struct device_node *node,
 		propstr = s;
 		s = strchr(s, ':');
 		if (!s) {
-			pr_err("%s: Illegal symbol entry '%s' (2)\n",
-				__func__, (char *)rprop->value);
 			err = -EINVAL;
 			goto err_fail;
 		}
@@ -148,15 +142,11 @@ static int __of_adjust_phandle_ref(struct device_node *node,
 		*s++ = '\0';
 		err = kstrtoint(s, 10, &offset);
 		if (err != 0) {
-			pr_err("%s: Could get offset '%s'\n",
-				__func__, (char *)rprop->value);
 			goto err_fail;
 		}
 
 		refnode = __of_find_node_by_full_name(node, nodestr);
 		if (!refnode) {
-			pr_warn("%s: Could not find refnode '%s'\n",
-				__func__, (char *)rprop->value);
 			continue;
 		}
 
@@ -167,8 +157,6 @@ static int __of_adjust_phandle_ref(struct device_node *node,
 		of_node_put(refnode);
 
 		if (!sprop) {
-			pr_err("%s: Could not find property '%s'\n",
-				__func__, (char *)rprop->value);
 			err = -ENOENT;
 			goto err_fail;
 		}
@@ -220,8 +208,6 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
 			continue;
 
 		if ((rprop->length % 4) != 0 || rprop->length == 0) {
-			pr_err("%s: Illegal property (size) '%s' @%s\n",
-					__func__, rprop->name, node->full_name);
 			return -EINVAL;
 		}
 		count = rprop->length / sizeof(__be32);
@@ -232,8 +218,6 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
 		}
 
 		if (sprop == NULL) {
-			pr_err("%s: Could not find target property '%s' @%s\n",
-					__func__, rprop->name, node->full_name);
 			return -EINVAL;
 		}
 
@@ -241,9 +225,6 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
 			off = be32_to_cpu(((__be32 *)rprop->value)[i]);
 			if (off >= sprop->length ||
 					(off + 4) > sprop->length) {
-				pr_err("%s: Illegal property '%s' @%s\n",
-						__func__, rprop->name,
-						node->full_name);
 				return -EINVAL;
 			}
 
@@ -262,8 +243,6 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
 				break;
 
 		if (!childtarget) {
-			pr_err("%s: Could not find target child '%s' @%s\n",
-					__func__, child->name, node->full_name);
 			return -EINVAL;
 		}
 
@@ -364,15 +343,11 @@ int of_resolve_phandles(struct device_node *resolve)
 		err = of_property_read_string(root_sym,
 				rprop->name, &refpath);
 		if (err != 0) {
-			pr_err("%s: Could not find symbol '%s'\n",
-					__func__, rprop->name);
 			goto out;
 		}
 
 		refnode = of_find_node_by_path(refpath);
 		if (!refnode) {
-			pr_err("%s: Could not find node by path '%s'\n",
-					__func__, refpath);
 			err = -ENOENT;
 			goto out;
 		}
@@ -380,9 +355,6 @@ int of_resolve_phandles(struct device_node *resolve)
 		phandle = refnode->phandle;
 		of_node_put(refnode);
 
-		pr_debug("%s: %s phandle is 0x%08x\n",
-				__func__, rprop->name, phandle);
-
 		err = __of_adjust_phandle_ref(resolve, rprop, phandle);
 		if (err)
 			break;
-- 
1.9.1

Re: [RFC PATCH 02/13] of: Remove excessive printks to reduce clutter

[Rob Herring]

On Tue, Oct 25, 2016 at 3:58 PM,  <frowand.list@gmail.com> wrote:
> From: Frank Rowand <frank.rowand@am.sony.com>

Maybe some should be debug?

> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
> ---
>  drivers/of/resolver.c | 28 ----------------------------
>  1 file changed, 28 deletions(-)
>
> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> index 4ff0220d7aa2..93a7ca0bf98c 100644
> --- a/drivers/of/resolver.c
> +++ b/drivers/of/resolver.c
> @@ -116,8 +116,6 @@ static int __of_adjust_phandle_ref(struct device_node *node,
>
>         propval = kmalloc(rprop->length, GFP_KERNEL);
>         if (!propval) {
> -               pr_err("%s: Could not copy value of '%s'\n",
> -                               __func__, rprop->name);
>                 return -ENOMEM;
>         }

I would remove the brackets in this patch rather than separately.

>         memcpy(propval, rprop->value, rprop->length);

Re: [RFC PATCH 02/13] of: Remove excessive printks to reduce clutter

[Pantelis Antoniou]

Hi Rob, Frank,

> On Oct 27, 2016, at 15:21 , Rob Herring <robh+dt@kernel.org> wrote:
> 
> On Tue, Oct 25, 2016 at 3:58 PM,  <frowand.list@gmail.com> wrote:
>> From: Frank Rowand <frank.rowand@am.sony.com>
> 
> Maybe some should be debug?
> 

Yes, please do not get rid of them completely.
Leave them at least as debug level so that if there’s a problem
there’s a way to figure out why something happened.

>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
>> ---
>> drivers/of/resolver.c | 28 ----------------------------
>> 1 file changed, 28 deletions(-)
>> 
>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>> index 4ff0220d7aa2..93a7ca0bf98c 100644
>> --- a/drivers/of/resolver.c
>> +++ b/drivers/of/resolver.c
>> @@ -116,8 +116,6 @@ static int __of_adjust_phandle_ref(struct device_node *node,
>> 
>>        propval = kmalloc(rprop->length, GFP_KERNEL);
>>        if (!propval) {
>> -               pr_err("%s: Could not copy value of '%s'\n",
>> -                               __func__, rprop->name);
>>                return -ENOMEM;
>>        }
> 
> I would remove the brackets in this patch rather than separately.
> 
>>        memcpy(propval, rprop->value, rprop->length);


Regards

— Pantelis

Re: [RFC PATCH 02/13] of: Remove excessive printks to reduce clutter

[Frank Rowand]

On 10/27/16 06:51, Pantelis Antoniou wrote:
> Hi Rob, Frank,
> 
>> On Oct 27, 2016, at 15:21 , Rob Herring <robh+dt@kernel.org> wrote:
>>
>> On Tue, Oct 25, 2016 at 3:58 PM,  <frowand.list@gmail.com> wrote:
>>> From: Frank Rowand <frank.rowand@am.sony.com>
>>
>> Maybe some should be debug?
>>
> 
> Yes, please do not get rid of them completely.
> Leave them at least as debug level so that if there’s a problem
> there’s a way to figure out why something happened.

After patch "Add back an error message, restructured" is applied,
a lot of the messages return, but hopefully keeping readability.
Note that the one message added back covers a number of error
locations.

Are there any additional key messages that you think I missed in
the add back an error message patch?

Keep in mind that many of the debug messages address malformed
dtb, which would be a bug in dtc.  It made sense for these to
exist while dtc was being modified, but now that you have
created and tested the dtc changes, I think most of those
debug messages no longer make sense for mainline code.


>>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
>>> ---
>>> drivers/of/resolver.c | 28 ----------------------------
>>> 1 file changed, 28 deletions(-)
>>>
>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>>> index 4ff0220d7aa2..93a7ca0bf98c 100644
>>> --- a/drivers/of/resolver.c
>>> +++ b/drivers/of/resolver.c
>>> @@ -116,8 +116,6 @@ static int __of_adjust_phandle_ref(struct device_node *node,
>>>
>>>        propval = kmalloc(rprop->length, GFP_KERNEL);
>>>        if (!propval) {
>>> -               pr_err("%s: Could not copy value of '%s'\n",
>>> -                               __func__, rprop->name);
>>>                return -ENOMEM;
>>>        }
>>
>> I would remove the brackets in this patch rather than separately.
>>
>>>        memcpy(propval, rprop->value, rprop->length);
> 
> 
> Regards
> 
> — Pantelis
> 
> 

Re: [RFC PATCH 02/13] of: Remove excessive printks to reduce clutter

[Frank Rowand]

On 10/27/16 05:21, Rob Herring wrote:
> On Tue, Oct 25, 2016 at 3:58 PM,  <frowand.list@gmail.com> wrote:
>> From: Frank Rowand <frank.rowand@am.sony.com>
> 
> Maybe some should be debug?
> 
>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
>> ---
>>  drivers/of/resolver.c | 28 ----------------------------
>>  1 file changed, 28 deletions(-)
>>
>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>> index 4ff0220d7aa2..93a7ca0bf98c 100644
>> --- a/drivers/of/resolver.c
>> +++ b/drivers/of/resolver.c
>> @@ -116,8 +116,6 @@ static int __of_adjust_phandle_ref(struct device_node *node,
>>
>>         propval = kmalloc(rprop->length, GFP_KERNEL);
>>         if (!propval) {
>> -               pr_err("%s: Could not copy value of '%s'\n",
>> -                               __func__, rprop->name);
>>                 return -ENOMEM;
>>         }
> 
> I would remove the brackets in this patch rather than separately.
> 
>>         memcpy(propval, rprop->value, rprop->length);
> .
> 

OK, I will collapse the "remove braces" patch into this patch.

[RFC PATCH 03/13] of: Remove braces around single line blocks.

[frowand.list]

From: Frank Rowand <frank.rowand@am.sony.com>

The single line blocks were created by previous patches in the series.

Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
---
 drivers/of/resolver.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 93a7ca0bf98c..c61ba99a1792 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -115,9 +115,8 @@ static int __of_adjust_phandle_ref(struct device_node *node,
 	int err = 0;
 
 	propval = kmalloc(rprop->length, GFP_KERNEL);
-	if (!propval) {
+	if (!propval)
 		return -ENOMEM;
-	}
 	memcpy(propval, rprop->value, rprop->length);
 
 	propend = propval + rprop->length;
@@ -141,14 +140,12 @@ static int __of_adjust_phandle_ref(struct device_node *node,
 
 		*s++ = '\0';
 		err = kstrtoint(s, 10, &offset);
-		if (err != 0) {
+		if (err != 0)
 			goto err_fail;
-		}
 
 		refnode = __of_find_node_by_full_name(node, nodestr);
-		if (!refnode) {
+		if (!refnode)
 			continue;
-		}
 
 		for_each_property_of_node(refnode, sprop) {
 			if (of_prop_cmp(sprop->name, propstr) == 0)
@@ -207,9 +204,8 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
 		    of_prop_cmp(rprop->name, "linux,phandle") == 0)
 			continue;
 
-		if ((rprop->length % 4) != 0 || rprop->length == 0) {
+		if ((rprop->length % 4) != 0 || rprop->length == 0)
 			return -EINVAL;
-		}
 		count = rprop->length / sizeof(__be32);
 
 		for_each_property_of_node(target, sprop) {
@@ -217,16 +213,13 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
 				break;
 		}
 
-		if (sprop == NULL) {
+		if (sprop == NULL)
 			return -EINVAL;
-		}
 
 		for (i = 0; i < count; i++) {
 			off = be32_to_cpu(((__be32 *)rprop->value)[i]);
-			if (off >= sprop->length ||
-					(off + 4) > sprop->length) {
+			if (off >= sprop->length || (off + 4) > sprop->length)
 				return -EINVAL;
-			}
 
 			if (phandle_delta) {
 				phandle = be32_to_cpu(*(__be32 *)(sprop->value + off));
@@ -242,9 +235,8 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
 			if (__of_node_name_cmp(child, childtarget) == 0)
 				break;
 
-		if (!childtarget) {
+		if (!childtarget)
 			return -EINVAL;
-		}
 
 		err = __of_adjust_tree_phandle_references(child, childtarget,
 				phandle_delta);
@@ -342,9 +334,8 @@ int of_resolve_phandles(struct device_node *resolve)
 
 		err = of_property_read_string(root_sym,
 				rprop->name, &refpath);
-		if (err != 0) {
+		if (err != 0)
 			goto out;
-		}
 
 		refnode = of_find_node_by_path(refpath);
 		if (!refnode) {
-- 
1.9.1

[RFC PATCH 04/13] of: Convert comparisons to zero or NULL to simplify logical expressions

[frowand.list]

From: Frank Rowand <frank.rowand@am.sony.com>

A small number of such comparisons remain where they provide more
clarity of the numeric nature of a variable.

Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
---
 drivers/of/resolver.c | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index c61ba99a1792..31fd3800787a 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -33,10 +33,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
 {
 	struct device_node *child, *found;
 
-	if (node == NULL)
+	if (!node)
 		return NULL;
 
-	if (of_node_cmp(node->full_name, full_name) == 0)
+	if (!of_node_cmp(node->full_name, full_name))
 		return of_node_get(node);
 
 	for_each_child_of_node(node, child) {
@@ -86,8 +86,8 @@ static void __of_adjust_tree_phandles(struct device_node *node,
 
 	for_each_property_of_node(node, prop) {
 
-		if (of_prop_cmp(prop->name, "phandle") != 0 &&
-		    of_prop_cmp(prop->name, "linux,phandle") != 0)
+		if (of_prop_cmp(prop->name, "phandle") &&
+		    of_prop_cmp(prop->name, "linux,phandle"))
 			continue;
 
 		if (prop->length < 4)
@@ -140,7 +140,7 @@ static int __of_adjust_phandle_ref(struct device_node *node,
 
 		*s++ = '\0';
 		err = kstrtoint(s, 10, &offset);
-		if (err != 0)
+		if (err)
 			goto err_fail;
 
 		refnode = __of_find_node_by_full_name(node, nodestr);
@@ -148,7 +148,7 @@ static int __of_adjust_phandle_ref(struct device_node *node,
 			continue;
 
 		for_each_property_of_node(refnode, sprop) {
-			if (of_prop_cmp(sprop->name, propstr) == 0)
+			if (!of_prop_cmp(sprop->name, propstr))
 				break;
 		}
 		of_node_put(refnode);
@@ -193,15 +193,15 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
 	unsigned int off;
 	phandle phandle;
 
-	if (node == NULL)
+	if (!node)
 		return 0;
 
 	for_each_property_of_node(node, rprop) {
 
 		/* skip properties added automatically */
-		if (of_prop_cmp(rprop->name, "name") == 0 ||
-		    of_prop_cmp(rprop->name, "phandle") == 0 ||
-		    of_prop_cmp(rprop->name, "linux,phandle") == 0)
+		if (!of_prop_cmp(rprop->name, "name") ||
+		    !of_prop_cmp(rprop->name, "phandle") ||
+		    !of_prop_cmp(rprop->name, "linux,phandle"))
 			continue;
 
 		if ((rprop->length % 4) != 0 || rprop->length == 0)
@@ -209,11 +209,11 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
 		count = rprop->length / sizeof(__be32);
 
 		for_each_property_of_node(target, sprop) {
-			if (of_prop_cmp(sprop->name, rprop->name) == 0)
+			if (!of_prop_cmp(sprop->name, rprop->name))
 				break;
 		}
 
-		if (sprop == NULL)
+		if (!sprop)
 			return -EINVAL;
 
 		for (i = 0; i < count; i++) {
@@ -232,7 +232,7 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
 	for_each_child_of_node(node, child) {
 
 		for_each_child_of_node(target, childtarget)
-			if (__of_node_name_cmp(child, childtarget) == 0)
+			if (!__of_node_name_cmp(child, childtarget))
 				break;
 
 		if (!childtarget)
@@ -240,7 +240,7 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
 
 		err = __of_adjust_tree_phandle_references(child, childtarget,
 				phandle_delta);
-		if (err != 0)
+		if (err)
 			return err;
 	}
 
@@ -282,13 +282,13 @@ int of_resolve_phandles(struct device_node *resolve)
 
 	childroot = NULL;
 	for_each_child_of_node(resolve, childroot)
-		if (of_node_cmp(childroot->name, "__local_fixups__") == 0)
+		if (!of_node_cmp(childroot->name, "__local_fixups__"))
 			break;
 
 	if (childroot != NULL) {
 		err = __of_adjust_tree_phandle_references(childroot,
 				resolve, 0);
-		if (err != 0)
+		if (err)
 			return err;
 
 		BUG_ON(__of_adjust_tree_phandle_references(childroot,
@@ -303,12 +303,10 @@ int of_resolve_phandles(struct device_node *resolve)
 
 	for_each_child_of_node(resolve, child) {
 
-		if (!resolve_sym &&
-				of_node_cmp(child->name, "__symbols__") == 0)
+		if (!resolve_sym && !of_node_cmp(child->name, "__symbols__"))
 			resolve_sym = child;
 
-		if (!resolve_fix &&
-				of_node_cmp(child->name, "__fixups__") == 0)
+		if (!resolve_fix && !of_node_cmp(child->name, "__fixups__"))
 			resolve_fix = child;
 
 		if (resolve_sym && resolve_fix)
@@ -329,12 +327,12 @@ int of_resolve_phandles(struct device_node *resolve)
 	for_each_property_of_node(resolve_fix, rprop) {
 
 		/* skip properties added automatically */
-		if (of_prop_cmp(rprop->name, "name") == 0)
+		if (!of_prop_cmp(rprop->name, "name"))
 			continue;
 
 		err = of_property_read_string(root_sym,
 				rprop->name, &refpath);
-		if (err != 0)
+		if (err)
 			goto out;
 
 		refnode = of_find_node_by_path(refpath);
-- 
1.9.1

[RFC PATCH 05/13] of: Rename functions to more accurately reflect what they do

[frowand.list]

From: Frank Rowand <frank.rowand@am.sony.com>

Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
---
 drivers/of/resolver.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 31fd3800787a..3d123b612789 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -53,7 +53,7 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
 /*
  * Find live tree's maximum phandle value.
  */
-static phandle of_get_tree_max_phandle(void)
+static phandle live_tree_max_phandle(void)
 {
 	struct device_node *node;
 	phandle phandle;
@@ -74,7 +74,7 @@ static phandle of_get_tree_max_phandle(void)
 /*
  * Adjust a subtree's phandle values by a given delta.
  */
-static void __of_adjust_tree_phandles(struct device_node *node,
+static void adjust_overlay_phandles(struct device_node *node,
 		int phandle_delta)
 {
 	struct device_node *child;
@@ -101,10 +101,10 @@ static void __of_adjust_tree_phandles(struct device_node *node,
 	}
 
 	for_each_child_of_node(node, child)
-		__of_adjust_tree_phandles(child, phandle_delta);
+		adjust_overlay_phandles(child, phandle_delta);
 }
 
-static int __of_adjust_phandle_ref(struct device_node *node,
+static int update_usages_of_a_phandle_reference(struct device_node *node,
 		struct property *rprop, int value)
 {
 	phandle phandle;
@@ -184,7 +184,7 @@ static int __of_node_name_cmp(const struct device_node *dn1,
  * Does not take any devtree locks so make sure you call this on a tree
  * which is at the detached state.
  */
-static int __of_adjust_tree_phandle_references(struct device_node *node,
+static int adjust_local_phandle_references(struct device_node *node,
 		struct device_node *target, int phandle_delta)
 {
 	struct device_node *child, *childtarget;
@@ -238,7 +238,7 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
 		if (!childtarget)
 			return -EINVAL;
 
-		err = __of_adjust_tree_phandle_references(child, childtarget,
+		err = adjust_local_phandle_references(child, childtarget,
 				phandle_delta);
 		if (err)
 			return err;
@@ -277,8 +277,8 @@ int of_resolve_phandles(struct device_node *resolve)
 	if (!resolve || !of_node_check_flag(resolve, OF_DETACHED))
 		return -EINVAL;
 
-	phandle_delta = of_get_tree_max_phandle() + 1;
-	__of_adjust_tree_phandles(resolve, phandle_delta);
+	phandle_delta = live_tree_max_phandle() + 1;
+	adjust_overlay_phandles(resolve, phandle_delta);
 
 	childroot = NULL;
 	for_each_child_of_node(resolve, childroot)
@@ -286,12 +286,12 @@ int of_resolve_phandles(struct device_node *resolve)
 			break;
 
 	if (childroot != NULL) {
-		err = __of_adjust_tree_phandle_references(childroot,
+		err = adjust_local_phandle_references(childroot,
 				resolve, 0);
 		if (err)
 			return err;
 
-		BUG_ON(__of_adjust_tree_phandle_references(childroot,
+		BUG_ON(adjust_local_phandle_references(childroot,
 				resolve, phandle_delta));
 	}
 
@@ -344,7 +344,7 @@ int of_resolve_phandles(struct device_node *resolve)
 		phandle = refnode->phandle;
 		of_node_put(refnode);
 
-		err = __of_adjust_phandle_ref(resolve, rprop, phandle);
+		err = update_usages_of_a_phandle_reference(resolve, rprop, phandle);
 		if (err)
 			break;
 	}
-- 
1.9.1

[RFC PATCH 06/13] of: Remove prefix "__of_" and prefix "__" from local function names

[frowand.list]

From: Frank Rowand <frank.rowand@am.sony.com>

Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
---
 drivers/of/resolver.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 3d123b612789..0ce38aa0ed3c 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -28,7 +28,7 @@
  * Find a node with the give full name by recursively following any of
  * the child node links.
  */
-static struct device_node *__of_find_node_by_full_name(struct device_node *node,
+static struct device_node *find_node_by_full_name(struct device_node *node,
 		const char *full_name)
 {
 	struct device_node *child, *found;
@@ -40,7 +40,7 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
 		return of_node_get(node);
 
 	for_each_child_of_node(node, child) {
-		found = __of_find_node_by_full_name(child, full_name);
+		found = find_node_by_full_name(child, full_name);
 		if (found != NULL) {
 			of_node_put(child);
 			return found;
@@ -143,7 +143,7 @@ static int update_usages_of_a_phandle_reference(struct device_node *node,
 		if (err)
 			goto err_fail;
 
-		refnode = __of_find_node_by_full_name(node, nodestr);
+		refnode = find_node_by_full_name(node, nodestr);
 		if (!refnode)
 			continue;
 
@@ -168,7 +168,7 @@ static int update_usages_of_a_phandle_reference(struct device_node *node,
 }
 
 /* compare nodes taking into account that 'name' strips out the @ part */
-static int __of_node_name_cmp(const struct device_node *dn1,
+static int node_name_cmp(const struct device_node *dn1,
 		const struct device_node *dn2)
 {
 	const char *n1 = strrchr(dn1->full_name, '/') ? : "/";
@@ -232,7 +232,7 @@ static int adjust_local_phandle_references(struct device_node *node,
 	for_each_child_of_node(node, child) {
 
 		for_each_child_of_node(target, childtarget)
-			if (!__of_node_name_cmp(child, childtarget))
+			if (!node_name_cmp(child, childtarget))
 				break;
 
 		if (!childtarget)
-- 
1.9.1

Re: [RFC PATCH 06/13] of: Remove prefix "__of_" and prefix "__" from local function names

[Rob Herring]

On Tue, Oct 25, 2016 at 3:58 PM,  <frowand.list@gmail.com> wrote:
> From: Frank Rowand <frank.rowand@am.sony.com>

I prefer to leave the prefixes and this is getting into pointless churn.

>
> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
> ---
>  drivers/of/resolver.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Re: [RFC PATCH 06/13] of: Remove prefix "__of_" and prefix "__" from local function names

[Frank Rowand]

On 10/27/16 05:47, Rob Herring wrote:
> On Tue, Oct 25, 2016 at 3:58 PM,  <frowand.list@gmail.com> wrote:
>> From: Frank Rowand <frank.rowand@am.sony.com>
> 
> I prefer to leave the prefixes and this is getting into pointless churn.
> 
>>
>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
>> ---
>>  drivers/of/resolver.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
> 

If I was just submitting this as a single patch, I would agree.

But since I am making so many other changes, I think it makes
sense to do this as part of this series.  It is broken apart
as a separate patch to be easy to review and not pollute any
of the other patches in the series.

The prefixes add no value for a local function, but they do
add noise when reading code.

The changes are local to this file and do not impact anything
else.

Looking at the single patch, it does seem like churn.  But
looking at the entire file before the set of changes and
after the set of changes, I find the file much easier to
read afterwards.  Each individual patch may make a small
contribution to the end result, but the combination of all
of them is significant.

Re: [RFC PATCH 06/13] of: Remove prefix "__of_" and prefix "__" from local function names

[Rob Herring]

On Thu, Oct 27, 2016 at 11:35 AM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 10/27/16 05:47, Rob Herring wrote:
>> On Tue, Oct 25, 2016 at 3:58 PM,  <frowand.list@gmail.com> wrote:
>>> From: Frank Rowand <frank.rowand@am.sony.com>
>>
>> I prefer to leave the prefixes and this is getting into pointless churn.
>>
>>>
>>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
>>> ---
>>>  drivers/of/resolver.c | 10 +++++-----
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>
> If I was just submitting this as a single patch, I would agree.
>
> But since I am making so many other changes, I think it makes
> sense to do this as part of this series.  It is broken apart
> as a separate patch to be easy to review and not pollute any
> of the other patches in the series.
>
> The prefixes add no value for a local function, but they do
> add noise when reading code.

The value is when reading the calling function, you know the function
is a DT related function. You don't know it's a static function
without looking up the function name. That said, I wouldn't object to
code originally written either way, I just don't see the value in
changing it.

Rob

Re: [RFC PATCH 06/13] of: Remove prefix "__of_" and prefix "__" from local function names

[Frank Rowand]

On 10/27/16 09:58, Rob Herring wrote:
> On Thu, Oct 27, 2016 at 11:35 AM, Frank Rowand <frowand.list@gmail.com> wrote:
>> On 10/27/16 05:47, Rob Herring wrote:
>>> On Tue, Oct 25, 2016 at 3:58 PM,  <frowand.list@gmail.com> wrote:
>>>> From: Frank Rowand <frank.rowand@am.sony.com>
>>>
>>> I prefer to leave the prefixes and this is getting into pointless churn.
>>>
>>>>
>>>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
>>>> ---
>>>>  drivers/of/resolver.c | 10 +++++-----
>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>
>> If I was just submitting this as a single patch, I would agree.
>>
>> But since I am making so many other changes, I think it makes
>> sense to do this as part of this series.  It is broken apart
>> as a separate patch to be easy to review and not pollute any
>> of the other patches in the series.
>>
>> The prefixes add no value for a local function, but they do
>> add noise when reading code.
> 
> The value is when reading the calling function, you know the function
> is a DT related function. You don't know it's a static function

It is more than that.  A common convention in drivers/of/ is that
function blah() acquires a lock, calls function __blah(), and
releases the lock.  Any function other than blah() that wants
to call __blah() must also hold the proper lock.  The functions
whose name this patch changes do not fit this pattern.


> without looking up the function name. That said, I wouldn't object to
> code originally written either way, I just don't see the value in
> changing it.
> 
> Rob
> 

Re: [RFC PATCH 06/13] of: Remove prefix "__of_" and prefix "__" from local function names

[Rob Herring]

On Thu, Oct 27, 2016 at 1:25 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 10/27/16 09:58, Rob Herring wrote:
>> On Thu, Oct 27, 2016 at 11:35 AM, Frank Rowand <frowand.list@gmail.com> wrote:
>>> On 10/27/16 05:47, Rob Herring wrote:
>>>> On Tue, Oct 25, 2016 at 3:58 PM,  <frowand.list@gmail.com> wrote:
>>>>> From: Frank Rowand <frank.rowand@am.sony.com>
>>>>
>>>> I prefer to leave the prefixes and this is getting into pointless churn.
>>>>
>>>>>
>>>>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
>>>>> ---
>>>>>  drivers/of/resolver.c | 10 +++++-----
>>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>
>>> If I was just submitting this as a single patch, I would agree.
>>>
>>> But since I am making so many other changes, I think it makes
>>> sense to do this as part of this series.  It is broken apart
>>> as a separate patch to be easy to review and not pollute any
>>> of the other patches in the series.
>>>
>>> The prefixes add no value for a local function, but they do
>>> add noise when reading code.
>>
>> The value is when reading the calling function, you know the function
>> is a DT related function. You don't know it's a static function
>
> It is more than that.  A common convention in drivers/of/ is that
> function blah() acquires a lock, calls function __blah(), and
> releases the lock.  Any function other than blah() that wants
> to call __blah() must also hold the proper lock.  The functions
> whose name this patch changes do not fit this pattern.

Okay, fair enough.

Rob

[RFC PATCH 07/13] of: Rename variables to better reflect purpose or follow convention

[frowand.list]

From: Frank Rowand <frank.rowand@am.sony.com>

Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
---
 drivers/of/resolver.c | 172 +++++++++++++++++++++++++-------------------------
 1 file changed, 85 insertions(+), 87 deletions(-)

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 0ce38aa0ed3c..0778747cdd58 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -74,17 +74,17 @@ static phandle live_tree_max_phandle(void)
 /*
  * Adjust a subtree's phandle values by a given delta.
  */
-static void adjust_overlay_phandles(struct device_node *node,
+static void adjust_overlay_phandles(struct device_node *overlay,
 		int phandle_delta)
 {
 	struct device_node *child;
 	struct property *prop;
 	phandle phandle;
 
-	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL)
-		node->phandle += phandle_delta;
+	if (overlay->phandle != 0 && overlay->phandle != OF_PHANDLE_ILLEGAL)
+		overlay->phandle += phandle_delta;
 
-	for_each_property_of_node(node, prop) {
+	for_each_property_of_node(overlay, prop) {
 
 		if (of_prop_cmp(prop->name, "phandle") &&
 		    of_prop_cmp(prop->name, "linux,phandle"))
@@ -97,41 +97,40 @@ static void adjust_overlay_phandles(struct device_node *node,
 		if (phandle == OF_PHANDLE_ILLEGAL)
 			continue;
 
-		*(uint32_t *)prop->value = cpu_to_be32(node->phandle);
+		*(uint32_t *)prop->value = cpu_to_be32(overlay->phandle);
 	}
 
-	for_each_child_of_node(node, child)
+	for_each_child_of_node(overlay, child)
 		adjust_overlay_phandles(child, phandle_delta);
 }
 
-static int update_usages_of_a_phandle_reference(struct device_node *node,
-		struct property *rprop, int value)
+static int update_usages_of_a_phandle_reference(struct device_node *overlay,
+		struct property *prop_fixup, phandle phandle)
 {
-	phandle phandle;
 	struct device_node *refnode;
-	struct property *sprop;
-	char *propval, *propcur, *propend, *nodestr, *propstr, *s;
-	int offset, propcurlen;
+	struct property *prop;
+	char *value, *cur, *end, *node_path, *prop_name, *s;
+	int offset, len;
 	int err = 0;
 
-	propval = kmalloc(rprop->length, GFP_KERNEL);
-	if (!propval)
+	value = kmalloc(prop_fixup->length, GFP_KERNEL);
+	if (!value)
 		return -ENOMEM;
-	memcpy(propval, rprop->value, rprop->length);
+	memcpy(value, prop_fixup->value, prop_fixup->length);
 
-	propend = propval + rprop->length;
-	for (propcur = propval; propcur < propend; propcur += propcurlen + 1) {
-		propcurlen = strlen(propcur);
+	end = value + prop_fixup->length;
+	for (cur = value; cur < end; cur += len + 1) {
+		len = strlen(cur);
 
-		nodestr = propcur;
-		s = strchr(propcur, ':');
+		node_path = cur;
+		s = strchr(cur, ':');
 		if (!s) {
 			err = -EINVAL;
 			goto err_fail;
 		}
 		*s++ = '\0';
 
-		propstr = s;
+		prop_name = s;
 		s = strchr(s, ':');
 		if (!s) {
 			err = -EINVAL;
@@ -143,27 +142,26 @@ static int update_usages_of_a_phandle_reference(struct device_node *node,
 		if (err)
 			goto err_fail;
 
-		refnode = find_node_by_full_name(node, nodestr);
+		refnode = find_node_by_full_name(overlay, node_path);
 		if (!refnode)
 			continue;
 
-		for_each_property_of_node(refnode, sprop) {
-			if (!of_prop_cmp(sprop->name, propstr))
+		for_each_property_of_node(refnode, prop) {
+			if (!of_prop_cmp(prop->name, prop_name))
 				break;
 		}
 		of_node_put(refnode);
 
-		if (!sprop) {
+		if (!prop) {
 			err = -ENOENT;
 			goto err_fail;
 		}
 
-		phandle = value;
-		*(__be32 *)(sprop->value + offset) = cpu_to_be32(phandle);
+		*(__be32 *)(prop->value + offset) = cpu_to_be32(phandle);
 	}
 
 err_fail:
-	kfree(propval);
+	kfree(value);
 	return err;
 }
 
@@ -184,61 +182,61 @@ static int node_name_cmp(const struct device_node *dn1,
  * Does not take any devtree locks so make sure you call this on a tree
  * which is at the detached state.
  */
-static int adjust_local_phandle_references(struct device_node *node,
-		struct device_node *target, int phandle_delta)
+static int adjust_local_phandle_references(struct device_node *local_fixups,
+		struct device_node *overlay, int phandle_delta)
 {
-	struct device_node *child, *childtarget;
-	struct property *rprop, *sprop;
+	struct device_node *child, *overlay_child;
+	struct property *prop_fix, *prop;
 	int err, i, count;
 	unsigned int off;
 	phandle phandle;
 
-	if (!node)
+	if (!local_fixups)
 		return 0;
 
-	for_each_property_of_node(node, rprop) {
+	for_each_property_of_node(local_fixups, prop_fix) {
 
 		/* skip properties added automatically */
-		if (!of_prop_cmp(rprop->name, "name") ||
-		    !of_prop_cmp(rprop->name, "phandle") ||
-		    !of_prop_cmp(rprop->name, "linux,phandle"))
+		if (!of_prop_cmp(prop_fix->name, "name") ||
+		    !of_prop_cmp(prop_fix->name, "phandle") ||
+		    !of_prop_cmp(prop_fix->name, "linux,phandle"))
 			continue;
 
-		if ((rprop->length % 4) != 0 || rprop->length == 0)
+		if ((prop_fix->length % 4) != 0 || prop_fix->length == 0)
 			return -EINVAL;
-		count = rprop->length / sizeof(__be32);
+		count = prop_fix->length / sizeof(__be32);
 
-		for_each_property_of_node(target, sprop) {
-			if (!of_prop_cmp(sprop->name, rprop->name))
+		for_each_property_of_node(overlay, prop) {
+			if (!of_prop_cmp(prop->name, prop_fix->name))
 				break;
 		}
 
-		if (!sprop)
+		if (!prop)
 			return -EINVAL;
 
 		for (i = 0; i < count; i++) {
-			off = be32_to_cpu(((__be32 *)rprop->value)[i]);
-			if (off >= sprop->length || (off + 4) > sprop->length)
+			off = be32_to_cpu(((__be32 *)prop_fix->value)[i]);
+			if (off >= prop->length || (off + 4) > prop->length)
 				return -EINVAL;
 
 			if (phandle_delta) {
-				phandle = be32_to_cpu(*(__be32 *)(sprop->value + off));
+				phandle = be32_to_cpu(*(__be32 *)(prop->value + off));
 				phandle += phandle_delta;
-				*(__be32 *)(sprop->value + off) = cpu_to_be32(phandle);
+				*(__be32 *)(prop->value + off) = cpu_to_be32(phandle);
 			}
 		}
 	}
 
-	for_each_child_of_node(node, child) {
+	for_each_child_of_node(local_fixups, child) {
 
-		for_each_child_of_node(target, childtarget)
-			if (!node_name_cmp(child, childtarget))
+		for_each_child_of_node(overlay, overlay_child)
+			if (!node_name_cmp(child, overlay_child))
 				break;
 
-		if (!childtarget)
+		if (!overlay_child)
 			return -EINVAL;
 
-		err = adjust_local_phandle_references(child, childtarget,
+		err = adjust_local_phandle_references(child, overlay_child,
 				phandle_delta);
 		if (err)
 			return err;
@@ -260,78 +258,78 @@ static int adjust_local_phandle_references(struct device_node *node,
  * are fit to be inserted or operate upon the live tree.
  * Returns 0 on success or a negative error value on error.
  */
-int of_resolve_phandles(struct device_node *resolve)
+int of_resolve_phandles(struct device_node *overlay)
 {
-	struct device_node *child, *childroot, *refnode;
-	struct device_node *root_sym, *resolve_sym, *resolve_fix;
-	struct property *rprop;
+	struct device_node *child, *local_fixups, *refnode;
+	struct device_node *tree_symbols, *overlay_symbols, *overlay_fixups;
+	struct property *prop;
 	const char *refpath;
 	phandle phandle, phandle_delta;
 	int err;
 
-	if (!resolve)
-		pr_err("%s: null node\n", __func__);
-	if (resolve && !of_node_check_flag(resolve, OF_DETACHED))
+	if (!overlay)
+		pr_err("%s: null overlay\n", __func__);
+	if (overlay && !of_node_check_flag(overlay, OF_DETACHED))
 		pr_err("%s: node %s not detached\n", __func__,
-			 resolve->full_name);
-	if (!resolve || !of_node_check_flag(resolve, OF_DETACHED))
+			 overlay->full_name);
+	if (!overlay || !of_node_check_flag(overlay, OF_DETACHED))
 		return -EINVAL;
 
 	phandle_delta = live_tree_max_phandle() + 1;
-	adjust_overlay_phandles(resolve, phandle_delta);
+	adjust_overlay_phandles(overlay, phandle_delta);
 
-	childroot = NULL;
-	for_each_child_of_node(resolve, childroot)
-		if (!of_node_cmp(childroot->name, "__local_fixups__"))
+	local_fixups = NULL;
+	for_each_child_of_node(overlay, local_fixups)
+		if (!of_node_cmp(local_fixups->name, "__local_fixups__"))
 			break;
 
-	if (childroot != NULL) {
-		err = adjust_local_phandle_references(childroot,
-				resolve, 0);
+	if (local_fixups != NULL) {
+		err = adjust_local_phandle_references(local_fixups,
+				overlay, 0);
 		if (err)
 			return err;
 
-		BUG_ON(adjust_local_phandle_references(childroot,
-				resolve, phandle_delta));
+		BUG_ON(adjust_local_phandle_references(local_fixups,
+				overlay, phandle_delta));
 	}
 
-	root_sym = NULL;
-	resolve_sym = NULL;
-	resolve_fix = NULL;
+	tree_symbols = NULL;
+	overlay_symbols = NULL;
+	overlay_fixups = NULL;
 
-	root_sym = of_find_node_by_path("/__symbols__");
+	tree_symbols = of_find_node_by_path("/__symbols__");
 
-	for_each_child_of_node(resolve, child) {
+	for_each_child_of_node(overlay, child) {
 
-		if (!resolve_sym && !of_node_cmp(child->name, "__symbols__"))
-			resolve_sym = child;
+		if (!overlay_symbols && !of_node_cmp(child->name, "__symbols__"))
+			overlay_symbols = child;
 
-		if (!resolve_fix && !of_node_cmp(child->name, "__fixups__"))
-			resolve_fix = child;
+		if (!overlay_fixups && !of_node_cmp(child->name, "__fixups__"))
+			overlay_fixups = child;
 
-		if (resolve_sym && resolve_fix)
+		if (overlay_symbols && overlay_fixups)
 			break;
 	}
 
-	if (!resolve_fix) {
+	if (!overlay_fixups) {
 		err = 0;
 		goto out;
 	}
 
-	if (!root_sym) {
+	if (!tree_symbols) {
 		pr_err("%s: no symbols in root of device tree.\n", __func__);
 		err = -EINVAL;
 		goto out;
 	}
 
-	for_each_property_of_node(resolve_fix, rprop) {
+	for_each_property_of_node(overlay_fixups, prop) {
 
 		/* skip properties added automatically */
-		if (!of_prop_cmp(rprop->name, "name"))
+		if (!of_prop_cmp(prop->name, "name"))
 			continue;
 
-		err = of_property_read_string(root_sym,
-				rprop->name, &refpath);
+		err = of_property_read_string(tree_symbols,
+				prop->name, &refpath);
 		if (err)
 			goto out;
 
@@ -344,13 +342,13 @@ int of_resolve_phandles(struct device_node *resolve)
 		phandle = refnode->phandle;
 		of_node_put(refnode);
 
-		err = update_usages_of_a_phandle_reference(resolve, rprop, phandle);
+		err = update_usages_of_a_phandle_reference(overlay, prop, phandle);
 		if (err)
 			break;
 	}
 
 out:
-	of_node_put(root_sym);
+	of_node_put(tree_symbols);
 
 	return err;
 }
-- 
1.9.1

[RFC PATCH 08/13] of: Update structure of code, remove BUG_ON()

[frowand.list]

From: Frank Rowand <frank.rowand@am.sony.com>

Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
---
 drivers/of/resolver.c | 48 +++++++++++++++++-------------------------------
 1 file changed, 17 insertions(+), 31 deletions(-)

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 0778747cdd58..708daca1d522 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -136,8 +136,8 @@ static int update_usages_of_a_phandle_reference(struct device_node *overlay,
 			err = -EINVAL;
 			goto err_fail;
 		}
-
 		*s++ = '\0';
+
 		err = kstrtoint(s, 10, &offset);
 		if (err)
 			goto err_fail;
@@ -219,11 +219,9 @@ static int adjust_local_phandle_references(struct device_node *local_fixups,
 			if (off >= prop->length || (off + 4) > prop->length)
 				return -EINVAL;
 
-			if (phandle_delta) {
-				phandle = be32_to_cpu(*(__be32 *)(prop->value + off));
-				phandle += phandle_delta;
-				*(__be32 *)(prop->value + off) = cpu_to_be32(phandle);
-			}
+			phandle = be32_to_cpu(*(__be32 *)(prop->value + off));
+			phandle += phandle_delta;
+			*(__be32 *)(prop->value + off) = cpu_to_be32(phandle);
 		}
 	}
 
@@ -267,48 +265,36 @@ int of_resolve_phandles(struct device_node *overlay)
 	phandle phandle, phandle_delta;
 	int err;
 
-	if (!overlay)
-		pr_err("%s: null overlay\n", __func__);
-	if (overlay && !of_node_check_flag(overlay, OF_DETACHED))
-		pr_err("%s: node %s not detached\n", __func__,
-			 overlay->full_name);
-	if (!overlay || !of_node_check_flag(overlay, OF_DETACHED))
+	if (!overlay) {
+		pr_err("null overlay\n");
+		return -EINVAL;
+	}
+	if (!of_node_check_flag(overlay, OF_DETACHED)) {
+		pr_err("overlay not detached\n");
 		return -EINVAL;
+	}
 
 	phandle_delta = live_tree_max_phandle() + 1;
 	adjust_overlay_phandles(overlay, phandle_delta);
 
-	local_fixups = NULL;
 	for_each_child_of_node(overlay, local_fixups)
 		if (!of_node_cmp(local_fixups->name, "__local_fixups__"))
 			break;
 
-	if (local_fixups != NULL) {
-		err = adjust_local_phandle_references(local_fixups,
-				overlay, 0);
-		if (err)
-			return err;
+	err = adjust_local_phandle_references(local_fixups, overlay, phandle_delta);
+	if (err)
+		return err;
 
-		BUG_ON(adjust_local_phandle_references(local_fixups,
-				overlay, phandle_delta));
-	}
-
-	tree_symbols = NULL;
 	overlay_symbols = NULL;
 	overlay_fixups = NULL;
 
 	tree_symbols = of_find_node_by_path("/__symbols__");
 
 	for_each_child_of_node(overlay, child) {
-
-		if (!overlay_symbols && !of_node_cmp(child->name, "__symbols__"))
+		if (!of_node_cmp(child->name, "__symbols__"))
 			overlay_symbols = child;
-
-		if (!overlay_fixups && !of_node_cmp(child->name, "__fixups__"))
+		if (!of_node_cmp(child->name, "__fixups__"))
 			overlay_fixups = child;
-
-		if (overlay_symbols && overlay_fixups)
-			break;
 	}
 
 	if (!overlay_fixups) {
@@ -317,7 +303,7 @@ int of_resolve_phandles(struct device_node *overlay)
 	}
 
 	if (!tree_symbols) {
-		pr_err("%s: no symbols in root of device tree.\n", __func__);
+		pr_err("no symbols in root of device tree.\n");
 		err = -EINVAL;
 		goto out;
 	}
-- 
1.9.1

[RFC PATCH 09/13] of: Remove redundant size check

[frowand.list]

From: Frank Rowand <frank.rowand@am.sony.com>

Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
---
 drivers/of/resolver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 708daca1d522..76c09cb57eae 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -216,7 +216,7 @@ static int adjust_local_phandle_references(struct device_node *local_fixups,
 
 		for (i = 0; i < count; i++) {
 			off = be32_to_cpu(((__be32 *)prop_fix->value)[i]);
-			if (off >= prop->length || (off + 4) > prop->length)
+			if ((off + 4) > prop->length)
 				return -EINVAL;
 
 			phandle = be32_to_cpu(*(__be32 *)(prop->value + off));
-- 
1.9.1

[RFC PATCH 10/13] of: Update comments to reflect changes and increase clarity

[frowand.list]

From: Frank Rowand <frank.rowand@am.sony.com>

Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
---
 drivers/of/resolver.c | 51 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 76c09cb57eae..4e6df385118b 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -50,9 +50,6 @@ static struct device_node *find_node_by_full_name(struct device_node *node,
 	return NULL;
 }
 
-/*
- * Find live tree's maximum phandle value.
- */
 static phandle live_tree_max_phandle(void)
 {
 	struct device_node *node;
@@ -71,9 +68,6 @@ static phandle live_tree_max_phandle(void)
 	return phandle;
 }
 
-/*
- * Adjust a subtree's phandle values by a given delta.
- */
 static void adjust_overlay_phandles(struct device_node *overlay,
 		int phandle_delta)
 {
@@ -118,6 +112,7 @@ static int update_usages_of_a_phandle_reference(struct device_node *overlay,
 		return -ENOMEM;
 	memcpy(value, prop_fixup->value, prop_fixup->length);
 
+	/* prop_fixup contains a list of tuples of path:property_name:offset */
 	end = value + prop_fixup->length;
 	for (cur = value; cur < end; cur += len + 1) {
 		len = strlen(cur);
@@ -177,10 +172,14 @@ static int node_name_cmp(const struct device_node *dn1,
 
 /*
  * Adjust the local phandle references by the given phandle delta.
- * Assumes the existances of a __local_fixups__ node at the root.
- * Assumes that __of_verify_tree_phandle_references has been called.
- * Does not take any devtree locks so make sure you call this on a tree
- * which is at the detached state.
+ *
+ * Subtree @local_fixups, which is overlay node __local_fixups__,
+ * mirrors the fragment node structure at the root of the overlay.
+ *
+ * For each property in the fragments that contains a phandle reference,
+ * @local_fixups has a property of the same name that contains a list
+ * of offsets of the phandle reference(s) within the respective property
+ * value(s).  The values at these offsets will be fixed up.
  */
 static int adjust_local_phandle_references(struct device_node *local_fixups,
 		struct device_node *overlay, int phandle_delta)
@@ -225,6 +224,13 @@ static int adjust_local_phandle_references(struct device_node *local_fixups,
 		}
 	}
 
+	/*
+	 * These nested loops recurse down two subtrees in parallel, where the
+	 * node names in the two subtrees match.
+	 *
+	 * The roots of the subtrees are the overlay's __local_fixups__ node
+	 * and the overlay's root node.
+	 */
 	for_each_child_of_node(local_fixups, child) {
 
 		for_each_child_of_node(overlay, overlay_child)
@@ -244,17 +250,24 @@ static int adjust_local_phandle_references(struct device_node *local_fixups,
 }
 
 /**
- * of_resolve	- Resolve the given node against the live tree.
+ * of_resolve_phandles - Relocate and resolve overlay against live tree
+ *
+ * @overlay:	Pointer to devicetree overlay to relocate and resolve
+ *
+ * Modify (relocate) values of local phandles in @overlay to a range that
+ * does not conflict with the live expanded devicetree.  Update references
+ * to the local phandles in @overlay.  Update (resolve) phandle references
+ * in @overlay that refer to the live expanded devicetree.
+ *
+ * @overlay must be detached.
  *
- * @resolve:	Node to resolve
+ * Resolving and applying @overlay to the live expanded devicetree must be
+ * protected by a mechanism to ensure that multiple overlays are processed
+ * in a single threaded manner so that multiple overlays will not relocate
+ * phandles to overlapping ranges.  The mechanism to enforce this is not
+ * yet implemented.
  *
- * Perform dynamic Device Tree resolution against the live tree
- * to the given node to resolve. This depends on the live tree
- * having a __symbols__ node, and the resolve node the __fixups__ &
- * __local_fixups__ nodes (if needed).
- * The result of the operation is a resolve node that it's contents
- * are fit to be inserted or operate upon the live tree.
- * Returns 0 on success or a negative error value on error.
+ * Return: %0 on success or a negative error value on error.
  */
 int of_resolve_phandles(struct device_node *overlay)
 {
-- 
1.9.1

[RFC PATCH 11/13] of: Add back an error message, restructured

[frowand.list]

From: Frank Rowand <frank.rowand@am.sony.com>

Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
---
 drivers/of/resolver.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 4e6df385118b..664c97e1ecb4 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -278,13 +278,17 @@ int of_resolve_phandles(struct device_node *overlay)
 	phandle phandle, phandle_delta;
 	int err;
 
+	tree_symbols = NULL;
+
 	if (!overlay) {
 		pr_err("null overlay\n");
-		return -EINVAL;
+		err = -EINVAL;
+		goto err_out;
 	}
 	if (!of_node_check_flag(overlay, OF_DETACHED)) {
 		pr_err("overlay not detached\n");
-		return -EINVAL;
+		err = -EINVAL;
+		goto err_out;
 	}
 
 	phandle_delta = live_tree_max_phandle() + 1;
@@ -296,7 +300,7 @@ int of_resolve_phandles(struct device_node *overlay)
 
 	err = adjust_local_phandle_references(local_fixups, overlay, phandle_delta);
 	if (err)
-		return err;
+		goto err_out;
 
 	overlay_symbols = NULL;
 	overlay_fixups = NULL;
@@ -318,7 +322,7 @@ int of_resolve_phandles(struct device_node *overlay)
 	if (!tree_symbols) {
 		pr_err("no symbols in root of device tree.\n");
 		err = -EINVAL;
-		goto out;
+		goto err_out;
 	}
 
 	for_each_property_of_node(overlay_fixups, prop) {
@@ -330,12 +334,12 @@ int of_resolve_phandles(struct device_node *overlay)
 		err = of_property_read_string(tree_symbols,
 				prop->name, &refpath);
 		if (err)
-			goto out;
+			goto err_out;
 
 		refnode = of_find_node_by_path(refpath);
 		if (!refnode) {
 			err = -ENOENT;
-			goto out;
+			goto err_out;
 		}
 
 		phandle = refnode->phandle;
@@ -346,6 +350,8 @@ int of_resolve_phandles(struct device_node *overlay)
 			break;
 	}
 
+err_out:
+	pr_err("overlay phandle fixup failed: %d\n", err);
 out:
 	of_node_put(tree_symbols);
 
-- 
1.9.1

[RFC PATCH 12/13] of: Move setting of pointer to beside test for non-null

[frowand.list]

From: Frank Rowand <frank.rowand@am.sony.com>

Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
---
 drivers/of/resolver.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 664c97e1ecb4..3f7cf569c7ea 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -305,8 +305,6 @@ int of_resolve_phandles(struct device_node *overlay)
 	overlay_symbols = NULL;
 	overlay_fixups = NULL;
 
-	tree_symbols = of_find_node_by_path("/__symbols__");
-
 	for_each_child_of_node(overlay, child) {
 		if (!of_node_cmp(child->name, "__symbols__"))
 			overlay_symbols = child;
@@ -319,6 +317,7 @@ int of_resolve_phandles(struct device_node *overlay)
 		goto out;
 	}
 
+	tree_symbols = of_find_node_by_path("/__symbols__");
 	if (!tree_symbols) {
 		pr_err("no symbols in root of device tree.\n");
 		err = -EINVAL;
-- 
1.9.1

[RFC PATCH 13/13] of: Remove unused variable overlay_symbols

[frowand.list]

From: Frank Rowand <frank.rowand@am.sony.com>

This unused variable is a reminder that symbols in overlays are
not available to subsequent overlays.  If such a feature is
desired then there are several ways it could be implemented.

Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
---
 drivers/of/resolver.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 3f7cf569c7ea..b48d16200ccd 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -272,7 +272,7 @@ static int adjust_local_phandle_references(struct device_node *local_fixups,
 int of_resolve_phandles(struct device_node *overlay)
 {
 	struct device_node *child, *local_fixups, *refnode;
-	struct device_node *tree_symbols, *overlay_symbols, *overlay_fixups;
+	struct device_node *tree_symbols, *overlay_fixups;
 	struct property *prop;
 	const char *refpath;
 	phandle phandle, phandle_delta;
@@ -302,12 +302,9 @@ int of_resolve_phandles(struct device_node *overlay)
 	if (err)
 		goto err_out;
 
-	overlay_symbols = NULL;
 	overlay_fixups = NULL;
 
 	for_each_child_of_node(overlay, child) {
-		if (!of_node_cmp(child->name, "__symbols__"))
-			overlay_symbols = child;
 		if (!of_node_cmp(child->name, "__fixups__"))
 			overlay_fixups = child;
 	}
-- 
1.9.1

Re: [RFC PATCH 13/13] of: Remove unused variable overlay_symbols

[Pantelis Antoniou]

Hi Frank,


> On Oct 25, 2016, at 23:59 , frowand.list@gmail.com wrote:
> 
> From: Frank Rowand <frank.rowand@am.sony.com>
> 
> This unused variable is a reminder that symbols in overlays are
> not available to subsequent overlays.  If such a feature is
> desired then there are several ways it could be implemented.
> 

Please don’t apply that. There’s a patch that actually imports
the symbol table from overlays that subsequent operations
work.

Please see:

https://patchwork.kernel.org/patch/9104701/

> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
> ---
> drivers/of/resolver.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> index 3f7cf569c7ea..b48d16200ccd 100644
> --- a/drivers/of/resolver.c
> +++ b/drivers/of/resolver.c
> @@ -272,7 +272,7 @@ static int adjust_local_phandle_references(struct device_node *local_fixups,
> int of_resolve_phandles(struct device_node *overlay)
> {
> 	struct device_node *child, *local_fixups, *refnode;
> -	struct device_node *tree_symbols, *overlay_symbols, *overlay_fixups;
> +	struct device_node *tree_symbols, *overlay_fixups;
> 	struct property *prop;
> 	const char *refpath;
> 	phandle phandle, phandle_delta;
> @@ -302,12 +302,9 @@ int of_resolve_phandles(struct device_node *overlay)
> 	if (err)
> 		goto err_out;
> 
> -	overlay_symbols = NULL;
> 	overlay_fixups = NULL;
> 
> 	for_each_child_of_node(overlay, child) {
> -		if (!of_node_cmp(child->name, "__symbols__"))
> -			overlay_symbols = child;
> 		if (!of_node_cmp(child->name, "__fixups__"))
> 			overlay_fixups = child;
> 	}
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards

— Pantelis

Re: [RFC PATCH 13/13] of: Remove unused variable overlay_symbols

[Frank Rowand]

On 10/27/16 07:41, Pantelis Antoniou wrote:
> Hi Frank,
> 
> 
>> On Oct 25, 2016, at 23:59 , frowand.list@gmail.com wrote:
>>
>> From: Frank Rowand <frank.rowand@am.sony.com>
>>
>> This unused variable is a reminder that symbols in overlays are
>> not available to subsequent overlays.  If such a feature is
>> desired then there are several ways it could be implemented.
>>
> 
> Please don’t apply that. There’s a patch that actually imports
> the symbol table from overlays that subsequent operations
> work.
> 
> Please see:
> 
> https://patchwork.kernel.org/patch/9104701/

Thanks for the pointer!  When the import symbols patch is applied
then the comment in my patch header becomes incorrect.  I will
change the patch comment to act is if the import symbols patch
is in place.

But the node pointer that my patch removes is still not used
for anything, even if the import symbols patch is applied.

Am I missing something?


> 
>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
>> ---
>> drivers/of/resolver.c | 5 +----
>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>> index 3f7cf569c7ea..b48d16200ccd 100644
>> --- a/drivers/of/resolver.c
>> +++ b/drivers/of/resolver.c
>> @@ -272,7 +272,7 @@ static int adjust_local_phandle_references(struct device_node *local_fixups,
>> int of_resolve_phandles(struct device_node *overlay)
>> {
>> 	struct device_node *child, *local_fixups, *refnode;
>> -	struct device_node *tree_symbols, *overlay_symbols, *overlay_fixups;
>> +	struct device_node *tree_symbols, *overlay_fixups;
>> 	struct property *prop;
>> 	const char *refpath;
>> 	phandle phandle, phandle_delta;
>> @@ -302,12 +302,9 @@ int of_resolve_phandles(struct device_node *overlay)
>> 	if (err)
>> 		goto err_out;
>>
>> -	overlay_symbols = NULL;
>> 	overlay_fixups = NULL;
>>
>> 	for_each_child_of_node(overlay, child) {
>> -		if (!of_node_cmp(child->name, "__symbols__"))
>> -			overlay_symbols = child;
>> 		if (!of_node_cmp(child->name, "__fixups__"))
>> 			overlay_fixups = child;
>> 	}
>> -- 
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Regards
> 
> — Pantelis
> 
> 

Re: [RFC PATCH 13/13] of: Remove unused variable overlay_symbols

[Frank Rowand]

On 10/27/16 09:27, Frank Rowand wrote:
> On 10/27/16 07:41, Pantelis Antoniou wrote:
>> Hi Frank,
>>
>>
>>> On Oct 25, 2016, at 23:59 , frowand.list@gmail.com wrote:
>>>
>>> From: Frank Rowand <frank.rowand@am.sony.com>
>>>
>>> This unused variable is a reminder that symbols in overlays are
>>> not available to subsequent overlays.  If such a feature is
>>> desired then there are several ways it could be implemented.
>>>
>>
>> Please don’t apply that. There’s a patch that actually imports
>> the symbol table from overlays that subsequent operations
>> work.
>>
>> Please see:
>>
>> https://patchwork.kernel.org/patch/9104701/
> 
> Thanks for the pointer!  When the import symbols patch is applied
> then the comment in my patch header becomes incorrect.  I will
> change the patch comment to act is if the import symbols patch
> is in place.
> 
> But the node pointer that my patch removes is still not used
> for anything, even if the import symbols patch is applied.
> 
> Am I missing something?

I was missing a later patch in the symbol import patch set that
updated resolver.c to use the imported symbols.  I'll go look
at that.

Re: [RFC PATCH 13/13] of: Remove unused variable overlay_symbols

[Frank Rowand]

On 10/27/16 09:53, Frank Rowand wrote:
> On 10/27/16 09:27, Frank Rowand wrote:
>> On 10/27/16 07:41, Pantelis Antoniou wrote:
>>> Hi Frank,
>>>
>>>
>>>> On Oct 25, 2016, at 23:59 , frowand.list@gmail.com wrote:
>>>>
>>>> From: Frank Rowand <frank.rowand@am.sony.com>
>>>>
>>>> This unused variable is a reminder that symbols in overlays are
>>>> not available to subsequent overlays.  If such a feature is
>>>> desired then there are several ways it could be implemented.
>>>>
>>>
>>> Please don’t apply that. There’s a patch that actually imports
>>> the symbol table from overlays that subsequent operations
>>> work.
>>>
>>> Please see:
>>>
>>> https://patchwork.kernel.org/patch/9104701/
>>
>> Thanks for the pointer!  When the import symbols patch is applied
>> then the comment in my patch header becomes incorrect.  I will
>> change the patch comment to act is if the import symbols patch
>> is in place.
>>
>> But the node pointer that my patch removes is still not used
>> for anything, even if the import symbols patch is applied.
>>
>> Am I missing something?
> 
> I was missing a later patch in the symbol import patch set that
> updated resolver.c to use the imported symbols.  I'll go look
> at that.

Crap.  I misread the file name in the patch that I thought was
updating resolver.c.  It was actually overlay.c.  So I am back
to my question of:

   Am I missing something?

Re: [RFC PATCH 00/13] of: Make drivers/of/resolver.c more readable

[Frank Rowand]

On 10/25/16 13:58, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@am.sony.com>
> 
> drivers/of/resolve.c is a bit difficult to read.  Clean it up so
> that review of future overlay related patches will be easier.

< snip >

Hi Pantelis,

Can you test this patch series on some real hardware?

Thanks,

Frank

Re: [RFC PATCH 00/13] of: Make drivers/of/resolver.c more readable

[Rob Herring]

On Tue, Oct 25, 2016 at 4:02 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 10/25/16 13:58, frowand.list@gmail.com wrote:
>> From: Frank Rowand <frank.rowand@am.sony.com>
>>
>> drivers/of/resolve.c is a bit difficult to read.  Clean it up so
>> that review of future overlay related patches will be easier.
>
> < snip >
>
> Hi Pantelis,
>
> Can you test this patch series on some real hardware?

Did you run unit tests?

Rob

Re: [RFC PATCH 00/13] of: Make drivers/of/resolver.c more readable

[Frank Rowand]

On 10/27/16 05:03, Rob Herring wrote:
> On Tue, Oct 25, 2016 at 4:02 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>> On 10/25/16 13:58, frowand.list@gmail.com wrote:
>>> From: Frank Rowand <frank.rowand@am.sony.com>
>>>
>>> drivers/of/resolve.c is a bit difficult to read.  Clean it up so
>>> that review of future overlay related patches will be easier.
>>
>> < snip >
>>
>> Hi Pantelis,
>>
>> Can you test this patch series on some real hardware?
> 
> Did you run unit tests?

Yes.

But I will be much happier after Pantelis runs tests on real
hardware (which he agrees to do in another reply).

Re: [RFC PATCH 00/13] of: Make drivers/of/resolver.c more readable

[Pantelis Antoniou]

Hi Frank,

> On Oct 26, 2016, at 00:02 , Frank Rowand <frowand.list@gmail.com> wrote:
> 
> On 10/25/16 13:58, frowand.list@gmail.com wrote:
>> From: Frank Rowand <frank.rowand@am.sony.com>
>> 
>> drivers/of/resolve.c is a bit difficult to read.  Clean it up so
>> that review of future overlay related patches will be easier.
> 
> < snip >
> 
> Hi Pantelis,
> 
> Can you test this patch series on some real hardware?
> 

Sure, I’ll give it whirl today. Been swamped after ELCE but now
I have a little bit of time.

> Thanks,
> 
> Frank

Regards

— Pantelis