[PATCH v2 0/3] hwmon: Add max31732 quad remote temperature sensor driver

[PATCH v2 0/3] hwmon: Add max31732 quad remote temperature sensor driver

[Sinan Divarci]

changes in v2:
  - remove Reviewed-by tag

Sinan Divarci (3):
  drivers: hwmon: Add max31732 quad remote temperature sensor driver
  docs: hwmon: add max31732 documentation
  dt-bindings: hwmon: Add bindings for max31732

 .../bindings/hwmon/adi,max31732.yaml          |  83 +++
 Documentation/hwmon/index.rst                 |   1 +
 Documentation/hwmon/max31732.rst              |  62 ++
 drivers/hwmon/Kconfig                         |  11 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/max31732.c                      | 620 ++++++++++++++++++
 6 files changed, 778 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/adi,max31732.yaml
 create mode 100644 Documentation/hwmon/max31732.rst
 create mode 100644 drivers/hwmon/max31732.c


base-commit: e2ca6ba6ba0152361aa4fcbf6067db71b2c7a770
-- 
2.25.1

[PATCH v2 1/3] drivers: hwmon: Add max31732 quad remote temperature sensor driver

[Sinan Divarci]

The MAX31732 is a multi-channel temperature sensor that monitors its own
temperature and the temperatures of up to four external diodeconnected
transistors.

The MAX31732 offers two open-drain, active-low alarm outputs, ALARM1
and ALARM2. When the measured temperature of a channel crosses the
respective primary over/under temperature threshold levels ALARM1
asserts low and a status bit is set in the corresponding thermal status
registers. When the measured temperature of a channel crosses the
secondary over/under temperature threshold levels, ALARM2 asserts low
and a status bit is set in the corresponding thermal status registers.

Signed-off-by: Sinan Divarci <Sinan.Divarci@analog.com>
---
 drivers/hwmon/Kconfig    |  11 +
 drivers/hwmon/Makefile   |   1 +
 drivers/hwmon/max31732.c | 620 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 632 insertions(+)
 create mode 100644 drivers/hwmon/max31732.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 3176c33af..f498f3867 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1076,6 +1076,17 @@ config SENSORS_MAX31730
 	  This driver can also be built as a module. If so, the module
 	  will be called max31730.
 
+config SENSORS_MAX31732
+	tristate "MAX31732 temperature sensor"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Support for the Analog Devices MAX31732 4-Channel Remote
+	  Temperature Sensor.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called max31732.
+
 config SENSORS_MAX31760
 	tristate "MAX31760 fan speed controller"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index e2e4e87b2..6b2871cc5 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -140,6 +140,7 @@ obj-$(CONFIG_SENSORS_MAX1668)	+= max1668.o
 obj-$(CONFIG_SENSORS_MAX197)	+= max197.o
 obj-$(CONFIG_SENSORS_MAX31722)	+= max31722.o
 obj-$(CONFIG_SENSORS_MAX31730)	+= max31730.o
+obj-$(CONFIG_SENSORS_MAX31732)	+= max31732.o
 obj-$(CONFIG_SENSORS_MAX31760)  += max31760.o
 obj-$(CONFIG_SENSORS_MAX6620)	+= max6620.o
 obj-$(CONFIG_SENSORS_MAX6621)	+= max6621.o
diff --git a/drivers/hwmon/max31732.c b/drivers/hwmon/max31732.c
new file mode 100644
index 000000000..cf075c990
--- /dev/null
+++ b/drivers/hwmon/max31732.c
@@ -0,0 +1,620 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Driver for MAX31732 4-Channel Remote Temperature Sensor
+ */
+
+#include <linux/bitfield.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/regmap.h>
+
+/* common definitions*/
+#define MAX3173X_STOP			BIT(7)
+#define MAX3173X_ALARM_MODE		BIT(4)
+#define MAX3173X_ALARM_FAULT_QUEUE_MASK	GENMASK(3, 2)
+#define MAX3173X_EXTRANGE		BIT(1)
+#define MAX3173X_TEMP_OFFSET_BASELINE	0x77
+#define MAX3173X_TEMP_MIN		(-128000)
+#define MAX3173X_TEMP_MAX		127937
+#define MAX3173X_OFFSET_MIN		(-14875)
+#define MAX3173X_OFFSET_MAX		17000
+#define MAX3173X_OFFSET_ZERO		14875
+#define MAX31732_SECOND_TEMP_MIN	(-128000)
+#define MAX31732_SECOND_TEMP_MAX	127000
+#define MAX31732_CUSTOM_OFFSET_RES	125
+#define MAX31732_ALL_CHANNEL_MASK	0x1F
+#define MAX31732_ALARM_INT_MODE		0
+#define MAX31732_ALARM_COMP_MODE	1
+#define MAX31732_ALARM_FAULT_QUE	1
+#define MAX31732_ALARM_FAULT_QUE_MAX	3
+
+/* The MAX31732 registers */
+#define MAX31732_REG_TEMP_R		0x02
+#define MAX31732_REG_TEMP_L		0x0A
+#define MAX31732_REG_PRIM_HIGH_STATUS	0x0C
+#define MAX31732_REG_PRIM_LOW_STATUS	0x0D
+#define MAX31732_REG_CHANNEL_ENABLE	0x0E
+#define MAX31732_REG_CONF1		0x0F
+#define MAX31732_REG_CONF2		0x10
+#define MAX31732_REG_TEMP_OFFSET	0x16
+#define MAX31732_REG_OFFSET_ENABLE	0x17
+#define MAX31732_REG_ALARM1_MASK	0x1B
+#define MAX31732_REG_ALARM2_MASK	0x1C
+#define MAX31732_REG_PRIM_HIGH_TEMP_R	0x1D
+#define MAX31732_REG_PRIM_HIGH_TEMP_L	0x25
+#define MAX31732_REG_PRIM_LOW_TEMP	0x27
+#define MAX31732_REG_SECOND_HIGH_TEMP_R	0x29
+#define MAX31732_REG_SECOND_HIGH_TEMP_L	0x2D
+#define MAX31732_REG_SECOND_LOW_TEMP	0x2E
+#define MAX31732_REG_SECOND_HIGH_STATUS	0x42
+#define MAX31732_REG_SECOND_LOW_STATUS	0x43
+#define MAX31732_REG_TEMP_FAULT		0x44
+
+enum max31732_temp_type {
+	MAX31732_TEMP,
+	MAX31732_PRIM_HIGH,
+	MAX31732_SECOND_HIGH
+};
+
+struct max31732_data {
+	struct i2c_client	*client;
+	struct device		*hwmon_dev;
+	struct regmap		*regmap;
+	s32			irqs[2];
+};
+
+static u32 max31732_get_temp_reg(enum max31732_temp_type temp_type, u32 channel)
+{
+	switch (temp_type) {
+	case MAX31732_PRIM_HIGH:
+		if (channel == 0)
+			return MAX31732_REG_PRIM_HIGH_TEMP_L;
+		else
+			return (MAX31732_REG_PRIM_HIGH_TEMP_R + (channel - 1) * 2);
+		break;
+	case MAX31732_SECOND_HIGH:
+		if (channel == 0)
+			return MAX31732_REG_SECOND_HIGH_TEMP_L;
+		else
+			return (MAX31732_REG_SECOND_HIGH_TEMP_R + (channel - 1));
+		break;
+	case MAX31732_TEMP:
+	default:
+		if (channel == 0)
+			return MAX31732_REG_TEMP_L;
+		else
+			return (MAX31732_REG_TEMP_R + (channel - 1) * 2);
+		break;
+	}
+}
+
+static bool max31732_volatile_reg(struct device *dev, u32 reg)
+{
+	if (reg >= MAX31732_REG_TEMP_R && reg <= MAX31732_REG_PRIM_LOW_STATUS)
+		return true;
+
+	if (reg == MAX31732_REG_SECOND_HIGH_STATUS || reg == MAX31732_REG_SECOND_LOW_STATUS)
+		return true;
+
+	if (reg == MAX31732_REG_TEMP_FAULT)
+		return true;
+
+	return false;
+}
+
+static const struct regmap_config regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.cache_type = REGCACHE_RBTREE,
+	.volatile_reg = max31732_volatile_reg,
+};
+
+static inline long max31732_reg_to_mc(s16 temp)
+{
+	return DIV_ROUND_CLOSEST((temp / 16) * 1000, 16);
+}
+
+static int max31732_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, s32 channel,
+			 long *val)
+{
+	struct max31732_data *data = dev_get_drvdata(dev);
+	s32 ret;
+	u32 reg_val, reg_addr;
+	s16 temp_reg_val;
+	u8 regs[2];
+
+	if (type != hwmon_temp)
+		return -EINVAL;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		ret = regmap_test_bits(data->regmap, MAX31732_REG_CHANNEL_ENABLE, BIT(channel));
+		if (ret < 0)
+			return ret;
+
+		if (!ret)
+			return -ENODATA;
+
+		reg_addr = max31732_get_temp_reg(MAX31732_TEMP, channel);
+		break;
+	case hwmon_temp_max:
+		reg_addr = max31732_get_temp_reg(MAX31732_PRIM_HIGH, channel);
+		break;
+	case hwmon_temp_min:
+		reg_addr = MAX31732_REG_PRIM_LOW_TEMP;
+		break;
+	case hwmon_temp_lcrit:
+		ret = regmap_read(data->regmap, MAX31732_REG_SECOND_LOW_TEMP, &reg_val);
+		if (ret)
+			return ret;
+
+		*val = reg_val * 1000;
+		return 0;
+	case hwmon_temp_crit:
+		reg_addr = max31732_get_temp_reg(MAX31732_SECOND_HIGH, channel);
+		ret = regmap_read(data->regmap, reg_addr, &reg_val);
+		if (ret)
+			return ret;
+
+		*val = reg_val * 1000;
+		return 0;
+	case hwmon_temp_enable:
+		ret = regmap_test_bits(data->regmap, MAX31732_REG_CHANNEL_ENABLE, BIT(channel));
+		if (ret < 0)
+			return ret;
+
+		*val = ret;
+		return 0;
+	case hwmon_temp_offset:
+		if (channel == 0)
+			return -EINVAL;
+
+		ret = regmap_test_bits(data->regmap, MAX31732_REG_OFFSET_ENABLE, BIT(channel));
+		if (ret < 0)
+			return ret;
+
+		if (!ret)
+			return 0;
+
+		ret = regmap_read(data->regmap, MAX31732_REG_TEMP_OFFSET, &reg_val);
+		if (ret)
+			return ret;
+
+		*val = (reg_val - MAX3173X_TEMP_OFFSET_BASELINE) * MAX31732_CUSTOM_OFFSET_RES;
+		return 0;
+	case hwmon_temp_fault:
+		ret = regmap_test_bits(data->regmap, MAX31732_REG_TEMP_FAULT, BIT(channel));
+		if (ret < 0)
+			return ret;
+
+		*val = ret;
+		return 0;
+	case hwmon_temp_lcrit_alarm:
+		ret = regmap_test_bits(data->regmap, MAX31732_REG_SECOND_LOW_STATUS, BIT(channel));
+		if (ret < 0)
+			return ret;
+
+		*val = ret;
+		return 0;
+	case hwmon_temp_min_alarm:
+		ret = regmap_test_bits(data->regmap, MAX31732_REG_PRIM_LOW_STATUS, BIT(channel));
+		if (ret < 0)
+			return ret;
+
+		*val = ret;
+		return 0;
+	case hwmon_temp_max_alarm:
+		ret = regmap_test_bits(data->regmap, MAX31732_REG_PRIM_HIGH_STATUS, BIT(channel));
+		if (ret < 0)
+			return ret;
+
+		*val = ret;
+		return 0;
+	case hwmon_temp_crit_alarm:
+		ret = regmap_test_bits(data->regmap, MAX31732_REG_SECOND_HIGH_STATUS, BIT(channel));
+		if (ret < 0)
+			return ret;
+
+		*val = ret;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_bulk_read(data->regmap, reg_addr, &regs, 2);
+	if (ret < 0)
+		return ret;
+
+	temp_reg_val = regs[1] | regs[0] << 8;
+	*val = max31732_reg_to_mc(temp_reg_val);
+	return 0;
+}
+
+static int max31732_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, s32 channel,
+			  long val)
+{
+	struct max31732_data *data = dev_get_drvdata(dev);
+	s32 reg_addr, ret;
+	u16 temp_reg_val;
+
+	if (type != hwmon_temp)
+		return -EINVAL;
+
+	switch (attr) {
+	case hwmon_temp_max:
+		reg_addr = max31732_get_temp_reg(MAX31732_PRIM_HIGH, channel);
+		break;
+	case hwmon_temp_min:
+		reg_addr = MAX31732_REG_PRIM_LOW_TEMP;
+		break;
+	case hwmon_temp_enable:
+		if (val == 0) {
+			return regmap_clear_bits(data->regmap, MAX31732_REG_CHANNEL_ENABLE,
+						 BIT(channel));
+		} else if (val == 1) {
+			return regmap_set_bits(data->regmap, MAX31732_REG_CHANNEL_ENABLE,
+					       BIT(channel));
+		} else {
+			return -EINVAL;
+		}
+	case hwmon_temp_offset:
+		val = clamp_val(val, MAX3173X_OFFSET_MIN, MAX3173X_OFFSET_MAX) +
+				MAX3173X_OFFSET_ZERO;
+		val = DIV_ROUND_CLOSEST(val, 125);
+
+		if (val == MAX3173X_TEMP_OFFSET_BASELINE) {
+			ret = regmap_clear_bits(data->regmap, MAX31732_REG_OFFSET_ENABLE,
+						BIT(channel));
+		} else {
+			ret = regmap_set_bits(data->regmap, MAX31732_REG_OFFSET_ENABLE,
+					      BIT(channel));
+		}
+		if (ret)
+			return ret;
+
+		return regmap_write(data->regmap, MAX31732_REG_TEMP_OFFSET, val);
+	case hwmon_temp_crit:
+		val = clamp_val(val, MAX31732_SECOND_TEMP_MIN, MAX31732_SECOND_TEMP_MAX);
+		val = DIV_ROUND_CLOSEST(val, 1000);
+		reg_addr = max31732_get_temp_reg(MAX31732_SECOND_HIGH, channel);
+		return regmap_write(data->regmap, reg_addr, val);
+	case hwmon_temp_lcrit:
+		val = clamp_val(val, MAX31732_SECOND_TEMP_MIN, MAX31732_SECOND_TEMP_MAX);
+		val = DIV_ROUND_CLOSEST(val, 1000);
+		return regmap_write(data->regmap, MAX31732_REG_SECOND_LOW_TEMP, val);
+	default:
+		return -EINVAL;
+	}
+
+	val = clamp_val(val, MAX3173X_TEMP_MIN, MAX3173X_TEMP_MAX);
+	val = DIV_ROUND_CLOSEST(val << 4, 1000) << 4;
+
+	temp_reg_val = (u16)val;
+	temp_reg_val = swab16(temp_reg_val);
+
+	return regmap_bulk_write(data->regmap, reg_addr, &temp_reg_val, sizeof(temp_reg_val));
+}
+
+static umode_t max31732_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr,
+				   s32 channel)
+{
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+		case hwmon_temp_lcrit_alarm:
+		case hwmon_temp_min_alarm:
+		case hwmon_temp_max_alarm:
+		case hwmon_temp_crit_alarm:
+		case hwmon_temp_fault:
+			return 0444;
+		case hwmon_temp_min:
+		case hwmon_temp_lcrit:
+			return channel ? 0444 : 0644;
+		case hwmon_temp_offset:
+		case hwmon_temp_enable:
+		case hwmon_temp_max:
+		case hwmon_temp_crit:
+			return 0644;
+		}
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static irqreturn_t max31732_irq_handler(s32 irq, void *data)
+{
+	struct device *dev = data;
+	struct max31732_data *drvdata = dev_get_drvdata(dev);
+	s32 ret;
+	u32 reg_val;
+	bool reported = false;
+
+	ret = regmap_read(drvdata->regmap, MAX31732_REG_PRIM_HIGH_STATUS, &reg_val);
+	if (ret)
+		return ret;
+
+	if (reg_val != 0) {
+		dev_crit(dev, "Primary Overtemperature Alarm, R4:%d R3:%d R2:%d R1:%d L:%d.\n",
+			 !!(reg_val & BIT(4)), !!(reg_val & BIT(3)), !!(reg_val & BIT(2)),
+			 !!(reg_val & BIT(1)), !!(reg_val & BIT(0)));
+		hwmon_notify_event(drvdata->hwmon_dev, hwmon_temp, hwmon_temp_max_alarm, 0);
+		reported = true;
+	}
+
+	ret = regmap_read(drvdata->regmap, MAX31732_REG_PRIM_LOW_STATUS, &reg_val);
+	if (ret)
+		return ret;
+
+	if (reg_val != 0) {
+		dev_crit(dev, "Primary Undertemperature Alarm, R4:%d R3:%d R2:%d R1:%d L:%d.\n",
+			 !!(reg_val & BIT(4)), !!(reg_val & BIT(3)), !!(reg_val & BIT(2)),
+			 !!(reg_val & BIT(1)), !!(reg_val & BIT(0)));
+		hwmon_notify_event(drvdata->hwmon_dev, hwmon_temp, hwmon_temp_min_alarm, 0);
+		reported = true;
+	}
+
+	ret = regmap_read(drvdata->regmap, MAX31732_REG_SECOND_HIGH_STATUS, &reg_val);
+	if (ret)
+		return ret;
+
+	if (reg_val != 0) {
+		dev_crit(dev, "Secondary Overtemperature Alarm, R4:%d R3:%d R2:%d R1:%d L:%d.\n",
+			 !!(reg_val & BIT(4)), !!(reg_val & BIT(3)), !!(reg_val & BIT(2)),
+			 !!(reg_val & BIT(1)), !!(reg_val & BIT(0)));
+		hwmon_notify_event(drvdata->hwmon_dev, hwmon_temp, hwmon_temp_crit_alarm, 0);
+		reported = true;
+	}
+
+	ret = regmap_read(drvdata->regmap, MAX31732_REG_SECOND_LOW_STATUS, &reg_val);
+	if (ret)
+		return ret;
+
+	if (reg_val != 0) {
+		dev_crit(dev, "Secondary Undertemperature Alarm, R4:%d R3:%d R2:%d R1:%d L:%d.\n",
+			 !!(reg_val & BIT(4)), !!(reg_val & BIT(3)), !!(reg_val & BIT(2)),
+			 !!(reg_val & BIT(1)), !!(reg_val & BIT(0)));
+		hwmon_notify_event(drvdata->hwmon_dev, hwmon_temp, hwmon_temp_lcrit_alarm, 0);
+		reported = true;
+	}
+
+	if (!reported) {
+		if (irq == drvdata->irqs[0])
+			dev_err(dev, "ALARM1 interrupt received but status registers not set.\n");
+		else if (irq == drvdata->irqs[1])
+			dev_err(dev, "ALARM2 interrupt received but status registers not set.\n");
+		else
+			dev_err(dev, "Undefined interrupt source.\n");
+	}
+
+	return IRQ_HANDLED;
+}
+
+static const struct hwmon_channel_info *max31732_info[] = {
+	HWMON_CHANNEL_INFO(chip,
+			   HWMON_C_REGISTER_TZ),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_LCRIT |
+			   HWMON_T_CRIT |
+			   HWMON_T_ENABLE |
+			   HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_LCRIT_ALARM,
+			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_LCRIT |
+			   HWMON_T_CRIT |
+			   HWMON_T_OFFSET | HWMON_T_ENABLE |
+			   HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_LCRIT_ALARM |
+			   HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_LCRIT |
+			   HWMON_T_CRIT |
+			   HWMON_T_OFFSET | HWMON_T_ENABLE |
+			   HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_LCRIT_ALARM |
+			   HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_LCRIT |
+			   HWMON_T_CRIT |
+			   HWMON_T_OFFSET | HWMON_T_ENABLE |
+			   HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_LCRIT_ALARM |
+			   HWMON_T_FAULT,
+			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_LCRIT |
+			   HWMON_T_CRIT |
+			   HWMON_T_OFFSET | HWMON_T_ENABLE |
+			   HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
+			   HWMON_T_LCRIT_ALARM |
+			   HWMON_T_FAULT
+			   ),
+	NULL
+};
+
+static const struct hwmon_ops max31732_hwmon_ops = {
+	.is_visible = max31732_is_visible,
+	.read = max31732_read,
+	.write = max31732_write,
+};
+
+static const struct hwmon_chip_info max31732_chip_info = {
+	.ops = &max31732_hwmon_ops,
+	.info = max31732_info,
+};
+
+static int max31732_parse_alarms(struct device *dev, struct max31732_data *data)
+{
+	s32 ret;
+	u32 alarm_que;
+
+	if (fwnode_property_read_bool(dev_fwnode(dev), "adi,alarm1-interrupt-mode"))
+		ret = regmap_clear_bits(data->regmap, MAX31732_REG_CONF1, MAX3173X_ALARM_MODE);
+	else
+		ret = regmap_set_bits(data->regmap, MAX31732_REG_CONF1, MAX3173X_ALARM_MODE);
+
+	if (ret)
+		return ret;
+
+	if (fwnode_property_read_bool(dev_fwnode(dev), "adi,alarm2-interrupt-mode"))
+		ret = regmap_clear_bits(data->regmap, MAX31732_REG_CONF2, MAX3173X_ALARM_MODE);
+	else
+		ret = regmap_set_bits(data->regmap, MAX31732_REG_CONF2, MAX3173X_ALARM_MODE);
+
+	if (ret)
+		return ret;
+
+	alarm_que = MAX31732_ALARM_FAULT_QUE;
+	fwnode_property_read_u32(dev_fwnode(dev), "adi,alarm1-fault-queue", &alarm_que);
+
+	if ((alarm_que / 2) <= MAX31732_ALARM_FAULT_QUE_MAX) {
+		ret = regmap_write_bits(data->regmap, MAX31732_REG_CONF1,
+					MAX3173X_ALARM_FAULT_QUEUE_MASK,
+					FIELD_PREP(MAX3173X_ALARM_FAULT_QUEUE_MASK,
+						   (alarm_que / 2)));
+		if (ret)
+			return ret;
+	} else {
+		return dev_err_probe(dev, -EINVAL, "Invalid adi,alarm1-fault-queue.\n");
+	}
+
+	alarm_que = MAX31732_ALARM_FAULT_QUE;
+	fwnode_property_read_u32(dev_fwnode(dev), "adi,alarm2-fault-queue", &alarm_que);
+
+	if ((alarm_que / 2) <= MAX31732_ALARM_FAULT_QUE_MAX) {
+		ret = regmap_write_bits(data->regmap, MAX31732_REG_CONF2,
+					MAX3173X_ALARM_FAULT_QUEUE_MASK,
+					FIELD_PREP(MAX3173X_ALARM_FAULT_QUEUE_MASK,
+						   (alarm_que / 2)));
+	} else {
+		return dev_err_probe(dev, -EINVAL, "Invalid adi,alarm2-fault-queue.\n");
+	}
+
+	return ret;
+}
+
+static int max31732_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct max31732_data *data;
+	s32 ret;
+	u32 reg_val;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+
+	data->regmap = devm_regmap_init_i2c(client, &regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(dev, PTR_ERR(data->regmap), "regmap init failed\n");
+
+	ret = regmap_read(data->regmap, MAX31732_REG_CHANNEL_ENABLE, &reg_val);
+	if (ret)
+		return ret;
+
+	if (reg_val == 0)
+		ret = regmap_set_bits(data->regmap, MAX31732_REG_CONF1, MAX3173X_STOP);
+	else
+		ret = regmap_clear_bits(data->regmap, MAX31732_REG_CONF1, MAX3173X_STOP);
+
+	if (ret)
+		return ret;
+
+	ret = regmap_clear_bits(data->regmap, MAX31732_REG_CONF1, MAX3173X_EXTRANGE);
+	if (ret)
+		return ret;
+
+	ret = max31732_parse_alarms(dev, data);
+	if (ret)
+		return ret;
+
+	dev_set_drvdata(dev, data);
+
+	data->irqs[0] = fwnode_irq_get_byname(dev_fwnode(dev), "ALARM1");
+	if (data->irqs[0] > 0) {
+		ret = devm_request_threaded_irq(dev, data->irqs[0], NULL, max31732_irq_handler,
+						IRQF_ONESHOT, client->name, dev);
+		if (ret)
+			return dev_err_probe(dev, ret, "cannot request irq\n");
+
+		ret = regmap_set_bits(data->regmap, MAX31732_REG_ALARM1_MASK,
+				      MAX31732_ALL_CHANNEL_MASK);
+		if (ret)
+			return ret;
+	} else {
+		ret = regmap_clear_bits(data->regmap, MAX31732_REG_ALARM1_MASK,
+					MAX31732_ALL_CHANNEL_MASK);
+		if (ret)
+			return ret;
+	}
+
+	data->irqs[1] = fwnode_irq_get_byname(dev_fwnode(dev), "ALARM2");
+	if (data->irqs[1] > 0) {
+		ret = devm_request_threaded_irq(dev, data->irqs[1], NULL, max31732_irq_handler,
+						IRQF_ONESHOT, client->name, dev);
+		if (ret)
+			return dev_err_probe(dev, ret, "cannot request irq\n");
+
+		ret = regmap_set_bits(data->regmap, MAX31732_REG_ALARM2_MASK,
+				      MAX31732_ALL_CHANNEL_MASK);
+		if (ret)
+			return ret;
+	} else {
+		ret = regmap_clear_bits(data->regmap, MAX31732_REG_ALARM2_MASK,
+					MAX31732_ALL_CHANNEL_MASK);
+		if (ret)
+			return ret;
+	}
+
+	data->hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
+							       &max31732_chip_info, NULL);
+
+	return PTR_ERR_OR_ZERO(data->hwmon_dev);
+}
+
+static const struct i2c_device_id max31732_ids[] = {
+	{ "max31732" },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(i2c, max31732_ids);
+
+static const struct of_device_id __maybe_unused max31732_of_match[] = {
+	{ .compatible = "adi,max31732", },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, max31732_of_match);
+
+static int __maybe_unused max31732_suspend(struct device *dev)
+{
+	struct max31732_data *data = dev_get_drvdata(dev);
+
+	return regmap_set_bits(data->regmap, MAX31732_REG_CONF1, MAX3173X_STOP);
+}
+
+static int __maybe_unused max31732_resume(struct device *dev)
+{
+	struct max31732_data *data = dev_get_drvdata(dev);
+
+	return regmap_clear_bits(data->regmap, MAX31732_REG_CONF1, MAX3173X_STOP);
+}
+
+static SIMPLE_DEV_PM_OPS(max31732_pm_ops, max31732_suspend, max31732_resume);
+
+static struct i2c_driver max31732_driver = {
+	.class		= I2C_CLASS_HWMON,
+	.driver = {
+		.name	= "max31732-driver",
+		.of_match_table = of_match_ptr(max31732_of_match),
+		.pm	= &max31732_pm_ops,
+	},
+	.probe_new	= max31732_probe,
+	.id_table	= max31732_ids,
+};
+
+module_i2c_driver(max31732_driver);
+
+MODULE_AUTHOR("Sinan Divarci <sinan.divarci@analog.com>");
+MODULE_DESCRIPTION("MAX31732 driver");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("1.0");
-- 
2.25.1

Re: [PATCH v2 1/3] drivers: hwmon: Add max31732 quad remote temperature sensor driver

[Krzysztof Kozlowski]

On 14/12/2022 15:22, Sinan Divarci wrote:
> The MAX31732 is a multi-channel temperature sensor that monitors its own
> temperature and the temperatures of up to four external diodeconnected
> transistors.

Use subject prefixes matching the subsystem (git log --oneline -- ...).
There is no such prefix as "drivers".

I did not review the code, but it is easily visible that it does not
conform to Linux coding style wrapping at 80. Wrap at 80.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] drivers: hwmon: Add max31732 quad remote temperature sensor driver

[Guenter Roeck]

On 12/14/22 09:02, Krzysztof Kozlowski wrote:
> On 14/12/2022 15:22, Sinan Divarci wrote:
>> The MAX31732 is a multi-channel temperature sensor that monitors its own
>> temperature and the temperatures of up to four external diodeconnected
>> transistors.
> 
> Use subject prefixes matching the subsystem (git log --oneline -- ...).
> There is no such prefix as "drivers".
> 
> I did not review the code, but it is easily visible that it does not
> conform to Linux coding style wrapping at 80. Wrap at 80.
> 

I accept line lengths up to 100 to avoid excessive and misaligned
continuation lines. As long as checkpatch --strict doesn't complain,
I won't complain either. checkpatch --strict doesn't complain, and a
brief look into the code tells me that - in terms of line length -
it is fine. Please do not introduce additional continuation lines.

Thanks,
Guenter

Re: [PATCH v2 1/3] drivers: hwmon: Add max31732 quad remote temperature sensor driver

[Krzysztof Kozlowski]

On 14/12/2022 18:24, Guenter Roeck wrote:
> On 12/14/22 09:02, Krzysztof Kozlowski wrote:
>> On 14/12/2022 15:22, Sinan Divarci wrote:
>>> The MAX31732 is a multi-channel temperature sensor that monitors its own
>>> temperature and the temperatures of up to four external diodeconnected
>>> transistors.
>>
>> Use subject prefixes matching the subsystem (git log --oneline -- ...).
>> There is no such prefix as "drivers".
>>
>> I did not review the code, but it is easily visible that it does not
>> conform to Linux coding style wrapping at 80. Wrap at 80.
>>
> 
> I accept line lengths up to 100 to avoid excessive and misaligned
> continuation lines. As long as checkpatch --strict doesn't complain,
> I won't complain either. checkpatch --strict doesn't complain, and a
> brief look into the code tells me that - in terms of line length -
> it is fine. Please do not introduce additional continuation lines.

Sure.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] drivers: hwmon: Add max31732 quad remote temperature sensor driver

[Guenter Roeck]

On Wed, Dec 14, 2022 at 05:22:04PM +0300, Sinan Divarci wrote:
> The MAX31732 is a multi-channel temperature sensor that monitors its own
> temperature and the temperatures of up to four external diodeconnected
> transistors.
> 
> The MAX31732 offers two open-drain, active-low alarm outputs, ALARM1
> and ALARM2. When the measured temperature of a channel crosses the
> respective primary over/under temperature threshold levels ALARM1
> asserts low and a status bit is set in the corresponding thermal status
> registers. When the measured temperature of a channel crosses the
> secondary over/under temperature threshold levels, ALARM2 asserts low
> and a status bit is set in the corresponding thermal status registers.
> 
> Signed-off-by: Sinan Divarci <Sinan.Divarci@analog.com>

There is no public documentation about a MAX31732 chip. Google search
only points to this driver submission. I'll need it to determine if the
chip even exists and to evaluate some implementation details such as
interupt handling.

> ---
>  drivers/hwmon/Kconfig    |  11 +
>  drivers/hwmon/Makefile   |   1 +
>  drivers/hwmon/max31732.c | 620 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 632 insertions(+)
>  create mode 100644 drivers/hwmon/max31732.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 3176c33af..f498f3867 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1076,6 +1076,17 @@ config SENSORS_MAX31730
>  	  This driver can also be built as a module. If so, the module
>  	  will be called max31730.
>  
> +config SENSORS_MAX31732
> +	tristate "MAX31732 temperature sensor"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  Support for the Analog Devices MAX31732 4-Channel Remote
> +	  Temperature Sensor.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called max31732.
> +
>  config SENSORS_MAX31760
>  	tristate "MAX31760 fan speed controller"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e2e4e87b2..6b2871cc5 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -140,6 +140,7 @@ obj-$(CONFIG_SENSORS_MAX1668)	+= max1668.o
>  obj-$(CONFIG_SENSORS_MAX197)	+= max197.o
>  obj-$(CONFIG_SENSORS_MAX31722)	+= max31722.o
>  obj-$(CONFIG_SENSORS_MAX31730)	+= max31730.o
> +obj-$(CONFIG_SENSORS_MAX31732)	+= max31732.o
>  obj-$(CONFIG_SENSORS_MAX31760)  += max31760.o
>  obj-$(CONFIG_SENSORS_MAX6620)	+= max6620.o
>  obj-$(CONFIG_SENSORS_MAX6621)	+= max6621.o
> diff --git a/drivers/hwmon/max31732.c b/drivers/hwmon/max31732.c
> new file mode 100644
> index 000000000..cf075c990
> --- /dev/null
> +++ b/drivers/hwmon/max31732.c
> @@ -0,0 +1,620 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Driver for MAX31732 4-Channel Remote Temperature Sensor
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/regmap.h>
> +
> +/* common definitions*/
> +#define MAX3173X_STOP			BIT(7)
> +#define MAX3173X_ALARM_MODE		BIT(4)
> +#define MAX3173X_ALARM_FAULT_QUEUE_MASK	GENMASK(3, 2)
> +#define MAX3173X_EXTRANGE		BIT(1)
> +#define MAX3173X_TEMP_OFFSET_BASELINE	0x77
> +#define MAX3173X_TEMP_MIN		(-128000)
> +#define MAX3173X_TEMP_MAX		127937
> +#define MAX3173X_OFFSET_MIN		(-14875)
> +#define MAX3173X_OFFSET_MAX		17000
> +#define MAX3173X_OFFSET_ZERO		14875
> +#define MAX31732_SECOND_TEMP_MIN	(-128000)
> +#define MAX31732_SECOND_TEMP_MAX	127000
> +#define MAX31732_CUSTOM_OFFSET_RES	125
> +#define MAX31732_ALL_CHANNEL_MASK	0x1F
> +#define MAX31732_ALARM_INT_MODE		0
> +#define MAX31732_ALARM_COMP_MODE	1
> +#define MAX31732_ALARM_FAULT_QUE	1
> +#define MAX31732_ALARM_FAULT_QUE_MAX	3
> +
> +/* The MAX31732 registers */
> +#define MAX31732_REG_TEMP_R		0x02
> +#define MAX31732_REG_TEMP_L		0x0A
> +#define MAX31732_REG_PRIM_HIGH_STATUS	0x0C
> +#define MAX31732_REG_PRIM_LOW_STATUS	0x0D
> +#define MAX31732_REG_CHANNEL_ENABLE	0x0E
> +#define MAX31732_REG_CONF1		0x0F
> +#define MAX31732_REG_CONF2		0x10
> +#define MAX31732_REG_TEMP_OFFSET	0x16
> +#define MAX31732_REG_OFFSET_ENABLE	0x17
> +#define MAX31732_REG_ALARM1_MASK	0x1B
> +#define MAX31732_REG_ALARM2_MASK	0x1C
> +#define MAX31732_REG_PRIM_HIGH_TEMP_R	0x1D
> +#define MAX31732_REG_PRIM_HIGH_TEMP_L	0x25
> +#define MAX31732_REG_PRIM_LOW_TEMP	0x27
> +#define MAX31732_REG_SECOND_HIGH_TEMP_R	0x29
> +#define MAX31732_REG_SECOND_HIGH_TEMP_L	0x2D
> +#define MAX31732_REG_SECOND_LOW_TEMP	0x2E
> +#define MAX31732_REG_SECOND_HIGH_STATUS	0x42
> +#define MAX31732_REG_SECOND_LOW_STATUS	0x43
> +#define MAX31732_REG_TEMP_FAULT		0x44
> +
> +enum max31732_temp_type {
> +	MAX31732_TEMP,
> +	MAX31732_PRIM_HIGH,
> +	MAX31732_SECOND_HIGH
> +};
> +
> +struct max31732_data {
> +	struct i2c_client	*client;
> +	struct device		*hwmon_dev;
> +	struct regmap		*regmap;
> +	s32			irqs[2];
> +};
> +
> +static u32 max31732_get_temp_reg(enum max31732_temp_type temp_type, u32 channel)
> +{
> +	switch (temp_type) {
> +	case MAX31732_PRIM_HIGH:
> +		if (channel == 0)
> +			return MAX31732_REG_PRIM_HIGH_TEMP_L;
> +		else

else after return is unnecessary (static analyzers will complain).

> +			return (MAX31732_REG_PRIM_HIGH_TEMP_R + (channel - 1) * 2);

Unnecessary outer ()

> +		break;
> +	case MAX31732_SECOND_HIGH:
> +		if (channel == 0)
> +			return MAX31732_REG_SECOND_HIGH_TEMP_L;
> +		else
> +			return (MAX31732_REG_SECOND_HIGH_TEMP_R + (channel - 1));
> +		break;
> +	case MAX31732_TEMP:
> +	default:
> +		if (channel == 0)
> +			return MAX31732_REG_TEMP_L;
> +		else

Unnecessary else after return

> +			return (MAX31732_REG_TEMP_R + (channel - 1) * 2);

Unnecessary outer ()

> +		break;
> +	}
> +}
> +
> +static bool max31732_volatile_reg(struct device *dev, u32 reg)
> +{
> +	if (reg >= MAX31732_REG_TEMP_R && reg <= MAX31732_REG_PRIM_LOW_STATUS)
> +		return true;
> +
> +	if (reg == MAX31732_REG_SECOND_HIGH_STATUS || reg == MAX31732_REG_SECOND_LOW_STATUS)
> +		return true;
> +
> +	if (reg == MAX31732_REG_TEMP_FAULT)
> +		return true;
> +
> +	return false;
> +}
> +
> +static const struct regmap_config regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.cache_type = REGCACHE_RBTREE,
> +	.volatile_reg = max31732_volatile_reg,
> +};
> +
> +static inline long max31732_reg_to_mc(s16 temp)
> +{
> +	return DIV_ROUND_CLOSEST((temp / 16) * 1000, 16);
> +}
> +
> +static int max31732_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, s32 channel,
> +			 long *val)
> +{
> +	struct max31732_data *data = dev_get_drvdata(dev);
> +	s32 ret;
> +	u32 reg_val, reg_addr;
> +	s16 temp_reg_val;
> +	u8 regs[2];
> +
> +	if (type != hwmon_temp)
> +		return -EINVAL;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		ret = regmap_test_bits(data->regmap, MAX31732_REG_CHANNEL_ENABLE, BIT(channel));
> +		if (ret < 0)
> +			return ret;
> +
> +		if (!ret)
> +			return -ENODATA;
> +
> +		reg_addr = max31732_get_temp_reg(MAX31732_TEMP, channel);
> +		break;
> +	case hwmon_temp_max:
> +		reg_addr = max31732_get_temp_reg(MAX31732_PRIM_HIGH, channel);
> +		break;
> +	case hwmon_temp_min:
> +		reg_addr = MAX31732_REG_PRIM_LOW_TEMP;
> +		break;
> +	case hwmon_temp_lcrit:
> +		ret = regmap_read(data->regmap, MAX31732_REG_SECOND_LOW_TEMP, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		*val = reg_val * 1000;
> +		return 0;
> +	case hwmon_temp_crit:
> +		reg_addr = max31732_get_temp_reg(MAX31732_SECOND_HIGH, channel);
> +		ret = regmap_read(data->regmap, reg_addr, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		*val = reg_val * 1000;
> +		return 0;
> +	case hwmon_temp_enable:
> +		ret = regmap_test_bits(data->regmap, MAX31732_REG_CHANNEL_ENABLE, BIT(channel));
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret;
> +		return 0;
> +	case hwmon_temp_offset:
> +		if (channel == 0)
> +			return -EINVAL;
> +
> +		ret = regmap_test_bits(data->regmap, MAX31732_REG_OFFSET_ENABLE, BIT(channel));
> +		if (ret < 0)
> +			return ret;
> +
> +		if (!ret)
> +			return 0;
> +
> +		ret = regmap_read(data->regmap, MAX31732_REG_TEMP_OFFSET, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		*val = (reg_val - MAX3173X_TEMP_OFFSET_BASELINE) * MAX31732_CUSTOM_OFFSET_RES;
> +		return 0;
> +	case hwmon_temp_fault:
> +		ret = regmap_test_bits(data->regmap, MAX31732_REG_TEMP_FAULT, BIT(channel));
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret;
> +		return 0;
> +	case hwmon_temp_lcrit_alarm:
> +		ret = regmap_test_bits(data->regmap, MAX31732_REG_SECOND_LOW_STATUS, BIT(channel));
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret;
> +		return 0;
> +	case hwmon_temp_min_alarm:
> +		ret = regmap_test_bits(data->regmap, MAX31732_REG_PRIM_LOW_STATUS, BIT(channel));
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret;
> +		return 0;
> +	case hwmon_temp_max_alarm:
> +		ret = regmap_test_bits(data->regmap, MAX31732_REG_PRIM_HIGH_STATUS, BIT(channel));
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret;
> +		return 0;
> +	case hwmon_temp_crit_alarm:
> +		ret = regmap_test_bits(data->regmap, MAX31732_REG_SECOND_HIGH_STATUS, BIT(channel));
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_bulk_read(data->regmap, reg_addr, &regs, 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	temp_reg_val = regs[1] | regs[0] << 8;
> +	*val = max31732_reg_to_mc(temp_reg_val);
> +	return 0;
> +}
> +
> +static int max31732_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, s32 channel,
> +			  long val)
> +{
> +	struct max31732_data *data = dev_get_drvdata(dev);
> +	s32 reg_addr, ret;
> +	u16 temp_reg_val;
> +
> +	if (type != hwmon_temp)
> +		return -EINVAL;
> +
> +	switch (attr) {
> +	case hwmon_temp_max:
> +		reg_addr = max31732_get_temp_reg(MAX31732_PRIM_HIGH, channel);
> +		break;
> +	case hwmon_temp_min:
> +		reg_addr = MAX31732_REG_PRIM_LOW_TEMP;
> +		break;
> +	case hwmon_temp_enable:
> +		if (val == 0) {
> +			return regmap_clear_bits(data->regmap, MAX31732_REG_CHANNEL_ENABLE,
> +						 BIT(channel));
> +		} else if (val == 1) {
> +			return regmap_set_bits(data->regmap, MAX31732_REG_CHANNEL_ENABLE,
> +					       BIT(channel));
> +		} else {

else after return is unnecessary.

> +			return -EINVAL;
> +		}
> +	case hwmon_temp_offset:
> +		val = clamp_val(val, MAX3173X_OFFSET_MIN, MAX3173X_OFFSET_MAX) +
> +				MAX3173X_OFFSET_ZERO;
> +		val = DIV_ROUND_CLOSEST(val, 125);
> +
> +		if (val == MAX3173X_TEMP_OFFSET_BASELINE) {
> +			ret = regmap_clear_bits(data->regmap, MAX31732_REG_OFFSET_ENABLE,
> +						BIT(channel));
> +		} else {
> +			ret = regmap_set_bits(data->regmap, MAX31732_REG_OFFSET_ENABLE,
> +					      BIT(channel));
> +		}
> +		if (ret)
> +			return ret;
> +
> +		return regmap_write(data->regmap, MAX31732_REG_TEMP_OFFSET, val);
> +	case hwmon_temp_crit:
> +		val = clamp_val(val, MAX31732_SECOND_TEMP_MIN, MAX31732_SECOND_TEMP_MAX);
> +		val = DIV_ROUND_CLOSEST(val, 1000);
> +		reg_addr = max31732_get_temp_reg(MAX31732_SECOND_HIGH, channel);
> +		return regmap_write(data->regmap, reg_addr, val);
> +	case hwmon_temp_lcrit:
> +		val = clamp_val(val, MAX31732_SECOND_TEMP_MIN, MAX31732_SECOND_TEMP_MAX);
> +		val = DIV_ROUND_CLOSEST(val, 1000);
> +		return regmap_write(data->regmap, MAX31732_REG_SECOND_LOW_TEMP, val);
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	val = clamp_val(val, MAX3173X_TEMP_MIN, MAX3173X_TEMP_MAX);
> +	val = DIV_ROUND_CLOSEST(val << 4, 1000) << 4;
> +
> +	temp_reg_val = (u16)val;
> +	temp_reg_val = swab16(temp_reg_val);

Why not just
	temp_reg_val = swab16(val);
?

> +
> +	return regmap_bulk_write(data->regmap, reg_addr, &temp_reg_val, sizeof(temp_reg_val));
> +}
> +
> +static umode_t max31732_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr,
> +				   s32 channel)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +		case hwmon_temp_lcrit_alarm:
> +		case hwmon_temp_min_alarm:
> +		case hwmon_temp_max_alarm:
> +		case hwmon_temp_crit_alarm:
> +		case hwmon_temp_fault:
> +			return 0444;
> +		case hwmon_temp_min:
> +		case hwmon_temp_lcrit:
> +			return channel ? 0444 : 0644;
> +		case hwmon_temp_offset:
> +		case hwmon_temp_enable:
> +		case hwmon_temp_max:
> +		case hwmon_temp_crit:
> +			return 0644;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static irqreturn_t max31732_irq_handler(s32 irq, void *data)
> +{
> +	struct device *dev = data;
> +	struct max31732_data *drvdata = dev_get_drvdata(dev);
> +	s32 ret;
> +	u32 reg_val;
> +	bool reported = false;
> +
> +	ret = regmap_read(drvdata->regmap, MAX31732_REG_PRIM_HIGH_STATUS, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	if (reg_val != 0) {
> +		dev_crit(dev, "Primary Overtemperature Alarm, R4:%d R3:%d R2:%d R1:%d L:%d.\n",
> +			 !!(reg_val & BIT(4)), !!(reg_val & BIT(3)), !!(reg_val & BIT(2)),
> +			 !!(reg_val & BIT(1)), !!(reg_val & BIT(0)));

dev_crit seems excessive here.

> +		hwmon_notify_event(drvdata->hwmon_dev, hwmon_temp, hwmon_temp_max_alarm, 0);
> +		reported = true;
> +	}
> +
> +	ret = regmap_read(drvdata->regmap, MAX31732_REG_PRIM_LOW_STATUS, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	if (reg_val != 0) {
> +		dev_crit(dev, "Primary Undertemperature Alarm, R4:%d R3:%d R2:%d R1:%d L:%d.\n",
> +			 !!(reg_val & BIT(4)), !!(reg_val & BIT(3)), !!(reg_val & BIT(2)),
> +			 !!(reg_val & BIT(1)), !!(reg_val & BIT(0)));

dev_crit seems excessive here.

> +		hwmon_notify_event(drvdata->hwmon_dev, hwmon_temp, hwmon_temp_min_alarm, 0);
> +		reported = true;
> +	}
> +
> +	ret = regmap_read(drvdata->regmap, MAX31732_REG_SECOND_HIGH_STATUS, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	if (reg_val != 0) {
> +		dev_crit(dev, "Secondary Overtemperature Alarm, R4:%d R3:%d R2:%d R1:%d L:%d.\n",
> +			 !!(reg_val & BIT(4)), !!(reg_val & BIT(3)), !!(reg_val & BIT(2)),
> +			 !!(reg_val & BIT(1)), !!(reg_val & BIT(0)));
> +		hwmon_notify_event(drvdata->hwmon_dev, hwmon_temp, hwmon_temp_crit_alarm, 0);

Why always report events against channel 0 ?

> +		reported = true;
> +	}
> +
> +	ret = regmap_read(drvdata->regmap, MAX31732_REG_SECOND_LOW_STATUS, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	if (reg_val != 0) {
> +		dev_crit(dev, "Secondary Undertemperature Alarm, R4:%d R3:%d R2:%d R1:%d L:%d.\n",
> +			 !!(reg_val & BIT(4)), !!(reg_val & BIT(3)), !!(reg_val & BIT(2)),
> +			 !!(reg_val & BIT(1)), !!(reg_val & BIT(0)));
> +		hwmon_notify_event(drvdata->hwmon_dev, hwmon_temp, hwmon_temp_lcrit_alarm, 0);
> +		reported = true;
> +	}
> +
> +	if (!reported) {
> +		if (irq == drvdata->irqs[0])
> +			dev_err(dev, "ALARM1 interrupt received but status registers not set.\n");
> +		else if (irq == drvdata->irqs[1])
> +			dev_err(dev, "ALARM2 interrupt received but status registers not set.\n");
> +		else
> +			dev_err(dev, "Undefined interrupt source.\n");
> +	}

I am a bit concerned with the volume of alarm log messages. What happens
if the alarm is persistent ? Will there be just one or many reports ?

The second and third else statements are unnecessary. There are two
interrupts, and the reported interrupt will be one of those.
In fact, I wonder if it even makes sense to have a single interrupt
handler for both interrupts; that seems to defeat the purpose
of having separate interrupts. I'll need to see the datasheet to
help me determine if and how this makes sense.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct hwmon_channel_info *max31732_info[] = {
> +	HWMON_CHANNEL_INFO(chip,
> +			   HWMON_C_REGISTER_TZ),
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_LCRIT |
> +			   HWMON_T_CRIT |
> +			   HWMON_T_ENABLE |
> +			   HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
> +			   HWMON_T_LCRIT_ALARM,
> +			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_LCRIT |
> +			   HWMON_T_CRIT |
> +			   HWMON_T_OFFSET | HWMON_T_ENABLE |
> +			   HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
> +			   HWMON_T_LCRIT_ALARM |
> +			   HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_LCRIT |
> +			   HWMON_T_CRIT |
> +			   HWMON_T_OFFSET | HWMON_T_ENABLE |
> +			   HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
> +			   HWMON_T_LCRIT_ALARM |
> +			   HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_LCRIT |
> +			   HWMON_T_CRIT |
> +			   HWMON_T_OFFSET | HWMON_T_ENABLE |
> +			   HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
> +			   HWMON_T_LCRIT_ALARM |
> +			   HWMON_T_FAULT,
> +			   HWMON_T_INPUT | HWMON_T_MIN | HWMON_T_MAX | HWMON_T_LCRIT |
> +			   HWMON_T_CRIT |
> +			   HWMON_T_OFFSET | HWMON_T_ENABLE |
> +			   HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM | HWMON_T_CRIT_ALARM |
> +			   HWMON_T_LCRIT_ALARM |
> +			   HWMON_T_FAULT
> +			   ),
> +	NULL
> +};
> +
> +static const struct hwmon_ops max31732_hwmon_ops = {
> +	.is_visible = max31732_is_visible,
> +	.read = max31732_read,
> +	.write = max31732_write,
> +};
> +
> +static const struct hwmon_chip_info max31732_chip_info = {
> +	.ops = &max31732_hwmon_ops,
> +	.info = max31732_info,
> +};
> +
> +static int max31732_parse_alarms(struct device *dev, struct max31732_data *data)
> +{
> +	s32 ret;
> +	u32 alarm_que;
> +
> +	if (fwnode_property_read_bool(dev_fwnode(dev), "adi,alarm1-interrupt-mode"))
> +		ret = regmap_clear_bits(data->regmap, MAX31732_REG_CONF1, MAX3173X_ALARM_MODE);
> +	else
> +		ret = regmap_set_bits(data->regmap, MAX31732_REG_CONF1, MAX3173X_ALARM_MODE);

What is the impact of this ? It is not normally configurable. One would expect
one interrupt when an alarm is raised, and another interrupt when the alarm
condition no longer exists. I have no access to the datasheet, so I can not
determine if and how it might make sense to have this configurable, and what
the impact might be.

> +
> +	if (ret)
> +		return ret;
> +
> +	if (fwnode_property_read_bool(dev_fwnode(dev), "adi,alarm2-interrupt-mode"))
> +		ret = regmap_clear_bits(data->regmap, MAX31732_REG_CONF2, MAX3173X_ALARM_MODE);
> +	else
> +		ret = regmap_set_bits(data->regmap, MAX31732_REG_CONF2, MAX3173X_ALARM_MODE);
> +
> +	if (ret)
> +		return ret;
> +
> +	alarm_que = MAX31732_ALARM_FAULT_QUE;
> +	fwnode_property_read_u32(dev_fwnode(dev), "adi,alarm1-fault-queue", &alarm_que);
> +
> +	if ((alarm_que / 2) <= MAX31732_ALARM_FAULT_QUE_MAX) {

Norm is to check for errors first.

> +		ret = regmap_write_bits(data->regmap, MAX31732_REG_CONF1,
> +					MAX3173X_ALARM_FAULT_QUEUE_MASK,
> +					FIELD_PREP(MAX3173X_ALARM_FAULT_QUEUE_MASK,
> +						   (alarm_que / 2)));
> +		if (ret)
> +			return ret;
> +	} else {
> +		return dev_err_probe(dev, -EINVAL, "Invalid adi,alarm1-fault-queue.\n");
> +	}
> +
> +	alarm_que = MAX31732_ALARM_FAULT_QUE;
> +	fwnode_property_read_u32(dev_fwnode(dev), "adi,alarm2-fault-queue", &alarm_que);
> +
> +	if ((alarm_que / 2) <= MAX31732_ALARM_FAULT_QUE_MAX) {

Unnecessary ( )

This accepts values of 3 and 5 which are invalid according to the
devicetree file. The code should check for that.


> +		ret = regmap_write_bits(data->regmap, MAX31732_REG_CONF2,
> +					MAX3173X_ALARM_FAULT_QUEUE_MASK,
> +					FIELD_PREP(MAX3173X_ALARM_FAULT_QUEUE_MASK,
> +						   (alarm_que / 2)));
> +	} else {
> +		return dev_err_probe(dev, -EINVAL, "Invalid adi,alarm2-fault-queue.\n");
> +	}
> +
> +	return ret;
> +}
> +
> +static int max31732_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct max31732_data *data;
> +	s32 ret;
> +	u32 reg_val;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;

I don't see this used anywhere.

> +
> +	data->regmap = devm_regmap_init_i2c(client, &regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap), "regmap init failed\n");
> +
> +	ret = regmap_read(data->regmap, MAX31732_REG_CHANNEL_ENABLE, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	if (reg_val == 0)
> +		ret = regmap_set_bits(data->regmap, MAX31732_REG_CONF1, MAX3173X_STOP);
> +	else
> +		ret = regmap_clear_bits(data->regmap, MAX31732_REG_CONF1, MAX3173X_STOP);
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_clear_bits(data->regmap, MAX31732_REG_CONF1, MAX3173X_EXTRANGE);
> +	if (ret)
> +		return ret;
> +
> +	ret = max31732_parse_alarms(dev, data);
> +	if (ret)
> +		return ret;
> +
> +	dev_set_drvdata(dev, data);
> +
> +	data->irqs[0] = fwnode_irq_get_byname(dev_fwnode(dev), "ALARM1");
> +	if (data->irqs[0] > 0) {
> +		ret = devm_request_threaded_irq(dev, data->irqs[0], NULL, max31732_irq_handler,
> +						IRQF_ONESHOT, client->name, dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "cannot request irq\n");
> +
> +		ret = regmap_set_bits(data->regmap, MAX31732_REG_ALARM1_MASK,
> +				      MAX31732_ALL_CHANNEL_MASK);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = regmap_clear_bits(data->regmap, MAX31732_REG_ALARM1_MASK,
> +					MAX31732_ALL_CHANNEL_MASK);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	data->irqs[1] = fwnode_irq_get_byname(dev_fwnode(dev), "ALARM2");
> +	if (data->irqs[1] > 0) {
> +		ret = devm_request_threaded_irq(dev, data->irqs[1], NULL, max31732_irq_handler,
> +						IRQF_ONESHOT, client->name, dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "cannot request irq\n");
> +
> +		ret = regmap_set_bits(data->regmap, MAX31732_REG_ALARM2_MASK,
> +				      MAX31732_ALL_CHANNEL_MASK);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = regmap_clear_bits(data->regmap, MAX31732_REG_ALARM2_MASK,
> +					MAX31732_ALL_CHANNEL_MASK);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	data->hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
> +							       &max31732_chip_info, NULL);
> +
> +	return PTR_ERR_OR_ZERO(data->hwmon_dev);
> +}
> +
> +static const struct i2c_device_id max31732_ids[] = {
> +	{ "max31732" },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, max31732_ids);
> +
> +static const struct of_device_id __maybe_unused max31732_of_match[] = {
> +	{ .compatible = "adi,max31732", },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, max31732_of_match);
> +
> +static int __maybe_unused max31732_suspend(struct device *dev)
> +{
> +	struct max31732_data *data = dev_get_drvdata(dev);
> +
> +	return regmap_set_bits(data->regmap, MAX31732_REG_CONF1, MAX3173X_STOP);
> +}
> +
> +static int __maybe_unused max31732_resume(struct device *dev)
> +{
> +	struct max31732_data *data = dev_get_drvdata(dev);
> +
> +	return regmap_clear_bits(data->regmap, MAX31732_REG_CONF1, MAX3173X_STOP);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(max31732_pm_ops, max31732_suspend, max31732_resume);
> +
> +static struct i2c_driver max31732_driver = {
> +	.class		= I2C_CLASS_HWMON,
> +	.driver = {
> +		.name	= "max31732-driver",
> +		.of_match_table = of_match_ptr(max31732_of_match),
> +		.pm	= &max31732_pm_ops,
> +	},
> +	.probe_new	= max31732_probe,
> +	.id_table	= max31732_ids,
> +};
> +
> +module_i2c_driver(max31732_driver);
> +
> +MODULE_AUTHOR("Sinan Divarci <sinan.divarci@analog.com>");
> +MODULE_DESCRIPTION("MAX31732 driver");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION("1.0");

[PATCH v2 2/3] docs: hwmon: add max31732 documentation

[Sinan Divarci]

Adding documentation for max31732 quad remote temperature sensor

Signed-off-by: Sinan Divarci <Sinan.Divarci@analog.com>
---
 Documentation/hwmon/index.rst    |  1 +
 Documentation/hwmon/max31732.rst | 62 ++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)
 create mode 100644 Documentation/hwmon/max31732.rst

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index fe2cc6b73..e521bf555 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -133,6 +133,7 @@ Hardware Monitoring Kernel Drivers
    max20751
    max31722
    max31730
+   max31732
    max31760
    max31785
    max31790
diff --git a/Documentation/hwmon/max31732.rst b/Documentation/hwmon/max31732.rst
new file mode 100644
index 000000000..67bfcf393
--- /dev/null
+++ b/Documentation/hwmon/max31732.rst
@@ -0,0 +1,62 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver max31732
+======================
+
+Supported chips:
+
+  * Analog Devices MAX31732
+
+    Prefix: 'max31732'
+
+    Addresses scanned: none
+
+Author: Sinan Divarci <Sinan.Divarci@analog.com>
+
+
+Description
+-----------
+
+This driver implements support for Maxim MAX31732.
+
+The MAX31732 is a multi-channel temperature sensor that monitors its
+own temperature and the temperatures of up to four external diodeconnected
+transistors. The device operates with 3.0V to 3.6V supply range
+and consume TBDμA of current in standby mode of operation. Resistance
+cancellation feature compensates for high series resistance between
+circuit-board traces and the external thermal diode, while beta
+compensation corrects for temperature-measurement errors due to lowbeta
+sensing transistors.
+
+The MAX31732 offers two open-drain, active-low alarm outputs,
+ALARM1 and ALARM2. When the measured temperature of a channel
+crosses the respective primary over/under temperature threshold levels
+ALARM1 asserts low and a status bit is set in the corresponding thermal
+status registers. When the measured temperature of a channel crosses the
+secondary over/under temperature threshold levels, ALARM2 asserts low
+and a status bit is set in the corresponding thermal status registers.
+
+Temperature measurement range: from -64°C to 150°C
+
+Temperature Resolution: 12 Bits, ±0.0625°C
+
+Sysfs entries
+-------------
+
+===================== == =======================================================
+temp[1-5]_enable      RW Temperature enable/disable
+                         Set to 1 to enable channel, 0 to disable
+temp[1-5]_input       RO Temperature input
+temp[2-5]_fault       RO Fault indicator for remote channels
+temp[1-5]_max         RW Temperature max value. Asserts "ALARM1" pin when exceeded
+temp[1-5]_max_alarm   RO Temperature max alarm status
+temp[1-5]_crit        RW Temperature critical value. Asserts "ALARM2" pin when exceeded
+temp[1-5]_crit_alarm  RO Temperature critical alarm status
+temp[1-5]_min         RW Temperature min value. Common for all channels.
+                         Only temp1_min is writeable. Asserts "ALARM1" pin when exceeded
+temp[1-5]_min_alarm   RO Temperature min alarm status
+temp[1-5]_lcrit       RW Temperature critical low value. Common for all channels.
+                         Only temp1_min is writeable. Asserts "ALARM2" pin when exceeded
+temp[1-5]_lcrit_alarm RO Temperature critical low alarm status
+temp[2-5]_offset      RW Temperature offset for remote channels
+===================== == =======================================================
-- 
2.25.1

Re: [PATCH v2 2/3] docs: hwmon: add max31732 documentation

[Guenter Roeck]

On Wed, Dec 14, 2022 at 05:22:05PM +0300, Sinan Divarci wrote:
> Adding documentation for max31732 quad remote temperature sensor
> 
> Signed-off-by: Sinan Divarci <Sinan.Divarci@analog.com>
> ---
>  Documentation/hwmon/index.rst    |  1 +
>  Documentation/hwmon/max31732.rst | 62 ++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+)
>  create mode 100644 Documentation/hwmon/max31732.rst
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index fe2cc6b73..e521bf555 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -133,6 +133,7 @@ Hardware Monitoring Kernel Drivers
>     max20751
>     max31722
>     max31730
> +   max31732
>     max31760
>     max31785
>     max31790
> diff --git a/Documentation/hwmon/max31732.rst b/Documentation/hwmon/max31732.rst
> new file mode 100644
> index 000000000..67bfcf393
> --- /dev/null
> +++ b/Documentation/hwmon/max31732.rst
> @@ -0,0 +1,62 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver max31732
> +======================
> +
> +Supported chips:
> +
> +  * Analog Devices MAX31732
> +
> +    Prefix: 'max31732'
> +
> +    Addresses scanned: none
> +
> +Author: Sinan Divarci <Sinan.Divarci@analog.com>
> +
> +
> +Description
> +-----------
> +
> +This driver implements support for Maxim MAX31732.
> +
> +The MAX31732 is a multi-channel temperature sensor that monitors its
> +own temperature and the temperatures of up to four external diodeconnected
> +transistors. The device operates with 3.0V to 3.6V supply range
> +and consume TBDμA of current in standby mode of operation. Resistance

TBDμA ?

> +cancellation feature compensates for high series resistance between
> +circuit-board traces and the external thermal diode, while beta
> +compensation corrects for temperature-measurement errors due to lowbeta
> +sensing transistors.
> +
> +The MAX31732 offers two open-drain, active-low alarm outputs,
> +ALARM1 and ALARM2. When the measured temperature of a channel
> +crosses the respective primary over/under temperature threshold levels
> +ALARM1 asserts low and a status bit is set in the corresponding thermal
> +status registers. When the measured temperature of a channel crosses the
> +secondary over/under temperature threshold levels, ALARM2 asserts low
> +and a status bit is set in the corresponding thermal status registers.
> +
> +Temperature measurement range: from -64°C to 150°C
> +
> +Temperature Resolution: 12 Bits, ±0.0625°C
> +
> +Sysfs entries
> +-------------
> +
> +===================== == =======================================================
> +temp[1-5]_enable      RW Temperature enable/disable
> +                         Set to 1 to enable channel, 0 to disable
> +temp[1-5]_input       RO Temperature input
> +temp[2-5]_fault       RO Fault indicator for remote channels
> +temp[1-5]_max         RW Temperature max value. Asserts "ALARM1" pin when exceeded
> +temp[1-5]_max_alarm   RO Temperature max alarm status
> +temp[1-5]_crit        RW Temperature critical value. Asserts "ALARM2" pin when exceeded
> +temp[1-5]_crit_alarm  RO Temperature critical alarm status
> +temp[1-5]_min         RW Temperature min value. Common for all channels.
> +                         Only temp1_min is writeable. Asserts "ALARM1" pin when exceeded
> +temp[1-5]_min_alarm   RO Temperature min alarm status
> +temp[1-5]_lcrit       RW Temperature critical low value. Common for all channels.
> +                         Only temp1_min is writeable. Asserts "ALARM2" pin when exceeded
> +temp[1-5]_lcrit_alarm RO Temperature critical low alarm status
> +temp[2-5]_offset      RW Temperature offset for remote channels
> +===================== == =======================================================

[PATCH v2 3/3] dt-bindings: hwmon: Add bindings for max31732

[Sinan Divarci]

Adding bindings for max31732 quad remote temperature sensor

Signed-off-by: Sinan Divarci <Sinan.Divarci@analog.com>
---
 .../bindings/hwmon/adi,max31732.yaml          | 83 +++++++++++++++++++
 1 file changed, 83 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/adi,max31732.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/adi,max31732.yaml b/Documentation/devicetree/bindings/hwmon/adi,max31732.yaml
new file mode 100644
index 000000000..c701cda95
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/adi,max31732.yaml
@@ -0,0 +1,83 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2022 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/adi,max31732.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices MAX31732 Temperature Sensor Device Driver
+
+maintainers:
+  - Sinan Divarci <Sinan.Divarci@analog.com>
+
+description: Bindings for the Analog Devices MAX31732 Temperature Sensor Device.
+
+properties:
+  compatible:
+    enum:
+      - adi,max31732
+
+  reg:
+    description: I2C address of the Temperature Sensor Device.
+    maxItems: 1
+
+  interrupts:
+    minItems: 1
+    maxItems: 2
+
+  interrupt-names:
+    description: Name of the interrupt pin of max31732 used for IRQ.
+    minItems: 1
+    items:
+      - enum: [ALARM1, ALARM2]
+      - enum: [ALARM1, ALARM2]
+
+  adi,alarm1-interrupt-mode:
+    description: |
+      Enables the ALARM1 output to function in interrupt mode.
+      Default ALARM1 output function is comparator mode.
+    type: boolean
+
+  adi,alarm2-interrupt-mode:
+    description: |
+      Enables the ALARM2 output to function in interrupt mode.
+      Default ALARM2 output function is comparator mode.
+    type: boolean
+
+  adi,alarm1-fault-queue:
+    description: The number of consecutive faults required to assert ALARM1.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 2, 4, 6]
+    default: 1
+
+  adi,alarm2-fault-queue:
+    description: The number of consecutive faults required to assert ALARM2.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [1, 2, 4, 6]
+    default: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        sensor@1c {
+            compatible = "adi,max31732";
+            reg = <0x1c>;
+            interrupt-parent = <&gpio>;
+            interrupts = <17 IRQ_TYPE_EDGE_BOTH>, <27 IRQ_TYPE_EDGE_BOTH>;
+            interrupt-names = "ALARM1", "ALARM2";
+            adi,alarm1-fault-queue = <4>;
+            adi,alarm2-fault-queue = <2>;
+            adi,alarm2-interrupt-mode;
+      };
+    };
-- 
2.25.1

Re: [PATCH v2 3/3] dt-bindings: hwmon: Add bindings for max31732

[Krzysztof Kozlowski]

On 14/12/2022 15:22, Sinan Divarci wrote:
> Adding bindings for max31732 quad remote temperature sensor

Full stop.

Subject: drop second, redundant "bindings for".

> 
> Signed-off-by: Sinan Divarci <Sinan.Divarci@analog.com>
> ---
>  .../bindings/hwmon/adi,max31732.yaml          | 83 +++++++++++++++++++
>  1 file changed, 83 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/adi,max31732.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/adi,max31732.yaml b/Documentation/devicetree/bindings/hwmon/adi,max31732.yaml
> new file mode 100644
> index 000000000..c701cda95
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/adi,max31732.yaml
> @@ -0,0 +1,83 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2022 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/adi,max31732.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices MAX31732 Temperature Sensor Device Driver

Drop "Device Driver"

> +
> +maintainers:
> +  - Sinan Divarci <Sinan.Divarci@analog.com>
> +
> +description: Bindings for the Analog Devices MAX31732 Temperature Sensor Device.

Drop "Bindings for". Actually, either drop entire description or write
something else than title.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,max31732
> +
> +  reg:
> +    description: I2C address of the Temperature Sensor Device.

Drop description.

> +    maxItems: 1
> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 2
> +
> +  interrupt-names:
> +    description: Name of the interrupt pin of max31732 used for IRQ.

Drop description.

> +    minItems: 1
> +    items:
> +      - enum: [ALARM1, ALARM2]
> +      - enum: [ALARM1, ALARM2]

This should be fixed, not flexible. Why it's flexible?

lowercase letters only

> +
> +  adi,alarm1-interrupt-mode:
> +    description: |
> +      Enables the ALARM1 output to function in interrupt mode.
> +      Default ALARM1 output function is comparator mode.

Why this is a property of DT/hardware? Don't encode policy in DT.

> +    type: boolean
> +
> +  adi,alarm2-interrupt-mode:
> +    description: |
> +      Enables the ALARM2 output to function in interrupt mode.
> +      Default ALARM2 output function is comparator mode.

Same question.

> +    type: boolean
> +
> +  adi,alarm1-fault-queue:
> +    description: The number of consecutive faults required to assert ALARM1.

Same question - why this number differs with hardware?

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 4, 6]
> +    default: 1
> +
> +  adi,alarm2-fault-queue:
> +    description: The number of consecutive faults required to assert ALARM2.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [1, 2, 4, 6]
> +    default: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        sensor@1c {
> +            compatible = "adi,max31732";
> +            reg = <0x1c>;
> +            interrupt-parent = <&gpio>;
> +            interrupts = <17 IRQ_TYPE_EDGE_BOTH>, <27 IRQ_TYPE_EDGE_BOTH>;
> +            interrupt-names = "ALARM1", "ALARM2";
> +            adi,alarm1-fault-queue = <4>;
> +            adi,alarm2-fault-queue = <2>;
> +            adi,alarm2-interrupt-mode;
> +      };

Messed indentation.


> +    };

Best regards,
Krzysztof

Re: [PATCH v2 3/3] dt-bindings: hwmon: Add bindings for max31732

[Guenter Roeck]

On Wed, Dec 14, 2022 at 06:00:03PM +0100, Krzysztof Kozlowski wrote:
> On 14/12/2022 15:22, Sinan Divarci wrote:
> > Adding bindings for max31732 quad remote temperature sensor
> 
> Full stop.
> 
> Subject: drop second, redundant "bindings for".
> 
> > 
> > Signed-off-by: Sinan Divarci <Sinan.Divarci@analog.com>
> > ---
> >  .../bindings/hwmon/adi,max31732.yaml          | 83 +++++++++++++++++++
> >  1 file changed, 83 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/hwmon/adi,max31732.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/adi,max31732.yaml b/Documentation/devicetree/bindings/hwmon/adi,max31732.yaml
> > new file mode 100644
> > index 000000000..c701cda95
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/adi,max31732.yaml
> > @@ -0,0 +1,83 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +# Copyright 2022 Analog Devices Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwmon/adi,max31732.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices MAX31732 Temperature Sensor Device Driver
> 
> Drop "Device Driver"
> 
> > +
> > +maintainers:
> > +  - Sinan Divarci <Sinan.Divarci@analog.com>
> > +
> > +description: Bindings for the Analog Devices MAX31732 Temperature Sensor Device.
> 
> Drop "Bindings for". Actually, either drop entire description or write
> something else than title.
> 
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,max31732
> > +
> > +  reg:
> > +    description: I2C address of the Temperature Sensor Device.
> 
> Drop description.
> 
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    minItems: 1
> > +    maxItems: 2
> > +
> > +  interrupt-names:
> > +    description: Name of the interrupt pin of max31732 used for IRQ.
> 
> Drop description.
> 
> > +    minItems: 1
> > +    items:
> > +      - enum: [ALARM1, ALARM2]
> > +      - enum: [ALARM1, ALARM2]
> 
> This should be fixed, not flexible. Why it's flexible?
> 
> lowercase letters only
> 
> > +
> > +  adi,alarm1-interrupt-mode:
> > +    description: |
> > +      Enables the ALARM1 output to function in interrupt mode.
> > +      Default ALARM1 output function is comparator mode.
> 
> Why this is a property of DT/hardware? Don't encode policy in DT.
> 

I would not call this "policy". Normally it is an implementation
question or decision, since interrupts behave differently depending
on the mode. Impact is difficult to see, though, since the chip
documentation is not available to the public.

> > +    type: boolean
> > +
> > +  adi,alarm2-interrupt-mode:
> > +    description: |
> > +      Enables the ALARM2 output to function in interrupt mode.
> > +      Default ALARM2 output function is comparator mode.
> 
> Same question.
> 
> > +    type: boolean
> > +
> > +  adi,alarm1-fault-queue:
> > +    description: The number of consecutive faults required to assert ALARM1.
> 
> Same question - why this number differs with hardware?
> 

Noisier hardware will require more samples to avoid spurious faults.
Trade-off is speed of reporting a fault. Normally the board designer
would determine a value which is low enough to avoid spurious faults.

Note that the chip (according to patch 2/3) supports resistance
cancellation as well as beta compensation, which are also board specific.
I don't have access to the datasheet, so I don't know for sure if those
are configurable (typically they are). If they are configurable, I would
expect to see respective properties.

Guenter

Re: [PATCH v2 3/3] dt-bindings: hwmon: Add bindings for max31732

[Krzysztof Kozlowski]

On 29/12/2022 16:52, Guenter Roeck wrote:
>>> +  adi,alarm1-interrupt-mode:
>>> +    description: |
>>> +      Enables the ALARM1 output to function in interrupt mode.
>>> +      Default ALARM1 output function is comparator mode.
>>
>> Why this is a property of DT/hardware? Don't encode policy in DT.
>>
> 
> I would not call this "policy". Normally it is an implementation
> question or decision, since interrupts behave differently depending
> on the mode. Impact is difficult to see, though, since the chip
> documentation is not available to the public.

Some more background would be useful here from the author...

> 
>>> +    type: boolean
>>> +
>>> +  adi,alarm2-interrupt-mode:
>>> +    description: |
>>> +      Enables the ALARM2 output to function in interrupt mode.
>>> +      Default ALARM2 output function is comparator mode.
>>
>> Same question.
>>
>>> +    type: boolean
>>> +
>>> +  adi,alarm1-fault-queue:
>>> +    description: The number of consecutive faults required to assert ALARM1.
>>
>> Same question - why this number differs with hardware?
>>
> 
> Noisier hardware will require more samples to avoid spurious faults.
> Trade-off is speed of reporting a fault. Normally the board designer
> would determine a value which is low enough to avoid spurious faults.
> 
> Note that the chip (according to patch 2/3) supports resistance
> cancellation as well as beta compensation, which are also board specific.
> I don't have access to the datasheet, so I don't know for sure if those
> are configurable (typically they are). If they are configurable, I would
> expect to see respective properties.

If that's the argument behind property, then it's fine.

Best regards,
Krzysztof

Re: [PATCH v2 3/3] dt-bindings: hwmon: Add bindings for max31732

[Guenter Roeck]

On Thu, Dec 29, 2022 at 05:39:30PM +0100, Krzysztof Kozlowski wrote:
> On 29/12/2022 16:52, Guenter Roeck wrote:
> >>> +  adi,alarm1-interrupt-mode:
> >>> +    description: |
> >>> +      Enables the ALARM1 output to function in interrupt mode.
> >>> +      Default ALARM1 output function is comparator mode.
> >>
> >> Why this is a property of DT/hardware? Don't encode policy in DT.
> >>
> > 
> > I would not call this "policy". Normally it is an implementation
> > question or decision, since interrupts behave differently depending
> > on the mode. Impact is difficult to see, though, since the chip
> > documentation is not available to the public.
> 
> Some more background would be useful here from the author...
> 

Indeed. Oddly enough I found the datasheet (as Google employee), but 
it is under NDA which means that I can't say much but "this is wrong"
or "this doesn't work" in some or even all situations. I am not even
sure if I can say that much because saying so implies providing
information about the chip which I am bound to not expose. Sigh.

Anyway, from public information and the driver.

- The chip has one internal and four external channels. Channel
  configuration is per channel (there is an enable flag for each
  channel). This means devicetree configuration must be nested and
  include per-channel information (at the very least information
  if a channel is enabled). Other configuration information, if it exists,
  may also be per channel. That includes both resistance cancellation,
  beta compensation, and possibly other configuration data. One thing
  that came to mind was diode linearity factor. Examples for existing
  properties in other drivers:
	adi,ideal-factor-value (ltc2983)
	transistor-ideality (max6697)
	ti,n-factor (tmp421)
	extended-range-enable (may be dynamic; the driver currently
		disables it)
	ti,extended-range-enable (lm90)
	resistance-cancellation (max6697)
	beta-compensation-enable (max6697)
	ti,beta-compensation (tmp401)
	smbus-timeout-disable (max6697)
	alert-mask, over-temperature-mask (max6697)
		I am really happy with those; I think it should be
		automatic based on enabled channels
	temperature-offset-millicelsius (lm90)

Then there is the question if temperature limits should be provided
in devicetree, in addition to the above where appropriate. After all,
those are thermal system properties.

Examples for channel based devicetree property descriptions:

devicetree/bindings/hwmon/
	ti,tmp421.yaml
	nuvoton,nct7802.yaml
	national,lm90.yaml

Looking into interrupts and interrupt modes:

For the interrupt mode, I'll assume for the time being that its
implementation is similar to that of other chips. I'll use MAX31730
as example, and subsequently refer to it as if MAX31732 would
implement the same mechanism.

In interrupt mode, interrupts are cleared by writing into the
status register. If the condition still exists, the interrupt will
be re-asserted after the next conversion. Clearly, that does not work
with the driver as written - it would fill the log with messages,
and send an endless amount of notifications to userspace.

In comparator mode, interrupts are asserted whenever a temperature
measurement exceeds a threshold value, and is deasserted when the
temperature is back in the acceptable range. This would be equivalent
to an edge triggered interrupt.

The example below configures interrupts in interrupt mode but with
IRQ_TYPE_EDGE_BOTH. No idea how that is supposed to work, because
in interrupt mode the interrupt would have to be level triggered,
not edge triggered, to make sense. Edge triggered interrupt only
makes sense if the chip is in comparator mode, since only in that mode
the edges have any meaning.

The driver would need to know if the interrupt is edge triggered or
level triggered, and that should decide how to handle it. Edge triggered
should result in comparator mode, and level triggered should result
in interrupt mode.

Overall I am not sure if comparator mode even makes sense, since
the interrupt line(s) would become active if a single temperature
is out of range, and only become inactive if all temperatures are
within range. Effecively this mode could only be used on a subset
of channels, but that would require the ability to enable alerts
on a per-channel basis.

Either case, the interrupt handler would need to cache any previously
reported status, since it should only generate a notification if the
status changes, and not for each interrupt and each channel. 

With that, enough for now.

Guenter

> > 
> >>> +    type: boolean
> >>> +
> >>> +  adi,alarm2-interrupt-mode:
> >>> +    description: |
> >>> +      Enables the ALARM2 output to function in interrupt mode.
> >>> +      Default ALARM2 output function is comparator mode.
> >>
> >> Same question.
> >>
> >>> +    type: boolean
> >>> +
> >>> +  adi,alarm1-fault-queue:
> >>> +    description: The number of consecutive faults required to assert ALARM1.
> >>
> >> Same question - why this number differs with hardware?
> >>
> > 
> > Noisier hardware will require more samples to avoid spurious faults.
> > Trade-off is speed of reporting a fault. Normally the board designer
> > would determine a value which is low enough to avoid spurious faults.
> > 
> > Note that the chip (according to patch 2/3) supports resistance
> > cancellation as well as beta compensation, which are also board specific.
> > I don't have access to the datasheet, so I don't know for sure if those
> > are configurable (typically they are). If they are configurable, I would
> > expect to see respective properties.
> 
> If that's the argument behind property, then it's fine.
> 
> Best regards,
> Krzysztof
>