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

[Marcelo Schmitt]

Hello Kanak, just a couple minor suggestions to this one.

On 09/17, Kanak Shilledar 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.

The device supports I2C, SPI and I3C, though, only I2C support is currently
being implemented.

Would sound a bit more natural.

> Provide basic support for raw sensor reads via sysfs. There is
> also a built-in temperature sensor but it can not be turned off similar
> to the ICM-42607. Return with `dev_err_probe()` when WHO_AM_I read
> fails.
> 
> 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>
> ---
...
>  	/* Warn, but don't fail. */
>  	if (val != st->hw->whoami)
> @@ -630,15 +648,20 @@ 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 */
> +	/*
> +	 * Invensense, ICM42607 and ICM42607P both have accelerometer
> +	 * and gyroscope functionality. Whereas, Invensense, ICM42370
> +	 * has only accelerometer.
> +	 */
I would specify ICM42370P (with 'P') here. Since there is ICM42607 and ICM42607P,
it wouldn't be surprising if ICM42370 (different from ICM42370P) eventually
shows up.

>  	st->indio_accel = inv_icm42607_accel_init(st);
>  	if (IS_ERR(st->indio_accel))
>  		return PTR_ERR(st->indio_accel);
>