From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Once we allow software nodes to reference other kinds of firmware nodes,
the node in args will no longer necessarily be a software node so bump
its reference count using its fwnode interface.
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
drivers/base/swnode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 2994efaf1d5d74c82df70e7df8bddf61ba0bfd41..b7c3926b67be72671ba4e4c442b3acca80688cf7 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -554,7 +554,7 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
if (!args)
return 0;
- args->fwnode = software_node_get(refnode);
+ args->fwnode = fwnode_handle_get(refnode);
args->nargs = nargs;
for (i = 0; i < nargs; i++)
--
2.48.1On Wed, Oct 29, 2025 at 01:28:36PM +0100, Bartosz Golaszewski wrote: > > Once we allow software nodes to reference other kinds of firmware nodes, > the node in args will no longer necessarily be a software node so bump > its reference count using its fwnode interface. Same, a short comment (or an update of a kernel-doc if present, I don't remember). -- With Best Regards, Andy Shevchenko
On Thu, 30 Oct 2025 10:34:46 +0100, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> said:
> On Wed, Oct 29, 2025 at 01:28:36PM +0100, Bartosz Golaszewski wrote:
>>
>> Once we allow software nodes to reference other kinds of firmware nodes,
>> the node in args will no longer necessarily be a software node so bump
>> its reference count using its fwnode interface.
>
> Same, a short comment (or an update of a kernel-doc if present, I don't
> remember).
>
Andy: the resulting code after patch 3/10 looks like this:
struct fwnode_handle *refnode;
(...)
if (ref->swnode)
refnode = software_node_fwnode(ref->swnode);
else if (ref->fwnode)
refnode = ref->fwnode;
else
return -EINVAL;
if (!refnode)
return -ENOENT;
if (nargs_prop) {
error = fwnode_property_read_u32(refnode, nargs_prop,
&nargs_prop_val);
if (error)
return error;
nargs = nargs_prop_val;
}
(...)
args->fwnode = fwnode_handle_get(refnode);
I'm typically all for comments but this code really is self-commenting.
There's nothing ambiguous about the above. We know the refnode is an fwnode,
we assign it and we pass it to the fwnode_ routines. What exactly would you
add here that would make it clearer?
Bartosz
On Thu, Oct 30, 2025 at 03:33:02AM -0700, Bartosz Golaszewski wrote:
> On Thu, 30 Oct 2025 10:34:46 +0100, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> said:
> > On Wed, Oct 29, 2025 at 01:28:36PM +0100, Bartosz Golaszewski wrote:
> >>
> >> Once we allow software nodes to reference other kinds of firmware nodes,
> >> the node in args will no longer necessarily be a software node so bump
> >> its reference count using its fwnode interface.
> >
> > Same, a short comment (or an update of a kernel-doc if present, I don't
> > remember).
> >
>
> Andy: the resulting code after patch 3/10 looks like this:
>
> struct fwnode_handle *refnode;
>
> (...)
Let's say something like below to be put here
/*
* The reference in software node may refer to a node of a different type.
* Depending on the type we choose either to use software node directly, or
* delegate that to fwnode API.
*/
> if (ref->swnode)
> refnode = software_node_fwnode(ref->swnode);
> else if (ref->fwnode)
> refnode = ref->fwnode;
> else
> return -EINVAL;
>
> if (!refnode)
> return -ENOENT;
>
> if (nargs_prop) {
> error = fwnode_property_read_u32(refnode, nargs_prop,
> &nargs_prop_val);
> if (error)
> return error;
>
> nargs = nargs_prop_val;
> }
>
> (...)
>
> args->fwnode = fwnode_handle_get(refnode);
>
> I'm typically all for comments but this code really is self-commenting.
> There's nothing ambiguous about the above. We know the refnode is an fwnode,
> we assign it and we pass it to the fwnode_ routines. What exactly would you
> add here that would make it clearer?
See above.
--
With Best Regards,
Andy Shevchenko
On Fri, Oct 31, 2025 at 9:30 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Oct 30, 2025 at 03:33:02AM -0700, Bartosz Golaszewski wrote: > > On Thu, 30 Oct 2025 10:34:46 +0100, Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> said: > > > On Wed, Oct 29, 2025 at 01:28:36PM +0100, Bartosz Golaszewski wrote: > > >> > > >> Once we allow software nodes to reference other kinds of firmware nodes, > > >> the node in args will no longer necessarily be a software node so bump > > >> its reference count using its fwnode interface. > > > > > > Same, a short comment (or an update of a kernel-doc if present, I don't > > > remember). > > > > > > > Andy: the resulting code after patch 3/10 looks like this: > > > > struct fwnode_handle *refnode; > > > > (...) > > Let's say something like below to be put here > > /* > * The reference in software node may refer to a node of a different type. > * Depending on the type we choose either to use software node directly, or > * delegate that to fwnode API. > */ > But this is incorrect: we're not really doing that. We either use the firmware node reference directly OR cast the software node to its firmware node representation. We ALWAYS use the firmware node API below. This really *is* evident from the code but if it'll make you happy and make you sign off on this, I'll add a corrected version. IMO It's completely redundant. Bartosz
On Fri, Oct 31, 2025 at 10:03:47AM +0100, Bartosz Golaszewski wrote: > On Fri, Oct 31, 2025 at 9:30 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Oct 30, 2025 at 03:33:02AM -0700, Bartosz Golaszewski wrote: > > > On Thu, 30 Oct 2025 10:34:46 +0100, Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> said: ... > > > Andy: the resulting code after patch 3/10 looks like this: > > > > > > struct fwnode_handle *refnode; > > > > > > (...) > > > > Let's say something like below to be put here > > > > /* > > * The reference in software node may refer to a node of a different type. > > * Depending on the type we choose either to use software node directly, or > > * delegate that to fwnode API. > > */ > > But this is incorrect: we're not really doing that. We either use the > firmware node reference directly OR cast the software node to its > firmware node representation. We ALWAYS use the firmware node API > below. > > This really *is* evident from the code but if it'll make you happy and > make you sign off on this, I'll add a corrected version. The comment should answer to the Q: "Why the heck are we calling fwnode APIs here?" > IMO It's completely redundant. This is unusual case for swnode API (see other functions, they call directly the low-level implementation instead of going to a round via fwnode). That's why I insist on a comment of this piece. It may be obvious for you, but the unprepared read would be surprised by this inconsistency. -- With Best Regards, Andy Shevchenko
On Fri, 31 Oct 2025 10:44:46 +0100, Andy Shevchenko <andriy.shevchenko@linux.intel.com> said: > On Fri, Oct 31, 2025 at 10:03:47AM +0100, Bartosz Golaszewski wrote: >> On Fri, Oct 31, 2025 at 9:30 AM Andy Shevchenko >> <andriy.shevchenko@linux.intel.com> wrote: >> > On Thu, Oct 30, 2025 at 03:33:02AM -0700, Bartosz Golaszewski wrote: >> > > On Thu, 30 Oct 2025 10:34:46 +0100, Andy Shevchenko >> > > <andriy.shevchenko@linux.intel.com> said: > > ... > >> > > Andy: the resulting code after patch 3/10 looks like this: >> > > >> > > struct fwnode_handle *refnode; >> > > >> > > (...) >> > >> > Let's say something like below to be put here >> > >> > /* >> > * The reference in software node may refer to a node of a different type. >> > * Depending on the type we choose either to use software node directly, or >> > * delegate that to fwnode API. >> > */ >> >> But this is incorrect: we're not really doing that. We either use the >> firmware node reference directly OR cast the software node to its >> firmware node representation. We ALWAYS use the firmware node API >> below. >> >> This really *is* evident from the code but if it'll make you happy and >> make you sign off on this, I'll add a corrected version. > > The comment should answer to the Q: "Why the heck are we calling fwnode APIs here?" > >> IMO It's completely redundant. > > This is unusual case for swnode API (see other functions, they call directly > the low-level implementation instead of going to a round via fwnode). That's > why I insist on a comment of this piece. It may be obvious for you, but the > unprepared read would be surprised by this inconsistency. > I propose to have the following: + /* + * A software node can reference other software nodes or firmware + * nodes (which are the abstraction layer sitting on top of them). + * This is done to ensure we can create references to static software + * nodes before they're registered with the firmware node framework. + * At the time the reference is being resolved, we expect the swnodes + * in question to already have been registered and to be backed by + * a firmware node. This is why we use the fwnode API below to read the + * relevant properties and bump the reference count. + */ This at least adds relevant information on *why* we're using the fwnode API. Bart
On Fri, Oct 31, 2025 at 05:27:10AM -0500, Bartosz Golaszewski wrote: > On Fri, 31 Oct 2025 10:44:46 +0100, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> said: > > On Fri, Oct 31, 2025 at 10:03:47AM +0100, Bartosz Golaszewski wrote: > >> On Fri, Oct 31, 2025 at 9:30 AM Andy Shevchenko > >> <andriy.shevchenko@linux.intel.com> wrote: > >> > On Thu, Oct 30, 2025 at 03:33:02AM -0700, Bartosz Golaszewski wrote: > >> > > On Thu, 30 Oct 2025 10:34:46 +0100, Andy Shevchenko > >> > > <andriy.shevchenko@linux.intel.com> said: ... > >> > > Andy: the resulting code after patch 3/10 looks like this: > >> > > > >> > > struct fwnode_handle *refnode; > >> > > > >> > > (...) > >> > > >> > Let's say something like below to be put here > >> > > >> > /* > >> > * The reference in software node may refer to a node of a different type. > >> > * Depending on the type we choose either to use software node directly, or > >> > * delegate that to fwnode API. > >> > */ > >> > >> But this is incorrect: we're not really doing that. We either use the > >> firmware node reference directly OR cast the software node to its > >> firmware node representation. We ALWAYS use the firmware node API > >> below. > >> > >> This really *is* evident from the code but if it'll make you happy and > >> make you sign off on this, I'll add a corrected version. > > > > The comment should answer to the Q: "Why the heck are we calling fwnode APIs here?" > > > >> IMO It's completely redundant. > > > > This is unusual case for swnode API (see other functions, they call directly > > the low-level implementation instead of going to a round via fwnode). That's > > why I insist on a comment of this piece. It may be obvious for you, but the > > unprepared read would be surprised by this inconsistency. > > > > I propose to have the following: > > + /* > + * A software node can reference other software nodes or firmware > + * nodes (which are the abstraction layer sitting on top of them). > + * This is done to ensure we can create references to static software > + * nodes before they're registered with the firmware node framework. > + * At the time the reference is being resolved, we expect the swnodes > + * in question to already have been registered and to be backed by > + * a firmware node. This is why we use the fwnode API below to read the > + * relevant properties and bump the reference count. > + */ > > This at least adds relevant information on *why* we're using the fwnode API. Yes, works for me, thanks! -- With Best Regards, Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.