Re: [PATCH v8 4/7] iio: imu: inv_icm42607: Initialize gyro based on chip_info

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] iio: imu: inv_icm42607: missing has_gyro check in inv_icm42607_temp_read()
--

commit bf4e9cd0c7e66bdca94f78eae37c6d71ba866c11
Author: Kanak Shilledar <kanak.shilledar@axis.com>

iio: imu: inv_icm42607: Initialize gyro based on chip_info

This commit adds a has_gyro boolean to struct inv_icm42607_hw to guard
gyroscope register accesses and initialization on accelerometer-only devices.
It also replaces implicit sensor mode boolean checks with explicit comparisons
against INV_ICM42607_SENSOR_MODE_OFF.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index f4ef75da22c76..84a7a7718dc2e 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c

[ ... ]

> @@ -160,7 +162,11 @@ int inv_icm42607_get_pwr_mgmt0(struct inv_icm42607_state *st,
>  	if (ret)
>  		return ret;
>  
> -	*gyro = FIELD_GET(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, val);
> +	if (st->hw->has_gyro)
> +		*gyro = FIELD_GET(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, val);
> +	else
> +		*gyro = INV_ICM42607_SENSOR_MODE_OFF;
> +
>  	*accel = FIELD_GET(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, val);
>  
>  	return 0;

[Severity: Medium]
Should inv_icm42607_temp_read() in drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c
also use inv_icm42607_get_pwr_mgmt0() and explicit comparisons against
INV_ICM42607_SENSOR_MODE_OFF?

Currently, inv_icm42607_temp_read() open-codes reading INV_ICM42607_REG_PWR_MGMT0
and unconditionally extracts INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK without
checking st->hw->has_gyro:

drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c:inv_icm42607_temp_read() {
	...
	ret = regmap_read(st->map, INV_ICM42607_REG_PWR_MGMT0, &val);
	if (ret)
		return ret;

	accel_mode = FIELD_GET(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, val);
	gyro_mode = FIELD_GET(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, val);
	if (!gyro_mode && !accel_mode) {
		/* enable accel sensor */
		conf.mode = INV_ICM42607_SENSOR_MODE_LOW_NOISE;
		ret = inv_icm42607_set_sensor_conf(st, &conf, IIO_ACCEL);
		if (ret)
			return ret;
	}
	...
}

When reading the temperature channel via inv_icm42607_accel_read_raw() ->
inv_icm42607_temp_read_raw() -> inv_icm42607_temp_read() on an
accelerometer-only device where st->hw->has_gyro is false, if the reserved
gyro mode bits in INV_ICM42607_REG_PWR_MGMT0 read as non-zero while the
accelerometer is off, inv_icm42607_temp_read() would skip enabling the
accelerometer and fail with -EINVAL when INV_ICM42607_REG_TEMP_DATA1 returns
INV_ICM42607_DATA_INVALID.

Could inv_icm42607_temp_read() call inv_icm42607_get_pwr_mgmt0() instead of
open-coding the register read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-b4-inv_icm42370p-v8-0-b44eb96020b6@axis.com?part=4