Re: [PATCH 1/2] iio: temperature: Add MAX31865 RTD Support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat,  3 Jul 2021 15:28:06 +0530
Navin Sankar Velliangiri <navin@xxxxxxxxxxx> wrote:

> This patch adds support for Maxim MAX31865 RTD temperature
> sensor support.
> 
> More information can be found in:
> https://datasheets.maximintegrated.com/en/ds/MAX31865.pdf
> 
> Signed-off-by: Navin Sankar Velliangiri <navin@xxxxxxxxxxx>

Hi Navin,

Biggest comment here is that userspace ABI needs documentation.
You define a few new things in here, and it is tricky to review that
without the docs.  As such, we may well have more comments on those once
those Docs are available.

Otherwise, various minor comments inline.

> ---
>  drivers/iio/temperature/Kconfig    |  10 +
>  drivers/iio/temperature/Makefile   |   1 +
>  drivers/iio/temperature/max31865.c | 325 +++++++++++++++++++++++++++++
>  3 files changed, 336 insertions(+)
>  create mode 100644 drivers/iio/temperature/max31865.c
> 
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index 4df60082c1fa..c9412abca069 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -128,4 +128,14 @@ config MAX31856
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called max31856.
>  
> +config MAX31865
> +	tristate "MAX31865 RTD to Digital converter"
> +	depends on SPI
> +	help
> +	  If you say yes here you get support for MAX31865
> +	  thermocouple sensor chip connected via SPI.
> +
> +	  This driver can also be build as a module. If so, the module
> +	  will be called max31865.
> +
>  endmenu
> diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
> index 90c113115422..a8f3187258dc 100644
> --- a/drivers/iio/temperature/Makefile
> +++ b/drivers/iio/temperature/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_LTC2983) += ltc2983.o
>  obj-$(CONFIG_HID_SENSOR_TEMP) += hid-sensor-temperature.o
>  obj-$(CONFIG_MAXIM_THERMOCOUPLE) += maxim_thermocouple.o
>  obj-$(CONFIG_MAX31856) += max31856.o
> +obj-$(CONFIG_MAX31865) += max31865.o
>  obj-$(CONFIG_MLX90614) += mlx90614.o
>  obj-$(CONFIG_MLX90632) += mlx90632.o
>  obj-$(CONFIG_TMP006) += tmp006.o
> diff --git a/drivers/iio/temperature/max31865.c b/drivers/iio/temperature/max31865.c
> new file mode 100644
> index 000000000000..581c46b12eee
> --- /dev/null
> +++ b/drivers/iio/temperature/max31865.c
> @@ -0,0 +1,325 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Copyright (c) Linumiz 2021
> + *
> + * max31865.c - Maxim MAX31865 RTD-to-Digital Converter sensor driver
> + *
> + * Author: Navin Sankar Velliangiri <navin@xxxxxxxxxxx>
> + */
> +
> +#include <linux/ctype.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/spi/spi.h>
> +#include <linux/util_macros.h>

why?  Check the others are used.

> +#include <asm/unaligned.h>
> +
> +/*
> + * The MSB of the register value determines whether the following byte will
> + * be written or read. If it is 0, read will follow and if it is 1, write
> + * will follow.
> + */
> +#define MAX31865_RD_WR_BIT			BIT(7)
> +
> +#define MAX31865_CFG_VBIAS			BIT(7)
> +#define MAX31865_CFG_1SHOT			BIT(5)
> +#define MAX31865_3WIRE_RTD			BIT(4)
> +#define MAX31865_FAULT_STATUS_CLEAR		BIT(1)
> +#define MAX31865_FILTER_50HZ			BIT(0)
> +
> +/* The MAX31865 registers */
> +#define MAX31865_CFG_REG			0x00
> +#define MAX31865_RTD_MSB			0x01
> +#define MAX31865_FAULT_STATUS			0x07
> +
> +#define MAX31865_FAULT_OVUV			BIT(2)
> +
> +static const struct iio_chan_spec max31865_channels[] = {
> +	{	/* RTD Temperature */
> +		.type = IIO_TEMP,
> +		.info_mask_separate =
> +			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)
> +	},
> +};
> +
> +struct max31865_data {
> +	struct spi_device *spi;
> +	bool filter_50hz;
> +	bool no_of_wires;
> +};
> +
> +static int max31865_read(struct max31865_data *data, u8 reg,
> +			 u8 val[], unsigned int read_size)
> +{
> +	return spi_write_then_read(data->spi, &reg, 1, val, read_size);
> +}
> +
> +static int max31865_write(struct max31865_data *data, u8 reg,
> +			  unsigned int val)
> +{
> +	u8 buf[2];
> +
> +	buf[0] = reg | (MAX31865_RD_WR_BIT);

Brackets not needed.

Also can just have it as...

	u8 buf[2] = {
		reg | MAX...,
		val
};
> +	buf[1] = val;
> +
> +	return spi_write(data->spi, buf, 2);

spi_write() directly calls spi_sync_transfer() which in turn calls spi_sync() and
that requires dma safe buffers.  For a buffer to dma safe it need to be in a
cacheline not shared with other data.  That basically means you can use a buffer
on the stack.

Take a look at the use of __cacheline_aligned in IIO drivers to allow
us to put this in the iio_priv() structure
(we go through a dance in the core to ensure that space is itself always
aligned to a cacheline).

Another fairly clean way around this is to call spi_write_then_read with 0 length
read.  That function uses bounce buffers to avoid any DMA issues.

Wolfram gave a good talk on this a few years back:
https://www.youtube.com/watch?v=JDwaMClvV-s

Having once run into this problem it is extremely hard to debug as we all love
extremely rare data corruption!

> +}
> +
> +static int enable_bias(struct max31865_data *data, bool enable)
prefix all functions with max31865 as it makes it obvious what is driver
local an what is a generic call.

Also, a function that has an enable parameter should have a name that
reflects that.

	max31865_set_bias_enable or something like that.

> +{
> +	u8 cfg;
> +	int ret;
> +
> +	ret = max31865_read(data, MAX31865_CFG_REG, &cfg, sizeof(cfg));

As below, feels like a cache definitely makes sense for this!

> +	if (ret)
> +		return ret;
> +
> +	if (enable)
> +		cfg |= MAX31865_CFG_VBIAS;
> +	else
> +		cfg &= ~MAX31865_CFG_VBIAS;
> +
> +	ret = max31865_write(data, MAX31865_CFG_REG, cfg);

return max... 

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int max31865_rtd_read(struct max31865_data *data, int *val)
> +{
> +	int ret;
> +	u8 cfg, rtd_val[2];
> +
> +	/* Enable BIAS to start the conversion */
> +	ret = enable_bias(data, true);
> +	if (ret)
> +		return ret;
> +
> +	/* wait 10.5ms before initiating the conversion */
> +	msleep(11);

No need for first version of this driver, but perhaps consider doing runtime
pm for this as a future patch.  That way we can avoid the sleep if
we are doing repeated reads (perhaps to do oversampling) but still get the
power benefit if we are reading at lower frequency.
If it doesn't matter for your application, then we can wait until someone
else cares (if ever!)

> +
> +	ret = max31865_read(data, MAX31865_CFG_REG, &cfg, sizeof(cfg));

Seems like something we should probably be caching? Nice to cut down on
transfers in the one remotely hot path in the driver.
One option would be to use regmap, but if this is the only register where
that makes sense, then perhaps just implement a cache for it in this driver.
Superficially it feels like you can build the content of cfg from things
you are already caching anyway.

> +	if (ret)
> +		return ret;
> +
> +	cfg |= MAX31865_CFG_1SHOT | MAX31865_FAULT_STATUS_CLEAR;
> +
> +	ret = max31865_write(data, MAX31865_CFG_REG, cfg);
> +	if (ret)
> +		return ret;
> +
> +	if (data->filter_50hz) {
> +		/* 50Hz filter mode requires 62.5ms to complete */
> +		msleep(63);
> +	} else {
> +		/* 60Hz filter mode requires 52ms to complete */
> +		msleep(52);
> +	}
> +
> +	ret = max31865_read(data, MAX31865_RTD_MSB, rtd_val, sizeof(rtd_val));
> +	if (ret)
> +		return ret;
> +
> +	*val = (rtd_val[0] << 8 | rtd_val[1]) >> 1;

*val = be16_to_cpup(rtd_val) >> 1;

> +
> +	/* disable bias */

Name function such that this is obvious and drop the comment.
Comment bit rot in a way that well named functions don't :)

> +	ret = enable_bias(data, false);

return enable_bias(data, false)

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int max31865_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct max31865_data *data = iio_priv(indio_dev);
> +	int ret = 0;

Always set so no need to initialize.

> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = max31865_rtd_read(data, val);

Nothing stops multiple simultaneous reads of the sysfs attribute.
As such, I think you need a mutex to protect the device state
when doing reads.

> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		/* Temp. Data resolution is 0.03125 */

give a unit "degrees centigrade"

> +		*val = 31;
> +		*val2 = 250000; /* 1000 * 0.03125 */
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		ret = -EINVAL;

		return -EINVAL; and drop the return below as
we won't be able to get to it.

> +	}
> +
> +	return ret;
> +}
> +
> +static int max31865_init(struct max31865_data *data)
> +{
> +	int ret;
> +	u8 cfg = 0;
> +
> +	if (data->no_of_wires)
> +		/* 3-wire RTD connection */
> +		cfg |= MAX31865_3WIRE_RTD;
> +
> +	if (data->filter_50hz)
> +		/* 50Hz noise rejection filter */
> +		cfg |= MAX31865_FILTER_50HZ;

filter_50hz is controlled from userspace, yet this init is only
called on probe.  Hence I can't see how changing this to 60Hz via
sysfs has any impact.

> +
> +	ret = max31865_write(data, MAX31865_CFG_REG, cfg);

return max31865_....

> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static ssize_t show_fault(struct device *dev, u8 faultbit, char *buf)
> +{
> +	int ret;
> +	bool fault;
> +	u8 reg_val;
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct max31865_data *data = iio_priv(indio_dev);
> +
> +	ret = max31865_read(data, MAX31865_FAULT_STATUS, &reg_val, 1);
> +	if (ret)
> +		return ret;
> +
> +	fault = reg_val & faultbit;
> +
> +	return sprintf(buf, "%d\n", fault);
> +}
> +
> +static ssize_t show_fault_ovuv(struct device *dev,
> +			      struct device_attribute *attr,
> +			      char *buf)
> +{
> +	return show_fault(dev, MAX31865_FAULT_OVUV, buf);
> +}
> +
> +static ssize_t show_filter(struct device *dev,
> +			   struct device_attribute *attr,
> +			   char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct max31865_data *data = iio_priv(indio_dev);
> +
> +	return sprintf(buf, "%d\n", data->filter_50hz ? 50 : 60);
> +}
> +
> +static ssize_t set_filter(struct device *dev,
> +			  struct device_attribute *attr,
> +			  const char *buf,
> +			  size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct max31865_data *data = iio_priv(indio_dev);
> +	unsigned int freq;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &freq);
> +	if (ret)
> +		return ret;
> +
> +	switch (freq) {
> +	case 50:
> +		data->filter_50hz = true;
> +		break;
> +	case 60:
> +		data->filter_50hz = false;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(fault_ovuv, 0444, show_fault_ovuv, NULL, 0);
> +static IIO_DEVICE_ATTR(in_temp_filter_notch_frequency, 0644,
> +		    show_filter, set_filter, 0);
> +
> +static struct attribute *max31865_attributes[] = {
> +	&iio_dev_attr_fault_ovuv.dev_attr.attr,
> +	&iio_dev_attr_in_temp_filter_notch_frequency.dev_attr.attr,

This defines new ABI.  All ABI must be documented in
Documentation/ABI/testing/sysfs-bus-iio-*

It will make it a lot easier to discuss if we have docs on what
these are.  Notch frequency is fine to support and indeed is
a sensible thing to add to the IIO core. The naming you have
also fits well with the existing filter ABI.
I would like to see an _available though.   

So could you define a new entry in iio_chan_info_enum include/linux/iio/types.h
and provide relevant strings in places where those are looked up. 
Do that as a precursor patch to this one, then use that new infrastructure
to implement this.

The fault one is a bit more controversial.  Fault conditions tend to
be annoyingly device specific, so we've never really defined a universal
ABI for them.

Where possible, mapping to IIO events at least keeps things within the
defined ABI, but I'm not sure if that can be sensibly done here.

> +	NULL,
> +};
> +
> +static const struct attribute_group max31865_group = {
> +	.attrs = max31865_attributes,
> +};
> +
> +static const struct iio_info max31865_info = {
> +	.read_raw = max31865_read_raw,
> +	.attrs = &max31865_group,
> +};
> +
> +static int max31865_probe(struct spi_device *spi)
> +{
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +	struct iio_dev *indio_dev;
> +	struct max31865_data *data;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->spi = spi;
> +	data->filter_50hz = false;
> +	data->no_of_wires = 0;

Rename that variable, as 0 wires is unlikely to make much sense.
Also, given it's a choice between two values, it would be
more intuitive to set this in an if/else statement below.

> +
> +	spi_set_drvdata(spi, indio_dev);

Not used so drop this.

> +
> +	indio_dev->info = &max31865_info;
> +	indio_dev->name = id->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = max31865_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max31865_channels);
> +
> +	if (of_property_read_bool(spi->dev.of_node, "maxim,no-of-wires"))
> +		data->no_of_wires = 1;
> +
> +	ret = max31865_init(data);
> +	if (ret) {
> +		dev_err(&spi->dev, "error: Failed to configure max31865\n");
> +		return ret;
> +	}
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static const struct spi_device_id max31865_id[] = {
> +	{ "max31865", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, max31865_id);
> +
> +static const struct of_device_id max31865_of_match[] = {
> +	{ .compatible = "maxim,max31865" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max31865_of_match);
> +
> +static struct spi_driver max31865_driver = {
> +	.driver = {
> +		.name	= "max31865",
> +		.of_match_table = max31865_of_match,
> +	},
> +	.probe = max31865_probe,
> +	.id_table = max31865_id,
> +};
> +module_spi_driver(max31865_driver);
> +
> +MODULE_AUTHOR("Navin Sankar Velliangiri <navin@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Maxim MAX31865 RTD-to-Digital Converter sensor driver");
> +MODULE_LICENSE("GPL v2");




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux