Re: [PATCH V7 06/11] iio: imu: inv_icm42607: Add Buffer support for icm42607

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 15 May 2026 08:00:11 -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>
Sashiko didn't do so well on this one so some of what it raised
looks wrong.  E.g. the fifo stuff where you are deliberately clearing
bits with the writes rather than 'accidentally' writing over them.


There is one odd misbalance in here for fifo_en() calls
that I'd like a some comments on in the code.
It's not unheard of that we can't deal with the complexity
of update_scan_mode() only occuring in setup path for buffers
but most of the time we can by pushing stuff to posteenable()
/predisable()

The model for how the buffer setup flow works vs tear down doesn't
quite fit everything despite there being a lot of possible callbacks
so what you have may well be the best that we can do.  However it
isn't obviously correct so a few comments please!

> ---
>  drivers/iio/imu/inv_icm42607/Makefile         |   1 +
>  drivers/iio/imu/inv_icm42607/inv_icm42607.h   |  27 +
>  .../imu/inv_icm42607/inv_icm42607_buffer.c    | 483 ++++++++++++++++++
>  .../imu/inv_icm42607/inv_icm42607_buffer.h    |  93 ++++
>  .../iio/imu/inv_icm42607/inv_icm42607_core.c  |  54 +-
>  5 files changed, 657 insertions(+), 1 deletion(-)
>  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/Makefile b/drivers/iio/imu/inv_icm42607/Makefile
> index be109102e203..3c9d08509793 100644
> --- a/drivers/iio/imu/inv_icm42607/Makefile
> +++ b/drivers/iio/imu/inv_icm42607/Makefile
> @@ -2,6 +2,7 @@
>  
>  obj-$(CONFIG_INV_ICM42607) += inv-icm42607.o
>  inv-icm42607-y += inv_icm42607_core.o
> +inv-icm42607-y += inv_icm42607_buffer.o
>  
>  obj-$(CONFIG_INV_ICM42607_I2C) += inv-icm42607-i2c.o
>  inv-icm42607-i2c-y += inv_icm42607_i2c.o
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607.h b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> index 5f37999e39a5..b00ee6a4d451 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> @@ -14,6 +14,10 @@
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  
> +#include <linux/iio/common/inv_sensors_timestamp.h>
> +
> +#include "inv_icm42607_buffer.h"
> +
>  /* serial bus slew rates */
>  enum inv_icm42607_slew_rate {
>  	INV_ICM42607_SLEW_RATE_20_60NS,
> @@ -84,6 +88,7 @@ struct inv_icm42607_sensor_conf {
>  	int odr;
>  	int filter;
>  };
> +#define INV_ICM42607_SENSOR_CONF_INIT		{-1, -1, -1, -1}
Spaces after { and before } but perhaps more interestingly - why is this
here given I don't see it used in this patch?


> 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..74e5213d9267
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c


> +
> +static int inv_icm42607_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	struct device *dev = regmap_get_device(st->map);
> +	unsigned int sensor;
> +	unsigned int *watermark;
> +	unsigned int sleep_temp = 0;
> +	unsigned int sleep_sensor = 0;
> +	unsigned int sleep;
> +	int ret;
> +
> +	if (indio_dev == st->indio_gyro) {
> +		sensor = INV_ICM42607_SENSOR_GYRO;
> +		watermark = &st->fifo.watermark.gyro;
> +	} else if (indio_dev == st->indio_accel) {
> +		sensor = INV_ICM42607_SENSOR_ACCEL;
> +		watermark = &st->fifo.watermark.accel;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&st->lock);
> +
> +	ret = inv_icm42607_buffer_set_fifo_en(st, st->fifo.en & ~sensor);

This is a little odd as normally I'd expect to see balance between
preenable and postdisable.

Seems you are unwinding stuff that ends up ultimately in update_scan_mode
If that's absolutely necessary (and you can't shift to the balanced calls
- given it's late in sequence postenable / predisable probably..)
then add a comment here.

> +	if (ret)
> +		goto out_unlock;
> +
> +	*watermark = 0;
> +	ret = inv_icm42607_buffer_update_watermark(st);
> +	if (ret)
> +		goto out_unlock;
> +
> +out_unlock:
> +	mutex_unlock(&st->lock);
> +
> +	/* sleep maximum required time */
> +	sleep = max(sleep_sensor, sleep_temp);
> +	if (sleep)
> +		msleep(sleep);

Maybe introduce this stuff only when the values are not 0.

> +
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return ret;
> +}
> +
> +const struct iio_buffer_setup_ops inv_icm42607_buffer_ops = {
> +	.preenable = inv_icm42607_buffer_preenable,
> +	.postenable = inv_icm42607_buffer_postenable,
> +	.predisable = inv_icm42607_buffer_predisable,
> +	.postdisable = inv_icm42607_buffer_postdisable,
> +};
> +



> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index e27ad0319a12..d5885fc3f7da 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> @@ -15,6 +15,7 @@
>  #include <linux/regulator/consumer.h>
>  
>  #include "inv_icm42607.h"
> +#include "inv_icm42607_buffer.h"
>  
>  static bool inv_icm42607_is_volatile_reg(struct device *dev, unsigned int reg)
>  {
> @@ -73,6 +74,40 @@ const struct inv_icm42607_hw inv_icm42607p_hw_data = {
>  };
>  EXPORT_SYMBOL_NS_GPL(inv_icm42607p_hw_data, "IIO_ICM42607");
>  
> +u32 inv_icm42607_odr_to_period(enum inv_icm42607_odr odr)
> +{
> +	static const u32 odr_periods[INV_ICM42607_ODR_NB] = {
> +		/* Reserved values */
> +		0, 0, 0, 0, 0,
> +		/* 1600Hz */
[INV_ICM42607_ODR_1600HZ] = 625000,
etc and drop the comments.

> +		625000,
> +		/* 800Hz */
> +		1250000,
> +		/* 400Hz */
> +		2500000,
> +		/* 200Hz */
> +		5000000,
> +		/* 100 Hz */
> +		10000000,
> +		/* 50Hz */
> +		20000000,
> +		/* 25Hz */
> +		40000000,
> +		/* 12.5Hz */
> +		80000000,
> +		/* 6.25Hz */
> +		160000000,
> +		/* 3.125Hz */
> +		320000000,
> +		/* 1.5625Hz */
> +		640000000,
> +	};
> +
> +	odr = clamp(odr, INV_ICM42607_ODR_1600HZ, INV_ICM42607_ODR_1_5625HZ_LP);

Then this clamp will be more obviously correct.

> +
> +	return odr_periods[odr];
> +}