Re: [PATCH v2 2/3] Input - aw8695: Add driver for AW8695 haptics

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] input: aw8695: request IRQ before hardware is initialized
- [Medium] input: aw8695: race between haptic work and device suspend
--

commit e001ac586c0b070dafa130dff509a98526cd1941
Author: Luca Weiss <luca.weiss@fairphone.com>

Input - aw8695: Add driver for AW8695 haptics

Add a driver for interfacing with the Awinic AW8695 LRA Haptic Driver.
The chip supports multiple modes of which only RAM mode is implemented,
allowing playback of a basic sine wave.

> diff --git a/drivers/input/misc/aw8695.c b/drivers/input/misc/aw8695.c
> new file mode 100644
> index 0000000000000..507c3081f3b81
> --- /dev/null
> +++ b/drivers/input/misc/aw8695.c

[ ... ]

> +	err = devm_request_threaded_irq(dev, client->irq, NULL, aw8695_irq,
> +		IRQF_ONESHOT, NULL, haptics);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to request interrupt\n");
> +
> +	INIT_WORK(&haptics->play_work, aw8695_haptics_play_work);
> +
> +	haptics->input_dev->name = "aw8695";
> +	haptics->input_dev->close = aw8695_close;
> +
> +	input_set_drvdata(haptics->input_dev, haptics);
> +	input_set_capability(haptics->input_dev, EV_FF, FF_RUMBLE);
> +
> +	err = input_ff_create_memless(haptics->input_dev, NULL,
> +				      aw8695_haptics_play);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to create FF dev\n");
> +
> +	err = aw8695_init(haptics);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to init aw8695\n");

[Severity: Medium]
Could this request and enable the interrupt before the hardware is brought out
of reset and configured?

If the IRQ line is floating or asserted by the device before reset, the
interrupt could fire immediately. Since the hardware isn't initialized yet,
the unhandled I2C read failures in the interrupt handler might fail to clear
the interrupt, leading to an interrupt storm and the kernel disabling the IRQ
line.

Should this be moved after aw8695_init() and aw8695_ram_init() to ensure the
chip is prepared?

[ ... ]

> +static int aw8695_suspend(struct device *dev)
> +{
> +	struct aw8695_data *haptics = dev_get_drvdata(dev);
> +
> +	if (haptics->suspend)
> +		return 0;
> +
> +	aw8695_stop(haptics);
> +	haptics->suspend = true;
> +	return 0;
> +}

[Severity: Medium]
Could this leave a race condition window open where the background worker can
reactivate the hardware?

If a haptic effect schedules play_work, and the system suspends before the
work completes, they could race:

1. aw8695_haptics_play_work() reads haptics->suspend as false, but is
   preempted.
2. aw8695_suspend() executes, calls aw8695_stop(), sets
   haptics->suspend = true, and the underlying I2C adapter suspends.
3. The work thread resumes and calls aw8695_play_sine().

This would perform I2C writes on a suspended bus, potentially leading to bus
errors or timeouts.

Should this call cancel_work_sync(&haptics->play_work) before returning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-aw8695_haptic_fp4_driver-v2-0-1bd38b1943de@fairphone.com?part=2