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

[Jonathan Cameron]

On Tue, 15 Sep 2026 08:21:17 +0200
Esben Haabendal <esben@xxxxxxxxxx> wrote:

> "Esben Haabendal" <esben@xxxxxxxxxx> writes:
> 
> > "Jonathan Cameron" <jic23@xxxxxxxxxx> writes:
> >  
> >> On Mon, 07 Sep 2026 16:51:04 +0200
> >> Esben Haabendal <esben@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>> ---
> >>>  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.

Jonathan