Re: [PATCH v5 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes

[Jonathan Cameron]

On Mon, 19 Sep 2022 19:09:13 +0200
Crt Mori <cmo@xxxxxxxxxxx> wrote:

> On Mon, 19 Sept 2022 at 18:24, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > On Mon, 19 Sep 2022 10:48:16 +0200
> > cmo@xxxxxxxxxxx wrote:
> >  
> > > From: Crt Mori <cmo@xxxxxxxxxxx>
> > >
> > > measurements in lower power mode (SLEEP_STEP), with the lowest refresh
> > > rate (2 seconds).  
> > Hi Crt,
> >
> > I'm a little nervous about one change in the flow from earlier versions.
> > I'm assuming you are sure it is always fine though!
> >
> > Previously before calling the _sleep() in remove we ensured the device
> > was powered up. Now that's no longer true. If runtime pm has it in
> > a low power state it will remain in that state at the point where we call
> > _sleep().
> >
> > One note/question on original code...  Why bother marking regcache dirty when
> > we are going down anyway?  It's not wrong as such, just probably not
> > that useful unless I'm missing something.  Same in the *_wakeup()
> > that puts the cache back but is only called in probe now.  
> 
> Previously when powered on the device the cache was not updated

ah. Got it.  Doing this makes sense if we don't provide the default register
values as there is nothing else to get them from.

However, I think the regmap core does this for us if defaults are not provided:
https://elixir.bootlin.com/linux/v6.0-rc5/source/drivers/base/regmap/regcache.c#L180

Does that not work here for some reason?  If so add a comment.

We do need the dance in the suspend and resume though as regcache code has no
way to know if the values are retained or not so we have to let it know.

>, so I
> added the marking of regcache at wakeup and saw that the same thing
> happens when in resume after powering on. I should keep this
> assumption still, so I will re-add the wakeup to resume (not runtime
> resume). I did not test this part as I focused on runtime resume so
> thanks for noticing.
> 
> >
> > Which then raises question of why we don't need to deal with the regcache
> > any more when we turn power off in suspend?
> >  
> 
> It just did not work properly without this. Not correct EEPROM
> coefficients were used for calculations.
> 
> > So either we need a statement of why the register state is maintained,
> > or add the maintenance for that.  Also probably makes sense to drop
> > the left over maintenance from the probe() and remove() (via devm) paths.
> >  
> I thought I did that by completely removing _remove() and using
> devm_actions for cleanup. Do you see a spot I missed?
> 

I don't think marking the regcache dirty in remove (via the _sleep() call)
does anything useful.  On fresh probe of the driver, we get a new regcache which
we can then sync as you are doing - so no point in marking the one we are about
to delete as dirty that I can see.


> > Jonathan
> >  
> > >
> > > -static int __maybe_unused mlx90632_pm_resume(struct device *dev)
> > > +static int mlx90632_pm_resume(struct device *dev)
> > >  {
> > > -     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > > -     struct mlx90632_data *data = iio_priv(indio_dev);
> > > +     struct mlx90632_data *data = iio_priv(dev_get_drvdata(dev));
> > > +
> > > +     return mlx90632_enable_regulator(data);
> > > +}
> > > +
> > > +static int mlx90632_pm_runtime_suspend(struct device *dev)
> > > +{
> > > +     struct mlx90632_data *data = iio_priv(dev_get_drvdata(dev));
> > >
> > > -     return mlx90632_wakeup(data);  
> > Previously we called wakeup here which writes the regcache back to
> > the device. Now I'm not seeing that happening anywhere in new code.
> > Why is it not needed?
> >  
> I had the same question before, why cache was needed to be marked
> dirty, but without it, CPU did not properly obtain the calculation
> coefficients. What happens now is that we are in step_sleep mode so
> measurements are triggered and it also takes the 2 seconds before they
> are updated. I did not check the line with scope, but I have yet to
> see the strange temperature output which would indicate that not
> proper EEPROM data is used. But I did focus on sleep mostly, so deeper
> sleep I did not retest.

I'd hope runtime pm doesn't need the dance with the cache as the
values should be retained.  It's the deeper sleep that is where I'd
see potential problems as you observed.

Jonathan

> 
> > > +     return mlx90632_pwr_set_sleep_step(data->regmap);
> > >  }  
> >