Re: [PATCH V2 1/5] dt-bindings: iio: imu: add icm42607

Re: [PATCH V2 1/5] dt-bindings: iio: imu: add icm42607

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 75 bytes --]

Acked-by: Conor Dooley <conor.dooley@microchip.com>
pw-bot: not-applicable

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 0/5] Add Invensense ICM42607

[Andy Shevchenko]

On Thu, Mar 19, 2026 at 01:29:36PM -0500, Chris Morgan wrote:

> Add support for the ICM42607 IMU. This sensor shares the same
> functionality but a different register layout with the existing
> ICM42600.
> 
> This driver should work with the ICM42607 and ICM42607P over both I2C
> and SPI, however only the ICM42607P over I2C could be tested.

Please, make sure each patch is ~750 ±150 LoC, so it's reviewable.
Last patch is unreviewable 2k+ LoC!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V2 3/5] iio: imu: inv_icm42600: Add registers for icm42607

[Jonathan Cameron]

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

> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Add the register layout for the icm_42607. Most of the registers are
> similar to that of the icm42600, though at different addresses.
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
Hi Chris,

Just taking a quick look given Jean-Baptiste's concerns
about doing this in the existing driver.

Thanks,

Jonathan

>  
> +/* ICM42607 Specific registers. */
> +
> +/* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
> +
> +/* Register Map for User Bank 0 */
> +#define INV_ICM42607_REG_DEVICE_CONFIG			0x01
> +#define INV_ICM42607_DEVICE_CONFIG_SPI_AP_4WIRE		BIT(2)
> +#define INV_ICM42607_DEVICE_CONFIG_SPI_MODE		BIT(0)
> +
> +#define INV_ICM42607_REG_SIGNAL_PATH_RESET		0x02
> +#define INV_ICM42607_SIGNAL_PATH_RESET_SOFT_RESET	BIT(4)
> +#define INV_ICM42607_SIGNAL_PATH_RESET_FIFO_FLUSH	BIT(2)
> +
> +#define INV_ICM42607_REG_DRIVE_CONFIG1			0x03
> +#define INV_ICM42607_DRIVE_CONFIG1_I3C_DDR_MASK		GENMASK(5, 3)
> +#define INV_ICM42607_DRIVE_CONFIG1_I3C_DDR(_rate)	\
> +FIELD_PREP(INV_ICM42607_DRIVE_CONFIG1_I3C_DDR_MASK, (_rate))

I'd indent the second line of these two line defines to make it easier to
see it's a continuation.

> +#define INV_ICM42607_DRIVE_CONFIG1_I3C_SDR_MASK		GENMASK(2, 0)
> +#define INV_ICM42607_DRIVE_CONFIG1_I3C_SDR(_rate)	\
> +FIELD_PREP(INV_ICM42607_DRIVE_CONFIG1_I3C_SDR_MASK, (_rate))

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

[Jonathan Cameron]

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;
> +}

Re: [PATCH V2 2/5] iio: imu: inv_icm42600: Add support for using alternate registers

[Jonathan Cameron]

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

> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Add support to the existing inv_icm42600 to support similar hardware
> with slightly different register layouts. For example the icm42607
> and icm42607p has most of the same functionality and even many of
> the same registers, but the addresses for indiviual registers differ.
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> index 11339ddf1da3..32aa2e52df2e 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c

> @@ -775,13 +774,13 @@ struct iio_dev *inv_icm42600_gyro_init(struct inv_icm42600_state *st)
>  	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
>  					  &inv_icm42600_buffer_ops);
>  	if (ret)
> -		return ERR_PTR(ret);
> +		return ret;
>  
>  	ret = devm_iio_device_register(dev, indio_dev);

return devm_iio_device_register() now make sense.

Re: [PATCH v2 0/5] Add Invensense ICM42607

[Chris Morgan]

On Thu, Mar 19, 2026 at 04:31:50PM -0500, Chris Morgan wrote:
> On Thu, Mar 19, 2026 at 07:55:19PM +0000, Jean-Baptiste Maneyrol wrote:
> > >
> > >________________________________________
> > >From: Chris Morgan <macroalpha82@gmail.com>
> > >Sent: Thursday, March 19, 2026 19:29
> > >To: linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>
> > >Cc: andy@kernel.org <andy@kernel.org>; nuno.sa@analog.com <nuno.sa@analog.com>; dlechner@baylibre.com <dlechner@baylibre.com>; jic23@kernel.org <jic23@kernel.org>; Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; linux-rockchip@lists.infradead.org <linux-rockchip@lists.infradead.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; heiko@sntech.de <heiko@sntech.de>; conor+dt@kernel.org <conor+dt@kernel.org>; krzk+dt@kernel.org <krzk+dt@kernel.org>; robh@kernel.org <robh@kernel.org>; andriy.shevchenko@intel.com <andriy.shevchenko@intel.com>; Chris Morgan <macromorgan@hotmail.com>
> > >Subject: [PATCH v2 0/5] Add Invensense ICM42607
> > > 
> > >From: Chris Morgan <macromorgan@ hotmail. com> Add support for the ICM42607 IMU. This sensor shares the same functionality but a different register layout with the existing ICM42600. This driver should work with the ICM42607 and ICM42607P
> > >ZjQcmQRYFpfptBannerStart
> > >This Message Is From an Untrusted Sender
> > >You have not previously corresponded with this sender.
> > > 
> > >ZjQcmQRYFpfptBannerEnd
> > >From: Chris Morgan <macromorgan@hotmail.com>
> > >
> > >Add support for the ICM42607 IMU. This sensor shares the same
> > >functionality but a different register layout with the existing
> > >ICM42600.
> > >
> > >This driver should work with the ICM42607 and ICM42607P over both I2C
> > >and SPI, however only the ICM42607P over I2C could be tested.
> > >
> > >Changes Since V1:
> > > - Instead of creating a new driver, merged with the existing inv_icm42600
> > >   driver. This necessitated adding some code to the existing driver to
> > >   permit using a different register layout for the same functionality.
> > > - Split changes up a bit more to decrease the size of the individual
> > >   patches. Note that patch 0004 is still pretty hefty; if I need to split
> > >   further I may need to create some temporary stub functions.
> > > - Used guard() and PM_RUNTIME_ACQUIRE_AUTOSUSPEND() on the new functions
> > >   per Jonathan's recommendations.
> > >
> > >Chris Morgan (5):
> > >  dt-bindings: iio: imu: add icm42607
> > >  iio: imu: inv_icm42600: Add support for using alternate registers
> > >  iio: imu: inv_icm42600: Add registers for icm42607
> > >  iio: imu: inv_icm42600: Add support for icm42607
> > >  arm64: dts: rockchip: Add icm42607p IMU for RG-DS
> > >
> > > .../bindings/iio/imu/invensense,icm42600.yaml |   4 +
> > > .../dts/rockchip/rk3568-anbernic-rg-ds.dts    |  20 +-
> > > drivers/iio/imu/inv_icm42600/inv_icm42600.h   | 333 +++++++-
> > > .../iio/imu/inv_icm42600/inv_icm42600_accel.c | 497 ++++++++++-
> > > .../imu/inv_icm42600/inv_icm42600_buffer.c    | 240 +++++-
> > > .../imu/inv_icm42600/inv_icm42600_buffer.h    |   5 +
> > > .../iio/imu/inv_icm42600/inv_icm42600_core.c  | 795 +++++++++++++++---
> > > .../iio/imu/inv_icm42600/inv_icm42600_gyro.c  | 379 ++++++++-
> > > .../iio/imu/inv_icm42600/inv_icm42600_i2c.c   |  53 +-
> > > .../iio/imu/inv_icm42600/inv_icm42600_spi.c   |  59 +-
> > > .../iio/imu/inv_icm42600/inv_icm42600_temp.c  |  64 ++
> > > .../iio/imu/inv_icm42600/inv_icm42600_temp.h  |   4 +
> > > 12 files changed, 2289 insertions(+), 164 deletions(-)
> > >
> > >-- 
> > >2.43.0
> > >
> > >
> > 
> > Hello Chris,
> > 
> > thanks for the patch, but beware that there is a major difference between this
> > chip and inv_icm42600 chips family that is preventing to have a common driver.
> > 
> > inv_icm42600 chips are using direct register access with bank while icm42607
> > chip is using indirect register access using IREG specific registers. Some
> > registers cannot be read/write directly and requires programming specific
> > registers. The mechanism is similar to the one in inv_icm45600 driver, you can
> > have a look in this driver.
> 
> I didn't see anything in the datasheet for the 42607P in regards to the IREG,
> but I did see something in the 42607C. That said, near as I can tell from the
> drivers that I used as a reference from the Rockchip and Lineage trees this driver
> doesn't access anything from those registers.
> 
> Still, I'll defer to you on what you think is best. As long as I can use my
> accelerometer that's all I care about.

So closer looking at the 42607-C datasheet, it looks like they have MREG banks, is
that what you're talking about?

If so I can either remove this and create a new driver (as I did with V1),
or I can just keep the 42607P in this driver since it does appear to use only
direct register access.

Let me know what you think so I can rewrite and resubmit it. I'll try to better
break it up this time too by seeing if I can make temp, gyro, and accel functions
all with different commits.

Thank you,
Chris

> 
> > 
> > Using the same driver is not possible I think, since register access is different.
> > 
> > And there are also more differences between the 2 chip families that is making
> > a common driver quite difficult to do.
> > 
> > Thanks,
> > JB
> 
> Chris