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

Hi Andy,

On Sat, May 30, 2026 at 9:33 PM Andy Shevchenko
<[email protected]> wrote:
> > Switch the trigger handler from explicit mutex_lock/unlock
> > to scoped_guard() for consistency with the locking style
> > used elsewhere in the driver.
>
> Split these two. Moving to scoped_guard() is a separate refactoring.

Will do. I'll move the scoped_guard() conversion to its own patch in
v2.

> > No functional change intended.
>
> But this is a lie. The change is a (big enough) functional change and
> actually it feels and sounds like it deserves a Fixes tag.

You're right, the IRQ_NONE change is a functional change. I'll split
it into its own patch with a Fixes tag and drop the "no functional
change" wording.

> > +reg1_cleanup:
> > +       i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > +                                 data->ctrl_reg1);
>
> And if this fails?.. (Okay, this is not this patch issue, it's an
> original code.)

Right, that's pre-existing and outside the scope of this series.

> > Move mpl3115_config_interrupt() above the interrupt handler
> > in preparation for the FIFO support added in a subsequent
> > patch.
>
> If you need to move it up, split this move to a separate patch.

Will split the move into its own patch in v2.

Thanks for the review.

On Sat, May 30, 2026 at 9:33 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Sat, May 30, 2026 at 1:40 PM SeungJu Cheon <[email protected]> wrote:
> >
> > Return IRQ_NONE instead of IRQ_HANDLED when reading
> > INT_SOURCE fails.
> >
> > On shared interrupt lines, returning IRQ_HANDLED after a
> > failed register read may prevent other handlers from being
> > invoked.
>
> > Switch the trigger handler from explicit mutex_lock/unlock
> > to scoped_guard() for consistency with the locking style
> > used elsewhere in the driver.
>
> Split these two. Moving to scoped_guard() is a separate refactoring.
>
> > Move mpl3115_config_interrupt() above the interrupt handler
> > in preparation for the FIFO support added in a subsequent
> > patch.
>
> > No functional change intended.
>
> But this is a lie. The change is a (big enough) functional change and
> actually it feels and sounds like it deserves a Fixes tag.
>
> ...
>
> > +reg1_cleanup:
> > +       i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > +                                 data->ctrl_reg1);
>
> And if this fails?.. (Okay, this is not this patch issue, it's an
> original code.)
>
> > +       return ret;
> > +}
>
> ...
>
> > -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;
> > -}
>
> If you need to move it up, split this move to a separate patch.
>
> --
> With Best Regards,
> Andy Shevchenko