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

[Jonathan Cameron <jic23@kernel.org>]

> >   
> > > 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.

 
> 
> > 
> >   
> > > +	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;
> > > +}