| Message ID | 20251006-reset-gpios-swnodes-v1-1-6d3325b9af42@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | reset: rework reset-gpios handling | expand |
On Mon, Oct 06, 2025 at 03:00:16PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Once we allow software nodes to reference all kinds of firmware nodes, > the refnode here will no longer necessarily be a software node so read > its proprties going through its fwnode implementation. This needs a comment in the code.
On Sat, Oct 18, 2025 at 7:35 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Oct 06, 2025 at 03:00:16PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Once we allow software nodes to reference all kinds of firmware nodes, > > the refnode here will no longer necessarily be a software node so read > > its proprties going through its fwnode implementation. > > This needs a comment in the code. > Honestly after a second glance, I disagree. Literally a few lines before we do: refnode = software_node_fwnode(ref->node); We know very well what refnode is here and why we should use fwnode API. If anything, the previous use of direct property routines was unusual. A comment would be redundant as the code is self-describing, what do you even want me to write there? Bartosz
Hi Bartosz, On Wed, Oct 22, 2025 at 09:51:44AM +0200, Bartosz Golaszewski wrote: > On Sat, Oct 18, 2025 at 7:35 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Oct 06, 2025 at 03:00:16PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > Once we allow software nodes to reference all kinds of firmware nodes, > > > the refnode here will no longer necessarily be a software node so read > > > its proprties going through its fwnode implementation. > > > > This needs a comment in the code. > > > > Honestly after a second glance, I disagree. Literally a few lines before we do: > > refnode = software_node_fwnode(ref->node); > > We know very well what refnode is here and why we should use fwnode > API. If anything, the previous use of direct property routines was > unusual. A comment would be redundant as the code is self-describing, > what do you even want me to write there? Given that the only way the three implementations of fwnode have interacted in the past has been via the secondary pointer (for software nodes) and that this will continue to be an exception, I'd also add a comment. E.g. /* ref->node may be non-software node fwnode */
On Wed, Oct 22, 2025 at 10:24 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Bartosz, > > On Wed, Oct 22, 2025 at 09:51:44AM +0200, Bartosz Golaszewski wrote: > > On Sat, Oct 18, 2025 at 7:35 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Mon, Oct 06, 2025 at 03:00:16PM +0200, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > Once we allow software nodes to reference all kinds of firmware nodes, > > > > the refnode here will no longer necessarily be a software node so read > > > > its proprties going through its fwnode implementation. > > > > > > This needs a comment in the code. > > > > > > > Honestly after a second glance, I disagree. Literally a few lines before we do: > > > > refnode = software_node_fwnode(ref->node); > > > > We know very well what refnode is here and why we should use fwnode > > API. If anything, the previous use of direct property routines was > > unusual. A comment would be redundant as the code is self-describing, > > what do you even want me to write there? > > Given that the only way the three implementations of fwnode have interacted > in the past has been via the secondary pointer (for software nodes) and > that this will continue to be an exception, I'd also add a comment. E.g. > > /* ref->node may be non-software node fwnode */ > But this becomes very clear after patch 3/9 just from looking at the code. Even after I removed the union, we still check for ref->swnode and ref->fwnode and proceeded accordingly. Let me send a v2 and please look at the resulting code after patch 3/9. Tell me if you still think it needs a comment. Bart
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index be1e9e61a7bf4d1301a3e109628517cfd9214704..cc48cff54d9c3d4d257095b6cb4a7869bf657373 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -540,9 +540,8 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode, return -ENOENT; if (nargs_prop) { - error = property_entry_read_int_array(ref->node->properties, - nargs_prop, sizeof(u32), - &nargs_prop_val, 1); + error = fwnode_property_read_u32_array(refnode, nargs_prop, + &nargs_prop_val, 1); if (error) return error;