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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 23 Jun 2026 13:08:02 -0500
Chris Morgan <macromorgan@hotmail.com> wrote:

> On Mon, Jun 22, 2026 at 09:34:10PM -0500, Chris Morgan wrote:
> > On Sun, Jun 21, 2026 at 06:19:48PM +0100, Jonathan Cameron wrote:  
> > > On Mon, 15 Jun 2026 12:25:48 -0500
> > > Chris Morgan <macroalpha82@gmail.com> wrote:
> > >   
> > > > From: Chris Morgan <macromorgan@hotmail.com>
> > > > 
> > > > Add power management support for the ICM42607 device driver.
> > > > 
> > > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>  
> > > A few things from taking a look at the sashiko report:
> > > https://sashiko.dev/#/patchset/20260615172554.160910-1-macroalpha82%40gmail.com
> > >   
> > > > ---
> > > >  drivers/iio/imu/inv_icm42607/inv_icm42607.h   |  18 +++
> > > >  .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 139 ++++++++++++++++++
> > > >  .../iio/imu/inv_icm42607/inv_icm42607_i2c.c   |   1 +
> > > >  .../iio/imu/inv_icm42607/inv_icm42607_spi.c   |   1 +
> > > >  4 files changed, 159 insertions(+)
> > > > 
> > > > diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607.h b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > > > index a6a58571935f..4f4f541027dc 100644
> > > > --- 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)  
> > > 
> > > Sashiko had a valid comment on this.  MSEC_PER_SEC seems more
> > > appropriate given this is 2 seconds in milli seconds.
> > >   
> > > > +
> > > >  typedef int (*inv_icm42607_bus_setup)(struct inv_icm42607_state *);
> > > >  
> > > >  extern const struct regmap_config inv_icm42607_regmap_config;
> > > >  extern const struct inv_icm42607_hw inv_icm42607_hw_data;
> > > >  extern const struct inv_icm42607_hw inv_icm42607p_hw_data;
> > > > +extern const struct dev_pm_ops inv_icm42607_pm_ops;
> > > >  
> > > >  int inv_icm42607_core_probe(struct regmap *regmap,
> > > >  			    const struct inv_icm42607_hw *hw,
> > > > diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > > > index 4b8e19091786..64f5d263de4f 100644
> > > > --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > > > +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > > > @@ -4,6 +4,7 @@
> > > >   */
> > > >  
> > > >  #include <linux/bitfield.h>
> > > > +#include <linux/cleanup.h>
> > > >  #include <linux/delay.h>
> > > >  #include <linux/dev_printk.h>
> > > >  #include <linux/device/devres.h>
> > > > @@ -11,6 +12,7 @@
> > > >  #include <linux/iio/iio.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/mutex.h>
> > > > +#include <linux/pm_runtime.h>
> > > >  #include <linux/regmap.h>
> > > >  #include <linux/regulator/consumer.h>
> > > >  #include <linux/time.h>
> > > > @@ -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.  
> > > 
> > > Sashiko commented on this..  I think what we could do with adding to the
> > > comment is what the path is that didn't pass through this function which would
> > > ensure we have been on for 30 of this msecs already.  
> > 
> > I'm going to track whatever time the gyro started, and then if less
> > than 45ms has elapsed just pause the remaining amount of time.
> >   
> > >   
> > > > +	 */
> > > > +	if (!gyro && oldgyro)
> > > > +		fsleep(INV_ICM42607_GYRO_STOP_TIME_MS * USEC_PER_MSEC);
> > > > +
> > > > +	val = FIELD_PREP(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, gyro);
> > > > +	val |= FIELD_PREP(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, accel);
> > > > +	ret = regmap_write(st->map, INV_ICM42607_REG_PWR_MGMT0, val);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	st->conf.gyro.mode = gyro;
> > > > +	st->conf.accel.mode = accel;
> > > > +	st->conf.temp_en = temp;
> > > > +
> > > > +	/*
> > > > +	 * If a state change occurs from off to on, sleep for the startup
> > > > +	 * time of the sensor, unless a sleep_ms is specified. Since more
> > > > +	 * than one sensor can be transitioned from off to on, select the
> > > > +	 * maximum time from each of the sensors changing from off to on.
> > > > +	 */
> > > > +	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;
> > > > +}  
> > >   
> > > >  
> > > >  int inv_icm42607_core_probe(struct regmap *regmap,
> > > > @@ -236,6 +305,8 @@ int inv_icm42607_core_probe(struct regmap *regmap,
> > > >  	if (!st)
> > > >  		return -ENOMEM;
> > > >  
> > > > +	dev_set_drvdata(dev, st);
> > > > +
> > > >  	ret = devm_mutex_init(dev, &st->lock);
> > > >  	if (ret)
> > > >  		return ret;
> > > > @@ -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);  
> > > Sashiko does put out some stuff here.  Please take a look and work out or
> > > test if it is right (I think not but haven't checked that carefully!)
> > > From a quick look I think that the auto disabling of autosuspend does a
> > > rpm_idle() that should result in it suspending...
> > >   
> > 
> > I see a few other drivers adding one more call to
> > devm_pm_runtime_enable() so I'm going to see how that works out.  
> 
> Obviously that didn't work, but digging in much deeper into the PM code
> I see that devm_pm_runtime_set_active_enabled() returns
> devm_pm_runtime_enable(), which sets devm_add_action_or_reset() on
> pm_runtime_disable_action(), which calls
> pm_runtime_dont_use_autosuspend() and pm_runtime_disable(). Shouldn't
> this work, or am I missing something? Basically when the driver
> detaches shouldn't this chain end up calling
> pm_runtime_dont_use_autosuspend()?
Agreed. That was how I read things.  The only thing I didn't entirely
confirm (states involved were fiddly enough I'd want to check it
with the actual driver!) was that pm_runtime_dont_use_autosuspend()
ends up calling rpm_idle() in this case.

Jonathan

> 
> Thank you,
> Chris
> 
> >   
> > >   
> > > > +
> > > >  	return 0;
> > > >  }
> > > >  EXPORT_SYMBOL_NS_GPL(inv_icm42607_core_probe, "IIO_ICM42607");  
> > 
> > Thank you,
> > Chris