Re: [PATCH v10] iio: temperature: Add driver support for Maxim MAX30208

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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;
> >> +}
> >> +