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@xxxxxxxxx> wrote:
>> From: Frank Rowand <frank.rowand@xxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxx>
>> ---
>>  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 &&
>