Re: [PATCH v8 9/9] iio: accel: mma8452: Support interrupt sharing

[Esben Haabendal <esben@geanix.com>]

"Jonathan Cameron" <jic23@kernel.org> writes:

> On Tue, 15 Sep 2026 08:21:17 +0200
> Esben Haabendal <esben@geanix.com> wrote:
>
>> "Esben Haabendal" <esben@geanix.com> writes:
>>
>> > "Jonathan Cameron" <jic23@kernel.org> writes:
>> >
>> >> On Mon, 07 Sep 2026 16:51:04 +0200
>> >> Esben Haabendal <esben@geanix.com> 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.
>> >>>
>> >>> 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.
>> >>>
>> >>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> >>> ---
>> >>>  drivers/iio/accel/mma8452.c | 67 +++++++++++++++++++++++++++++++++++++++------
>> >>>  1 file changed, 58 insertions(+), 9 deletions(-)
>> >>>
>> >>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> >>> index fda29df5d109..e521dca37f76 100644
>> >>> --- a/drivers/iio/accel/mma8452.c
>> >>> +++ b/drivers/iio/accel/mma8452.c
>> >>> @@ -120,6 +120,7 @@
>> >>>   * @sleep_val:			time in ms to sleep while waiting for drdy
>> >>>   * @ctrl_reg1:			CTRL_REG1 register shadow value
>> >>>   * @data_cfg:			DATA_CFG register shadow value
>> >>> + * @ctrl_reg4:			CTRL_REG4 register value to restore on resume
>> >>>   * @open_drain:			true for irq pin in open-drain mode
>> >>>   */
>> >>>  struct mma8452_data {
>> >>> @@ -138,6 +139,7 @@ struct mma8452_data {
>> >>>  	int sleep_val;
>> >>>  	u8 ctrl_reg1;
>> >>>  	u8 data_cfg;
>> >>> +	u8 ctrl_reg4;
>> >>>  	bool open_drain;
>> >>>  };
>> >>>
>> >>> @@ -1083,15 +1085,21 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
>> >>>  {
>> >>>  	struct iio_dev *indio_dev = p;
>> >>>  	struct mma8452_data *data = iio_priv(indio_dev);
>> >>> +	struct device *dev = &data->client->dev;
>> >>>  	irqreturn_t ret = IRQ_NONE;
>> >>> +	int pm_status;
>> >>>  	int src;
>> >>>
>> >>> +	pm_status = pm_runtime_get_if_active(dev);
>> >>
>> >> Sashiko raises the question of what happens if you actually get an error
>> >> return from this. You may be deliberately ignoring those, but if
>> >> so add a comment.
>> >
>> > Yes, sounds like a good idea.
>> >
>> >> The fun race around tear down is worth considering in particular (see
>> >> sashiko comment).
>> >
>> > I will look into that. Although very unlikely, it does sound like a real
>> > issue that *could* occur.
>> >
>> > Adding a boolean flag (remove_in_progress = true) to the mma8452_data
>> > struct, and set that in mma8452_remove() before disabling runtime pm
>> > seems like a KISS solution. Should we be returning IRQ_HANDLED or
>> > IRQ_NONE in that case?  There is no IRQ_MAYBE return value :D
>>
>> Or maybe instead:
>>
>> In mma8452_remove(), we switch the order of
>> pm_runtime_disable()/pm_runtime_set_suspended() with free_irq(), and
>> race condition will magically disappear without any further changes.
>>
>> I will push a new version with this change.
>
> I'm not keen if that means we are disabling in a different order to setup.
> Now if you can flip setup as well it is probably fine.

Yes, we should be using same order in setup. I will do that.

Can I keep it in this patch, or do you want this to be split into a
separate patch (together with change to mma8452_remove())?

AFAICS, the changes doesn't fix anything without the changes in
mma8452_interrupt() in this patch, so you could argue that it doesn't
make sense as a separate change.

/Esben