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 <redacted>
---
drivers/of/property.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
On Tue, Aug 31, 2021 at 3:21 AM Ulf Hansson [off-list ref] wrote:
quoted hunk
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 <redacted>
---
drivers/of/property.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
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.
-Saravana
On Tue, 31 Aug 2021 at 19:31, Saravana Kannan [off-list ref] wrote:
On Tue, Aug 31, 2021 at 3:21 AM Ulf Hansson [off-list ref] wrote:
quoted
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 <redacted>
---
drivers/of/property.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
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 <redacted>
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 <redacted>
---
drivers/of/property.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
On Wed, Sep 1, 2021 at 12:45 AM Ulf Hansson [off-list ref] wrote:
On Tue, 31 Aug 2021 at 19:31, Saravana Kannan [off-list ref] wrote:
quoted
On Tue, Aug 31, 2021 at 3:21 AM Ulf Hansson [off-list ref] wrote:
quoted
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 <redacted>
---
drivers/of/property.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
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
quoted hunk
Along the lines of the below patch then?
From: Ulf Hansson <redacted>
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 <redacted>
---
drivers/of/property.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
On Wed, 1 Sept 2021 at 22:56, Saravana Kannan [off-list ref] wrote:
On Wed, Sep 1, 2021 at 12:45 AM Ulf Hansson [off-list ref] wrote:
quoted
On Tue, 31 Aug 2021 at 19:31, Saravana Kannan [off-list ref] wrote:
quoted
On Tue, Aug 31, 2021 at 3:21 AM Ulf Hansson [off-list ref] wrote:
quoted
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 <redacted>
---
drivers/of/property.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
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
quoted
Along the lines of the below patch then?
From: Ulf Hansson <redacted>
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 <redacted>
---
drivers/of/property.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
On Wed, Sep 1, 2021 at 2:27 PM Ulf Hansson [off-list ref] wrote:
On Wed, 1 Sept 2021 at 22:56, Saravana Kannan [off-list ref] wrote:
quoted
On Wed, Sep 1, 2021 at 12:45 AM Ulf Hansson [off-list ref] wrote:
quoted
On Tue, 31 Aug 2021 at 19:31, Saravana Kannan [off-list ref] wrote:
quoted
On Tue, Aug 31, 2021 at 3:21 AM Ulf Hansson [off-list ref] wrote:
quoted
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 <redacted>
---
drivers/of/property.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, 2 Sept 2021 at 00:06, Saravana Kannan [off-list ref] wrote:
On Wed, Sep 1, 2021 at 2:27 PM Ulf Hansson [off-list ref] wrote:
quoted
On Wed, 1 Sept 2021 at 22:56, Saravana Kannan [off-list ref] wrote:
quoted
On Wed, Sep 1, 2021 at 12:45 AM Ulf Hansson [off-list ref] wrote:
quoted
On Tue, 31 Aug 2021 at 19:31, Saravana Kannan [off-list ref] wrote:
quoted
On Tue, Aug 31, 2021 at 3:21 AM Ulf Hansson [off-list ref] wrote:
quoted
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 <redacted>
---
drivers/of/property.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
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?
On Wed, Sep 1, 2021 at 3:24 PM Ulf Hansson [off-list ref] wrote:
On Thu, 2 Sept 2021 at 00:06, Saravana Kannan [off-list ref] wrote:
quoted
On Wed, Sep 1, 2021 at 2:27 PM Ulf Hansson [off-list ref] wrote:
quoted
On Wed, 1 Sept 2021 at 22:56, Saravana Kannan [off-list ref] wrote:
quoted
On Wed, Sep 1, 2021 at 12:45 AM Ulf Hansson [off-list ref] wrote:
quoted
On Tue, 31 Aug 2021 at 19:31, Saravana Kannan [off-list ref] wrote:
quoted
On Tue, Aug 31, 2021 at 3:21 AM Ulf Hansson [off-list ref] wrote:
quoted
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 <redacted>
---
drivers/of/property.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel