From: Jonathan Cameron <jic23@kernel.org>
To: Crt Mori <cmo@melexis.com>
Cc: linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
Andy Shevchenko <andy.shevchenko@gmail.com>
Subject: Re: [PATCH v5 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes
Date: Wed, 21 Sep 2022 21:19:23 +0100 [thread overview]
Message-ID: <20220921211923.237453b2@jic23-huawei> (raw)
In-Reply-To: <CAKv63uvDNbwn=Jb9Ee0fhSEBPJx94ckZTRCTQw7PfAH4UdN2Dg@mail.gmail.com>
On Mon, 19 Sep 2022 19:59:47 +0200
Crt Mori <cmo@melexis.com> wrote:
> On Mon, 19 Sept 2022 at 19:30, Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 19 Sep 2022 19:09:13 +0200
> > Crt Mori <cmo@melexis.com> wrote:
> >
> > > On Mon, 19 Sept 2022 at 18:24, Jonathan Cameron <jic23@kernel.org> wrote:
> > > >
> > > > On Mon, 19 Sep 2022 10:48:16 +0200
> > > > cmo@melexis.com wrote:
> > > >
> > > > > From: Crt Mori <cmo@melexis.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
> >
> > 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).
I'd definitely like to understand if there is a problem here as it would
effect a load of other devices.
>
> >
> > 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)?
yes - it would indeed be something like you describe.
>
> >
> > > > 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);
> > > > > }
> > > >
> >
next prev parent reply other threads:[~2022-09-21 20:19 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-19 8:48 [PATCH v5 0/3] iio: temperature: mlx90632: Add powermanagement cmo
2022-09-19 8:48 ` [PATCH v5 1/3] iio: temperature: mlx90632 Add runtime powermanagement modes cmo
2022-09-19 16:24 ` Jonathan Cameron
2022-09-19 17:09 ` Crt Mori
2022-09-19 17:30 ` Jonathan Cameron
2022-09-19 17:59 ` Crt Mori
2022-09-21 20:19 ` Jonathan Cameron [this message]
2022-09-19 8:48 ` [PATCH v5 2/3] iio: temperature: mlx90632 Read sampling frequency cmo
2022-09-19 8:48 ` [PATCH v5 3/3] iio: temperature: mlx90632 Change return value of sensor measurement channel cmo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220921211923.237453b2@jic23-huawei \
--to=jic23@kernel.org \
--cc=andy.shevchenko@gmail.com \
--cc=cmo@melexis.com \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).