| Message ID | 20210831102125.624661-1-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] of: property: fw_devlink: Rename 'node_not_dev' to 'optional_con_dev' | expand |
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? Along the lines of the below patch then? From: Ulf Hansson <ulf.hansson@linaro.org> 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@linaro.org> --- 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++;
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. -Saravana > > Along the lines of the below patch then? > > From: Ulf Hansson <ulf.hansson@linaro.org> > 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@linaro.org> > --- > 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
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. -Saravana
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
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. * * Returns: * parse_prop() return values are @@ -1261,7 +1262,7 @@ struct supplier_bindings { struct device_node *(*parse_prop)(struct device_node *np, const char *prop_name, int index); bool optional; - bool node_not_dev; + bool optional_con_dev; }; DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells") @@ -1370,7 +1371,7 @@ static const struct supplier_bindings of_supplier_bindings[] = { { .parse_prop = parse_pinctrl6, }, { .parse_prop = parse_pinctrl7, }, { .parse_prop = parse_pinctrl8, }, - { .parse_prop = parse_remote_endpoint, .node_not_dev = true, }, + { .parse_prop = parse_remote_endpoint, .optional_con_dev = true, }, { .parse_prop = parse_gpio_compat, }, { .parse_prop = parse_interrupts, }, { .parse_prop = parse_regulators, }, @@ -1415,7 +1416,7 @@ static int of_link_property(struct device_node *con_np, const char *prop_name) while ((phandle = s->parse_prop(con_np, prop_name, i))) { struct device_node *con_dev_np; - con_dev_np = s->node_not_dev + con_dev_np = s->optional_con_dev ? of_get_compat_node(con_np) : of_node_get(con_np); matched = true;
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(-)