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

[Crt Mori]

On Sun, 26 Nov 2023 at 17:01, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Sat, 25 Nov 2023 22:26:46 +0100
> Crt Mori <cmo@xxxxxxxxxxx> wrote:
>
> > On Sat, 25 Nov 2023 at 18:53, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> > >
> > > On Wed, 22 Nov 2023 11:24:06 +0100
> > > Crt Mori <cmo@xxxxxxxxxxx> wrote:
> > >
> > > > MLX90635 is an Infra Red contactless temperature sensor most suitable
> > > > for consumer applications where measured object temperature is in range
> > > > between -20 to 100 degrees Celsius. It has improved accuracy for
> > > > measurements within temperature range of human body and can operate in
> > > > ambient temperature range between -20 to 85 degrees Celsius.
> > > >
> > > > Driver provides simple power management possibility as it returns to
> > > > lowest possible power mode (Step sleep mode) in which temperature
> > > > measurements can still be performed, yet for continuous measuring it
> > > > switches to Continuous power mode where measurements constantly change
> > > > without triggering.
> > > >
> > > > Signed-off-by: Crt Mori<cmo@xxxxxxxxxxx>
> > > Hi Crt,
> > >
> > > Very nice. A few minor bits inline.
> > >
> > > Note (as normal for me), I haven't sanity checked any calibration maths - just assuming
> > > you got that bit right as don't want to spend ages comparing datasheet maths to what
> > > you have coded up + I'm not sure I can get the datasheet anyway :)
> > >
> > > Jonathan
> > >
> > Hi Jonathan,
> > Maths I have unit tests where I did floating point (which will be
> > released as embedded library same as for 90632) to integer conversion
> > and ensure that the delta is less then the error of the sensor. So
> > math I take full responsibility :)
> >
> > Datasheet will be public probably in March, when hopefully the sensor
> > is already part of the main Android kernel as well. But your review is
> > anyway very valuable and detailed. Thanks for all the remarks - there
> > is just one discussion below I would love to complete for future
> > reference.
> >
> > Best regards,
> > Crt
...
> > any reads on oscilloscope and why I need to physically read registers
> > below, so that they are cached.
>
> I agree the docs are a little vague, but looking at the code there
> aren't any reads, only writes to the device so I think this behaviour
> is by design.  The cache is considered correctly synced if an entry
> is simply not cached (valid I suppose as no wrong values cached).
>
> > regmap totally knows which registers
> > it should cache, but it does not at init, nor at regcache_sync
> > request. And if you remember in 90632 I had a similar remark, but
> > could not reproduce as EEPROM was readable in most powermodes (well
> > all used in driver), now I checked with scope and since I know this
> > chip does not allow EEPROM reading during the step sleep mode, so
> > everything was much easier to conclude.
>
> I think the key here is that the cache isn't really meant to provide
> access to values when the device is powered down; it is there to
> reduce bus traffic. Hence the last thing you normally want to do is
> read back all the values.  There is a way to get that to happen
> on init though.
>
> You want to hit this path:
> https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regcache.c#L183
> I think you set num_reg_defaults_raw = Number of registers, but don't provide any
> default values, so as to indicate they should be read from the device.
>

Ok, then I have understood this correctly I should only retain
regcache_sync and abandon any other call to sync or complete and just
rely on the natural state of the cache (and the toggling of the
cache_only flag I am doing here). Will roll v2 with that, after I
confirm it still works.

> > ...
> > > > +     mlx90635 = iio_priv(indio_dev);
> > > > +     i2c_set_clientdata(client, indio_dev);
> > > > +     mlx90635->client = client;
> > > > +     mlx90635->regmap = regmap;
> > > > +     mlx90635->powerstatus = MLX90635_PWR_STATUS_SLEEP_STEP;
> > > > +
> > > > +     mutex_init(&mlx90635->lock);
> > > > +     indio_dev->name = id->name;
> > >
> > > Not keen on doing this as it can be fragile if id and of tables get out of sync
> > > or we are using backwards compatibles in dt bindings.
> > >
> > > Given only one part supported, just hard code the name for now.
> > >
> >
> > Can you elaborate? Because I have the same thing in 90632 and I would
> > fix there as well. I assumed this is for linking to dt, to ensure it
> > is defined there?
>
> Exactly, the dt table and the i2c_id one can end up out of sync - perhaps
> deliberately and when compatible = "device1", "device2" is used
> I'm not sure exactly which one will turn up in this id if the dt table
> only includes device2.
>
> So rather than arguing that out and pinning down the expected behaviour
> we tend to avoid using id->name in new drivers.
> It's probably not broken to do so but lets make life easier for any
> future cases by not doing it.
>
>

Ok, will also roll the fix for the existing two drivers.

> >
...
> > >
> > > I'd like to understand what breaks if this happens but we carry on anyway?
> > > I'd 'hope' that any future DSP version is backwards compatible or that there was some way to know if
> > > the difference between backwards compatible versions and ones that aren't.
> > >
> > The top bit in high nibble is fixed to 1, to ensure that we have
> > endianness correct in the wild. We did the same later on in 90632
> > where we had plenty of trouble the way people read 16 bits. So if that
> > top bit is not there (bottom nibble has it hardcoded to 0), then for
> > certain we do not have the correct chip. And as for compatible DSP
> > versions: when we release incompatible one, I will upgrade the driver,
> > otherwise we have some more slack in the driver to keep on working,
> > because that was also lesson learnt from my side in 90632 as there are
> > compatible DSP versions possible and used, but we are still honest and
> > bump also the DSP version here.
>
> So there isn't a clear division between 'minor and major' version numbers
> as used in many similar cases? Minor can tick without a driver change as the
> interface only grows (nothing breaks), but major implies incompatible.
>
> Anyhow, sounds like you carry on with just a dbg print if an unexpected version
> seen. That's fine.
>

I am trying to convince them to do that, but your comment will provide
me some leverage for my case.  As usual, I was assured there would be
no DSPv2 anyway. Thanks for the feedback.

Crt
>
> Jonathan
>
>
>