Re: [RFC PATCH] reset: always bail out on missing RESET_GPIO driver

[Philipp Zabel]

Hi Wolfram,

On Mi, 2025-10-15 at 13:28 +0200, Wolfram Sang wrote:
> Optional GPIOs mean they can be omitted. If they are described, a
> failure in acquiring them still needs to be reported. When the
> RESET_GPIO is not enabled so the reset core cannot provide its assumed
> fallback, the user should be informed about it. So, not only bail out
> but also give a hint how to fix the situation.
> 
> Reported-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> Closes: https://lore.kernel.org/r/87a51um1y1.wl-kuninori.morimoto.gx@xxxxxxxxxxx
> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> ---
> 
> This happened because of this (in general) nice cleanup patch for the
> pca954x driver (690de2902dca ("i2c: muxes: pca954x: Use reset controller
> only")). Our .config didn't have the RESET_GPIO enabled before, so sound
> regressed on some boards.

Ouf, I should have noticed and asked if RESET_GPIO is enabled on all
affected platforms when that patch was proposed.

> Actually, my preferred solution would be to make the reset-gpio driver
> 'obj-y' but I guess its dependency on GPIOLIB makes this a no-go?

I think so, yes. Also it's only needed in (currently) a very small
number of cases.

> On the other hand, the fallback is a really nice feature which could
> remove duplicated code. But if the fallback is not present by default,
> it makes it cumbersome to use IMO.

And it's not easy to automatically determine whether RESET_GPIO is
actually required, because that depends on both device tree and
individual drivers.

> Has this been discussed before? I couldn't find any pointers...

I don't remember this being discussed before.

> Happy hacking, everyone!
> 
> 
>  drivers/reset/core.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 22f67fc77ae5..8a0f41963f6b 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -1028,8 +1028,10 @@ __of_reset_control_get(struct device_node *node, const char *id, int index,
>  	if (ret == -EINVAL)
>  		return ERR_PTR(ret);
>  	if (ret) {
> -		if (!IS_ENABLED(CONFIG_RESET_GPIO))
> -			return optional ? NULL : ERR_PTR(ret);
> +		if (!IS_ENABLED(CONFIG_RESET_GPIO)) {
> +			pr_warn("%s(): RESET_GPIO driver not enabled, cannot fall back\n", __func__);
> +			return ERR_PTR(ret);
> +		}
>  
>  		/*
>  		 * There can be only one reset-gpio for regular devices, so

The reset-gpios phandle check should be done first, then. The warning
only makes sense if that property exist, and returning -ENOENT for an
optional reset is wrong if neither phandle property exists in the DT.

I think putting the IS_ENABLED check first was intended to save an
unnecessary "reset-gpios" phandle lookup on kernels with
CONFIG_RESET_GPIO=n.

In short, if both of_parse_phandle_with_args() return -ENOENT, we
should continue to silently return (optional ? NULL : -ENOENT), even if
CONFIG_RESET_GPIO=n.

I think the message should be pr_err() level if we return an error that
will cause the consumer driver probe to fail.

regards
Philipp