lkml.org 
[lkml]   [2016]   [Oct]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH 01/13] of: Remove comments that state the obvious
From
Date
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 &&
>

\
 
 \ /
  Last update: 2016-10-27 18:03    [W:0.106 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site