Message-ID: <CAKv63usAjTh9b-HuVdikbtdPKeyFYPy0kENQpkGo6jWXkE_Quw@mail.gmail.com>
Date:   Mon, 19 Sep 2022 19:09:13 +0200
From:   Crt Mori <cmo@...exis.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Andy Shevchenko <andy.shevchenko@...il.com>
Subject: Re: [PATCH v5 1/3] iio: temperature: mlx90632 Add runtime
 powermanagement modes

On Mon, 19 Sept 2022 at 18:24, Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Mon, 19 Sep 2022 10:48:16 +0200
> cmo@...exis.com wrote:
>
> > From: Crt Mori <cmo@...exis.com>
> >
> > 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, 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?

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

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

Powered by blists - more mailing lists