Re: [PATCH V3 4/9] iio: imu: inv_icm42607: Add Buffer support functions to icm42607

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 30 Mar 2026 14:58:48 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:

> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Add all FIFO parsing and reading functions to support
> inv_icm42607 hardware.
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
A few minor things inline.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/inv_icm42607/inv_icm42607.h   |   4 +
>  .../imu/inv_icm42607/inv_icm42607_buffer.c    | 496 ++++++++++++++++++
>  .../imu/inv_icm42607/inv_icm42607_buffer.h    |  98 ++++
>  .../iio/imu/inv_icm42607/inv_icm42607_core.c  |  25 +
>  4 files changed, 623 insertions(+)
>  create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
>  create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.h
> 
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607.h b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> index 7d13091aa8df..5530fd3bc03f 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607.h

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> new file mode 100644
> index 000000000000..4f5f199586fc
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> @@ -0,0 +1,496 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2026 InvenSense, Inc.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/minmax.h>
> +#include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/delay.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/common/inv_sensors_timestamp.h>
> +#include <linux/iio/iio.h>

Alphabetical order (+ a block for IIO) for headers.

> +
> +static unsigned int inv_icm42607_wm_truncate(unsigned int watermark,
> +					     size_t packet_size)
> +{
> +	size_t wm_size;
> +	unsigned int wm;
> +
> +	wm_size = watermark * packet_size;
> +	if (wm_size > INV_ICM42607_FIFO_WATERMARK_MAX)
> +		wm_size = INV_ICM42607_FIFO_WATERMARK_MAX;
> +
> +	wm = wm_size / packet_size;
> +
> +	return wm;

Why not

	return wm_size / packet_size;

> +}


> +int inv_icm42607_buffer_hwfifo_flush(struct inv_icm42607_state *st,
> +				     unsigned int count)
> +{
> +	s64 gyro_ts, accel_ts;
> +	int ret;
> +
> +	gyro_ts = iio_get_time_ns(st->indio_gyro);
> +	accel_ts = iio_get_time_ns(st->indio_accel);
> +
> +	ret = inv_icm42607_buffer_fifo_read(st, count);
> +
> +	return ret;
Either
	return inv_...
or
	ret = 
	if (ret)
		return ret;

	return 0;

2nd one only if this is going to get more complex in later patches.

> +}
> +
> +int inv_icm42607_buffer_init(struct inv_icm42607_state *st)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	st->fifo.watermark.eff_gyro = 1;
> +	st->fifo.watermark.eff_accel = 1;
> +
> +	/* Configure FIFO_COUNT format in bytes and big endian */
> +	val = INV_ICM42607_INTF_CONFIG0_FIFO_COUNT_ENDIAN;
> +	ret = regmap_update_bits(st->map, INV_ICM42607_REG_INTF_CONFIG0,
> +				 val, val);
regmap_set_bits();


> +	if (ret)
> +		return ret;
> +
> +	/* Initialize FIFO in bypass mode */
> +	return regmap_write(st->map, INV_ICM42607_REG_FIFO_CONFIG1,
> +			    INV_ICM42607_FIFO_CONFIG1_BYPASS);
> +}

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.h b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.h
> new file mode 100644
> index 000000000000..64a66c00a861
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.h
...

> +/* FIFO data packet */
> +struct inv_icm42607_fifo_sensor_data {
> +	__be16 x;
> +	__be16 y;
> +	__be16 z;
> +} __packed;

Why packed? It will be anyway unless I'm missing something.

> +#define INV_ICM42607_FIFO_DATA_INVALID		-32768

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index da04c820dab2..344071089042 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c

...

> @@ -436,6 +449,8 @@ static int inv_icm42607_suspend(struct device *dev)
>  static int inv_icm42607_resume(struct device *dev)
>  {
>  	struct inv_icm42607_state *st = dev_get_drvdata(dev);
> +	struct inv_icm42607_sensor_state *gyro_st = iio_priv(st->indio_gyro);
> +	struct inv_icm42607_sensor_state *accel_st = iio_priv(st->indio_accel);
>  	struct device *accel_dev;
>  	bool wakeup;
>  	int ret;
> @@ -462,6 +477,16 @@ static int inv_icm42607_resume(struct device *dev)
>  	ret = inv_icm42607_set_pwr_mgmt0(st, st->suspended.gyro,
>  					 st->suspended.accel,
>  					 st->suspended.temp, NULL);
> +	if (ret)
> +		return ret;
> +
> +	if (st->fifo.on) {
I've not checked, but if this doesn't get more complex you can do
	if (!st->fifo.on)
		return 0;

	inv_....

	return regmap_write();

> +		inv_sensors_timestamp_reset(&gyro_st->ts);
> +		inv_sensors_timestamp_reset(&accel_st->ts);
> +		ret = regmap_write(st->map, INV_ICM42607_REG_FIFO_CONFIG1,
> +				   INV_ICM42607_FIFO_CONFIG1_MODE);
> +	}
> +
>  	return ret;
>  }
>  


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip