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

[Philipp Zabel <p.zabel@pengutronix.de>]

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