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

[Crt Mori]

On Mon, 19 Sept 2022 at 19:30, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> 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.

It did not work in past, but I can make a few tests and file a bug
later on if indeed we should not need to mark cache refresh at
startup. I would here keep it as it was, because I remember having a
big headache trying to figure out what I was missing with regmap_read
conversion (I remember I started with simple i2c reads).

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

So you would rather that I make a new function which basically will be
a wrapper around mlx90632_pwr_set_sleep_step (as I don't want to
change that function to return nothing and take a void pointer)
instead of using mlx90632_sleep in remove (beside using it in
pm_suspend after this change)?

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

You are correct - runtime_pm never needed any of the cache stuff.

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