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 | 126 +++++++++++++++++++++++++++++----------------------
1 file changed, 73 insertions(+), 53 deletions(-)
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index cefeff10f6c82f5aef269a6d3a58d9d204ed6b7e..8262879e3f0d9ce67683c6baa00d9eea9e3c3ca3 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,8 +871,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 property_entry properties[2] = { };
+ unsigned int offset, of_flags, lflags;
struct reset_gpio_lookup *rgpio_dev;
- struct auxiliary_device *adev;
+ struct device *parent;
int id, ret;
/*
@@ -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,25 @@ 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);
+ ret = PTR_ERR(rgpio_dev->swnode);
if (ret)
- goto err_put;
+ goto err_put_of_node;
+
+ ret = reset_add_gpio_aux_device(parent, rgpio_dev->swnode, id,
+ &rgpio_dev->of_args);
+ if (ret)
+ 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 Do, 2025-11-06 at 15:32 +0100, 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.
>
> 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 | 126 +++++++++++++++++++++++++++++----------------------
> 1 file changed, 73 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index cefeff10f6c82f5aef269a6d3a58d9d204ed6b7e..8262879e3f0d9ce67683c6baa00d9eea9e3c3ca3 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,8 +871,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 property_entry properties[2] = { };
> + unsigned int offset, of_flags, lflags;
> struct reset_gpio_lookup *rgpio_dev;
> - struct auxiliary_device *adev;
> + struct device *parent;
> int id, ret;
>
> /*
> @@ -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,25 @@ 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);
> + ret = PTR_ERR(rgpio_dev->swnode);
I'll apply this with the following patch squashed in:
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 3edf04ae8a95..8a7b112a9a77 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -945,7 +945,7 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
of_node_get(rgpio_dev->of_args.np);
rgpio_dev->swnode = fwnode_create_software_node(properties, NULL);
- ret = PTR_ERR(rgpio_dev->swnode);
+ ret = PTR_ERR_OR_ZERO(rgpio_dev->swnode);
if (ret)
goto err_put_of_node;
regards
Philipp
On Tue, Nov 18, 2025 at 5:44 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > On Do, 2025-11-06 at 15:32 +0100, 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. > > > > 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> > > --- > > I'll apply this with the following patch squashed in: > > diff --git a/drivers/reset/core.c b/drivers/reset/core.c > index 3edf04ae8a95..8a7b112a9a77 100644 > --- a/drivers/reset/core.c > +++ b/drivers/reset/core.c > @@ -945,7 +945,7 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args) > of_node_get(rgpio_dev->of_args.np); > > rgpio_dev->swnode = fwnode_create_software_node(properties, NULL); > - ret = PTR_ERR(rgpio_dev->swnode); > + ret = PTR_ERR_OR_ZERO(rgpio_dev->swnode); > if (ret) > goto err_put_of_node; Huh? Why? Bartosz
On Di, 2025-11-18 at 18:08 +0100, Bartosz Golaszewski wrote: > On Tue, Nov 18, 2025 at 5:44 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > On Do, 2025-11-06 at 15:32 +0100, 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. > > > > > > 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> > > > --- > > > > I'll apply this with the following patch squashed in: Strike that, I'll have to wait for the SPI issue to be resolved. > > diff --git a/drivers/reset/core.c b/drivers/reset/core.c > > index 3edf04ae8a95..8a7b112a9a77 100644 > > --- a/drivers/reset/core.c > > +++ b/drivers/reset/core.c > > @@ -945,7 +945,7 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args) > > of_node_get(rgpio_dev->of_args.np); > > > > rgpio_dev->swnode = fwnode_create_software_node(properties, NULL); > > - ret = PTR_ERR(rgpio_dev->swnode); > > + ret = PTR_ERR_OR_ZERO(rgpio_dev->swnode); > > if (ret) > > goto err_put_of_node; > > Huh? Why? PTR_ERR(ptr) is just (long)ptr, so a valid swnode pointer makes ret non-zero and takes us into the error path. PTR_ERR_OR_ZERO() includes the IS_ERR() check and returns 0 for non-error pointers. And there is a (false-positive) sparse warning: drivers/reset/core.c:978 __reset_add_reset_gpio_device() warn: passing zero to 'PTR_ERR' I think it would be better to return to the explicit IS_ERR() check from v5. regards Philipp
On Wed, 19 Nov 2025 09:19:30 +0100, Philipp Zabel <p.zabel@pengutronix.de> said: > On Di, 2025-11-18 at 18:08 +0100, Bartosz Golaszewski wrote: >> On Tue, Nov 18, 2025 at 5:44 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: >> > >> > On Do, 2025-11-06 at 15:32 +0100, 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. >> > > >> > > 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> >> > > --- >> > >> > I'll apply this with the following patch squashed in: > > Strike that, I'll have to wait for the SPI issue to be resolved. > >> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c >> > index 3edf04ae8a95..8a7b112a9a77 100644 >> > --- a/drivers/reset/core.c >> > +++ b/drivers/reset/core.c >> > @@ -945,7 +945,7 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args) >> > of_node_get(rgpio_dev->of_args.np); >> > >> > rgpio_dev->swnode = fwnode_create_software_node(properties, NULL); >> > - ret = PTR_ERR(rgpio_dev->swnode); >> > + ret = PTR_ERR_OR_ZERO(rgpio_dev->swnode); >> > if (ret) >> > goto err_put_of_node; >> >> Huh? Why? > > PTR_ERR(ptr) is just (long)ptr, so a valid swnode pointer makes ret > non-zero and takes us into the error path. PTR_ERR_OR_ZERO() includes > the IS_ERR() check and returns 0 for non-error pointers. > > And there is a (false-positive) sparse warning: > > drivers/reset/core.c:978 __reset_add_reset_gpio_device() warn: passing zero to 'PTR_ERR' > > I think it would be better to return to the explicit IS_ERR() check > from v5. > Yes, it was like this in my initial submission and seems like it's more readable and doesn't cause this confusion. I will go back to it. Though I'm not sure if the SPI issue will require a v5 - I'm looking into fixing it in the affected driver. Bart
On Mi, 2025-11-19 at 00:28 -0800, Bartosz Golaszewski wrote: > On Wed, 19 Nov 2025 09:19:30 +0100, Philipp Zabel <p.zabel@pengutronix.de> said: > > On Di, 2025-11-18 at 18:08 +0100, Bartosz Golaszewski wrote: > > > On Tue, Nov 18, 2025 at 5:44 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > > > > > On Do, 2025-11-06 at 15:32 +0100, 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. > > > > > > > > > > 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> > > > > > --- > > > > > > > > I'll apply this with the following patch squashed in: > > > > Strike that, I'll have to wait for the SPI issue to be resolved. > > > > > > diff --git a/drivers/reset/core.c b/drivers/reset/core.c > > > > index 3edf04ae8a95..8a7b112a9a77 100644 > > > > --- a/drivers/reset/core.c > > > > +++ b/drivers/reset/core.c > > > > @@ -945,7 +945,7 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args) > > > > of_node_get(rgpio_dev->of_args.np); > > > > > > > > rgpio_dev->swnode = fwnode_create_software_node(properties, NULL); > > > > - ret = PTR_ERR(rgpio_dev->swnode); > > > > + ret = PTR_ERR_OR_ZERO(rgpio_dev->swnode); > > > > if (ret) > > > > goto err_put_of_node; > > > > > > Huh? Why? > > > > PTR_ERR(ptr) is just (long)ptr, so a valid swnode pointer makes ret > > non-zero and takes us into the error path. PTR_ERR_OR_ZERO() includes > > the IS_ERR() check and returns 0 for non-error pointers. > > > > And there is a (false-positive) sparse warning: > > > > drivers/reset/core.c:978 __reset_add_reset_gpio_device() warn: passing zero to 'PTR_ERR' > > > > I think it would be better to return to the explicit IS_ERR() check > > from v5. > > > > Yes, it was like this in my initial submission and seems like it's more > readable and doesn't cause this confusion. I will go back to it. Though > I'm not sure if the SPI issue will require a v5 v7 :) If there are no other changes required, I can fix it up: https://git.pengutronix.de/cgit/pza/linux/commit/?id=8e72a48fc1df > - I'm looking into fixing it in the affected driver. Great, can we make sure the fix is applied first, to avoid breaking git bisect? regards Philipp
On Tue, Nov 18, 2025 at 06:08:17PM +0100, Bartosz Golaszewski wrote: > On Tue, Nov 18, 2025 at 5:44 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > On Do, 2025-11-06 at 15:32 +0100, Bartosz Golaszewski wrote: ... > > I'll apply this with the following patch squashed in: > > > > diff --git a/drivers/reset/core.c b/drivers/reset/core.c > > index 3edf04ae8a95..8a7b112a9a77 100644 > > --- a/drivers/reset/core.c > > +++ b/drivers/reset/core.c > > @@ -945,7 +945,7 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args) > > of_node_get(rgpio_dev->of_args.np); > > > > rgpio_dev->swnode = fwnode_create_software_node(properties, NULL); > > - ret = PTR_ERR(rgpio_dev->swnode); > > + ret = PTR_ERR_OR_ZERO(rgpio_dev->swnode); > > if (ret) > > goto err_put_of_node; > > Huh? Why? Hmm... Maybe we need a kernel-doc for fwnode_create_software_node() to be more explicit on what it might return (and no, NULL is not there, Bart is correct). -- With Best Regards, Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.