Re: [PATCH V2 4/5] iio: imu: inv_icm42600: Add support for icm42607

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 19 Mar 2026 13:29:40 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:

> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Add support for the icm42607 and icm42607p over both I2C and SPI.
> Note that at this time only the icm42607 over i2c has been tested.
> 
> This driver was updated based on the existing icm42600 along with
> the datasheet from Invensense and out-of-tree sources included
> in the LineageOS kernels [1] and Rockchip kernels [2], both derived
> from sources provided by Invensense.
> 
> [1] https://github.com/LineageOS/android_kernel_nvidia_kernel-nx/tree/lineage-23.0/drivers/iio/imu/inv_icm42607x
> [2] https://github.com/rockchip-linux/kernel/tree/develop-6.6/drivers/iio/imu/inv_icm42670
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
Hi Chris,

Just a quick look to see what was here.

A few comments inline but the big question is clearly whether or not it makes sense
to share the driver at all.

Thanks,

Jonathan

>  int inv_icm42600_accel_init(struct inv_icm42600_state *st, struct iio_dev *indio_dev)
>  {
>  	struct device *dev = regmap_get_device(st->map);
> @@ -1202,6 +1610,62 @@ int inv_icm42600_accel_init(struct inv_icm42600_state *st, struct iio_dev *indio
>  	return 0;
>  }
>  
> +int inv_icm42607_accel_init(struct inv_icm42600_state *st, struct iio_dev *indio_dev)
> +{

> +	/* accel events are wakeup capable */
> +	ret = devm_device_init_wakeup(&indio_dev->dev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

return devm_device_init_wakeup();

> +}


> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 29c8c1871e06..949dbf9c2cd3 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c

>  	.gyro = {
>  		.mode = INV_ICM42600_SENSOR_MODE_OFF,
> @@ -157,6 +193,16 @@ static const struct inv_icm42600_hw inv_icm42600_hw[INV_CHIP_NB] = {
>  		.name = "icm42605",
>  		.conf = &inv_icm42600_default_conf,
>  	},
> +	[INV_CHIP_ICM42607] = {

If this goes ahead, we'd probably want to break up the structure arrays, drop
the enum and instead have extern lines for each individual names structure in the header.

The aim is both give more readable handling using the generic functions to get
the data from whatever firmware is in use and ensure all control is via 'data' in
the chip type specific structures.

We used to do the enum thing a lot but it proved to be a not particularly
good idea and we've been slowly ripping it out again in recent years!


> +		.whoami = INV_ICM42607_WHOAMI,
> +		.name = "icm42607",
> +		.conf = &inv_icm42607_default_conf,
> +	},
> +	[INV_CHIP_ICM42607P] = {
> +		.whoami = INV_ICM42607P_WHOAMI,
> +		.name = "icm42607p",
> +		.conf = &inv_icm42607_default_conf,
> +	},

> +/**
> + *  inv_icm42607_setup() - check and setup chip
> + *  @st:	driver internal state
> + *  @bus_setup:	callback for setting up bus specific registers
> + *
> + *  Returns 0 on success, a negative error code otherwise.
> + */
> +static int inv_icm42607_setup(struct inv_icm42600_state *st,
> +			      inv_icm42600_bus_setup bus_setup)
> +{
> +	const struct inv_icm42600_hw *hw = &inv_icm42600_hw[st->chip];
> +	const struct device *dev = regmap_get_device(st->map);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(st->map, INV_ICM42607_REG_WHOAMI, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val != hw->whoami)

I guess the driver is doing this for other devices.  That's a historical
thing.  For new code (+ slowly fixing up drivers) we always accept the
dt binding value even if there is a whoami match failure because that enables
fallback compatibles.  That is newer devices can be supported by older
kernels without requiring code changes.

Instead just print an info message as it might help with debugging.

> +		return dev_err_probe(dev, -ENODEV,
> +				     "invalid whoami %#02x expected %#02x (%s)\n",
> +				     val, hw->whoami, hw->name);
> +
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> index 32aa2e52df2e..eb590eb4f397 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c

>  static int inv_icm42600_gyro_read_sensor(struct inv_icm42600_state *st,
>  					 struct iio_chan_spec const *chan,
>  					 s16 *val)
> @@ -188,6 +265,58 @@ static int inv_icm42600_gyro_read_sensor(struct inv_icm42600_state *st,
>  	return ret;
>  }
>  
> +static int inv_icm42607_gyro_read_sensor(struct inv_icm42600_state *st,
> +					 struct iio_chan_spec const *chan,
> +					 s16 *val)
> +{
...

> +	conf.mode = INV_ICM42600_SENSOR_MODE_LOW_NOISE;
> +	ret = inv_icm42607_set_gyro_conf(st, &conf, NULL);
> +	if (ret)
> +		return ret;
> +
> +	/* read gyro register data */
> +	data = (__be16 *)&st->buffer[0];

Similar to below, I'd be use the buffer directly in the call and
an unaligned get to reduce alignment assumptions. 
And sizeof(__be16) given then there won't be a local variable to use.

> +	ret = regmap_bulk_read(st->map, reg, data, sizeof(*data));
> +	if (ret)
> +		return ret;
> +
> +	*val = (s16)be16_to_cpup(data);
> +	if (*val == INV_ICM42607_DATA_INVALID)
> +		ret = -EINVAL;
		return -EINVAL;

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

>  
>  static int inv_icm42600_gyro_read_scale(struct iio_dev *indio_dev,
>  					int *val, int *val2)
> @@ -276,12 +419,21 @@ static int inv_icm42600_gyro_write_scale(struct iio_dev *indio_dev,
>  
>  	conf.fs = idx / 2;
>  
> -	pm_runtime_get_sync(dev);
> -
> -	scoped_guard(mutex, &st->lock)
> -		ret = inv_icm42600_set_gyro_conf(st, &conf, NULL);
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
> +	if (PM_RUNTIME_ACQUIRE_ERR(&pm))
> +		return -ENXIO;
>  
> -	pm_runtime_put_autosuspend(dev);

Do this in a precursor patch, not buried in here.

> +	switch (st->chip) {
> +	case INV_CHIP_ICM42607:
> +	case INV_CHIP_ICM42607P:
> +		scoped_guard(mutex, &st->lock)
> +			ret = inv_icm42607_set_gyro_conf(st, &conf, NULL);
> +		break;
> +	default:
> +		scoped_guard(mutex, &st->lock)
> +			ret = inv_icm42600_set_gyro_conf(st, &conf, NULL);
> +		break;
> +	}
>  
>  	return ret;
>  }



> @@ -783,6 +1089,56 @@ int inv_icm42600_gyro_init(struct inv_icm42600_state *st, struct iio_dev *indio_
>  	return 0;
>  }
>  
> +int inv_icm42607_gyro_init(struct inv_icm42600_state *st, struct iio_dev *indio_dev)
> +{
...

> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return ret;

	return devm_iio_device_register();

> +
> +	return 0;
> +}


> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c
> index 13e2e7d38638..6bfd5598b262 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c

>  static int inv_icm42600_probe(struct spi_device *spi)
>  {
>  	const void *match;
> @@ -59,12 +88,22 @@ static int inv_icm42600_probe(struct spi_device *spi)
>  		return -EINVAL;
>  	chip = (uintptr_t)match;
>  
> -	/* use SPI specific regmap */
> -	regmap = devm_regmap_init_spi(spi, &inv_icm42600_spi_regmap_config);
> -	if (IS_ERR(regmap))
> -		return PTR_ERR(regmap);
> -
> -	return inv_icm42600_core_probe(regmap, chip, inv_icm42600_spi_bus_setup);
> +	switch (chip) {
> +	case INV_CHIP_ICM42607:
> +	case INV_CHIP_ICM42607P:
> +		regmap = devm_regmap_init_spi(spi, &inv_icm42607_regmap_config);
> +		if (IS_ERR(regmap))
> +			return PTR_ERR(regmap);
> +		return inv_icm42600_core_probe(regmap, chip,
> +					       inv_icm42607_spi_bus_setup);
> +	default:
> +		/* use SPI specific regmap */

This comment is a bit odd given the above is also an SPI specific regmap
and the naming makes it obvious.  Drop the comment.

> +		regmap = devm_regmap_init_spi(spi, &inv_icm42600_spi_regmap_config);
> +		if (IS_ERR(regmap))
> +			return PTR_ERR(regmap);
> +		return inv_icm42600_core_probe(regmap, chip,
> +					       inv_icm42600_spi_bus_setup);
> +	}
>  }

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_temp.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_temp.c
> index 727b03d541a5..fdda555dbd16 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_temp.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_temp.c
> @@ -46,6 +46,34 @@ static int inv_icm42600_temp_read(struct inv_icm42600_state *st, s16 *temp)
>  	return ret;
>  }
>  
> +static int inv_icm42607_temp_read(struct inv_icm42600_state *st, s16 *temp)
> +{
> +	struct device *dev = regmap_get_device(st->map);
> +	__be16 *raw;
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
> +	if (PM_RUNTIME_ACQUIRE_ERR(&pm))
> +		return -ENXIO;

No problem with this in general as it makes the code cleaner, but I'd kind
of expect to ensure consistent local style by updating other places in a
precursor patch.

> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = inv_icm42607_set_temp_conf(st, true, NULL);
> +	if (ret)
> +		return ret;
> +
> +	raw = (__be16 *)&st->buffer[0];

Whilst existing code does this, I'm not sure why we'd pass cast to __be16 * before
passing it to a function taking a void *

> +	ret = regmap_bulk_read(st->map, INV_ICM42607_REG_TEMP_DATA1, raw, sizeof(*raw));
> +	if (ret)
> +		return ret;
> +
> +	*temp = (s16)be16_to_cpup(raw);

Also true in existing code, but this is making a strong assumption about continued
behaviour of IIO_DMA_MINALIGN and that being at least 2.
I'd be tempted to use an unaligned get just to avoid that assumption.


> +	if (*temp == INV_ICM42600_DATA_INVALID)
> +		ret = -EINVAL;

		return -EINVAL;  ? 
No point in setting it otherwise.

> +
> +	return 0;
> +}