Re: [PATCH 2/2] Input: aw8697 - Add Awinic AW8697 haptics driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] input: aw8697: race condition during suspend can issue I2C transactions on suspended bus
- [Low] input: aw8697: coding style violation using return action() instead of returning 0
- [Low] input: aw8697: NULL devname passed to devm_request_threaded_irq() degrades /proc/interrupts
--

commit dd92c3df60822c4f873b6120f364c5b675f8a5a7
Author: Armandas Kvietkus <armandas.kvietkus@proton.me>

Input: aw8697 - Add Awinic AW8697 haptics driver

This commit adds a driver for the Awinic AW8697 LRA haptic driver IC,
providing force feedback support with rumble capabilities.

> diff --git a/drivers/input/misc/aw8697.c b/drivers/input/misc/aw8697.c
> new file mode 100644
> index 0000000000000..3155321cb37be
> --- /dev/null
> +++ b/drivers/input/misc/aw8697.c
[ ... ]
> +static int aw8697_play(struct aw8697_data *haptics)
> +{
[ ... ]
> +	err = regmap_update_bits(haptics->regmap, AW8697_SYSCTRL_REG,
> +				 AW8697_SYSCTRL_PLAY_MODE_MASK |
> +					AW8697_SYSCTRL_BST_MODE_BOOST |
> +					AW8697_SYSCTRL_WORK_MODE_STANDBY,
> +				 FIELD_PREP(AW8697_SYSCTRL_PLAY_MODE_MASK,
> +					    AW8697_SYSCTRL_PLAY_MODE_RAM) |
> +					AW8697_SYSCTRL_BST_MODE_BOOST);
> +	if (err)
> +		return err;
> +
> +	return regmap_set_bits(haptics->regmap, AW8697_GO_REG, AW8697_GO);
> +}

[Severity: Low]
Does this violate the input subsystem maintainer style preference for
explicit error paths? The guideline suggests avoiding the return action()
pattern when returning error codes in functions with multiple failure
points, and instead explicitly checking the error and returning 0 on
success.

[ ... ]
> +static int aw8697_hw_init(struct aw8697_data *haptics)
> +{
[ ... ]
> +	err = regmap_write(haptics->regmap, AW8697_RSPARE_REG, 0x68);
> +	if (err)
> +		return err;
> +
> +	return regmap_clear_bits(haptics->regmap, AW8697_BSTAUTO_REG,
> +				 AW8697_BSTAUTO_AUTOSW);
> +}

[Severity: Low]
This isn't a bug, but should this also be updated to use the expanded
error check form, similar to the style suggestion above?

[ ... ]
> +static int aw8697_ram_init(struct aw8697_data *haptics)
> +{
[ ... ]
> +	err = regmap_noinc_write(haptics->regmap, AW8697_RAMDATA_REG,
> +				 aw8697_waveform, ARRAY_SIZE(aw8697_waveform));
> +	if (err)
> +		return err;
> +
> +	return regmap_clear_bits(haptics->regmap, AW8697_SYSCTRL_REG,
> +				 AW8697_SYSCTRL_RAMINIT);
> +}

[Severity: Low]
This isn't a bug, but it also uses the return action() pattern that is
discouraged by input subsystem style guidelines.

[ ... ]
> +static int aw8697_f0_calibrate(struct aw8697_data *haptics)
> +{
[ ... ]
> +	dev_dbg(haptics->dev, "LRA f0 %d.%d Hz, trim %d\n",
> +		f0 / 10, f0 % 10, step);
> +
> +	return regmap_write(haptics->regmap, AW8697_TRIMLRA_REG,
> +			    step & AW8697_TRIMLRA_MASK);
> +}

[Severity: Low]
This isn't a bug, but it also uses the return action() pattern.

[ ... ]
> +static int aw8697_probe(struct i2c_client *client)
> +{
[ ... ]
> +	err = regmap_read(haptics->regmap, AW8697_SYSINT_REG, &val);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to clear interrupts\n");
> +
> +	err = devm_request_threaded_irq(dev, client->irq, NULL, aw8697_irq,
> +					IRQF_ONESHOT, NULL, haptics);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to request irq\n");

[Severity: Low]
Does passing a NULL devname to devm_request_threaded_irq() degrade
/proc/interrupts readability? Since the IRQ core uses the devname pointer
to display the interrupt name, passing NULL causes it to display
generically as (null). Could this be replaced with a descriptive string?

[ ... ]
> +static int aw8697_suspend(struct device *dev)
> +{
> +	struct aw8697_data *haptics = dev_get_drvdata(dev);
> +
> +	guard(mutex)(&haptics->input_dev->mutex);
> +
> +	if (input_device_enabled(haptics->input_dev))
> +		aw8697_halt(haptics);
> +
> +	return 0;
> +}

[Severity: High]
Is there a race condition here during suspend that could issue I2C
transactions on a suspended bus?

When a force-feedback effect is active and the system enters suspend,
aw8697_suspend() halts the motor and cancels pending work via
aw8697_halt(). However, this doesn't appear to inhibit the ff-memless
background timer from re-firing.

If the timer fires concurrently or after this suspend callback, it will
call aw8697_play_effect() which schedules play_work on the non-freezable
system_wq:

aw8697_play_effect() {
        ...
        haptics->level = level;
        schedule_work(&haptics->play_work);
        ...
}

Could this cause the workqueue to execute and issue I2C writes
after the bus is suspended, leading to bus timeouts or a kernel hang?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-mainlining-v1-0-c303cbdb00c8@proton.me?part=2