Re: [PATCH v2 2/3] iio: accel: mma8452: Allow open drain interrupt pin configuration

[sashiko-bot@kernel.org]

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

New issues:
- [High] Adding IRQF_SHARED allows the interrupt handler to be invoked by other devices sharing the same IRQ line, even when the MMA8452 is runtime-suspended. The interrupt handler unconditionally performs an I2C read, which causes bus errors when the device is unpowered.

Pre-existing issues:
- [High] Device configuration state is lost on runtime suspend and never restored on resume, leading to silent scale corruption and loss of interrupts.
--

--- Patch [2]: [PATCH v2 2/3] iio: accel: mma8452: Allow open drain interrupt pin configuration ---
commit 066cff55fa993aded66754d740665ed28050f829
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Allow open drain interrupt pin configuration

This patch allows configuring the interrupt pin in open-drain mode by setting
the PP_OD bit in CTRL_REG3 based on the "drive-open-drain" device property.
It also adds the IRQF_SHARED flag to request_threaded_irq() when open-drain
is enabled, allowing the interrupt line to be shared with other devices.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 7d683686dd9d6..fdf7364d7fede 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
[ ... ]
> @@ -1683,7 +1707,8 @@ static int mma8452_probe(struct i2c_client *client)
>  
>  	if (client->irq) {
>  		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
> -					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +					   IRQF_TRIGGER_LOW | IRQF_ONESHOT |
> +					   (data->open_drain ? IRQF_SHARED : 0),

[Severity: High]
With the addition of IRQF_SHARED here, could the interrupt handler be invoked
by other devices sharing the line while the MMA8452 is runtime-suspended?

Looking at mma8452_interrupt(), it unconditionally performs an I2C read:

mma8452_interrupt()
{
	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
	if (src < 0)
		return IRQ_NONE;
...
}

If the MMA8452 goes into runtime suspend (cutting power to the sensor) and
another device triggers the shared interrupt line, wouldn't this unconditional
I2C read attempt to access an unpowered device, leading to I2C bus timeouts or
NACK errors?

>  					   client->name, indio_dev);
>  		if (ret)
>  			goto buffer_cleanup;
[ ... ]
> @@ -1800,6 +1825,10 @@ static int mma8452_runtime_resume(struct device *dev)
>  		return ret;
>  	}
>  
> +	ret = mma8452_set_interrupt_pin_mode(data);

[Severity: High]
This is a pre-existing issue, but I noticed it while looking at the resume
path. Are we losing device configuration state across runtime suspend?

The mma8452_runtime_suspend() function disables vdd_reg and vddio_reg,
cutting power and resetting registers to their Power-On-Reset defaults.
When mma8452_runtime_resume() is called, it only restores CTRL_REG1 (via
mma8452_active) and now CTRL_REG3.

Does this leave other configurations unrestored? For example, MMA8452_DATA_CFG
defaults back to 2G while the software cache retains user configurations, and
MMA8452_CTRL_REG4 and MMA8452_CTRL_REG5 lose their event interrupt routings.
Couldn't this cause corrupted scale calculations or lost interrupts after the
device's first idle suspend period?

> +	if (ret)
> +		goto runtime_resume_failed;
> +
>  	ret = mma8452_active(data);
>  	if (ret < 0)
>  		goto runtime_resume_failed;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-mma8452-open-drain-v2-0-95be9f5f4795@geanix.com?part=2