Re: [PATCH togreg v3 2/2] iio: imu: inv_icm42607: restore runtime PM on system resume errors

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 11 Aug 2026 18:33:01 +0800
Linmao Li <lilinmao@kylinos.cn> wrote:

> pm_runtime_force_suspend() leaves runtime PM disabled after it succeeds and
> expects pm_runtime_force_resume() to restore runtime PM management during
> system resume.
> 
> The resume callback returns early if enabling the vddio regulator or
> synchronizing the register cache fails, skipping the matching
> pm_runtime_force_resume() call. Runtime PM consequently remains disabled
> after the system has resumed, so runtime autosuspend can no longer turn off
> sensors enabled afterward.
> 
> Call pm_runtime_force_resume() on both error paths. Keep the first error as
> the return value and report a runtime PM restore failure separately.
> 
> Fixes: 3007c1530f96 ("iio: imu: inv_icm42607: Add PM support for icm42607")
> Signed-off-by: Linmao Li <lilinmao@kylinos.cn>

Sashiko has some comments on this:
https://sashiko.dev/#/patchset/20260811103301.1157404-1-lilinmao%40kylinos.cn

I would note that in some paths error handling is best effort.
There isn't always a sequence that leaves us in a remotely
useful state.  So maybe what you have here is the best we can do
even though it is a bit crazy to expect the driver to do anything
useful if it can't power the device.

> ---
> Changes since v2:
> - Restructure inv_icm42607_resume() along the lines Andy suggested:
>   handle the error case in its own block and call
>   pm_runtime_force_resume() directly on the success path.  No
>   functional change.
> 
> Changes since v1:
> - Split the device side of inv_icm42607_resume() into a helper so the
>   PM bookkeeping stays in the wrapper.  No functional change.
> 
>  .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 23 +++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 0da362967f63b..f4ef75da22c76 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> @@ -664,9 +664,8 @@ static int inv_icm42607_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static int inv_icm42607_resume(struct device *dev)
> +static int inv_icm42607_resume_core(struct inv_icm42607_state *st)
>  {
> -	struct inv_icm42607_state *st = dev_get_drvdata(dev);
>  	int ret;
>  
>  	ret = inv_icm42607_enable_vddio_reg(st);
> @@ -675,9 +674,25 @@ static int inv_icm42607_resume(struct device *dev)
>  
>  	/* Sync the regcache again after regulator shutdown. */
>  	regcache_mark_dirty(st->map);
> -	ret = regcache_sync(st->map);
> -	if (ret)
> +
> +	return regcache_sync(st->map);
> +}
> +
> +static int inv_icm42607_resume(struct device *dev)
> +{
> +	struct inv_icm42607_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = inv_icm42607_resume_core(st);
> +	if (ret) {
> +		int rc;
> +
> +		rc = pm_runtime_force_resume(dev);
> +		if (rc)
> +			dev_warn(dev, "Failed to restore runtime PM state: %d\n", rc);
> +

There is a question from sashiko on whether this can be reached. 
Even though that may be the case I'd keep the the error print because
it hardens us against future changes.

>  		return ret;
> +	}
>  
>  	return pm_runtime_force_resume(dev);
>  }