[PATCH v3 00/10] reset: rework reset-gpios handling

[PATCH v3 00/10] reset: rework reset-gpios handling

[Bartosz Golaszewski]

Machine GPIO lookup is a nice, if a bit clunky, mechanism when we have
absolutely no idea what the GPIO provider is or when it will be created.
However in the case of reset-gpios, we not only know if the chip is
there - we also already hold a reference to its firmware node.

In this case using fwnode lookup makes more sense. However, since the
reset provider is created dynamically, it doesn't have a corresponding
firmware node (in this case: an OF-node). That leaves us with software
nodes which currently cannot reference other implementations of the
fwnode API, only other struct software_node objects. This is a needless
limitation as it's imaginable that a dynamic auxiliary device (with a
software node attached) would want to reference a real device with an OF
node.

This series does three things: extends the software node implementation,
allowing its properties to reference not only static software nodes but
also existing firmware nodes, updates the GPIO property interface to use
the reworked swnode macros and finally makes the reset-gpio code the
first user by converting the GPIO lookup from machine to swnode.

Another user of the software node changes in the future could become the
shared GPIO modules that's in the works in parallel[1].

Merging strategy: the series is logically split into three parts: driver
core, GPIO and reset respectively. However there are build-time
dependencies between all three parts so I suggest the reset tree as the
right one to take it upstream with an immutable branch provided to
driver core and GPIO.

[1] https://lore.kernel.org/all/20250924-gpio-shared-v1-0-775e7efeb1a3@linaro.org/

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Changes in v3:
- Really fix the typo in commit message in patch 7/9
- Update the commit message in patch 3/9 after implementation changes
- Don't remove checking the refnode for NULL and returning -ENOENT
- Move lockdep assertion higher up in the reset code
- Simplify patch 4/9: don't change the logic of inspecting the gpio
  device's software node
- Add new patch that still allows GPIO lookup from software nodes to
  find chips associated with any firmware nodes
- Drop the comma in reset-gpio auxiliary ID
- Drop the no longer used type argument from software node reference
  macros
- Link to v2: https://lore.kernel.org/r/20251022-reset-gpios-swnodes-v2-0-69088530291b@linaro.org

Changes in v2:
- Don't use a union for different pointer types in the software node
  reference struct
- Use fwnode_property_read_u32() instead of
  fwnode_property_read_u32_array() as we're only reading a single
  integer
- Rename reset_aux_device_release() to reset_gpio_aux_device_release()
- Initialize the device properties instead of memsetting them
- Fix typo in commit message
- As discussed on the list: I didn't change patch 7/9 because most of
  it goes away anyway in patch 9/9 and the cleanup issues will be fixed
  in the upcoming fwnode conversion
- Link to v1: https://lore.kernel.org/r/20251006-reset-gpios-swnodes-v1-0-6d3325b9af42@linaro.org

---
Bartosz Golaszewski (10):
      software node: read the reference args via the fwnode API
      software node: increase the reference of the swnode by its fwnode
      software node: allow referencing firmware nodes
      gpio: swnode: don't use the swnode's name as the key for GPIO lookup
      gpio: swnode: allow referencing GPIO chips by firmware nodes
      gpio: swnode: update the property definitions
      reset: order includes alphabetically in reset/core.c
      reset: make the provider of reset-gpios the parent of the reset device
      reset: gpio: convert the driver to using the auxiliary bus
      reset: gpio: use software nodes to setup the GPIO lookup

 drivers/base/swnode.c         |  20 ++++--
 drivers/gpio/gpiolib-swnode.c |  21 +++---
 drivers/reset/Kconfig         |   1 +
 drivers/reset/core.c          | 149 ++++++++++++++++++++++++------------------
 drivers/reset/reset-gpio.c    |  19 +++---
 include/linux/gpio/property.h |   5 +-
 include/linux/property.h      |  38 +++++++++--
 7 files changed, 156 insertions(+), 97 deletions(-)
---
base-commit: 5218492f1eb93da3929f0d993d2995a4665ca760
change-id: 20250925-reset-gpios-swnodes-db553e67095b

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[PATCH v3 01/10] software node: read the reference args via the fwnode API

[Bartosz Golaszewski]

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.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/base/swnode.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index be1e9e61a7bf4d1301a3e109628517cfd9214704..2994efaf1d5d74c82df70e7df8bddf61ba0bfd41 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(refnode, nargs_prop,
+						 &nargs_prop_val);
 		if (error)
 			return error;
 

-- 
2.48.1

[PATCH v3 02/10] software node: increase the reference of the swnode by its fwnode

[Bartosz Golaszewski]

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.1

[PATCH v3 03/10] software node: allow referencing firmware nodes

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

At the moment software nodes can only reference other software nodes.
This is a limitation for devices created, for instance, on the auxiliary
bus with a dynamic software node attached which cannot reference devices
the firmware node of which is "real" (as an OF node or otherwise).

Make it possible for a software node to reference all firmware nodes in
addition to static software nodes. To that end: add a second pointer to
struct software_node_ref_args of type struct fwnode_handle. The core
swnode code will first check the swnode pointer and if it's NULL, it
will assume the fwnode pointer should be set. Rework the helper macros
and deprecate the existing ones whose names don't indicate the reference
type.

Software node graphs remain the same, as in: the remote endpoints still
have to be software nodes.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/base/swnode.c    | 13 +++++++++++--
 include/linux/property.h | 38 +++++++++++++++++++++++++++++++-------
 2 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index b7c3926b67be72671ba4e4c442b3acca80688cf7..8601d1612be31febb6abbbe1fb35228499480c56 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -535,7 +535,13 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 	ref_array = prop->pointer;
 	ref = &ref_array[index];
 
-	refnode = software_node_fwnode(ref->node);
+	if (ref->swnode)
+		refnode = software_node_fwnode(ref->swnode);
+	else if (ref->fwnode)
+		refnode = ref->fwnode;
+	else
+		return -EINVAL;
+
 	if (!refnode)
 		return -ENOENT;
 
@@ -634,7 +640,10 @@ software_node_graph_get_remote_endpoint(const struct fwnode_handle *fwnode)
 
 	ref = prop->pointer;
 
-	return software_node_get(software_node_fwnode(ref[0].node));
+	if (!ref->swnode)
+		return NULL;
+
+	return software_node_get(software_node_fwnode(ref[0].swnode));
 }
 
 static struct fwnode_handle *
diff --git a/include/linux/property.h b/include/linux/property.h
index 50b26589dd70d1756f3b8644255c24a011e2617c..66640b3a4cba21e65e562694691f18ecb2aeae18 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -355,23 +355,35 @@ struct software_node;
 
 /**
  * struct software_node_ref_args - Reference property with additional arguments
- * @node: Reference to a software node
+ * @swnode: Reference to a software node
+ * @fwnode: Alternative reference to a firmware node handle
  * @nargs: Number of elements in @args array
  * @args: Integer arguments
  */
 struct software_node_ref_args {
-	const struct software_node *node;
+	const struct software_node *swnode;
+	struct fwnode_handle *fwnode;
 	unsigned int nargs;
 	u64 args[NR_FWNODE_REFERENCE_ARGS];
 };
 
-#define SOFTWARE_NODE_REFERENCE(_ref_, ...)			\
+#define __SOFTWARE_NODE_REF(_ref, _node, ...)			\
 (const struct software_node_ref_args) {				\
-	.node = _ref_,						\
+	._node = _ref,						\
 	.nargs = COUNT_ARGS(__VA_ARGS__),			\
 	.args = { __VA_ARGS__ },				\
 }
 
+#define SOFTWARE_NODE_REF_SWNODE(_ref, ...)			\
+	__SOFTWARE_NODE_REF(_ref, swnode, __VA_ARGS__)
+
+#define SOFTWARE_NODE_REF_FWNODE(_ref, ...)			\
+	__SOFTWARE_NODE_REF(_ref, fwnode, __VA_ARGS__)
+
+/* DEPRECATED, use SOFTWARE_NODE_REF_SWNODE() instead. */
+#define SOFTWARE_NODE_REFERENCE(_ref, ...)			\
+	SOFTWARE_NODE_REF_SWNODE(_ref, __VA_ARGS__)
+
 /**
  * struct property_entry - "Built-in" device property representation.
  * @name: Name of the property.
@@ -463,14 +475,26 @@ struct property_entry {
 #define PROPERTY_ENTRY_STRING(_name_, _val_)				\
 	__PROPERTY_ENTRY_ELEMENT(_name_, str, STRING, _val_)
 
-#define PROPERTY_ENTRY_REF(_name_, _ref_, ...)				\
+#define __PROPERTY_ENTRY_REF(_type, _name, _ref, ...)			\
 (struct property_entry) {						\
-	.name = _name_,							\
+	.name = _name,							\
 	.length = sizeof(struct software_node_ref_args),		\
 	.type = DEV_PROP_REF,						\
-	{ .pointer = &SOFTWARE_NODE_REFERENCE(_ref_, ##__VA_ARGS__), },	\
+	{ .pointer = &_type(_ref, ##__VA_ARGS__), },			\
 }
 
+#define PROPERTY_ENTRY_REF_SWNODE(_name, _ref, ...)			\
+	__PROPERTY_ENTRY_REF(SOFTWARE_NODE_REF_SWNODE,			\
+			     _name, _ref, __VA_ARGS__)
+
+#define PROPERTY_ENTRY_REF_FWNODE(_name, _ref, ...)			\
+	__PROPERTY_ENTRY_REF(SOFTWARE_NODE_REF_FWNODE,			\
+			    _name, _ref, __VA_ARGS__)
+
+/* DEPRECATED, use PROPERTY_ENTRY_REF_SWNODE() instead. */
+#define PROPERTY_ENTRY_REF(_name, _ref, ...)				\
+	PROPERTY_ENTRY_REF_SWNODE(_name, _ref, __VA_ARGS__)
+
 #define PROPERTY_ENTRY_BOOL(_name_)		\
 (struct property_entry) {			\
 	.name = _name_,				\

-- 
2.48.1

[PATCH v3 04/10] gpio: swnode: don't use the swnode's name as the key for GPIO lookup

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Looking up a GPIO controller by label that is the name of the software
node is wonky at best - the GPIO controller driver is free to set
a different label than the name of its firmware node. We're already being
passed a firmware node handle attached to the GPIO device to
swnode_get_gpio_device() so use it instead for a more precise lookup.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-swnode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-swnode.c
index f21dbc28cf2c8c2d06d034b7c89d302cc52bb9b5..e3806db1c0e077d76fcc71a50ca40bbf6872ca40 100644
--- a/drivers/gpio/gpiolib-swnode.c
+++ b/drivers/gpio/gpiolib-swnode.c
@@ -41,7 +41,7 @@ static struct gpio_device *swnode_get_gpio_device(struct fwnode_handle *fwnode)
 	    !strcmp(gdev_node->name, GPIOLIB_SWNODE_UNDEFINED_NAME))
 		return ERR_PTR(-ENOENT);
 
-	gdev = gpio_device_find_by_label(gdev_node->name);
+	gdev = gpio_device_find_by_fwnode(fwnode);
 	return gdev ?: ERR_PTR(-EPROBE_DEFER);
 }
 

-- 
2.48.1

[PATCH v3 05/10] gpio: swnode: allow referencing GPIO chips by firmware nodes

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

When doing a software node lookup, we require both the fwnode that
references a GPIO chip as well as the node associated with that chip to
be software nodes. However, we now allow referencing generic firmware
nodes from software nodes in driver core so we should allow the same in
GPIO core. Make the software node name check optional and dependent on
whether the referenced firmware node is a software node. If it's not,
just continue with the lookup.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-swnode.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-swnode.c
index e3806db1c0e077d76fcc71a50ca40bbf6872ca40..16af83fcc5aa886dd009dedc26b1ac23e5cbc4ea 100644
--- a/drivers/gpio/gpiolib-swnode.c
+++ b/drivers/gpio/gpiolib-swnode.c
@@ -30,16 +30,15 @@ static struct gpio_device *swnode_get_gpio_device(struct fwnode_handle *fwnode)
 	struct gpio_device *gdev;
 
 	gdev_node = to_software_node(fwnode);
-	if (!gdev_node || !gdev_node->name)
-		return ERR_PTR(-EINVAL);
-
-	/*
-	 * Check for a special node that identifies undefined GPIOs, this is
-	 * primarily used as a key for internal chip selects in SPI bindings.
-	 */
-	if (IS_ENABLED(CONFIG_GPIO_SWNODE_UNDEFINED) &&
-	    !strcmp(gdev_node->name, GPIOLIB_SWNODE_UNDEFINED_NAME))
-		return ERR_PTR(-ENOENT);
+	if (gdev_node && gdev_node->name) {
+		/*
+		 * Check for a special node that identifies undefined GPIOs, this is
+		 * primarily used as a key for internal chip selects in SPI bindings.
+		 */
+		if (IS_ENABLED(CONFIG_GPIO_SWNODE_UNDEFINED) &&
+		    !strcmp(gdev_node->name, GPIOLIB_SWNODE_UNDEFINED_NAME))
+			return ERR_PTR(-ENOENT);
+	}
 
 	gdev = gpio_device_find_by_fwnode(fwnode);
 	return gdev ?: ERR_PTR(-EPROBE_DEFER);

-- 
2.48.1

[PATCH v3 06/10] gpio: swnode: update the property definitions

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Use the recommended macros for creating references to software and
firmware nodes attached to GPIO providers.

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 include/linux/gpio/property.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/gpio/property.h b/include/linux/gpio/property.h
index 0d220930800276a21b5ba96a68371ce66fc4ae3e..6b1c2ed9c57594bf3ead5edc82439f9fb7f514fd 100644
--- a/include/linux/gpio/property.h
+++ b/include/linux/gpio/property.h
@@ -7,7 +7,10 @@
 struct software_node;
 
 #define PROPERTY_ENTRY_GPIO(_name_, _chip_node_, _idx_, _flags_) \
-	PROPERTY_ENTRY_REF(_name_, _chip_node_, _idx_, _flags_)
+	PROPERTY_ENTRY_REF_SWNODE(_name_, _chip_node_, _idx_, _flags_)
+
+#define PROPERTY_ENTRY_GPIO_FWNODE(_name_, _chip_node_, _idx_, _flags_) \
+	PROPERTY_ENTRY_REF_FWNODE(_name_, _chip_node_, _idx_, _flags_)
 
 extern const struct software_node swnode_gpio_undefined;
 

-- 
2.48.1

[PATCH v3 07/10] reset: order includes alphabetically in reset/core.c

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

For better readability and easier maintenance order the includes
alphabetically.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/reset/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 22f67fc77ae531c6efba3ce92cc73a2d57397762..5a696e2dbcc224a633e2b321da53b7bc699cb5f3 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -4,19 +4,20 @@
  *
  * Copyright 2013 Philipp Zabel, Pengutronix
  */
+
+#include <linux/acpi.h>
 #include <linux/atomic.h>
 #include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/export.h>
-#include <linux/kernel.h>
-#include <linux/kref.h>
 #include <linux/gpio/driver.h>
 #include <linux/gpio/machine.h>
 #include <linux/idr.h>
+#include <linux/kernel.h>
+#include <linux/kref.h>
 #include <linux/module.h>
 #include <linux/of.h>
-#include <linux/acpi.h>
 #include <linux/platform_device.h>
 #include <linux/reset.h>
 #include <linux/reset-controller.h>

-- 
2.48.1

[PATCH v3 08/10] reset: make the provider of reset-gpios the parent of the reset device

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Auxiliary devices really do need a parent so ahead of converting the
reset-gpios driver to registering on the auxiliary bus, make the GPIO
device that provides the reset GPIO the parent of the reset-gpio device.
To that end move the lookup of the GPIO device by fwnode to the
beginning of __reset_add_reset_gpio_device() which has the added benefit
of bailing out earlier, before allocating resources for the virtual
device, if the chip is not up yet.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/reset/core.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 5a696e2dbcc224a633e2b321da53b7bc699cb5f3..13236ab69f10ec80e19b982be2bee5e4b0f99388 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -849,11 +849,11 @@ static void __reset_control_put_internal(struct reset_control *rstc)
 	kref_put(&rstc->refcnt, __reset_control_release);
 }
 
-static int __reset_add_reset_gpio_lookup(int id, struct device_node *np,
+static int __reset_add_reset_gpio_lookup(struct gpio_device *gdev, int id,
+					 struct device_node *np,
 					 unsigned int gpio,
 					 unsigned int of_flags)
 {
-	const struct fwnode_handle *fwnode = of_fwnode_handle(np);
 	unsigned int lookup_flags;
 	const char *label_tmp;
 
@@ -868,10 +868,6 @@ static int __reset_add_reset_gpio_lookup(int id, struct device_node *np,
 		return -EINVAL;
 	}
 
-	struct gpio_device *gdev __free(gpio_device_put) = gpio_device_find_by_fwnode(fwnode);
-	if (!gdev)
-		return -EPROBE_DEFER;
-
 	label_tmp = gpio_device_get_label(gdev);
 	if (!label_tmp)
 		return -EINVAL;
@@ -926,6 +922,11 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 	 */
 	lockdep_assert_not_held(&reset_list_mutex);
 
+	struct gpio_device *gdev __free(gpio_device_put) =
+		gpio_device_find_by_fwnode(of_fwnode_handle(args->np));
+	if (!gdev)
+		return -EPROBE_DEFER;
+
 	guard(mutex)(&reset_gpio_lookup_mutex);
 
 	list_for_each_entry(rgpio_dev, &reset_gpio_lookup_list, list) {
@@ -946,7 +947,7 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 		goto err_ida_free;
 	}
 
-	ret = __reset_add_reset_gpio_lookup(id, args->np, args->args[0],
+	ret = __reset_add_reset_gpio_lookup(gdev, id, args->np, args->args[0],
 					    args->args[1]);
 	if (ret < 0)
 		goto err_kfree;
@@ -958,7 +959,8 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 	 * Hold reference as long as rgpio_dev memory is valid.
 	 */
 	of_node_get(rgpio_dev->of_args.np);
-	pdev = platform_device_register_data(NULL, "reset-gpio", id,
+	pdev = platform_device_register_data(gpio_device_to_device(gdev),
+					     "reset-gpio", id,
 					     &rgpio_dev->of_args,
 					     sizeof(rgpio_dev->of_args));
 	ret = PTR_ERR_OR_ZERO(pdev);

-- 
2.48.1

[PATCH v3 09/10] reset: gpio: convert the driver to using the auxiliary bus

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

As the reset-gpio devices are purely virtual and never instantiated from
real firmware nodes, let's convert the driver to using the - more
fitting - auxiliary bus.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/reset/Kconfig      |  1 +
 drivers/reset/core.c       | 14 ++++++--------
 drivers/reset/reset-gpio.c | 19 ++++++++++---------
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 3102f5d7a93690f262722733e475b1215f61051c..24c9048cc7a31d3a6c9fb9af0726a8387bb3154a 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -89,6 +89,7 @@ config RESET_EYEQ
 config RESET_GPIO
 	tristate "GPIO reset controller"
 	depends on GPIOLIB
+	select AUXILIARY_BUS
 	help
 	  This enables a generic reset controller for resets attached via
 	  GPIOs.  Typically for OF platforms this driver expects "reset-gpios"
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 13236ab69f10ec80e19b982be2bee5e4b0f99388..e129c4c803eaa7e7e7122d96e9eff187f8dd826f 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -7,6 +7,7 @@
 
 #include <linux/acpi.h>
 #include <linux/atomic.h>
+#include <linux/auxiliary_bus.h>
 #include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -18,7 +19,6 @@
 #include <linux/kref.h>
 #include <linux/module.h>
 #include <linux/of.h>
-#include <linux/platform_device.h>
 #include <linux/reset.h>
 #include <linux/reset-controller.h>
 #include <linux/slab.h>
@@ -882,7 +882,7 @@ static int __reset_add_reset_gpio_lookup(struct gpio_device *gdev, int id,
 	if (!lookup)
 		return -ENOMEM;
 
-	lookup->dev_id = kasprintf(GFP_KERNEL, "reset-gpio.%d", id);
+	lookup->dev_id = kasprintf(GFP_KERNEL, "reset.gpio.%d", id);
 	if (!lookup->dev_id)
 		return -ENOMEM;
 
@@ -903,7 +903,7 @@ static int __reset_add_reset_gpio_lookup(struct gpio_device *gdev, int id,
 static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 {
 	struct reset_gpio_lookup *rgpio_dev;
-	struct platform_device *pdev;
+	struct auxiliary_device *adev;
 	int id, ret;
 
 	/*
@@ -959,11 +959,9 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 	 * Hold reference as long as rgpio_dev memory is valid.
 	 */
 	of_node_get(rgpio_dev->of_args.np);
-	pdev = platform_device_register_data(gpio_device_to_device(gdev),
-					     "reset-gpio", id,
-					     &rgpio_dev->of_args,
-					     sizeof(rgpio_dev->of_args));
-	ret = PTR_ERR_OR_ZERO(pdev);
+	adev = auxiliary_device_create(gpio_device_to_device(gdev), "reset",
+				       "gpio", &rgpio_dev->of_args, id);
+	ret = PTR_ERR_OR_ZERO(adev);
 	if (ret)
 		goto err_put;
 
diff --git a/drivers/reset/reset-gpio.c b/drivers/reset/reset-gpio.c
index 2290b25b6703536f2245f15cab870bd7092d3453..e5512b3b596b5290af20e5fdd99a38f81e670d2b 100644
--- a/drivers/reset/reset-gpio.c
+++ b/drivers/reset/reset-gpio.c
@@ -1,10 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0
 
+#include <linux/auxiliary_bus.h>
 #include <linux/gpio/consumer.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/of.h>
-#include <linux/platform_device.h>
 #include <linux/reset-controller.h>
 
 struct reset_gpio_priv {
@@ -61,9 +61,10 @@ static void reset_gpio_of_node_put(void *data)
 	of_node_put(data);
 }
 
-static int reset_gpio_probe(struct platform_device *pdev)
+static int reset_gpio_probe(struct auxiliary_device *adev,
+			    const struct auxiliary_device_id *id)
 {
-	struct device *dev = &pdev->dev;
+	struct device *dev = &adev->dev;
 	struct of_phandle_args *platdata = dev_get_platdata(dev);
 	struct reset_gpio_priv *priv;
 	int ret;
@@ -75,7 +76,7 @@ static int reset_gpio_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
-	platform_set_drvdata(pdev, &priv->rc);
+	auxiliary_set_drvdata(adev, &priv->rc);
 
 	priv->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(priv->reset))
@@ -99,20 +100,20 @@ static int reset_gpio_probe(struct platform_device *pdev)
 	return devm_reset_controller_register(dev, &priv->rc);
 }
 
-static const struct platform_device_id reset_gpio_ids[] = {
-	{ .name = "reset-gpio", },
+static const struct auxiliary_device_id reset_gpio_ids[] = {
+	{ .name = "reset.gpio" },
 	{}
 };
-MODULE_DEVICE_TABLE(platform, reset_gpio_ids);
+MODULE_DEVICE_TABLE(auxiliary, reset_gpio_ids);
 
-static struct platform_driver reset_gpio_driver = {
+static struct auxiliary_driver reset_gpio_driver = {
 	.probe		= reset_gpio_probe,
 	.id_table	= reset_gpio_ids,
 	.driver	= {
 		.name = "reset-gpio",
 	},
 };
-module_platform_driver(reset_gpio_driver);
+module_auxiliary_driver(reset_gpio_driver);
 
 MODULE_AUTHOR("Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>");
 MODULE_DESCRIPTION("Generic GPIO reset driver");

-- 
2.48.1

[PATCH v3 10/10] reset: gpio: use software nodes to setup the GPIO lookup

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

GPIO machine lookup is a nice mechanism for associating GPIOs with
consumers if we don't know what kind of device the GPIO provider is or
when it will become available. However in the case of the reset-gpio, we
are already holding a reference to the device and so can reference its
firmware node. Let's setup a software node that references the relevant
GPIO and attach it to the auxiliary device we're creating.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/reset/core.c | 130 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 76 insertions(+), 54 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index e129c4c803eaa7e7e7122d96e9eff187f8dd826f..4617bbac58314b1f37b937e7b0ffff745a81fcde 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -14,6 +14,7 @@
 #include <linux/export.h>
 #include <linux/gpio/driver.h>
 #include <linux/gpio/machine.h>
+#include <linux/gpio/property.h>
 #include <linux/idr.h>
 #include <linux/kernel.h>
 #include <linux/kref.h>
@@ -77,10 +78,12 @@ struct reset_control_array {
 /**
  * struct reset_gpio_lookup - lookup key for ad-hoc created reset-gpio devices
  * @of_args: phandle to the reset controller with all the args like GPIO number
+ * @swnode: Software node containing the reference to the GPIO provider
  * @list: list entry for the reset_gpio_lookup_list
  */
 struct reset_gpio_lookup {
 	struct of_phandle_args of_args;
+	struct fwnode_handle *swnode;
 	struct list_head list;
 };
 
@@ -849,52 +852,45 @@ static void __reset_control_put_internal(struct reset_control *rstc)
 	kref_put(&rstc->refcnt, __reset_control_release);
 }
 
-static int __reset_add_reset_gpio_lookup(struct gpio_device *gdev, int id,
-					 struct device_node *np,
-					 unsigned int gpio,
-					 unsigned int of_flags)
+static void reset_gpio_aux_device_release(struct device *dev)
 {
-	unsigned int lookup_flags;
-	const char *label_tmp;
+	struct auxiliary_device *adev = to_auxiliary_dev(dev);
 
-	/*
-	 * Later we map GPIO flags between OF and Linux, however not all
-	 * constants from include/dt-bindings/gpio/gpio.h and
-	 * include/linux/gpio/machine.h match each other.
-	 */
-	if (of_flags > GPIO_ACTIVE_LOW) {
-		pr_err("reset-gpio code does not support GPIO flags %u for GPIO %u\n",
-		       of_flags, gpio);
-		return -EINVAL;
+	kfree(adev);
+}
+
+static int reset_add_gpio_aux_device(struct device *parent,
+				     struct fwnode_handle *swnode,
+				     int id, void *pdata)
+{
+	struct auxiliary_device *adev;
+	int ret;
+
+	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+	if (!adev)
+		return -ENOMEM;
+
+	adev->id = id;
+	adev->name = "gpio";
+	adev->dev.parent = parent;
+	adev->dev.platform_data = pdata;
+	adev->dev.release = reset_gpio_aux_device_release;
+	device_set_node(&adev->dev, swnode);
+
+	ret = auxiliary_device_init(adev);
+	if (ret) {
+		kfree(adev);
+		return ret;
 	}
 
-	label_tmp = gpio_device_get_label(gdev);
-	if (!label_tmp)
-		return -EINVAL;
+	ret = __auxiliary_device_add(adev, "reset");
+	if (ret) {
+		auxiliary_device_uninit(adev);
+		kfree(adev);
+		return ret;
+	}
 
-	char *label __free(kfree) = kstrdup(label_tmp, GFP_KERNEL);
-	if (!label)
-		return -ENOMEM;
-
-	/* Size: one lookup entry plus sentinel */
-	struct gpiod_lookup_table *lookup __free(kfree) = kzalloc(struct_size(lookup, table, 2),
-								  GFP_KERNEL);
-	if (!lookup)
-		return -ENOMEM;
-
-	lookup->dev_id = kasprintf(GFP_KERNEL, "reset.gpio.%d", id);
-	if (!lookup->dev_id)
-		return -ENOMEM;
-
-	lookup_flags = GPIO_PERSISTENT;
-	lookup_flags |= of_flags & GPIO_ACTIVE_LOW;
-	lookup->table[0] = GPIO_LOOKUP(no_free_ptr(label), gpio, "reset",
-				       lookup_flags);
-
-	/* Not freed on success, because it is persisent subsystem data. */
-	gpiod_add_lookup_table(no_free_ptr(lookup));
-
-	return 0;
+	return ret;
 }
 
 /*
@@ -902,9 +898,11 @@ static int __reset_add_reset_gpio_lookup(struct gpio_device *gdev, int id,
  */
 static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 {
+	struct property_entry properties[] = { {}, {} };
 	struct reset_gpio_lookup *rgpio_dev;
-	struct auxiliary_device *adev;
-	int id, ret;
+	unsigned int offset, of_flags;
+	struct device *parent;
+	int id, ret, lflags;
 
 	/*
 	 * Currently only #gpio-cells=2 is supported with the meaning of:
@@ -922,6 +920,23 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 	 */
 	lockdep_assert_not_held(&reset_list_mutex);
 
+	offset = args->args[0];
+	of_flags = args->args[1];
+
+	/*
+	 * Later we map GPIO flags between OF and Linux, however not all
+	 * constants from include/dt-bindings/gpio/gpio.h and
+	 * include/linux/gpio/machine.h match each other.
+	 *
+	 * FIXME: Find a better way of translating OF flags to GPIO lookup
+	 * flags.
+	 */
+	if (of_flags > GPIO_ACTIVE_LOW) {
+		pr_err("reset-gpio code does not support GPIO flags %u for GPIO %u\n",
+		       of_flags, offset);
+		return -EINVAL;
+	}
+
 	struct gpio_device *gdev __free(gpio_device_put) =
 		gpio_device_find_by_fwnode(of_fwnode_handle(args->np));
 	if (!gdev)
@@ -936,6 +951,13 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 		}
 	}
 
+	lflags = GPIO_PERSISTENT | (of_flags & GPIO_ACTIVE_LOW);
+	parent = gpio_device_to_device(gdev);
+
+	properties[0] = PROPERTY_ENTRY_GPIO_FWNODE("reset-gpios",
+						   parent->fwnode,
+						   offset, lflags);
+
 	id = ida_alloc(&reset_gpio_ida, GFP_KERNEL);
 	if (id < 0)
 		return id;
@@ -947,11 +969,6 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 		goto err_ida_free;
 	}
 
-	ret = __reset_add_reset_gpio_lookup(gdev, id, args->np, args->args[0],
-					    args->args[1]);
-	if (ret < 0)
-		goto err_kfree;
-
 	rgpio_dev->of_args = *args;
 	/*
 	 * We keep the device_node reference, but of_args.np is put at the end
@@ -959,19 +976,24 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 	 * Hold reference as long as rgpio_dev memory is valid.
 	 */
 	of_node_get(rgpio_dev->of_args.np);
-	adev = auxiliary_device_create(gpio_device_to_device(gdev), "reset",
-				       "gpio", &rgpio_dev->of_args, id);
-	ret = PTR_ERR_OR_ZERO(adev);
+
+	rgpio_dev->swnode = fwnode_create_software_node(properties, NULL);
+	if (IS_ERR(rgpio_dev->swnode))
+		goto err_put_of_node;
+
+	ret = reset_add_gpio_aux_device(parent, rgpio_dev->swnode, id,
+					&rgpio_dev->of_args);
 	if (ret)
-		goto err_put;
+		goto err_del_swnode;
 
 	list_add(&rgpio_dev->list, &reset_gpio_lookup_list);
 
 	return 0;
 
-err_put:
+err_del_swnode:
+	fwnode_remove_software_node(rgpio_dev->swnode);
+err_put_of_node:
 	of_node_put(rgpio_dev->of_args.np);
-err_kfree:
 	kfree(rgpio_dev);
 err_ida_free:
 	ida_free(&reset_gpio_ida, id);

-- 
2.48.1

Re: [PATCH v3 03/10] software node: allow referencing firmware nodes

[Philipp Zabel]

On Mi, 2025-10-29 at 13:28 +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> At the moment software nodes can only reference other software nodes.
> This is a limitation for devices created, for instance, on the auxiliary
> bus with a dynamic software node attached which cannot reference devices
> the firmware node of which is "real" (as an OF node or otherwise).
> 
> Make it possible for a software node to reference all firmware nodes in
> addition to static software nodes. To that end: add a second pointer to
> struct software_node_ref_args of type struct fwnode_handle. The core
> swnode code will first check the swnode pointer and if it's NULL, it
> will assume the fwnode pointer should be set. Rework the helper macros
> and deprecate the existing ones whose names don't indicate the reference
> type.
> 
> Software node graphs remain the same, as in: the remote endpoints still
> have to be software nodes.
> 
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/base/swnode.c    | 13 +++++++++++--
>  include/linux/property.h | 38 +++++++++++++++++++++++++++++++-------
>  2 files changed, 42 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index b7c3926b67be72671ba4e4c442b3acca80688cf7..8601d1612be31febb6abbbe1fb35228499480c56 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -535,7 +535,13 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>  	ref_array = prop->pointer;
>  	ref = &ref_array[index];
>  
> -	refnode = software_node_fwnode(ref->node);
> +	if (ref->swnode)
> +		refnode = software_node_fwnode(ref->swnode);

software_node_fwnode(ref->swnode) never returns NULL if given a non-
NULL parameter.

> +	else if (ref->fwnode)
> +		refnode = ref->fwnode;
> +	else
> +		return -EINVAL;
> +
>  	if (!refnode)
>  		return -ENOENT;

So this check is not needed anymore.

regards
Philipp

Re: [PATCH v3 03/10] software node: allow referencing firmware nodes

[Bartosz Golaszewski]

On Wed, Oct 29, 2025 at 1:51 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> > @@ -535,7 +535,13 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
> >       ref_array = prop->pointer;
> >       ref = &ref_array[index];
> >
> > -     refnode = software_node_fwnode(ref->node);
> > +     if (ref->swnode)
> > +             refnode = software_node_fwnode(ref->swnode);
>
> software_node_fwnode(ref->swnode) never returns NULL if given a non-
> NULL parameter.
>

That's not true, it *will* return NULL if the software node in
question has not yet been registered with the fwnode framework.

Bart

Re: [PATCH v3 03/10] software node: allow referencing firmware nodes

[Philipp Zabel]

On Mi, 2025-10-29 at 13:55 +0100, Bartosz Golaszewski wrote:
> On Wed, Oct 29, 2025 at 1:51 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > 
> > > @@ -535,7 +535,13 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
> > >       ref_array = prop->pointer;
> > >       ref = &ref_array[index];
> > > 
> > > -     refnode = software_node_fwnode(ref->node);
> > > +     if (ref->swnode)
> > > +             refnode = software_node_fwnode(ref->swnode);
> > 
> > software_node_fwnode(ref->swnode) never returns NULL if given a non-
> > NULL parameter.
> > 
> 
> That's not true, it *will* return NULL if the software node in
> question has not yet been registered with the fwnode framework.

I completely missed the list lookup in software_node_to_swnode().
Thank you for the quick correction and additional context.

regards
Philipp

Re: [PATCH v3 00/10] reset: rework reset-gpios handling

[Philipp Zabel]

On Mi, 2025-10-29 at 13:28 +0100, Bartosz Golaszewski wrote:
> Machine GPIO lookup is a nice, if a bit clunky, mechanism when we have
> absolutely no idea what the GPIO provider is or when it will be created.
> However in the case of reset-gpios, we not only know if the chip is
> there - we also already hold a reference to its firmware node.
> 
> In this case using fwnode lookup makes more sense. However, since the
> reset provider is created dynamically, it doesn't have a corresponding
> firmware node (in this case: an OF-node). That leaves us with software
> nodes which currently cannot reference other implementations of the
> fwnode API, only other struct software_node objects. This is a needless
> limitation as it's imaginable that a dynamic auxiliary device (with a
> software node attached) would want to reference a real device with an OF
> node.
> 
> This series does three things: extends the software node implementation,
> allowing its properties to reference not only static software nodes but
> also existing firmware nodes, updates the GPIO property interface to use
> the reworked swnode macros and finally makes the reset-gpio code the
> first user by converting the GPIO lookup from machine to swnode.
> 
> Another user of the software node changes in the future could become the
> shared GPIO modules that's in the works in parallel[1].
> 
> Merging strategy: the series is logically split into three parts: driver
> core, GPIO and reset respectively. However there are build-time
> dependencies between all three parts so I suggest the reset tree as the
> right one to take it upstream with an immutable branch provided to
> driver core and GPIO.

Should that branch include the reset changes, or only up to patch 6?

regards
Philipp

Re: [PATCH v3 00/10] reset: rework reset-gpios handling

[Bartosz Golaszewski]

On Wed, Oct 29, 2025 at 2:16 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Mi, 2025-10-29 at 13:28 +0100, Bartosz Golaszewski wrote:
> > Machine GPIO lookup is a nice, if a bit clunky, mechanism when we have
> > absolutely no idea what the GPIO provider is or when it will be created.
> > However in the case of reset-gpios, we not only know if the chip is
> > there - we also already hold a reference to its firmware node.
> >
> > In this case using fwnode lookup makes more sense. However, since the
> > reset provider is created dynamically, it doesn't have a corresponding
> > firmware node (in this case: an OF-node). That leaves us with software
> > nodes which currently cannot reference other implementations of the
> > fwnode API, only other struct software_node objects. This is a needless
> > limitation as it's imaginable that a dynamic auxiliary device (with a
> > software node attached) would want to reference a real device with an OF
> > node.
> >
> > This series does three things: extends the software node implementation,
> > allowing its properties to reference not only static software nodes but
> > also existing firmware nodes, updates the GPIO property interface to use
> > the reworked swnode macros and finally makes the reset-gpio code the
> > first user by converting the GPIO lookup from machine to swnode.
> >
> > Another user of the software node changes in the future could become the
> > shared GPIO modules that's in the works in parallel[1].
> >
> > Merging strategy: the series is logically split into three parts: driver
> > core, GPIO and reset respectively. However there are build-time
> > dependencies between all three parts so I suggest the reset tree as the
> > right one to take it upstream with an immutable branch provided to
> > driver core and GPIO.
>
> Should that branch include the reset changes, or only up to patch 6?
>

I was thinking about it containing the entire series, somewhat similar
to what Lee Jones does with MFD changes.

Bartosz

Re: [PATCH v3 01/10] software node: read the reference args via the fwnode API

[Andy Shevchenko]

On Wed, Oct 29, 2025 at 01:28:35PM +0100, Bartosz Golaszewski wrote:
> 
> 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.

As pointed out in previous reviews this would have benefit of a short comment.
explaining the indirect call to read an array.

...

>  	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(refnode, nargs_prop,
> +						 &nargs_prop_val);

It can be one line now. We are not fanatics of 80 limitation. :-)

>  		if (error)
>  			return error;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 02/10] software node: increase the reference of the swnode by its fwnode

[Andy Shevchenko]

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).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 03/10] software node: allow referencing firmware nodes

[Andy Shevchenko]

On Wed, Oct 29, 2025 at 01:28:37PM +0100, Bartosz Golaszewski wrote:
> 
> At the moment software nodes can only reference other software nodes.
> This is a limitation for devices created, for instance, on the auxiliary
> bus with a dynamic software node attached which cannot reference devices
> the firmware node of which is "real" (as an OF node or otherwise).
> 
> Make it possible for a software node to reference all firmware nodes in
> addition to static software nodes. To that end: add a second pointer to
> struct software_node_ref_args of type struct fwnode_handle. The core
> swnode code will first check the swnode pointer and if it's NULL, it
> will assume the fwnode pointer should be set. Rework the helper macros
> and deprecate the existing ones whose names don't indicate the reference
> type.
> 
> Software node graphs remain the same, as in: the remote endpoints still
> have to be software nodes.

...

> -#define SOFTWARE_NODE_REFERENCE(_ref_, ...)			\
> +#define __SOFTWARE_NODE_REF(_ref, _node, ...)			\
>  (const struct software_node_ref_args) {				\
> -	.node = _ref_,						\
> +	._node = _ref,						\
>  	.nargs = COUNT_ARGS(__VA_ARGS__),			\
>  	.args = { __VA_ARGS__ },				\
>  }

Okay, looking at this again I think we don't need a new parameter.
We may check the type of _ref_
(actually why are the macro parameters got renamed here and elsewhere?)
and assign the correct one accordingly. I think this is what _Generic()
is good for.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 02/10] software node: increase the reference of the swnode by its fwnode

[Bartosz Golaszewski]

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

Re: [PATCH v3 03/10] software node: allow referencing firmware nodes

[Bartosz Golaszewski]

On Thu, 30 Oct 2025 10:41:39 +0100, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> said:
> On Wed, Oct 29, 2025 at 01:28:37PM +0100, Bartosz Golaszewski wrote:
>>
>> At the moment software nodes can only reference other software nodes.
>> This is a limitation for devices created, for instance, on the auxiliary
>> bus with a dynamic software node attached which cannot reference devices
>> the firmware node of which is "real" (as an OF node or otherwise).
>>
>> Make it possible for a software node to reference all firmware nodes in
>> addition to static software nodes. To that end: add a second pointer to
>> struct software_node_ref_args of type struct fwnode_handle. The core
>> swnode code will first check the swnode pointer and if it's NULL, it
>> will assume the fwnode pointer should be set. Rework the helper macros
>> and deprecate the existing ones whose names don't indicate the reference
>> type.
>>
>> Software node graphs remain the same, as in: the remote endpoints still
>> have to be software nodes.
>
> ...
>
>> -#define SOFTWARE_NODE_REFERENCE(_ref_, ...)			\
>> +#define __SOFTWARE_NODE_REF(_ref, _node, ...)			\
>>  (const struct software_node_ref_args) {				\
>> -	.node = _ref_,						\
>> +	._node = _ref,						\
>>  	.nargs = COUNT_ARGS(__VA_ARGS__),			\
>>  	.args = { __VA_ARGS__ },				\
>>  }
>
> Okay, looking at this again I think we don't need a new parameter.
> We may check the type of _ref_
> (actually why are the macro parameters got renamed here and elsewhere?)
> and assign the correct one accordingly. I think this is what _Generic()
> is good for.
>

Oh, that's neat, I would love to use _Generic() here but I honest to god have
no idea how to make it work. I tried something like:

#define __SOFTWARE_NODE_REF(_ref, ...)                          \
_Generic(_ref,                                                  \
        const struct software_node *:                           \
                (const struct software_node_ref_args) {         \
                        .swnode = _ref,                         \
                        .nargs = COUNT_ARGS(__VA_ARGS__),       \
                        .args = { __VA_ARGS__ },                \
                },                                              \
        struct fwnode_handle *:                                 \
                (const struct software_node_ref_args) {         \
                        .fwnode = _ref,                         \
                        .nargs = COUNT_ARGS(__VA_ARGS__),       \
                        .args = { __VA_ARGS__ },                \
                }                                               \
        )


But this fails like this:

In file included from ./include/linux/acpi.h:16,
                 from drivers/reset/core.c:8:
drivers/reset/core.c: In function ‘__reset_add_reset_gpio_device’:
drivers/reset/core.c:958:52: error: initialization of ‘const struct
software_node *’ from incompatible pointer type ‘struct fwnode_handle
*’ [-Wincompatible-pointer-types]
  958 |                                                    parent->fwnode,
      |                                                    ^~~~~~
./include/linux/property.h:374:35: note: in definition of macro
‘__SOFTWARE_NODE_REF’
  374 |                         .swnode = _ref,                         \

So the right branch is not selected. How exactly would you use it here?

Bart

Re: [PATCH v3 03/10] software node: allow referencing firmware nodes

[Andy Shevchenko]

On Thu, Oct 30, 2025 at 04:17:48AM -0700, Bartosz Golaszewski wrote:
> On Thu, 30 Oct 2025 10:41:39 +0100, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> said:
> > On Wed, Oct 29, 2025 at 01:28:37PM +0100, Bartosz Golaszewski wrote:
> >>
> >> At the moment software nodes can only reference other software nodes.
> >> This is a limitation for devices created, for instance, on the auxiliary
> >> bus with a dynamic software node attached which cannot reference devices
> >> the firmware node of which is "real" (as an OF node or otherwise).
> >>
> >> Make it possible for a software node to reference all firmware nodes in
> >> addition to static software nodes. To that end: add a second pointer to
> >> struct software_node_ref_args of type struct fwnode_handle. The core
> >> swnode code will first check the swnode pointer and if it's NULL, it
> >> will assume the fwnode pointer should be set. Rework the helper macros
> >> and deprecate the existing ones whose names don't indicate the reference
> >> type.
> >>
> >> Software node graphs remain the same, as in: the remote endpoints still
> >> have to be software nodes.
> >
> > ...
> >
> >> -#define SOFTWARE_NODE_REFERENCE(_ref_, ...)			\
> >> +#define __SOFTWARE_NODE_REF(_ref, _node, ...)			\
> >>  (const struct software_node_ref_args) {				\
> >> -	.node = _ref_,						\
> >> +	._node = _ref,						\
> >>  	.nargs = COUNT_ARGS(__VA_ARGS__),			\
> >>  	.args = { __VA_ARGS__ },				\
> >>  }
> >
> > Okay, looking at this again I think we don't need a new parameter.
> > We may check the type of _ref_
> > (actually why are the macro parameters got renamed here and elsewhere?)
> > and assign the correct one accordingly. I think this is what _Generic()
> > is good for.
> >
> 
> Oh, that's neat, I would love to use _Generic() here but I honest to god have
> no idea how to make it work. I tried something like:
> 
> #define __SOFTWARE_NODE_REF(_ref, ...)                          \
> _Generic(_ref,                                                  \
>         const struct software_node *:                           \
>                 (const struct software_node_ref_args) {         \
>                         .swnode = _ref,                         \
>                         .nargs = COUNT_ARGS(__VA_ARGS__),       \
>                         .args = { __VA_ARGS__ },                \
>                 },                                              \
>         struct fwnode_handle *:                                 \
>                 (const struct software_node_ref_args) {         \
>                         .fwnode = _ref,                         \
>                         .nargs = COUNT_ARGS(__VA_ARGS__),       \
>                         .args = { __VA_ARGS__ },                \
>                 }                                               \
>         )
> 
> 
> But this fails like this:
> 
> In file included from ./include/linux/acpi.h:16,
>                  from drivers/reset/core.c:8:
> drivers/reset/core.c: In function ‘__reset_add_reset_gpio_device’:
> drivers/reset/core.c:958:52: error: initialization of ‘const struct
> software_node *’ from incompatible pointer type ‘struct fwnode_handle
> *’ [-Wincompatible-pointer-types]
>   958 |                                                    parent->fwnode,
>       |                                                    ^~~~~~
> ./include/linux/property.h:374:35: note: in definition of macro
> ‘__SOFTWARE_NODE_REF’
>   374 |                         .swnode = _ref,                         \
> 
> So the right branch is not selected. How exactly would you use it here?

I believe this is an easy task.

But first of all, your series doesn't compile AFAICS:

drivers/reset/core.c:981:6: error: variable 'ret' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
  981 |         if (IS_ERR(rgpio_dev->swnode))
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/reset/core.c:1001:9: note: uninitialized use occurs here
       1001 |         return ret;
            |                ^~~
drivers/reset/core.c:981:2: note: remove the 'if' if its condition is always false
  981 |         if (IS_ERR(rgpio_dev->swnode))
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  982 |                 goto err_put_of_node;
      |                 ~~~~~~~~~~~~~~~~~~~~
drivers/reset/core.c:905:13: note: initialize the variable 'ret' to silence this warning
  905 |         int id, ret, lflags;
      |                    ^
      |                     = 0
1 error generated.

So, but to the topic

I have applied this and get the only error as per above

 (const struct software_node_ref_args) {                                \
 -       ._node = _ref,                                          \
 +       .swnode = _Generic(_ref, const struct software_node *: _ref, default: NULL), \
 +       .fwnode = _Generic(_ref, struct fwnode_handle *: _ref, default: NULL), \


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 02/10] software node: increase the reference of the swnode by its fwnode

[Andy Shevchenko]

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

Re: [PATCH v3 03/10] software node: allow referencing firmware nodes

[Bartosz Golaszewski]

On Fri, Oct 31, 2025 at 9:24 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Oct 30, 2025 at 04:17:48AM -0700, Bartosz Golaszewski wrote:
> > On Thu, 30 Oct 2025 10:41:39 +0100, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> said:
> > > On Wed, Oct 29, 2025 at 01:28:37PM +0100, Bartosz Golaszewski wrote:
> > >>
> > >> At the moment software nodes can only reference other software nodes.
> > >> This is a limitation for devices created, for instance, on the auxiliary
> > >> bus with a dynamic software node attached which cannot reference devices
> > >> the firmware node of which is "real" (as an OF node or otherwise).
> > >>
> > >> Make it possible for a software node to reference all firmware nodes in
> > >> addition to static software nodes. To that end: add a second pointer to
> > >> struct software_node_ref_args of type struct fwnode_handle. The core
> > >> swnode code will first check the swnode pointer and if it's NULL, it
> > >> will assume the fwnode pointer should be set. Rework the helper macros
> > >> and deprecate the existing ones whose names don't indicate the reference
> > >> type.
> > >>
> > >> Software node graphs remain the same, as in: the remote endpoints still
> > >> have to be software nodes.
> > >
> > > ...
> > >
> > >> -#define SOFTWARE_NODE_REFERENCE(_ref_, ...)                       \
> > >> +#define __SOFTWARE_NODE_REF(_ref, _node, ...)                     \
> > >>  (const struct software_node_ref_args) {                           \
> > >> -  .node = _ref_,                                          \
> > >> +  ._node = _ref,                                          \
> > >>    .nargs = COUNT_ARGS(__VA_ARGS__),                       \
> > >>    .args = { __VA_ARGS__ },                                \
> > >>  }
> > >
> > > Okay, looking at this again I think we don't need a new parameter.
> > > We may check the type of _ref_
> > > (actually why are the macro parameters got renamed here and elsewhere?)
> > > and assign the correct one accordingly. I think this is what _Generic()
> > > is good for.
> > >
> >
> > Oh, that's neat, I would love to use _Generic() here but I honest to god have
> > no idea how to make it work. I tried something like:
> >
> > #define __SOFTWARE_NODE_REF(_ref, ...)                          \
> > _Generic(_ref,                                                  \
> >         const struct software_node *:                           \
> >                 (const struct software_node_ref_args) {         \
> >                         .swnode = _ref,                         \
> >                         .nargs = COUNT_ARGS(__VA_ARGS__),       \
> >                         .args = { __VA_ARGS__ },                \
> >                 },                                              \
> >         struct fwnode_handle *:                                 \
> >                 (const struct software_node_ref_args) {         \
> >                         .fwnode = _ref,                         \
> >                         .nargs = COUNT_ARGS(__VA_ARGS__),       \
> >                         .args = { __VA_ARGS__ },                \
> >                 }                                               \
> >         )
> >
> >
> > But this fails like this:
> >
> > In file included from ./include/linux/acpi.h:16,
> >                  from drivers/reset/core.c:8:
> > drivers/reset/core.c: In function ‘__reset_add_reset_gpio_device’:
> > drivers/reset/core.c:958:52: error: initialization of ‘const struct
> > software_node *’ from incompatible pointer type ‘struct fwnode_handle
> > *’ [-Wincompatible-pointer-types]
> >   958 |                                                    parent->fwnode,
> >       |                                                    ^~~~~~
> > ./include/linux/property.h:374:35: note: in definition of macro
> > ‘__SOFTWARE_NODE_REF’
> >   374 |                         .swnode = _ref,                         \
> >
> > So the right branch is not selected. How exactly would you use it here?
>
> I believe this is an easy task.
>
> But first of all, your series doesn't compile AFAICS:
>
> drivers/reset/core.c:981:6: error: variable 'ret' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
>   981 |         if (IS_ERR(rgpio_dev->swnode))
>       |             ^~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/reset/core.c:1001:9: note: uninitialized use occurs here
>        1001 |         return ret;
>             |                ^~~
> drivers/reset/core.c:981:2: note: remove the 'if' if its condition is always false
>   981 |         if (IS_ERR(rgpio_dev->swnode))
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   982 |                 goto err_put_of_node;
>       |                 ~~~~~~~~~~~~~~~~~~~~
> drivers/reset/core.c:905:13: note: initialize the variable 'ret' to silence this warning
>   905 |         int id, ret, lflags;
>       |                    ^
>       |                     = 0
> 1 error generated.
>

You're not wrong but for the record: it builds fine for me with
aarch64-linux-gnu-gcc 14.2 for some reason so I didn't notice it. I'll
fix it.

> So, but to the topic
>
> I have applied this and get the only error as per above
>
>  (const struct software_node_ref_args) {                                \
>  -       ._node = _ref,                                          \
>  +       .swnode = _Generic(_ref, const struct software_node *: _ref, default: NULL), \
>  +       .fwnode = _Generic(_ref, struct fwnode_handle *: _ref, default: NULL), \
>

That works, thanks for the idea.

Bartosz

Re: [PATCH v3 02/10] software node: increase the reference of the swnode by its fwnode

[Bartosz Golaszewski]

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

Re: [PATCH v3 02/10] software node: increase the reference of the swnode by its fwnode

[Andy Shevchenko]

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

Re: [PATCH v3 03/10] software node: allow referencing firmware nodes

[Andy Shevchenko]

On Fri, Oct 31, 2025 at 10:00:37AM +0100, Bartosz Golaszewski wrote:
> On Fri, Oct 31, 2025 at 9:24 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Oct 30, 2025 at 04:17:48AM -0700, Bartosz Golaszewski wrote:

...

> > But first of all, your series doesn't compile AFAICS:
> >
> > drivers/reset/core.c:981:6: error: variable 'ret' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]
> >   981 |         if (IS_ERR(rgpio_dev->swnode))
> >       |             ^~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/reset/core.c:1001:9: note: uninitialized use occurs here
> >        1001 |         return ret;
> >             |                ^~~
> > drivers/reset/core.c:981:2: note: remove the 'if' if its condition is always false
> >   981 |         if (IS_ERR(rgpio_dev->swnode))
> >       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   982 |                 goto err_put_of_node;
> >       |                 ~~~~~~~~~~~~~~~~~~~~
> > drivers/reset/core.c:905:13: note: initialize the variable 'ret' to silence this warning
> >   905 |         int id, ret, lflags;
> >       |                    ^
> >       |                     = 0
> > 1 error generated.
> 
> You're not wrong but for the record: it builds fine for me with
> aarch64-linux-gnu-gcc 14.2 for some reason so I didn't notice it. I'll
> fix it.

GCC is not _the_ compiler nowadays. And building with `make W=1` should be a good
practice for subsystem maintainers :-)

...

> > So, but to the topic
> >
> > I have applied this and get the only error as per above
> >
> >  (const struct software_node_ref_args) {                                \
> >  -       ._node = _ref,                                          \
> >  +       .swnode = _Generic(_ref, const struct software_node *: _ref, default: NULL), \
> >  +       .fwnode = _Generic(_ref, struct fwnode_handle *: _ref, default: NULL), \
> >
> 
> That works, thanks for the idea.

You're welcome!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 02/10] software node: increase the reference of the swnode by its fwnode

[Bartosz Golaszewski]

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

Re: [PATCH v3 02/10] software node: increase the reference of the swnode by its fwnode

[Andy Shevchenko]

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