Message-ID: <20221024183148.GA3170088@roeck-us.net>
Date:   Mon, 24 Oct 2022 11:31:48 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     "Khandelwal, Rajat" <rajat.khandelwal@...el.com>
Cc:     Rajat Khandelwal <rajat.khandelwal@...ux.intel.com>,
        "jic23@...nel.org" <jic23@...nel.org>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "jdelvare@...e.com" <jdelvare@...e.com>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>
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

Powered by blists - more mailing lists