Re: [PATCH V10 7/9] iio: imu: inv_icm42607: Add Accelerometer for icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Incomplete IIO buffer support results in dead code and logically flawed `update_scan_mode` callback.
- [Medium] Accelerometer initialized with conflicting power mode and filter macro.
- [Medium] Failing to configure the hardware ODR leaves the software timestamp structure in an inconsistent state.
- [Medium] Lockless reads of shared state in `inv_icm42607_accel_read_scale()` and `inv_icm42607_accel_read_odr()`.
--

commit aa64241d5631fbdd46e4e9a9ec9bb1ee1fabadb5
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..6c1f88688c3f4
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c

[ ... ]

> +static int inv_icm42607_accel_read_scale(struct iio_dev *indio_dev,
> +					 int *val, int *val2)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	unsigned int idx;
> +
> +	idx = st->conf.accel.fs;

[Severity: Medium]
Is it safe to read st->conf.accel.fs here without acquiring st->lock? 

The write path in inv_icm42607_accel_write_scale() mutates this state while 
holding the lock, so concurrent accesses might result in a data race. Should
this read be protected using guard(mutex)(&st->lock)?

[ ... ]

> +static int inv_icm42607_accel_read_odr(struct inv_icm42607_state *st,
> +				       int *val, int *val2)
> +{
> +	unsigned int odr;
> +	unsigned int i;
> +
> +	odr = st->conf.accel.odr;

[Severity: Medium]
Does this read of st->conf.accel.odr need to be protected by st->lock, 
similar to how it is protected during writes in 
inv_icm42607_accel_write_odr()?

[ ... ]

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

[ ... ]

> +	ret = inv_sensors_timestamp_update_odr(ts, inv_icm42607_odr_to_period(conf.odr),
> +					       iio_buffer_enabled(indio_dev));
> +	if (ret)
> +		return ret;
> +
> +	ret = inv_icm42607_set_accel_conf(st, &conf, NULL);
> +	if (ret)
> +		return ret;

[Severity: Medium]
If inv_icm42607_set_accel_conf() fails here, do we leave the software 
timestamp module expecting the new ODR while the hardware remains at the 
old ODR?

[ ... ]

> +struct iio_dev *inv_icm42607_accel_init(struct inv_icm42607_state *st)
> +{

[ ... ]

> +	accel_st->power_mode = INV_ICM42607_SENSOR_MODE_LOW_NOISE;
> +	accel_st->filter = INV_ICM42607_FILTER_AVG_16X;

[Severity: Medium]
Is this the intended filter configuration? 

We are setting the power_mode to INV_ICM42607_SENSOR_MODE_LOW_NOISE, but 
using an averaging filter macro (INV_ICM42607_FILTER_AVG_16X) meant for low 
power mode. 

[ ... ]

> +	iio_device_set_drvdata(indio_dev, st);
> +	indio_dev->name = name;
> +	indio_dev->info = &inv_icm42607_accel_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = inv_icm42607_accel_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(inv_icm42607_accel_channels);
> +	indio_dev->available_scan_masks = inv_icm42607_accel_scan_masks;

[Severity: Medium]
Since INDIO_DIRECT_MODE is the only mode configured and there is no buffer 
setup (like devm_iio_kfifo_buffer_setup), does this make the buffer-specific 
logic like inv_icm42607_accel_update_scan_mode, available_scan_masks, and 
IIO_CHAN_SOFT_TIMESTAMP dead code? 

Also, if inv_icm42607_accel_update_scan_mode were to be called, it lacks a 
PM runtime acquisition, which could lead to accessing hardware registers on a 
suspended device.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c

[ ... ]

> +int inv_icm42607_set_accel_conf(struct inv_icm42607_state *st,
> +				struct inv_icm42607_sensor_conf *conf,
> +				unsigned int *sleep_ms)
> +{

[ ... ]

> +	if (conf->filter != oldconf->filter) {
> +		if (conf->mode == INV_ICM42607_SENSOR_MODE_LOW_POWER) {
> +			val = FIELD_PREP(INV_ICM42607_ACCEL_CONFIG1_AVG_MASK, conf->filter);
> +			ret = regmap_update_bits(st->map, INV_ICM42607_REG_ACCEL_CONFIG1,
> +						 INV_ICM42607_ACCEL_CONFIG1_AVG_MASK, val);
> +		} else {
> +			val = FIELD_PREP(INV_ICM42607_ACCEL_CONFIG1_FILTER_MASK,
> +					 conf->filter);

[Severity: Medium]
Since conf->mode is initialized to LOW_NOISE, this branch will apply the 
filter value (3 for AVG_16X) to INV_ICM42607_ACCEL_CONFIG1_FILTER_MASK. 

Value 3 in the bandwidth enum corresponds to INV_ICM42607_FILTER_BW_73HZ. 
Will this silently set a 73Hz bandwidth instead of the intended 16x 
averaging?

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