Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1508630 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 00/13] of: Make drivers/of/resolver.c more readable frowand.list@gmail.com - 2016-10-25 23:10 +0200
    [RFC PATCH 05/13] of: Rename functions to more accurately reflect what they do frowand.list@gmail.com - 2016-10-25 23:10 +0200
    Re: [RFC PATCH 00/13] of: Make drivers/of/resolver.c more readable Frank Rowand <frowand.list@gmail.com> - 2016-10-25 23:10 +0200
      Re: [RFC PATCH 00/13] of: Make drivers/of/resolver.c more readable Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2016-10-27 16:00 +0200
      Re: [RFC PATCH 00/13] of: Make drivers/of/resolver.c more readable Rob Herring <robh+dt@kernel.org> - 2016-10-27 16:40 +0200
        Re: [RFC PATCH 00/13] of: Make drivers/of/resolver.c more readable Frank Rowand <frowand.list@gmail.com> - 2016-10-27 18:40 +0200
    [RFC PATCH 09/13] of: Remove redundant size check frowand.list@gmail.com - 2016-10-25 23:10 +0200
    [RFC PATCH 03/13] of: Remove braces around single line blocks. frowand.list@gmail.com - 2016-10-25 23:10 +0200
    [RFC PATCH 08/13] of: Update structure of code, remove BUG_ON() frowand.list@gmail.com - 2016-10-25 23:10 +0200
    [RFC PATCH 07/13] of: Rename variables to better reflect purpose or follow convention frowand.list@gmail.com - 2016-10-25 23:10 +0200
    [RFC PATCH 06/13] of: Remove prefix "__of_" and prefix "__" from local function names frowand.list@gmail.com - 2016-10-25 23:10 +0200
      Re: [RFC PATCH 06/13] of: Remove prefix "__of_" and prefix "__" from  local function names Rob Herring <robh+dt@kernel.org> - 2016-10-27 16:20 +0200
        Re: [RFC PATCH 06/13] of: Remove prefix "__of_" and prefix "__" from  local function names Frank Rowand <frowand.list@gmail.com> - 2016-10-27 18:40 +0200
          Re: [RFC PATCH 06/13] of: Remove prefix "__of_" and prefix "__" from  local function names Rob Herring <robh+dt@kernel.org> - 2016-10-27 19:00 +0200
            Re: [RFC PATCH 06/13] of: Remove prefix "__of_" and prefix "__" from  local function names Frank Rowand <frowand.list@gmail.com> - 2016-10-27 20:30 +0200
              Re: [RFC PATCH 06/13] of: Remove prefix "__of_" and prefix "__" from  local function names Rob Herring <robh+dt@kernel.org> - 2016-10-27 22:30 +0200
    [RFC PATCH 01/13] of: Remove comments that state the obvious frowand.list@gmail.com - 2016-10-25 23:10 +0200
      Re: [RFC PATCH 01/13] of: Remove comments that state the obvious Joe Perches <joe@perches.com> - 2016-10-25 23:40 +0200
      Re: [RFC PATCH 01/13] of: Remove comments that state the obvious Rob Herring <robh+dt@kernel.org> - 2016-10-27 16:50 +0200
        Re: [RFC PATCH 01/13] of: Remove comments that state the obvious Frank Rowand <frowand.list@gmail.com> - 2016-10-27 18:10 +0200
    [RFC PATCH 04/13] of: Convert comparisons to zero or NULL to simplify logical expressions frowand.list@gmail.com - 2016-10-25 23:10 +0200
    [RFC PATCH 12/13] of: Move setting of pointer to beside test for non-null frowand.list@gmail.com - 2016-10-25 23:10 +0200
    [RFC PATCH 10/13] of: Update comments to reflect changes and increase clarity frowand.list@gmail.com - 2016-10-25 23:10 +0200
    [RFC PATCH 02/13] of: Remove excessive printks to reduce clutter frowand.list@gmail.com - 2016-10-25 23:10 +0200
      Re: [RFC PATCH 02/13] of: Remove excessive printks to reduce clutter Pantelis Antoniou <pantelis.antoniou@konsulko.com> - 2016-10-27 16:00 +0200
        Re: [RFC PATCH 02/13] of: Remove excessive printks to reduce clutter Frank Rowand <frowand.list@gmail.com> - 2016-10-27 18:20 +0200
      Re: [RFC PATCH 02/13] of: Remove excessive printks to reduce clutter Rob Herring <robh+dt@kernel.org> - 2016-10-27 17:00 +0200
        Re: [RFC PATCH 02/13] of: Remove excessive printks to reduce clutter Frank Rowand <frowand.list@gmail.com> - 2016-10-27 18:10 +0200

Page 1 of 2  [1] 2  Next page →

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1508633

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

[toc] | [prev] | [next] | [standalone]

#1510187

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

[toc] | [prev] | [next] | [standalone]

#1510297

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

[toc] | [prev] | [next] | [standalone]

#1510474

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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
> 

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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 &&

[toc] | [prev] | [next] | [standalone]

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

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 &&
> 

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web