Re: [PATCH 1/2] of: property: Get rid of code duplication in port getting

[Sam Protsenko]

Hi,

On Thu, 10 Dec 2020 at 22:59, Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
>
> 10.12.2020 23:29, Sam Protsenko пишет:
> > Both of_graph_is_present() and of_graph_get_next_endpoint() functions
> > share common piece of code for obtaining the graph port. Extract it into
> > 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@xxxxxxxxxx>
> > ---
> >  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 *node)
> >
> >       port = 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 = 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(const struct device_node *parent,
> >        * parent port node.
> >        */
> >       if (!prev) {
> > -             struct device_node *node;
> > -
> > -             node = of_get_child_by_name(parent, "ports");
> > -             if (node)
> > -                     parent = node;
> > -
> > -             port = of_get_child_by_name(parent, "port");
> > -             of_node_put(node);
> > -
> > +             port = of_graph_get_port(parent);
> >               if (!port) {
> >                       pr_err("graph: no port node found in %pOF\n", parent);
> >                       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