Re: [PATCH v3 3/4] arm64: dts: qcom: sm7225-fairphone-fp4: Add AW8695 haptics

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Input: aw8695: broken mode and work handling in aw8695_resume()
- [Medium] Input: aw8695: unnecessary motor stop/restart on magnitude change
--

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

arm64: dts: qcom: sm7225-fairphone-fp4: Add AW8695 haptics

This commit adds the device tree node and pinctrl configuration for the
Awinic AW8695 haptics controller on the Fairphone 4.

> diff --git a/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts b/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
> index 5d20ad93b0c54..19ffd652b0eb8 100644
> --- a/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
> +++ b/arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts
> @@ -707,7 +707,17 @@ vreg_l7p: ldo7 {
>  
>  	/* PX8618 @ 26 */
>  	/* SMB1395 PMIC @ 34 */
> -	/* awinic,aw8695 @ 5a */
> +
> +	haptics@5a {
> +		compatible = "awinic,aw8695";
> +		reg = <0x5a>;
> +
> +		interrupts-extended = <&tlmm 85 IRQ_TYPE_EDGE_FALLING>;
> +		reset-gpios = <&tlmm 90 GPIO_ACTIVE_LOW>;
> +
> +		pinctrl-0 = <&aw8695_int_default>, <&aw8695_reset_default>;
> +		pinctrl-names = "default";
> +	};

[Severity: Medium]
Enabling the haptics@5a node in
arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts binds the driver in
drivers/input/misc/aw8695.c. Could system suspend and resume cycles cause
unnecessary idle power drain and dropped haptic playback in
aw8695_suspend() and aw8695_resume()?

drivers/input/misc/aw8695.c:
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;
}

During normal initialization in aw8695_init() and whenever playback stops
in aw8695_stop(), aw8695_play_mode() places the hardware into standby mode
and masks under-voltage lockout interrupts. aw8695_haptic_set_active() is
otherwise only called when entering ram mode right before starting
playback in aw8695_play_sine(). However, aw8695_resume() unconditionally
calls aw8695_haptic_set_active(haptics), which switches the device out of
standby into active mode and unmasks under-voltage lockout interrupts even
when no effect is playing.

In addition, if the system suspends while an effect is playing or queued
(haptics->level != 0), aw8695_suspend() disables play_work and calls
aw8695_stop(haptics) without resetting haptics->level, and
aw8695_haptics_play() can also update haptics->level while play_work is
disabled. Because aw8695_resume() neither restarts playback nor resets
haptics->level to 0, a subsequent aw8695_haptics_play() call with the same
magnitude will hit:

	if (haptics->level == level)
		return 0;

and fail to start the motor.

Furthermore, aw8695_resume() calls enable_work(&haptics->play_work) before
the sleeping regmap transfers in aw8695_haptic_set_active(haptics) and
before clearing haptics->suspend. If play_work runs during that window,
aw8695_haptics_play_work() sees haptics->suspend still set:

	if (haptics->suspend)
		return;

and drops the playback request. Both aw8695_suspend() and aw8695_resume()
also ignore the error return values from aw8695_stop() and
aw8695_haptic_set_active(). Should aw8695_suspend() and aw8695_resume()
keep the device in standby when idle, order haptics->suspend updates
around play_work properly, and handle haptics->level and error returns?

[Severity: Medium]
When userspace updates the magnitude of a playing rumble effect on
haptics@5a (for example via periodic envelope updates in
ml_effect_timer() -> ml_play_effects() -> aw8695_haptics_play()), why does
aw8695_play_sine() in drivers/input/misc/aw8695.c stop and restart the
motor without applying the new magnitude?

drivers/input/misc/aw8695.c:
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;
}

aw8695_haptics_play() compares the full 16-bit magnitude against
haptics->level and schedules haptics->play_work on any change between two
non-zero levels. However, aw8695_haptics_play_work() only checks
haptics->level as a boolean:

	if (haptics->level)
		err = aw8695_play_sine(haptics);
	else
		err = aw8695_stop(haptics);

And aw8695_play_sine() never writes haptics->level to a hardware gain
register, while always beginning with aw8695_stop(haptics):

static int aw8695_play_sine(struct aw8695_data *haptics)
{
	int err;

	err = aw8695_stop(haptics);
	if (err)
		return err;

As a result, every change between two non-zero magnitudes halts the motor,
polls up to 200 ms for standby in aw8695_stop(), and restarts the waveform
at the same fixed amplitude. Should aw8695_play_sine() program the hardware
gain from haptics->level, or should aw8695_haptics_play() avoid restarting
the motor when haptics->level is only used as an on/off state?

>  };

[ ... ]

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