Re: [PATCH 1/2] Input: drv260x: Make vbat supply optional

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the resume callback attempts I2C writes before powering on the device and fails to reinitialize volatile registers after power loss. In `drv260x_resume()`, `regmap_update_bits()` is called to clear the standby bit over I2C before asserting the enable GPIO. Because the DRV260X entirely disables its I2C interface when the EN pin is low, the I2C write will invariably fail with `-EREMOTEIO`. This failure causes the resume function to abort prematurely without ever asserting the enable GPIO, leaving the device permanently unpowered and broken after a single suspend/resume cycle. Additionally, because power is cut during suspend, the device loses all volatile configurations (e.g., calibration, rated/overdrive voltages), which are never restored upon resume.
- [High] This problem wasn't introduced by this patch, but there is concurrent access and a lack of synchronization between the async force-feedback worker and device suspend paths. The force-feedback worker (`drv260x_worker`) executes asynchronously and does not hold a lock to serialize it against system suspend. During suspend, `drv260x_suspend` shuts down the device by asserting the EN pin low and disabling the vbat regulator. Because the worker is not canceled or flushed during suspend, it can execute while the device is powered down, driving the EN GPIO high and attempting I2C writes while the vbat supply is disabled. This can cause I2C timeouts, corrupt device state, and physically stress the chip by back-powering it via the GPIO.
--

commit 71f6122d604ec11f8df6957cd8212e9dd11f888a
Author: Maurizio Casciano <mauriziocasciano7@gmail.com>

Input: drv260x: Make vbat supply optional

This patch makes the vbat regulator lookup optional to support systems that
do not describe a software-controllable supply. Systems with a vbat supply
described will retain the existing enable, disable, and power management
behaviors.

> diff --git a/drivers/input/misc/drv260x.c b/drivers/input/misc/drv260x.c
> --- a/drivers/input/misc/drv260x.c
> +++ b/drivers/input/misc/drv260x.c

[ ... ]

> @@ -585,13 +589,15 @@ static int drv260x_suspend(struct device *dev)
>  
>  		gpiod_set_value(haptics->enable_gpio, 0);
>  
> -		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;
> +		if (haptics->regulator) {
> +			error = regulator_disable(haptics->regulator);

[Severity: High]
This is a pre-existing issue, but is there a lack of synchronization between
the async force-feedback worker and device suspend paths?

The force-feedback worker (drv260x_worker) executes asynchronously and does
not hold a lock to serialize it against system suspend. During suspend, the
device is shut down here by asserting the EN pin low and disabling the vbat
regulator.

Because the worker is not canceled or flushed during suspend, couldn't it
execute while the device is powered down, driving the EN GPIO high and
attempting I2C writes while the vbat supply is disabled? This might cause I2C
timeouts, corrupt device state, and physically stress the chip by
back-powering it via the GPIO.

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

[ ... ]

> @@ -606,10 +612,12 @@ static int drv260x_resume(struct device *dev)
>  	guard(mutex)(&haptics->input_dev->mutex);
>  
>  	if (input_device_enabled(haptics->input_dev)) {
> -		error = regulator_enable(haptics->regulator);
> -		if (error) {
> -			dev_err(dev, "Failed to enable regulator\n");
> -			return error;
> +		if (haptics->regulator) {
> +			error = regulator_enable(haptics->regulator);
> +			if (error) {
> +				dev_err(dev, "Failed to enable regulator\n");
> +				return error;
> +			}
>  		}
>  
>  		error = regmap_update_bits(haptics->regmap,
> @@ -617,7 +625,8 @@ static int drv260x_resume(struct device *dev)
>  					   DRV260X_STANDBY_MASK, 0);
>  		if (error) {
>  			dev_err(dev, "Failed to unset standby mode\n");
> -			regulator_disable(haptics->regulator);
> +			if (haptics->regulator)
> +				regulator_disable(haptics->regulator);
>  			return error;
>  		}
>  

[Severity: High]
This isn't a bug introduced by this patch, but does the resume callback
attempt I2C writes before powering on the device?

Here in drv260x_resume(), regmap_update_bits() is called to clear the standby
bit over I2C before asserting the enable GPIO. Because the DRV260X entirely
disables its I2C interface when the EN pin is low, won't this I2C write
invariably fail with -EREMOTEIO?

This failure would cause the resume function to abort prematurely without ever
asserting the enable GPIO, leaving the device permanently unpowered and broken
after a single suspend/resume cycle.

Additionally, because power is cut during suspend, does the device lose all
volatile configurations (e.g., calibration, rated/overdrive voltages) that
should be restored upon resume?

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