Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201210202944.6747-1-semen.protsenko@linaro.org> <f18dea6e-5806-370c-0d83-e56f3dc118b4@gmail.com>
In-Reply-To: <f18dea6e-5806-370c-0d83-e56f3dc118b4@gmail.com>
From:   Sam Protsenko <semen.protsenko@linaro.org>
Date:   Fri, 11 Dec 2020 15:26:54 +0200
Message-ID: <CAPLW+4nLi2236-EL0d=X9bzv11LiBJd2c5NY5aY4qZHL3rUCgw@mail.gmail.com>
Subject: Re: [PATCH 1/2] of: property: Get rid of code duplication in port getting
To:     Dmitry Osipenko <digetx@gmail.com>
Cc:     devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Rob Herring <robh+dt@kernel.org>,
        Frank Rowand <frowand.list@gmail.com>,
        John Stultz <john.stultz@linaro.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Sam Ravnborg <sam@ravnborg.org>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Thu, 10 Dec 2020 at 22:59, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 10.12.2020 23:29, Sam Protsenko =D0=BF=D0=B8=D1=88=D0=B5=D1=82:
> > Both of_graph_is_present() and of_graph_get_next_endpoint() functions
> > share common piece of code for obtaining the graph port. Extract it int=
o
> > separate static function to get rid of code duplication and avoid
> > possible coding errors in future.
> >
> > Fixes: 4ec0a44ba8d7 ("of_graph: add of_graph_is_present()")
>
> The "fixes" tag should be used only for bug-fixes and there is no bug
> fixed in this patch.
>
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> >  drivers/of/property.c | 34 ++++++++++++++++++++--------------
> >  1 file changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 408a7b5f06a9..da111fcf37ac 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -30,13 +30,13 @@
> >  #include "of_private.h"
> >
> >  /**
> > - * of_graph_is_present() - check graph's presence
> > + * of_graph_get_port - find the "port" node in a given node
> >   * @node: pointer to device_node containing graph port
> >   *
> > - * Return: True if @node has a port or ports (with a port) sub-node,
> > - * false otherwise.
> > + * Return: A 'port' node pointer with refcount incremented if found or=
 NULL
> > + * otherwise. The caller has to use of_node_put() on it when done.
> >   */
> > -bool of_graph_is_present(const struct device_node *node)
> > +static struct device_node *of_graph_get_port(const struct device_node =
*node)
> >  {
> >       struct device_node *ports, *port;
> >
> > @@ -46,8 +46,22 @@ bool of_graph_is_present(const struct device_node *n=
ode)
> >
> >       port =3D of_get_child_by_name(node, "port");
> >       of_node_put(ports);
> > -     of_node_put(port);
> >
> > +     return port;
> > +}
> > +
> > +/**
> > + * of_graph_is_present() - check graph's presence
> > + * @node: pointer to device_node containing graph port
> > + *
> > + * Return: True if @node has a port or ports (with a port) sub-node,
> > + * false otherwise.
> > + */
> > +bool of_graph_is_present(const struct device_node *node)
> > +{
> > +     struct device_node *port =3D of_graph_get_port(node);
> > +
> > +     of_node_put(port);
> >       return !!port;
> >  }
> >  EXPORT_SYMBOL(of_graph_is_present);
> > @@ -631,15 +645,7 @@ struct device_node *of_graph_get_next_endpoint(con=
st struct device_node *parent,
> >        * parent port node.
> >        */
> >       if (!prev) {
> > -             struct device_node *node;
> > -
> > -             node =3D of_get_child_by_name(parent, "ports");
> > -             if (node)
> > -                     parent =3D node;
> > -
> > -             port =3D of_get_child_by_name(parent, "port");
> > -             of_node_put(node);
> > -
> > +             port =3D of_graph_get_port(parent);
> >               if (!port) {
> >                       pr_err("graph: no port node found in %pOF\n", par=
ent);
> >                       return NULL;
> >
>
> This repeats the problem which was made once before:
>

You are right. Inlining is probably the best solution here. Let's drop
this patch and keep everything as is. Thanks for catching this!

> https://lore.kernel.org/patchwork/patch/1266028/#1461493