Re: [PATCH v4 5/6] iio: accel: mma8452: Allow open drain interrupt pin configuration

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

Pre-existing issues:
- [High] The PM runtime resume handler fails to restore most device configuration registers after a power cycle, reverting to POR defaults.
- [High] The driver ignores the requested interrupt polarity from irq_get_trigger_type() and fails to configure the hardware's WAKE_INT_POL bit.
- [High] Sysfs read/write handlers modify device registers and poll STATUS without acquiring a PM runtime reference.
--

--- Patch [5]: [PATCH v4 5/6] iio: accel: mma8452: Allow open drain interrupt pin configuration ---
commit 7603b79afdeeb3c4d36936afa7e16bc8d738683c
Author: Esben Haabendal <[email protected]>

    iio: accel: mma8452: Allow open drain interrupt pin configuration
    
    When designing systems sharing the interrupt for mma8452 chips, it is
    helpful to be able to configure the irq pin in open-drain mode (default is
    push-pull).
    
    Reviewed-by: Andy Shevchenko <[email protected]>
    Signed-off-by: Esben Haabendal <[email protected]>

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 106cdd904dbfd..7992f9e388180 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
[ ... ]
> @@ -647,6 +650,22 @@ static int mma8452_set_power_mode(struct mma8452_data *data, u8 mode)
>  	return mma8452_change_config(data, MMA8452_CTRL_REG2, reg);
>  }
>  
> +static int mma8452_set_interrupt_pin_mode(struct mma8452_data *data)
> +{
> +	int reg;
> +
> +	reg = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG3);
> +	if (reg < 0)
> +		return reg;
> +
> +	if (data->open_drain)
> +		reg |= MMA8452_CTRL_REG3_PP_OD;
> +	else
> +		reg &= ~MMA8452_CTRL_REG3_PP_OD;

[Severity: High]
This is a pre-existing issue, but could this function also configure the
interrupt polarity bit to match the requested irq polarity?

If a board's device tree specifies an active-high interrupt, the driver
requests the irq with that flag but leaves the chip generating active-low
signals.

Since this function modifies the open-drain setting in MMA8452_CTRL_REG3,
might it also ensure the polarity bit is set correctly so we don't end up
with missing interrupts or continuous interrupt storms?

> +
> +	return i2c_smbus_write_byte_data(data->client, MMA8452_CTRL_REG3, reg);
> +}
> +
[ ... ]
> @@ -1817,6 +1841,10 @@ static int mma8452_runtime_resume(struct device *dev)
>  		return ret;
>  	}
>  
> +	ret = mma8452_set_interrupt_pin_mode(data);
> +	if (ret)
> +		goto runtime_resume_failed;
> +
>  	ret = mma8452_active(data);

[Severity: High]
This is a pre-existing issue, but does mma8452_runtime_resume() fully restore
the device state after a power cycle?

When the device goes to runtime suspend after a period of inactivity,
mma8452_runtime_suspend() disables the vdd and vddio regulators, causing the
hardware to lose all its register state.

When it wakes up, only MMA8452_CTRL_REG1 and MMA8452_CTRL_REG3 are restored
here. Does this leave crucial configurations like MMA8452_DATA_CFG,
MMA8452_CTRL_REG4, and MMA8452_CTRL_REG5 uninitialized and revert to power-on
defaults?

Could this cause the device to operate with incorrect full-scale settings and
disabled interrupts?

>  	if (ret < 0)
>  		goto runtime_resume_failed;


[Severity: High]
This is a pre-existing issue, but do the sysfs read/write handlers modify
device registers and poll the status without acquiring a pm runtime reference?

For example, looking at mma8452_read() in drivers/iio/accel/mma8452.c:

mma8452_read() {
    ...
    int ret = mma8452_drdy(data);

    if (ret < 0)
        return ret;

    ret = mma8452_set_runtime_pm_state(data->client, true);
    ...
}

The driver polls the status register before waking up the device.
Since the device enters autosuspend and turns off its regulators, will any
subsequent sysfs read or write attempt to access an unpowered chip?

Could this result in bus errors and completely break the sysfs interface?

Also, __mma8452_write_raw() calls into mma8452_change_config() which reads
from the device without holding a pm runtime reference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5