[v5] iio: temperature: Add driver support for Maxim MAX30208

  Maxim MAX30208 is a digital temperature sensor with 0.1°C accuracy.

Add support for max30208 driver in iio subsystem.
Datasheet: https://datasheets.maximintegrated.com/en/ds/MAX30208.pdf

Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
---

v5:
1. Fixed comment position in max30208_request
2. Use of local u8 variable to build register values
3. Using u8 instead of s8 in data_count
4. Removed global MAX30208_RES_MILLICELCIUS
5. Removed 'comma' on NULL terminators

v4: Version comments go below line separator of signed-off-by

v3: Release the mutex lock after error gets returned

v2:
1. Removed TODO
2. Removed unnecessary blank spaces
3. Corrected MC->MILLICELCIUS
4. Comments added wherever required
5. dev_err on i2c fails
6. Rearranged some flows
7. Removed PROCESSED
8. int error return on gpio setup
9. device_register at the end of probe
10. Return on unsuccessful reset
11. acpi_match_table and of_match_table added
12. Minor quirks

 MAINTAINERS                        |   6 +
 drivers/iio/temperature/Kconfig    |  10 +
 drivers/iio/temperature/Makefile   |   1 +
 drivers/iio/temperature/max30208.c | 323 +++++++++++++++++++++++++++++
 4 files changed, 340 insertions(+)
 create mode 100644 drivers/iio/temperature/max30208.c
  

On Mon, Oct 24, 2022 at 10:26:58PM +0530, Rajat Khandelwal wrote:
> Maxim MAX30208 is a digital temperature sensor with 0.1°C accuracy.
> 
> Add support for max30208 driver in iio subsystem.
> Datasheet: https://datasheets.maximintegrated.com/en/ds/MAX30208.pdf
> 
> Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>

Question was:

> with the open question on whether they consider it acceptable for this driver
> to be in IIO. Main argument in favour is it's an expensive clinical grade sensor
> so fairly unlikely to be used in classic hardware monitoring circumstances.

Agreed; the sensor doesn't seem to be very useful for traditional hardware
monitoring. The driver better resides in IIO.

I don't understand why readings are discarded. Why trigger multiple
readings just to discard all but the last one ? I thought iio would
be expected to return all values.

Additional comment inline.

> ---
> 
> v5:
> 1. Fixed comment position in max30208_request
> 2. Use of local u8 variable to build register values
> 3. Using u8 instead of s8 in data_count
> 4. Removed global MAX30208_RES_MILLICELCIUS
> 5. Removed 'comma' on NULL terminators
> 
> v4: Version comments go below line separator of signed-off-by
> 
> v3: Release the mutex lock after error gets returned
> 
> v2:
> 1. Removed TODO
> 2. Removed unnecessary blank spaces
> 3. Corrected MC->MILLICELCIUS
> 4. Comments added wherever required
> 5. dev_err on i2c fails
> 6. Rearranged some flows
> 7. Removed PROCESSED
> 8. int error return on gpio setup
> 9. device_register at the end of probe
> 10. Return on unsuccessful reset
> 11. acpi_match_table and of_match_table added
> 12. Minor quirks
> 
>  MAINTAINERS                        |   6 +
>  drivers/iio/temperature/Kconfig    |  10 +
>  drivers/iio/temperature/Makefile   |   1 +
>  drivers/iio/temperature/max30208.c | 323 +++++++++++++++++++++++++++++
>  4 files changed, 340 insertions(+)
>  create mode 100644 drivers/iio/temperature/max30208.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f1390b8270b2..7f1fd2e31b94 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12373,6 +12373,12 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/regulator/maxim,max20086.yaml
>  F:	drivers/regulator/max20086-regulator.c
>  
> +MAXIM MAX30208 TEMPERATURE SENSOR DRIVER
> +M:	Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
> +L:	linux-iio@vger.kernel.org
> +S:	Maintained
> +F:	drivers/iio/temperature/max30208.c
> +
>  MAXIM MAX77650 PMIC MFD DRIVER
>  M:	Bartosz Golaszewski <brgl@bgdev.pl>
>  L:	linux-kernel@vger.kernel.org
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index e8ed849e3b76..ed384f33e0c7 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -128,6 +128,16 @@ config TSYS02D
>  	  This driver can also be built as a module. If so, the module will
>  	  be called tsys02d.
>  
> +config MAX30208
> +	tristate "Maxim MAX30208 digital temperature sensor"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for Maxim MAX30208
> +	  digital temperature sensor connected via I2C.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called max30208.
> +
>  config MAX31856
>  	tristate "MAX31856 thermocouple sensor"
>  	depends on SPI
> diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
> index dd08e562ffe0..dfec8c6d3019 100644
> --- a/drivers/iio/temperature/Makefile
> +++ b/drivers/iio/temperature/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_IQS620AT_TEMP) += iqs620at-temp.o
>  obj-$(CONFIG_LTC2983) += ltc2983.o
>  obj-$(CONFIG_HID_SENSOR_TEMP) += hid-sensor-temperature.o
>  obj-$(CONFIG_MAXIM_THERMOCOUPLE) += maxim_thermocouple.o
> +obj-$(CONFIG_MAX30208) += max30208.o
>  obj-$(CONFIG_MAX31856) += max31856.o
>  obj-$(CONFIG_MAX31865) += max31865.o
>  obj-$(CONFIG_MLX90614) += mlx90614.o
> diff --git a/drivers/iio/temperature/max30208.c b/drivers/iio/temperature/max30208.c
> new file mode 100644
> index 000000000000..4f78337c78fe
> --- /dev/null
> +++ b/drivers/iio/temperature/max30208.c
> @@ -0,0 +1,323 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Copyright (c) Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
> + *
> + * Maxim MAX30208 digital temperature sensor with 0.1°C accuracy
> + * (7-bit I2C slave address (0x50 - 0x53))
> + *
> + * Note: This driver sets GPIO1 to behave as input for temperature
> + * conversion and GPIO0 to act as interrupt for temperature conversion.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +
> +#define MAX30208_DRV_NAME		"max30208"
> +
> +#define MAX30208_STATUS			0x00
> +#define MAX30208_STATUS_TEMP_RDY	BIT(0)
> +#define MAX30208_INT_ENABLE		0x01
> +#define MAX30208_INT_ENABLE_TEMP_RDY	BIT(0)
> +
> +#define MAX30208_FIFO_OVF_CNTR		0x06
> +#define MAX30208_FIFO_DATA_CNTR		0x07
> +#define MAX30208_FIFO_DATA		0x08
> +
> +#define MAX30208_SYSTEM_CTRL		0x0c
> +#define MAX30208_SYSTEM_CTRL_RESET	0x01
> +
> +#define MAX30208_TEMP_SENSOR_SETUP	0x14
> +#define MAX30208_TEMP_SENSOR_SETUP_CONV	BIT(0)
> +
> +#define MAX30208_GPIO_SETUP		0x20
> +#define MAX30208_GPIO1_SETUP		GENMASK(7, 6)
> +#define MAX30208_GPIO0_SETUP		GENMASK(1, 0)
> +#define MAX30208_GPIO_CTRL		0x21
> +#define MAX30208_GPIO1_CTRL		BIT(3)
> +#define MAX30208_GPIO0_CTRL		BIT(0)
> +
> +struct max30208_data {
> +	struct i2c_client *client;
> +	struct iio_dev *indio_dev;
> +	struct mutex lock; /* Lock to prevent concurrent reads */
> +};
> +
> +static const struct iio_chan_spec max30208_channels[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +};
> +
> +/**
> + * 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) {
> +		dev_err(&data->client->dev, "Error reading reg temperature setup\n");
> +		return ret;
> +	}
> +
> +	regval = ret | MAX30208_TEMP_SENSOR_SETUP_CONV;
> +

This is really pointless. The register has only one bit to set.
Just write that bit; reading the register before that is pointless. 

Also, the code assumes that one of the gpio input registers would be used
to trigger temperature readings. Why trigger another one if this is indeed
the case ? Triggering a temperature reading should only be necessary if
there is no data in the fifo.

> +	ret = i2c_smbus_write_byte_data(data->client, MAX30208_TEMP_SENSOR_SETUP, regval);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg temperature setup\n");

Not my call to make, but this driver is really noisy.

> +		return ret;
> +	}
> +
> +	while (retries--) {
> +		ret = i2c_smbus_read_byte_data(data->client, MAX30208_STATUS);
> +		if (ret < 0) {
> +			dev_err(&data->client->dev, "Error reading reg status\n");
> +			goto sleep;
> +		}
> +
> +		if (ret & MAX30208_STATUS_TEMP_RDY)
> +			return 0;
> +
> +		msleep(50);
> +	}

The datasheet says that it can take up to 50 ms to report a result.
10 retries with 50ms wait each time seems overkill.

> +	dev_warn(&data->client->dev, "Temperature conversion failed\n");
> +
> +	return 0;
> +
> +sleep:
> +	usleep_range(50000, 60000);
> +	return 0;

Odd error handling. And why use usleep_range() here
but msleep() above ?

> +}
> +
> +static int max30208_update_temp(struct max30208_data *data)
> +{
> +	u8 data_count;
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +
> +	ret = max30208_request(data);
> +	if (ret < 0)
> +		goto unlock;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MAX30208_FIFO_OVF_CNTR);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg FIFO overflow counter\n");
> +		goto unlock;
> +	} else if (!ret) {
> +		ret = i2c_smbus_read_byte_data(data->client,
> +					       MAX30208_FIFO_DATA_CNTR);
> +		if (ret < 0) {
> +			dev_err(&data->client->dev, "Error reading reg FIFO data counter\n");
> +			goto unlock;
> +		}
> +	}
> +
> +	data_count = ret;

This is wrong. It uses the overflow counter as data counter if it
is != 0. The overflow counter counts the number of overflows, not
the number of entries in the fifo.

> +
> +	/*
> +	 * Ideally, counter should decrease by 1 each time a word is read from FIFO.
> +	 * However, practically, the device behaves erroneously and counter sometimes
> +	 * decreases by more than 1. Hence, do not loop the counter until it becomes 0
> +	 * rather, use the exact counter value after each FIFO word is read.
> +	 * Return the last reading from FIFO as the most recently triggered one.
> +	 */
> +	while (data_count) {
> +		ret = i2c_smbus_read_word_swapped(data->client,
> +						  MAX30208_FIFO_DATA);
> +		if (ret < 0) {
> +			dev_err(&data->client->dev, "Error reading reg FIFO data\n");
> +			goto unlock;
> +		}
> +
> +		data_count = i2c_smbus_read_byte_data(data->client,
> +						      MAX30208_FIFO_DATA_CNTR);
> +		if (data_count < 0) {
> +			dev_err(&data->client->dev, "Error reading reg FIFO data counter\n");
> +			ret = data_count;
> +			goto unlock;
> +		}

data_count is declared as u8 and will never be < 0.

> +	}
> +
> +unlock:
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
> +static int max30208_read(struct iio_dev *indio_dev,
> +			 struct iio_chan_spec const *chan,
> +			 int *val, int *val2, long mask)
> +{
> +	struct max30208_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = max30208_update_temp(data);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = sign_extend32(ret, 15);
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 5;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int max30208_gpio_setup(struct max30208_data *data)
> +{
> +	u8 regval;
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client,
> +				       MAX30208_GPIO_SETUP);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg GPIO setup\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * Setting GPIO1 to trigger temperature conversion
> +	 * when driven low.
> +	 * Setting GPIO0 to trigger interrupt when temperature
> +	 * conversion gets completed.
> +	 */
> +	regval = ret | MAX30208_GPIO1_SETUP | MAX30208_GPIO0_SETUP;
> +	ret = i2c_smbus_write_byte_data(data->client,
> +					MAX30208_GPIO_SETUP, regval);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg GPIO setup\n");
> +		return ret;
> +	}
> +
> +	ret = i2c_smbus_read_byte_data(data->client,
> +				       MAX30208_INT_ENABLE);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg interrupt enable\n");
> +		return ret;
> +	}
> +
> +	/* Enabling GPIO0 interrupt */
> +	regval = ret | MAX30208_INT_ENABLE_TEMP_RDY;
> +	ret = i2c_smbus_write_byte_data(data->client,
> +					MAX30208_INT_ENABLE, regval);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg interrupt enable\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct iio_info max30208_info = {
> +	.read_raw = max30208_read,
> +};
> +
> +static int max30208_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct max30208_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->client = i2c;
> +	mutex_init(&data->lock);
> +
> +	indio_dev->name = MAX30208_DRV_NAME;
> +	indio_dev->channels = max30208_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max30208_channels);
> +	indio_dev->info = &max30208_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	/* Reset the device initially */
> +	ret = i2c_smbus_write_byte_data(data->client, MAX30208_SYSTEM_CTRL,
> +					MAX30208_SYSTEM_CTRL_RESET);
> +	if (ret) {
> +		dev_err(dev, "Failure in performing reset\n");
> +		return ret;
> +	}
> +
> +	usleep_range(50000, 60000);
> +
> +	ret = max30208_gpio_setup(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to register IIO device\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id max30208_id_table[] = {
> +	{ "max30208" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max30208_id_table);
> +
> +static const struct acpi_device_id max30208_acpi_match[] = {
> +	{ "MAX30208" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, max30208_acpi_match);
> +
> +static const struct of_device_id max30208_of_match[] = {
> +	{ .compatible = "maxim,max30208" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max30208_of_match);
> +
> +static struct i2c_driver max30208_driver = {
> +	.driver = {
> +		.name = MAX30208_DRV_NAME,
> +		.of_match_table = max30208_of_match,
> +		.acpi_match_table = ACPI_PTR(max30208_acpi_match),
> +	},
> +	.probe_new = max30208_probe,
> +	.id_table = max30208_id_table,
> +};
> +
> +static int __init max30208_init(void)
> +{
> +	return i2c_add_driver(&max30208_driver);
> +}
> +module_init(max30208_init);
> +
> +static void __exit max30208_exit(void)
> +{
> +	i2c_del_driver(&max30208_driver);
> +}
> +module_exit(max30208_exit);
> +
> +MODULE_AUTHOR("Rajat Khandelwal <rajat.khandelwal@linux.intel.com>");
> +MODULE_DESCRIPTION("Maxim MAX30208 digital temperature sensor");
> +MODULE_LICENSE("GPL");
  

Hi Guenter,
Thanks for the acknowledgement.

>Agreed; the sensor doesn't seem to be very useful for traditional hardware
>monitoring. The driver better resides in IIO.
Cool! I didn't know the categorical reasoning behind this but since this is 
accepted in IIO, I don't have to do anything more. 

>I don't understand why readings are discarded. Why trigger multiple
>readings just to discard all but the last one ? I thought iio would
>be expected to return all values.
Ok. The plan is to trigger temperature conversion on the GPIO input also.
The user can trigger as many times the temperature conversion he wants (I accept unnecessary),
which will keep the FIFO increasing (without reading converted values) but the driver should be
resilient to all the erroneous zones. Also, when the user does really make a syscall to read the
temperature, it definitely should be the last converted reading. 

>This is really pointless. The register has only one bit to set.
>Just write that bit; reading the register before that is pointless.
I think the register also has some bits which are reserved. Hence, rather than to make a number
for specifically the value keeping those bits the same, I read whatever is there and only store the
required one. 

>Also, the code assumes that one of the gpio input registers would be used
>to trigger temperature readings. Why trigger another one if this is indeed
>the case ? Triggering a temperature reading should only be necessary if
>there is no data in the fifo.
GPIO input triggering is yet not implemented as I would have to work on ACPI interrupts and I have
written the driver for now to get it included in Linux. 
There are 2 ways - via GPIO and making a syscall. I agree that temperature reading should be
necessary only when there is no data in FIFO but since we intend to keep GPIO as a trigger point,
user can keep triggering conversions and not reading them out. (As pointed above, driver should be
resilient to all erroneous zones).

>The datasheet says that it can take up to 50 ms to report a result.
>10 retries with 50ms wait each time seems overkill.
That's correct. But, the response time can be up to 500 ms. Also, while debugging I had put timestamps
which when analyzed, indicated that time may go beyond 50 ms. 

>And why use usleep_range() here
>but msleep() above ?
I am sorry about that. I have converted usleep_range into msleep (2 places). 

>This is wrong. It uses the overflow counter as data counter if it
>is != 0. The overflow counter counts the number of overflows, not
>the number of entries in the fifo.
So there is no such thing as 'overflow counter'. The point is if the overflow counter has
even one word, I use the data count equal to the overflow counter value. However, if it
has zero, then use the number of words in actual FIFO. 
This logic is just used to count how many values to pop to get the most recent reading.

> data_count is declared as u8 and will never be < 0.
Data count can never be <0 as only first few bits of the 8 bits are used in the register. 

I have further pushed v6 for your perusal.

Thanks
Rajat

-----Original Message-----
From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
Sent: Monday, October 24, 2022 4:58 PM
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-hwmon@vger.kernel.org; Khandelwal, Rajat <rajat.khandelwal@intel.com>
Subject: Re: [PATCH v5] iio: temperature: Add driver support for Maxim MAX30208

On Mon, Oct 24, 2022 at 10:26:58PM +0530, Rajat Khandelwal wrote:
> Maxim MAX30208 is a digital temperature sensor with 0.1°C accuracy.
> 
> Add support for max30208 driver in iio subsystem.
> Datasheet: https://datasheets.maximintegrated.com/en/ds/MAX30208.pdf
> 
> Signed-off-by: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>

Question was:

> with the open question on whether they consider it acceptable for this driver
> to be in IIO. Main argument in favour is it's an expensive clinical grade sensor
> so fairly unlikely to be used in classic hardware monitoring circumstances.

Agreed; the sensor doesn't seem to be very useful for traditional hardware
monitoring. The driver better resides in IIO.

I don't understand why readings are discarded. Why trigger multiple
readings just to discard all but the last one ? I thought iio would
be expected to return all values.

Additional comment inline.

> ---
> 
> v5:
> 1. Fixed comment position in max30208_request
> 2. Use of local u8 variable to build register values
> 3. Using u8 instead of s8 in data_count
> 4. Removed global MAX30208_RES_MILLICELCIUS
> 5. Removed 'comma' on NULL terminators
> 
> v4: Version comments go below line separator of signed-off-by
> 
> v3: Release the mutex lock after error gets returned
> 
> v2:
> 1. Removed TODO
> 2. Removed unnecessary blank spaces
> 3. Corrected MC->MILLICELCIUS
> 4. Comments added wherever required
> 5. dev_err on i2c fails
> 6. Rearranged some flows
> 7. Removed PROCESSED
> 8. int error return on gpio setup
> 9. device_register at the end of probe
> 10. Return on unsuccessful reset
> 11. acpi_match_table and of_match_table added
> 12. Minor quirks
> 
>  MAINTAINERS                        |   6 +
>  drivers/iio/temperature/Kconfig    |  10 +
>  drivers/iio/temperature/Makefile   |   1 +
>  drivers/iio/temperature/max30208.c | 323 +++++++++++++++++++++++++++++
>  4 files changed, 340 insertions(+)
>  create mode 100644 drivers/iio/temperature/max30208.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f1390b8270b2..7f1fd2e31b94 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12373,6 +12373,12 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/regulator/maxim,max20086.yaml
>  F:	drivers/regulator/max20086-regulator.c
>  
> +MAXIM MAX30208 TEMPERATURE SENSOR DRIVER
> +M:	Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
> +L:	linux-iio@vger.kernel.org
> +S:	Maintained
> +F:	drivers/iio/temperature/max30208.c
> +
>  MAXIM MAX77650 PMIC MFD DRIVER
>  M:	Bartosz Golaszewski <brgl@bgdev.pl>
>  L:	linux-kernel@vger.kernel.org
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index e8ed849e3b76..ed384f33e0c7 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -128,6 +128,16 @@ config TSYS02D
>  	  This driver can also be built as a module. If so, the module will
>  	  be called tsys02d.
>  
> +config MAX30208
> +	tristate "Maxim MAX30208 digital temperature sensor"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for Maxim MAX30208
> +	  digital temperature sensor connected via I2C.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called max30208.
> +
>  config MAX31856
>  	tristate "MAX31856 thermocouple sensor"
>  	depends on SPI
> diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
> index dd08e562ffe0..dfec8c6d3019 100644
> --- a/drivers/iio/temperature/Makefile
> +++ b/drivers/iio/temperature/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_IQS620AT_TEMP) += iqs620at-temp.o
>  obj-$(CONFIG_LTC2983) += ltc2983.o
>  obj-$(CONFIG_HID_SENSOR_TEMP) += hid-sensor-temperature.o
>  obj-$(CONFIG_MAXIM_THERMOCOUPLE) += maxim_thermocouple.o
> +obj-$(CONFIG_MAX30208) += max30208.o
>  obj-$(CONFIG_MAX31856) += max31856.o
>  obj-$(CONFIG_MAX31865) += max31865.o
>  obj-$(CONFIG_MLX90614) += mlx90614.o
> diff --git a/drivers/iio/temperature/max30208.c b/drivers/iio/temperature/max30208.c
> new file mode 100644
> index 000000000000..4f78337c78fe
> --- /dev/null
> +++ b/drivers/iio/temperature/max30208.c
> @@ -0,0 +1,323 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Copyright (c) Rajat Khandelwal <rajat.khandelwal@linux.intel.com>
> + *
> + * Maxim MAX30208 digital temperature sensor with 0.1°C accuracy
> + * (7-bit I2C slave address (0x50 - 0x53))
> + *
> + * Note: This driver sets GPIO1 to behave as input for temperature
> + * conversion and GPIO0 to act as interrupt for temperature conversion.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +
> +#define MAX30208_DRV_NAME		"max30208"
> +
> +#define MAX30208_STATUS			0x00
> +#define MAX30208_STATUS_TEMP_RDY	BIT(0)
> +#define MAX30208_INT_ENABLE		0x01
> +#define MAX30208_INT_ENABLE_TEMP_RDY	BIT(0)
> +
> +#define MAX30208_FIFO_OVF_CNTR		0x06
> +#define MAX30208_FIFO_DATA_CNTR		0x07
> +#define MAX30208_FIFO_DATA		0x08
> +
> +#define MAX30208_SYSTEM_CTRL		0x0c
> +#define MAX30208_SYSTEM_CTRL_RESET	0x01
> +
> +#define MAX30208_TEMP_SENSOR_SETUP	0x14
> +#define MAX30208_TEMP_SENSOR_SETUP_CONV	BIT(0)
> +
> +#define MAX30208_GPIO_SETUP		0x20
> +#define MAX30208_GPIO1_SETUP		GENMASK(7, 6)
> +#define MAX30208_GPIO0_SETUP		GENMASK(1, 0)
> +#define MAX30208_GPIO_CTRL		0x21
> +#define MAX30208_GPIO1_CTRL		BIT(3)
> +#define MAX30208_GPIO0_CTRL		BIT(0)
> +
> +struct max30208_data {
> +	struct i2c_client *client;
> +	struct iio_dev *indio_dev;
> +	struct mutex lock; /* Lock to prevent concurrent reads */
> +};
> +
> +static const struct iio_chan_spec max30208_channels[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +};
> +
> +/**
> + * 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) {
> +		dev_err(&data->client->dev, "Error reading reg temperature setup\n");
> +		return ret;
> +	}
> +
> +	regval = ret | MAX30208_TEMP_SENSOR_SETUP_CONV;
> +

This is really pointless. The register has only one bit to set.
Just write that bit; reading the register before that is pointless. 

Also, the code assumes that one of the gpio input registers would be used
to trigger temperature readings. Why trigger another one if this is indeed
the case ? Triggering a temperature reading should only be necessary if
there is no data in the fifo.

> +	ret = i2c_smbus_write_byte_data(data->client, MAX30208_TEMP_SENSOR_SETUP, regval);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg temperature setup\n");

Not my call to make, but this driver is really noisy.

> +		return ret;
> +	}
> +
> +	while (retries--) {
> +		ret = i2c_smbus_read_byte_data(data->client, MAX30208_STATUS);
> +		if (ret < 0) {
> +			dev_err(&data->client->dev, "Error reading reg status\n");
> +			goto sleep;
> +		}
> +
> +		if (ret & MAX30208_STATUS_TEMP_RDY)
> +			return 0;
> +
> +		msleep(50);
> +	}

The datasheet says that it can take up to 50 ms to report a result.
10 retries with 50ms wait each time seems overkill.

> +	dev_warn(&data->client->dev, "Temperature conversion failed\n");
> +
> +	return 0;
> +
> +sleep:
> +	usleep_range(50000, 60000);
> +	return 0;

Odd error handling. And why use usleep_range() here
but msleep() above ?

> +}
> +
> +static int max30208_update_temp(struct max30208_data *data)
> +{
> +	u8 data_count;
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +
> +	ret = max30208_request(data);
> +	if (ret < 0)
> +		goto unlock;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MAX30208_FIFO_OVF_CNTR);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg FIFO overflow counter\n");
> +		goto unlock;
> +	} else if (!ret) {
> +		ret = i2c_smbus_read_byte_data(data->client,
> +					       MAX30208_FIFO_DATA_CNTR);
> +		if (ret < 0) {
> +			dev_err(&data->client->dev, "Error reading reg FIFO data counter\n");
> +			goto unlock;
> +		}
> +	}
> +
> +	data_count = ret;

This is wrong. It uses the overflow counter as data counter if it
is != 0. The overflow counter counts the number of overflows, not
the number of entries in the fifo.

> +
> +	/*
> +	 * Ideally, counter should decrease by 1 each time a word is read from FIFO.
> +	 * However, practically, the device behaves erroneously and counter sometimes
> +	 * decreases by more than 1. Hence, do not loop the counter until it becomes 0
> +	 * rather, use the exact counter value after each FIFO word is read.
> +	 * Return the last reading from FIFO as the most recently triggered one.
> +	 */
> +	while (data_count) {
> +		ret = i2c_smbus_read_word_swapped(data->client,
> +						  MAX30208_FIFO_DATA);
> +		if (ret < 0) {
> +			dev_err(&data->client->dev, "Error reading reg FIFO data\n");
> +			goto unlock;
> +		}
> +
> +		data_count = i2c_smbus_read_byte_data(data->client,
> +						      MAX30208_FIFO_DATA_CNTR);
> +		if (data_count < 0) {
> +			dev_err(&data->client->dev, "Error reading reg FIFO data counter\n");
> +			ret = data_count;
> +			goto unlock;
> +		}

data_count is declared as u8 and will never be < 0.

> +	}
> +
> +unlock:
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
> +static int max30208_read(struct iio_dev *indio_dev,
> +			 struct iio_chan_spec const *chan,
> +			 int *val, int *val2, long mask)
> +{
> +	struct max30208_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = max30208_update_temp(data);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = sign_extend32(ret, 15);
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 5;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int max30208_gpio_setup(struct max30208_data *data)
> +{
> +	u8 regval;
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client,
> +				       MAX30208_GPIO_SETUP);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg GPIO setup\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * Setting GPIO1 to trigger temperature conversion
> +	 * when driven low.
> +	 * Setting GPIO0 to trigger interrupt when temperature
> +	 * conversion gets completed.
> +	 */
> +	regval = ret | MAX30208_GPIO1_SETUP | MAX30208_GPIO0_SETUP;
> +	ret = i2c_smbus_write_byte_data(data->client,
> +					MAX30208_GPIO_SETUP, regval);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg GPIO setup\n");
> +		return ret;
> +	}
> +
> +	ret = i2c_smbus_read_byte_data(data->client,
> +				       MAX30208_INT_ENABLE);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg interrupt enable\n");
> +		return ret;
> +	}
> +
> +	/* Enabling GPIO0 interrupt */
> +	regval = ret | MAX30208_INT_ENABLE_TEMP_RDY;
> +	ret = i2c_smbus_write_byte_data(data->client,
> +					MAX30208_INT_ENABLE, regval);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg interrupt enable\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct iio_info max30208_info = {
> +	.read_raw = max30208_read,
> +};
> +
> +static int max30208_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct max30208_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->client = i2c;
> +	mutex_init(&data->lock);
> +
> +	indio_dev->name = MAX30208_DRV_NAME;
> +	indio_dev->channels = max30208_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max30208_channels);
> +	indio_dev->info = &max30208_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	/* Reset the device initially */
> +	ret = i2c_smbus_write_byte_data(data->client, MAX30208_SYSTEM_CTRL,
> +					MAX30208_SYSTEM_CTRL_RESET);
> +	if (ret) {
> +		dev_err(dev, "Failure in performing reset\n");
> +		return ret;
> +	}
> +
> +	usleep_range(50000, 60000);
> +
> +	ret = max30208_gpio_setup(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to register IIO device\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id max30208_id_table[] = {
> +	{ "max30208" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max30208_id_table);
> +
> +static const struct acpi_device_id max30208_acpi_match[] = {
> +	{ "MAX30208" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, max30208_acpi_match);
> +
> +static const struct of_device_id max30208_of_match[] = {
> +	{ .compatible = "maxim,max30208" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max30208_of_match);
> +
> +static struct i2c_driver max30208_driver = {
> +	.driver = {
> +		.name = MAX30208_DRV_NAME,
> +		.of_match_table = max30208_of_match,
> +		.acpi_match_table = ACPI_PTR(max30208_acpi_match),
> +	},
> +	.probe_new = max30208_probe,
> +	.id_table = max30208_id_table,
> +};
> +
> +static int __init max30208_init(void)
> +{
> +	return i2c_add_driver(&max30208_driver);
> +}
> +module_init(max30208_init);
> +
> +static void __exit max30208_exit(void)
> +{
> +	i2c_del_driver(&max30208_driver);
> +}
> +module_exit(max30208_exit);
> +
> +MODULE_AUTHOR("Rajat Khandelwal <rajat.khandelwal@linux.intel.com>");
> +MODULE_DESCRIPTION("Maxim MAX30208 digital temperature sensor");
> +MODULE_LICENSE("GPL");
  

On Mon, Oct 24, 2022 at 05:11:17PM +0000, Khandelwal, Rajat wrote:
> Hi Guenter,
> Thanks for the acknowledgement.
> 
> >Agreed; the sensor doesn't seem to be very useful for traditional hardware
> >monitoring. The driver better resides in IIO.
> Cool! I didn't know the categorical reasoning behind this but since this is 
> accepted in IIO, I don't have to do anything more. 

Huh. There is no "categorical" reasoning. Call it a gut feeling.
I can not imagine anyone using this chip for hardware monitoring,
and presumably you have an IIO use case or you would not have
implemented an IIO driver.

> 
> >I don't understand why readings are discarded. Why trigger multiple
> >readings just to discard all but the last one ? I thought iio would
> >be expected to return all values.
> Ok. The plan is to trigger temperature conversion on the GPIO input also.
> The user can trigger as many times the temperature conversion he wants (I accept unnecessary),
> which will keep the FIFO increasing (without reading converted values) but the driver should be
> resilient to all the erroneous zones. Also, when the user does really make a syscall to read the
> temperature, it definitely should be the last converted reading. 

That is your use case. I don't know how IIO drivers are normally
implemented, but I would expect a generic driver. In this case,
I would expect userspace to decide what it wants to with the data
and not let the kernel driver discard most of it.

> 
> >This is really pointless. The register has only one bit to set.
> >Just write that bit; reading the register before that is pointless.
> I think the register also has some bits which are reserved. Hence, rather than to make a number
> for specifically the value keeping those bits the same, I read whatever is there and only store the
> required one. 
> 
I personally would not accept that kind of code, but that is just
me.

> >Also, the code assumes that one of the gpio input registers would be used
> >to trigger temperature readings. Why trigger another one if this is indeed
> >the case ? Triggering a temperature reading should only be necessary if
> >there is no data in the fifo.
> GPIO input triggering is yet not implemented as I would have to work on ACPI interrupts and I have
> written the driver for now to get it included in Linux. 
> There are 2 ways - via GPIO and making a syscall. I agree that temperature reading should be
> necessary only when there is no data in FIFO but since we intend to keep GPIO as a trigger point,
> user can keep triggering conversions and not reading them out. (As pointed above, driver should be
> resilient to all erroneous zones).

What does that have to do with interrupts ? Anything connected to the
gpio pin would trigger a reading.

> 
> >The datasheet says that it can take up to 50 ms to report a result.
> >10 retries with 50ms wait each time seems overkill.
> That's correct. But, the response time can be up to 500 ms. Also, while debugging I had put timestamps
> which when analyzed, indicated that time may go beyond 50 ms. 
> 

It seems to me that this would warrant an explanation in the driver.
500ms seems hard to believe.

> >And why use usleep_range() here
> >but msleep() above ?
> I am sorry about that. I have converted usleep_range into msleep (2 places). 
> 
> >This is wrong. It uses the overflow counter as data counter if it
> >is != 0. The overflow counter counts the number of overflows, not
> >the number of entries in the fifo.
> So there is no such thing as 'overflow counter'. The point is if the overflow counter has

Interesting statement. MAX30208_FIFO_OVF_CNTR very much
sounds like overflow counter to me, and the datasheet
suggests the same.

> even one word, I use the data count equal to the overflow counter value. However, if it
> has zero, then use the number of words in actual FIFO. 
> This logic is just used to count how many values to pop to get the most recent reading.
> 

The code is

+       ret = i2c_smbus_read_byte_data(data->client, MAX30208_FIFO_OVF_CNTR);
+       if (ret < 0) {
+               dev_err(&data->client->dev, "Error reading reg FIFO overflow counter\n");
+               goto unlock;
+       } else if (!ret) {
+               ret = i2c_smbus_read_byte_data(data->client,
+                                              MAX30208_FIFO_DATA_CNTR);
+               if (ret < 0) {
+                       dev_err(&data->client->dev, "Error reading reg FIFO data counter\n");
+                       goto unlock;
+               }
+       }
+
+       data_count = ret;

If reading MAX30208_FIFO_OVF_CNTR returns a value > 0, it is used as
data_count. That does not seem correct. The data sheet says if
MAX30208_FIFO_OVF_CNTR is != 0, data_count is 32. Maybe the datasheet
is wrong all over the place, but at least in this case that seems
very unlikely.

> > data_count is declared as u8 and will never be < 0.
> Data count can never be <0 as only first few bits of the 8 bits are used in the register. 
> 
	u8 data_count;
...
	data_count = i2c_smbus_read_byte_data(data->client,
                                              MAX30208_FIFO_DATA_CNTR);
        if (data_count < 0) {

Really ? Static analyzers will have a field day with this code.

Anyway, I don't really care much about this code, so I'll let
Jonathan take it from here. I just wanted to share my observations.

Thanks,
Guenter
  

Hi Guenter,
Thanks for the acknowledgement and your comments. 

>That is your use case. I don't know how IIO drivers are normally implemented, but I would expect a generic driver. In this case, I would expect >userspace to decide what it wants to with the data and not let the kernel driver discard most of it.
So, essentially, this driver is not discarding but there is no way to get to the recent most reading without popping all the values
before it. Call it a device limitation? Hence, if FIFO contains more than 1 reading, there is no other way than to pop out all
the values before the recent most to get there.

> What does that have to do with interrupts ? Anything connected to the gpio pin would trigger a reading.
Yes that’s correct. However, I intend to give out all readings in the FIFO through a defined user space attribute so that user can check all
the triggered conversions instead of popping out and only reading the most recent one. 
I am thinking of doing this via ACPI GPIO interrupt which stores values in a kernel data structure whenever triggered and a user space
attribute printing out all of them. 

> It seems to me that this would warrant an explanation in the driver.
>500ms seems hard to believe.
Yes, I proofread the spec many times to give a reasoning behind this. All I could find is this in the datasheet:
Response Time TA = +0°C to +50°C Unmounted, 63% (Note 2) 0.5s
I assumed this response time would be the one which gives the maximum amount of time to respond. Your comments are also welcome.

>If reading MAX30208_FIFO_OVF_CNTR returns a value > 0, it is used as data_count. That does not seem correct. The data sheet says if >MAX30208_FIFO_OVF_CNTR is != 0, data_count is 32. Maybe the datasheet is wrong all over the place, but at least in this case that seems very >unlikely.
I think you are confusing data_count with "data counter". So, overflow counter becomes active only when there are about 32 readings left in the FIFO to read. Now, in this situation, if I trigger temperature conversion 'x' (x<32) more times, I would want to pop out only (x-1) value to read the most recent one, right?, and not 32 readings. So the fact that data counter gets stuck at the value of '32' is correct but what we want is the number of readings to pop to get the most recent one and if overflow counter is >0, the number is indicated by the overflow counter itself. 

Inviting Jonathan and Guenter for further speculations and to comment on v6.

Thanks
Rajat

-----Original Message-----
From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
Sent: Tuesday, October 25, 2022 12:02 AM
To: Khandelwal, Rajat <rajat.khandelwal@intel.com>
Cc: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>; jic23@kernel.org; lars@metafoo.de; linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; jdelvare@suse.com; linux-hwmon@vger.kernel.org
Subject: Re: [PATCH v5] iio: temperature: Add driver support for Maxim MAX30208

On Mon, Oct 24, 2022 at 05:11:17PM +0000, Khandelwal, Rajat wrote:
> Hi Guenter,
> Thanks for the acknowledgement.
> 
> >Agreed; the sensor doesn't seem to be very useful for traditional 
> >hardware monitoring. The driver better resides in IIO.
> Cool! I didn't know the categorical reasoning behind this but since 
> this is accepted in IIO, I don't have to do anything more.

Huh. There is no "categorical" reasoning. Call it a gut feeling.
I can not imagine anyone using this chip for hardware monitoring, and presumably you have an IIO use case or you would not have implemented an IIO driver.

> 
> >I don't understand why readings are discarded. Why trigger multiple 
> >readings just to discard all but the last one ? I thought iio would 
> >be expected to return all values.
> Ok. The plan is to trigger temperature conversion on the GPIO input also.
> The user can trigger as many times the temperature conversion he wants 
> (I accept unnecessary), which will keep the FIFO increasing (without 
> reading converted values) but the driver should be resilient to all 
> the erroneous zones. Also, when the user does really make a syscall to read the temperature, it definitely should be the last converted reading.

That is your use case. I don't know how IIO drivers are normally implemented, but I would expect a generic driver. In this case, I would expect userspace to decide what it wants to with the data and not let the kernel driver discard most of it.

> 
> >This is really pointless. The register has only one bit to set.
> >Just write that bit; reading the register before that is pointless.
> I think the register also has some bits which are reserved. Hence, 
> rather than to make a number for specifically the value keeping those 
> bits the same, I read whatever is there and only store the required one.
> 
I personally would not accept that kind of code, but that is just me.

> >Also, the code assumes that one of the gpio input registers would be 
> >used to trigger temperature readings. Why trigger another one if this 
> >is indeed the case ? Triggering a temperature reading should only be 
> >necessary if there is no data in the fifo.
> GPIO input triggering is yet not implemented as I would have to work 
> on ACPI interrupts and I have written the driver for now to get it included in Linux.
> There are 2 ways - via GPIO and making a syscall. I agree that 
> temperature reading should be necessary only when there is no data in 
> FIFO but since we intend to keep GPIO as a trigger point, user can 
> keep triggering conversions and not reading them out. (As pointed above, driver should be resilient to all erroneous zones).

What does that have to do with interrupts ? Anything connected to the gpio pin would trigger a reading.

> 
> >The datasheet says that it can take up to 50 ms to report a result.
> >10 retries with 50ms wait each time seems overkill.
> That's correct. But, the response time can be up to 500 ms. Also, 
> while debugging I had put timestamps which when analyzed, indicated that time may go beyond 50 ms.
> 

It seems to me that this would warrant an explanation in the driver.
500ms seems hard to believe.

> >And why use usleep_range() here
> >but msleep() above ?
> I am sorry about that. I have converted usleep_range into msleep (2 places). 
> 
> >This is wrong. It uses the overflow counter as data counter if it is 
> >!= 0. The overflow counter counts the number of overflows, not the 
> >number of entries in the fifo.
> So there is no such thing as 'overflow counter'. The point is if the 
> overflow counter has

Interesting statement. MAX30208_FIFO_OVF_CNTR very much sounds like overflow counter to me, and the datasheet suggests the same.

> even one word, I use the data count equal to the overflow counter 
> value. However, if it has zero, then use the number of words in actual FIFO.
> This logic is just used to count how many values to pop to get the most recent reading.
> 

The code is

+       ret = i2c_smbus_read_byte_data(data->client, MAX30208_FIFO_OVF_CNTR);
+       if (ret < 0) {
+               dev_err(&data->client->dev, "Error reading reg FIFO overflow counter\n");
+               goto unlock;
+       } else if (!ret) {
+               ret = i2c_smbus_read_byte_data(data->client,
+                                              MAX30208_FIFO_DATA_CNTR);
+               if (ret < 0) {
+                       dev_err(&data->client->dev, "Error reading reg FIFO data counter\n");
+                       goto unlock;
+               }
+       }
+
+       data_count = ret;

If reading MAX30208_FIFO_OVF_CNTR returns a value > 0, it is used as data_count. That does not seem correct. The data sheet says if MAX30208_FIFO_OVF_CNTR is != 0, data_count is 32. Maybe the datasheet is wrong all over the place, but at least in this case that seems very unlikely.

> > data_count is declared as u8 and will never be < 0.
> Data count can never be <0 as only first few bits of the 8 bits are used in the register. 
> 
	u8 data_count;
...
	data_count = i2c_smbus_read_byte_data(data->client,
                                              MAX30208_FIFO_DATA_CNTR);
        if (data_count < 0) {

Really ? Static analyzers will have a field day with this code.

Anyway, I don't really care much about this code, so I'll let Jonathan take it from here. I just wanted to share my observations.

Thanks,
Guenter
  

On Mon, 24 Oct 2022 11:31:48 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On Mon, Oct 24, 2022 at 05:11:17PM +0000, Khandelwal, Rajat wrote:
> > Hi Guenter,
> > Thanks for the acknowledgement.
> >   
> > >Agreed; the sensor doesn't seem to be very useful for traditional hardware
> > >monitoring. The driver better resides in IIO.  
> > Cool! I didn't know the categorical reasoning behind this but since this is 
> > accepted in IIO, I don't have to do anything more.   
> 
> Huh. There is no "categorical" reasoning. Call it a gut feeling.
> I can not imagine anyone using this chip for hardware monitoring,
> and presumably you have an IIO use case or you would not have
> implemented an IIO driver.
> 
> >   
> > >I don't understand why readings are discarded. Why trigger multiple
> > >readings just to discard all but the last one ? I thought iio would
> > >be expected to return all values.  
> > Ok. The plan is to trigger temperature conversion on the GPIO input also.
> > The user can trigger as many times the temperature conversion he wants (I accept unnecessary),
> > which will keep the FIFO increasing (without reading converted values) but the driver should be
> > resilient to all the erroneous zones. Also, when the user does really make a syscall to read the
> > temperature, it definitely should be the last converted reading.   
> 
> That is your use case. I don't know how IIO drivers are normally
> implemented, but I would expect a generic driver. In this case,
> I would expect userspace to decide what it wants to with the data
> and not let the kernel driver discard most of it.

Two separate interfaces - the sysfs one this driver initially supports (in common
with many other first submissions) - that is normally for polling of the channel
value - we want the latest. Second interface is chardev version that uses
a kfifo to avoid dropping data and is commonly interrupt driven.  For that
interface, we indeed try to pass all the data to userspace.

> 
> >   
> > >This is really pointless. The register has only one bit to set.
> > >Just write that bit; reading the register before that is pointless.  
> > I think the register also has some bits which are reserved. Hence, rather than to make a number
> > for specifically the value keeping those bits the same, I read whatever is there and only store the
> > required one. 
> >   
> I personally would not accept that kind of code, but that is just
> me.

I'm completely lost on this.  Please don't move comments to the top, put
them inline alongside the code.  Do crop the code to only include relevant
parts though to save us all scrolling through uncomnented code!
If you agree with a reviewer comment it's also fine to crop that bit out -
if we don't see a reply we assume you accept the feedback.


> 
> > >Also, the code assumes that one of the gpio input registers would be used
> > >to trigger temperature readings. Why trigger another one if this is indeed
> > >the case ? Triggering a temperature reading should only be necessary if
> > >there is no data in the fifo.  
> > GPIO input triggering is yet not implemented as I would have to work on ACPI interrupts and I have
> > written the driver for now to get it included in Linux. 
> > There are 2 ways - via GPIO and making a syscall. I agree that temperature reading should be
> > necessary only when there is no data in FIFO but since we intend to keep GPIO as a trigger point,
> > user can keep triggering conversions and not reading them out. (As pointed above, driver should be
> > resilient to all erroneous zones).  
> 
> What does that have to do with interrupts ? Anything connected to the
> gpio pin would trigger a reading.
> 
> >   
> > >The datasheet says that it can take up to 50 ms to report a result.
> > >10 retries with 50ms wait each time seems overkill.  
> > That's correct. But, the response time can be up to 500 ms. Also, while debugging I had put timestamps
> > which when analyzed, indicated that time may go beyond 50 ms. 
> >   
> 
> It seems to me that this would warrant an explanation in the driver.
> 500ms seems hard to believe.
> 
> > >And why use usleep_range() here
> > >but msleep() above ?  
> > I am sorry about that. I have converted usleep_range into msleep (2 places). 
> >   
> > >This is wrong. It uses the overflow counter as data counter if it
> > >is != 0. The overflow counter counts the number of overflows, not
> > >the number of entries in the fifo.  
> > So there is no such thing as 'overflow counter'. The point is if the overflow counter has  
> 
> Interesting statement. MAX30208_FIFO_OVF_CNTR very much
> sounds like overflow counter to me, and the datasheet
> suggests the same.
> 
> > even one word, I use the data count equal to the overflow counter value. However, if it
> > has zero, then use the number of words in actual FIFO. 
> > This logic is just used to count how many values to pop to get the most recent reading.
> >   
> 
> The code is
> 
> +       ret = i2c_smbus_read_byte_data(data->client, MAX30208_FIFO_OVF_CNTR);
> +       if (ret < 0) {
> +               dev_err(&data->client->dev, "Error reading reg FIFO overflow counter\n");
> +               goto unlock;
> +       } else if (!ret) {
> +               ret = i2c_smbus_read_byte_data(data->client,
> +                                              MAX30208_FIFO_DATA_CNTR);
> +               if (ret < 0) {
> +                       dev_err(&data->client->dev, "Error reading reg FIFO data counter\n");
> +                       goto unlock;
> +               }
> +       }
> +
> +       data_count = ret;
> 
> If reading MAX30208_FIFO_OVF_CNTR returns a value > 0, it is used as
> data_count. That does not seem correct. The data sheet says if
> MAX30208_FIFO_OVF_CNTR is != 0, data_count is 32. Maybe the datasheet
> is wrong all over the place, but at least in this case that seems
> very unlikely.
> 
> > > data_count is declared as u8 and will never be < 0.  
> > Data count can never be <0 as only first few bits of the 8 bits are used in the register. 
> >   
> 	u8 data_count;
> ...
> 	data_count = i2c_smbus_read_byte_data(data->client,
>                                               MAX30208_FIFO_DATA_CNTR);
>         if (data_count < 0) {
> 
> Really ? Static analyzers will have a field day with this code.
> 
> Anyway, I don't really care much about this code, so I'll let
> Jonathan take it from here. I just wanted to share my observations.
> 
> Thanks,
> Guenter
  

On Tue, 25 Oct 2022 05:40:36 +0000
"Khandelwal, Rajat" <rajat.khandelwal@intel.com> wrote:

> Hi Guenter,
> Thanks for the acknowledgement and your comments. 
> 
> >That is your use case. I don't know how IIO drivers are normally implemented, but I would expect a generic driver. In this case, I would expect >userspace to decide what it wants to with the data and not let the kernel driver discard most of it.  
> So, essentially, this driver is not discarding but there is no way to get to the recent most reading without popping all the values
> before it. Call it a device limitation? Hence, if FIFO contains more than 1 reading, there is no other way than to pop out all
> the values before the recent most to get there.
> 
> > What does that have to do with interrupts ? Anything connected to the gpio pin would trigger a reading.  
> Yes that’s correct. However, I intend to give out all readings in the FIFO through a defined user space attribute so that user can check all
> the triggered conversions instead of popping out and only reading the most recent one. 

Don't invent new ABI - use the buffered interface in IIO as all the other devices with Fifos do.
Fine not doing that in first version of the driver however and just reporting most
recent value.

> I am thinking of doing this via ACPI GPIO interrupt which stores values in a kernel data structure whenever triggered and a user space
> attribute printing out all of them. 

This is all handled by standard IIO driver interfaces.  Just look for another
device with a fifo - there are lots of them.

> 
> > It seems to me that this would warrant an explanation in the driver.
> >500ms seems hard to believe.  
> Yes, I proofread the spec many times to give a reasoning behind this. All I could find is this in the datasheet:
> Response Time TA = +0°C to +50°C Unmounted, 63% (Note 2) 0.5s
> I assumed this response time would be the one which gives the maximum amount of time to respond. Your comments are also welcome.
> 
> >If reading MAX30208_FIFO_OVF_CNTR returns a value > 0, it is used as data_count. That does not seem correct. The data sheet says if >MAX30208_FIFO_OVF_CNTR is != 0, data_count is 32. Maybe the datasheet is wrong all over the place, but at least in this case that seems very >unlikely.  
> I think you are confusing data_count with "data counter". So, overflow counter becomes active only when there are about 32 readings left in the FIFO to read. Now, in this situation, if I trigger temperature conversion 'x' (x<32) more times, I would want to pop out only (x-1) value to read the most recent one, right?, and not 32 readings. So the fact that data counter gets stuck at the value of '32' is correct but what we want is the number of readings to pop to get the most recent one and if overflow counter is >0, the number is indicated by the overflow counter itself. 
> 
> Inviting Jonathan and Guenter for further speculations and to comment on v6.
> 
> Thanks
> Rajat
> 
> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Tuesday, October 25, 2022 12:02 AM
> To: Khandelwal, Rajat <rajat.khandelwal@intel.com>
> Cc: Rajat Khandelwal <rajat.khandelwal@linux.intel.com>; jic23@kernel.org; lars@metafoo.de; linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; jdelvare@suse.com; linux-hwmon@vger.kernel.org
> Subject: Re: [PATCH v5] iio: temperature: Add driver support for Maxim MAX30208
> 
> On Mon, Oct 24, 2022 at 05:11:17PM +0000, Khandelwal, Rajat wrote:
> > Hi Guenter,
> > Thanks for the acknowledgement.
> >   
> > >Agreed; the sensor doesn't seem to be very useful for traditional 
> > >hardware monitoring. The driver better resides in IIO.  
> > Cool! I didn't know the categorical reasoning behind this but since 
> > this is accepted in IIO, I don't have to do anything more.  
> 
> Huh. There is no "categorical" reasoning. Call it a gut feeling.
> I can not imagine anyone using this chip for hardware monitoring, and presumably you have an IIO use case or you would not have implemented an IIO driver.
> 
> >   
> > >I don't understand why readings are discarded. Why trigger multiple 
> > >readings just to discard all but the last one ? I thought iio would 
> > >be expected to return all values.  
> > Ok. The plan is to trigger temperature conversion on the GPIO input also.
> > The user can trigger as many times the temperature conversion he wants 
> > (I accept unnecessary), which will keep the FIFO increasing (without 
> > reading converted values) but the driver should be resilient to all 
> > the erroneous zones. Also, when the user does really make a syscall to read the temperature, it definitely should be the last converted reading.  
> 
> That is your use case. I don't know how IIO drivers are normally implemented, but I would expect a generic driver. In this case, I would expect userspace to decide what it wants to with the data and not let the kernel driver discard most of it.
> 
> >   
> > >This is really pointless. The register has only one bit to set.
> > >Just write that bit; reading the register before that is pointless.  
> > I think the register also has some bits which are reserved. Hence, 
> > rather than to make a number for specifically the value keeping those 
> > bits the same, I read whatever is there and only store the required one.
> >   
> I personally would not accept that kind of code, but that is just me.
> 
> > >Also, the code assumes that one of the gpio input registers would be 
> > >used to trigger temperature readings. Why trigger another one if this 
> > >is indeed the case ? Triggering a temperature reading should only be 
> > >necessary if there is no data in the fifo.  
> > GPIO input triggering is yet not implemented as I would have to work 
> > on ACPI interrupts and I have written the driver for now to get it included in Linux.
> > There are 2 ways - via GPIO and making a syscall. I agree that 
> > temperature reading should be necessary only when there is no data in 
> > FIFO but since we intend to keep GPIO as a trigger point, user can 
> > keep triggering conversions and not reading them out. (As pointed above, driver should be resilient to all erroneous zones).  
> 
> What does that have to do with interrupts ? Anything connected to the gpio pin would trigger a reading.
> 
> >   
> > >The datasheet says that it can take up to 50 ms to report a result.
> > >10 retries with 50ms wait each time seems overkill.  
> > That's correct. But, the response time can be up to 500 ms. Also, 
> > while debugging I had put timestamps which when analyzed, indicated that time may go beyond 50 ms.
> >   
> 
> It seems to me that this would warrant an explanation in the driver.
> 500ms seems hard to believe.
> 
> > >And why use usleep_range() here
> > >but msleep() above ?  
> > I am sorry about that. I have converted usleep_range into msleep (2 places). 
> >   
> > >This is wrong. It uses the overflow counter as data counter if it is 
> > >!= 0. The overflow counter counts the number of overflows, not the 
> > >number of entries in the fifo.  
> > So there is no such thing as 'overflow counter'. The point is if the 
> > overflow counter has  
> 
> Interesting statement. MAX30208_FIFO_OVF_CNTR very much sounds like overflow counter to me, and the datasheet suggests the same.
> 
> > even one word, I use the data count equal to the overflow counter 
> > value. However, if it has zero, then use the number of words in actual FIFO.
> > This logic is just used to count how many values to pop to get the most recent reading.
> >   
> 
> The code is
> 
> +       ret = i2c_smbus_read_byte_data(data->client, MAX30208_FIFO_OVF_CNTR);
> +       if (ret < 0) {
> +               dev_err(&data->client->dev, "Error reading reg FIFO overflow counter\n");
> +               goto unlock;
> +       } else if (!ret) {
> +               ret = i2c_smbus_read_byte_data(data->client,
> +                                              MAX30208_FIFO_DATA_CNTR);
> +               if (ret < 0) {
> +                       dev_err(&data->client->dev, "Error reading reg FIFO data counter\n");
> +                       goto unlock;
> +               }
> +       }
> +
> +       data_count = ret;
> 
> If reading MAX30208_FIFO_OVF_CNTR returns a value > 0, it is used as data_count. That does not seem correct. The data sheet says if MAX30208_FIFO_OVF_CNTR is != 0, data_count is 32. Maybe the datasheet is wrong all over the place, but at least in this case that seems very unlikely.
> 
> > > data_count is declared as u8 and will never be < 0.  
> > Data count can never be <0 as only first few bits of the 8 bits are used in the register. 
> >   
> 	u8 data_count;
> ...
> 	data_count = i2c_smbus_read_byte_data(data->client,
>                                               MAX30208_FIFO_DATA_CNTR);
>         if (data_count < 0) {
> 
> Really ? Static analyzers will have a field day with this code.
> 
> Anyway, I don't really care much about this code, so I'll let Jonathan take it from here. I just wanted to share my observations.
> 
> Thanks,
> Guenter