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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing synchronous suspend during driver unbind leaves hardware sensors powered on and leaking power.
- [Medium] Unconditional 45ms sleep during gyro power-off injects unnecessary delay on every suspend transition.
- [Low] Incorrect constant used for defining the autosuspend delay.
--

commit c053e5ed4a86d71f412c402ec80e9602b6677c2a
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.h b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
[ ... ]
> @@ -334,11 +345,18 @@ struct inv_icm42607_state {
>  #define INV_ICM42607_GYRO_STOP_TIME_MS			45
>  #define INV_ICM42607_TEMP_STARTUP_TIME_MS		77
>  
> +/*
> + * Suspend delay assumed from other icm42600 series device, not
> + * documented in datasheet.
> + */
> +#define INV_ICM42607_SUSPEND_DELAY_MS			(2 * USEC_PER_MSEC)

[Severity: Low]
Is this using the correct conversion macro? While (2 * USEC_PER_MSEC)
evaluates to 2000, USEC_PER_MSEC means microseconds per millisecond. Using
it to convert seconds to milliseconds is semantically confusing. Should this be
2 * MSEC_PER_SEC or just the literal 2000?

> +
>  typedef int (*inv_icm42607_bus_setup)(struct inv_icm42607_state *);
>  
>  extern const struct regmap_config inv_icm42607_regmap_config;
> 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
[ ... ]
> @@ -103,6 +105,63 @@ const struct inv_icm42607_hw inv_icm42607p_hw_data = {
>  };
>  EXPORT_SYMBOL_NS_GPL(inv_icm42607p_hw_data, "IIO_ICM42607");
>  
> +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)
> +{
> +	enum inv_icm42607_sensor_mode oldaccel = st->conf.accel.mode;
> +	enum inv_icm42607_sensor_mode oldgyro = st->conf.gyro.mode;
> +	bool oldtemp = st->conf.temp_en;
> +	unsigned int sleepval_ms;
> +	unsigned int val;
> +	int ret;
> +
> +	if (gyro == oldgyro && accel == oldaccel && temp == oldtemp)
> +		return 0;
> +
> +	/*
> +	 * Datasheet on page 14.26 says we need to ensure the gyro sensor is on
> +	 * for a minimum of 45ms. So if we transition from an on state to an
> +	 * off state wait 45ms to ensure a sufficient pause before power off.
> +	 */
> +	if (!gyro && oldgyro)
> +		fsleep(INV_ICM42607_GYRO_STOP_TIME_MS * USEC_PER_MSEC);

[Severity: Medium]
Does this inject a 45ms delay into every runtime suspend transition? 

The code unconditionally sleeps for 45ms when turning off the gyro.
If the datasheet requires the gyro to remain ON for a minimum of 45ms
after power-on, wouldn't it be better to track the power-on timestamp
instead of delaying every power-off sequence, even if the gyro has
been active for hours?

> +
> +	val = FIELD_PREP(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, gyro);
> +	val |= FIELD_PREP(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, accel);
[ ... ]
> @@ -271,10 +342,78 @@ int inv_icm42607_core_probe(struct regmap *regmap,
>  	if (ret)
>  		return ret;
>  
> +	ret = devm_pm_runtime_set_active_enabled(dev);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_set_autosuspend_delay(dev, INV_ICM42607_SUSPEND_DELAY_MS);
> +	pm_runtime_use_autosuspend(dev);
> +
>  	return 0;
>  }

[Severity: High]
Does this code leak power on unbind? 

Since autosuspend is used, the device doesn't suspend synchronously when
pm_runtime_put_sync() is called by the driver core on removal. Instead, an
async autosuspend is queued. 

When the devres cleanup action invokes pm_runtime_disable(), the pending
autosuspend work is cancelled, meaning inv_icm42607_runtime_suspend() is
never executed.

If the sensors remain fully powered on in the PWR_MGMT0 register, and the
vddio regulator is shared with other devices, will this cause a permanent
power leak? Would adding a driver remove callback that calls
pm_runtime_dont_use_autosuspend() or pm_runtime_put_sync_suspend()
ensure the device powers down synchronously?

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