Re: [PATCH v2 3/3] iio: accel: icm42370: Add FIFO buffer functionality

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 13 Aug 2026 14:26:12 +0200
Kanak Shilledar <kanak.shilledar@axis.com> wrote:

> Add support for the hardware FIFO buffer to support high data rate
> capture. This includes necessary IIO buffer setup ops, watermark
> management and FIFO packet decoding. The accelerometer has 2.25kB FIFO
> size. The buffer can be handled via sysfs.
> 
> Signed-off-by: Kanak Shilledar <kanak.shilledar@axis.com>
Hi Kanak,

I'm out of time for today so this is a little superficial.
I'll take a closer look at the buffer handling in v3.

Thanks,

Jonathan

> diff --git a/drivers/iio/accel/inv_icm42370_buffer.c b/drivers/iio/accel/inv_icm42370_buffer.c
> new file mode 100644
> index 0000000000000..c6a4d313c92c5
> --- /dev/null
> +++ b/drivers/iio/accel/inv_icm42370_buffer.c
> @@ -0,0 +1,496 @@

> +
> +static int inv_icm42370_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct inv_icm42370_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
> +
> +	/* Exit if there are several sensors using the FIFO. */
> +	if (data->fifo.on > 1) {
> +		data->fifo.on--;
> +		return 0;
> +	}
> +
> +	/* set FIFO in bypass mode */
> +	ret = regmap_write(data->map, INV_ICM42370_REG_FIFO_CONFIG1,
> +			   INV_ICM42370_FIFO_CONFIG_BYPASS);
> +	if (ret)
> +		return ret;
> +
> +	/* when FIFO is bypassed it gets disabled, so reduce the
> +	 * count
> +	 */
> +	data->fifo.on--;
> +
> +	/* flush FIFO data */
> +	ret = regmap_write(data->map, INV_ICM42370_REG_SIGNAL_PATH_RESET,
> +			   INV_ICM42370_SIGNAL_PATH_RESET_FIFO_FLUSH);
> +	if (ret)
> +		return ret;
> +
> +	/* disable FIFO threshold interrupt */
> +	ret = regmap_clear_bits(data->map, INV_ICM42370_REG_INT_SOURCE0,
> +				INV_ICM42370_INT_SOURCE0_FIFO_THS_INT1_EN);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

return regmap_clear_bits()

> +}
> +
> +static int inv_icm42370_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct inv_icm42370_data *data = iio_priv(indio_dev);
> +	struct inv_sensors_timestamp *ts = &data->ts;
> +	struct device *dev = regmap_get_device(data->map);
> +	unsigned int sensor;
> +	unsigned int *watermark;
> +	struct inv_icm42370_conf conf = INV_ICM42370_SENSOR_CONF_INIT;
> +	unsigned int sleep_temp = 0;
> +	unsigned int sleep_sensor = 0;
> +	unsigned int sleep;
> +	int ret;
> +
> +	if (indio_dev == data->indio_accel) {
> +		sensor = INV_ICM42370_SENSOR_ACCEL;
> +		watermark = &data->fifo.watermark.accel;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	guard(mutex)(&data->lock);

Read the documentation in cleanup.h.  Functions with stuff from
that and gotos are a non starter.  This one isn't buggy but it
is fragile to later code movement.

> +
> +	inv_sensors_timestamp_apply_odr(ts, 0, 0, 0);
> +
> +	ret = inv_icm42370_buffer_set_fifo_en(data, data->fifo.en & ~sensor);
> +	if (ret)
> +		goto out_unlock;
> +
> +	*watermark = 0;
> +	ret = inv_icm42370_buffer_update_watermark(data);
> +	if (ret)
> +		goto out_unlock;
> +
> +	conf.mode = INV_ICM42370_SENSOR_MODE_OFF;
> +	ret = inv_icm42370_set_accel_conf(data, &conf, &sleep_sensor);
> +	if (ret)
> +		goto out_unlock;
> +
> +out_unlock:
No lock, so reanme that.

> +	/* sleep maximum required time */
> +	sleep = max(sleep_sensor, sleep_temp);
> +	if (sleep)
> +		msleep(sleep);
> +
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return ret;
> +}

> +int inv_icm42370_buffer_fifo_parse(struct inv_icm42370_data *data)
> +{
> +	struct inv_sensors_timestamp *ts;
> +	int ret;
> +
> +	if (data->fifo.nb.total == 0)
> +		return 0;
> +
> +	/* handle accelerometer timestamp and FIFO data parsing */
> +	if (data->fifo.nb.accel > 0) {
> +		ts = &data->ts;
> +		inv_sensors_timestamp_interrupt(
> +			ts, data->fifo.watermark.eff_accel, data->timestamp);
Go long on line to format that more nicely

> +		ret = inv_icm42370_accel_parse_fifo(data->indio_accel);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

> +int inv_icm42370_buffer_init(struct inv_icm42370_data *data)
> +{
> +	unsigned int val;
> +	u8 regval;
> +	int ret;
> +
> +	data->fifo.watermark.eff_accel = 1;
> +
> +	/* watermark should be set to a non-zero value before enabling interrupts */
> +	data->fifo.watermark.accel = 1;
> +	ret = inv_icm42370_buffer_update_watermark(data);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Default FIFO configuration (bits 6 to 5)
> +	 * - FIFO count in bytes
> +	 * - FIFO count in big endian
> +	 */
> +	val = INV_ICM42370_INTF_CONFIG0_FIFO_COUNT_ENDIAN;
> +	ret = regmap_update_bits(data->map, INV_ICM42370_REG_INTF_CONFIG0,
> +				 GENMASK(6, 5), val);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Enable FIFO partial read interrupt.
> +	 * Disable all FIFO EN bits.
> +	 */
> +	ret = inv_icm42370_mreg_read(data, INV_ICM42370_MREG1,
> +				     INV_ICM42370_REG_FIFO_CONFIG5, &regval);
> +	if (ret)
> +		return ret;
> +
> +	regval &= ~(GENMASK(6, 5) | GENMASK(3, 0));

Those need defines so we know what they are.

> +	regval |= INV_ICM42370_FIFO_CONFIG5_WM_GT_TH;
> +	regval |= INV_ICM42370_FIFO_CONFIG5_RESUME_PARTIAL_RD;
> +
> +	return inv_icm42370_mreg_write(data, INV_ICM42370_MREG1,
> +				       INV_ICM42370_REG_FIFO_CONFIG5, regval);
> +}

> diff --git a/drivers/iio/accel/inv_icm42370_buffer.h b/drivers/iio/accel/inv_icm42370_buffer.h
> new file mode 100644
> index 0000000000000..d43a4209a5640
> --- /dev/null
> +++ b/drivers/iio/accel/inv_icm42370_buffer.h

...

> +
> +/**
> + * struct inv_icm42370_fifo - FIFO state variables
> + * @on:		reference counter for FIFO on.
> + * @en:		bits field of INV_ICM42370_SENSOR_* for FIFO EN bits.
> + * @period:	FIFO internal period.
> + * @watermark:	watermark configuration values for accel.
> + * @count:	number of bytes in the FIFO data buffer.
> + * @nb:		accel and total samples in the FIFO data buffer.
> + * @data:	FIFO data buffer aligned for DMA.
> + */
> +struct inv_icm42370_fifo {
> +	struct {
> +		size_t accel;
> +		size_t total;
> +	} nb;
> +	struct {
> +		unsigned int accel;
> +		unsigned int eff_accel;
> +	} watermark;
> +	unsigned int on;
> +	unsigned int en;
> +	size_t count;
> +	u32 period;
> +	u8 *data __aligned(IIO_DMA_MINALIGN);

What do you think that does?  It doesn't do anything useful

> +};

> diff --git a/drivers/iio/accel/inv_icm42370_core.c b/drivers/iio/accel/inv_icm42370_core.c
> index 6266362e83f6a..b1713a2f533c8 100644
> --- a/drivers/iio/accel/inv_icm42370_core.c
> +++ b/drivers/iio/accel/inv_icm42370_core.c
> @@ -20,10 +20,25 @@
>  #include <linux/types.h>
>  #include <linux/units.h>
>  
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/common/inv_sensors_timestamp.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/kfifo_buf.h>
>  
>  #include "inv_icm42370.h"
> +#include "inv_icm42370_buffer.h"
> +
> +#define INV_ICM42370_SCAN_MASK_ACCEL_3AXIS				\
> +	(BIT(INV_ICM42370_ACCEL_SCAN_X) |				\
> +	BIT(INV_ICM42370_ACCEL_SCAN_Y) |				\
> +	BIT(INV_ICM42370_ACCEL_SCAN_Z))
> +
Align after (


> @@ -75,6 +92,18 @@ static const int inv_icm42370_accel_scale[] = {
>  	[2 * INV_ICM42370_ACCEL_FS_2G + 1] = 598550,
>  };
>  
> +/*
> + * IIO buffer layout: must match channel scan types.
> + * Accel: 3 x s16 BE (6 bytes), Temp: 1 x s16 native (2 bytes) = 8 bytes data.
> + * Timestamp: s64 at 8-byte aligned offset.
> + */
> +struct inv_icm42370_accel_buffer {
> +	struct inv_icm42370_fifo_sensor_data accel;
> +	s16 temp;
> +
> +	s64 timestamp __aligned(8);

	aligned_s64 timestamp;

> +};



> @@ -1161,15 +1263,54 @@ static int inv_icm42370_accel_read_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int inv_icm42370_accel_hwfifo_set_watermark(struct iio_dev *indio_dev,
> +						   unsigned int val)
> +{
> +	struct inv_icm42370_data *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	st->fifo.watermark.accel = val;
> +	ret = inv_icm42370_buffer_update_watermark(st);
> +
> +	return ret;

	return inv_icm...

> +}
> +
> +static int inv_icm42370_accel_hwfifo_flush(struct iio_dev *indio_dev,
> +					   unsigned int count)
> +{
> +	struct inv_icm42370_data *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (count == 0)
> +		return 0;
> +
> +	mutex_lock(&st->lock);
	
	guard(mutex)(&st->lock);

> +
> +	ret = inv_icm42370_buffer_hwfifo_flush(st, count);
> +	if (!ret)
> +		ret = st->fifo.nb.accel;
	if (ret)
		return ret;

	return st->fifo.nb.accel;

> +
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}

>  
>  struct iio_dev *inv_icm42370_accel_init(struct iio_dev *indio_dev,
>  					struct inv_icm42370_data *data)
>  {
> +	struct device *dev = regmap_get_device(data->map);
>  	struct inv_sensors_timestamp_chip ts_chip;
> +	int ret;
>  
>  	data->scales = inv_icm42370_accel_scale;
>  	data->scales_len = ARRAY_SIZE(inv_icm42370_accel_scale);
> @@ -1187,13 +1328,64 @@ struct iio_dev *inv_icm42370_accel_init(struct iio_dev *indio_dev,
>  
>  	indio_dev->name = "inv_icm42370";
>  	indio_dev->info = &inv_icm42370_info;
> -	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_ALL_BUFFER_MODES;

That's very unlikely as there are a bunch of those modes.
No driver should ever set ALL_BUFFER_MODES

>  	indio_dev->channels = inv_icm42370_accel_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(inv_icm42370_accel_channels);
>  
> +	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> +					  &inv_icm42370_buffer_ops);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	return indio_dev;
>  }
>  
> +int inv_icm42370_accel_parse_fifo(struct iio_dev *indio_dev)
> +{
> +	struct inv_icm42370_data *data = iio_priv(indio_dev);
> +	struct inv_sensors_timestamp *ts = &data->ts;
> +	ssize_t i, size;
> +	unsigned int no;
> +	const void *accel, *timestamp;
> +	const s8 *temp;
> +	unsigned int odr;
> +	s64 ts_val;
> +	struct inv_icm42370_accel_buffer buffer = {};
{ }; preferred style in IIO (I picked randomly a few years ago!)

Reverse xmas tree.

> +
> +	for (i = 0, no = 0; i < data->fifo.count; i += size, ++no) {
> +		size = inv_icm42370_fifo_decode_packet(&data->fifo.data[i],
> +				&accel, &temp, &timestamp, &odr);
> +		if (size <= 0)
> +			return size;
> +
> +		if (accel == NULL || !inv_icm42370_fifo_is_data_valid(accel))
> +			continue;
> +
> +		if (odr & INV_ICM42370_SENSOR_ACCEL)
> +			inv_sensors_timestamp_apply_odr(ts, data->fifo.period,
> +							data->fifo.nb.total, no);
> +
> +		memcpy(&buffer.accel, accel, sizeof(buffer.accel));
> +
> +		/*
> +		 * FIFO 8-bit temp has sensitivity ~2 LSB/°C.
> +		 * Register 16-bit temp has sensitivity 128 LSB/°C.
> +		 * Scale factor: 128 / 2 = 64.
> +		 * This lets the IIO scale (1000/128) and offset (3200) work
> +		 * correctly for both register reads and FIFO data.
> +		 */
> +		if (temp)
> +			buffer.temp = (s16)*temp * 64;
> +		else
> +			buffer.temp = (s16)INV_ICM42370_DATA_INVALID;

Why put anything at all in there if no temp?

> +
> +		ts_val = inv_sensors_timestamp_pop(ts);
> +		iio_push_to_buffers_with_timestamp(indio_dev, &buffer, ts_val);

iio_push_to_buffers_with_ts() for new code.

> +	}
> +
> +	return 0;
> +}