Re: [PATCH v2 1/2] iio: temperature: mlx90635 MLX90635 IR Temperature sensor

[Crt Mori]

On Mon, 4 Dec 2023 at 18:06, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Mon, 4 Dec 2023 16:34:30 +0100
> Crt Mori <cmo@xxxxxxxxxxx> wrote:
>
> > On Mon, 4 Dec 2023 at 15:22, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> > >
...
> > While in Sleep Step mode, the EEPROM is powered down, but the cache
> > buffers those values. Still when you try to write or read a volatile
> > register (which should not be prevented by cache enabled as per my
> > opinion, but code says differently) in that mode, it returns -EBUSY
> > (as we discovered by code), so this kind of manipulation is needed to
> > enable write and read operations from volatile registers.
>
> So the cache trick is just meant for the eeprom?  Can you use two regmaps.
> (I've seen similar done for devices with different ways of reading which
> this 'kind of' corresponds to).
> One to cover the eeprom and the other the registers that always work.
> That should let you separately control if they are in caching state or
> not.
> Or just read the eeprom into a manually created cache on boot?
>

It did not seem correct to create a manual cache, since regcache does
this job. I tried two separated regmaps, but when I tried to
initialize them I got into kernel panic/crash, so I could not get it
working on same device. Do you have any device in mind I could
template this against?

...
> > "invalid" data (shouldn't differ much, but I wanted to prevent that as
> > it might be 0).
>
> ok.  Just give a little bit more of that detail.  I'd not understood
> intent is to ensure one trigger -> one measurement.

OK.
> >
...
> >
> > Burst is from 90632 terminology (and our chip register map), but maybe
> > more general would be "trigger_measurement"?
>
> ok. But why only if in SLEEP_STEP?
>

Because in continuous mode (other mode used here) the measurement
table is constantly updated, so trigger is not useful and would only
slow down the reading. And I did not want to block the data retrieval
when person wants to read the data fast.

> >
> > > > +static int mlx90635_get_refresh_rate(struct mlx90635_data *data,
> > > > +                                  unsigned int *refresh_rate)
> > > > +{
> > > > +     unsigned int reg;
> > > > +     int ret;
> > > > +
> > > > +     if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP)
> > > > +             regcache_cache_only(data->regmap, false);
> > >
> > > Definitely needs a comment on why this is needed in this case.
> > >
> >
> > Here and below (where we turn it back to true?), but then I assume in
> > all other instances as well? Maybe a more general comment in the
> > sleep_step mode function?
>
> If we keep this, then yes I think we need comments on these - even if
> it's as simple as 'not accessing an eeprom register so we want to
> talk to the device'.

OK, then this is an option if I cannot make two regmaps work.

> >
> > > > +
...
> > changed we should end up in correct state. I can wrap a mutex around
> > though.
>
> Assuming regcache_cache_only() isn't refcounted, you could end up with a
> second copy of this racing through and accessing the data after the
> first one turned the cache back on so the -EBUSY your mentioned.
>

True. I will use mutex then for this action.