Re: [PATCH 1/2] of: property: fw_devlink: Rename 'node_not_dev' to 'optional_con_dev'

[Ulf Hansson]

On Wed, 1 Sept 2021 at 22:56, Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> On Wed, Sep 1, 2021 at 12:45 AM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> >
> > On Tue, 31 Aug 2021 at 19:31, Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Aug 31, 2021 at 3:21 AM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> > > >
> > > > In the struct supplier_bindings the member 'node_not_dev' is described as
> > > > "The consumer node containing the property is never a device.", but that
> > > > doesn't match the behaviour of the code in of_link_property().
> > > >
> > > > To make the behaviour consistent with the description, let's rename the
> > > > member to "optional_con_dev" and clarify the corresponding comment.
> > > >
> > > > Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > > > ---
> > > >  drivers/of/property.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > > index 6c028632f425..2babb1807228 100644
> > > > --- a/drivers/of/property.c
> > > > +++ b/drivers/of/property.c
> > > > @@ -1249,7 +1249,8 @@ static struct device_node *parse_##fname(struct device_node *np,       \
> > > >   * @parse_prop.index: For properties holding a list of phandles, this is the
> > > >   *                   index into the list
> > > >   * @optional: Describes whether a supplier is mandatory or not
> > > > - * @node_not_dev: The consumer node containing the property is never a device.
> > > > + * @optional_con_dev: The consumer node containing the property may not be a
> > > > + *                   device, then try finding one from an ancestor node.
> > >
> > > Nak. This flag is not about "may not be". This is explicitly for
> > > "never a device". It has to do with stuff like remote-endpoint which
> > > is never listed under the root node of the device node. Your
> > > documentation change is changing the meaning of the flag.
> >
> > Okay, fair enough.
> >
> > Although, as stated in the commit message this isn't the way code
> > behaves. Shouldn't we at least make the behaviour consistent with the
> > description of the 'node_not_dev' flag?
>
> I know what you mean, but if you use the flag correctly (where the
> phandle pointed to will never be a device with compatible property),
> the existing code would work correctly. And since the flag is relevant
> only in this file, it's easy to keep it correct. I'd just leave it as
> is.

Sorry, but that just sounds lazy to me, I am sure we can do better.
The current code and the name of the flag is confusing, at least to me
(and I bet to others as well).

Moreover, I don't quite understand your objections to changing this.
Why leave this to be inconsistent when it can be easily fixed?

>
> -Saravana

Kind regards
Uffe

>
>
> >
> > Along the lines of the below patch then?
> >
> > From: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > Date: Wed, 1 Sep 2021 09:28:03 +0200
> > Subject: [PATCH] of: property: fw_devlink: Fixup behaviour when 'node_not_dev'
> >  is set
> >
> > In the struct supplier_bindings the member 'node_not_dev' is described as
> > "The consumer node containing the property is never a device.", but that is
> > inconsistent with the behaviour of the code in of_link_property(), as it
> > calls of_get_compat_node() that starts parsing for a compatible property,
> > starting from the node it gets passed to it.
> >
> > Make the behaviour consistent with the description of the 'node_not_dev'
> > flag, by passing the parent node to of_get_compat_node() instead.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > ---
> >  drivers/of/property.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 6c028632f425..16ee017884b8 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1075,6 +1075,17 @@ static struct device_node
> > *of_get_compat_node(struct device_node *np)
> >         return np;
> >  }
> >
> > +static struct device_node *of_get_compat_node_parent(struct device_node *np)
> > +{
> > +       struct device_node *parent, *node;
> > +
> > +       parent = of_get_parent(np);
> > +       node = of_get_compat_node(parent);
> > +       of_node_put(parent);
> > +
> > +       return node;
> > +}
> > +
> >  /**
> >   * of_link_to_phandle - Add fwnode link to supplier from supplier phandle
> >   * @con_np: consumer device tree node
> > @@ -1416,7 +1427,7 @@ static int of_link_property(struct device_node
> > *con_np, const char *prop_name)
> >                         struct device_node *con_dev_np;
> >
> >                         con_dev_np = s->node_not_dev
> > -                                       ? of_get_compat_node(con_np)
> > +                                       ? of_get_compat_node_parent(con_np)
> >                                         : of_node_get(con_np);
> >                         matched = true;
> >                         i++;
> > --
> > 2.25.1
> >
> > [...]
> >
> > Kind regards
> > Uffe