| Message ID | 20251006-reset-gpios-swnodes-v1-9-6d3325b9af42@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | reset: rework reset-gpios handling | expand |
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
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
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);