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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Operator precedence bug in irqflags expression strips mandatory IRQ flags, causing probe to fail unconditionally.
- [Medium] Ignored return value of mma8452_set_interrupt_pin_mode() in probe can lead to silent hardware misconfiguration.

Pre-existing issues:
- [High] Critical hardware configuration registers are not restored during runtime resume, breaking device operation.
--

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

iio: accel: mma8452: Allow open drain interrupt pin configuration
    
When sharing interrupt line with other chips, the interrupt pin most
likely needs to be configured in open-drain mode instead of push-pull.
If this is needed, you must add drive-open-drain property to the
device-tree.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 7d683686dd9d6..a20c02ce0b9cd 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c

[ ... ]

> @@ -1666,6 +1685,9 @@ static int mma8452_probe(struct i2c_client *client)
>  			goto disable_regulators;
>  	}
>  
> +	data->open_drain = device_property_read_bool(&client->dev, "drive-open-drain");
> +	mma8452_set_interrupt_pin_mode(data);

[Severity: Medium]
Is it safe to ignore the return value of mma8452_set_interrupt_pin_mode()
here? 

If the underlying I2C transaction fails and returns an error like -EREMOTEIO,
the hardware interrupt pin will silently remain in push-pull mode instead of
being configured for open-drain. On a shared bus, this could lead to 
electrical contention.

> +
>  	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
>  			  (MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
>  
> @@ -1683,7 +1705,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]
Does this expression evaluate the way it is intended? 

In C, the bitwise OR operator (|) has higher precedence than the ternary 
operator (?:). This means the flags expression evaluates as:

(IRQF_TRIGGER_LOW | IRQF_ONESHOT | data->open_drain) ? IRQF_SHARED : 0

Since the condition part of the expression is always non-zero, this evaluates
strictly to IRQF_SHARED (0x80), entirely stripping away IRQF_TRIGGER_LOW and
IRQF_ONESHOT. 

Because IRQF_ONESHOT is mandatory for threaded IRQs without a primary handler,
request_threaded_irq() will reject the call with -EINVAL, causing the probe
function to unconditionally fail for any device using an interrupt.

>  					   client->name, indio_dev);
>  		if (ret)
>  			goto buffer_cleanup;

[ ... ]

> @@ -1800,6 +1823,10 @@ static int mma8452_runtime_resume(struct device *dev)
>  		return ret;
>  	}
>  
> +	ret = mma8452_set_interrupt_pin_mode(data);
> +	if (ret < 0)
> +		goto runtime_resume_failed;
> +
>  	ret = mma8452_active(data);

[Severity: High]
This is a pre-existing issue, but since this patch modifies the runtime
resume handler, is the device state fully restored here?

The driver uses runtime PM, and mma8452_runtime_suspend() disables vdd_reg,
the main power supply. When power is removed, all volatile registers revert
to their power-on defaults. 

During mma8452_runtime_resume(), power is restored, but it appears the driver
only reconfigures CTRL_REG1 and CTRL_REG3.

Are vital registers configured in probe(), such as MMA8452_DATA_CFG (full
scale), MMA8452_CTRL_REG4 (interrupt enables), MMA8452_CTRL_REG5 (interrupt
routing), and MMA8452_TRANSIENT_THS missed during this resume? If so, the
device might stop producing interrupts and return invalid data after the first
autosuspend delay.

>  	if (ret < 0)
>  		goto runtime_resume_failed;

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