[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <e6a120d4ada6d032f69812f14a7e794ac1796a85.camel@pengutronix.de>
Date: Mon, 06 Oct 2025 17:55:05 +0200
From: Philipp Zabel <p.zabel@...gutronix.de>
To: Bartosz Golaszewski <brgl@...ev.pl>, Linus Walleij
<linus.walleij@...aro.org>, Andy Shevchenko
<andriy.shevchenko@...ux.intel.com>, Daniel Scally <djrscally@...il.com>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>, Sakari Ailus
<sakari.ailus@...ux.intel.com>, Greg Kroah-Hartman
<gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
Danilo Krummrich <dakr@...nel.org>, Krzysztof Kozlowski <krzk@...nel.org>
Cc: linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-acpi@...r.kernel.org, Bartosz Golaszewski
<bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH 9/9] reset: gpio: use software nodes to setup the GPIO
lookup
On Mo, 2025-10-06 at 15:00 +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.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@...aro.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
Powered by blists - more mailing lists