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 > > >
