Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S942760AbcJ0QCz (ORCPT ); Thu, 27 Oct 2016 12:02:55 -0400 Received: from mail-pf0-f194.google.com ([209.85.192.194]:35591 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933709AbcJ0QCw (ORCPT ); Thu, 27 Oct 2016 12:02:52 -0400 Subject: Re: [RFC PATCH 01/13] of: Remove comments that state the obvious To: Rob Herring References: <1477429146-27039-1-git-send-email-frowand.list@gmail.com> <1477429146-27039-2-git-send-email-frowand.list@gmail.com> Cc: Pantelis Antoniou , Pantelis Antoniou , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" From: Frank Rowand Message-ID: <58122503.2070201@gmail.com> Date: Thu, 27 Oct 2016 09:02:11 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3838 Lines: 117 On 10/27/16 05:18, Rob Herring wrote: > On Tue, Oct 25, 2016 at 3:58 PM, wrote: >> From: Frank Rowand >> >> 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 >> --- >> 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 && >