RE: [PATCH v5] iio: temperature: Add driver support for Maxim MAX30208

[Khandelwal, Rajat]

Hi Guenter,
Thanks for the acknowledgement and your comments. 

>That is your use case. I don't know how IIO drivers are normally implemented, but I would expect a generic driver. In this case, I would expect >userspace to decide what it wants to with the data and not let the kernel driver discard most of it.
So, essentially, this driver is not discarding but there is no way to get to the recent most reading without popping all the values
before it. Call it a device limitation? Hence, if FIFO contains more than 1 reading, there is no other way than to pop out all
the values before the recent most to get there.

> What does that have to do with interrupts ? Anything connected to the gpio pin would trigger a reading.
Yes that’s correct. However, I intend to give out all readings in the FIFO through a defined user space attribute so that user can check all
the triggered conversions instead of popping out and only reading the most recent one. 
I am thinking of doing this via ACPI GPIO interrupt which stores values in a kernel data structure whenever triggered and a user space
attribute printing out all of them. 

> It seems to me that this would warrant an explanation in the driver.
>500ms seems hard to believe.
Yes, I proofread the spec many times to give a reasoning behind this. All I could find is this in the datasheet:
Response Time TA = +0°C to +50°C Unmounted, 63% (Note 2) 0.5s
I assumed this response time would be the one which gives the maximum amount of time to respond. Your comments are also welcome.

>If reading MAX30208_FIFO_OVF_CNTR returns a value > 0, it is used as data_count. That does not seem correct. The data sheet says if >MAX30208_FIFO_OVF_CNTR is != 0, data_count is 32. Maybe the datasheet is wrong all over the place, but at least in this case that seems very >unlikely.
I think you are confusing data_count with "data counter". So, overflow counter becomes active only when there are about 32 readings left in the FIFO to read. Now, in this situation, if I trigger temperature conversion 'x' (x<32) more times, I would want to pop out only (x-1) value to read the most recent one, right?, and not 32 readings. So the fact that data counter gets stuck at the value of '32' is correct but what we want is the number of readings to pop to get the most recent one and if overflow counter is >0, the number is indicated by the overflow counter itself. 

Inviting Jonathan and Guenter for further speculations and to comment on v6.

Thanks
Rajat

-----Original Message-----
From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
Sent: Tuesday, October 25, 2022 12:02 AM
To: Khandelwal, Rajat <rajat.khandelwal@xxxxxxxxx>
Cc: Rajat Khandelwal <rajat.khandelwal@xxxxxxxxxxxxxxx>; jic23@xxxxxxxxxx; lars@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx; jdelvare@xxxxxxxx; linux-hwmon@xxxxxxxxxxxxxxx
Subject: Re: [PATCH v5] iio: temperature: Add driver support for Maxim MAX30208

On Mon, Oct 24, 2022 at 05:11:17PM +0000, Khandelwal, Rajat wrote:
> Hi Guenter,
> Thanks for the acknowledgement.
> 
> >Agreed; the sensor doesn't seem to be very useful for traditional 
> >hardware monitoring. The driver better resides in IIO.
> Cool! I didn't know the categorical reasoning behind this but since 
> this is accepted in IIO, I don't have to do anything more.

Huh. There is no "categorical" reasoning. Call it a gut feeling.
I can not imagine anyone using this chip for hardware monitoring, and presumably you have an IIO use case or you would not have implemented an IIO driver.

> 
> >I don't understand why readings are discarded. Why trigger multiple 
> >readings just to discard all but the last one ? I thought iio would 
> >be expected to return all values.
> Ok. The plan is to trigger temperature conversion on the GPIO input also.
> The user can trigger as many times the temperature conversion he wants 
> (I accept unnecessary), which will keep the FIFO increasing (without 
> reading converted values) but the driver should be resilient to all 
> the erroneous zones. Also, when the user does really make a syscall to read the temperature, it definitely should be the last converted reading.

That is your use case. I don't know how IIO drivers are normally implemented, but I would expect a generic driver. In this case, I would expect userspace to decide what it wants to with the data and not let the kernel driver discard most of it.

> 
> >This is really pointless. The register has only one bit to set.
> >Just write that bit; reading the register before that is pointless.
> I think the register also has some bits which are reserved. Hence, 
> rather than to make a number for specifically the value keeping those 
> bits the same, I read whatever is there and only store the required one.
> 
I personally would not accept that kind of code, but that is just me.

> >Also, the code assumes that one of the gpio input registers would be 
> >used to trigger temperature readings. Why trigger another one if this 
> >is indeed the case ? Triggering a temperature reading should only be 
> >necessary if there is no data in the fifo.
> GPIO input triggering is yet not implemented as I would have to work 
> on ACPI interrupts and I have written the driver for now to get it included in Linux.
> There are 2 ways - via GPIO and making a syscall. I agree that 
> temperature reading should be necessary only when there is no data in 
> FIFO but since we intend to keep GPIO as a trigger point, user can 
> keep triggering conversions and not reading them out. (As pointed above, driver should be resilient to all erroneous zones).

What does that have to do with interrupts ? Anything connected to the gpio pin would trigger a reading.

> 
> >The datasheet says that it can take up to 50 ms to report a result.
> >10 retries with 50ms wait each time seems overkill.
> That's correct. But, the response time can be up to 500 ms. Also, 
> while debugging I had put timestamps which when analyzed, indicated that time may go beyond 50 ms.
> 

It seems to me that this would warrant an explanation in the driver.
500ms seems hard to believe.

> >And why use usleep_range() here
> >but msleep() above ?
> I am sorry about that. I have converted usleep_range into msleep (2 places). 
> 
> >This is wrong. It uses the overflow counter as data counter if it is 
> >!= 0. The overflow counter counts the number of overflows, not the 
> >number of entries in the fifo.
> So there is no such thing as 'overflow counter'. The point is if the 
> overflow counter has

Interesting statement. MAX30208_FIFO_OVF_CNTR very much sounds like overflow counter to me, and the datasheet suggests the same.

> even one word, I use the data count equal to the overflow counter 
> value. However, if it has zero, then use the number of words in actual FIFO.
> This logic is just used to count how many values to pop to get the most recent reading.
> 

The code is

+       ret = i2c_smbus_read_byte_data(data->client, MAX30208_FIFO_OVF_CNTR);
+       if (ret < 0) {
+               dev_err(&data->client->dev, "Error reading reg FIFO overflow counter\n");
+               goto unlock;
+       } else if (!ret) {
+               ret = i2c_smbus_read_byte_data(data->client,
+                                              MAX30208_FIFO_DATA_CNTR);
+               if (ret < 0) {
+                       dev_err(&data->client->dev, "Error reading reg FIFO data counter\n");
+                       goto unlock;
+               }
+       }
+
+       data_count = ret;

If reading MAX30208_FIFO_OVF_CNTR returns a value > 0, it is used as data_count. That does not seem correct. The data sheet says if MAX30208_FIFO_OVF_CNTR is != 0, data_count is 32. Maybe the datasheet is wrong all over the place, but at least in this case that seems very unlikely.

> > data_count is declared as u8 and will never be < 0.
> Data count can never be <0 as only first few bits of the 8 bits are used in the register. 
> 
	u8 data_count;
...
	data_count = i2c_smbus_read_byte_data(data->client,
                                              MAX30208_FIFO_DATA_CNTR);
        if (data_count < 0) {

Really ? Static analyzers will have a field day with this code.

Anyway, I don't really care much about this code, so I'll let Jonathan take it from here. I just wanted to share my observations.

Thanks,
Guenter