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, ®, 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, ®_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");
