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
