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. > 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! > + > +unlock: > + mutex_unlock(&data->lock); > + return ret; > +} > +
