[PATCH] Input: drv260x - switch to using fsleep()

[PATCH] Input: drv260x - switch to using fsleep()

[Dmitry Torokhov]

Switch from udelay() to fsleep() in drv260x_worker() as it runs in a
sleepable workqueue context where busy-waiting is unnecessary.

Assisted-by: LLM
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/drv260x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/misc/drv260x.c b/drivers/input/misc/drv260x.c
index 6c5c4c53753b..477ec84ea09b 100644
--- a/drivers/input/misc/drv260x.c
+++ b/drivers/input/misc/drv260x.c
@@ -222,7 +222,7 @@ static void drv260x_worker(struct work_struct *work)
 
 	gpiod_set_value(haptics->enable_gpio, 1);
 	/* Data sheet says to wait 250us before trying to communicate */
-	udelay(250);
+	fsleep(250);
 
 	error = regmap_write(haptics->regmap,
 			     DRV260X_MODE, DRV260X_RT_PLAYBACK);
-- 
2.55.0.897.gb25b4bd76c-goog


-- 
Dmitry

Re: [PATCH] Input: drv260x - switch to using fsleep()

[sashiko-bot]

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

Pre-existing issues:
- [High] I2C communication failure in drv260x_resume due to incorrect GPIO sequence.
- [High] Lack of synchronization between haptic workqueue and system suspend leads to I2C transfers to a powered-off device.
- [Medium] Incorrect timeout handling in drv260x_init loop may cause spurious calibration failures.
--

commit 00b4c6fa5b7a9a1b697cc946bec3e6c6ae2eff9c
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

    Input: drv260x - switch to using fsleep()
    
    Switch from udelay() to fsleep() in drv260x_worker() as it runs in a
    sleepable workqueue context where busy-waiting is unnecessary.

> diff --git a/drivers/input/misc/drv260x.c b/drivers/input/misc/drv260x.c
> index 6c5c4c53753b1..477ec84ea09b8 100644
> --- a/drivers/input/misc/drv260x.c
> +++ b/drivers/input/misc/drv260x.c
> @@ -222,7 +222,7 @@ static void drv260x_worker(struct work_struct *work)
>  
>  	gpiod_set_value(haptics->enable_gpio, 1);
>  	/* Data sheet says to wait 250us before trying to communicate */
> -	udelay(250);
> +	fsleep(250);

[Severity: High]
This is a pre-existing issue, but does switching to fsleep() widen the race
window between drv260x_worker() and system suspend?

If a haptic effect is playing during system suspend, drv260x_suspend() disables
the regulator without canceling haptics->work:

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

	error = regulator_disable(haptics->regulator);
    ...
}

Since fsleep() voluntarily yields the CPU, could this allow the suspend thread
to run and power down the device, resulting in regmap_write() attempting I2C
transfers to a powered-off chip when the worker resumes?

>  
>  	error = regmap_write(haptics->regmap,
>  			     DRV260X_MODE, DRV260X_RT_PLAYBACK);

[Severity: High]
This isn't a bug introduced by this patch, but can I2C communication fail in
drv260x_resume() due to the GPIO enable sequence?

In drv260x_resume(), regmap_update_bits() is called before setting the
enable_gpio pin high:

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");
			regulator_disable(haptics->regulator);
			return error;
		}

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

Since the hardware disables the I2C interface when the EN pin is low, won't
this transaction NACK and cause the resume sequence to abort?

[Severity: Medium]
This isn't a bug introduced by this patch, but could the timeout handling in
drv260x_init() cause spurious calibration failures?

In drv260x_init(), the loop checks for a timeout unconditionally after reading
the register:

drivers/input/misc/drv260x.c:drv260x_init() {
    ...
		if (time_after(jiffies, timeout)) {
			dev_err(&haptics->client->dev,
				"Calibration timeout. The device cannot be used.\n");
			return -ETIMEDOUT;
		}
	} while (cal_buf == DRV260X_GO_BIT);
    ...
}

If the timeout expires on the exact same iteration that the calibration
successfully completes (the GO bit is cleared), will this erroneously return
-ETIMEDOUT instead of successfully exiting the loop?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/apLOXaWFZ8ueIEAm@google.com?part=1

Re: [PATCH] Input: drv260x - switch to using fsleep()

[Maurizio Casciano]

Hi Dmitry,

I tested this patch unchanged on a Lenovo Yoga Book YB1-X91L with
both onboard TI DRV2604 devices (ACPI DRV2604:00 and DRV2604:01).

The test kernel was built from commit
e5e85128bb7ce1bda7cfd7055be9821ac42b8108 and reported
7.2.0-yogabook-20260831-202310. The validation covered:

- 20 open/upload/close cycles on each device;
- strong and weak FF_RUMBLE effects on both devices;
- three s2idle suspend/resume cycles;
- repeated FF_RUMBLE playback on both devices after every resume.

All operations completed successfully, and the kernel log contained no
drv260x, I2C, regulator, or workqueue errors related to these devices.

Tested-by: Maurizio Casciano <mauriziocasciano7@gmail.com>

With Best Regards,
Maurizio Casciano