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

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 30 May 2026 15:32:23 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sat, May 30, 2026 at 1:40 PM SeungJu Cheon <suunj1331@gmail.com> 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.  
> 
Some fun stuff that Andy has picked out here, so a few comment on top
(I was wondering some of the same things!)
> ...
> 
> > +       u8 fifo_buf[MPL3115_FIFO_SIZE * MPL3115_FIFO_SAMPLE_SIZE]
> > +               __aligned(IIO_DMA_MINALIGN);  
> 
> We have a macro for this,
Which one?  We have the buffer + timestamp ones for IIO but that' not
relevant here.
> 
> ...
> 
> > +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?
This one seems more likely to be one we cover IIO_DECLARE_BUFFER_WITH_TS()

However, looks like the ts is always in the same place and all channels are
captured so I'd prefer this as something like

	struct {
		__be32 pressure;
		__be16 temp;
//there is a gap here hence need to force initialization to avoid stack content leaking.
		aligned_s64 ts;
	} scan = { };

> 
> > +       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.

They are big endian channels so this 'should' be fine but we should make that explicit.
Not the chan_spec for the 20 bit read has a shift of 12 to get around the last byte
not being filled by this memcpy.

(other stuff Andy pointed out cropped)

J