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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 14 Sep 2026 09:15:55 +0200
Esben Haabendal <esben@geanix.com> wrote:

> "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

This has been much debated. Personally I think IRQ_HANDLED because
we don't know it is a spurious interrupt which is more or less
all IRQ_NONE is useful for.  Arguably doesn't matter as we don't
expect many hits on this path and the spurious handling needs a lot
before it does anything!

> 
> >> +	if (pm_status == 0)
> >> +		return IRQ_NONE; /* device is powered down */  
> >  
> >> @@ -1784,29 +1796,62 @@ static void mma8452_remove(struct i2c_client *client)
> >>  #ifdef CONFIG_PM
> >>  static int mma8452_runtime_suspend(struct device *dev)
> >>  {
> >> -	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> >> +	struct i2c_client *client = to_i2c_client(dev);
> >> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> >>  	struct mma8452_data *data = iio_priv(indio_dev);
> >>  	int ret;
> >>
> >> -	scoped_guard(mutex, &data->lock)
> >> -		ret = mma8452_standby(data);
> >> +	guard(mutex)(&data->lock);  
> > Mixing guards...  
> 
> Yes, I know. Resolving that turned out to be a bit more painful than I
> thought. I do have that resolved in the next series I keep on talking
> about. But distilling that as a separate patch for adding to this series
> turned out to be impossible, as it relies on some of the other work,
> like refactoring of the ACTIVE/STANDBY state handling (synchronization),
> and most importantly, the switch to using regmap caching to properly
> handle restoring of register values on resume.
> 
> I am therefore hoping that we can find some way to agree on things here,
> and then properly resolve it in the next series...

Ok. It is fine for now.  Perhaps add a comment along the lines of
/* Must be first action in scope that needs to be undone on error */

And hopefully it won't be around long enough for anyone to copy it
into more code!
> 
> For what it is worth, in this very specific case, the only guard used is
> first in the LIFO order, and will therefore be executed in exactly the
> right order on exit, as it will run after all the goto cleanup, just as
> the LIFO cleanup order requires.
> 
> Obviously a fragile and undesirable way of doing error handling, but it
> will go away again in the next series where I will eliminate the goto
> error handling in mma8452_runtime_suspend().
>