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
