RE: [PATCH v6] iio: temperature: Add driver support for Maxim MAX30208

[Khandelwal, Rajat]

Hi Jonathan,
Upstreaming a driver surely requires a lot of inquisitive readings. :)
Thanks for acknowledging in detail which eventually helps me to write better code. 

On to your comments!
>It doesn't use them, so unless you have to be in that state to use the current
>method, drop the note and don't set them to do that.
>
>You can add that support if / when the driver supports it.
I have dropped the entire GPIO settings for now. I will incorporate it in subsequent
patches for your perusal and verification. 

> Mentioned below, but I'd prefer to see the string directly inline.
Done. 

> Be more explicit - reads of what?
Done.

>I agree with Guenter's comment that this is a bit overly noisy. We don't expect
>random register reads to fail + IIRC there is tracing in the i2c subsystem if we
>are getting such errors.   Hence probably reduce the error to cover only larger
>blocks of code.  Reasonable to report that the temperature request failed perhaps.
So, I kind of just started upstreaming matches in the IIO community also for change of
domains and on seeing other drivers which were returning on each error, I thought
maybe its required in IIO? Thanks for this comment. I have removed them. 

>dev_err()  Failing isn't expected so it's a device error not something
>we should merely warn about.
Ok, so here I have incorporated dev_err but what I wanted was if MAX30208_STATUS_TEMP_RDY
is written successfully and any error happens after that, the user still gets returned the most recent
reading when this operation fails. So, I have changed it into dev_err with printing
"Temperature conversion failed, reporting the last known reading...") Will that be ok?

> Error return to indicate what happened - there is one for timeouts.
As indicated above, if you are in for that, then here we should ideally return 0 since even after
the error, we want update_temp to do its job and return the last reading. 

>Whilst you debated this logic with Guenter in v5, I don't follow the conclusion.
>If FIFO_OVF_CNTR > 0 then FIFO_DATA_CNTR == maximum value.  Logic of this is
>given in FIFO_DATA Read Example (Page 16 of the spec)  It doesn't matter as such
>because you read FIFO_DATA_CNTR again anyway, but it would be more obvious what
>is going on if you just set data_count = 32 if overflow has occured.
>
>The only thing I would argue you 'might' want to do with the overflow counter
>is to use it to indicate we need to read at least the number of elements in the fifo.
>If there are no additional elements at the end, wait until there is one. Otherwise
>you potentially get very stale data - it might have been overflowing for a long time)
>This decrease by more than 1 is worrying.  I can understand it not decreasing, or even
>increasing (new data came in), but this condition sounds either like we are doing something
>wrong or there is a hardware bug.
Ok, even I started doubting the nature of the device I encountered when I tested it.
The thing is, this driver also comes with an EV kit wherein you can plug into
PC, download the software and interface it via USB. Whenever I emulated reads from the
OS, I encountered erroneous counter decrements. 
However, I have tested it using native I2C on my microcontroller and I don't ever encounter
such readings, thus concluding that the spurious erroneous zones only exist when the EV kit
is interfaced via USB. 
Thanks for creating this doubt. I have solved it by only decrementing data_count unless it
becomes 0. 
Regarding data_count variable, so when overflow counter is >0, I want to decrease 'overflow counter'
number of times to get the required reading and when overflow counter is =0, I want to decrease
'data counter' number of times to get the reading. Hope it makes sense now in the new version.

>Excessive line breaks.  This is under 80 chars.  In cases where
>readability is helped by going above that (though under 100 chars) that
>is fine too.  Make sure you tidy up all similar cases.
Done.

>If the driver 'works' in current form without setting this stuff up I would
>prefer that you leave this until you have a patch actually using the GPIO signals.
>That way we can review all the GPIO related code together.
As stated above, I have dropped the GPIOs altogether for now and will make a patch in future
for you to review it. 

>The expressive nature of the field define makes it obvious this is a reset.
>so I would drop the comment.
Done.

>module_i2c_driver() to get rid of this boilerplate.
Done.

Sending v7 incorporating all comments. Please review.

Thanks
Rajat

-----Original Message-----
From: Jonathan Cameron <jic23@xxxxxxxxxx> 
Sent: Saturday, October 29, 2022 8:34 PM
To: Rajat Khandelwal <rajat.khandelwal@xxxxxxxxxxxxxxx>
Cc: lars@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx; jdelvare@xxxxxxxx; linux@xxxxxxxxxxxx; linux-hwmon@xxxxxxxxxxxxxxx; Khandelwal, Rajat <rajat.khandelwal@xxxxxxxxx>
Subject: Re: [PATCH v6] iio: temperature: Add driver support for Maxim MAX30208

On Tue, 25 Oct 2022 22:48:58 +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.
> Datasheet: https://datasheets.maximintegrated.com/en/ds/MAX30208.pdf
> 
> Signed-off-by: Rajat Khandelwal <rajat.khandelwal@xxxxxxxxxxxxxxx>

Hi Rajat,

Consider using regmap for this. It will provide you with some helpful utility functions.  I don't mind you sticking to direct i2c calls though if you prefer.

Quite a few things inline that have been commented on in previous reviews.
Make sure you incorporate all feedback or you'll end up going through more versions than would otherwise be necessary.

Jonathan

> 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..41b26755ce27
> --- /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@xxxxxxxxxxxxxxx>
> + *
> + * 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.

It doesn't use them, so unless you have to be in that state to use the current method, drop the note and don't set them to do that.

You can add that support if / when the driver supports it.

> + */
> +
> +#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"

Mentioned below, but I'd prefer to see the string directly inline.

> +
> +#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 */

Be more explicit - reads of what?

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

I agree with Guenter's comment that this is a bit overly noisy. We don't expect random register reads to fail + IIRC there is tracing in the i2c subsystem if we
are getting such errors.   Hence probably reduce the error to cover only larger
blocks of code.  Reasonable to report that the temperature request failed perhaps.


> +		return ret;
> +	}
> +
> +	regval = ret | MAX30208_TEMP_SENSOR_SETUP_CONV;
> +
> +	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");
> +		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;

Can this happen for a documented reason?  If not treat it as a comms error and return it.

> +		}
> +
> +		if (ret & MAX30208_STATUS_TEMP_RDY)
> +			return 0;
> +
> +		msleep(50);
> +	}
> +	dev_warn(&data->client->dev, "Temperature conversion failed\n");

dev_err()  Failing isn't expected so it's a device error not something we should merely warn about.

> +
> +	return 0;
Error return to indicate what happened - there is one for timeouts.

> +
> +sleep:

Why sleep in an error path?  It's failed - why do we think sleeping is a good idea?

> +	msleep(50);
> +	return 0;
> +}
> +
> +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);
Whilst you debated this logic with Guenter in v5, I don't follow the conclusion.
If FIFO_OVF_CNTR > 0 then FIFO_DATA_CNTR == maximum value.  Logic of this is given in FIFO_DATA Read Example (Page 16 of the spec)  It doesn't matter as such because you read FIFO_DATA_CNTR again anyway, but it would be more obvious what is going on if you just set data_count = 32 if overflow has occured.

The only thing I would argue you 'might' want to do with the overflow counter is to use it to indicate we need to read at least the number of elements in the fifo.
If there are no additional elements at the end, wait until there is one. Otherwise you potentially get very stale data - it might have been overflowing for a long time)

> +	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;
> +
> +	/*
> +	 * 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.

This decrease by more than 1 is worrying.  I can understand it not decreasing, or even increasing (new data came in), but this condition sounds either like we are doing something wrong or there is a hardware bug.

> +	 * Return the last reading from FIFO as the most recently triggered 
> +one

Not necessarily recent (even if most recent available). Imagine this runs after overflow and runs really quickly.
You may not get a new reading.

> +	 */
> +	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;

Flip this around so you consistently use ret for return values.  We still have problem that data_count is a u8 so the above test is invalid.

		ret = i2c_smbus_read_byte_data(data->client, ...)
		if (ret < 0) {
			dev_err(..);
			goto unlock
		}
		data_count = ret;

Guenter pointed this out in v5.

> +			goto unlock;
> +		}
> +	}
> +
> +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);

Excessive line breaks.  This is under 80 chars.  In cases where readability is helped by going above that (though under 100 chars) that is fine too.  Make sure you tidy up all similar cases.

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

If the driver 'works' in current form without setting this stuff up I would prefer that you leave this until you have a patch actually using the GPIO signals.
That way we can review all the GPIO related code together.

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

This belongs in a patch adding interrupt support. It should not be here until then.

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

I'm not a huge fan of defines either here or in the driver structure initializer.
The reason being that we need clear visibility of these strings and there is no particular reason why they are the same.
So I'd prefer getting rid of that define and putting the strings directly in both locations.

> +	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 */

The expressive nature of the field define makes it obvious this is a reset.
so I would drop the comment.  There is a price in maintainability to comments (they often become wrong over time as a driver evolves).  As such keep them for places where the comment tells us something not easily seen from the code.

> +	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;
> +	}
> +
> +	msleep(50);
> +
> +	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),

Try building without ACPI support and you should see a warning from the compiler.  Sadly ACPI_PTR() is not as smart as it should be.
If interested, take a look at how pm_ptr() deals with the same issue.

Anyhow, best option is just don't bother with ACPI_PTR().
The saving in module size is trivial and not worth the ifdef magic needed to make it work warning free.


> +	},
> +	.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_i2c_driver() to get rid of this boilerplate.

Note this was a comment I made on v1... I wondered if I'd been half asleep so went looking :)

Jonathan