lkml.org 
[lkml]   [2022]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v10] iio: temperature: Add driver support for Maxim MAX30208
On Fri, 18 Nov 2022 18:27:10 +0530
Rajat Khandelwal <rajat.khandelwal@linux.intel.com> wrote:

> Have provided inline comments.
> Please provide your comments for me to spin a v11 :)
>
> On 11/17/2022 10:00 PM, Jonathan Cameron wrote:
> > On Fri, 18 Nov 2022 21:07:29 +0530
> > Rajat Khandelwal<rajat.khandelwal@linux.intel.com> wrote:
> >
> >> Maxim MAX30208 is a digital temperature sensor with 0.1°C accuracy.
> >>
> >> Add support for max30208 driver in iio subsystem.
> > Blank line here.
> >
> >> Datasheet:https://datasheets.maximintegrated.com/en/ds/MAX30208.pdf
> >>
> > Datasheet part of the tags block, so no blank line between that and the SoB.
> > That makes life easy for tools parsing git messages.
>
> - Got it. Will do that.
>
> >
> >> Signed-off-by: Rajat Khandelwal<rajat.khandelwal@linux.intel.com>
> > One query inline. Basically boils down to what we do after
> > overflow occurs. I assume you are right and the first reading is the most recent, but
> > I think we still want to flush the whole fifo in that case to get back to
> > a sane state for future reads.
> >
> > Jonathan
> >
> >> +/**
> >> + * max30208_request() - Request a reading
> >> + * @data: Struct comprising member elements of the device
> >> + *
> >> + * Requests a reading from the device and waits until the conversion is ready.
> >> + */
> >> +static int max30208_request(struct max30208_data *data)
> >> +{
> >> + /*
> >> + * Sensor can take up to 500 ms to respond so execute a total of
> >> + * 10 retries to give the device sufficient time.
> >> + */
> >> + int retries = 10;
> >> + u8 regval;
> >> + int ret;
> >> +
> >> + ret = i2c_smbus_read_byte_data(data->client, MAX30208_TEMP_SENSOR_SETUP);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + regval = ret | MAX30208_TEMP_SENSOR_SETUP_CONV;
> >> +
> >> + ret = i2c_smbus_write_byte_data(data->client, MAX30208_TEMP_SENSOR_SETUP, regval);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + while (retries--) {
> >> + ret = i2c_smbus_read_byte_data(data->client, MAX30208_STATUS);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + if (ret & MAX30208_STATUS_TEMP_RDY)
> >> + return 0;
> >> +
> >> + msleep(50);
> >> + }
> >> + dev_err(&data->client->dev, "Temperature conversion failed\n");
> >> +
> >> + return -ETIMEDOUT;
> >> +}
> >> +
> >> +static int max30208_update_temp(struct max30208_data *data)
> >> +{
> >> + u8 data_count;
> >> + int ret;
> >> +
> >> + mutex_lock(&data->lock);
> >> +
> >> + ret = max30208_request(data);
> >> + if (ret)
> >> + goto unlock;
> >> +
> >> + ret = i2c_smbus_read_byte_data(data->client, MAX30208_FIFO_OVF_CNTR);
> >> + if (ret < 0)
> >> + goto unlock;
> >> + else if (!ret) {
> >> + ret = i2c_smbus_read_byte_data(data->client, MAX30208_FIFO_DATA_CNTR);
> >> + if (ret < 0)
> >> + goto unlock;
> >> +
> >> + data_count = ret;
> >> + } else
> >> + data_count = 1;
> >> +
> >> + while (data_count) {
> >> + ret = i2c_smbus_read_word_swapped(data->client, MAX30208_FIFO_DATA);
> >> + if (ret < 0)
> >> + goto unlock;
> >> +
> >> + data_count--;
> >> + }
> > Hmm. Given you've been poking this a lot, I guess this works and the part is
> > as just odd. Just to check one last case... Does max30208_request() guarantee we can't
> > get...
> >
> > 1. Read first time, overflow set so we read latest result - leaving
> > 31 ancient values in the fifo.
> > 2. Read again really quickly and get those ancient values.
> > ?
> >
> > Perhaps we should flush out those unwanted values from the fifo, so after
> > overflow we get back to a normal state rather than immediately overflowing again.
> >
> > More than possible that I still don't understand how this device works though!
>
> - Ok, so whenever user wants a temperature reading, conversion first takes place and then
> the reading gets returned. So, user will always get the latest converted reading despite
> the number of ancient readings.
> Flushing everytime we get an overflow is not required I think because even though overflow
> could happen again, user still gets the latest updated reading. Also, I plan to incorporate
> buffered flow in IIO. Even though, I think let FIFO remain intact because it doesn't impact
> the recent readings.

Fair enough. Sounds like yes, we are guaranteed there will always be a new reading before
we start popping entries off the fifo again. If that's the case, all is fine as is - was
just really hard to figure that out from the code / datasheet, so I wanted to check.

Jonathan

>
> >> +
> >> +unlock:
> >> + mutex_unlock(&data->lock);
> >> + return ret;
> >> +}
> >> +

\
 
 \ /
  Last update: 2022-11-21 17:43    [W:0.262 / U:0.344 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site