[PATCH 0/9] reset: rework reset-gpios handling

[PATCH 0/9] 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>
---
Bartosz Golaszewski (9):
      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: 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         |  28 +++++---
 drivers/gpio/gpiolib-swnode.c |  16 ++---
 drivers/reset/Kconfig         |   1 +
 drivers/reset/core.c          | 151 ++++++++++++++++++++++++------------------
 drivers/reset/reset-gpio.c    |  19 +++---
 include/linux/gpio/property.h |   5 +-
 include/linux/property.h      |  51 ++++++++++++--
 7 files changed, 174 insertions(+), 97 deletions(-)
---
base-commit: 097d5ce7a680da489516958e943910fa962e574a
change-id: 20250925-reset-gpios-swnodes-db553e67095b

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

[PATCH 1/9] 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.

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

-- 
2.48.1

[PATCH 2/9] 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.

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 cc48cff54d9c3d4d257095b6cb4a7869bf657373..a60ba4327db8b967034b296b73c948aa5746a094 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 3/9] 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: use a union of different
pointers in struct software_node_ref_args and add an enum indicating
what kind of reference given instance of it is. 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.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/base/swnode.c    | 21 ++++++++++++++++----
 include/linux/property.h | 51 +++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 61 insertions(+), 11 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index a60ba4327db8b967034b296b73c948aa5746a094..b7e91c60c1d51b167adc88afb0f06feeeccf900c 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -535,9 +535,19 @@ 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 (!refnode)
-		return -ENOENT;
+	switch (ref->type) {
+	case SOFTWARE_NODE_REF_SWNODE:
+		refnode = software_node_fwnode(ref->swnode);
+		if (!refnode)
+			return -ENOENT;
+		break;
+	case SOFTWARE_NODE_REF_FWNODE:
+		refnode = ref->fwnode;
+		break;
+	default:
+		WARN_ON(1);
+		return -EINVAL;
+	}
 
 	if (nargs_prop) {
 		error = fwnode_property_read_u32_array(refnode, nargs_prop,
@@ -634,7 +644,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->type != SOFTWARE_NODE_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..af72f579e8026cee2456b0983819e7a4bf0c6805 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -353,25 +353,50 @@ fwnode_property_string_array_count(const struct fwnode_handle *fwnode,
 
 struct software_node;
 
+enum software_node_ref_type {
+	/* References a software node. */
+	SOFTWARE_NODE_REF_SWNODE = 0,
+	/* References a firmware node. */
+	SOFTWARE_NODE_REF_FWNODE,
+};
+
 /**
  * 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;
+	enum software_node_ref_type type;
+	union {
+		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, _type, _node, ...)		\
 (const struct software_node_ref_args) {				\
-	.node = _ref_,						\
+	.type = _type,						\
+	._node = _ref,						\
 	.nargs = COUNT_ARGS(__VA_ARGS__),			\
 	.args = { __VA_ARGS__ },				\
 }
 
+#define SOFTWARE_NODE_REF_SWNODE(_ref, ...)			\
+	__SOFTWARE_NODE_REF(_ref, SOFTWARE_NODE_REF_SWNODE,	\
+			    swnode, __VA_ARGS__)
+
+#define SOFTWARE_NODE_REF_FWNODE(_ref, ...)			\
+	__SOFTWARE_NODE_REF(_ref, SOFTWARE_NODE_REF_FWNODE,	\
+			    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 +488,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 4/9] 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.

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

diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-swnode.c
index f21dbc28cf2c8c2d06d034b7c89d302cc52bb9b5..573b5216cfda105bafa58e04fc5ad3a38d283698 100644
--- a/drivers/gpio/gpiolib-swnode.c
+++ b/drivers/gpio/gpiolib-swnode.c
@@ -7,6 +7,7 @@
 
 #define pr_fmt(fmt) "gpiolib: swnode: " fmt
 
+#include <linux/cleanup.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/export.h>
@@ -26,23 +27,20 @@
 
 static struct gpio_device *swnode_get_gpio_device(struct fwnode_handle *fwnode)
 {
-	const struct software_node *gdev_node;
-	struct gpio_device *gdev;
-
-	gdev_node = to_software_node(fwnode);
-	if (!gdev_node || !gdev_node->name)
-		return ERR_PTR(-EINVAL);
+	struct gpio_device *gdev __free(gpio_device_put) =
+					gpio_device_find_by_fwnode(fwnode);
+	if (!gdev)
+		return ERR_PTR(-EPROBE_DEFER);
 
 	/*
 	 * 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))
+	    !strcmp(fwnode_get_name(fwnode), GPIOLIB_SWNODE_UNDEFINED_NAME))
 		return ERR_PTR(-ENOENT);
 
-	gdev = gpio_device_find_by_label(gdev_node->name);
-	return gdev ?: ERR_PTR(-EPROBE_DEFER);
+	return no_free_ptr(gdev);
 }
 
 static int swnode_gpio_get_reference(const struct fwnode_handle *fwnode,

-- 
2.48.1

[PATCH 5/9] 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.

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 6/9] 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.

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 7/9] 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 benefor
of bailing out earlier, before allocating resources for the virtual
device, if the chip is not up yet.

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..ad85ddc8dd9fcf8b512cb09168586e0afca257f1 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;
@@ -919,6 +915,11 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 	if (args->args_count != 2)
 		return -ENOENT;
 
+	struct gpio_device *gdev __free(gpio_device_put) =
+		gpio_device_find_by_fwnode(of_fwnode_handle(args->np));
+	if (!gdev)
+		return -EPROBE_DEFER;
+
 	/*
 	 * Registering reset-gpio device might cause immediate
 	 * bind, resulting in its probe() registering new reset controller thus
@@ -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 8/9] 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.

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 ad85ddc8dd9fcf8b512cb09168586e0afca257f1..c9f13020ca3a7b9273488497a7d4240d0af762b0 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..7b43d61d0467aef5fbbad53d531294fa62f8084a 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 9/9] 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.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/reset/core.c | 132 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 78 insertions(+), 54 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index c9f13020ca3a7b9273488497a7d4240d0af762b0..b3e6ba7a9c3d756d2e30dc20edda9c02b624aefd 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_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_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;
 }
 
 /*
@@ -903,8 +899,10 @@ 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 auxiliary_device *adev;
-	int id, ret;
+	struct property_entry properties[2];
+	unsigned int offset, of_flags;
+	struct device *parent;
+	int id, ret, lflags;
 
 	/*
 	 * Currently only #gpio-cells=2 is supported with the meaning of:
@@ -915,11 +913,30 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 	if (args->args_count != 2)
 		return -ENOENT;
 
+	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)
 		return -EPROBE_DEFER;
 
+	parent = gpio_device_to_device(gdev);
+
 	/*
 	 * Registering reset-gpio device might cause immediate
 	 * bind, resulting in its probe() registering new reset controller thus
@@ -936,6 +953,13 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
 		}
 	}
 
+	lflags = GPIO_PERSISTENT | (of_flags & GPIO_ACTIVE_LOW);
+
+	memset(properties, 0, sizeof(properties));
+	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 +971,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 +978,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 7/9] reset: make the provider of reset-gpios the parent of the reset device

[Philipp Zabel]

Hi Bartosz,

On Mo, 2025-10-06 at 15:00 +0200, Bartosz Golaszewski wrote:
> 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 benefor

Typo: benefit.

> of bailing out earlier, before allocating resources for the virtual
> device, if the chip is not up yet.
> 
> 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..ad85ddc8dd9fcf8b512cb09168586e0afca257f1 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);

This is the only remaining use of gdev in
__reset_add_reset_gpio_lookup().
It would make sense to move this as well and only pass the label.

Given that all this is removed in patch 9, this is not super important.

>  	if (!label_tmp)
>  		return -EINVAL;
> @@ -919,6 +915,11 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
>  	if (args->args_count != 2)
>  		return -ENOENT;
>  
> +	struct gpio_device *gdev __free(gpio_device_put) =
> +		gpio_device_find_by_fwnode(of_fwnode_handle(args->np));

We are mixing cleanup helpers with gotos in this function, which the
documentation in cleanup.h explicitly advises against.

I know the current code is already guilty, but could you take this
opportunity to prepend a patch that splits the part under guard() into
a separate function?

I'd also move this block after the lockdep_assert_not_held() below.


regards
Philipp

Re: [PATCH 6/9] reset: order includes alphabetically in reset/core.c

[Philipp Zabel]

On Mo, 2025-10-06 at 15:00 +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> For better readability and easier maintenance order the includes
> alphabetically.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH 8/9] reset: gpio: convert the driver to using the auxiliary bus

[Philipp Zabel]

On Mo, 2025-10-06 at 15:00 +0200, Bartosz Golaszewski wrote:
> 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.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Thank you,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH 9/9] reset: gpio: use software nodes to setup the GPIO lookup

[Philipp Zabel]

On Mo, 2025-10-06 at 15:00 +0200, Bartosz Golaszewski wrote:
> 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.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/reset/core.c | 132 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 78 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index c9f13020ca3a7b9273488497a7d4240d0af762b0..b3e6ba7a9c3d756d2e30dc20edda9c02b624aefd 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
[...]
> @@ -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_aux_device_release(struct device *dev)

static void reset_gpio_aux_device_release(struct device *dev)

[...]
> @@ -903,8 +899,10 @@ 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 auxiliary_device *adev;
> -	int id, ret;
> +	struct property_entry properties[2];

It would be nice if this could be initialized instead of the memset() +
assignment below. Maybe splitting the function will make this more
convenient.

> +	unsigned int offset, of_flags;
> +	struct device *parent;
> +	int id, ret, lflags;

Should this be unsigned int, or enum gpio_lookup_flags?

>  
>  	/*
>  	 * Currently only #gpio-cells=2 is supported with the meaning of:
> @@ -915,11 +913,30 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
>  	if (args->args_count != 2)
>  		return -ENOENT;
>  
> +	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)
>  		return -EPROBE_DEFER;
>  
> +	parent = gpio_device_to_device(gdev);
> +
>  	/*
>  	 * Registering reset-gpio device might cause immediate
>  	 * bind, resulting in its probe() registering new reset controller thus
> @@ -936,6 +953,13 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
>  		}
>  	}
>  
> +	lflags = GPIO_PERSISTENT | (of_flags & GPIO_ACTIVE_LOW);

Could we get an of_flags_to_gpio_lookup_flags() kind of helper for
this?

> +
> +	memset(properties, 0, sizeof(properties));
> +	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;

regards
Philipp

Re: [PATCH 9/9] reset: gpio: use software nodes to setup the GPIO lookup

[Bartosz Golaszewski]

On Mon, Oct 6, 2025 at 5:55 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Mo, 2025-10-06 at 15:00 +0200, Bartosz Golaszewski wrote:
> > 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.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/reset/core.c | 132 ++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 78 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> > index c9f13020ca3a7b9273488497a7d4240d0af762b0..b3e6ba7a9c3d756d2e30dc20edda9c02b624aefd 100644
> > --- a/drivers/reset/core.c
> > +++ b/drivers/reset/core.c
> [...]
> > @@ -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_aux_device_release(struct device *dev)
>
> static void reset_gpio_aux_device_release(struct device *dev)
>
> [...]
> > @@ -903,8 +899,10 @@ 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 auxiliary_device *adev;
> > -     int id, ret;
> > +     struct property_entry properties[2];
>
> It would be nice if this could be initialized instead of the memset() +
> assignment below. Maybe splitting the function will make this more
> convenient.
>
> > +     unsigned int offset, of_flags;
> > +     struct device *parent;
> > +     int id, ret, lflags;
>
> Should this be unsigned int, or enum gpio_lookup_flags?

It's represented as u64 in struct software_node_ref_args so unsigned
int works fine.

>
> >
> >       /*
> >        * Currently only #gpio-cells=2 is supported with the meaning of:
> > @@ -915,11 +913,30 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
> >       if (args->args_count != 2)
> >               return -ENOENT;
> >
> > +     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)
> >               return -EPROBE_DEFER;
> >
> > +     parent = gpio_device_to_device(gdev);
> > +
> >       /*
> >        * Registering reset-gpio device might cause immediate
> >        * bind, resulting in its probe() registering new reset controller thus
> > @@ -936,6 +953,13 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
> >               }
> >       }
> >
> > +     lflags = GPIO_PERSISTENT | (of_flags & GPIO_ACTIVE_LOW);
>
> Could we get an of_flags_to_gpio_lookup_flags() kind of helper for
> this?
>

I have this on my TODO list but it's not straightforward. The defines
under include/dt-bindings/gpio/gpio.h and include/linux/gpio/machine.h
are named the same in some instances but have different values.
Additionally gpiolib-of.c (re)defines its own of_gpio_flags with the
values taken from the dt bindings. It's a mess to untangle. I will get
there but not just yet, hence the FIXME comment.

Bart

Re: [PATCH 1/9] software node: read the reference args via the fwnode API

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/9] software node: allow referencing firmware nodes

[Andy Shevchenko]

On Mon, Oct 06, 2025 at 03:00:18PM +0200, 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: use a union of different
> pointers in struct software_node_ref_args and add an enum indicating
> what kind of reference given instance of it is. 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.

...

> +enum software_node_ref_type {
> +	/* References a software node. */
> +	SOFTWARE_NODE_REF_SWNODE = 0,


I don't see why we need an explicit value here.

> +	/* References a firmware node. */
> +	SOFTWARE_NODE_REF_FWNODE,
> +};

...

>  /**
>   * 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;
> +	enum software_node_ref_type type;
> +	union {
> +		const struct software_node *swnode;
> +		struct fwnode_handle *fwnode;
> +	};

Can't we always have an fwnode reference?

>  	unsigned int nargs;
>  	u64 args[NR_FWNODE_REFERENCE_ARGS];
>  };

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/9] reset: rework reset-gpios handling

[Bartosz Golaszewski]

On Mon, Oct 6, 2025 at 3:00 PM Bartosz Golaszewski <brgl@bgdev.pl> 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.
>
> [1] https://lore.kernel.org/all/20250924-gpio-shared-v1-0-775e7efeb1a3@linaro.org/
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---

Are there any comments on the software node part before I respin it
with Philipp's comments addressed?

Bartosz

Re: [PATCH 2/9] software node: increase the reference of the swnode by its fwnode

[Andy Shevchenko]

On Mon, Oct 06, 2025 at 03:00:17PM +0200, 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 remark, add a comment or expand a kernel doc, if exists, to cover this nuance.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/9] software node: allow referencing firmware nodes

[Bartosz Golaszewski]

On Sat, Oct 18, 2025 at 7:34 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Oct 06, 2025 at 03:00:18PM +0200, 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: use a union of different
> > pointers in struct software_node_ref_args and add an enum indicating
> > what kind of reference given instance of it is. 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.
>
> ...
>
> > +enum software_node_ref_type {
> > +     /* References a software node. */
> > +     SOFTWARE_NODE_REF_SWNODE = 0,
>
>
> I don't see why we need an explicit value here.
>

It was to make it clear, this is the default value and it's the one
used in older code with the legacy macros. I can drop it, it's no big
deal.

> > +     /* References a firmware node. */
> > +     SOFTWARE_NODE_REF_FWNODE,
> > +};
>
> ...
>
> >  /**
> >   * 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;
> > +     enum software_node_ref_type type;
> > +     union {
> > +             const struct software_node *swnode;
> > +             struct fwnode_handle *fwnode;
> > +     };
>
> Can't we always have an fwnode reference?
>

Unfortunately no. A const struct software_node is not yet a full
fwnode, it's just a template that becomes an actual firmware node when
it's registered with the swnode framework. However in order to allow
creating a graph of software nodes before we register them, we need a
way to reference those templates and then look them up internally in
swnode code.

Bart

> >       unsigned int nargs;
> >       u64 args[NR_FWNODE_REFERENCE_ARGS];
> >  };
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>

Re: [PATCH 3/9] software node: allow referencing firmware nodes

[Andy Shevchenko]

On Mon, Oct 20, 2025 at 10:06:43AM +0200, Bartosz Golaszewski wrote:
> On Sat, Oct 18, 2025 at 7:34 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Oct 06, 2025 at 03:00:18PM +0200, Bartosz Golaszewski wrote:

...

> > > +enum software_node_ref_type {
> > > +     /* References a software node. */
> > > +     SOFTWARE_NODE_REF_SWNODE = 0,
> >
> > I don't see why we need an explicit value here.
> 
> It was to make it clear, this is the default value and it's the one
> used in older code with the legacy macros. I can drop it, it's no big
> deal.

Usually when we assign a default value(s) in enum, it should be justified.
The common mistake here (not this case) is to use autoincrement feature
with some of the values explicitly defined for the enums that reflect the
HW bits / states, which obviously makes code fragile and easy to break.

> > > +     /* References a firmware node. */
> > > +     SOFTWARE_NODE_REF_FWNODE,
> > > +};

...

> > >  /**
> > >   * 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;
> > > +     enum software_node_ref_type type;
> > > +     union {
> > > +             const struct software_node *swnode;
> > > +             struct fwnode_handle *fwnode;
> > > +     };
> >
> > Can't we always have an fwnode reference?
> 
> Unfortunately no. A const struct software_node is not yet a full
> fwnode, it's just a template that becomes an actual firmware node when
> it's registered with the swnode framework. However in order to allow
> creating a graph of software nodes before we register them, we need a
> way to reference those templates and then look them up internally in
> swnode code.

Strange that you need this way. The IPU3 bridge driver (that creates a graph of
fwnodes at run-time for being consumed by the respective parts of v4l2
framework) IIRC has no such issue. Why your case is different?

> > >       unsigned int nargs;
> > >       u64 args[NR_FWNODE_REFERENCE_ARGS];
> > >  };

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/9] software node: allow referencing firmware nodes

[Bartosz Golaszewski]

On Mon, Oct 20, 2025 at 12:05 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> > >
> > > Can't we always have an fwnode reference?
> >
> > Unfortunately no. A const struct software_node is not yet a full
> > fwnode, it's just a template that becomes an actual firmware node when
> > it's registered with the swnode framework. However in order to allow
> > creating a graph of software nodes before we register them, we need a
> > way to reference those templates and then look them up internally in
> > swnode code.
>
> Strange that you need this way. The IPU3 bridge driver (that creates a graph of
> fwnodes at run-time for being consumed by the respective parts of v4l2
> framework) IIRC has no such issue. Why your case is different?
>

From what I can tell the ipu-bridge driver only references software
nodes (as struct software_node) from other software nodes. I need to
reference ANY implementation of firmware node from a software node.

Bartosz

Re: [PATCH 7/9] reset: make the provider of reset-gpios the parent of the reset device

[Bartosz Golaszewski]

On Mon, Oct 6, 2025 at 5:19 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> >       if (!label_tmp)
> >               return -EINVAL;
> > @@ -919,6 +915,11 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
> >       if (args->args_count != 2)
> >               return -ENOENT;
> >
> > +     struct gpio_device *gdev __free(gpio_device_put) =
> > +             gpio_device_find_by_fwnode(of_fwnode_handle(args->np));
>
> We are mixing cleanup helpers with gotos in this function, which the
> documentation in cleanup.h explicitly advises against.
>
> I know the current code is already guilty, but could you take this
> opportunity to prepend a patch that splits the part under guard() into
> a separate function?
>

If I'm being honest, I'd just make everything else use __free() as
well. Except for IDA, it's possible.

That being said: I have another thing in the works, namely converting
the OF code to fwnode in reset core. I may address this there as I'll
be moving stuff around. Does this make sense?

> I'd also move this block after the lockdep_assert_not_held() below.
>

Yeah lockdep asserts should be at the top of the function.

Bart

Re: [PATCH 7/9] reset: make the provider of reset-gpios the parent of the reset device

[Bartosz Golaszewski]

On Mon, Oct 6, 2025 at 5:19 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> >
> > @@ -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);
>
> This is the only remaining use of gdev in
> __reset_add_reset_gpio_lookup().
> It would make sense to move this as well and only pass the label.
>
> Given that all this is removed in patch 9, this is not super important.
>

I'll allow myself to skip it then because it causes a surprising
number of conflicts later into the series.

Bartosz

Re: [PATCH 3/9] software node: allow referencing firmware nodes

[Sakari Ailus]

Hi Bartosz, Andy,

On Mon, Oct 20, 2025 at 01:26:59PM +0200, Bartosz Golaszewski wrote:
> On Mon, Oct 20, 2025 at 12:05 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > > >
> > > > Can't we always have an fwnode reference?
> > >
> > > Unfortunately no. A const struct software_node is not yet a full
> > > fwnode, it's just a template that becomes an actual firmware node when
> > > it's registered with the swnode framework. However in order to allow
> > > creating a graph of software nodes before we register them, we need a
> > > way to reference those templates and then look them up internally in
> > > swnode code.
> >
> > Strange that you need this way. The IPU3 bridge driver (that creates a graph of
> > fwnodes at run-time for being consumed by the respective parts of v4l2
> > framework) IIRC has no such issue. Why your case is different?
> >
> 
> From what I can tell the ipu-bridge driver only references software
> nodes (as struct software_node) from other software nodes. I need to
> reference ANY implementation of firmware node from a software node.

Yes, the IPU bridge only references software nodes.

I might use two distinct pointers instead of an union and an integer field
that tells which type is the right one. I don't expect more such cases
here; it's either a software node or an fwnode handle (ACPI or OF node).

-- 
Regards,

Sakari Ailus

Re: [PATCH 3/9] software node: allow referencing firmware nodes

[Bartosz Golaszewski]

On Tue, 21 Oct 2025 08:54:22 +0200, Sakari Ailus
<sakari.ailus@linux.intel.com> said:
> Hi Bartosz, Andy,
>
> On Mon, Oct 20, 2025 at 01:26:59PM +0200, Bartosz Golaszewski wrote:
>> On Mon, Oct 20, 2025 at 12:05 PM Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>> >
>> > > >
>> > > > Can't we always have an fwnode reference?
>> > >
>> > > Unfortunately no. A const struct software_node is not yet a full
>> > > fwnode, it's just a template that becomes an actual firmware node when
>> > > it's registered with the swnode framework. However in order to allow
>> > > creating a graph of software nodes before we register them, we need a
>> > > way to reference those templates and then look them up internally in
>> > > swnode code.
>> >
>> > Strange that you need this way. The IPU3 bridge driver (that creates a graph of
>> > fwnodes at run-time for being consumed by the respective parts of v4l2
>> > framework) IIRC has no such issue. Why your case is different?
>> >
>>
>> From what I can tell the ipu-bridge driver only references software
>> nodes (as struct software_node) from other software nodes. I need to
>> reference ANY implementation of firmware node from a software node.
>
> Yes, the IPU bridge only references software nodes.
>
> I might use two distinct pointers instead of an union and an integer field
> that tells which type is the right one. I don't expect more such cases
> here; it's either a software node or an fwnode handle (ACPI or OF node).
>

Like:

struct software_node_ref_args {
	const struct software_node *swnode;
	struct fwnode_handle *fwnode;
	unsigned int nargs;
	u64 args[NR_FWNODE_REFERENCE_ARGS];
};

And then if swnode is NULL then assume fwnode must not be?

I'm not sure if it's necessarily better but I don't have a strong opinion on
this either.

Bartosz

Re: [PATCH 3/9] software node: allow referencing firmware nodes

[Andy Shevchenko]

On Tue, Oct 21, 2025 at 02:06:36AM -0700, Bartosz Golaszewski wrote:
> On Tue, 21 Oct 2025 08:54:22 +0200, Sakari Ailus
> <sakari.ailus@linux.intel.com> said:
> > On Mon, Oct 20, 2025 at 01:26:59PM +0200, Bartosz Golaszewski wrote:
> >> On Mon, Oct 20, 2025 at 12:05 PM Andy Shevchenko
> >> <andriy.shevchenko@linux.intel.com> wrote:

...

> >> > > > Can't we always have an fwnode reference?
> >> > >
> >> > > Unfortunately no. A const struct software_node is not yet a full
> >> > > fwnode, it's just a template that becomes an actual firmware node when
> >> > > it's registered with the swnode framework. However in order to allow
> >> > > creating a graph of software nodes before we register them, we need a
> >> > > way to reference those templates and then look them up internally in
> >> > > swnode code.
> >> >
> >> > Strange that you need this way. The IPU3 bridge driver (that creates a graph of
> >> > fwnodes at run-time for being consumed by the respective parts of v4l2
> >> > framework) IIRC has no such issue. Why your case is different?
> >>
> >> From what I can tell the ipu-bridge driver only references software
> >> nodes (as struct software_node) from other software nodes. I need to
> >> reference ANY implementation of firmware node from a software node.
> >
> > Yes, the IPU bridge only references software nodes.
> >
> > I might use two distinct pointers instead of an union and an integer field
> > that tells which type is the right one. I don't expect more such cases
> > here; it's either a software node or an fwnode handle (ACPI or OF node).
> 
> Like:
> 
> struct software_node_ref_args {
> 	const struct software_node *swnode;
> 	struct fwnode_handle *fwnode;
> 	unsigned int nargs;
> 	u64 args[NR_FWNODE_REFERENCE_ARGS];
> };
> 
> And then if swnode is NULL then assume fwnode must not be?
> 
> I'm not sure if it's necessarily better but I don't have a strong opinion on
> this either.

At least it is slightly closer to what I ideally want to have (but not in this
design seems), so +1 to Sakari's proposal.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/9] reset: make the provider of reset-gpios the parent of the reset device

[Philipp Zabel]

On Mo, 2025-10-20 at 17:25 +0200, Bartosz Golaszewski wrote:
> On Mon, Oct 6, 2025 at 5:19 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > [...] could you take this
> > opportunity to prepend a patch that splits the part under guard() into
> > a separate function?
> 
> If I'm being honest, I'd just make everything else use __free() as
> well. Except for IDA, it's possible.
> 
> That being said: I have another thing in the works, namely converting
> the OF code to fwnode in reset core. I may address this there as I'll
> be moving stuff around. Does this make sense?

Yes. There was already a previous attempt at fwnode support [1], but we
abandoned that when there was no use case anymore.

[1] https://lore.kernel.org/r/20220323095022.453708-3-clement.leger@bootlin.com

> 
regards
Philipp

Re: [PATCH 7/9] reset: make the provider of reset-gpios the parent of the reset device

[Bartosz Golaszewski]

On Tue, Oct 21, 2025 at 11:17 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Mo, 2025-10-20 at 17:25 +0200, Bartosz Golaszewski wrote:
> > On Mon, Oct 6, 2025 at 5:19 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > [...] could you take this
> > > opportunity to prepend a patch that splits the part under guard() into
> > > a separate function?
> >
> > If I'm being honest, I'd just make everything else use __free() as
> > well. Except for IDA, it's possible.
> >
> > That being said: I have another thing in the works, namely converting
> > the OF code to fwnode in reset core. I may address this there as I'll
> > be moving stuff around. Does this make sense?
>
> Yes. There was already a previous attempt at fwnode support [1], but we
> abandoned that when there was no use case anymore.
>
> [1] https://lore.kernel.org/r/20220323095022.453708-3-clement.leger@bootlin.com
>

Ah, what was the exact reason for abandoning this? It's not clear from
the email thread.

To be clear: I think that we can convert the core reset code to fwnode
without necessarily converting all the drivers right away.

Bart

Re: [PATCH 7/9] reset: make the provider of reset-gpios the parent of the reset device

[Philipp Zabel]

On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:
> On Tue, Oct 21, 2025 at 11:17 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > 
> > On Mo, 2025-10-20 at 17:25 +0200, Bartosz Golaszewski wrote:
> > > On Mon, Oct 6, 2025 at 5:19 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > [...] could you take this
> > > > opportunity to prepend a patch that splits the part under guard() into
> > > > a separate function?
> > > 
> > > If I'm being honest, I'd just make everything else use __free() as
> > > well. Except for IDA, it's possible.
> > > 
> > > That being said: I have another thing in the works, namely converting
> > > the OF code to fwnode in reset core. I may address this there as I'll
> > > be moving stuff around. Does this make sense?
> > 
> > Yes. There was already a previous attempt at fwnode support [1], but we
> > abandoned that when there was no use case anymore.
> > 
> > [1] https://lore.kernel.org/r/20220323095022.453708-3-clement.leger@bootlin.com
> > 
> 
> Ah, what was the exact reason for abandoning this? It's not clear from
> the email thread.
> 
> To be clear: I think that we can convert the core reset code to fwnode
> without necessarily converting all the drivers right away.

The use case vanished in patch review.

No need to convert all existing drivers right away, but I'd like to see
a user that benefits from the conversion.

regards
Philipp

Re: [PATCH 7/9] reset: make the provider of reset-gpios the parent of the reset device

[Bartosz Golaszewski]

On Tue, Oct 21, 2025 at 11:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:
> > On Tue, Oct 21, 2025 at 11:17 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > >
> > > On Mo, 2025-10-20 at 17:25 +0200, Bartosz Golaszewski wrote:
> > > > On Mon, Oct 6, 2025 at 5:19 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > > [...] could you take this
> > > > > opportunity to prepend a patch that splits the part under guard() into
> > > > > a separate function?
> > > >
> > > > If I'm being honest, I'd just make everything else use __free() as
> > > > well. Except for IDA, it's possible.
> > > >
> > > > That being said: I have another thing in the works, namely converting
> > > > the OF code to fwnode in reset core. I may address this there as I'll
> > > > be moving stuff around. Does this make sense?
> > >
> > > Yes. There was already a previous attempt at fwnode support [1], but we
> > > abandoned that when there was no use case anymore.
> > >
> > > [1] https://lore.kernel.org/r/20220323095022.453708-3-clement.leger@bootlin.com
> > >
> >
> > Ah, what was the exact reason for abandoning this? It's not clear from
> > the email thread.
> >
> > To be clear: I think that we can convert the core reset code to fwnode
> > without necessarily converting all the drivers right away.
>
> The use case vanished in patch review.
>
> No need to convert all existing drivers right away, but I'd like to see
> a user that benefits from the conversion.
>

The first obvious user will be the reset-gpio driver which will see
its core code simplified as we won't need to cast between OF and
fwnodes.

Bartosz

Re: [PATCH 7/9] reset: make the provider of reset-gpios the parent of the reset device

[Andy Shevchenko]

On Tue, Oct 21, 2025 at 11:39:41AM +0200, Bartosz Golaszewski wrote:
> On Tue, Oct 21, 2025 at 11:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:

[...]

> > No need to convert all existing drivers right away, but I'd like to see
> > a user that benefits from the conversion.
> >
> 
> The first obvious user will be the reset-gpio driver which will see
> its core code simplified as we won't need to cast between OF and
> fwnodes.

+1 to Bart's work. reset-gpio in current form is useless in all my cases
(it's OF-centric in 2025! We should not do that in a new code).

More over, conversion to reset-gpio from open coded GPIO APIs is a clear
regression and I want to NAK all those changes (if any already done) for
the discrete components that may be used outside of certainly OF-only niche of
the platforms.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/9] reset: make the provider of reset-gpios the parent of the reset device

[Andy Shevchenko]

On Tue, Oct 21, 2025 at 05:55:02PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 21, 2025 at 11:39:41AM +0200, Bartosz Golaszewski wrote:
> > On Tue, Oct 21, 2025 at 11:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:

[...]

> > > No need to convert all existing drivers right away, but I'd like to see
> > > a user that benefits from the conversion.
> > >
> > 
> > The first obvious user will be the reset-gpio driver which will see
> > its core code simplified as we won't need to cast between OF and
> > fwnodes.
> 
> +1 to Bart's work. reset-gpio in current form is useless in all my cases
> (it's OF-centric in 2025! We should not do that in a new code).
> 
> More over, conversion to reset-gpio from open coded GPIO APIs is a clear
> regression and I want to NAK all those changes (if any already done) for
> the discrete components that may be used outside of certainly OF-only niche of
> the platforms.

To be clear, the conversion that's done while reset-gpio is kept OF-centric.
I'm in favour of using it, but we need to make it agnostic.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/9] reset: make the provider of reset-gpios the parent of the reset device

[Bartosz Golaszewski]

On Tue, Oct 21, 2025 at 5:03 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Oct 21, 2025 at 05:55:02PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 21, 2025 at 11:39:41AM +0200, Bartosz Golaszewski wrote:
> > > On Tue, Oct 21, 2025 at 11:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:
>
> [...]
>
> > > > No need to convert all existing drivers right away, but I'd like to see
> > > > a user that benefits from the conversion.
> > > >
> > >
> > > The first obvious user will be the reset-gpio driver which will see
> > > its core code simplified as we won't need to cast between OF and
> > > fwnodes.
> >
> > +1 to Bart's work. reset-gpio in current form is useless in all my cases
> > (it's OF-centric in 2025! We should not do that in a new code).
> >
> > More over, conversion to reset-gpio from open coded GPIO APIs is a clear
> > regression and I want to NAK all those changes (if any already done) for
> > the discrete components that may be used outside of certainly OF-only niche of
> > the platforms.
>
> To be clear, the conversion that's done while reset-gpio is kept OF-centric.
> I'm in favour of using it, but we need to make it agnostic.
>

As of now, the whole reset framework is completely OF-centric, I don't
know what good blocking any such conversions would bring? I intend to
convert the reset core but not individual drivers.

Bart

Re: [PATCH 7/9] reset: make the provider of reset-gpios the parent of the reset device

[Andy Shevchenko]

On Tue, Oct 21, 2025 at 05:23:33PM +0200, Bartosz Golaszewski wrote:
> On Tue, Oct 21, 2025 at 5:03 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Oct 21, 2025 at 05:55:02PM +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 21, 2025 at 11:39:41AM +0200, Bartosz Golaszewski wrote:
> > > > On Tue, Oct 21, 2025 at 11:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > > On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:

[...]

> > > > > No need to convert all existing drivers right away, but I'd like to see
> > > > > a user that benefits from the conversion.
> > > > >
> > > >
> > > > The first obvious user will be the reset-gpio driver which will see
> > > > its core code simplified as we won't need to cast between OF and
> > > > fwnodes.
> > >
> > > +1 to Bart's work. reset-gpio in current form is useless in all my cases
> > > (it's OF-centric in 2025! We should not do that in a new code).
> > >
> > > More over, conversion to reset-gpio from open coded GPIO APIs is a clear
> > > regression and I want to NAK all those changes (if any already done) for
> > > the discrete components that may be used outside of certainly OF-only niche of
> > > the platforms.
> >
> > To be clear, the conversion that's done while reset-gpio is kept OF-centric.
> > I'm in favour of using it, but we need to make it agnostic.
> 
> As of now, the whole reset framework is completely OF-centric, I don't
> know what good blocking any such conversions would bring? I intend to
> convert the reset core but not individual drivers.

Blocking making new regressions?

Otherwise as long as reset framework and reset-gpio are agnostic, I'm pretty
much fine with the idea and conversion.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/9] software node: read the reference args via the fwnode API

[Bartosz Golaszewski]

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

Re: [PATCH 1/9] software node: read the reference args via the fwnode API

[Sakari Ailus]

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 */

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/9] software node: read the reference args via the fwnode API

[Bartosz Golaszewski]

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

Re: [PATCH 7/9] reset: make the provider of reset-gpios the parent of the reset device

[Philipp Zabel]

On Di, 2025-10-21 at 18:47 +0300, Andy Shevchenko wrote:
> On Tue, Oct 21, 2025 at 05:23:33PM +0200, Bartosz Golaszewski wrote:
> > On Tue, Oct 21, 2025 at 5:03 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Tue, Oct 21, 2025 at 05:55:02PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Oct 21, 2025 at 11:39:41AM +0200, Bartosz Golaszewski wrote:
> > > > > On Tue, Oct 21, 2025 at 11:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > > > On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:
> 
> [...]
> 
> > > > > > No need to convert all existing drivers right away, but I'd like to see
> > > > > > a user that benefits from the conversion.
> > > > > > 
> > > > > 
> > > > > The first obvious user will be the reset-gpio driver which will see
> > > > > its core code simplified as we won't need to cast between OF and
> > > > > fwnodes.
> > > > 
> > > > +1 to Bart's work. reset-gpio in current form is useless in all my cases
> > > > (it's OF-centric in 2025! We should not do that in a new code).
> > > > 
> > > > More over, conversion to reset-gpio from open coded GPIO APIs is a clear
> > > > regression and I want to NAK all those changes (if any already done) for
> > > > the discrete components that may be used outside of certainly OF-only niche of
> > > > the platforms.
> > > 
> > > To be clear, the conversion that's done while reset-gpio is kept OF-centric.
> > > I'm in favour of using it, but we need to make it agnostic.
> > 
> > As of now, the whole reset framework is completely OF-centric, I don't
> > know what good blocking any such conversions would bring? I intend to
> > convert the reset core but not individual drivers.
> 
> Blocking making new regressions?
> 
> Otherwise as long as reset framework and reset-gpio are agnostic, I'm pretty
> much fine with the idea and conversion.

I think we might be talking about different "conversions" and different
"blocking" here?

1) Conversion of the reset core from of_node to fwnode.
2) Conversion of reset controller drivers from of_node to fwnode.
3) Conversion of consumer drivers from gpiod to reset_control API.

My understanding is:

Bartosz would like to convert the reset core to fwnode (1) but not
convert all the individual reset controller drivers (2). He doesn't
like blocking (1) - this statement was partially in reaction to me
bringing up a previous attempt that didn't go through.

Andy would like to block consumer driver conversions from gpiod to
reset_control API (3) while the reset-gpio driver only works on OF
platforms.

Please correct me if and where I misunderstood.

I think fwnode conversion of the reset controller framework core is a
good idea, I'd just like to see a use case accompanying the conversion.
It seems like enabling the reset-gpio driver to be used on non-OF
platforms could be that. Andy, do you have an actual case in mind?

Certainly dropping the gpiod reset handling from consumer drivers
should not be done while it introduces regressions.

regards
Philipp

Re: [PATCH 7/9] reset: make the provider of reset-gpios the parent of the reset device

[Bartosz Golaszewski]

On Wed, Oct 22, 2025 at 10:39 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Di, 2025-10-21 at 18:47 +0300, Andy Shevchenko wrote:
> > On Tue, Oct 21, 2025 at 05:23:33PM +0200, Bartosz Golaszewski wrote:
> > > On Tue, Oct 21, 2025 at 5:03 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Tue, Oct 21, 2025 at 05:55:02PM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Oct 21, 2025 at 11:39:41AM +0200, Bartosz Golaszewski wrote:
> > > > > > On Tue, Oct 21, 2025 at 11:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > > > > On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:
> >
> > [...]
> >
> > > > > > > No need to convert all existing drivers right away, but I'd like to see
> > > > > > > a user that benefits from the conversion.
> > > > > > >
> > > > > >
> > > > > > The first obvious user will be the reset-gpio driver which will see
> > > > > > its core code simplified as we won't need to cast between OF and
> > > > > > fwnodes.
> > > > >
> > > > > +1 to Bart's work. reset-gpio in current form is useless in all my cases
> > > > > (it's OF-centric in 2025! We should not do that in a new code).
> > > > >
> > > > > More over, conversion to reset-gpio from open coded GPIO APIs is a clear
> > > > > regression and I want to NAK all those changes (if any already done) for
> > > > > the discrete components that may be used outside of certainly OF-only niche of
> > > > > the platforms.
> > > >
> > > > To be clear, the conversion that's done while reset-gpio is kept OF-centric.
> > > > I'm in favour of using it, but we need to make it agnostic.
> > >
> > > As of now, the whole reset framework is completely OF-centric, I don't
> > > know what good blocking any such conversions would bring? I intend to
> > > convert the reset core but not individual drivers.
> >
> > Blocking making new regressions?
> >
> > Otherwise as long as reset framework and reset-gpio are agnostic, I'm pretty
> > much fine with the idea and conversion.
>
> I think we might be talking about different "conversions" and different
> "blocking" here?
>
> 1) Conversion of the reset core from of_node to fwnode.
> 2) Conversion of reset controller drivers from of_node to fwnode.
> 3) Conversion of consumer drivers from gpiod to reset_control API.
>
> My understanding is:
>
> Bartosz would like to convert the reset core to fwnode (1) but not
> convert all the individual reset controller drivers (2). He doesn't
> like blocking (1) - this statement was partially in reaction to me
> bringing up a previous attempt that didn't go through.
>
> Andy would like to block consumer driver conversions from gpiod to
> reset_control API (3) while the reset-gpio driver only works on OF
> platforms.
>
> Please correct me if and where I misunderstood.
>

I think Andy is afraid that people will convert drivers that are used
in the fwnode world to reset-gpio which only works with OF. I don't
think that anyone's trying to do it though.

> I think fwnode conversion of the reset controller framework core is a
> good idea, I'd just like to see a use case accompanying the conversion.
> It seems like enabling the reset-gpio driver to be used on non-OF
> platforms could be that. Andy, do you have an actual case in mind?
>

I'd say converting the reset core to fwnode has merits on its own. We
should typically use the highest available abstraction layer (which is
fwnode in this case) unless we absolutely have no choice (for
instance: using some very OF-specific APIs).

That being said: the reset-gpio driver will be able to work with any
firmware node once we do the conversion which is a good first
use-case.

Bartosz

Re: [PATCH 7/9] reset: make the provider of reset-gpios the parent of the reset device

[Andy Shevchenko]

On Wed, Oct 22, 2025 at 02:17:53PM +0200, Bartosz Golaszewski wrote:
> On Wed, Oct 22, 2025 at 10:39 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > On Di, 2025-10-21 at 18:47 +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 21, 2025 at 05:23:33PM +0200, Bartosz Golaszewski wrote:
> > > > On Tue, Oct 21, 2025 at 5:03 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Tue, Oct 21, 2025 at 05:55:02PM +0300, Andy Shevchenko wrote:
> > > > > > On Tue, Oct 21, 2025 at 11:39:41AM +0200, Bartosz Golaszewski wrote:
> > > > > > > On Tue, Oct 21, 2025 at 11:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > > > > > > > On Di, 2025-10-21 at 11:27 +0200, Bartosz Golaszewski wrote:

[...]

> > > > > > > > No need to convert all existing drivers right away, but I'd like to see
> > > > > > > > a user that benefits from the conversion.
> > > > > > > >
> > > > > > >
> > > > > > > The first obvious user will be the reset-gpio driver which will see
> > > > > > > its core code simplified as we won't need to cast between OF and
> > > > > > > fwnodes.
> > > > > >
> > > > > > +1 to Bart's work. reset-gpio in current form is useless in all my cases
> > > > > > (it's OF-centric in 2025! We should not do that in a new code).
> > > > > >
> > > > > > More over, conversion to reset-gpio from open coded GPIO APIs is a clear
> > > > > > regression and I want to NAK all those changes (if any already done) for
> > > > > > the discrete components that may be used outside of certainly OF-only niche of
> > > > > > the platforms.
> > > > >
> > > > > To be clear, the conversion that's done while reset-gpio is kept OF-centric.
> > > > > I'm in favour of using it, but we need to make it agnostic.
> > > >
> > > > As of now, the whole reset framework is completely OF-centric, I don't
> > > > know what good blocking any such conversions would bring? I intend to
> > > > convert the reset core but not individual drivers.
> > >
> > > Blocking making new regressions?
> > >
> > > Otherwise as long as reset framework and reset-gpio are agnostic, I'm pretty
> > > much fine with the idea and conversion.
> >
> > I think we might be talking about different "conversions" and different
> > "blocking" here?
> >
> > 1) Conversion of the reset core from of_node to fwnode.
> > 2) Conversion of reset controller drivers from of_node to fwnode.
> > 3) Conversion of consumer drivers from gpiod to reset_control API.
> >
> > My understanding is:
> >
> > Bartosz would like to convert the reset core to fwnode (1) but not
> > convert all the individual reset controller drivers (2). He doesn't
> > like blocking (1) - this statement was partially in reaction to me
> > bringing up a previous attempt that didn't go through.
> >
> > Andy would like to block consumer driver conversions from gpiod to
> > reset_control API (3) while the reset-gpio driver only works on OF
> > platforms.
> >
> > Please correct me if and where I misunderstood.
> 
> I think Andy is afraid that people will convert drivers that are used
> in the fwnode world to reset-gpio which only works with OF. I don't
> think that anyone's trying to do it though.

You are both right about my worries and there is of course the case.
https://patch.msgid.link/1720009575-11677-1-git-send-email-shengjiu.wang@nxp.com

The mentioned change should be reverted.

And this was just found by a couple of minutes of `git log --grep`. I am pretty
sure there are handful of a such wrong patches.

Compare to https://patch.msgid.link/20250815172353.2430981-3-mohammad.rafi.shaik@oss.qualcomm.com
which is done correctly (it doesn't  break old functionality on non-OF platforms).

> > I think fwnode conversion of the reset controller framework core is a
> > good idea, I'd just like to see a use case accompanying the conversion.
> > It seems like enabling the reset-gpio driver to be used on non-OF
> > platforms could be that. Andy, do you have an actual case in mind?
> 
> I'd say converting the reset core to fwnode has merits on its own. We
> should typically use the highest available abstraction layer (which is
> fwnode in this case) unless we absolutely have no choice (for
> instance: using some very OF-specific APIs).
> 
> That being said: the reset-gpio driver will be able to work with any
> firmware node once we do the conversion which is a good first
> use-case.

+1, as I already mentioned I am in favour of this change.

-- 
With Best Regards,
Andy Shevchenko