Message-ID: <CO1PR11MB4835A5C17C3D72395C45850B96279@CO1PR11MB4835.namprd11.prod.outlook.com>
Date:   Sat, 15 Oct 2022 17:57:19 +0000
From:   "Khandelwal, Rajat" <rajat.khandelwal@...el.com>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Rajat Khandelwal <rajat.khandelwal@...ux.intel.com>
CC:     "jic23@...nel.org" <jic23@...nel.org>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>
Subject: RE: [PATCH] iio: temperature: Add driver support for Maxim MAX30208

Hi Jonathan,
Thanks for the suggestions and remarks. 
Couple of things before I send out v2. 

Yes, I agree that hwmon could also be a receptacle for this. AFAIK,
IIO is directed towards devices which are ADC/DAC converters in some sense, which
makes the driver I wrote also fall in the category. 
I am not a better judge but personally, I would choose iio over hwmon anytime since
the interface is something which has fancied me and enamored me to it. :)

FYI, I am also writing another driver in parallel (a temperature sensor again) in IIO
subsystem, so will ask you in this thread itself if it is considerable?

Nevertheless, I genuinely would be grateful if you consider this driver to be a part of IIO
subsystem. As a maintainer for this, I would be happy to shift this to a better subsystem
if something comes in the future or even hwmon. 

Switching to another topic, I have answered your comments and have some doubts which
I would like you to answer before I send out v2. 

>> That's a large todo.  Why set it up if the support to actually use it
isn't there?  Is there no mode where these GPIOs are disabled that would
be more suitable until you add support (optionally as gpios may well
not be wired up).
	There is no specific mode to "disable" the GPIOs. I wanted to trigger
temperature conversion having GPIO1 as a trigger interrupt so I put this as
TODO. Yes, its possible that GPIOs are not wired up but this would provide a
feature for this sensor for the GPIO to use. 
Also, I don't intend to keep the TODO for long. The device tables and kernel code
are already in process to remove the TODO. 

>>> +static void max30208_gpio_setup(struct max30208_data *data)
int return so you can indicate an error to the caller.
	Here, I am configuring GPIOs to act as interrupt and temperature
conversion events. But if configuration fails, I don't want to fail the driver probe
as this is secondary and not absolutely necessary. Thus, there is no point to even
check the return value. 
Can we proceed with the function as 'void'? I will give a warning if any of the GPIO fails
to be configured as interrupt/conversion event. That would suffice.  Will that be ok?

>>That leaves is in nasy unknown state. Error out.  If the device fails
the i2c call because it resets quicker than sending the Ack (quite
a few drivers do this), just drop the error check.
	The failure to perform reset will not considerable affect the device. Some things
like FIFO buffer could be not empty initially but since temperature readings are taken
at the end of the buffer, those shouldn't affect. Hence, I have implemented dev_warn. 
The warning indicates that if the user thinks that on triggering temperature conversion 'n'
times, there would be 'n' FIFO readings, then he is wrong as device failed to reset.
This is the only motive of giving warning on reset. 

>>> +	max30208_gpio_setup(data);

As mentioned above, this should return an error if it hits one and
probe should fail if so.  It is very unwise to carry on once we have
a device in known state because an error occurred during setup.
	The GPIO configuration error won't affect the device's usability. 
Hence, I haven't returned an error on failure. The main objective is to
provide readings. GPIO functionality is secondary. Nevertheless, I will issue
a warning if GPIO configuration fails to let user know GPIO's won't work. 
Would that be ok?

>>I'd prefer seeing the of_match_table() as well.
Otherwise we rely on the fallback matching and lose the advantage
of having the vendor name in there.
	Sure Jonathan. The device table is in the process of testings. However,
I am implementing acpi_match_table in the driver. Will that work? Is the
of_match_table necessary?

Thanks
Rajat

-----Original Message-----
From: Jonathan Cameron <Jonathan.Cameron@...wei.com> 
Sent: Friday, October 14, 2022 6:40 PM
To: Rajat Khandelwal <rajat.khandelwal@...ux.intel.com>
Cc: jic23@...nel.org; lars@...afoo.de; linux-kernel@...r.kernel.org; linux-iio@...r.kernel.org; Khandelwal, Rajat <rajat.khandelwal@...el.com>
Subject: Re: [PATCH] iio: temperature: Add driver support for Maxim MAX30208

On Thu, 13 Oct 2022 20:42:03 +0530
Rajat Khandelwal <rajat.khandelwal@...ux.intel.com> wrote:

> Maxim MAX30208 is a digital temperature sensor with 0.1°C accuracy.
> Add support for max30208 driver in iio subsystem.

Hi Rajat,

Opening question for all temperature drivers is why IIO rather than hwmon?

There are a couple of standard reasons why the existing temp sensors are there.

1) They are weird.  E.g. the infrared sensors.
2) They are very industrially oriented (read expensive).  Usually things like
   high precision thermocouple front ends.
3) (this one is a bit nasty) they share silicon with more complex sensors.
   This sometimes happens with things like pressure sensors. Same interface
   covers devices with an without the pressure part.

Definitely hwmon if typically used for monitoring temp in a PC etc.
Other cases are somewhat trickier to answer.  We have iio-hwmon bridge for the really unclear cases (so it can appear in both ;)

Either way I did a quick review. Comments inline.

Thanks,

Jonathan

> 
> Datasheet: https://datasheets.maximintegrated.com/en/ds/MAX30208.pdf
No blank line here.  Datasheet is part of the tags block that can be scraped by automated tools so it needs to be in that block, not on it's own.

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


> ---
>  MAINTAINERS                        |   6 +
>  drivers/iio/temperature/Kconfig    |  10 ++
>  drivers/iio/temperature/Makefile   |   1 +
>  drivers/iio/temperature/max30208.c | 273 
> +++++++++++++++++++++++++++++
>  4 files changed, 290 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@...ux.intel.com>
> +L:	linux-iio@...r.kernel.org
> +S:	Maintained
> +F:	drivers/iio/temperature/max30208.c
> +
>  MAXIM MAX77650 PMIC MFD DRIVER
>  M:	Bartosz Golaszewski <brgl@...ev.pl>
>  L:	linux-kernel@...r.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..e16c31621065
> --- /dev/null
> +++ b/drivers/iio/temperature/max30208.c
> @@ -0,0 +1,273 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Copyright (c) Rajat Khandelwal <rajat.khandelwal@...ux.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.
> + *
> + * TODO: trigger temperature conversion via GPIO1

That's a large todo.  Why set it up if the support to actually use it isn't there?  Is there no mode where these GPIOs are disabled that would be more suitable until you add support (optionally as gpios may well not be wired up).

> + */
> +
> +#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)
> +
> +#define MAX30208_RESOLUTION_MC		5

I would spell out as MILLICELCIUS

MC is not a standard abbreviation.


> +
> +struct max30208_data {
> +	struct i2c_client *client;
> +	struct iio_dev *indio_dev;
> +	struct mutex lock;

All locks need a comment to say what data / device state they are protecting.

> +};
> +
> +static const struct iio_chan_spec max30208_channels[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) |
> +				      BIT(IIO_CHAN_INFO_PROCESSED),
> +	},
> +};
> +
> +static int max30208_request(struct max30208_data *data)

I'd like a comment on what exactly this causes to happen.  Single reading or lots of readings?  If lots, how do we turn it off again?

> +{
> +	int retries = 10;

Any retry counter needs a comment explaining why it is the particular value chosen..
I'd guess here because sensor can take up to half a second to read?

> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MAX30208_TEMP_SENSOR_SETUP);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret |= MAX30208_TEMP_SENSOR_SETUP_CONV;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, MAX30208_TEMP_SENSOR_SETUP, ret);
> +	if (ret < 0)
> +		return ret;
> +
> +	while (retries--) {
> +		ret = i2c_smbus_read_byte_data(data->client, MAX30208_STATUS);
> +		if (ret < 0)
> +			goto sleep;

The device fails to respond to i2c reads?  That's nasty.  Add a comment on what is going on here.

> +
> +		if (ret & MAX30208_STATUS_TEMP_RDY)
> +			return 0;
> +
> +		msleep(50);
> +	}
> +	dev_warn(&data->client->dev, "Temperature conversion failed\n");
> +
> +	return 0;
> +
> +sleep:
> +	usleep_range(50000, 60000);
> +	return 0;
> +}
> +
> +static int max30208_update_temp(struct max30208_data *data) {
> +	u16 temp_raw = 0;

Value isn't used, so don't assign it.

> +	s8 data_count;
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +
> +	ret = max30208_request(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MAX30208_FIFO_OVF_CNTR);
> +	if (ret < 0)
> +		return ret;
> +	else if (!ret)
> +		data_count = i2c_smbus_read_byte_data(data->client,
> +						      MAX30208_FIFO_DATA_CNTR);

See below, the error check should be here.

> +	else
> +		data_count = ret;
I'd like a comment here to explain what this flow is.  
> +
> +	if (data_count < 0)
Can only happen after the read, so move the error check up there...^^^ Also, use ret = i2c_smbus ... there and then unconditionally set data_count = ret after the error check.

> +		return data_count;
> +
> +	while (data_count) {
> +		ret = i2c_smbus_read_word_swapped(data->client,
> +						  MAX30208_FIFO_DATA);
> +		if (ret < 0)
> +			return ret;
> +
> +		data_count = i2c_smbus_read_byte_data(data->client,
> +						      MAX30208_FIFO_DATA_CNTR);

I'd guess that the counter never goes down other than due to reads?  If so perhaps save on i2c_reads of the fifo count, but first clearing the ones we
know about, then checking the count again.   Arguably, if they are new they
are after we asked for a reading anyway, so perhaps we shouldn't be looking at them - but instead using the most recent one before we starting reading?


> +		if (data_count < 0)
> +			return data_count;
> +	}
> +	temp_raw = ret;
> +
> +	mutex_unlock(&data->lock);
> +
> +	return temp_raw;
> +}
> +
> +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 = MAX30208_RESOLUTION_MC;
> +		*val2 = 1;
> +
> +		return IIO_VAL_FRACTIONAL;

return IIO_VAL_INT; 

> +
> +	case IIO_CHAN_INFO_PROCESSED:
> +		ret = max30208_update_temp(data);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = sign_extend32(ret, 15) * MAX30208_RESOLUTION_MC;

Don't have PROCESSED.  Drivers should never provide both raw and processed.
There are a few obscure reasons why they sometimes do, but non are applicable here that I can see.

> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static void max30208_gpio_setup(struct max30208_data *data)
int return so you can indicate an error to the caller.

> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client,
> +				       MAX30208_GPIO_SETUP);
> +	if (ret >= 0) {

	if (ret < 0)
		return ret;  //An error is an error, if you hit one fail the probe.

> +		/*
> +		 * Setting GPIO1 to trigger temperature conversion
> +		 * when driven low.
> +		 * Setting GPIO0 to trigger interrupt when temperature
> +		 * conversion gets completed.
> +		 */
> +		ret |= MAX30208_GPIO1_SETUP | MAX30208_GPIO0_SETUP;
> +		i2c_smbus_write_byte_data(data->client,
> +					  MAX30208_GPIO_SETUP, ret);
check error on that.
> +	}
> +
> +	ret = i2c_smbus_read_byte_data(data->client,
> +				       MAX30208_INT_ENABLE);
> +	if (ret >= 0) {
if (ret < 0)
	reutrn ret;

> +		/* Enabling GPIO0 interrupt */
> +		ret |= MAX30208_INT_ENABLE_TEMP_RDY;
> +		i2c_smbus_write_byte_data(data->client,
> +					  MAX30208_INT_ENABLE, ret);
check for error on that.
> +	}


> +}
> +
> +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;
> +
> +	ret = devm_iio_device_register(dev, indio_dev);

This exposes user interfaces of the driver so to avoid races it must be last thing done in probe.

> +	if (ret) {
> +		dev_err(dev, "Failed to register IIO device\n");
> +		return ret;
> +	}
> +
> +	/* Reset the device after registering */
> +	ret = i2c_smbus_write_byte_data(data->client, MAX30208_SYSTEM_CTRL,
> +					MAX30208_SYSTEM_CTRL_RESET);
> +	if (ret)
> +		dev_warn(dev, "Proceeding without successful reset\n");

That leaves is in nasy unknown state. Error out.  If the device fails the i2c call because it resets quicker than sending the Ack (quite a few drivers do this), just drop the error check.

> +
> +	usleep_range(50000, 60000);
> +
> +	max30208_gpio_setup(data);

As mentioned above, this should return an error if it hits one and probe should fail if so.  It is very unwise to carry on once we have a device in known state because an error occurred during setup.

> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id max30208_id_table[] = {
> +	{ "max30208", 0 },

If data is always 0, don't bother setting it.
	{ "max30208" },
is fine.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max30208_id_table);
> +
> +static struct i2c_driver max30208_driver = {
> +	.driver = {
> +		.name = MAX30208_DRV_NAME,
> +	},
> +	.probe_new = max30208_probe,
> +	.id_table = max30208_id_table,

I'd prefer seeing the of_match_table() as well.
Otherwise we rely on the fallback matching and lose the advantage of having the vendor name in there.


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

> +
> +MODULE_AUTHOR("Rajat Khandelwal <rajat.khandelwal@...ux.intel.com>");
> +MODULE_DESCRIPTION("Maxim MAX30208 digital temperature sensor"); 
> +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists