Re: [PATCH] mmc: pwrseq_simple: fix probe failure when CONFIG_RESET_GPIO is enabled

[Bartosz Golaszewski]

On Thu, 5 Mar 2026 10:43:54 +0100, Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> said:
> On Mi, 2026-03-04 at 08:41 -0800, Bartosz Golaszewski wrote:
>> On Fri, 20 Feb 2026 20:47:25 +0100, Sergey Suloev
>> <sergey.suloev@xxxxxxxxx> said:
>> > The driver uses a heuristic — checking if exactly one entry exists in
>> > 'reset-gpios' — to decide whether to attempt reset controller API usage
>> > via devm_reset_control_get_optional_shared(). This is semantically
>> > incorrect and causes a permanent probe failure when CONFIG_RESET_GPIO
>> > is enabled.
>> >
>> > When CONFIG_RESET_GPIO=y, __of_reset_control_get() intercepts the
>> > 'reset-gpios' DT property and attempts to register the GPIO as a reset
>> > controller via __reset_add_reset_gpio_device(). If the GPIO controller
>> > is not yet fully initialized at probe time, this returns -ENOENT instead
>> > of -EPROBE_DEFER, and the error path does not respect the 'optional'
>>
>> If this is what's happening, then it is not the expected behavior[1].
>
> Is this -ENOENT returned because of #gpio-cells != 2 [1]?
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/core.c#n883
>
> It looks like it [2].
>
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/allwinner/sunxi-bananapi-m2-plus.dtsi#n103
>
>> > flag, causing pwrseq_simple to fail permanently rather than defer.
>> > This prevents mmc1 from probing on boards that use 'reset-gpios' for
>> > WiFi power sequencing, such as BananaPi M2 Magic with AP6212/BCM43430
>> > on Allwinner R16.
>> >
>> > The correct indicator of reset controller intent is the explicit presence
>> > of a 'resets' property in the device tree node, not the count of
>> > 'reset-gpios' entries. A single 'reset-gpios' entry is just as likely
>> > to be a plain GPIO as multiple entries.
>> >
>>
>> Am I getting it right? You have a node that has both the "resets" property
>> as well as "reset-gpios" with more than one entry?
>
> According to the mmc-pwrseq-simple binding doc, a "resets" property is
> not valid at all. Apparently, this is just a "reset-gpios" property
> with a single GPIO with three cells.
>
>> If that's the case then it should be fixed in reset core, not in a user
>> because there'll surely be another one in no time.
>>
>> > Replace the ngpio == 1 heuristic with an explicit check for the 'resets'
>> > DT property, making the behavior unambiguous and immune to
>> > CONFIG_RESET_GPIO interference.
>> >
>> > Fixes: 73bf4b7381f7 ("mmc: pwrseq_simple: add support for one reset control")
>> > Signed-off-by: Sergey Suloev <sergey.suloev@xxxxxxxxx>
>> > ---
>> >  drivers/mmc/core/pwrseq_simple.c | 4 +---
>> >  1 file changed, 1 insertion(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
>> > index 4b47e6c3b04b..780c8818a273 100644
>> > --- a/drivers/mmc/core/pwrseq_simple.c
>> > +++ b/drivers/mmc/core/pwrseq_simple.c
>> > @@ -124,7 +124,6 @@ static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
>> >  {
>> >  	struct mmc_pwrseq_simple *pwrseq;
>> >  	struct device *dev = &pdev->dev;
>> > -	int ngpio;
>> >
>> >  	pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
>> >  	if (!pwrseq)
>> > @@ -134,8 +133,7 @@ static int mmc_pwrseq_simple_probe(struct platform_device *pdev)
>> >  	if (IS_ERR(pwrseq->ext_clk) && PTR_ERR(pwrseq->ext_clk) != -ENOENT)
>> >  		return dev_err_probe(dev, PTR_ERR(pwrseq->ext_clk), "external clock not ready\n");
>> >
>> > -	ngpio = of_count_phandle_with_args(dev->of_node, "reset-gpios", "#gpio-cells");
>> > -	if (ngpio == 1) {
>> > +	if (device_property_present(dev, "resets")) {
>
> This effectively turns the following into dead code because the binding
> does not allow a "resets" property.
>
>> >  		pwrseq->reset_ctrl = devm_reset_control_get_optional_shared(dev, NULL);
>> >  		if (IS_ERR(pwrseq->reset_ctrl))
>> >  			return dev_err_probe(dev, PTR_ERR(pwrseq->reset_ctrl),
>> > --
>> > 2.43.0
>> >
>> >
>> >
>>
>> I think reset core should check if the client node has both "reset" and
>> "reset-gpios".
>
> Why check both? If the "resets" property exists, we use it [3].
> "reset-gpios" is only checked as a fallback if the "resets" property is
> not present [4].
>
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/core.c#n1019
> [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/core.c#n1031
>
>> If so - it should assume the client wants to resolve "resets"
>> and only fall-back to "reset-gpios" if there's no "resets" and number of GPIOs
>> is 1.
>
> That is exactly what happens right now.
>

Right.

>> Philipp, Krzysztof: what do you think?
>
> I think this is a missing feature in reset-gpio.
> This driver could fall back to to devm_gpiod_get_array() also if
> devm_reset_control_get_optional_shared() returns -ENOENT instead of
> erroring out (maybe -ENOTSUPP would be a better return code for [1]?).
>

I did not write the initial version but Krzysztof: is there any reason we only
support a single, two-cell GPIO? Would it be problematic to support any kind of
reset GPIO setup?

Bartosz