Re: [PATCH V12 5/9] iio: imu: inv_icm42607: Add PM support for icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The driver incorrectly waits for the sensor's startup stabilization time (up to 30ms) when powering OFF the sensor, adding unnecessary latency to the runtime suspend path.
--

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

iio: imu: inv_icm42607: Add PM support for icm42607
    
Add power management support for the ICM42607 device driver.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 5d40f1ee53d6d..8073317088ec0 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
[ ... ]
> +static int inv_icm42607_set_pwr_mgmt0(struct inv_icm42607_state *st,
> +				      enum inv_icm42607_sensor_mode gyro,
> +				      enum inv_icm42607_sensor_mode accel,
> +				      bool temp, unsigned int *sleep_ms)
> +{
[ ... ]
> +	sleepval_ms = 0;
> +	if (temp && !oldtemp)
> +		sleepval_ms = max(sleepval_ms, INV_ICM42607_TEMP_STARTUP_TIME_MS);
> +
> +	if (accel != oldaccel)
> +		sleepval_ms = max(sleepval_ms, INV_ICM42607_ACCEL_STARTUP_TIME_MS);
> +
> +	if (gyro != oldgyro)
> +		sleepval_ms = max(sleepval_ms, INV_ICM42607_GYRO_STARTUP_TIME_MS);
> +
> +	if (sleep_ms)
> +		*sleep_ms = sleepval_ms;
> +	else if (sleepval_ms)
> +		fsleep(sleepval_ms * USEC_PER_MSEC);
> +
> +	return 0;
> +}

[Severity: Medium]
Does this unintentionally add a delay when powering off the sensors?

While the temperature check correctly verifies if the sensor is being turned on
(temp && !oldtemp), the accel and gyro checks only verify that the state has
changed (accel != oldaccel).

When inv_icm42607_runtime_suspend() calls this function to power down the
sensors:

[ ... ]
> +static int inv_icm42607_runtime_suspend(struct device *dev)
> +{
> +	struct inv_icm42607_state *st = dev_get_drvdata(dev);
> +
> +	guard(mutex)(&st->lock);
> +
> +	/* save sensors state */
> +	st->suspended.gyro = st->conf.gyro.mode;
> +	st->suspended.accel = st->conf.accel.mode;
> +	st->suspended.temp = st->conf.temp_en;
> +
> +	return inv_icm42607_set_pwr_mgmt0(st, INV_ICM42607_SENSOR_MODE_OFF,
> +					  INV_ICM42607_SENSOR_MODE_OFF, false,
> +					  NULL);
> +}

The accel != oldaccel and gyro != oldgyro conditions will evaluate to true if
the sensors were previously active. This appears to cause the runtime suspend
path to needlessly sleep for the startup stabilization time (up to 30ms) while
powering down.

Could the delay logic be updated to only sleep when the new mode is not
INV_ICM42607_SENSOR_MODE_OFF?

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