Re: [PATCH v10 10/10] iio: accel: mma8452: Support interrupt sharing

[Esben Haabendal]

"Esben Haabendal" <esben@xxxxxxxxxx> writes:

> "Andy Shevchenko" <andriy.shevchenko@xxxxxxxxx> writes:
>
>> On Mon, Sep 28, 2026 at 10:26:26AM +0200, Esben Haabendal wrote:
>>> Adding support for sharing interrupt line with other device requires the
>>> interrupt handler to handle runtime PM suspension properly, ignoring the
>>> irq if the device is suspended (maybe even off). And while at it, we use
>>> the PM reference to ensure we do not get suspended while processing an irq.
>>>
>>> In order to prevent the chip from raising irq while suspended (that is when
>>> using fixed regulator, where suspend just means setting the device in
>>> STANDBY mode), we disable all interrupt sources by clearing CTRL_REG4, and
>>> then restores the value again when resuming.
>>>
>>> The scoped_guard in mma8452_runtime_suspend() is changed to a plain
>>> mutex_lock() instead, both to prevent mixing guards and goto, but also to
>>> ensure that we stay in STANDBY mode while writing to CTRL_REG4 and all the
>>> way up to disabling the device as much as possible.
>>>
>>> With that in place, it is safe to add the IRQF_SHARED flag.
>>>
>>> Keep in mind that the device by default is using push-pull for the irq pin,
>>> which might require additional hardware design to allow interrupt sharing.
>>
>> ...
>>
>>> +	pm_status = pm_runtime_get_if_active(dev);
>>> +	if (pm_status == 0)
>>> +		/* device is powered down */
>>> +		return IRQ_NONE;
>>
>>> +	if (IS_ENABLED(CONFIG_PM) && pm_status < 0)
>>> +		/* runtime PM was disabled, possibly suspending */
>>> +		return IRQ_HANDLED;
>>
>> This is very interesting part. I bet this will be the first driver using this.
>> A big question "why?"
>
> Because as I understood it, I needed to add this for the patch to be
> accepted [1]. It is definitely possible that I have misunderstood that.

Sorry, forgot to add the reference.

[1] https://lore.kernel.org/all/20260921015115.4c7d79f3@jic23-hlaptop/

>
> The technical reason I believe is that during system suspend,
> pm_runtime_force_suspend() disabled runtime PM, and if a shared
> interrupt durings this, pm_runtime_get_if_active returns -EINVAL, and we
> could therefore end up doing I2C reads on unpowered hardware.
>
> Per that reasoning, we probably should be doing this in other drivers as
> well.
>
>>> +	/*
>>> +	 * pm_status is now 1 or -EINVAL (with CONFIG_PM not enabled). If
>>> +	 * pm_status==1, runtime PM is enabled and device is RPM_ACTIVE. If
>>> +	 * pm_status==-EINVAL, runtime PM is build-time disabled (i.e. CONFIG_PM
>>> +	 * not enabled), and we can/must assume device is active.
>>> +	 */
>>> +
>>>  	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
>>>  	if (src < 0)
>>> -		return IRQ_NONE;
>>> +		goto out_runtime_put;
>>>
>>>  	if (!(src & (data->chip_info->enabled_events | MMA8452_INT_DRDY)))
>>> -		return IRQ_NONE;
>>> +		goto out_runtime_put;
>>>
>>>  	if (src & MMA8452_INT_DRDY) {
>>>  		iio_trigger_poll_nested(indio_dev->trig);
>>> @@ -1120,6 +1139,10 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
>>>  		ret = IRQ_HANDLED;
>>>  	}
>>>
>>> +out_runtime_put:
>>> +	if (pm_status > 0)
>>> +		pm_runtime_put_autosuspend(dev);
>>> +
>>>  	return ret;
>>>  }
>>
>> ...
>>
>>> +	pm_runtime_enable(dev);
>>> +	pm_runtime_set_autosuspend_delay(dev, MMA8452_AUTO_SUSPEND_DELAY_MS);
>>> +	pm_runtime_use_autosuspend(dev);
>>
>> I don't see the respective _dont_use_autosuspend() call anywhere.
>
> Ah. I was not aware it was attached to a usage counter as well. I will
> add a pm_runtime_dont_use_autosuspend() to the runtime_suspend: error handling.
>
>> ...
>>
>> I am wondering how many of the above possible scenarios you were able
>> to test.
>
> Not enough. I haven't done any additional instrumentation to cause all
> the various error conditions that is handled here in _probe().
>
>> ...
>>
>> P.S.  I bet you can now make a presentation "PM runtime in Linux and
>> why it is so hard."
>
> LOL. I am getting there, yes. I have definitely learned a lot about how
> runtime PM works. Especially since I was mostly starting from scratch on
> that.
>
> But i fear it would easily end up being quite a chaotic presentation :)
>
> /Esben