Re: [PATCH v7 3/8] iio: accel: mma8452: Only apply trigger type when not set by firmware

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Hardware Configuration Mismatch for Interrupt Trigger Type

Pre-existing issues:
- [High] Unbalanced regulator disables and unpowered I2C access on driver remove if runtime PM is suspended
--

commit 4e5790a8b828438ae278194b6cc6f95e4fa39c50
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Only apply trigger type when not set by firmware

Instead of unconditionally overriding the trigger type, it is better to
only apply a default when no trigger type is set by firmware. This should
be reasonably backward compatible, and should only potentially cause
problems if systems exist where firmware specifies an incorrect trigger type.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index f645a5c6fd1c2..1fb43c5b0b72f 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -1683,9 +1683,16 @@ static int mma8452_probe(struct i2c_client *client)
>  		goto trigger_cleanup;
>  
>  	if (client->irq) {
> +		unsigned long irq_flags;
> +
> +		irq_flags = irq_get_trigger_type(client->irq);
> +		if (irq_flags == IRQ_TYPE_NONE) {
> +			dev_info(dev, "invalid irq type, setting default active low\n");
> +			irq_flags = IRQF_TRIGGER_LOW;
> +		}
> +		irq_flags |= IRQF_ONESHOT;
>  		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
> -					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> -					   client->name, indio_dev);

[Severity: High]
Does this code configure the sensor's hardware polarity to match the requested
trigger type?

If the firmware configures the interrupt with an active-high trigger (such as
IRQ_TYPE_LEVEL_HIGH), the patch reads this via irq_get_trigger_type() and
requests the interrupt from the host controller using this flag. However, the
driver never sets the IPOL bit (Interrupt Polarity, bit 1 of CTRL_REG3) to
match this configuration.

Since the sensor remains in its default active-low state, can this hardware
configuration mismatch result in the interrupt signal going unrecognized or
causing an interrupt storm on systems with an active-high trigger?

While reviewing this, I also noticed a pre-existing issue in mma8452_remove():

drivers/iio/accel/mma8452.c:mma8452_remove() {
	...
	pm_runtime_disable(&client->dev);
	pm_runtime_set_suspended(&client->dev);

	if (client->irq)
		free_irq(client->irq, indio_dev);

	iio_triggered_buffer_cleanup(indio_dev);
	mma8452_trigger_cleanup(indio_dev);
	mma8452_standby(iio_priv(indio_dev));

	regulator_disable(data->vddio_reg);
	regulator_disable(data->vdd_reg);
}

[Severity: High]
This is a pre-existing issue, but can this cause a regulator enable count
underflow and I2C timeouts?

If the module is removed while the device is in a runtime-suspended state
(where mma8452_runtime_suspend() has already disabled both regulators),
mma8452_remove() calls pm_runtime_disable() which does not resume the device.

It then calls mma8452_standby(), which performs I2C writes to the unpowered
device, and unconditionally disables the regulators again, causing an underflow.

Is there a way to ensure the device is woken up or the regulator states are
checked before proceeding with removal?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-mma8452-open-drain-v7-0-22946812c928@geanix.com?part=3