Re: [PATCH 2/2] iio: imu: inv_icm42600: add support of ICM-42686-P

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 22 Apr 2024 15:22:40 +0000
inv.git-commit@tdk.com wrote:

> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> 
> Add ICM-42686-P chip supporting high FSRs (32G, 4000dps).
> 
> Create accel and gyro iio device states with dynamic scales table
> set at device init.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
Some comments inline, but given this is in line with the existing driver
approaches, those can wait for another day. I'll tidy up the blank line
comment.

Applied to the togreg branch of iio.git and pushed out initially as
testing for all the normal boring reasons.

Jonathan


> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 82e0a2e2ad70..42316fb72674 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -65,6 +65,21 @@ static const struct inv_icm42600_conf inv_icm42600_default_conf = {
>  	},
>  	.temp_en = false,
>  };
Blank line here.

> +static const struct inv_icm42600_conf inv_icm42686_default_conf = {
> +	.gyro = {
> +		.mode = INV_ICM42600_SENSOR_MODE_OFF,
> +		.fs = INV_ICM42686_GYRO_FS_4000DPS,
> +		.odr = INV_ICM42600_ODR_50HZ,
> +		.filter = INV_ICM42600_FILTER_BW_ODR_DIV_2,
> +	},
> +	.accel = {
> +		.mode = INV_ICM42600_SENSOR_MODE_OFF,
> +		.fs = INV_ICM42686_ACCEL_FS_32G,
> +		.odr = INV_ICM42600_ODR_50HZ,
> +		.filter = INV_ICM42600_FILTER_BW_ODR_DIV_2,
> +	},
> +	.temp_en = false,
> +};
>  
>  static const struct inv_icm42600_hw inv_icm42600_hw[INV_CHIP_NB] = {
>  	[INV_CHIP_ICM42600] = {
> @@ -82,6 +97,11 @@ static const struct inv_icm42600_hw inv_icm42600_hw[INV_CHIP_NB] = {
>  		.name = "icm42605",
>  		.conf = &inv_icm42600_default_conf,
>  	},
> +	[INV_CHIP_ICM42686] = {
> +		.whoami = INV_ICM42600_WHOAMI_ICM42686,
> +		.name = "icm42686",
> +		.conf = &inv_icm42686_default_conf,
> +	},
>  	[INV_CHIP_ICM42622] = {
>  		.whoami = INV_ICM42600_WHOAMI_ICM42622,
>  		.name = "icm42622",
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> index 3df0a715e885..e6f8de80128c 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c

> @@ -716,8 +750,8 @@ struct iio_dev *inv_icm42600_gyro_init(struct inv_icm42600_state *st)
>  {
>  	struct device *dev = regmap_get_device(st->map);
>  	const char *name;
> +	struct inv_icm42600_sensor_state *gyro_st;
>  	struct inv_sensors_timestamp_chip ts_chip;
> -	struct inv_sensors_timestamp *ts;
>  	struct iio_dev *indio_dev;
>  	int ret;
>  
> @@ -725,9 +759,21 @@ struct iio_dev *inv_icm42600_gyro_init(struct inv_icm42600_state *st)
>  	if (!name)
>  		return ERR_PTR(-ENOMEM);
>  
> -	indio_dev = devm_iio_device_alloc(dev, sizeof(*ts));
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*gyro_st));
>  	if (!indio_dev)
>  		return ERR_PTR(-ENOMEM);
> +	gyro_st = iio_priv(indio_dev);
> +
> +	switch (st->chip) {
> +	case INV_CHIP_ICM42686:
> +		gyro_st->scales = inv_icm42686_gyro_scale;
> +		gyro_st->scales_len = ARRAY_SIZE(inv_icm42686_gyro_scale);

I'd have preferred this data being in the inv_icm42600_hw structures but that
is a more major refactor.   Longer term having any cases like this be simple
'data' rather than code would make for a cleaner and more flexible driver.

> +		break;
> +	default:
> +		gyro_st->scales = inv_icm42600_gyro_scale;
> +		gyro_st->scales_len = ARRAY_SIZE(inv_icm42600_gyro_scale);
> +		break;
> +	}