Re: [PATCH V15 6/9] iio: imu: inv_icm42607: Add Accelerometer for icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver fails to update the cached sensor mode (`st->conf.accel.mode` and `st->conf.gyro.mode`) in `inv_icm42607_set_sensor_conf()`, causing active sensors to be inadvertently powered down during subsequent operations on other channels.
- [Low] Comment mismatch regarding reserved values in ODR arrays.
--

commit 21180a0d12af14de0e8d6f0ad4323cd153c83b4d
Author: Chris Morgan <macromorgan@hotmail.com>

iio: imu: inv_icm42607: Add Accelerometer for icm42607
    
Add icm42607 accelerometer sensor for icm42607.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
> new file mode 100644
> index 0000000000000..8ef9fdae1bc84
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c

[ ... ]

> +static int inv_icm42607_accel_write_scale(struct iio_dev *indio_dev,
> +					  int val, int val2)
> +{

[ ... ]

> +	return inv_icm42607_set_sensor_conf(st, &conf, IIO_ACCEL);
> +}
> +
> +/* IIO format int + micro , values 0-5 reserved. */
> +static const int inv_icm42607_accel_odr[][2] = {
> +	[INV_ICM42607_ODR_1600HZ] = { 1600, 0 },
> +	[INV_ICM42607_ODR_800HZ] = { 800, 0 },

[Severity: Low]
This isn't a bug, but the comment states values 0-5 are reserved.
However, the array defines index 5 (INV_ICM42607_ODR_1600HZ) and the iteration
logic later starts at index 5. Should this say values 0-4 are reserved
instead?

[ ... ]

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 738970ed5c66f..300c583aba81d 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c

[ ... ]

> +int inv_icm42607_set_sensor_conf(struct inv_icm42607_state *st,
> +				 struct inv_icm42607_sensor_conf *conf,
> +				 enum iio_chan_type chan_type)
> +{
> +	struct inv_icm42607_sensor_conf *oldconf;
> +	bool config0, config1;
> +	unsigned int val;
> +	int ret;

[ ... ]

> +	if (config1) {
> +		if (chan_type == IIO_ANGL_VEL) {
> +			val = FIELD_PREP(INV_ICM42607_GYRO_CONFIG1_FILTER_MASK,
> +					 conf->filter);
> +			ret = regmap_update_bits(st->map, INV_ICM42607_REG_GYRO_CONFIG1,
> +						 INV_ICM42607_GYRO_CONFIG1_FILTER_MASK, val);
> +		} else {
> +			val = FIELD_PREP(INV_ICM42607_ACCEL_CONFIG1_FILTER_MASK,
> +					 conf->filter);
> +			ret = regmap_update_bits(st->map, INV_ICM42607_REG_ACCEL_CONFIG1,
> +						 INV_ICM42607_ACCEL_CONFIG1_FILTER_MASK, val);
> +		}
> +		if (ret)
> +			return ret;
> +
> +		oldconf->filter = conf->filter;
> +	}
> +

[Severity: High]
Should conf->mode be saved to oldconf->mode before updating the power
management state?

Since oldconf->mode is never updated here, the software state permanently
tracks both sensors as OFF. When reading one sensor, the stale OFF state
for the other sensor is passed to inv_icm42607_set_pwr_mgmt0(), which
inadvertently powers it down. This can cause the driver to ping-pong power
states and incur 45ms delays on alternating reads.

> +	if (chan_type == IIO_ANGL_VEL)
> +		return inv_icm42607_set_pwr_mgmt0(st, conf->mode, st->conf.accel.mode);
> +
> +	return inv_icm42607_set_pwr_mgmt0(st, st->conf.gyro.mode, conf->mode);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626161230.93069-1-macroalpha82@gmail.com?part=6