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

Hi Andy,

On Sat, May 30, 2026 at 10:33 PM Andy Shevchenko
<[email protected]> wrote:
> > +       u8 fifo_buf[MPL3115_FIFO_SIZE * MPL3115_FIFO_SAMPLE_SIZE]
> > +               __aligned(IIO_DMA_MINALIGN);
>
> We have a macro for this,
...
> > +       u8 buffer[MPL3115_BUF_SIZE] __aligned(sizeof(s64)) = { };
>
> Don't we have a macro for this?

I'll rework the per-sample buffer as a struct with explicit fields
(__be32 pressure, __be16 temp, aligned_s64 ts), as Jonathan suggested,
which removes the manual offsets and alignment here.

> > +       ts = iio_get_time_ns(indio_dev);
> > +       for (i = 0; i < sample_count; i++) {
>
>   for (unsigned int i = 0; ...) {

Will declare i in the loop.

> > +               memcpy(&buffer[MPL3115_BUF_PRESS_OFFSET], &sample[0], 3);
> > +               memcpy(&buffer[MPL3115_BUF_TEMP_OFFSET], &sample[3], 2);
>
> These two sound to me like something which might require endianness
> handling, but if you put this as CPU native one and leave user space
> to take care of, it may be okay.

These are big-endian channels (scan_type.endianness = IIO_BE), pushed
as-is for user space to decode. The struct rework will make that
explicit with __be32/__be16 fields.

> > +       value = clamp_val(value, 1, MPL3115_FIFO_SIZE - 1);
>
> Why not clamp()?

Will use clamp().

> > +               mpl3115_set_active(data, true);
>
> Instead of having an argument, just make two helpers: set_active() /
> set_inactive().

Will split into mpl3115_set_active() / mpl3115_set_inactive().

> > +       return mpl3115_config_interrupt(data,
> > +               data->ctrl_reg1 | MPL3115_CTRL1_ACTIVE, ctrl_reg4);
>
> In some cases the FIELD_PREP()/FIELD_GET() is used, why not use FIELD_MODIFY()?

I'll review the FIELD_MODIFY() usage and apply it where appropriate.

> > +static const unsigned long mpl3115_scan_masks[] = {
> > +       BIT(0) | BIT(1),
> > +       0,
>
> No comma in terminator entry.

Will fix.

> > -       ret = devm_iio_triggered_buffer_setup(&client->dev,
> > [...]
>
> Before modifying this, it might make sense to split necessary bits to
> the helper function and then simply reuse it as needed.

Makes sense. I'll look into factoring the common setup into a helper
so the FIFO and triggered-buffer paths can reuse it.

Thanks for the review.

On Sat, May 30, 2026 at 10:33 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Sat, May 30, 2026 at 1:40 PM 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.
>
> ...
>
> > +       u8 fifo_buf[MPL3115_FIFO_SIZE * MPL3115_FIFO_SAMPLE_SIZE]
> > +               __aligned(IIO_DMA_MINALIGN);
>
> We have a macro for this,
>
> ...
>
> > +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)) = { };
>
> Don't we have a macro for this?
>
> > +       unsigned int sample_count, i;
> > +       int ret;
> > +       bool overflow;
> > +       s64 ts;
> > +
> > +       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)
> > +               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);
> > +       for (i = 0; i < sample_count; i++) {
>
>   for (unsigned int i = 0; ...) {
>
> > +               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);
>
> These two sound to me like something which might require endianness
> handling, but if you put this as CPU native one and leave user space
> to take care of, it may be okay.
>
> > +
> > +               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);
>
> Why not clamp()?
>
> > +       guard(mutex)(&data->lock);
> > +       data->watermark = value;
> > +
> > +       return 0;
> > +}
>
> ...
>
> > +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);
>
> Instead of having an argument, just make two helpers: set_active() /
> set_inactive().
>
> > +               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);
>
> In some cases the FIELD_PREP()/FIELD_GET() is used, why not use FIELD_MODIFY()?
>
> > +}
>
> ...
>
> > +static const unsigned long mpl3115_scan_masks[] = {
> > +       BIT(0) | BIT(1),
> > +       0,
>
> No comma in terminator entry.
>
> > +};
>
> ...
>
> > -       ret = devm_iio_triggered_buffer_setup(&client->dev,
> > -                                     indio_dev,
> > -                                     NULL,
> > -                                     mpl3115_trigger_handler,
> > -                                     NULL);
> > -       if (ret)
> > -               return ret;
>
> Before modifying this, it might make sense to split necessary bits to
> the helper function and then simply reuse it as needed.
>
> --
> With Best Regards,
> Andy Shevchenko