Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 17 Nov 2022 16:30:40 +0000
From:   Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To:     Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
CC:     <jic23@kernel.org>, <lars@metafoo.de>,
        <linux-kernel@vger.kernel.org>, <linux-iio@vger.kernel.org>,
        <jdelvare@suse.com>, <linux@roeck-us.net>,
        <linux-hwmon@vger.kernel.org>, <rajat.khandelwal@intel.com>
Subject: Re: [PATCH v10] iio: temperature: Add driver support for Maxim
 MAX30208
Message-ID: <20221117163040.00001f5a@Huawei.com>
In-Reply-To: <20221118153729.762018-1-rajat.khandelwal@linux.intel.com>
References: <20221118153729.762018-1-rajat.khandelwal@linux.intel.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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.

> 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!

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