Re: [PATCH 2/4] iio: pressure: mpl3115: clean up interrupt handling and locking

Hi Jonathan,

On Sat, 30 May 2026 12:23 ... Jonathan Cameron wrote:
> > Return IRQ_NONE instead of IRQ_HANDLED when reading
> > INT_SOURCE fails.
>
> As in previous, wrap to 75 chars for commit messges.

Will rewrap to 75 chars.

> > On shared interrupt lines, returning IRQ_HANDLED after a
> > failed register read may prevent other handlers from being
> > invoked.
>
> From what I recall that is simply not true. Given interrupts might
> occur at same moment, the shared interrupt code calls all handlers
> as it can't tell if how many interrupt sources have signalled an
> interrupt.
> Look more closely at what isn't invoked (hint it's about when things
> go wrong!)
> Also, doesn't look to me like this driver supports sharing of the
> interrupt anyway.

You're right, the shared-handler rationale is wrong, and this driver
doesn't share the interrupt anyway. That's a better rationale. I'll
rework the commit message around the spurious-interrupt accounting
aspect rather than shared interrupt handling.

> > Switch the trigger handler from explicit mutex_lock/unlock
> > to scoped_guard() for consistency with the locking style
> > used elsewhere in the driver.
>
> I'm a little dubious about the value of scoped_guard() for such
> simple cases but I guess it is harmless. As Andy called out though,
> different type of change, different patch.
> Hmm. Actually do it by pushing the lock down into the function
> called instead and there you can use a guard(). [...]
> So put it in mpl3115_fill_trig_buffer().

Good idea. I'll move the locking into mpl3115_fill_trig_buffer() with
a guard() so the scope sits next to the accesses it protects, and put
that in its own patch.

> > Move mpl3115_config_interrupt() above the interrupt handler
> > in preparation for the FIFO support added in a subsequent
> > patch.
>
> Separate patch as moving and doing other things just makes for
> harder to review code.

Agreed, I'll split the move into its own patch.

> > No functional change intended.
>
> Except for the one you call out above.

Right. I'll drop that line; the IRQ_NONE change moves to its own
patch and will be described there.

Thanks for the detailed review.

On Sun, May 31, 2026 at 12:23 AM Jonathan Cameron <[email protected]> wrote:
>
> On Sat, 30 May 2026 20:39:36 +0900
> SeungJu Cheon <[email protected]> wrote:
>
> > Return IRQ_NONE instead of IRQ_HANDLED when reading
> > INT_SOURCE fails.
>
> As in previous, wrap to 75 chars for commit messges.
>
> >
> > On shared interrupt lines, returning IRQ_HANDLED after a
> > failed register read may prevent other handlers from being
> > invoked.
>
> From what I recall that is simply not true.  Given interrupts might occur
> at same moment, the shared interrupt code calls all handlers as it can't
> tell if how many interrupt sources have signalled an interrupt.
>
> Look more closely at what isn't invoked (hint it's about when things
> go wrong!)
>
> Also, doesn't look to me like this driver supports sharing of the interrupt
> anyway.
>
>
> >
> > Switch the trigger handler from explicit mutex_lock/unlock
> > to scoped_guard() for consistency with the locking style
> > used elsewhere in the driver.
>
> I'm a little dubious about the value of scoped_guard() for such simple
> cases but I guess it is harmless.  As Andy called out though, different
> type of change, different patch.
>
> Hmm. Actually do it by pushing the lock down into the function called
> instead and there you can use a guard(). Has the added advantage of making
> the exact scope of the locking visible next to the calls in it.
> So put it in mpl3115_fill_trig_buffer().
>
> >
> > Move mpl3115_config_interrupt() above the interrupt handler
> > in preparation for the FIFO support added in a subsequent
> > patch.
>
> Separate patch as moving and doing other things just makes for harder
> to review code.
>
> >
> > No functional change intended.
>
> Except for the one you call out above.
>
> >
> > Signed-off-by: SeungJu Cheon <[email protected]>
> > ---
> >  drivers/iio/pressure/mpl3115.c | 59 +++++++++++++++++-----------------
> >  1 file changed, 29 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> > index befb6d48efa9..52a3d0d59769 100644
> > --- a/drivers/iio/pressure/mpl3115.c
> > +++ b/drivers/iio/pressure/mpl3115.c
> > @@ -308,9 +308,8 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
> >       u8 buffer[16] __aligned(8) = { };
> >       int ret;
> >
> > -     mutex_lock(&data->lock);
> > -     ret = mpl3115_fill_trig_buffer(indio_dev, buffer);
> > -     mutex_unlock(&data->lock);
> > +     scoped_guard(mutex, &data->lock)
> > +             ret = mpl3115_fill_trig_buffer(indio_dev, buffer);
> >       if (ret)
> >               goto done;
> >
> > @@ -322,6 +321,32 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
> >       return IRQ_HANDLED;
> >  }
> >
> > +static int mpl3115_config_interrupt(struct mpl3115_data *data,
> > +                                 u8 ctrl_reg1, u8 ctrl_reg4)
> > +{
> > +     int ret;
> > +
> > +     ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > +                                     ctrl_reg1);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG4,
> > +                                     ctrl_reg4);
> > +     if (ret < 0)
> > +             goto reg1_cleanup;
> > +
> > +     data->ctrl_reg1 = ctrl_reg1;
> > +     data->ctrl_reg4 = ctrl_reg4;
> > +
> > +     return 0;
> > +
> > +reg1_cleanup:
> > +     i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > +                               data->ctrl_reg1);
> > +     return ret;
> > +}
> > +
> >  static const struct iio_event_spec mpl3115_temp_press_event[] = {
> >       {
> >               .type = IIO_EV_TYPE_THRESH,
> > @@ -381,7 +406,7 @@ static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
> >
> >       ret = i2c_smbus_read_byte_data(data->client, MPL3115_INT_SOURCE);
> >       if (ret < 0)
> > -             return IRQ_HANDLED;
> > +             return IRQ_NONE;
>
> So the fun of error handling in interrupt threads.  Arguably we have no idea
> if it was our interrupt or not if this occurs - which I suspect is the motivation
> of the author in returning IRQ_HANDLED here (and IRQ_NONE when we did read but there
> were no interrupts).
>
> I think IRQ_NONE still makes more sense because if we do have an intermittent fault
> and the state is such that the interrupt will retrigger, then it doesn't matter if we
> flag one spurious interrupt.
>
>
> >
> >       if (!(ret & (MPL3115_INT_SRC_TTH | MPL3115_INT_SRC_PTH |
> >                    MPL3115_INT_SRC_DRDY)))
> > @@ -420,32 +445,6 @@ static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
> >       return IRQ_HANDLED;
> >  }
> >
> > -static int mpl3115_config_interrupt(struct mpl3115_data *data,
> > -                                 u8 ctrl_reg1, u8 ctrl_reg4)
> > -{
> > -     int ret;
> > -
> > -     ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > -                                     ctrl_reg1);
> > -     if (ret < 0)
> > -             return ret;
> > -
> > -     ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG4,
> > -                                     ctrl_reg4);
> > -     if (ret < 0)
> > -             goto reg1_cleanup;
> > -
> > -     data->ctrl_reg1 = ctrl_reg1;
> > -     data->ctrl_reg4 = ctrl_reg4;
> > -
> > -     return 0;
> > -
> > -reg1_cleanup:
> > -     i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > -                               data->ctrl_reg1);
> > -     return ret;
> > -}
> > -
> >  static int mpl3115_set_trigger_state(struct iio_trigger *trig, bool state)
> >  {
> >       struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
>