On Thu, 18 Oct 2018 13:36:28 -0500 Matt Weber <matthew.weber@xxxxxxxxxxxxxxxxxxx> wrote: > From: Paresh Chaudhary <paresh.chaudhary@xxxxxxxxxxxxxxxxxxx> > > This patch adds support for Maxim MAX31856 thermocouple > temperature sensor support. > > More information can be found in: > https://www.maximintegrated.com/en/ds/MAX31856.pdf > > NOTE: Driver support only Comparator Mode. > > Signed-off-by: Paresh Chaudhary <paresh.chaudhary@xxxxxxxxxxxxxxxxxxx> > Signed-off-by: Matt Weber <matthew.weber@xxxxxxxxxxxxxxxxxxx> Hi Matt, Paresh, A few more comments below. Generally looking pretty good. Thanks, Jonathan > --- > Changes > v1 -> v2 > [Peter > 1. Fixed all space & 'return' related comments > 2. Removed 'sysfs_create_group' api because > iio_device_register function is handling sysfs entry > 3. Return -EIO if there is any fault > 4. Added check for 'read_size' before spi read call > 5. Removed license text from the source file > 6. Added .o file in alphabetic order > 7. Used #defines instead of magic bits > > v2 -> v3 > [Jonathan > 1. Used bool for fault_oc and fault_ovuv > 2. Changed 'max31856_read' function and use byte array to > store registers value. > 3. Used 'GENMASK' where required > 4. Changed logic 'max31856_thermocouple_read' function. Used > array to read registers value. > 5. Used 'devm_iio_device_register' and removed 'max31856_remove'. > 6. Fixed other cosmetic changes. > 7. Added 'sysfs-bus-iio-temperature-max31856' file and updated > 'MAINTAINERS' file. > --- > .../ABI/testing/sysfs-bus-iio-temperature-max31856 | 23 ++ > MAINTAINERS | 7 + > drivers/iio/temperature/Kconfig | 10 + > drivers/iio/temperature/Makefile | 1 + > drivers/iio/temperature/max31856.c | 359 +++++++++++++++++++++ > 5 files changed, 400 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-temperature-max31856 > create mode 100644 drivers/iio/temperature/max31856.c > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-temperature-max31856 b/Documentation/ABI/testing/sysfs-bus-iio-temperature-max31856 > new file mode 100644 > index 0000000..d3238b8 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-iio-temperature-max31856 > @@ -0,0 +1,23 @@ > +What: /sys/bus/iio/devices/iio:deviceX/fault_oc > +KernelVersion: 4.19 > +Contact: linux-iio@xxxxxxxxxxxxxxx > +Description: > + Open-circuit fault. The detection of open-circuit faults, > + such as those caused by broken thermocouple wires. > + Reading returns either '1' or '0'. > + '1' = An open circuit such as broken thermocouple wires > + has been detected. > + '0' = No open circuit or broken thermocouple wires are detected > + > +What: /sys/bus/iio/devices/iio:deviceX/fault_ovuv > +KernelVersion: 4.19 > +Contact: linux-iio@xxxxxxxxxxxxxxx > +Description: > + Overvoltage or Undervoltage Input Fault. The internal circuitry > + is protected from excessive voltages applied to the thermocouple > + cables by integrated MOSFETs at the T+ and T- inputs, and the > + BIAS output. These MOSFETs turn off when the input voltage is > + negative or greater than VDD. > + Reading returns either '1' or '0'. > + '1' = The input voltage is negative or greater than VDD. > + '0' = The input voltage is positive and less than VDD(default). Thanks, good docs. I think we can live with those two bits of non standard ABI. > diff --git a/MAINTAINERS b/MAINTAINERS > index f642044..3cfa518 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -7157,6 +7157,13 @@ F: drivers/staging/iio/ > F: include/linux/iio/ > F: tools/iio/ > > +MAX31856 IIO DRIVER > +M: Matthew Weber <matthew.weber@xxxxxxxxxxxxxxxxxxx> > +L: linux-iio@xxxxxxxxxxxxxxx > +S: Maintained > +F: drivers/iio/temperature/max31856.c > +F: Documentation/ABI/testing/sysfs-bus-iio-temperature-max31856 > + > IIO UNIT CONVERTER > M: Peter Rosin <peda@xxxxxxxxxx> > L: linux-iio@xxxxxxxxxxxxxxx > diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig > index 82e4a62..c66eeb2 100644 > --- a/drivers/iio/temperature/Kconfig > +++ b/drivers/iio/temperature/Kconfig > @@ -97,4 +97,14 @@ config TSYS02D > This driver can also be built as a module. If so, the module will > be called tsys02d. > > +config MAX31856 > + tristate "MAX31856 thermocouple sensor" > + depends on SPI > + help > + If you say yes here you get support for MAX31856 > + thermocouple sensor chip connected via SPI. > + > + This driver can also be built as a module. If so, the module > + will be called max31856. > + > endmenu > diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile > index 34a31db..baca477 100644 > --- a/drivers/iio/temperature/Makefile > +++ b/drivers/iio/temperature/Makefile > @@ -5,6 +5,7 @@ > > obj-$(CONFIG_HID_SENSOR_TEMP) += hid-sensor-temperature.o > obj-$(CONFIG_MAXIM_THERMOCOUPLE) += maxim_thermocouple.o > +obj-$(CONFIG_MAX31856) += max31856.o > obj-$(CONFIG_MLX90614) += mlx90614.o > obj-$(CONFIG_MLX90632) += mlx90632.o > obj-$(CONFIG_TMP006) += tmp006.o > diff --git a/drivers/iio/temperature/max31856.c b/drivers/iio/temperature/max31856.c > new file mode 100644 > index 0000000..8ba1b34 > --- /dev/null > +++ b/drivers/iio/temperature/max31856.c > @@ -0,0 +1,359 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* max31856.c > + * > + * Maxim MAX31856 thermocouple sensor driver > + * > + * Copyright (C) 2018 Rockwell Collins > + */ > + > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/err.h> > +#include <linux/spi/spi.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > + > +#define MAX31856_RD_WR_BIT_POS 7 > +#define MAX31856_CR0_AUTOCONVERT 0x80 > +#define MAX31856_CR0_1SHOT 0x40 > +#define MAX31856_CR0_OCFAULT BIT(4) > +#define MAX31856_CR0_OCFAULT_MASK GENMASK(5, 4) > +#define MAX31856_TC_TYPE_MASK GENMASK(3, 0) > +#define MAX31856_FAULT_OVUV 0x02 > +#define MAX31856_FAULT_OPEN 0x01 > + > +/* The MAX31856 registers */ > +#define MAX31856_CR0_REG 0x00 > +#define MAX31856_CR1_REG 0x01 > +#define MAX31856_MASK_REG 0x02 > +#define MAX31856_CJHF_REG 0x03 > +#define MAX31856_CJLF_REG 0x04 > +#define MAX31856_LTHFTH_REG 0x05 > +#define MAX31856_LTHFTL_REG 0x06 > +#define MAX31856_LTLFTH_REG 0x07 > +#define MAX31856_LTLFTL_REG 0x08 > +#define MAX31856_CJTO_REG 0x09 > +#define MAX31856_CJTH_REG 0x0A > +#define MAX31856_CJTL_REG 0x0B > +#define MAX31856_LTCBH_REG 0x0C > +#define MAX31856_LTCBM_REG 0x0D > +#define MAX31856_LTCBL_REG 0x0E > +#define MAX31856_SR_REG 0x0F > + > +static const struct iio_chan_spec max31856_channels[] = { > + { /* Thermocouple Temperature */ > + .type = IIO_TEMP, > + .address = 2, > + .info_mask_separate = > + BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE), > + }, > + { /* Cold Junction Temperature */ > + .type = IIO_TEMP, > + .channel2 = IIO_MOD_TEMP_AMBIENT, > + .address = 0, > + .modified = 1, > + .info_mask_separate = > + BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE), > + }, > +}; > + > +struct max31856_data { > + struct spi_device *spi; > + bool one_shot; > + u32 thermocouple_type; > + bool fault_ovuv; > + bool fault_oc; > + u8 buf[3] ____cacheline_aligned; > +}; > + > +static int max31856_read(struct max31856_data *data, unsigned int reg, > + u8 val[], int read_size) > +{ > + int ret; > + u8 *buf = data->buf; > + > + /* > + * Make sure top bit is not set, > + * The MSB(A7) of this byte determines whether the following byte will > + * be written or read. If A7 is 0, one or more byte reads will follow > + * the address byte > + */ > + > + reg &= ~(1 << MAX31856_RD_WR_BIT_POS); > + > + buf[0] = reg; > + buf[1] = 0x00; > + buf[2] = 0x00; > + > + if (read_size > 3) { > + pr_err("error: Unsupported read_size = %d by %s\n", > + read_size, __func__); > + return -EINVAL; > + } > + > + ret = spi_write_then_read(data->spi, buf, 1, buf, read_size); > + if (ret < 0) > + return ret; > + > + switch (read_size) { There is only one route to hit the default here which is a read_size of 0. I'll assume that never makes sense so why do we need the switch statement? > + case 1 ... 3: > + memcpy(val, buf, read_size); > + return 0; > + default: > + return -EINVAL; > + } > +} > + > +static int max31856_write(struct max31856_data *data, unsigned int reg, > + unsigned int val) > +{ > + u8 *buf = data->buf; > + > + /* > + * Make sure top bit is set, > + * The MSB(A7) of this byte determines whether the following byte will > + * be written or read. If A7 is 1, one or more byte writes will follow > + * the address byte > + */ > + > + buf[0] = reg | (1 << MAX31856_RD_WR_BIT_POS); > + buf[1] = val; > + > + return spi_write(data->spi, buf, 2); > +} > + > +static int max31856_init(struct max31856_data *data) > +{ > + int ret; > + u8 reg_val[1]; > + > + /* > + * Enable Open circuit fault detection [01] > + * Read datasheet for more information: Table 4. > + * BITS [5:4] | Fault Test > + * --------------------------- > + * 00 | Disabled > + * --------------------------- > + * 01 | Enabled (Once every 16 seconds) > + * --------------------------- > + * 10 | Enabled (Once every 16 seconds) > + * --------------------------- > + * 11 | Enabled (Once every 16 seconds) > + */ > + ret = max31856_read(data, MAX31856_CR0_REG, reg_val, 1); > + if (ret) > + return ret; > + reg_val[0] &= ~MAX31856_CR0_OCFAULT_MASK; > + reg_val[0] |= MAX31856_CR0_OCFAULT; > + ret = max31856_write(data, MAX31856_CR0_REG, reg_val[0]); > + if (ret) > + return ret; > + > + /* Set thermocouple type based on dts property */ > + ret = max31856_read(data, MAX31856_CR1_REG, reg_val, 1); > + if (ret) > + return ret; > + > + reg_val[0] &= ~MAX31856_TC_TYPE_MASK; > + reg_val[0] |= data->thermocouple_type; > + ret = max31856_write(data, MAX31856_CR1_REG, reg_val[0]); > + if (ret) > + return ret; > + > + /* Set Conversion Mode (Auto or Oneshot) based on dts property */ > + ret = max31856_read(data, MAX31856_CR0_REG, reg_val, 1); > + if (ret) > + return ret; > + if (data->one_shot) { > + reg_val[0] &= ~MAX31856_CR0_AUTOCONVERT; > + reg_val[0] |= MAX31856_CR0_1SHOT; > + } else { > + reg_val[0] |= MAX31856_CR0_AUTOCONVERT; > + reg_val[0] &= ~MAX31856_CR0_1SHOT; > + } > + > + return max31856_write(data, MAX31856_CR0_REG, reg_val[0]); > +} > + > +static int max31856_thermocouple_read(struct max31856_data *data, > + struct iio_chan_spec const *chan, > + int *val) > +{ > + int ret, offset_cjto; > + u8 reg_val[3]; > + > + switch (chan->channel2) { > + case IIO_MOD_TEMP_AMBIENT: > + /* > + * Multibyte Read > + * MAX31856_CJTO_REG, MAX31856_CJTH_REG, MAX31856_CJTL_REG > + */ > + ret = max31856_read(data, MAX31856_CJTO_REG, reg_val, 3); > + if (ret) > + return ret; > + /* Get Cold Junction Temp. offset register value */ > + offset_cjto = reg_val[0]; > + /* Get CJTH and CJTL value and skip last 2 dead bits of CJTL*/ > + *val = (reg_val[1] << 8 | reg_val[2]) >> 2; > + /* As per datasheet add offset into CJTH and CJTL */ > + *val += offset_cjto; > + /* Check 7th bit of CJTH reg. value for sign */ > + if (reg_val[1] & 0x80) > + *val -= 0x4000; > + break; > + default: > + /* > + * Multibyte Read > + * MAX31856_LTCBH_REG, MAX31856_LTCBM_REG, MAX31856_LTCBL_REG > + */ > + ret = max31856_read(data, MAX31856_LTCBH_REG, reg_val, 3); I think an error here will get eaten up by what follows. Should be checked and handled. > + /* Skip last 5 dead bits of LTCBL */ > + *val = (reg_val[0] << 16 | reg_val[1] << 8 | reg_val[2]) >> 5; > + /* Check 7th bit of LTCBH reg. value for sign*/ > + if (reg_val[0] & 0x80) > + *val -= 0x80000; > + } > + > + ret = max31856_read(data, MAX31856_SR_REG, reg_val, 1); If this has read garbage for some reason, we should probably return an error from that rather than hoping we are fine to detect the oc or ovuv faults. > + /* Check for over voltage/under voltage fault */ > + data->fault_ovuv = (reg_val[0] & MAX31856_FAULT_OVUV) ? true : false; > + /* Check for open circuit fault */ > + data->fault_oc = (reg_val[0] & MAX31856_FAULT_OPEN) ? true : false; > + if (data->fault_oc || data->fault_ovuv) > + return -EIO; > + > + return ret; > +} > + > +static int max31856_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct max31856_data *data = iio_priv(indio_dev); > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + ret = max31856_thermocouple_read(data, chan, val); > + if (ret) > + return ret; > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SCALE: > + switch (chan->channel2) { > + case IIO_MOD_TEMP_AMBIENT: > + /* Cold junction Temp. Data resolution is 0.015625 */ > + *val = 15; > + *val2 = 625000; /* 1000 * 0.015625 */ > + ret = IIO_VAL_INT_PLUS_MICRO; > + break; > + default: > + /* Thermocouple Temp. Data resolution is 0.0078125 */ > + *val = 7; > + *val2 = 812500; /* 1000 * 0.0078125) */ > + return IIO_VAL_INT_PLUS_MICRO; > + } > + break; > + } > + > + return ret; > +} > + > +static ssize_t show_fault_ovuv(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct max31856_data *data = iio_priv(indio_dev); > + > + return sprintf(buf, "%d\n", data->fault_ovuv); > +} > + > +static ssize_t show_fault_oc(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct max31856_data *data = iio_priv(indio_dev); > + > + return sprintf(buf, "%d\n", data->fault_oc); > +} > + > +static IIO_DEVICE_ATTR(fault_ovuv, 0444, show_fault_ovuv, NULL, 0); > +static IIO_DEVICE_ATTR(fault_oc, 0444, show_fault_oc, NULL, 0); > + > +static struct attribute *max31856_attributes[] = { > + &iio_dev_attr_fault_ovuv.dev_attr.attr, > + &iio_dev_attr_fault_oc.dev_attr.attr, > + NULL, > +}; > + > +static const struct attribute_group max31856_group = { > + .attrs = max31856_attributes, > +}; > + > +static const struct iio_info max31856_info = { > + .driver_module = THIS_MODULE, > + .read_raw = max31856_read_raw, > + .attrs = &max31856_group, > +}; > + > +static int max31856_probe(struct spi_device *spi) > +{ > + const struct spi_device_id *id = spi_get_device_id(spi); > + struct iio_dev *indio_dev; > + struct max31856_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; > + > + spi_set_drvdata(spi, indio_dev); > + > + indio_dev->info = &max31856_info; > + indio_dev->name = id->name; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = max31856_channels; > + indio_dev->num_channels = ARRAY_SIZE(max31856_channels); > + > + data->one_shot = of_property_read_bool(spi->dev.of_node, "one-shot"); For v4, best bet is probably to just drop one_shot until we have figured out how to support it well. > + > + ret = of_property_read_u32(spi->dev.of_node, "type", > + &data->thermocouple_type); > + > + if (ret) { > + pr_info("Could not read thermocouple type DT property, Configuring as a K-Type\n"); > + data->thermocouple_type = 0x03; /* K-Type */ > + } > + > + ret = max31856_init(data); > + if (ret) { > + pr_err("error: Failed to configure max31856\n"); > + return ret; > + } > + > + return devm_iio_device_register(&spi->dev, indio_dev); > +} > + > +static const struct spi_device_id max31856_id[] = { > + { "max31856", 0 }, > + { } > +}; Please add an explicit devicetree id table to include the manufacturer ID. In theory (and it may happen one day) the code that instantiates them from the old school spi_device_id table is going away. > +MODULE_DEVICE_TABLE(spi, max31856_id); > + > +static struct spi_driver max31856_driver = { > + .driver = { > + .name = "max31856", > + .owner = THIS_MODULE, I'm fairly sure that hasn't been needed for a long time. It's dealt with the in the registration functions via some macro magic. > + }, > + .probe = max31856_probe, > + .id_table = max31856_id, > +}; > +module_spi_driver(max31856_driver); > + > +MODULE_AUTHOR("Paresh Chaudhary <paresh.chaudhary@xxxxxxxxxxxxxxxxxxx>"); > +MODULE_DESCRIPTION("Maxim MAX31856 thermocouple sensor driver"); > +MODULE_LICENSE("GPL");
