Re: [PATCH V8 05/10] iio: imu: inv_icm42607: Add PM support for icm42607

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 22 May 2026 11:23:59 -0500
Chris Morgan <macromorgan@hotmail.com> wrote:

> On Fri, May 22, 2026 at 12:05:15PM +0100, Jonathan Cameron wrote:
> >   
> > > >     
> > > > > diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > > > > index e9c81b52f9ef..bc0cefa2fb77 100644
> > > > > --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > > > > +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > > > > @@ -9,6 +9,7 @@
> > > > >  #include <linux/irq.h>
> > > > >  #include <linux/module.h>
> > > > >  #include <linux/mutex.h>
> > > > > +#include <linux/pm_runtime.h>
> > > > >  #include <linux/property.h>
> > > > >  #include <linux/regmap.h>
> > > > >  #include <linux/regulator/consumer.h>
> > > > > @@ -72,6 +73,62 @@ 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 oldgyro = st->conf.gyro.mode;
> > > > > +	enum inv_icm42607_sensor_mode oldaccel = st->conf.accel.mode;
> > > > > +	bool oldtemp = st->conf.temp_en;
> > > > > +	unsigned int sleepval;
> > > > > +	unsigned int val;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (gyro == oldgyro && accel == oldaccel && temp == oldtemp)
> > > > > +		return 0;
> > > > > +
> > > > > +	val = FIELD_PREP(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, gyro);
> > > > > +	val |= FIELD_PREP(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, accel);
> > > > > +	if (!temp)
> > > > > +		val |= INV_ICM42607_PWR_MGMT0_ACCEL_LP_CLK_SEL;
> > > > > +	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;
> > > > > +
> > > > > +	sleepval = 0;
> > > > > +	if (temp && !oldtemp) {
> > > > > +		if (sleepval < INV_ICM42607_TEMP_STARTUP_TIME_MS)
> > > > > +			sleepval = INV_ICM42607_TEMP_STARTUP_TIME_MS;    
> > > > 		sleepval = max(sleepval,)
> > > > or just assign it here if not later patches add stuff in between
> > > > the assignment to 0 and here.    
> > Wow I write some garbage English sometimes (no excuse, it is my
> > native language!)   
> > > 
> > > I'm going to assign it to 0 here (unless you think I should define it
> > > at the beginning as 0) and then tweak as needed. I think this code
> > > here can be further optimized, especially if we make the assumption
> > > that START and STOP time for each sensor is comparable (the datasheet
> > > doesn't say, so I'm going to go with yes since that greatly simplifies
> > > things).  
> > 
> > I'm a bit lost. Suggestion was just to do
> > 		sleepval = INV_ICM42607_TEMP_STARTUP_TIME_MS;
> > as we know it is 0.   Probably not worth it though as ends up with fragile
> > code.  Fine to keep it to what you have but use max() rather than
> > if()
> > 
> > ...
> >   
> > > > > +static int inv_icm42607_resume(struct device *dev)
> > > > > +{
> > > > > +	struct inv_icm42607_state *st = dev_get_drvdata(dev);
> > > > > +	int ret;
> > > > > +
> > > > > +	guard(mutex)(&st->lock);
> > > > > +    
> > > > Given the bunch of stuff we've run into recently around these
> > > > I'm getting more paranoid.
> > > > Similar to above, could you use pm_runtime_force_resume()
> > > > You would need to gate stuff added later to not occur
> > > > though if it wasn't runtime suspended.    
> > > 
> > > This I'm having trouble understanding. If I use
> > > pm_force_runtime_resume() I'm assuming that either I got an error (in
> > > which case I'd return the error) or the device is runtime resumed
> > > after the call completes. If that's the case, wouldn't my suspend and
> > > resume steps just be pm_force_runtime_suspend/resume, and enabling the
> > > regulator (first for resume) or disabling the regulator (last for
> > > suspend) as needed?  
> > 
> > If you call pm_runtime_force_resume() it will do the right thing
> > wrt to runtime PM state prior to suspend.  If it wasn't runtime suspended
> > it will runtime resume - if it was it'll no do anything. It won't
> > directly tell you which one it did though.
> > 
> > The extra stuff that you know can't be the case if runtime pm is on
> > will need some sort of gating.  However, looking again it may already
> > be protected by more specific checks.
> > 
> > 
> > 	pm_runtime_force_resume();
> > 
> > 	if (st->fifo.on) { //I'd failed to look at what was added.
> > 		ret = regmap_write(st->map, INV_ICM42607_REG_FIFO_CONFIG1,
> > 				   INV_ICM42607_FIFO_CONFIG1_MODE);
> > 		if (ret)
> > 			return ret;
> > 	}
> > 
> > That if (st->fifo.on) previously didn't get checked if we were runtime
> > suspended because in fifo mode we never are.  So I was thinking you'd
> > need that check to be
> > 	if (!pm_runtime_suspended(dev) && st->fifo.on)
> > but the fifo.on check is sufficient by the same argument that if fifo.on
> > is true we aren't in runtime suspend.
> > 
> > Basically I overthought it and didn't check what got added where the
> > comment is in this patch.  
> 
> I'm still lost here... are you saying the existing logic is sufficient or
> that we need to do something more (like force the runtime resume/suspend
> in different places).

I'm saying you should call pm_runtime_force_resume() in the resume()
callback and the equivalent in the suspend() callback + call anything
extra that is done in those callbacks in appropriate order.

My original thought that some of the 'extra' stuff not covered by
pm_runtime_force...() would need extra gating but I think what you have
there is effectively the same via checks that happen to align with the
runtime resume / suspend states.

Jonathan

> 
> Thank you again for all of your help, you've been amazing.
> 
> Chris
> 
> > 
> >    
> > >   
> > > > 
> > > >     
> > > > > +	if (pm_runtime_suspended(dev))
> > > > > +		return 0;
> > > > > +
> > > > > +	ret = inv_icm42607_enable_vddio_reg(st);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	/* Nothing else to restore at this time. */
> > > > > +
> > > > > +	return 0;
> > > > > +}  
> >