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

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