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

[Jonathan Cameron]

> 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@xxxxxxxx>
>
> 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.

>  
>  /*
>   * 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.


> +	.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.

> +		/*
> +		 * 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.

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

Jonanthan

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>