Re: [PATCH v3 2/4] Input - aw8695: Add driver for AW8695 haptics

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Input: aw8695: aw8695_play_sine() never programs haptics->level into hardware
- [Medium] Input: aw8695: stale haptics->level after aw8695_stop() in suspend and close
- [Medium] Input: aw8695: broken PM state and work ordering in aw8695_resume()
--

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

Input - aw8695: Add driver for AW8695 haptics

This commit adds an input force-feedback driver for the Awinic AW8695
haptic controller using RAM mode playback of a sine wave.

> diff --git a/drivers/input/misc/aw8695.c b/drivers/input/misc/aw8695.c
> new file mode 100644
> index 0000000000000..45c68c673c26c
> --- /dev/null
> +++ b/drivers/input/misc/aw8695.c
> @@ -0,0 +1,805 @@

[ ... ]

> +static int aw8695_haptics_play(struct input_dev *dev, void *data,
> +			       struct ff_effect *effect)
> +{
> +	struct aw8695_data *haptics = input_get_drvdata(dev);
> +	int level;
> +
> +	level = effect->u.rumble.strong_magnitude;
> +	if (!level)
> +		level = effect->u.rumble.weak_magnitude;
> +
> +	if (haptics->level == level)
> +		return 0;
> +
> +	haptics->level = level;
> +	schedule_work(&haptics->play_work);
> +
> +	return 0;
> +}

[ ... ]

> +static int aw8695_play_sine(struct aw8695_data *haptics)
> +{
> +	int err;
> +
> +	err = aw8695_stop(haptics);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * Configure for waveform #1 to be played infinitely,
> +	 * and waveform #2 to not be played.
> +	 */
> +	err = regmap_write(haptics->regmap, AW8695_WAVSEQ1_REG, 0x1);
> +	if (err)
> +		return err;
> +
> +	err = regmap_write(haptics->regmap, AW8695_WAVSEQ2_REG, 0x0);
> +	if (err)
> +		return err;
> +
> +	err = regmap_write(haptics->regmap, AW8695_WAVLOOP1_REG,
> +			   FIELD_PREP(AW8695_WAVLOOP1_SEQ1_MASK,
> +				      AW8695_WAVLOOP_INFINITE) |
> +			   FIELD_PREP(AW8695_WAVLOOP1_SEQ2_MASK, 0));
> +	if (err)
> +		return err;
> +
> +	/* Configure for RAM mode */
> +	err = aw8695_play_mode(haptics, AW8695_RAM_MODE);
> +	if (err)
> +		return err;
> +
> +	/* Start vibration */
> +	err = regmap_update_bits(haptics->regmap, AW8695_GO_REG,
> +				 AW8695_GO_ENABLE, AW8695_GO_ENABLE);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static void aw8695_close(struct input_dev *input)
> +{
> +	struct aw8695_data *haptics = input_get_drvdata(input);
> +	struct device *dev = &haptics->client->dev;
> +	int err;
> +
> +	cancel_work_sync(&haptics->play_work);
> +	err = aw8695_stop(haptics);
> +	if (err)
> +		dev_err(dev, "Failed to stop haptics: %d\n", err);
> +}

[Severity: Medium]
When aw8695_close() or aw8695_suspend() stops the motor via aw8695_stop(),
haptics->level is not reset to 0. Similarly, if aw8695_haptics_play() is
called while play_work is disabled during system suspend, haptics->level is
updated even though schedule_work(&haptics->play_work) does not queue the
work and aw8695_resume() does not restart playback.

If a subsequent effect is played with the same magnitude after the device is
reopened or resumed, won't aw8695_haptics_play() see haptics->level == level
and return early without starting vibration?

> +
> +static void aw8695_haptics_play_work(struct work_struct *work)
> +{
> +	struct aw8695_data *haptics =
> +		container_of(work, struct aw8695_data, play_work);
> +	struct device *dev = &haptics->client->dev;
> +	int err;
> +
> +	if (haptics->suspend)
> +		return;
> +
> +	if (haptics->level)
> +		err = aw8695_play_sine(haptics);
> +	else
> +		err = aw8695_stop(haptics);
> +
> +	if (err)
> +		dev_err(dev, "Failed to execute work command: %d\n", err);
> +}

[Severity: Medium]
In aw8695_haptics_play(), the 16-bit rumble magnitude is stored in
haptics->level and play_work is scheduled whenever that value changes.
However, aw8695_haptics_play_work() only checks whether haptics->level is
non-zero, and aw8695_play_sine() never writes a scaled gain value to the
hardware.

As a result, all effects play at a fixed amplitude, and any change between
two non-zero magnitudes (for example, during an ff-memless periodic envelope
fade in ml_effect_timer() or a gain update) causes aw8695_play_sine() to call
aw8695_stop(), poll AW8695_GLB_STATE_REG for standby, and restart the same
fixed-amplitude waveform from the beginning.

Should aw8695_play_sine() program a hardware gain register based on
haptics->level?

[ ... ]

> +static int aw8695_suspend(struct device *dev)
> +{
> +	struct aw8695_data *haptics = dev_get_drvdata(dev);
> +
> +	if (haptics->suspend)
> +		return 0;
> +
> +	disable_work_sync(&haptics->play_work);
> +	aw8695_stop(haptics);
> +	haptics->suspend = true;
> +	return 0;
> +}
> +
> +static int aw8695_resume(struct device *dev)
> +{
> +	struct aw8695_data *haptics = dev_get_drvdata(dev);
> +
> +	if (!haptics->suspend)
> +		return 0;
> +
> +	enable_work(&haptics->play_work);
> +	aw8695_haptic_set_active(haptics);
> +	haptics->suspend = false;
> +	return 0;
> +}

[Severity: Medium]
There seem to be a few issues in aw8695_suspend() and aw8695_resume():

First, during normal operation aw8695_init() and aw8695_stop() leave the
device in AW8695_STANDBY_MODE when idle, only entering AW8695_SYSCTRL_ACTIVE
via aw8695_play_mode() when aw8695_play_sine() starts playback. However,
aw8695_resume() unconditionally calls aw8695_haptic_set_active(), which puts
the chip into active mode and clears AW8695_SYSINTM_UVLO_OFF even when no
effect is playing.

Second, aw8695_resume() calls enable_work(&haptics->play_work) before the
blocking regmap accesses in aw8695_haptic_set_active() and before clearing
haptics->suspend. If play_work is scheduled and runs during that window,
aw8695_haptics_play_work() will see haptics->suspend set to true and return
early, dropping the playback request.

Third, both aw8695_suspend() and aw8695_resume() ignore the error values
returned by aw8695_stop() and aw8695_haptic_set_active().

Could aw8695_resume() avoid forcing the chip active when idle, clear
haptics->suspend before calling enable_work(), and propagate errors from the
hardware helper calls?

[ ... ]

> +}
> +
> +module_i2c_driver(aw8695_driver);
> +
> +MODULE_AUTHOR("Luca Weiss <luca.weiss@fairphone.com>");
> +MODULE_DESCRIPTION("AW8695 LRA Haptic Driver");
> +MODULE_LICENSE("GPL");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-aw8695_haptic_fp4_driver-v3-0-ac8c14377e82@fairphone.com?part=2