On Fri, 18 Nov 2022 18:27:10 +0530 Rajat Khandelwal <rajat.khandelwal@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> > > 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; > >> +} > >> +
