[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <95bbec130437846d4b902ce4161ccf0f33c26c59.camel@pengutronix.de>
Date: Mon, 06 Oct 2025 17:19:43 +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 7/9] reset: make the provider of reset-gpios the parent
of the reset device
Hi Bartosz,
On Mo, 2025-10-06 at 15:00 +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
>
> Auxiliary devices really do need a parent so ahead of converting the
> reset-gpios driver to registering on the auxiliary bus, make the GPIO
> device that provides the reset GPIO the parent of the reset-gpio device.
> To that end move the lookup of the GPIO device by fwnode to the
> beginning of __reset_add_reset_gpio_device() which has the added benefor
Typo: benefit.
> of bailing out earlier, before allocating resources for the virtual
> device, if the chip is not up yet.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> ---
> drivers/reset/core.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 5a696e2dbcc224a633e2b321da53b7bc699cb5f3..ad85ddc8dd9fcf8b512cb09168586e0afca257f1 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -849,11 +849,11 @@ static void __reset_control_put_internal(struct reset_control *rstc)
> kref_put(&rstc->refcnt, __reset_control_release);
> }
>
> -static int __reset_add_reset_gpio_lookup(int id, struct device_node *np,
> +static int __reset_add_reset_gpio_lookup(struct gpio_device *gdev, int id,
> + struct device_node *np,
> unsigned int gpio,
> unsigned int of_flags)
> {
> - const struct fwnode_handle *fwnode = of_fwnode_handle(np);
> unsigned int lookup_flags;
> const char *label_tmp;
>
> @@ -868,10 +868,6 @@ static int __reset_add_reset_gpio_lookup(int id, struct device_node *np,
> return -EINVAL;
> }
>
> - struct gpio_device *gdev __free(gpio_device_put) = gpio_device_find_by_fwnode(fwnode);
> - if (!gdev)
> - return -EPROBE_DEFER;
> -
> label_tmp = gpio_device_get_label(gdev);
This is the only remaining use of gdev in
__reset_add_reset_gpio_lookup().
It would make sense to move this as well and only pass the label.
Given that all this is removed in patch 9, this is not super important.
> if (!label_tmp)
> return -EINVAL;
> @@ -919,6 +915,11 @@ static int __reset_add_reset_gpio_device(const struct of_phandle_args *args)
> if (args->args_count != 2)
> return -ENOENT;
>
> + struct gpio_device *gdev __free(gpio_device_put) =
> + gpio_device_find_by_fwnode(of_fwnode_handle(args->np));
We are mixing cleanup helpers with gotos in this function, which the
documentation in cleanup.h explicitly advises against.
I know the current code is already guilty, but could you take this
opportunity to prepend a patch that splits the part under guard() into
a separate function?
I'd also move this block after the lockdep_assert_not_held() below.
regards
Philipp
Powered by blists - more mailing lists