Re: [PATCH V10 6/9] iio: imu: inv_icm42607: Add Temp Support in icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The temperature sensor is explicitly enabled for a one-shot sysfs read but is never disabled, resulting in a permanent 77ms latency penalty on all subsequent runtime resumes, the disabling of the low-power RC oscillator clock, and increased power consumption during active states.
--

commit 4c6f897d3d27fc0ab2a068b2b3d0736751dc6150
Author: Chris Morgan <macromorgan@hotmail.com>

    iio: imu: inv_icm42607: Add Temp Support in icm42607

This commit adds support for reading temperature sensor data to the
inv_icm42607 driver. It introduces functions to configure the temperature
sensor and read its raw data values.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c

[ ... ]

> +static int inv_icm42607_temp_read(struct inv_icm42607_state *st, s16 *temp)
> +{
> +	struct device *dev = regmap_get_device(st->map);
> +	__be16 *raw;
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = inv_icm42607_set_temp_conf(st, true, NULL);
> +	if (ret)
> +		return ret;
> +
> +	raw = &st->buffer[0];
> +	ret = regmap_bulk_read(st->map, INV_ICM42607_REG_TEMP_DATA1,
> +			       raw, sizeof(*raw));

[Severity: High]
Does this leave the temperature sensor permanently enabled?

Calling inv_icm42607_set_temp_conf() with true sets the sensor to active,
but it doesn't appear to be disabled after the read completes or fails.

If the temperature sensor is left enabled, st->conf.temp_en remains true.
On subsequent runtime resumes, inv_icm42607_set_pwr_mgmt0() would be
called with temp = true. This appears to force a 77ms sleep
(INV_ICM42607_TEMP_STARTUP_TIME_MS) on every resume, rather than the
standard 10ms for accelerometer wakeups.

Additionally, since temp is permanently true, the low-power RC oscillator
clock (INV_ICM42607_PWR_MGMT0_ACCEL_LP_CLK_SEL) might be prevented from
being selected, leading to continuously increased power consumption during
all active states.

Should there be a call to inv_icm42607_set_temp_conf(st, false, NULL) before
returning to restore the previous power state?

> +	if (ret)
> +		return ret;
> +
> +	*temp = be16_to_cpup(raw);
> +	if (*temp == INV_ICM42607_DATA_INVALID)
> +		return -EINVAL;
> +
> +	return 0;
> +}

[ ... ]

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