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

[Esben Haabendal <esben@geanix.com>]

<sashiko-bot@kernel.org> writes:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [Medium] Hardware-Software Polarity Mismatch
>
> Pre-existing issues:
> - [High] PM runtime usage counter leaks permanently on I2C read/write errors.
> - [High] Hardware state is accessed over I2C before acquiring runtime PM references, failing if the device is suspended.
> --
>
> --- Patch [3]: [PATCH v6 3/9] iio: accel: mma8452: Only apply trigger type when not set by firmware ---
> commit b862ff26c69f226387993dc22a2537b8dc72d9fd
> Author: Esben Haabendal <esben@geanix.com>
>
> iio: accel: mma8452: Only apply trigger type when not set by firmware
>
> This patch modifies the mma8452 driver to only apply a default active-low
> trigger type if the firmware does not specify one. This aims to improve
> backward compatibility while respecting firmware configuration.
>
>> 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);
>> +					   irq_flags, client->name, indio_dev);
>
> [Severity: Medium]
> Does this configuration leave the hardware and software out of sync?
>
> The code reads the firmware's IRQ trigger type and requests the IRQ with
> it, but the actual MMA8452 sensor hardware polarity doesn't appear to be
> updated. If the firmware requests an active-high or rising-edge trigger,
> the host interrupt controller is configured for active-high, but the sensor
> would continue to drive its default active-low signal.
>
> Unless there is an external hardware inverter on the board, could this
> mismatch break interrupts? Should the driver read the requested trigger type
> and configure the IPOL bit in CTRL_REG3 to match?

This has been discussed during this series.

We don't want to modify MMA8452 irq polarity based on firmware IRQ
trigger type. If IRQ polarity should be modified, we should add a
separate device-tree property for that.

This way we can support designs that inverts polarity in hardware.

>
>>  		if (ret)
>>  			goto buffer_cleanup;
>>  	}
>
>
> While reviewing the I2C handling in this driver, I also noticed a couple of
> other issues.
>
> [Severity: High]
> This is a pre-existing issue, but I noticed a potential leak of the runtime
> PM usage counter when handling I2C errors.
>
> In mma8452_read(), the device is woken up, incrementing the PM counter:
>
> mma8452_read() {
>     ...
>     ret = mma8452_set_runtime_pm_state(data->client, true);
>     ...
>     ret = i2c_smbus_read_i2c_block_data(data->client, ...);
>     if (ret < 0)
>         return ret;
> }
>
> If the I2C read fails, the function returns immediately and bypasses the
> matching mma8452_set_runtime_pm_state() false call. Could this permanently
> prevent the device from re-entering autosuspend?
>
> A similar pattern appears to exist in mma8452_write_event_config() where
> an early return on a failed I2C read skips restoring the PM state.

I will address this in a follow-up patch series.

> [Severity: High]
> This is a pre-existing issue, but could some I2C accesses fail if the
> device is currently suspended?
>
> In mma8452_read(), the code checks for data readiness before waking up the
> device:
>
> mma8452_read() {
>     ...
>     ret = mma8452_drdy(data);
>     if (ret < 0)
>         return ret;
>
>     ret = mma8452_set_runtime_pm_state(data->client, true);
> }
>
> Because mma8452_drdy() executes an I2C read against the MMA8452_STATUS
> register, it happens before the power regulators are enabled. If the device
> has already autosuspended (after 2 seconds of inactivity), will this I2C
> read hit powered-off silicon and fail?
>
> A similar direct I2C read without a runtime PM reference seems to happen
> in mma8452_read_raw() for IIO_CHAN_INFO_CALIBBIAS.

I will address this in a follow-up patch series.

> Sashiko AI review · https://sashiko.dev/#/patchset/20260825-mma8452-open-drain-v6-0-9b252804ee80@geanix.com?part=3

/Esben