Re: [PATCH 3/7] Input: s6sy761 - add reset GPIO support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] input: Unbalanced devm cleanup calling disable_irq() and regulator_bulk_disable()
- [Medium] input: Regulator and GPIO state leak on resume failure
- [Low] input: Unconditional 10ms delay for optional reset GPIO
--

commit 4cab9258eec8d868d79c658c070f807a7b520161
Author: Lukas Timmermann <foss@timmermann.space>

Input: s6sy761 - add reset GPIO support

This commit introduces support for an optional reset GPIO to the s6sy761 driver.
It asserts the reset line on power down and deasserts it 10 ms after enabling
supplies during power on. This ensures the controller is held in reset while
supplies are coming up, mirroring the vendor driver's behavior.

> diff --git a/drivers/input/touchscreen/s6sy761_core.c b/drivers/input/touchscreen/s6sy761_core.c
> index 0e455060dbc0b..8541cd9aa930b 100644
> --- a/drivers/input/touchscreen/s6sy761_core.c
> +++ b/drivers/input/touchscreen/s6sy761_core.c

[ ... ]

> @@ -297,6 +299,9 @@ static int s6sy761_power_on(struct s6sy761_data *sdata)
>  	if (ret)
>  		return ret;
>  
> +	usleep_range(10000, 11000);
> +	gpiod_set_value_cansleep(sdata->reset_gpio, 0);

[Severity: Low]
Could this delay be skipped if reset_gpio is NULL?

The commit message states "The GPIO is optional, so boards without it are
unaffected", but the 10ms sleep appears to execute regardless of whether
reset_gpio is valid or NULL.

Would this unnecessarily slow down probe and system resume on boards without
the GPIO?

[Severity: Medium]
Will an early return later in this function fail to clean up the regulators
and the newly added reset GPIO?

If the hardware stops responding during the I2C boot checks in system resume,
s6sy761_power_on() returns an error here:

    /* double check whether the touch is functional */
    ret = s6sy761_read(sdata, S6SY761_READ_ONE_EVENT,
                       S6SY761_EVENT_SIZE, buffer);
    if (ret < 0)
        return ret;

Since devres does not run on resume failures, this error propagates to the PM
core without cleanup. Could this permanently leak the regulator enable count
and leave the GPIO deasserted?

> +
>  	msleep(140);
>  
>  	/* double check whether the touch is functional */

[ ... ]

> @@ -370,6 +375,7 @@ static void s6sy761_power_off(void *data)
>  	struct s6sy761_data *sdata = data;
>  
>  	disable_irq(sdata->irq);
> +	gpiod_set_value_cansleep(sdata->reset_gpio, 1);
>  	regulator_bulk_disable(ARRAY_SIZE(sdata->regulators),
>  						sdata->regulators);
>  }

[ ... ]

> @@ -406,6 +412,12 @@ int s6sy761_probe(struct device *dev, const struct s6sy761_bus_ops *bus_ops, int
>  	if (err)
>  		return err;
>  
> +	sdata->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +						    GPIOD_OUT_HIGH);
> +	if (IS_ERR(sdata->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(sdata->reset_gpio),
> +				     "failed to get reset GPIO\n");
> +
>  	err = devm_add_action_or_reset(dev, s6sy761_power_off, sdata);

[Severity: High]
Does this queue the devm action before the regulators are enabled and before
the IRQ is requested?

If probe fails early, devres unwinds and executes s6sy761_power_off(). Also,
on a successful probe, devres will execute in LIFO order during unbind, first
freeing the IRQ via devm_free_irq(), and then executing s6sy761_power_off().

In both cases, this would blindly call disable_irq() on an unrequested or
already-freed IRQ, and call regulator_bulk_disable() on unenabled regulators.

Could this corrupt the IRQ disable depth and trigger regulator framework
underflow warnings?

>  	if (err)
>  		return err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011-lat3st-google-raven-touch-upstream-v1-0-2a9f55159b9e@timmermann.space?part=3