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