Re: [PATCH v3 5/8] iio: imu: inv_icm42607: Add support for ICM-42370-P

[Kanak Shilledar <Kanak.Shilledar@axis.com>]

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

Hi Jonathan,

On Mon, 2026-09-07 at 02:54 +0100, Jonathan Cameron wrote:
> > Add support for the Invensense ICM-42370-P MEMS MotionTracking 3-
> > axis
> > accelerometer with built-in temperature sensor. This device is
> > almost
> > identical to the existing Invensense ICM-42607-P IMU, but lacks
> > gyroscope. The device supports I2C, SPI and I3C, implement only I2C
> > support. Provide basic support for raw sensor reads via sysfs.
> > There is
> > also a built-in temperature sensor but it can not be turned off.
> > 
> > Datasheet:
> > https://www.invensense.tdk.com/en-us/products/3-axis/icm-42370-p
> > Datasheet: https://www.lcsc.com/product-detail/C5129967.html
> > Signed-off-by: Kanak Shilledar <kanak.shilledar@axis.com>
> > 
> > diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > index fa85cf738cc0..a183a8566617 100644
> > --- a/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > @@ -368,6 +368,7 @@ struct inv_icm42607_sensor_state {
> >  #define
> > INV_ICM42607_REG_WHOAMI				0x75
> >  #define INV_ICM42607P_WHOAMI				0x60
> >  #define INV_ICM42607_WHOAMI				0x67
> > +#define INV_ICM42370P_WHOAMI				0x0D
> 
> I'd put this in alpha numeric order.  Either that or by WHOAMI value.
> Neither is true currently.

Will fix it

> 
> >  
> >  /*
> >   * Timings as listed in section 3 of datasheet, all values listed
> > in datasheet
> > @@ -392,6 +393,7 @@ typedef int (*inv_icm42607_bus_setup)(struct
> > inv_icm42607_state *);
> >  extern const struct regmap_config inv_icm42607_regmap_config;
> >  extern const struct inv_icm42607_hw inv_icm42607_hw_data;
> >  extern const struct inv_icm42607_hw inv_icm42607p_hw_data;
> > +extern const struct inv_icm42607_hw inv_icm42370p_hw_data;
> >  extern const struct dev_pm_ops inv_icm42607_pm_ops;
> >  
> >  const struct iio_mount_matrix *
> > diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > index 7eb486ff673b..e77d72e0f7bc 100644
> > --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > @@ -92,6 +92,17 @@ static const struct inv_icm42607_conf
> > inv_icm42607_default_conf = {
> >  	},
> >  };
> >  
> > +/* Chip initial default configuration */
> > +static const struct inv_icm42607_conf inv_icm42370_default_conf =
> > {
> > +	.gyro = { },
> 
> Don't specificy it.  C will fill it in anyway with 0s.

Will fix it


> > +	.accel = {
> > +		.mode = INV_ICM42607_SENSOR_MODE_OFF,
> > +		.fs = INV_ICM42607_ACCEL_FS_4G,
> > +		.odr = INV_ICM42607_ODR_100HZ,
> > +		.filter = INV_ICM42607_FILTER_BW_25HZ,
> > +	},
> > +};
> > +
> >  const struct inv_icm42607_hw inv_icm42607_hw_data = {
> >  	.whoami = INV_ICM42607_WHOAMI,
> >  	.name = "icm42607",
> 
> 
> > @@ -617,16 +635,38 @@ int inv_icm42607_core_probe(struct regmap
> > *regmap,
> >  	pm_runtime_set_autosuspend_delay(dev,
> > INV_ICM42607_SUSPEND_DELAY_MS);
> >  	pm_runtime_use_autosuspend(dev);
> >  
> > -	/* Initialize IIO device for Accel */
> > -	st->indio_accel = inv_icm42607_accel_init(st);
> > -	if (IS_ERR(st->indio_accel))
> > -		return PTR_ERR(st->indio_accel);
> > +	switch (st->hw->whoami) {
> > +	case INV_ICM42607_WHOAMI:
> > +	case INV_ICM42607P_WHOAMI:
> 
> It is almost never good design to match against whoami values.
> Doing so scales really badly as a driver ends up supporting more and
> more devices.
> 
> Instead encode what they mean in additional properties in the chip
> info structure.

Will modify to sue the chip info struct.

> 
> > +		/*
> > +		 * Invensense, ICM42607 and ICM42607P both have
> > accelerometer
> > +		 * and gyroscope functionality.
> > +		 */
> > +		st->indio_accel = inv_icm42607_accel_init(st);
> > +		if (IS_ERR(st->indio_accel))
> > +			return PTR_ERR(st->indio_accel);
> 
> This first bit is in btoh paths, so drop it out of this switch.
> 
> > +
> > +		st->indio_gyro = inv_icm42607_gyro_init(st);
> 
> Add a has_gyro flag or something like that to chip_info.
> 
> > +		if (IS_ERR(st->indio_gyro))
> > +			return PTR_ERR(st->indio_gyro);
> > +
> > +		break;
> > +	case INV_ICM42370P_WHOAMI:
> > +		/*
> > +		 * Invensense, ICM42370P has only accelerometer
> > functionality.
> > +		 * Thus, set the gryo property to NULL.
> > +		 */
> > +		st->indio_accel = inv_icm42607_accel_init(st);
> > +		if (IS_ERR(st->indio_accel))
> > +			return PTR_ERR(st->indio_accel);
> >  
> > -	/* Initialize IIO device for Gyro */
> > -	st->indio_gyro = inv_icm42607_gyro_init(st);
> > -	if (IS_ERR(st->indio_gyro))
> > -		return PTR_ERR(st->indio_gyro);
> > +		st->indio_gyro = NULL;
> 
> st should be zeroed anyway so shouldn't be any reason to do this.

Will fix this.

> 
> >  
> > +		break;
> > +	default:
> > +		/* No WHOAMI value matched */
> > +		return dev_err_probe(dev, -ENODEV, "Failed to find
> > a matching WHO_AM_I value\n");
> > +	}
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_NS_GPL(inv_icm42607_core_probe, "IIO_ICM42607");

Thanks and Regards,
Kanak Shilledar

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]