Re: [PATCH v3 3/8] iio: imu: inv_icm45600: add buffer support in iio devices

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 17 Jul 2025 13:25:55 +0000
Remi Buisson via B4 Relay <devnull+remi.buisson.tdk.com@kernel.org> wrote:

> From: Remi Buisson <remi.buisson@tdk.com>
> 
> Add FIFO control functions.
> Support hwfifo watermark by multiplexing gyro and accel settings.
> Support hwfifo flush.
> 
> Signed-off-by: Remi Buisson <remi.buisson@tdk.com>
A few minor things inline.

> ---
>  drivers/iio/imu/inv_icm45600/Makefile              |   1 +
>  drivers/iio/imu/inv_icm45600/inv_icm45600.h        |   7 +
>  drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.c | 514 +++++++++++++++++++++
>  drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.h |  99 ++++
>  drivers/iio/imu/inv_icm45600/inv_icm45600_core.c   | 137 +++++-
>  5 files changed, 757 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/imu/inv_icm45600/Makefile b/drivers/iio/imu/inv_icm45600/Makefile
> index 4f442b61896e91647c7947a044949792bae06a30..19c521ffba17b0d108a8ecb45ecdea35dff6fd18 100644
> --- a/drivers/iio/imu/inv_icm45600/Makefile
> +++ b/drivers/iio/imu/inv_icm45600/Makefile
> @@ -2,3 +2,4 @@
>  
>  obj-$(CONFIG_INV_ICM45600) += inv-icm45600.o
>  inv-icm45600-y += inv_icm45600_core.o
> +inv-icm45600-y += inv_icm45600_buffer.o
> \ No newline at end of file

Fix this by adding one.

> diff --git a/drivers/iio/imu/inv_icm45600/inv_icm45600.h b/drivers/iio/imu/inv_icm45600/inv_icm45600.h
> index 59aed59b94ca2d4709b0c986ddeda80b33064f90..5625c437b6f54336f6e652c2ae2e4ea8f88e2396 100644
> --- a/drivers/iio/imu/inv_icm45600/inv_icm45600.h
> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600.h

>  
> +#define INV_ICM45600_SENSOR_CONF_KEEP_VALUES {U8_MAX, U8_MAX, U8_MAX, U8_MAX, }
> +
>  struct inv_icm45600_conf {
>  	struct inv_icm45600_sensor_conf gyro;
>  	struct inv_icm45600_sensor_conf accel;
> @@ -127,6 +132,7 @@ extern const struct inv_icm45600_chip_info inv_icm45689_chip_info;
>   *  @indio_gyro:	gyroscope IIO device.
>   *  @indio_accel:	accelerometer IIO device.
>   *  @timestamp:		interrupt timestamps.
> + *  @fifo:		FIFO management structure.
>   *  @buffer:		data transfer buffer aligned for DMA.
>   */
>  struct inv_icm45600_state {
> @@ -143,6 +149,7 @@ struct inv_icm45600_state {
>  		s64 gyro;
>  		s64 accel;
>  	} timestamp;
> +	struct inv_icm45600_fifo fifo;

Having that 8 thousand element buffer in here seem unwise.  Normally I encourage putting
everything possible in here, but this maybe a case where a kzalloc() for that buffer is
going to wasted less space.

>  	union {
>  		u8 buff[2];
>  		__le16 u16;
> diff --git a/drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.c b/drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..b37607e52721097daf6362bac20001b0d4210f4b
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm45600/inv_icm45600_buffer.c
> @@ -0,0 +1,514 @@

>
> +void inv_icm45600_buffer_update_fifo_period(struct inv_icm45600_state *st)
> +{
> +	u32 period_gyro, period_accel, period;
> +
> +	if (st->fifo.en & INV_ICM45600_SENSOR_GYRO)
> +		period_gyro = inv_icm45600_odr_to_period(st->conf.gyro.odr);
> +	else
> +		period_gyro = U32_MAX;
> +
> +	if (st->fifo.en & INV_ICM45600_SENSOR_ACCEL)
> +		period_accel = inv_icm45600_odr_to_period(st->conf.accel.odr);
> +	else
> +		period_accel = U32_MAX;
> +
> +	if (period_gyro <= period_accel)
> +		period = period_gyro;

	st->fifo.period = min(period_gyro, period_accel);

> +	else
> +		period = period_accel;
> +
> +	st->fifo.period = period;
> +}

> +
> +static unsigned int inv_icm45600_wm_truncate(unsigned int watermark, size_t packet_size,
> +					     unsigned int fifo_period)
> +{
> +	size_t watermark_max, grace_samples;
> +
> +	/* Keep 20ms for processing FIFO. */
> +	grace_samples = (20U * 1000000U) / fifo_period;

I'mnot sure what the multipler here is but maybe 20U * MICRO
> +	if (grace_samples < 1)
> +		grace_samples = 1;
> +
> +	watermark_max = INV_ICM45600_FIFO_SIZE_MAX / packet_size;
> +	watermark_max -= grace_samples;
> +
> +	if (watermark > watermark_max)
> +		watermark = watermark_max;

min?

> +
> +	return watermark;
> +}