Message-ID: <20221029160353.3999849b@jic23-huawei>
Date:   Sat, 29 Oct 2022 16:03:53 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Rajat Khandelwal <rajat.khandelwal@...ux.intel.com>
Cc:     lars@...afoo.de, linux-kernel@...r.kernel.org,
        linux-iio@...r.kernel.org, jdelvare@...e.com, linux@...ck-us.net,
        linux-hwmon@...r.kernel.org, rajat.khandelwal@...el.com
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@...ux.intel.com> 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@...ux.intel.com>

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@...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.

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

Powered by blists - more mailing lists