[PATCH 0/6] iio: imu: inv_icm42600: pm_runtime fixes + various changes

[PATCH 0/6] iio: imu: inv_icm42600: pm_runtime fixes + various changes

[Sean Nyekjaer]

This series was triggered by "Runtime PM usage count underflow!" when
unloading the module(s).
By testing the driver in various use cases and reading code it was
obvious that it could need some tiding up.

I'm still not 100% satisfied with suspend/resume is calling directly to
vddio_enable/disable. In my mind it should be managed by pm_runtime.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
Sean Nyekjaer (6):
      iio: imu: inv_icm42600: Use inv_icm42600_disable_vddio_reg()
      iio: imu: inv_icm42600: Use devm_regulator_get_enable() for vdd regulator
      iio: imu: inv_icm42600: Remove redundant error msg on regulator_disable()
      iio: imu: inv_icm42600: Simplify pm_runtime setup
      iio: imu: inv_icm42600: Drop redundant pm_runtime reinitialization in resume
      iio: imu: inv_icm42600: Avoid configuring if already pm_runtime suspended

 drivers/iio/imu/inv_icm42600/inv_icm42600.h      |  1 -
 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 54 ++++++------------------
 2 files changed, 14 insertions(+), 41 deletions(-)
---
base-commit: 3e28fa06444e7031aba0b3552cce332b776fe267
change-id: 20250708-icm42pmreg-24d824d978c4

Best regards,
-- 
Sean Nyekjaer <sean@geanix.com>

[PATCH 1/6] iio: imu: inv_icm42600: Use inv_icm42600_disable_vddio_reg()

[Sean Nyekjaer]

Replace direct calls to regulator_disable() with the existing
inv_icm42600_disable_vddio_reg() helper. This improves consistency
and ensures any future changes to the disable logic are centralized.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index a4d42e7e21807f7954def431e9cf03dffaa5bd5e..69496d1c1ff73132f5e7bd076d18a62c293285a2 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -892,7 +892,7 @@ static int inv_icm42600_suspend(struct device *dev)
 
 	/* disable vddio regulator if chip is sleeping */
 	if (!wakeup)
-		regulator_disable(st->vddio_supply);
+		inv_icm42600_disable_vddio_reg(st);
 
 out_unlock:
 	mutex_unlock(&st->lock);
@@ -973,7 +973,7 @@ static int inv_icm42600_runtime_suspend(struct device *dev)
 	if (ret)
 		goto error_unlock;
 
-	regulator_disable(st->vddio_supply);
+	inv_icm42600_disable_vddio_reg(st);
 
 error_unlock:
 	mutex_unlock(&st->lock);

-- 
2.50.0

Re: [PATCH 1/6] iio: imu: inv_icm42600: Use inv_icm42600_disable_vddio_reg()

[Jonathan Cameron]

On Wed, 09 Jul 2025 14:35:09 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> Replace direct calls to regulator_disable() with the existing
> inv_icm42600_disable_vddio_reg() helper. This improves consistency
> and ensures any future changes to the disable logic are centralized.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>

That function only exists to match type for the devm_ callback
at the cost of using type safety.  So I'm not sure loosing type
safety in more of the code is a good direction to go in.

> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index a4d42e7e21807f7954def431e9cf03dffaa5bd5e..69496d1c1ff73132f5e7bd076d18a62c293285a2 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -892,7 +892,7 @@ static int inv_icm42600_suspend(struct device *dev)
>  
>  	/* disable vddio regulator if chip is sleeping */
>  	if (!wakeup)
> -		regulator_disable(st->vddio_supply);
> +		inv_icm42600_disable_vddio_reg(st);
>  
>  out_unlock:
>  	mutex_unlock(&st->lock);
> @@ -973,7 +973,7 @@ static int inv_icm42600_runtime_suspend(struct device *dev)
>  	if (ret)
>  		goto error_unlock;
>  
> -	regulator_disable(st->vddio_supply);
> +	inv_icm42600_disable_vddio_reg(st);
>  
>  error_unlock:
>  	mutex_unlock(&st->lock);
> 

[PATCH 2/6] iio: imu: inv_icm42600: Use devm_regulator_get_enable() for vdd regulator

[Sean Nyekjaer]

The vdd regulator is not used for runtime power management, so it does
not need explicit enable/disable handling.
Use devm_regulator_get_enable() to let the regulator be managed
automatically by devm.

This simplifies the code by removing the manual enable and cleanup
logic.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/imu/inv_icm42600/inv_icm42600.h      |  1 -
 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 25 ++++--------------------
 2 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
index 1430ab4f1dea5d5ba6277d74275fc44a6cd30eb8..c8b48a5c5ed0677bb35201d32934936faaf7a1a6 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
@@ -167,7 +167,6 @@ struct inv_icm42600_state {
 	enum inv_icm42600_chip chip;
 	const char *name;
 	struct regmap *map;
-	struct regulator *vdd_supply;
 	struct regulator *vddio_supply;
 	int irq;
 	struct iio_mount_matrix orientation;
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index 69496d1c1ff73132f5e7bd076d18a62c293285a2..35f7c66d77790829a739d2c54ba77e53903a9297 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -697,17 +697,6 @@ static int inv_icm42600_enable_regulator_vddio(struct inv_icm42600_state *st)
 	return 0;
 }
 
-static void inv_icm42600_disable_vdd_reg(void *_data)
-{
-	struct inv_icm42600_state *st = _data;
-	const struct device *dev = regmap_get_device(st->map);
-	int ret;
-
-	ret = regulator_disable(st->vdd_supply);
-	if (ret)
-		dev_err(dev, "failed to disable vdd error %d\n", ret);
-}
-
 static void inv_icm42600_disable_vddio_reg(void *_data)
 {
 	struct inv_icm42600_state *st = _data;
@@ -773,23 +762,17 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip,
 		return ret;
 	}
 
-	st->vdd_supply = devm_regulator_get(dev, "vdd");
-	if (IS_ERR(st->vdd_supply))
-		return PTR_ERR(st->vdd_supply);
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to get vdd regulator\n");
 
 	st->vddio_supply = devm_regulator_get(dev, "vddio");
 	if (IS_ERR(st->vddio_supply))
 		return PTR_ERR(st->vddio_supply);
 
-	ret = regulator_enable(st->vdd_supply);
-	if (ret)
-		return ret;
 	msleep(INV_ICM42600_POWER_UP_TIME_MS);
 
-	ret = devm_add_action_or_reset(dev, inv_icm42600_disable_vdd_reg, st);
-	if (ret)
-		return ret;
-
 	ret = inv_icm42600_enable_regulator_vddio(st);
 	if (ret)
 		return ret;

-- 
2.50.0

Re: [PATCH 2/6] iio: imu: inv_icm42600: Use devm_regulator_get_enable() for vdd regulator

[Jonathan Cameron]

On Wed, 09 Jul 2025 14:35:10 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> The vdd regulator is not used for runtime power management, so it does
> not need explicit enable/disable handling.
> Use devm_regulator_get_enable() to let the regulator be managed
> automatically by devm.
> 
> This simplifies the code by removing the manual enable and cleanup
> logic.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
In general good, but I think it has a small code organisation side
effect we probably don't want!

> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600.h      |  1 -
>  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 25 ++++--------------------
>  2 files changed, 4 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> index 1430ab4f1dea5d5ba6277d74275fc44a6cd30eb8..c8b48a5c5ed0677bb35201d32934936faaf7a1a6 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -167,7 +167,6 @@ struct inv_icm42600_state {
>  	enum inv_icm42600_chip chip;
>  	const char *name;
>  	struct regmap *map;
> -	struct regulator *vdd_supply;
>  	struct regulator *vddio_supply;
>  	int irq;
>  	struct iio_mount_matrix orientation;
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 69496d1c1ff73132f5e7bd076d18a62c293285a2..35f7c66d77790829a739d2c54ba77e53903a9297 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -697,17 +697,6 @@ static int inv_icm42600_enable_regulator_vddio(struct inv_icm42600_state *st)
>  	return 0;
>  }
>  
> -static void inv_icm42600_disable_vdd_reg(void *_data)
> -{
> -	struct inv_icm42600_state *st = _data;
> -	const struct device *dev = regmap_get_device(st->map);
> -	int ret;
> -
> -	ret = regulator_disable(st->vdd_supply);
> -	if (ret)
> -		dev_err(dev, "failed to disable vdd error %d\n", ret);
> -}
> -
>  static void inv_icm42600_disable_vddio_reg(void *_data)
>  {
>  	struct inv_icm42600_state *st = _data;
> @@ -773,23 +762,17 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip,
>  		return ret;
>  	}
>  
> -	st->vdd_supply = devm_regulator_get(dev, "vdd");
> -	if (IS_ERR(st->vdd_supply))
> -		return PTR_ERR(st->vdd_supply);
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get vdd regulator\n");
>  
>  	st->vddio_supply = devm_regulator_get(dev, "vddio");
>  	if (IS_ERR(st->vddio_supply))
>  		return PTR_ERR(st->vddio_supply);
>  
> -	ret = regulator_enable(st->vdd_supply);
> -	if (ret)
> -		return ret;
>  	msleep(INV_ICM42600_POWER_UP_TIME_MS);
This sleep is associated with the enabling of vdd.  So we probably want
to keep those together.  So either move this up, or pull the
vdd handling down to just above this.

Thanks,

Jonathan

>  
> -	ret = devm_add_action_or_reset(dev, inv_icm42600_disable_vdd_reg, st);
> -	if (ret)
> -		return ret;
> -
>  	ret = inv_icm42600_enable_regulator_vddio(st);
>  	if (ret)
>  		return ret;
> 

[PATCH 3/6] iio: imu: inv_icm42600: Remove redundant error msg on regulator_disable()

[Sean Nyekjaer]

The regulator framework already emits an error message when
regulator_disable() fails, making the local dev_err() redundant.
Remove the duplicate message to avoid cluttering the kernel log
with the same error twice.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index 35f7c66d77790829a739d2c54ba77e53903a9297..55a29b1e2b11355598b0ede7af22857aed3ae134 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -700,12 +700,8 @@ static int inv_icm42600_enable_regulator_vddio(struct inv_icm42600_state *st)
 static void inv_icm42600_disable_vddio_reg(void *_data)
 {
 	struct inv_icm42600_state *st = _data;
-	const struct device *dev = regmap_get_device(st->map);
-	int ret;
 
-	ret = regulator_disable(st->vddio_supply);
-	if (ret)
-		dev_err(dev, "failed to disable vddio error %d\n", ret);
+	regulator_disable(st->vddio_supply);
 }
 
 static void inv_icm42600_disable_pm(void *_data)

-- 
2.50.0

Re: [PATCH 3/6] iio: imu: inv_icm42600: Remove redundant error msg on regulator_disable()

[Andy Shevchenko]

On Wed, Jul 09, 2025 at 02:35:11PM +0200, Sean Nyekjaer wrote:
> The regulator framework already emits an error message when
> regulator_disable() fails, making the local dev_err() redundant.
> Remove the duplicate message to avoid cluttering the kernel log
> with the same error twice.

To me this sounds like a potential backporting material as it might full
the logs (in case the module probed-removed zillion of time. Hence,
I would put it to be the first patch in the series (yes, it will involve
to fix something that you are removing in the following change, but still).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/6] iio: imu: inv_icm42600: Remove redundant error msg on regulator_disable()

[Sean Nyekjaer]

On Thu, Jul 10, 2025 at 12:03:26PM +0100, Andy Shevchenko wrote:
> On Wed, Jul 09, 2025 at 02:35:11PM +0200, Sean Nyekjaer wrote:
> > The regulator framework already emits an error message when
> > regulator_disable() fails, making the local dev_err() redundant.
> > Remove the duplicate message to avoid cluttering the kernel log
> > with the same error twice.
> 
> To me this sounds like a potential backporting material as it might full
> the logs (in case the module probed-removed zillion of time. Hence,
> I would put it to be the first patch in the series (yes, it will involve
> to fix something that you are removing in the following change, but still).

I have never seen this printed, so I don't think it's a huge issue.
But it's quite easy to add a Fixes tag if prefered.

> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 

/Sean

Re: [PATCH 3/6] iio: imu: inv_icm42600: Remove redundant error msg on regulator_disable()

[Andy Shevchenko]

On Thu, Jul 10, 2025 at 10:47:08AM +0000, Sean Nyekjaer wrote:
> On Thu, Jul 10, 2025 at 12:03:26PM +0100, Andy Shevchenko wrote:
> > On Wed, Jul 09, 2025 at 02:35:11PM +0200, Sean Nyekjaer wrote:
> > > The regulator framework already emits an error message when
> > > regulator_disable() fails, making the local dev_err() redundant.
> > > Remove the duplicate message to avoid cluttering the kernel log
> > > with the same error twice.
> > 
> > To me this sounds like a potential backporting material as it might full
> > the logs (in case the module probed-removed zillion of time. Hence,
> > I would put it to be the first patch in the series (yes, it will involve
> > to fix something that you are removing in the following change, but still).
> 
> I have never seen this printed, so I don't think it's a huge issue.
> But it's quite easy to add a Fixes tag if prefered.

Or move it to the end and explain in cover letter that this doesn't fix
(apparently) any problem IRL.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/6] iio: imu: inv_icm42600: Remove redundant error msg on regulator_disable()

[Jonathan Cameron]

On Wed, 09 Jul 2025 14:35:11 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> The regulator framework already emits an error message when
> regulator_disable() fails, making the local dev_err() redundant.
> Remove the duplicate message to avoid cluttering the kernel log
> with the same error twice.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 35f7c66d77790829a739d2c54ba77e53903a9297..55a29b1e2b11355598b0ede7af22857aed3ae134 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -700,12 +700,8 @@ static int inv_icm42600_enable_regulator_vddio(struct inv_icm42600_state *st)
>  static void inv_icm42600_disable_vddio_reg(void *_data)
>  {
>  	struct inv_icm42600_state *st = _data;
> -	const struct device *dev = regmap_get_device(st->map);
> -	int ret;
>  
> -	ret = regulator_disable(st->vddio_supply);
> -	if (ret)
> -		dev_err(dev, "failed to disable vddio error %d\n", ret);

Dropping this message (which is sensible) reinforces my lack of warm
fuzzy feelings about patch 1.  I'd definitely leave those other calls
alone and keep this as just a tight wrapper around st.  Note though that
can just pass the regulator as the parameter to the devm_add_action_or_reset()
and simplify this a bit more now you don't need to get to the dev.

> +	regulator_disable(st->vddio_supply);
>  }
>  
>  static void inv_icm42600_disable_pm(void *_data)
> 

[PATCH 4/6] iio: imu: inv_icm42600: Simplify pm_runtime setup

[Sean Nyekjaer]

Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
calls during probe. These are not required when the device is marked
active via pm_runtime_set_active() before enabling pm_runtime with
pm_runtime_enable().

Also remove the redundant pm_runtime_put_sync() call from the cleanup
path, since the core is not incrementing the usage count beforehand.

This simplifies the PM setup and avoids manipulating the usage counter
unnecessarily.

Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index 55a29b1e2b11355598b0ede7af22857aed3ae134..1072bea11c73d09a9a0e6ea9d4a5c7a72248dca7 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -708,7 +708,6 @@ static void inv_icm42600_disable_pm(void *_data)
 {
 	struct device *dev = _data;
 
-	pm_runtime_put_sync(dev);
 	pm_runtime_disable(dev);
 }
 
@@ -806,11 +805,10 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip,
 	ret = pm_runtime_set_active(dev);
 	if (ret)
 		return ret;
-	pm_runtime_get_noresume(dev);
+
 	pm_runtime_enable(dev);
 	pm_runtime_set_autosuspend_delay(dev, INV_ICM42600_SUSPEND_DELAY_MS);
 	pm_runtime_use_autosuspend(dev);
-	pm_runtime_put(dev);
 
 	return devm_add_action_or_reset(dev, inv_icm42600_disable_pm, dev);
 }

-- 
2.50.0

Re: [PATCH 4/6] iio: imu: inv_icm42600: Simplify pm_runtime setup

[Andy Shevchenko]

On Wed, Jul 09, 2025 at 02:35:12PM +0200, Sean Nyekjaer wrote:
> Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
> calls during probe. These are not required when the device is marked
> active via pm_runtime_set_active() before enabling pm_runtime with
> pm_runtime_enable().

Hmm... What will happen if the autosuspend triggers just before going out from
the probe when this change is applied?

> Also remove the redundant pm_runtime_put_sync() call from the cleanup
> path, since the core is not incrementing the usage count beforehand.

This is interesting. Have anybody actually tried to see refcount WARN about this?

> This simplifies the PM setup and avoids manipulating the usage counter
> unnecessarily.

> Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>

This should be the first, or close to the beginning of the series, patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/6] iio: imu: inv_icm42600: Simplify pm_runtime setup

[Sean Nyekjaer]

Hi Andy,

On Thu, Jul 10, 2025 at 12:08:40PM +0100, Andy Shevchenko wrote:
> On Wed, Jul 09, 2025 at 02:35:12PM +0200, Sean Nyekjaer wrote:
> > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
> > calls during probe. These are not required when the device is marked
> > active via pm_runtime_set_active() before enabling pm_runtime with
> > pm_runtime_enable().
> 
> Hmm... What will happen if the autosuspend triggers just before going out from
> the probe when this change is applied?

Nothing, as pm_runtime is enabled as the last step in probe.

> 
> > Also remove the redundant pm_runtime_put_sync() call from the cleanup
> > path, since the core is not incrementing the usage count beforehand.
> 
> This is interesting. Have anybody actually tried to see refcount WARN about this?
> 
> > This simplifies the PM setup and avoids manipulating the usage counter
> > unnecessarily.
> 
> > Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> 
> This should be the first, or close to the beginning of the series, patch.

Ok, but help me understand why?

> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 

Thanks for the review :)

/Sean

Re: [PATCH 4/6] iio: imu: inv_icm42600: Simplify pm_runtime setup

[Andy Shevchenko]

On Thu, Jul 10, 2025 at 10:45:43AM +0000, Sean Nyekjaer wrote:
> On Thu, Jul 10, 2025 at 12:08:40PM +0100, Andy Shevchenko wrote:
> > On Wed, Jul 09, 2025 at 02:35:12PM +0200, Sean Nyekjaer wrote:
> > > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
> > > calls during probe. These are not required when the device is marked
> > > active via pm_runtime_set_active() before enabling pm_runtime with
> > > pm_runtime_enable().
> > 
> > Hmm... What will happen if the autosuspend triggers just before going out from
> > the probe when this change is applied?
> 
> Nothing, as pm_runtime is enabled as the last step in probe.

Note, that PM runtime can be enabled by userspace or disabled.

> > > Also remove the redundant pm_runtime_put_sync() call from the cleanup
> > > path, since the core is not incrementing the usage count beforehand.
> > 
> > This is interesting. Have anybody actually tried to see refcount WARN about this?
> > 
> > > This simplifies the PM setup and avoids manipulating the usage counter
> > > unnecessarily.
> > 
> > > Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
> > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > 
> > This should be the first, or close to the beginning of the series, patch.
> 
> Ok, but help me understand why?

The commits that are marked as Fixes should be easy to backport. When
the series is started with the refactoring, it's not good as potentially
backporting material might (even undeliberately) get a dependency on the
refactoring.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/6] iio: imu: inv_icm42600: Simplify pm_runtime setup

[Jonathan Cameron]

On Wed, 09 Jul 2025 14:35:12 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
> calls during probe. These are not required when the device is marked
> active via pm_runtime_set_active() before enabling pm_runtime with
> pm_runtime_enable().
> 
> Also remove the redundant pm_runtime_put_sync() call from the cleanup
> path, since the core is not incrementing the usage count beforehand.
> 
> This simplifies the PM setup and avoids manipulating the usage counter
> unnecessarily.

Could we switch directly to using devm_pm_runtime_enable() for this driver?

At first glance looks like this code is missing the disable of autosuspend
that should be there (which devm_pm_runtime_enable() will also handle).


> 
> Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 55a29b1e2b11355598b0ede7af22857aed3ae134..1072bea11c73d09a9a0e6ea9d4a5c7a72248dca7 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -708,7 +708,6 @@ static void inv_icm42600_disable_pm(void *_data)
>  {
>  	struct device *dev = _data;
>  
> -	pm_runtime_put_sync(dev);
>  	pm_runtime_disable(dev);
>  }
>  
> @@ -806,11 +805,10 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip,
>  	ret = pm_runtime_set_active(dev);
>  	if (ret)
>  		return ret;
> -	pm_runtime_get_noresume(dev);
> +
>  	pm_runtime_enable(dev);
>  	pm_runtime_set_autosuspend_delay(dev, INV_ICM42600_SUSPEND_DELAY_MS);
>  	pm_runtime_use_autosuspend(dev);
> -	pm_runtime_put(dev);
>  
>  	return devm_add_action_or_reset(dev, inv_icm42600_disable_pm, dev);
>  }
> 

Re: [PATCH 4/6] iio: imu: inv_icm42600: Simplify pm_runtime setup

[Sean Nyekjaer]

On Sun, Jul 13, 2025 at 03:28:10PM +0100, Jonathan Cameron wrote:
> On Wed, 09 Jul 2025 14:35:12 +0200
> Sean Nyekjaer <sean@geanix.com> wrote:
> 
> > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
> > calls during probe. These are not required when the device is marked
> > active via pm_runtime_set_active() before enabling pm_runtime with
> > pm_runtime_enable().
> >
> > Also remove the redundant pm_runtime_put_sync() call from the cleanup
> > path, since the core is not incrementing the usage count beforehand.
> >
> > This simplifies the PM setup and avoids manipulating the usage counter
> > unnecessarily.
> 
> Could we switch directly to using devm_pm_runtime_enable() for this driver?
> 
> At first glance looks like this code is missing the disable of autosuspend
> that should be there (which devm_pm_runtime_enable() will also handle).
> 

I have tried to use devm_pm_runtime_enable() but on rmmod it warns
"unbalanced disables for regulator"

If I remove this:
-	ret = devm_add_action_or_reset(dev, inv_icm42600_disable_vddio_reg, st);
-	if (ret)
-		return ret;

Everything seems okay again. I have checked with printk's that
inv_icm42600_disable_vddio_reg() is called twice with
devm_pm_runtime_enable() used.
Does it make sense?

/Sean

> 
> >
> > Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > ---
> >  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > index 55a29b1e2b11355598b0ede7af22857aed3ae134..1072bea11c73d09a9a0e6ea9d4a5c7a72248dca7 100644
> > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > @@ -708,7 +708,6 @@ static void inv_icm42600_disable_pm(void *_data)
> >  {
> >  	struct device *dev = _data;
> >
> > -	pm_runtime_put_sync(dev);
> >  	pm_runtime_disable(dev);
> >  }
> >
> > @@ -806,11 +805,10 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip,
> >  	ret = pm_runtime_set_active(dev);
> >  	if (ret)
> >  		return ret;
> > -	pm_runtime_get_noresume(dev);
> > +
> >  	pm_runtime_enable(dev);
> >  	pm_runtime_set_autosuspend_delay(dev, INV_ICM42600_SUSPEND_DELAY_MS);
> >  	pm_runtime_use_autosuspend(dev);
> > -	pm_runtime_put(dev);
> >
> >  	return devm_add_action_or_reset(dev, inv_icm42600_disable_pm, dev);
> >  }
> >
> 

Re: [PATCH 4/6] iio: imu: inv_icm42600: Simplify pm_runtime setup

[Sean Nyekjaer]

On Mon, Jul 14, 2025 at 07:24:57AM +0100, Sean Nyekjaer wrote:
> On Sun, Jul 13, 2025 at 03:28:10PM +0100, Jonathan Cameron wrote:
> > On Wed, 09 Jul 2025 14:35:12 +0200
> > Sean Nyekjaer <sean@geanix.com> wrote:
> > 
> > > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
> > > calls during probe. These are not required when the device is marked
> > > active via pm_runtime_set_active() before enabling pm_runtime with
> > > pm_runtime_enable().
> > >
> > > Also remove the redundant pm_runtime_put_sync() call from the cleanup
> > > path, since the core is not incrementing the usage count beforehand.
> > >
> > > This simplifies the PM setup and avoids manipulating the usage counter
> > > unnecessarily.
> > 
> > Could we switch directly to using devm_pm_runtime_enable() for this driver?
> > 
> > At first glance looks like this code is missing the disable of autosuspend
> > that should be there (which devm_pm_runtime_enable() will also handle).
> > 
> 
> I have tried to use devm_pm_runtime_enable() but on rmmod it warns
> "unbalanced disables for regulator"
> 
> If I remove this:
> -	ret = devm_add_action_or_reset(dev, inv_icm42600_disable_vddio_reg, st);
> -	if (ret)
> -		return ret;
> 
> Everything seems okay again. I have checked with printk's that
> inv_icm42600_disable_vddio_reg() is called twice with
> devm_pm_runtime_enable() used.
> Does it make sense?
> 
> /Sean

with pm_runtime_enable():
root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko
[ 3793.713077] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt
[ 3793.727728] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity...
[ 3793.737660] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator
[ 3793.856891] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator
[ 3793.866872] inv_icm42600_enable_regulator_vddio() enable vddio
[ 3793.920739] inv_icm42600_runtime_suspend() disable vddio
root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600
[ 3796.954850] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio()
[ 3796.954910] inv_icm42600_enable_regulator_vddio() enable vddio
[ 3796.985140] inv_icm42600_disable_vddio_reg() disable vddio

with devm_pm_runtime_enable():
root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko
[ 3852.873887] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt
[ 3852.888715] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity...
[ 3852.898514] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator
[ 3853.016890] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator
[ 3853.026860] inv_icm42600_enable_regulator_vddio() enable vddio
[ 3853.080835] inv_icm42600_runtime_suspend() disable vddio
root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600
[ 3854.448461] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio()
[ 3854.448540] inv_icm42600_enable_regulator_vddio() enable vddio
[ 3854.467061] inv_icm42600_runtime_suspend() disable vddio
[ 3854.477170] inv_icm42600_disable_vddio_reg() disable vddio
[ 3854.483835] ------------[ cut here ]------------
[ 3854.483912] WARNING: CPU: 0 PID: 582 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0
[ 3854.497853] unbalanced disables for regulator

Is the way from here to remove the devm_add_action_or_reset(dev,
inv_icm42600_disable_vddio_reg... ?

/Sean

Re: [PATCH 4/6] iio: imu: inv_icm42600: Simplify pm_runtime setup

[Jonathan Cameron]

On Mon, 14 Jul 2025 07:42:43 +0000
Sean Nyekjaer <sean@geanix.com> wrote:

> On Mon, Jul 14, 2025 at 07:24:57AM +0100, Sean Nyekjaer wrote:
> > On Sun, Jul 13, 2025 at 03:28:10PM +0100, Jonathan Cameron wrote:  
> > > On Wed, 09 Jul 2025 14:35:12 +0200
> > > Sean Nyekjaer <sean@geanix.com> wrote:
> > >   
> > > > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
> > > > calls during probe. These are not required when the device is marked
> > > > active via pm_runtime_set_active() before enabling pm_runtime with
> > > > pm_runtime_enable().
> > > >
> > > > Also remove the redundant pm_runtime_put_sync() call from the cleanup
> > > > path, since the core is not incrementing the usage count beforehand.
> > > >
> > > > This simplifies the PM setup and avoids manipulating the usage counter
> > > > unnecessarily.  
> > > 
> > > Could we switch directly to using devm_pm_runtime_enable() for this driver?
> > > 
> > > At first glance looks like this code is missing the disable of autosuspend
> > > that should be there (which devm_pm_runtime_enable() will also handle).
> > >   
> > 
> > I have tried to use devm_pm_runtime_enable() but on rmmod it warns
> > "unbalanced disables for regulator"
> > 
> > If I remove this:
> > -	ret = devm_add_action_or_reset(dev, inv_icm42600_disable_vddio_reg, st);
> > -	if (ret)
> > -		return ret;
> > 
> > Everything seems okay again. I have checked with printk's that
> > inv_icm42600_disable_vddio_reg() is called twice with
> > devm_pm_runtime_enable() used.
> > Does it make sense?
> > 
> > /Sean  
> 
> with pm_runtime_enable():
> root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko
> [ 3793.713077] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt
> [ 3793.727728] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity...
> [ 3793.737660] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator
> [ 3793.856891] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator
> [ 3793.866872] inv_icm42600_enable_regulator_vddio() enable vddio
> [ 3793.920739] inv_icm42600_runtime_suspend() disable vddio
> root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600
> [ 3796.954850] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio()
> [ 3796.954910] inv_icm42600_enable_regulator_vddio() enable vddio
> [ 3796.985140] inv_icm42600_disable_vddio_reg() disable vddio
> 
> with devm_pm_runtime_enable():
> root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko
> [ 3852.873887] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt
> [ 3852.888715] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity...
> [ 3852.898514] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator
> [ 3853.016890] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator
> [ 3853.026860] inv_icm42600_enable_regulator_vddio() enable vddio
> [ 3853.080835] inv_icm42600_runtime_suspend() disable vddio
> root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600
> [ 3854.448461] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio()
> [ 3854.448540] inv_icm42600_enable_regulator_vddio() enable vddio
> [ 3854.467061] inv_icm42600_runtime_suspend() disable vddio

As below What is the call path for this final suspend?
Is it coming from update_autosuspend()?

> [ 3854.477170] inv_icm42600_disable_vddio_reg() disable vddio
> [ 3854.483835] ------------[ cut here ]------------
> [ 3854.483912] WARNING: CPU: 0 PID: 582 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0
> [ 3854.497853] unbalanced disables for regulator
> 
> Is the way from here to remove the devm_add_action_or_reset(dev,
> inv_icm42600_disable_vddio_reg... ?

That will make a mess if runtime PM is not built into
the kernel which is why an PM state in remove should return to the state
before it was enabled in the first place (i.e. on!).
That final runtime suspend surprises me.

> 
> /Sean
> 

Re: [PATCH 4/6] iio: imu: inv_icm42600: Simplify pm_runtime setup

[Sean Nyekjaer]

On Wed, Jul 16, 2025 at 09:00:10AM +0100, Jonathan Cameron wrote:
> On Mon, 14 Jul 2025 07:42:43 +0000
> Sean Nyekjaer <sean@geanix.com> wrote:
> 
> > On Mon, Jul 14, 2025 at 07:24:57AM +0100, Sean Nyekjaer wrote:
> > > On Sun, Jul 13, 2025 at 03:28:10PM +0100, Jonathan Cameron wrote:
> > > > On Wed, 09 Jul 2025 14:35:12 +0200
> > > > Sean Nyekjaer <sean@geanix.com> wrote:
> > > >
> > > > > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
> > > > > calls during probe. These are not required when the device is marked
> > > > > active via pm_runtime_set_active() before enabling pm_runtime with
> > > > > pm_runtime_enable().
> > > > >
> > > > > Also remove the redundant pm_runtime_put_sync() call from the cleanup
> > > > > path, since the core is not incrementing the usage count beforehand.
> > > > >
> > > > > This simplifies the PM setup and avoids manipulating the usage counter
> > > > > unnecessarily.
> > > >
> > > > Could we switch directly to using devm_pm_runtime_enable() for this driver?
> > > >
> > > > At first glance looks like this code is missing the disable of autosuspend
> > > > that should be there (which devm_pm_runtime_enable() will also handle).
> > > >
> > >
> > > I have tried to use devm_pm_runtime_enable() but on rmmod it warns
> > > "unbalanced disables for regulator"
> > >
> > > If I remove this:
> > > -	ret = devm_add_action_or_reset(dev, inv_icm42600_disable_vddio_reg, st);
> > > -	if (ret)
> > > -		return ret;
> > >
> > > Everything seems okay again. I have checked with printk's that
> > > inv_icm42600_disable_vddio_reg() is called twice with
> > > devm_pm_runtime_enable() used.
> > > Does it make sense?
> > >
> > > /Sean
> >
> > with pm_runtime_enable():
> > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko
> > [ 3793.713077] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt
> > [ 3793.727728] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity...
> > [ 3793.737660] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator
> > [ 3793.856891] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator
> > [ 3793.866872] inv_icm42600_enable_regulator_vddio() enable vddio
> > [ 3793.920739] inv_icm42600_runtime_suspend() disable vddio
> > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600
> > [ 3796.954850] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio()
> > [ 3796.954910] inv_icm42600_enable_regulator_vddio() enable vddio
> > [ 3796.985140] inv_icm42600_disable_vddio_reg() disable vddio
> >
> > with devm_pm_runtime_enable():
> > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko
> > [ 3852.873887] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt
> > [ 3852.888715] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity...
> > [ 3852.898514] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator
> > [ 3853.016890] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator
> > [ 3853.026860] inv_icm42600_enable_regulator_vddio() enable vddio
> > [ 3853.080835] inv_icm42600_runtime_suspend() disable vddio
> > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600
> > [ 3854.448461] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio()
> > [ 3854.448540] inv_icm42600_enable_regulator_vddio() enable vddio
> > [ 3854.467061] inv_icm42600_runtime_suspend() disable vddio
> 
> As below What is the call path for this final suspend?
> Is it coming from update_autosuspend()?

Yeah it looks like pm_runtime_dont_use_autosuspend() is calling runtime_suspend().

root@v4:/data/root# rmmod inv-icm42600-i2c inv-icm42600
[  291.511085] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio()
[  291.511165] inv_icm42600_enable_regulator_vddio() enable vddio
[  291.532398] inv_icm42600_runtime_suspend() disable vddio
[  291.538517] CPU: 0 UID: 0 PID: 331 Comm: rmmod Tainted: G        W           6.16.0-rc1-00202-g7fe6e564b5c9-dirty #146 VOLUNTARY
[  291.538559] Tainted: [W]=WARN
[  291.538566] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
[  291.538575] Call trace:
[  291.538590]  unwind_backtrace from show_stack+0x10/0x14
[  291.538643]  show_stack from dump_stack_lvl+0x54/0x68
[  291.538679]  dump_stack_lvl from inv_icm42600_runtime_suspend+0x68/0x6c [inv_icm42600]
[  291.538728]  inv_icm42600_runtime_suspend [inv_icm42600] from __rpm_callback+0x48/0x18c
[  291.538775]  __rpm_callback from rpm_callback+0x5c/0x68
[  291.538815]  rpm_callback from rpm_suspend+0xdc/0x584
[  291.538853]  rpm_suspend from pm_runtime_disable_action+0x30/0x5c
[  291.538885]  pm_runtime_disable_action from devres_release_group+0x180/0x1a0
[  291.538917]  devres_release_group from i2c_device_remove+0x34/0x84
[  291.538949]  i2c_device_remove from device_release_driver_internal+0x180/0x1f4
[  291.538976]  device_release_driver_internal from driver_detach+0x54/0xa0
[  291.538998]  driver_detach from bus_remove_driver+0x58/0xa4
[  291.539030]  bus_remove_driver from sys_delete_module+0x16c/0x250
[  291.539065]  sys_delete_module from ret_fast_syscall+0x0/0x54
[  291.539089] Exception stack(0xd0ab1fa8 to 0xd0ab1ff0)
[  291.539108] 1fa0:                   be94fe46 be94fd1c 017ccfa4 00000800 0000000a 017ccf68
[  291.539126] 1fc0: be94fe46 be94fd1c 017ccf68 00000081 00000000 00000001 00000003 017cc190
[  291.539139] 1fe0: b6c8be41 be94fadc 000179cb b6c8be48
[  291.685102] inv_icm42600_disable_vddio_reg() disable vddio
[  291.694566] ------------[ cut here ]------------
[  291.694621] WARNING: CPU: 0 PID: 331 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0
[  291.708496] unbalanced disables for regulator-dummy
[  291.713391] Modules linked in: inv_icm42600_i2c(-) inv_icm42600 inv_sensors_timestamp [last unloaded: inv_icm42600]
[  291.723939] CPU: 0 UID: 0 PID: 331 Comm: rmmod Tainted: G        W           6.16.0-rc1-00202-g7fe6e564b5c9-dirty #146 VOLUNTARY
[  291.735620] Tainted: [W]=WARN
[  291.738598] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
[  291.744789] Call trace:
[  291.744807]  unwind_backtrace from show_stack+0x10/0x14
[  291.752614]  show_stack from dump_stack_lvl+0x54/0x68
[  291.757704]  dump_stack_lvl from __warn+0x7c/0xe0
[  291.762437]  __warn from warn_slowpath_fmt+0x124/0x18c
[  291.767602]  warn_slowpath_fmt from _regulator_disable+0x140/0x1a0
[  291.773812]  _regulator_disable from regulator_disable+0x48/0x80
[  291.779843]  regulator_disable from devres_release_group+0x180/0x1a0
[  291.786231]  devres_release_group from i2c_device_remove+0x34/0x84
[  291.792446]  i2c_device_remove from device_release_driver_internal+0x180/0x1f4
[  291.799699]  device_release_driver_internal from driver_detach+0x54/0xa0
[  291.806427]  driver_detach from bus_remove_driver+0x58/0xa4
[  291.812033]  bus_remove_driver from sys_delete_module+0x16c/0x250
[  291.818160]  sys_delete_module from ret_fast_syscall+0x0/0x54
[  291.823934] Exception stack(0xd0ab1fa8 to 0xd0ab1ff0)
[  291.829007] 1fa0:                   be94fe46 be94fd1c 017ccfa4 00000800 0000000a 017ccf68
[  291.837205] 1fc0: be94fe46 be94fd1c 017ccf68 00000081 00000000 00000001 00000003 017cc190
[  291.845397] 1fe0: b6c8be41 be94fadc 000179cb b6c8be48
[  291.850632] ---[ end trace 0000000000000000 ]---0

> 
> > [ 3854.477170] inv_icm42600_disable_vddio_reg() disable vddio
> > [ 3854.483835] ------------[ cut here ]------------
> > [ 3854.483912] WARNING: CPU: 0 PID: 582 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0
> > [ 3854.497853] unbalanced disables for regulator
> >
> > Is the way from here to remove the devm_add_action_or_reset(dev,
> > inv_icm42600_disable_vddio_reg... ?
> 
> That will make a mess if runtime PM is not built into
> the kernel which is why an PM state in remove should return to the state
> before it was enabled in the first place (i.e. on!).
> That final runtime suspend surprises me.

Got it :)

/Sean

Re: [PATCH 4/6] iio: imu: inv_icm42600: Simplify pm_runtime setup

[Jonathan Cameron]

On Fri, 18 Jul 2025 06:40:50 +0000
Sean Nyekjaer <sean@geanix.com> wrote:

> On Wed, Jul 16, 2025 at 09:00:10AM +0100, Jonathan Cameron wrote:
> > On Mon, 14 Jul 2025 07:42:43 +0000
> > Sean Nyekjaer <sean@geanix.com> wrote:
> >   
> > > On Mon, Jul 14, 2025 at 07:24:57AM +0100, Sean Nyekjaer wrote:  
> > > > On Sun, Jul 13, 2025 at 03:28:10PM +0100, Jonathan Cameron wrote:  
> > > > > On Wed, 09 Jul 2025 14:35:12 +0200
> > > > > Sean Nyekjaer <sean@geanix.com> wrote:
> > > > >  
> > > > > > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
> > > > > > calls during probe. These are not required when the device is marked
> > > > > > active via pm_runtime_set_active() before enabling pm_runtime with
> > > > > > pm_runtime_enable().
> > > > > >
> > > > > > Also remove the redundant pm_runtime_put_sync() call from the cleanup
> > > > > > path, since the core is not incrementing the usage count beforehand.
> > > > > >
> > > > > > This simplifies the PM setup and avoids manipulating the usage counter
> > > > > > unnecessarily.  
> > > > >
> > > > > Could we switch directly to using devm_pm_runtime_enable() for this driver?
> > > > >
> > > > > At first glance looks like this code is missing the disable of autosuspend
> > > > > that should be there (which devm_pm_runtime_enable() will also handle).
> > > > >  
> > > >
> > > > I have tried to use devm_pm_runtime_enable() but on rmmod it warns
> > > > "unbalanced disables for regulator"
> > > >
> > > > If I remove this:
> > > > -	ret = devm_add_action_or_reset(dev, inv_icm42600_disable_vddio_reg, st);
> > > > -	if (ret)
> > > > -		return ret;
> > > >
> > > > Everything seems okay again. I have checked with printk's that
> > > > inv_icm42600_disable_vddio_reg() is called twice with
> > > > devm_pm_runtime_enable() used.
> > > > Does it make sense?
> > > >
> > > > /Sean  
> > >
> > > with pm_runtime_enable():
> > > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko
> > > [ 3793.713077] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt
> > > [ 3793.727728] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity...
> > > [ 3793.737660] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator
> > > [ 3793.856891] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator
> > > [ 3793.866872] inv_icm42600_enable_regulator_vddio() enable vddio
> > > [ 3793.920739] inv_icm42600_runtime_suspend() disable vddio
> > > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600
> > > [ 3796.954850] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio()
> > > [ 3796.954910] inv_icm42600_enable_regulator_vddio() enable vddio
> > > [ 3796.985140] inv_icm42600_disable_vddio_reg() disable vddio
> > >
> > > with devm_pm_runtime_enable():
> > > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko
> > > [ 3852.873887] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt
> > > [ 3852.888715] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity...
> > > [ 3852.898514] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator
> > > [ 3853.016890] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator
> > > [ 3853.026860] inv_icm42600_enable_regulator_vddio() enable vddio
> > > [ 3853.080835] inv_icm42600_runtime_suspend() disable vddio
> > > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600
> > > [ 3854.448461] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio()
> > > [ 3854.448540] inv_icm42600_enable_regulator_vddio() enable vddio
> > > [ 3854.467061] inv_icm42600_runtime_suspend() disable vddio  
> > 
> > As below What is the call path for this final suspend?
> > Is it coming from update_autosuspend()?  
> 
> Yeah it looks like pm_runtime_dont_use_autosuspend() is calling runtime_suspend().

ok. So how are we supposed to handle this?  Seems like it would be a fairly common
situation.  devm_pm_runtime_set_active_enabled() might be relevant.
I get confused by all the reference counter
complexity in runtime pm but is devm_pm_runtime_get_noresume() what we want?
That will decrement the usage count with no action just before we hit the point
were the suspend or not decision is made.  So I think that will mean the device
things it is already suspended when you hit the path here and so not do it again?
 
There seems to be only one user of this stuff though:
https://elixir.bootlin.com/linux/v6.15.6/source/drivers/spi/atmel-quadspi.c#L1440
> 
> root@v4:/data/root# rmmod inv-icm42600-i2c inv-icm42600
> [  291.511085] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio()
> [  291.511165] inv_icm42600_enable_regulator_vddio() enable vddio
> [  291.532398] inv_icm42600_runtime_suspend() disable vddio
> [  291.538517] CPU: 0 UID: 0 PID: 331 Comm: rmmod Tainted: G        W           6.16.0-rc1-00202-g7fe6e564b5c9-dirty #146 VOLUNTARY
> [  291.538559] Tainted: [W]=WARN
> [  291.538566] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
> [  291.538575] Call trace:
> [  291.538590]  unwind_backtrace from show_stack+0x10/0x14
> [  291.538643]  show_stack from dump_stack_lvl+0x54/0x68
> [  291.538679]  dump_stack_lvl from inv_icm42600_runtime_suspend+0x68/0x6c [inv_icm42600]
> [  291.538728]  inv_icm42600_runtime_suspend [inv_icm42600] from __rpm_callback+0x48/0x18c
> [  291.538775]  __rpm_callback from rpm_callback+0x5c/0x68
> [  291.538815]  rpm_callback from rpm_suspend+0xdc/0x584
> [  291.538853]  rpm_suspend from pm_runtime_disable_action+0x30/0x5c
> [  291.538885]  pm_runtime_disable_action from devres_release_group+0x180/0x1a0
> [  291.538917]  devres_release_group from i2c_device_remove+0x34/0x84
> [  291.538949]  i2c_device_remove from device_release_driver_internal+0x180/0x1f4
> [  291.538976]  device_release_driver_internal from driver_detach+0x54/0xa0
> [  291.538998]  driver_detach from bus_remove_driver+0x58/0xa4
> [  291.539030]  bus_remove_driver from sys_delete_module+0x16c/0x250
> [  291.539065]  sys_delete_module from ret_fast_syscall+0x0/0x54
> [  291.539089] Exception stack(0xd0ab1fa8 to 0xd0ab1ff0)
> [  291.539108] 1fa0:                   be94fe46 be94fd1c 017ccfa4 00000800 0000000a 017ccf68
> [  291.539126] 1fc0: be94fe46 be94fd1c 017ccf68 00000081 00000000 00000001 00000003 017cc190
> [  291.539139] 1fe0: b6c8be41 be94fadc 000179cb b6c8be48
> [  291.685102] inv_icm42600_disable_vddio_reg() disable vddio
> [  291.694566] ------------[ cut here ]------------
> [  291.694621] WARNING: CPU: 0 PID: 331 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0
> [  291.708496] unbalanced disables for regulator-dummy
> [  291.713391] Modules linked in: inv_icm42600_i2c(-) inv_icm42600 inv_sensors_timestamp [last unloaded: inv_icm42600]
> [  291.723939] CPU: 0 UID: 0 PID: 331 Comm: rmmod Tainted: G        W           6.16.0-rc1-00202-g7fe6e564b5c9-dirty #146 VOLUNTARY
> [  291.735620] Tainted: [W]=WARN
> [  291.738598] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
> [  291.744789] Call trace:
> [  291.744807]  unwind_backtrace from show_stack+0x10/0x14
> [  291.752614]  show_stack from dump_stack_lvl+0x54/0x68
> [  291.757704]  dump_stack_lvl from __warn+0x7c/0xe0
> [  291.762437]  __warn from warn_slowpath_fmt+0x124/0x18c
> [  291.767602]  warn_slowpath_fmt from _regulator_disable+0x140/0x1a0
> [  291.773812]  _regulator_disable from regulator_disable+0x48/0x80
> [  291.779843]  regulator_disable from devres_release_group+0x180/0x1a0
> [  291.786231]  devres_release_group from i2c_device_remove+0x34/0x84
> [  291.792446]  i2c_device_remove from device_release_driver_internal+0x180/0x1f4
> [  291.799699]  device_release_driver_internal from driver_detach+0x54/0xa0
> [  291.806427]  driver_detach from bus_remove_driver+0x58/0xa4
> [  291.812033]  bus_remove_driver from sys_delete_module+0x16c/0x250
> [  291.818160]  sys_delete_module from ret_fast_syscall+0x0/0x54
> [  291.823934] Exception stack(0xd0ab1fa8 to 0xd0ab1ff0)
> [  291.829007] 1fa0:                   be94fe46 be94fd1c 017ccfa4 00000800 0000000a 017ccf68
> [  291.837205] 1fc0: be94fe46 be94fd1c 017ccf68 00000081 00000000 00000001 00000003 017cc190
> [  291.845397] 1fe0: b6c8be41 be94fadc 000179cb b6c8be48
> [  291.850632] ---[ end trace 0000000000000000 ]---0
> 
> >   
> > > [ 3854.477170] inv_icm42600_disable_vddio_reg() disable vddio
> > > [ 3854.483835] ------------[ cut here ]------------
> > > [ 3854.483912] WARNING: CPU: 0 PID: 582 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0
> > > [ 3854.497853] unbalanced disables for regulator
> > >
> > > Is the way from here to remove the devm_add_action_or_reset(dev,
> > > inv_icm42600_disable_vddio_reg... ?  
> > 
> > That will make a mess if runtime PM is not built into
> > the kernel which is why an PM state in remove should return to the state
> > before it was enabled in the first place (i.e. on!).
> > That final runtime suspend surprises me.  
> 
> Got it :)
> 
> /Sean
> 
> 

Re: [PATCH 4/6] iio: imu: inv_icm42600: Simplify pm_runtime setup

[Jonathan Cameron]

On Sat, 19 Jul 2025 12:47:11 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 18 Jul 2025 06:40:50 +0000
> Sean Nyekjaer <sean@geanix.com> wrote:
> 
> > On Wed, Jul 16, 2025 at 09:00:10AM +0100, Jonathan Cameron wrote:  
> > > On Mon, 14 Jul 2025 07:42:43 +0000
> > > Sean Nyekjaer <sean@geanix.com> wrote:
> > >     
> > > > On Mon, Jul 14, 2025 at 07:24:57AM +0100, Sean Nyekjaer wrote:    
> > > > > On Sun, Jul 13, 2025 at 03:28:10PM +0100, Jonathan Cameron wrote:    
> > > > > > On Wed, 09 Jul 2025 14:35:12 +0200
> > > > > > Sean Nyekjaer <sean@geanix.com> wrote:
> > > > > >    
> > > > > > > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
> > > > > > > calls during probe. These are not required when the device is marked
> > > > > > > active via pm_runtime_set_active() before enabling pm_runtime with
> > > > > > > pm_runtime_enable().
> > > > > > >
> > > > > > > Also remove the redundant pm_runtime_put_sync() call from the cleanup
> > > > > > > path, since the core is not incrementing the usage count beforehand.
> > > > > > >
> > > > > > > This simplifies the PM setup and avoids manipulating the usage counter
> > > > > > > unnecessarily.    
> > > > > >
> > > > > > Could we switch directly to using devm_pm_runtime_enable() for this driver?
> > > > > >
> > > > > > At first glance looks like this code is missing the disable of autosuspend
> > > > > > that should be there (which devm_pm_runtime_enable() will also handle).
> > > > > >    
> > > > >
> > > > > I have tried to use devm_pm_runtime_enable() but on rmmod it warns
> > > > > "unbalanced disables for regulator"
> > > > >
> > > > > If I remove this:
> > > > > -	ret = devm_add_action_or_reset(dev, inv_icm42600_disable_vddio_reg, st);
> > > > > -	if (ret)
> > > > > -		return ret;
> > > > >
> > > > > Everything seems okay again. I have checked with printk's that
> > > > > inv_icm42600_disable_vddio_reg() is called twice with
> > > > > devm_pm_runtime_enable() used.
> > > > > Does it make sense?
> > > > >
> > > > > /Sean    
> > > >
> > > > with pm_runtime_enable():
> > > > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko
> > > > [ 3793.713077] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt
> > > > [ 3793.727728] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity...
> > > > [ 3793.737660] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator
> > > > [ 3793.856891] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator
> > > > [ 3793.866872] inv_icm42600_enable_regulator_vddio() enable vddio
> > > > [ 3793.920739] inv_icm42600_runtime_suspend() disable vddio
> > > > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600
> > > > [ 3796.954850] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio()
> > > > [ 3796.954910] inv_icm42600_enable_regulator_vddio() enable vddio
> > > > [ 3796.985140] inv_icm42600_disable_vddio_reg() disable vddio
> > > >
> > > > with devm_pm_runtime_enable():
> > > > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko
> > > > [ 3852.873887] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt
> > > > [ 3852.888715] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity...
> > > > [ 3852.898514] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator
> > > > [ 3853.016890] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator
> > > > [ 3853.026860] inv_icm42600_enable_regulator_vddio() enable vddio
> > > > [ 3853.080835] inv_icm42600_runtime_suspend() disable vddio
> > > > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600
> > > > [ 3854.448461] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio()
> > > > [ 3854.448540] inv_icm42600_enable_regulator_vddio() enable vddio
> > > > [ 3854.467061] inv_icm42600_runtime_suspend() disable vddio    
> > > 
> > > As below What is the call path for this final suspend?
> > > Is it coming from update_autosuspend()?    
> > 
> > Yeah it looks like pm_runtime_dont_use_autosuspend() is calling runtime_suspend().  
> 
> ok. So how are we supposed to handle this?  Seems like it would be a fairly common
> situation.  devm_pm_runtime_set_active_enabled() might be relevant.
> I get confused by all the reference counter
> complexity in runtime pm but is devm_pm_runtime_get_noresume() what we want?
> That will decrement the usage count with no action just before we hit the point
> were the suspend or not decision is made.  So I think that will mean the device
> things it is already suspended when you hit the path here and so not do it again?
>  
> There seems to be only one user of this stuff though:
> https://elixir.bootlin.com/linux/v6.15.6/source/drivers/spi/atmel-quadspi.c#L1440
Note that example does a get in the remove callback().   Seems odd but maybe
we need a devm_pm_runtime_*put() that results in an a get in the remove path to ensure counts
all match.

> > 
> > root@v4:/data/root# rmmod inv-icm42600-i2c inv-icm42600
> > [  291.511085] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio()
> > [  291.511165] inv_icm42600_enable_regulator_vddio() enable vddio
> > [  291.532398] inv_icm42600_runtime_suspend() disable vddio
> > [  291.538517] CPU: 0 UID: 0 PID: 331 Comm: rmmod Tainted: G        W           6.16.0-rc1-00202-g7fe6e564b5c9-dirty #146 VOLUNTARY
> > [  291.538559] Tainted: [W]=WARN
> > [  291.538566] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
> > [  291.538575] Call trace:
> > [  291.538590]  unwind_backtrace from show_stack+0x10/0x14
> > [  291.538643]  show_stack from dump_stack_lvl+0x54/0x68
> > [  291.538679]  dump_stack_lvl from inv_icm42600_runtime_suspend+0x68/0x6c [inv_icm42600]
> > [  291.538728]  inv_icm42600_runtime_suspend [inv_icm42600] from __rpm_callback+0x48/0x18c
> > [  291.538775]  __rpm_callback from rpm_callback+0x5c/0x68
> > [  291.538815]  rpm_callback from rpm_suspend+0xdc/0x584
> > [  291.538853]  rpm_suspend from pm_runtime_disable_action+0x30/0x5c
> > [  291.538885]  pm_runtime_disable_action from devres_release_group+0x180/0x1a0
> > [  291.538917]  devres_release_group from i2c_device_remove+0x34/0x84
> > [  291.538949]  i2c_device_remove from device_release_driver_internal+0x180/0x1f4
> > [  291.538976]  device_release_driver_internal from driver_detach+0x54/0xa0
> > [  291.538998]  driver_detach from bus_remove_driver+0x58/0xa4
> > [  291.539030]  bus_remove_driver from sys_delete_module+0x16c/0x250
> > [  291.539065]  sys_delete_module from ret_fast_syscall+0x0/0x54
> > [  291.539089] Exception stack(0xd0ab1fa8 to 0xd0ab1ff0)
> > [  291.539108] 1fa0:                   be94fe46 be94fd1c 017ccfa4 00000800 0000000a 017ccf68
> > [  291.539126] 1fc0: be94fe46 be94fd1c 017ccf68 00000081 00000000 00000001 00000003 017cc190
> > [  291.539139] 1fe0: b6c8be41 be94fadc 000179cb b6c8be48
> > [  291.685102] inv_icm42600_disable_vddio_reg() disable vddio
> > [  291.694566] ------------[ cut here ]------------
> > [  291.694621] WARNING: CPU: 0 PID: 331 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0
> > [  291.708496] unbalanced disables for regulator-dummy
> > [  291.713391] Modules linked in: inv_icm42600_i2c(-) inv_icm42600 inv_sensors_timestamp [last unloaded: inv_icm42600]
> > [  291.723939] CPU: 0 UID: 0 PID: 331 Comm: rmmod Tainted: G        W           6.16.0-rc1-00202-g7fe6e564b5c9-dirty #146 VOLUNTARY
> > [  291.735620] Tainted: [W]=WARN
> > [  291.738598] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
> > [  291.744789] Call trace:
> > [  291.744807]  unwind_backtrace from show_stack+0x10/0x14
> > [  291.752614]  show_stack from dump_stack_lvl+0x54/0x68
> > [  291.757704]  dump_stack_lvl from __warn+0x7c/0xe0
> > [  291.762437]  __warn from warn_slowpath_fmt+0x124/0x18c
> > [  291.767602]  warn_slowpath_fmt from _regulator_disable+0x140/0x1a0
> > [  291.773812]  _regulator_disable from regulator_disable+0x48/0x80
> > [  291.779843]  regulator_disable from devres_release_group+0x180/0x1a0
> > [  291.786231]  devres_release_group from i2c_device_remove+0x34/0x84
> > [  291.792446]  i2c_device_remove from device_release_driver_internal+0x180/0x1f4
> > [  291.799699]  device_release_driver_internal from driver_detach+0x54/0xa0
> > [  291.806427]  driver_detach from bus_remove_driver+0x58/0xa4
> > [  291.812033]  bus_remove_driver from sys_delete_module+0x16c/0x250
> > [  291.818160]  sys_delete_module from ret_fast_syscall+0x0/0x54
> > [  291.823934] Exception stack(0xd0ab1fa8 to 0xd0ab1ff0)
> > [  291.829007] 1fa0:                   be94fe46 be94fd1c 017ccfa4 00000800 0000000a 017ccf68
> > [  291.837205] 1fc0: be94fe46 be94fd1c 017ccf68 00000081 00000000 00000001 00000003 017cc190
> > [  291.845397] 1fe0: b6c8be41 be94fadc 000179cb b6c8be48
> > [  291.850632] ---[ end trace 0000000000000000 ]---0
> >   
> > >     
> > > > [ 3854.477170] inv_icm42600_disable_vddio_reg() disable vddio
> > > > [ 3854.483835] ------------[ cut here ]------------
> > > > [ 3854.483912] WARNING: CPU: 0 PID: 582 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0
> > > > [ 3854.497853] unbalanced disables for regulator
> > > >
> > > > Is the way from here to remove the devm_add_action_or_reset(dev,
> > > > inv_icm42600_disable_vddio_reg... ?    
> > > 
> > > That will make a mess if runtime PM is not built into
> > > the kernel which is why an PM state in remove should return to the state
> > > before it was enabled in the first place (i.e. on!).
> > > That final runtime suspend surprises me.    
> > 
> > Got it :)
> > 
> > /Sean
> > 
> >   
> 
> 

Re: [PATCH 4/6] iio: imu: inv_icm42600: Simplify pm_runtime setup

[Sean Nyekjaer]

On Sat, Jul 19, 2025 at 02:32:50PM +0100, Jonathan Cameron wrote:
> On Sat, 19 Jul 2025 12:47:11 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Fri, 18 Jul 2025 06:40:50 +0000
> > Sean Nyekjaer <sean@geanix.com> wrote:
> >
> > > On Wed, Jul 16, 2025 at 09:00:10AM +0100, Jonathan Cameron wrote:
> > > > On Mon, 14 Jul 2025 07:42:43 +0000
> > > > Sean Nyekjaer <sean@geanix.com> wrote:
> > > >
> > > > > On Mon, Jul 14, 2025 at 07:24:57AM +0100, Sean Nyekjaer wrote:
> > > > > > On Sun, Jul 13, 2025 at 03:28:10PM +0100, Jonathan Cameron wrote:
> > > > > > > On Wed, 09 Jul 2025 14:35:12 +0200
> > > > > > > Sean Nyekjaer <sean@geanix.com> wrote:
> > > > > > >
> > > > > > > > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
> > > > > > > > calls during probe. These are not required when the device is marked
> > > > > > > > active via pm_runtime_set_active() before enabling pm_runtime with
> > > > > > > > pm_runtime_enable().
> > > > > > > >
> > > > > > > > Also remove the redundant pm_runtime_put_sync() call from the cleanup
> > > > > > > > path, since the core is not incrementing the usage count beforehand.
> > > > > > > >
> > > > > > > > This simplifies the PM setup and avoids manipulating the usage counter
> > > > > > > > unnecessarily.
> > > > > > >
> > > > > > > Could we switch directly to using devm_pm_runtime_enable() for this driver?
> > > > > > >
> > > > > > > At first glance looks like this code is missing the disable of autosuspend
> > > > > > > that should be there (which devm_pm_runtime_enable() will also handle).
> > > > > > >
> > > > > >
> > > > > > I have tried to use devm_pm_runtime_enable() but on rmmod it warns
> > > > > > "unbalanced disables for regulator"
> > > > > >
> > > > > > If I remove this:
> > > > > > -	ret = devm_add_action_or_reset(dev, inv_icm42600_disable_vddio_reg, st);
> > > > > > -	if (ret)
> > > > > > -		return ret;
> > > > > >
> > > > > > Everything seems okay again. I have checked with printk's that
> > > > > > inv_icm42600_disable_vddio_reg() is called twice with
> > > > > > devm_pm_runtime_enable() used.
> > > > > > Does it make sense?
> > > > > >
> > > > > > /Sean
> > > > >
> > > > > with pm_runtime_enable():
> > > > > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko
> > > > > [ 3793.713077] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt
> > > > > [ 3793.727728] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity...
> > > > > [ 3793.737660] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator
> > > > > [ 3793.856891] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator
> > > > > [ 3793.866872] inv_icm42600_enable_regulator_vddio() enable vddio
> > > > > [ 3793.920739] inv_icm42600_runtime_suspend() disable vddio
> > > > > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600
> > > > > [ 3796.954850] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio()
> > > > > [ 3796.954910] inv_icm42600_enable_regulator_vddio() enable vddio
> > > > > [ 3796.985140] inv_icm42600_disable_vddio_reg() disable vddio
> > > > >
> > > > > with devm_pm_runtime_enable():
> > > > > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko
> > > > > [ 3852.873887] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt
> > > > > [ 3852.888715] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity...
> > > > > [ 3852.898514] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator
> > > > > [ 3853.016890] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator
> > > > > [ 3853.026860] inv_icm42600_enable_regulator_vddio() enable vddio
> > > > > [ 3853.080835] inv_icm42600_runtime_suspend() disable vddio
> > > > > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600
> > > > > [ 3854.448461] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio()
> > > > > [ 3854.448540] inv_icm42600_enable_regulator_vddio() enable vddio
> > > > > [ 3854.467061] inv_icm42600_runtime_suspend() disable vddio
> > > >
> > > > As below What is the call path for this final suspend?
> > > > Is it coming from update_autosuspend()?
> > >
> > > Yeah it looks like pm_runtime_dont_use_autosuspend() is calling runtime_suspend().
> >
> > ok. So how are we supposed to handle this?  Seems like it would be a fairly common
> > situation.  devm_pm_runtime_set_active_enabled() might be relevant.
> > I get confused by all the reference counter
> > complexity in runtime pm but is devm_pm_runtime_get_noresume() what we want?
> > That will decrement the usage count with no action just before we hit the point
> > were the suspend or not decision is made.  So I think that will mean the device
> > things it is already suspended when you hit the path here and so not do it again?
> >
> > There seems to be only one user of this stuff though:
> > https://elixir.bootlin.com/linux/v6.15.6/source/drivers/spi/atmel-quadspi.c#L1440
> Note that example does a get in the remove callback().   Seems odd but maybe
> we need a devm_pm_runtime_*put() that results in an a get in the remove path to ensure counts
> all match.
> 

Hi,

For what is worth; It looks like drivers/iio/adc/ti-ads1100.c does same as we are trying to do here.
It also fails in rmmod:

root@v4:/data/root# insmod /tmp/ti-ads1100.ko
[   65.778107] ads1100 1-0049: supply vdd not found, using dummy regulator
root@v4:/data/root# rmmod /tmp/ti-ads1100.ko
[   73.625923] ------------[ cut here ]------------
[   73.625976] WARNING: CPU: 0 PID: 299 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0
[   73.639939] unbalanced disables for regulator-dummy
[   73.644833] Modules linked in: ti_ads1100(-)
[   73.649178] CPU: 0 UID: 0 PID: 299 Comm: rmmod Not tainted 6.16.0-rc1-00203-ge84dc7fb9038-dirty #160 VOLUNTARY
[   73.659289] Hardware name: Freescale i.MX6 Ultralite (Device Tree)
[   73.665479] Call trace:
[   73.665499]  unwind_backtrace from show_stack+0x10/0x14
[   73.673301]  show_stack from dump_stack_lvl+0x54/0x68
[   73.678389]  dump_stack_lvl from __warn+0x7c/0xe0
[   73.683120]  __warn from warn_slowpath_fmt+0x124/0x18c
[   73.688285]  warn_slowpath_fmt from _regulator_disable+0x140/0x1a0
[   73.694494]  _regulator_disable from regulator_disable+0x48/0x80
[   73.700529]  regulator_disable from devres_release_group+0x180/0x1a0
[   73.706925]  devres_release_group from i2c_device_remove+0x34/0x84
[   73.713146]  i2c_device_remove from device_release_driver_internal+0x180/0x1f4
[   73.720402]  device_release_driver_internal from driver_detach+0x54/0xa0
[   73.727130]  driver_detach from bus_remove_driver+0x58/0xa4
[   73.732735]  bus_remove_driver from sys_delete_module+0x16c/0x250
[   73.738857]  sys_delete_module from ret_fast_syscall+0x0/0x54
[   73.744627] Exception stack(0xd0d5dfa8 to 0xd0d5dff0)
[   73.749701] dfa0:                   be970e51 be970d2c 00233fbc 00000800 0000000a 00233f80
[   73.757896] dfc0: be970e51 be970d2c 00233f80 00000081 00000000 00000001 00000002 00233190
[   73.766086] dfe0: b6c87e41 be970aec 000179cb b6c87e48
[   73.771228] ---[ end trace 0000000000000000 ]---

I will try something similar to this:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=4154e767354140db7804207117e7238fb337b0e7

/Sean

[PATCH 5/6] iio: imu: inv_icm42600: Drop redundant pm_runtime reinitialization in resume

[Sean Nyekjaer]

Remove unnecessary calls to pm_runtime_disable(), pm_runtime_set_active(),
and pm_runtime_enable() from the resume path. These operations are not
required here and can interfere with proper pm_runtime state handling,
especially when resuming from a pm_runtime suspended state.

Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index 1072bea11c73d09a9a0e6ea9d4a5c7a72248dca7..37b3a7754da1c4e381e38c9871e55a941e19cef4 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -904,10 +904,6 @@ static int inv_icm42600_resume(struct device *dev)
 			goto out_unlock;
 	}
 
-	pm_runtime_disable(dev);
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
-
 	/* restore sensors state */
 	ret = inv_icm42600_set_pwr_mgmt0(st, st->suspended.gyro,
 					 st->suspended.accel,

-- 
2.50.0

[PATCH 6/6] iio: imu: inv_icm42600: Avoid configuring if already pm_runtime suspended

[Sean Nyekjaer]

Do as in suspend, skip resume configuration steps if the device is already
pm_runtime suspended. This avoids reconfiguring a device that is already
in the correct low-power state and ensures that pm_runtimeM handles the
power state transitions properly.

Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index 37b3a7754da1c4e381e38c9871e55a941e19cef4..d745a40b042e1c86b232aaae0820942d11d51c79 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -824,17 +824,15 @@ static int inv_icm42600_suspend(struct device *dev)
 	struct device *accel_dev;
 	bool wakeup;
 	int accel_conf;
-	int ret;
+	int ret = 0;
 
 	mutex_lock(&st->lock);
 
 	st->suspended.gyro = st->conf.gyro.mode;
 	st->suspended.accel = st->conf.accel.mode;
 	st->suspended.temp = st->conf.temp_en;
-	if (pm_runtime_suspended(dev)) {
-		ret = 0;
+	if (pm_runtime_suspended(dev))
 		goto out_unlock;
-	}
 
 	/* disable FIFO data streaming */
 	if (st->fifo.on) {
@@ -887,10 +885,13 @@ static int inv_icm42600_resume(struct device *dev)
 	struct inv_icm42600_sensor_state *accel_st = iio_priv(st->indio_accel);
 	struct device *accel_dev;
 	bool wakeup;
-	int ret;
+	int ret = 0;
 
 	mutex_lock(&st->lock);
 
+	if (pm_runtime_suspended(dev))
+		goto out_unlock;
+
 	/* check wakeup capability */
 	accel_dev = &st->indio_accel->dev;
 	wakeup = st->apex.on && device_may_wakeup(accel_dev);

-- 
2.50.0

Re: [PATCH 6/6] iio: imu: inv_icm42600: Avoid configuring if already pm_runtime suspended

[Andy Shevchenko]

On Wed, Jul 09, 2025 at 02:35:14PM +0200, Sean Nyekjaer wrote:
> Do as in suspend, skip resume configuration steps if the device is already
> pm_runtime suspended. This avoids reconfiguring a device that is already
> in the correct low-power state and ensures that pm_runtimeM handles the
> power state transitions properly.

Can't it be done by one of the hints to the runtime PM? Something like
SMART_SUSPEND ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/6] iio: imu: inv_icm42600: Avoid configuring if already pm_runtime suspended

[Jonathan Cameron]

On Wed, 09 Jul 2025 14:35:14 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> Do as in suspend, skip resume configuration steps if the device is already
> pm_runtime suspended. This avoids reconfiguring a device that is already
> in the correct low-power state and ensures that pm_runtimeM handles the
> power state transitions properly.
> 
> Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---

Not really related to what you have here, but this code would really
benefit from using guard(mutex)()

Jonathan

>  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 37b3a7754da1c4e381e38c9871e55a941e19cef4..d745a40b042e1c86b232aaae0820942d11d51c79 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -824,17 +824,15 @@ static int inv_icm42600_suspend(struct device *dev)
>  	struct device *accel_dev;
>  	bool wakeup;
>  	int accel_conf;
> -	int ret;
> +	int ret = 0;
>  
>  	mutex_lock(&st->lock);
>  
>  	st->suspended.gyro = st->conf.gyro.mode;
>  	st->suspended.accel = st->conf.accel.mode;
>  	st->suspended.temp = st->conf.temp_en;
> -	if (pm_runtime_suspended(dev)) {
> -		ret = 0;
> +	if (pm_runtime_suspended(dev))
>  		goto out_unlock;
> -	}
>  
>  	/* disable FIFO data streaming */
>  	if (st->fifo.on) {
> @@ -887,10 +885,13 @@ static int inv_icm42600_resume(struct device *dev)
>  	struct inv_icm42600_sensor_state *accel_st = iio_priv(st->indio_accel);
>  	struct device *accel_dev;
>  	bool wakeup;
> -	int ret;
> +	int ret = 0;
>  
>  	mutex_lock(&st->lock);
>  
> +	if (pm_runtime_suspended(dev))
> +		goto out_unlock;
> +
>  	/* check wakeup capability */
>  	accel_dev = &st->indio_accel->dev;
>  	wakeup = st->apex.on && device_may_wakeup(accel_dev);
> 

Re: [PATCH 6/6] iio: imu: inv_icm42600: Avoid configuring if already pm_runtime suspended

[Sean Nyekjaer]

On Sun, Jul 13, 2025 at 03:32:27PM +0100, Jonathan Cameron wrote:
> On Wed, 09 Jul 2025 14:35:14 +0200
> Sean Nyekjaer <sean@geanix.com> wrote:
> 
> > Do as in suspend, skip resume configuration steps if the device is already
> > pm_runtime suspended. This avoids reconfiguring a device that is already
> > in the correct low-power state and ensures that pm_runtimeM handles the
> > power state transitions properly.
> >
> > Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > ---
> 
> Not really related to what you have here, but this code would really
> benefit from using guard(mutex)()
> 
> Jonathan

I have converted most of this driver to use guard(mutex).

Does it make sense to use guard(mutex) in functions that still relies on
goto error out? Like...

static int inv_icm42600_temp_read(struct inv_icm42600_state *st, s16 *temp)
{
	struct device *dev = regmap_get_device(st->map);
	__be16 *raw;
	int ret;

	pm_runtime_get_sync(dev);
	mutex_lock(&st->lock);

	ret = inv_icm42600_set_temp_conf(st, true, NULL);
	if (ret)
		goto exit;

	raw = (__be16 *)&st->buffer[0];
	ret = regmap_bulk_read(st->map, INV_ICM42600_REG_TEMP_DATA, raw, sizeof(*raw));
	if (ret)
		goto exit;

	*temp = (s16)be16_to_cpup(raw);
	if (*temp == INV_ICM42600_DATA_INVALID)
		ret = -EINVAL;

exit:
	mutex_unlock(&st->lock);
	pm_runtime_mark_last_busy(dev);
	pm_runtime_put_autosuspend(dev);

	return ret;
}


If I use guard_scoped(..) it creates a lot of diff lines...

/Sean

Re: [PATCH 6/6] iio: imu: inv_icm42600: Avoid configuring if already pm_runtime suspended

[Jonathan Cameron]

On Mon, 14 Jul 2025 10:15:59 +0000
Sean Nyekjaer <sean@geanix.com> wrote:

> On Sun, Jul 13, 2025 at 03:32:27PM +0100, Jonathan Cameron wrote:
> > On Wed, 09 Jul 2025 14:35:14 +0200
> > Sean Nyekjaer <sean@geanix.com> wrote:
> >   
> > > Do as in suspend, skip resume configuration steps if the device is already
> > > pm_runtime suspended. This avoids reconfiguring a device that is already
> > > in the correct low-power state and ensures that pm_runtimeM handles the
> > > power state transitions properly.
> > >
> > > Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
> > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > > ---  
> > 
> > Not really related to what you have here, but this code would really
> > benefit from using guard(mutex)()
> > 
> > Jonathan  
> 
> I have converted most of this driver to use guard(mutex).
> 
> Does it make sense to use guard(mutex) in functions that still relies on
> goto error out? Like...

No - general rule is never combine cleanup.h magic like guard with gotos.

If you do want to use it in cases like this, factor out the stuff
> 
> static int inv_icm42600_temp_read(struct inv_icm42600_state *st, s16 *temp)
> {
> 	struct device *dev = regmap_get_device(st->map);
> 	__be16 *raw;
> 	int ret;
> 
> 	pm_runtime_get_sync(dev);
> 	mutex_lock(&st->lock);
> 

from here

> 	ret = inv_icm42600_set_temp_conf(st, true, NULL);
> 	if (ret)
> 		goto exit;
> 
> 	raw = (__be16 *)&st->buffer[0];
> 	ret = regmap_bulk_read(st->map, INV_ICM42600_REG_TEMP_DATA, raw, sizeof(*raw));
> 	if (ret)
> 		goto exit;
> 
> 	*temp = (s16)be16_to_cpup(raw);
> 	if (*temp == INV_ICM42600_DATA_INVALID)
> 		ret = -EINVAL;
> 

to here as a utility function.  Then can use direct returns in that 
function and there are no gotos to worry about.

> exit:
> 	mutex_unlock(&st->lock);
> 	pm_runtime_mark_last_busy(dev);
> 	pm_runtime_put_autosuspend(dev);
> 
> 	return ret;
> }
> 
> 
> If I use guard_scoped(..) it creates a lot of diff lines...
The only way to use that here and avoid the issue would be with breaks
which is ugly.   If we are going to pay the cost of churn, then
factoring out the guts of the function is much cleaner.

May not be worth bothering though!

Jonathan

> 
> /Sean
> 

Re: [PATCH 0/6] iio: imu: inv_icm42600: pm_runtime fixes + various changes

[Andy Shevchenko]

On Wed, Jul 09, 2025 at 02:35:08PM +0200, Sean Nyekjaer wrote:
> This series was triggered by "Runtime PM usage count underflow!" when
> unloading the module(s).
> By testing the driver in various use cases and reading code it was
> obvious that it could need some tiding up.

> I'm still not 100% satisfied with suspend/resume is calling directly to
> vddio_enable/disable. In my mind it should be managed by pm_runtime.

This patch series touches a quite sensitive area and needs a good,
comprehensive tests (better done independently from the author).
What have been done so far?

-- 
With Best Regards,
Andy Shevchenko