Re: [PATCH 2/2] Input: drv260x: Map ACPI enable GPIO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] I2C writes in `drv260x_resume()` and error paths of `drv260x_suspend()` will fail because the enable GPIO is not asserted.
--

commit 3813e86cb3a701e0633edd349e11dd4b241aa914
Author: Maurizio Casciano <mauriziocasciano7@gmail.com>

Input: drv260x: Map ACPI enable GPIO
    
Some ACPI DRV2604 devices describe the enable line as the first GPIO
resource in _CRS but provide no _DSD mapping for its function. Add a managed
ACPI GPIO mapping before requesting the optional enable line so these devices
can power their haptic path.

> diff --git a/drivers/input/misc/drv260x.c b/drivers/input/misc/drv260x.c
> index c4fbb10b254e9..b6243ce8157d6 100644
> --- a/drivers/input/misc/drv260x.c
> +++ b/drivers/input/misc/drv260x.c

[ ... ]

> @@ -448,6 +455,14 @@ static int drv260x_probe(struct i2c_client *client)
>  	u32 voltage;
>  	int error;
>  
> +	if (has_acpi_companion(dev)) {
> +		error = devm_acpi_dev_add_driver_gpios(dev,
> +						       drv260x_acpi_gpios);
> +		if (error)
> +			return dev_err_probe(dev, error,
> +					     "Failed to add ACPI GPIO mapping\n");
> +	}
> +
>  	haptics = devm_kzalloc(dev, sizeof(*haptics), GFP_KERNEL);
>  	if (!haptics)
>  		return -ENOMEM;

[Severity: High]
This isn't a bug introduced by this patch, but it appears there is a
pre-existing issue in the suspend and resume paths related to the enable GPIO.

Looking at drv260x_resume():

drivers/input/misc/drv260x.c:drv260x_resume() {
...
		error = regmap_update_bits(haptics->regmap,
					   DRV260X_MODE,
					   DRV260X_STANDBY_MASK, 0);
		if (error) {
			dev_err(dev, "Failed to unset standby mode\n");
			if (haptics->regulator)
				regulator_disable(haptics->regulator);
			return error;
		}

		gpiod_set_value(haptics->enable_gpio, 1);
...
}

Will this I2C transaction fail because it occurs while the chip is still held
in reset/shutdown by the GPIO? Since the enable GPIO is not asserted until
after the I2C write, and the driver aborts the resume on error, does this
leave the chip permanently disabled?

Additionally, because pulling the enable GPIO low resets all chip registers and
REGCACHE_NONE is used, is the device configuration (calibration, over-drive
voltage, library selection) lost and never restored during resume?

A similar issue appears to exist in the error path of drv260x_suspend():

drivers/input/misc/drv260x.c:drv260x_suspend() {
...
		gpiod_set_value(haptics->enable_gpio, 0);

		if (haptics->regulator) {
			error = regulator_disable(haptics->regulator);
			if (error) {
				dev_err(dev, "Failed to disable regulator\n");
				regmap_update_bits(haptics->regmap,
						   DRV260X_MODE,
						   DRV260X_STANDBY_MASK, 0);
				return error;
			}
		}
...
}

Does the error path attempt to communicate with the chip via I2C without
restoring the enable GPIO to 1?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826132211.3341936-1-mauriziocasciano7@gmail.com?part=2