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

[Jonathan Cameron <jic23@kernel.org>]

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
>