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

[Ulf Hansson <ulf.hansson@linaro.org>]

On Thu, 2 Sept 2021 at 00:06, Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Sep 1, 2021 at 2:27 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Wed, 1 Sept 2021 at 22:56, Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Wed, Sep 1, 2021 at 12:45 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Tue, 31 Aug 2021 at 19:31, Saravana Kannan <saravanak@google.com> wrote:
> > > > >
> > > > > On Tue, Aug 31, 2021 at 3:21 AM Ulf Hansson <ulf.hansson@linaro.org> 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@linaro.org>
> > > > > > ---
> > > > > >  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?
>
> If you feel so strong about it, go for it. No strong objections. Just
> double check the refcounts are done correctly.

The refcounts should be okay, I think.

I am fine with either of the two suggestions I have made. But another
option could be to come up with an alternative name (and a
description) for the flag, instead of "optional_con_dev", if you
perhaps have a better suggestion?

>
> -Saravana

Kind regards
Uffe