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.
