Skip to contentSkip to conversation

Input: drv260x - switch to using fsleep()

PATCH In review Dmitry Torokhov 3 participants 3 messages Aug 29 – Aug 31

Subsystems (2)
HID Input drivers
lore
Share & tools
lore

Dmitry Torokhov

author maintainer
#0 lore raw

PATCH 1/1 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.

Assisted-by LLM
Signed-off-by Dmitry Torokhov <[email protected]>
---
 drivers/input/misc/drv260x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/input/misc/drv260x.c +1 -1
index 6c5c4c53753b..477ec84ea09b 100644
@@ -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);
Signature · 5 lines
2.55.0.897.gb25b4bd76c-goog


-- 
Dmitry
AI review
#1 lore raw
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

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 <[email protected]>

    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.

drivers/input/misc/drv260x.c static void drv260x_worker(struct work_struct *work) · line 225

Quoted 10 lines
drivers/input/misc/drv260x.c +1 -1
index 6c5c4c53753b1..477ec84ea09b8 100644
@@ -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?

drivers/input/misc/drv260x.c static void drv260x_worker(struct work_struct *work) · line 228

Quoted 3 lines
drivers/input/misc/drv260x.c +0 -0
 	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?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Tested-by on v1 1/1
#2 lore raw
In reply to Dmitry Torokhov

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 <[email protected]>

With Best Regards,
Maurizio Casciano