Re: [PATCH 3/4] iio: pressure: mpl3115: generalize interrupt pin routing

Hi Jonathan,

On Sat, 30 May 2026 12:32 ... Jonathan Cameron wrote:
> > This avoids incorrect interrupt routing after a suspend/resume
> > cycle where the register may not return to its default state.
>
> Need more information on this. What do you actually mean by not
> return to it's default state? There are a lot of registers in this
> device that affect config - are all the others restored after
> resume?
> I suspect you are actually talking about a soft reboot where the
> driver is reloading. Given there is a reset call just above this
> that should never be an issue.

You're right, that justification doesn't hold. The reset call above
covers the reload case, so the register won't be in an unexpected
state here. I'll drop the suspend/resume reasoning and describe the
change simply as writing CTRL_REG5 explicitly instead of relying on
the power-on default when INT2 is selected.

> > Route both DRDY and FIFO interrupts to the selected pin. The
> > FIFO routing is not yet used but is required by the hardware
> > FIFO support added in the following patch.
>
> Do it in that patch then, not now. By all means introduce a
> local variable that you change in that patch, but don't set
> the fifo bit.

Will do. I'll introduce the ctrl_reg5 local here with only the DRDY
bit and add the FIFO bit in the FIFO patch.

> > +#define MPL3115_CTRL5_INT_CFG_FIFO BIT(6)
>
> Not in this patch.

Right, I'll move that define to the FIFO patch.

> > +     ctrl_reg5 = irq_pin == MPL3115_IRQ_INT1 ?
> > +         MPL3115_CTRL5_INT_CFG_DRDY | MPL3115_CTRL5_INT_CFG_FIFO : 0;
>
> That's complex enough that I'd prefer it as a simple if / else
> as will be easier to read. Also you are enabling the fifo
> interrupt. That should be in the next patch, not here.

Will switch to a plain if / else and drop the FIFO bit here.

> Oh that's fun. The data sheet has bit 1 as IPOL2 in the compact
> table 49, but PP_OD2 as bits 0 and bit 1 in table 50. If you are
> bored, might be wroth pointing that typo out to them!

Good catch, I hadn't noticed that one.

> > +             ret = i2c_smbus_write_byte_data(data->client,
> > +                                             MPL3115_CTRL_REG3,
> > +                                             ipol);
>
>                 ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG3, ipol);
>
> Fine to go a bit long where it helps readability (stay under 100
> chars though!)

Will put it on one line.

Thanks again.

On Sun, May 31, 2026 at 12:32 AM Jonathan Cameron <[email protected]> wrote:
>
> On Sat, 30 May 2026 20:39:37 +0900
> SeungJu Cheon <[email protected]> wrote:
>
> > Write CTRL_REG5 explicitly for both INT1 and INT2 rather than
> > relying on power-on defaults when INT2 is selected.
> >
> > This avoids incorrect interrupt routing after a suspend/resume
> > cycle where the register may not return to its default state.
>
> Need more information on this.  What do you actually mean by not return
> to it's default state?  There are a lot of registers in this device
> that affect config - are all the others restored after resume?
>
> I suspect you are actually talking about a soft reboot where the
> driver is reloading. Given there is a reset call just above this
> that should never be an issue.
>
> >
> > Route both DRDY and FIFO interrupts to the selected pin. The
> > FIFO routing is not yet used but is required by the hardware
> > FIFO support added in the following patch.
> Do it in that patch then, not now.  By all means introduce a
> local variable that you change in that patch, but don't set
> the fifo bit.
> >
> > Consolidate polarity configuration into a single code path
> > that handles both interrupt pins uniformly.
> >
> > No functional change intended.
> >
> > Signed-off-by: SeungJu Cheon <[email protected]>
> > ---
> >  drivers/iio/pressure/mpl3115.c | 33 +++++++++++++++++----------------
> >  1 file changed, 17 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> > index 52a3d0d59769..90e83e34ec43 100644
> > --- a/drivers/iio/pressure/mpl3115.c
> > +++ b/drivers/iio/pressure/mpl3115.c
> > @@ -67,6 +67,7 @@
> >  #define MPL3115_CTRL4_INT_EN_TTH BIT(2)
> >
> >  #define MPL3115_CTRL5_INT_CFG_DRDY BIT(7)
> > +#define MPL3115_CTRL5_INT_CFG_FIFO BIT(6)
>
> Not in this patch.
>
> >
> >  static const unsigned int mpl3115_samp_freq_table[][2] = {
> >       { 1,      0 },
> > @@ -624,6 +625,7 @@ static int mpl3115_trigger_probe(struct mpl3115_data *data,
> >  {
> >       struct fwnode_handle *fwnode = dev_fwnode(&data->client->dev);
> >       int ret, irq, irq_type, irq_pin = MPL3115_IRQ_INT1;
> > +     u8 ctrl_reg5;
> >
> >       irq = fwnode_irq_get_byname(fwnode, "INT1");
> >       if (irq < 0) {
> > @@ -634,6 +636,9 @@ static int mpl3115_trigger_probe(struct mpl3115_data *data,
> >               irq_pin = MPL3115_IRQ_INT2;
> >       }
> >
> > +     ctrl_reg5 = irq_pin == MPL3115_IRQ_INT1 ?
> > +         MPL3115_CTRL5_INT_CFG_DRDY | MPL3115_CTRL5_INT_CFG_FIFO : 0;
>
> That's complex enough that I'd prefer it as a simple if / else
> as will be easier to read.  Also you are enabling the fifo interrupt.
> That should be in the next patch, not here.
>
> > +
> >       irq_type = irq_get_trigger_type(irq);
> >       if (irq_type != IRQF_TRIGGER_RISING && irq_type != IRQF_TRIGGER_FALLING)
> >               return -EINVAL;
> > @@ -643,23 +648,19 @@ static int mpl3115_trigger_probe(struct mpl3115_data *data,
> >       if (ret < 0)
> >               return ret;
> >
> > -     if (irq_pin == MPL3115_IRQ_INT1) {
> > -             ret = i2c_smbus_write_byte_data(data->client,
> > -                                             MPL3115_CTRL_REG5,
> > -                                             MPL3115_CTRL5_INT_CFG_DRDY);
> > -             if (ret)
> > -                     return ret;
> > +     ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG5,
> > +                                     ctrl_reg5);
> > +     if (ret)
> > +             return ret;
> >
> > -             if (irq_type == IRQF_TRIGGER_RISING) {
> > -                     ret = i2c_smbus_write_byte_data(data->client,
> > -                                                     MPL3115_CTRL_REG3,
> > -                                                     MPL3115_CTRL3_IPOL1);
> > -                     if (ret)
> > -                             return ret;
> > -             }
> > -     } else if (irq_type == IRQF_TRIGGER_RISING) {
> > -             ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG3,
> > -                                             MPL3115_CTRL3_IPOL2);
> > +     if (irq_type == IRQF_TRIGGER_RISING) {
> > +             u8 ipol = irq_pin == MPL3115_IRQ_INT1 ?
> > +                       MPL3115_CTRL3_IPOL1 :
> > +                       MPL3115_CTRL3_IPOL2;
>
> Oh that's fun. The data sheet has bit 1 as IPOL2 in the compact
> table 49, but PP_OD2 as bits 0 and bit 1 in table 50.  If you are bored,
> might be wroth pointing that typo out to them!
>
> > +
> > +             ret = i2c_smbus_write_byte_data(data->client,
> > +                                             MPL3115_CTRL_REG3,
> > +                                             ipol);
>                 ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG3, ipol);
>
> Fine to go a bit long where it helps readability (stay under 100 chars though!)
>
>
>
> >               if (ret)
> >                       return ret;
> >       }
>