Re: [PATCH 4/4] iio: pressure: mpl3115: add hardware FIFO support

Hi Jonathan,

On Sat, 30 May 2026 01:05 ... Jonathan Cameron wrote:
> Note that even though you've had a couple of quick reviews wait at
> least a few days / 1 week to see if others have review comments
> before doing a new version.

Will do. I'll hold v2 for a week or so to let any further comments
come in.

> > +#define MPL3115_FIFO_SAMPLES_PER_READ 6U
>
> comment on why 6.

Will add a comment explaining the 6.

> > +     return i2c_smbus_write_byte_data(data->client, MPL3115_F_SETUP,
> > +                     f_setup);
>
> Align after ( Or just go a bit long and put it one line.

Will put it on one line.

> > +     while (remaining > 0) {
>
> Don't think this can got negative. I'd make that explicit as != 0

Will change to != 0.

> > +             ret = i2c_smbus_read_i2c_block_data(data->client,
> > [...]
>
> Given this is reading up to 6 samples, it feels odd to just keep
> reading without pushing any of them to the buffer. I'd rework the
> logic so for each iteration of the fifo we push the data we have.
> That will reduce the storage requirement quite a bit.

Good point. I'll push samples as I read each batch instead of
buffering them all first, which also shrinks fifo_buf considerably.

> > +     u8 buffer[MPL3115_BUF_SIZE] __aligned(sizeof(s64)) = { };
>
> This is only used in the loop below. Declare it down there with
> an appropriate struct (see reply I sent to Andy's review)

Will move it into the loop as a struct with __be32/__be16 fields and
an aligned s64 timestamp.

> > +     unsigned int sample_count, i;
> > +     int ret;
> > +     bool overflow;
> > +     s64 ts;
>
> When no other reason for ordering, reverse xmas tree preferred in
> IIO.

Will reorder.

> > +     if (sample_count == 0)
>
> Add a comment on what overflow with no samples means. That seems
> unusual.

Will add a comment for the overflow-with-no-samples case.

> > +     ts = iio_get_time_ns(indio_dev);
>
> This data is coming from a fifo. How is the time grabbed here
> relevant?
> Timestamp + fifo handling is hard. See how the invensense IMU
> drivers do it. For 1st version of this you may have to just fail the
> buffer enable if the timestamp is turned on. I'd revisit timestamps
> after the rest is upstream (or at least as additional patches).

Agreed, stamping FIFO samples with the current time isn't ideal. For
v2 I'll reject buffer enable if the timestamp channel is on, and leave
proper FIFO timestamping (along the lines of the invensense drivers)
to a follow-up once the rest is in.

> Maybe worth introducing an enum for those bits (use in the
> iio_chan_spec array elements as well)
> > +     0,
> No comma (Andy got this already)

Will drop the comma and introduce an enum for the scan indices, reused
in the channel specs.

> > +             if (flush_ret < 0)
> > +                     goto err_reset_fifo;
> > [...]
> > +err_reset_fifo:
> > +     scoped_guard(mutex, &data->lock)
>
> So this is technically not a bug but it runs into the basic argument
> that if you are using automatic cleanup you shouldn't be using gotos
> in the same function [...]
> Hmm. This is trickier given fifo_reset is called in fifo_flush.
> Maybe a wrapper and guard() The none guard version would typically
> use __mpl3115_fifo_reset() naming.

Makes sense. I'll push the locking into the functions and use guard()
there, with a __mpl3115_fifo_reset() helper for the already-locked
caller, so the handler no longer mixes scoped_guard() with a goto.

> It's hard to retrofit a fifo and not end up with a weird control
> interface. We can't remove the triggered buffer because that will be
> a regression for existing users.
> So the trick we have done in the past is to enable the fifo whenever
> a trigger is not set, but the buffer enabled.
> [...]
> Do this unconditionally. It will register a kfifo so we can use that
> anyway, but will also have the interface we can't remove [...]

Understood. I'll drop the use_fifo gating and keep the triggered
buffer registration unconditional, then select the FIFO path from
postenable when no trigger is configured. I'll try option (A) by first
making the triggered capture always read both channels, so the FIFO
path can avoid introducing available_scan_masks.

Thanks very much for the detailed review.

On Sun, May 31, 2026 at 1:05 AM Jonathan Cameron <[email protected]> wrote:
>
> On Sat, 30 May 2026 20:39:38 +0900
> SeungJu Cheon <[email protected]> wrote:
>
> > Add support for the MPL3115A2 hardware FIFO.
> >
> > The device provides a 32-sample FIFO with configurable
> > watermark interrupts, allowing buffered data acquisition
> > with reduced interrupt and CPU overhead.
> >
> > Use a kfifo-backed IIO buffer when a DRDY interrupt and
> > SMBus block reads are available, falling back to the
> > existing triggered buffer implementation otherwise.
> >
> > When enabled, the driver configures the FIFO in circular
> > mode and drains accumulated samples on watermark
> > interrupts.
> >
> > Overflow conditions are handled by flushing available
> > samples before resetting the FIFO, avoiding unnecessary
> > data loss.
> >
> > The hardware always captures pressure and temperature
> > samples together, so FIFO operation requires both channels
> > to be enabled.
> >
> > Register cache updates are performed only after successful
> > I2C writes to keep cached and hardware state consistent.
> >
> > No functional change for non-buffered operation.
> >
> > Signed-off-by: SeungJu Cheon <[email protected]>
>
> Various comments inline to add to what Andy pointed out.
>
> Note that even though you've had a couple of quick reviews wait at least a few
> days / 1 week to see if others have review comments before doing a new version.
> Any discussion will probably have finished by then.
>
> (I can't speak for Andy, but in the UK it's too hot to do much outside
> today - so I'm bored!)
>
> Thanks,
>
> Jonathan
>
> > ---
> >  drivers/iio/pressure/mpl3115.c | 266 +++++++++++++++++++++++++++++++--
> >  1 file changed, 256 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> > index 90e83e34ec43..26b6490c8c4c 100644
> > --- a/drivers/iio/pressure/mpl3115.c
> > +++ b/drivers/iio/pressure/mpl3115.c
>
> > +#define MPL3115_BUF_PRESS_OFFSET 0
> > +#define MPL3115_BUF_TEMP_OFFSET 4
> > +#define MPL3115_BUF_TS_OFFSET 8
> > +#define MPL3115_BUF_SIZE (MPL3115_BUF_TS_OFFSET + sizeof(s64))
> > +
> > +#define MPL3115_FIFO_SAMPLES_PER_READ 6U
> comment on why 6.
> > +
> >  #define MPL3115_PT_DATA_EVENT_ALL GENMASK(2, 0)
> >
> >  #define MPL3115_CTRL1_RESET BIT(2) /* software reset */
> > @@ -63,8 +89,12 @@
> >  #define MPL3115_CTRL3_IPOL2 BIT(1)
> >
> >  #define MPL3115_CTRL4_INT_EN_DRDY BIT(7)
> > +#define MPL3115_CTRL4_INT_EN_FIFO BIT(6)
> >  #define MPL3115_CTRL4_INT_EN_PTH BIT(3)
> >  #define MPL3115_CTRL4_INT_EN_TTH BIT(2)
> > +#define MPL3115_CTRL4_ACTIVE_MASK \
> > +     (MPL3115_CTRL4_INT_EN_DRDY | \
> > +      MPL3115_CTRL4_INT_EN_PTH | MPL3115_CTRL4_INT_EN_TTH)
> >
> >  #define MPL3115_CTRL5_INT_CFG_DRDY BIT(7)
> >  #define MPL3115_CTRL5_INT_CFG_FIFO BIT(6)
> > @@ -94,6 +124,9 @@ struct mpl3115_data {
> >       struct mutex lock;
> >       u8 ctrl_reg1;
> >       u8 ctrl_reg4;
> > +     u8 watermark;
> > +     u8 fifo_buf[MPL3115_FIFO_SIZE * MPL3115_FIFO_SAMPLE_SIZE]
> > +             __aligned(IIO_DMA_MINALIGN);
> >  };
> >
> >  enum mpl3115_irq_pin {
> > @@ -101,6 +134,25 @@ enum mpl3115_irq_pin {
> >       MPL3115_IRQ_INT2,
> >  };
> >
> > +static int mpl3115_set_active(struct mpl3115_data *data, bool active)
> > +{
> > +     u8 reg;
> > +     int ret;
> > +
> > +     if (active)
> > +             reg = data->ctrl_reg1 | MPL3115_CTRL1_ACTIVE;
> > +     else
> > +             reg = data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE;
> > +
> > +     ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > +                                     reg);
> One line.  Or as Andy suggested split this in two, then the reg local
> variable isn't needed.
>
> > +     if (ret)
> > +             return ret;
> > +
> > +     data->ctrl_reg1 = reg;
> > +     return 0;
> > +}
> > +
> >  static int mpl3115_request(struct mpl3115_data *data)
> >  {
> >       int ret, tries = 15;
> > @@ -322,6 +374,109 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
> >       return IRQ_HANDLED;
> >  }
> >
> > +static int mpl3115_set_fifo(struct mpl3115_data *data,
> > +                         enum mpl3115_fifo_mode mode)
> > +{
> > +     u8 f_setup;
> > +
> > +     f_setup = FIELD_PREP(MPL3115_F_SETUP_F_MODE, mode) |
> > +               FIELD_PREP(MPL3115_F_SETUP_F_WMRK, data->watermark);
> > +
> > +     return i2c_smbus_write_byte_data(data->client, MPL3115_F_SETUP,
> > +                     f_setup);
>
> Align after (  Or just go a bit long and put it one line.
>
>
> > +}
> > +
> > +static int mpl3115_fifo_reset(struct mpl3115_data *data)
> > +{
> > +     int ret;
> > +
> > +     ret = mpl3115_set_fifo(data, MPL3115_F_MODE_DISABLED);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return mpl3115_set_fifo(data, MPL3115_F_MODE_CIRCULAR);
> > +}
> > +
> > +static int mpl3115_fifo_transfer(struct mpl3115_data *data,
> > +                              unsigned int sample_count)
> > +{
> > +     unsigned int remaining = sample_count;
> > +     unsigned int offset = 0;
> > +     unsigned int batch;
> > +     int ret;
> > +
> > +     while (remaining > 0) {
>
> Don't think this can got negative. I'd make that explicit as != 0
>
> > +             batch = min(remaining, MPL3115_FIFO_SAMPLES_PER_READ);
> > +
> > +             ret = i2c_smbus_read_i2c_block_data(data->client,
> > +                                                 MPL3115_OUT_PRESS,
> > +                                                 batch * MPL3115_FIFO_SAMPLE_SIZE,
> > +                                                 &data->fifo_buf[offset]);
> Given this is reading up to 6 samples, it feels odd to just keep reading without
> pushing any of them to the buffer. I'd rework the logic so for each iteration
> of the fifo we push the data we have. That will reduce the storage requirement
> quite a bit.
> > +             if (ret < 0)
> > +                     return ret;
> > +             if (ret != batch * MPL3115_FIFO_SAMPLE_SIZE)
> > +                     return -EIO;
> > +
> > +             offset += batch * MPL3115_FIFO_SAMPLE_SIZE;
> > +             remaining -= batch;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int mpl3115_fifo_flush(struct iio_dev *indio_dev)
> > +{
> > +     struct mpl3115_data *data = iio_priv(indio_dev);
> > +     u8 buffer[MPL3115_BUF_SIZE] __aligned(sizeof(s64)) = { };
>
> This is only used int he loop below. Declare it down there with
> an appropriate struct (see reply I sent to Andy's review)
>
> > +     unsigned int sample_count, i;
> > +     int ret;
> > +     bool overflow;
> > +     s64 ts;
> When no other reason for ordering, reverse xmas tree preferred in IIO.
>
>
> See below. I'd pull the guard() in here.
>
> > +
> > +     ret = i2c_smbus_read_byte_data(data->client, MPL3115_F_STATUS);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     overflow = FIELD_GET(MPL3115_F_STATUS_F_OVF, ret);
> > +     sample_count = FIELD_GET(MPL3115_F_STATUS_F_CNT, ret);
> > +     if (sample_count == 0)
>
> Add a comment on what overflow with no samples means. That seems
> unusual.
>
> > +             return overflow ? mpl3115_fifo_reset(data) : 0;
> > +
> > +     ret = mpl3115_fifo_transfer(data, sample_count);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ts = iio_get_time_ns(indio_dev);
>
> This data is coming from a fifo. How is the time grabbed here relevant?
>
> Timestamp + fifo handling is hard. See how the invensense IMU drivers
> do it.  For 1st version of this you may have to just fail the buffer
> enable if the timestamp is turned on.  I'd revisit timestamps after the
> rest is upstream (or at least as additional patches).
>
> > +     for (i = 0; i < sample_count; i++) {
> > +             u8 *sample = &data->fifo_buf[i * MPL3115_FIFO_SAMPLE_SIZE];
> > +
> > +             memcpy(&buffer[MPL3115_BUF_PRESS_OFFSET], &sample[0], 3);
> > +             memcpy(&buffer[MPL3115_BUF_TEMP_OFFSET], &sample[3], 2);
> > +
> > +             iio_push_to_buffers_with_ts(indio_dev, buffer, sizeof(buffer), ts);
> > +     }
> > +
> > +     if (overflow) {
> > +             dev_warn_ratelimited(&data->client->dev,
> > +                                  "FIFO overflow, samples may be lost\n");
> > +             return mpl3115_fifo_reset(data);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int mpl3115_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> > +{
> > +     struct mpl3115_data *data = iio_priv(indio_dev);
> > +
> > +     value = clamp_val(value, 1, MPL3115_FIFO_SIZE - 1);
> > +
> > +     guard(mutex)(&data->lock);
> > +     data->watermark = value;
> > +
> > +     return 0;
> > +}
> > +
> >  static int mpl3115_config_interrupt(struct mpl3115_data *data,
> >                                   u8 ctrl_reg1, u8 ctrl_reg4)
> >  {
> > @@ -348,6 +503,67 @@ static int mpl3115_config_interrupt(struct mpl3115_data *data,
> >       return ret;
> >  }
> >
> > +static int mpl3115_buffer_postenable(struct iio_dev *indio_dev)
> > +{
> > +     struct mpl3115_data *data = iio_priv(indio_dev);
> > +     u8 ctrl_reg4;
> > +     int ret;
> > +
> > +     guard(mutex)(&data->lock);
> > +
> > +     ret = mpl3115_set_active(data, false);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = mpl3115_set_fifo(data, MPL3115_F_MODE_CIRCULAR);
> > +     if (ret) {
> > +             mpl3115_set_active(data, true);
> > +             return ret;
> > +     }
> > +
> > +     ctrl_reg4 = data->ctrl_reg4;
> > +     ctrl_reg4 |= MPL3115_CTRL4_INT_EN_FIFO;
> > +     ctrl_reg4 &= ~MPL3115_CTRL4_INT_EN_DRDY;
> > +
> > +     return mpl3115_config_interrupt(data,
> > +             data->ctrl_reg1 | MPL3115_CTRL1_ACTIVE, ctrl_reg4);
> > +}
> > +
> > +static int mpl3115_buffer_predisable(struct iio_dev *indio_dev)
> > +{
> > +     struct mpl3115_data *data = iio_priv(indio_dev);
> > +     u8 ctrl_reg1, ctrl_reg4;
> > +     int ret;
> > +
> > +     guard(mutex)(&data->lock);
> > +
> > +     ret = mpl3115_set_active(data, false);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = mpl3115_set_fifo(data, MPL3115_F_MODE_DISABLED);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ctrl_reg4 = data->ctrl_reg4 & ~MPL3115_CTRL4_INT_EN_FIFO;
> > +     ctrl_reg1 = data->ctrl_reg1;
> > +
> > +     if (ctrl_reg4 & MPL3115_CTRL4_ACTIVE_MASK)
> > +             ctrl_reg1 |= MPL3115_CTRL1_ACTIVE;
> > +
> > +     return mpl3115_config_interrupt(data, ctrl_reg1, ctrl_reg4);
> > +}
> > +
> > +static const struct iio_buffer_setup_ops mpl3115_buffer_ops = {
> > +     .postenable = mpl3115_buffer_postenable,
> > +     .predisable = mpl3115_buffer_predisable,
> > +};
> > +
> > +static const unsigned long mpl3115_scan_masks[] = {
> > +     BIT(0) | BIT(1),
> Maybe worth introducing an enum for those bits (use in the iio_chan_spec array elements
>  as well)
> > +     0,
> No comma (Andy got this already)
> > +};
> > +
> >  static const struct iio_event_spec mpl3115_temp_press_event[] = {
> >       {
> >               .type = IIO_EV_TYPE_THRESH,
> > @@ -409,10 +625,19 @@ static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
> >       if (ret < 0)
> >               return IRQ_NONE;
> >
> > -     if (!(ret & (MPL3115_INT_SRC_TTH | MPL3115_INT_SRC_PTH |
> > -                  MPL3115_INT_SRC_DRDY)))
> > +     if (!(ret & (MPL3115_INT_SRC_FIFO | MPL3115_INT_SRC_TTH |
> > +                  MPL3115_INT_SRC_PTH | MPL3115_INT_SRC_DRDY)))
> >               return IRQ_NONE;
> >
> > +     if (ret & MPL3115_INT_SRC_FIFO) {
> > +             int flush_ret;
> > +
> > +             scoped_guard(mutex, &data->lock)
> > +                     flush_ret = mpl3115_fifo_flush(indio_dev);
> > +             if (flush_ret < 0)
> So this is technically not a bug but it runs into the basic argument that if you are
> using automatic cleanup you shouldn't be using gotos in the same function
> (says that in comments in cleanup.h - Linus was very explicit about this when people
>  1st starting using these functions!)
>
> With it pushed into the function though that doesn't matter.
>
> > +                     goto err_reset_fifo;
> > +     }
> > +
> >       if (ret & MPL3115_INT_SRC_DRDY)
> >               iio_trigger_poll_nested(data->drdy_trig);
> >
> > @@ -444,6 +669,11 @@ static irqreturn_t mpl3115_interrupt_handler(int irq, void *private)
> >       }
> >
> >       return IRQ_HANDLED;
> > +
> > +err_reset_fifo:
> > +     scoped_guard(mutex, &data->lock)
>
> Hmm. This is trickier given fifo_reset is called in fifo_flush.
> Maybe a wrapper and guard()  The none guard version would typically use
> __mpl3115_fifo_reset() naming.
>
>
> > +             mpl3115_fifo_reset(data);
> > +     return IRQ_HANDLED;
> >  }
>
> >  static int mpl3115_trigger_probe(struct mpl3115_data *data,
> > @@ -723,6 +954,7 @@ static int mpl3115_probe(struct i2c_client *client)
> >       const struct i2c_device_id *id = i2c_client_get_device_id(client);
> >       struct mpl3115_data *data;
> >       struct iio_dev *indio_dev;
> > +     bool use_fifo;
> >       int ret;
> >
> >       ret = i2c_smbus_read_byte_data(client, MPL3115_WHO_AM_I);
> > @@ -762,17 +994,31 @@ static int mpl3115_probe(struct i2c_client *client)
> >       if (ret)
> >               return ret;
> >
> > +     data->watermark = MPL3115_DEFAULT_WATERMARK;
> > +
> >       ret = mpl3115_trigger_probe(data, indio_dev);
> >       if (ret)
> >               return ret;
> >
> > -     ret = devm_iio_triggered_buffer_setup(&client->dev,
> > -                                   indio_dev,
> > -                                   NULL,
> > -                                   mpl3115_trigger_handler,
> > -                                   NULL);
> > -     if (ret)
> > -             return ret;
> > +     use_fifo = data->drdy_trig &&
> > +        i2c_check_functionality(client->adapter,
> > +                                I2C_FUNC_SMBUS_READ_I2C_BLOCK);
>
> Hmm. So this is always a fun thing to work out how to handle.
> Gating on a particular i2c controller function isn't appropriate.
> More to the point existing code is relying on that being available.
>
> static int mpl3115_fill_trig_buffer(struct iio_dev *indio_dev, u8 *buffer)
> {
>         struct mpl3115_data *data = iio_priv(indio_dev);
>         int ret, pos = 0;
>
>         if (!(data->ctrl_reg1 & MPL3115_CTRL1_ACTIVE)) {
>                 ret = mpl3115_request(data);
>                 if (ret < 0)
>                         return ret;
>         }
>
>         if (test_bit(0, indio_dev->active_scan_mask)) {
>                 ret = i2c_smbus_read_i2c_block_data(data->client,
>                         MPL3115_OUT_PRESS, 3, &buffer[pos]);
>                 if (ret < 0)
>                         return ret;
>                 pos += 4;
>         }
>
>         if (test_bit(1, indio_dev->active_scan_mask)) {
>                 ret = i2c_smbus_read_i2c_block_data(data->client,
>                         MPL3115_OUT_TEMP, 2, &buffer[pos]);
>                 if (ret < 0)
>                         return ret;
>         }
>
>         return 0;
> }
>
> It's hard to retrofit a fifo and not end up with a weird control
> interface. We can't remove the triggered buffer because that will be
> a regression for existing users.
>
> So the trick we have done in the past is to enable the fifo whenever
> a trigger is not set, but the buffer enabled.
>
> > +
> > +     if (use_fifo) {
> > +             indio_dev->available_scan_masks = mpl3115_scan_masks;
> Given other changes I'm suggesting we have nothing to gate that on.
> 2 options:
> A) take the view that's not that costly and optimize the exiting triggered
>    capture for that case (as a precursor)
> B) Do manual data mangling so that you don't need this.  That is just
>    fill the buffer with enabled channels (that makes a mess of using a struct
>    though as the first channel type changes depending on what is enabled).
>
> I'd do (A)
>
> > +             ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
> > +                                               &mpl3115_buffer_ops);
> Don't do this - instead...
> > +             if (ret)
> > +                     return ret;
> > +     } else {
> > +             ret = devm_iio_triggered_buffer_setup(&client->dev,
> > +                                                   indio_dev,
> > +                                                   NULL,
> > +                                                   mpl3115_trigger_handler,
> > +                                                   NULL);
>
> Do this unconditionally. It will register a kfifo so we can use that
> anyway, but will also have the interface we can't remove (without it
> being an ABI regression) to set the trigger.
>
> > +             if (ret)
> > +                     return ret;
> > +     }
> >
> >       return devm_iio_device_register(&client->dev, indio_dev);
> >  }
>