Re: [PATCH 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] The haptic motor will vibrate indefinitely if the system enters suspend while an effect is playing, due to missing PM callbacks.
- [Low] Returning an error-producing action directly instead of using explicit failure paths in functions with multiple failure points.
--

commit aaa9fe6e99ccfee800cc251fa21a6409a47045f8
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.

> diff --git a/drivers/input/misc/aw8695.c b/drivers/input/misc/aw8695.c
> index 0000000000000..ea72562a7c2a7
> --- /dev/null
> +++ b/drivers/input/misc/aw8695.c
> @@ -0,0 +1,753 @@

[ ... ]

> +static int aw8695_stop(struct aw8695_data *haptics)
> +{
> +	int err;
> +	unsigned int read_buf;
> +	struct device *dev = &haptics->client->dev;
> +
> +	err = regmap_update_bits(haptics->regmap, AW8695_GO_REG,
> +				 AW8695_GO_ENABLE, 0);
> +	if (err)
> +		return err;
> +
> +	err = regmap_read_poll_timeout(haptics->regmap, AW8695_GLB_STATE_REG, read_buf,
> +			(read_buf & 0x0f) == 0, 2000, 2000 * 100);
> +	if (err) {
> +		dev_err(dev, "Did not enter standby: %d\n	Trying to force it...\n", err);
> +		err = aw8695_play_mode(haptics, AW8695_STANDBY_MODE);
> +		return err;
> +	}
> +
> +	return aw8695_play_mode(haptics, AW8695_STANDBY_MODE);

[Severity: Low]
This isn't a bug, but can we use explicit error handling here instead of
directly returning the aw8695_play_mode() error code? The input subsystem
prefers checking the error and returning 0 explicitly in functions with
multiple failure points.

> +}
> +
> +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));

[Severity: Medium]
Since this configures the hardware to play the waveform infinitely, it relies
entirely on the software timer to stop the haptic effect. If the system enters
suspend while an effect is playing, will the haptic motor vibrate indefinitely
until the system wakes up and the timer resumes?

> +	if (err)
> +		return err;
> +
> +	/* Configure for RAM mode */
> +	err = aw8695_play_mode(haptics, AW8695_RAM_MODE);
> +	if (err)
> +		return err;
> +
> +	/* Start vibration */
> +	return regmap_update_bits(haptics->regmap, AW8695_GO_REG,
> +				  AW8695_GO_ENABLE, AW8695_GO_ENABLE);

[Severity: Low]
This isn't a bug, but could this be converted to an explicit error check and
return 0 to match the input subsystem's preferred coding style?

[ ... ]

> +static int aw8695_init(struct aw8695_data *haptics)
> +{
> +	int err;
> +	unsigned int read_buf;
> +	struct device *dev = &haptics->client->dev;

[ ... ]

> +	/* bemf config */
> +	err = regmap_write(haptics->regmap, AW8695_BEMF_VTHH_H_REG,
> +			   FIELD_GET(AW8695_HIGH_MASK, AW8695_BEMF_UPPER_THRESHOLD));
> +	if (err)
> +		return err;
> +	err = regmap_write(haptics->regmap, AW8695_BEMF_VTHH_L_REG,
> +			   FIELD_GET(AW8695_LOW_MASK, AW8695_BEMF_UPPER_THRESHOLD));
> +	if (err)
> +		return err;
> +	err = regmap_write(haptics->regmap, AW8695_BEMF_VTHL_H_REG,
> +			   FIELD_GET(AW8695_HIGH_MASK, AW8695_BEMF_LOWER_THRESHOLD));
> +	if (err)
> +		return err;
> +	return regmap_write(haptics->regmap, AW8695_BEMF_VTHL_L_REG,
> +			    FIELD_GET(AW8695_LOW_MASK, AW8695_BEMF_LOWER_THRESHOLD));

[Severity: Low]
This isn't a bug, but should this return be expanded into an explicit error
check returning 0?

> +}
> +
> +static int aw8695_ram_init(struct aw8695_data *haptics)
> +{
> +	int err;

[ ... ]

> +	err = regmap_noinc_write(haptics->regmap, AW8695_RAMDATA_REG,
> +				  aw8695_sine_waveform, ARRAY_SIZE(aw8695_sine_waveform));
> +	if (err)
> +		return err;
> +
> +	/* Disable SRAM init */
> +	return regmap_update_bits(haptics->regmap, AW8695_SYSCTRL_REG,
> +				  AW8695_SYSCTRL_RAMINIT_EN, 0);

[Severity: Low]
This isn't a bug, but should this have an explicit error check instead of
directly returning the regmap_update_bits() result?

[ ... ]

> +static struct i2c_driver aw8695_driver = {
> +	.driver = {
> +		.name = "aw8695",
> +		.of_match_table = aw8695_of_id,
> +	},
> +	.probe = aw8695_probe,
> +};

[Severity: Medium]
Should this driver populate pm callbacks in the driver struct? If the system
enters suspend while playing an infinitely looping waveform, the software
timer will freeze, potentially leaving the motor running indefinitely. A
suspend handler might be needed to safely stop the hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914-aw8695_haptic_fp4_driver-v1-0-156f7549149a@fairphone.com?part=2