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

[Saravana Kannan]

On Wed, Sep 1, 2021 at 3:24 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>
> On Thu, 2 Sept 2021 at 00:06, Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> >
> > On Wed, Sep 1, 2021 at 2:27 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> > >
> > > 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?
> >
> > 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.

The one I Nak-ed earlier is not okay. So I think we have only one other option.

> 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?

It's not fully clear to me what part of node_not_dev is confusing, but
I'll take a stab at it. How about:
- * @node_not_dev: The consumer node containing the property is never a device.
+ * @con_node_never_dev: The consumer node containing the property is never
+ *                     converted to a struct device. The struct device will be
+ *                     created for one of the ancestor nodes, which fw_devlink
+ *                     assumes would an ancestor with the compatible property.

If you are happy with this, then go ahead and use this name/comment
with the last code suggestion you made and send out a patch?

Thanks,
Saravana