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 | 129 ++++++++++++++++++++++++++++++---------------------
1 file changed, 75 insertions(+), 54 deletions(-)
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index fcf1c24086e565015b0956fdd40334274a1edb00..66a99bdfb7093232d169c92d49cb953c4b1033e6 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;
};
@@ -822,52 +825,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;
}
/*
@@ -875,9 +871,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:
@@ -895,6 +893,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)
@@ -909,6 +924,10 @@ 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("reset-gpios", parent->fwnode, offset, lflags);
+
id = ida_alloc(&reset_gpio_ida, GFP_KERNEL);
if (id < 0)
return id;
@@ -920,11 +939,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
@@ -932,19 +946,26 @@ 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)) {
+ ret = PTR_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.51.0On Wed, Nov 05, 2025 at 09:47:39AM +0100, Bartosz Golaszewski wrote:
>
> 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.
...
> static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
> {
> + struct property_entry properties[] = { {}, {} };
It's an interesting way of saying this?
struct property_entry properties[2] = { };
> struct reset_gpio_lookup *rgpio_dev;
> + unsigned int offset, of_flags;
> + struct device *parent;
> + int id, ret, lflags;
I assumed that lflags shouldn't be signed. And IIRC they are unsigned long
elsewhere (yes, just confirmed).
...
> + rgpio_dev->swnode = fwnode_create_software_node(properties, NULL);
> + if (IS_ERR(rgpio_dev->swnode)) {
> + ret = PTR_ERR(rgpio_dev->swnode);
> + goto err_put_of_node;
> + }
Can save 1 LoC?
rgpio_dev->swnode = fwnode_create_software_node(properties, NULL);
ret = PTR_ERR_OR_ZERO(rgpio_dev->swnode);
if (ret)
goto err_put_of_node;
--
With Best Regards,
Andy Shevchenko
On Wed, Nov 5, 2025 at 3:10 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Nov 05, 2025 at 09:47:39AM +0100, Bartosz Golaszewski wrote:
> >
> > 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.
>
> ...
>
> > static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
> > {
> > + struct property_entry properties[] = { {}, {} };
>
> It's an interesting way of saying this?
>
I hope this is the final round of nit-picking...
> struct property_entry properties[2] = { };
>
> > struct reset_gpio_lookup *rgpio_dev;
> > + unsigned int offset, of_flags;
> > + struct device *parent;
> > + int id, ret, lflags;
>
> I assumed that lflags shouldn't be signed. And IIRC they are unsigned long
> elsewhere (yes, just confirmed).
>
It doesn't really matter that much here but whatever. I do plan on
tackling the duplication of machine and OF flags at some point though.
> ...
>
> > + rgpio_dev->swnode = fwnode_create_software_node(properties, NULL);
> > + if (IS_ERR(rgpio_dev->swnode)) {
> > + ret = PTR_ERR(rgpio_dev->swnode);
> > + goto err_put_of_node;
> > + }
>
> Can save 1 LoC?
>
Oh come on...
Bart
> rgpio_dev->swnode = fwnode_create_software_node(properties, NULL);
> ret = PTR_ERR_OR_ZERO(rgpio_dev->swnode);
> if (ret)
> goto err_put_of_node;
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
© 2016 - 2025 Red Hat, Inc.