[lm-sensors] [PATCH v2] hwmon: (max31722) Add threshold alarm support

[Tiberiu Breana]

Add threshold alarm support for the max31722 temperature sensor driver.

Signed-off-by: Tiberiu Breana <***@intel.com>
---
Changes from v1:
- addressed Guenter's comments
- chip is now set to "interrupt mode" at probe time
- changed minimum and maximum threshold values according to register limits
- the interrupt handler now compares the current temperature to the thresholds
before (re)setting the alarm
- added sysfs notifications for temp1_alarm
- added the max31722_update_alarm function used by the irq handler as well as
at probing time
---
drivers/hwmon/max31722.c | 156 +++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 151 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/max31722.c b/drivers/hwmon/max31722.c
index 30a100e..de3cd18 100644
--- a/drivers/hwmon/max31722.c
+++ b/drivers/hwmon/max31722.c
@@ -12,23 +12,42 @@
#include <linux/acpi.h>
#include <linux/hwmon.h>
#include <linux/hwmon-sysfs.h>
+#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/spi/spi.h>

#define MAX31722_REG_CFG 0x00
#define MAX31722_REG_TEMP_LSB 0x01
+#define MAX31722_REG_THIGH_LSB 0x03
+#define MAX31722_REG_TLOW_LSB 0x05

#define MAX31722_MODE_CONTINUOUS 0x00
#define MAX31722_MODE_STANDBY 0x01
#define MAX31722_MODE_MASK 0xFE
#define MAX31722_RESOLUTION_12BIT 0x06
+#define MAX31722_TM_INTERRUPT 0x08
#define MAX31722_WRITE_MASK 0x80
+/*
+ * The 12-bit scale of 62.5 millidegrees/bit is rounded up instead of down
+ * so we don't exceed the maximum register values when setting thresholds.
+ */
+#define MAX31722_12BIT_STEP 63
+/*
+ * Minimum temperature value: -2^(12-1) * 62.5 = -128,000
+ * Maximum temperature value: (2^(12-1) - 1) * 62.5 ~= 127,938
+ * Due to the rounding of the scale value, the actual threshold limits,
+ * after re-calculating register values, will be -127,000 and 126,937.
+ */
+#define MAX31722_MIN_TEMP -128000
+#define MAX31722_MAX_TEMP 127938

struct max31722_data {
struct device *hwmon_dev;
struct spi_device *spi_device;
u8 mode;
+ bool irq_enabled;
+ bool alarm_active;
};

static int max31722_set_mode(struct max31722_data *data, u8 mode)
@@ -56,23 +75,133 @@ static ssize_t max31722_show_temp(struct device *dev,
{
ssize_t ret;
struct max31722_data *data = dev_get_drvdata(dev);
+ struct sensor_device_attribute *dev_attr = to_sensor_dev_attr(attr);

- ret = spi_w8r16(data->spi_device, MAX31722_REG_TEMP_LSB);
+ ret = spi_w8r16(data->spi_device, dev_attr->index);
if (ret < 0)
return ret;
/* Keep 12 bits and multiply by the scale of 62.5 millidegrees/bit. */
return sprintf(buf, "%d\n", (s16)le16_to_cpu(ret) * 125 / 32);
}

-static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
- max31722_show_temp, NULL, 0);
+static ssize_t max31722_set_temp(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ret;
+ long val;
+ s16 thresh;
+ u8 tx_buf[3];
+ struct max31722_data *data = dev_get_drvdata(dev);
+ struct sensor_device_attribute *dev_attr = to_sensor_dev_attr(attr);
+
+ ret = kstrtol(buf, 10, &val);
+ if (ret < 0)
+ return ret;
+
+ val = clamp_val(val, MAX31722_MIN_TEMP, MAX31722_MAX_TEMP);
+ thresh = cpu_to_le16(DIV_ROUND_CLOSEST(val, MAX31722_12BIT_STEP) << 4);
+
+ tx_buf[0] = dev_attr->index | MAX31722_WRITE_MASK;
+ tx_buf[1] = (thresh << 8) >> 8;
+ tx_buf[2] = thresh >> 8;
+ ret = spi_write(data->spi_device, &tx_buf, sizeof(tx_buf));
+ if (ret < 0)
+ return ret;
+
+ return count;
+}
+
+static ssize_t max31722_show_alarm(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct spi_device *spi_device = to_spi_device(dev);
+ struct max31722_data *data = spi_get_drvdata(spi_device);
+
+ return sprintf(buf, "%d\n", data->alarm_active);
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, max31722_show_temp, NULL,
+ MAX31722_REG_TEMP_LSB);
+static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, max31722_show_temp,
+ max31722_set_temp, MAX31722_REG_THIGH_LSB);
+static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO, max31722_show_temp,
+ max31722_set_temp, MAX31722_REG_TLOW_LSB);
+static DEVICE_ATTR(temp1_alarm, S_IRUGO, max31722_show_alarm, NULL);

static struct attribute *max31722_attrs[] = {
&sensor_dev_attr_temp1_input.dev_attr.attr,
+ &sensor_dev_attr_temp1_max.dev_attr.attr,
+ &sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
+ &dev_attr_temp1_alarm.attr,
NULL,
};

-ATTRIBUTE_GROUPS(max31722);
+static umode_t max31722_is_visible(struct kobject *kobj, struct attribute *attr,
+ int index)
+{
+ struct device *dev = container_of(kobj, struct device, kobj);
+ struct max31722_data *data = dev_get_drvdata(dev);
+
+ /* Hide the alarm attribute if interrupts are disabled. */
+ if (!data->irq_enabled &&
+ strncmp(attr->name, "temp1_alarm", sizeof("temp1_alarm")) == 0)
+ return 0;
+
+ return attr->mode;
+}
+
+static const struct attribute_group max31722_group = {
+ .attrs = max31722_attrs, .is_visible = max31722_is_visible,
+};
+
+__ATTRIBUTE_GROUPS(max31722);
+
+static void max31722_update_alarm(struct max31722_data *data)
+{
+ u16 temp;
+ u16 thigh;
+ u16 tlow;
+ ssize_t ret;
+
+ /*
+ * Do a quick temperature reading and compare it with TLOW/THIGH
+ * so we can tell which threshold has been met.
+ */
+ ret = spi_w8r16(data->spi_device, MAX31722_REG_TEMP_LSB);
+ if (ret < 0)
+ goto exit_err;
+ temp = (s16)(le16_to_cpu(ret) * 125 / 32);
+ ret = spi_w8r16(data->spi_device, MAX31722_REG_TLOW_LSB);
+ if (ret < 0)
+ goto exit_err;
+ tlow = (s16)(le16_to_cpu(ret) * 125 / 32);
+ ret = spi_w8r16(data->spi_device, MAX31722_REG_THIGH_LSB);
+ if (ret < 0)
+ goto exit_err;
+ thigh = (s16)(le16_to_cpu(ret) * 125 / 32);
+
+ if (temp > thigh || (temp < thigh && temp > tlow))
+ data->alarm_active = true;
+ else
+ data->alarm_active = false;
+
+ sysfs_notify(&data->spi_device->dev.kobj, NULL, "temp1_alarm");
+ return;
+
+exit_err:
+ dev_err(&data->spi_device->dev, "failed to read temperature register\n");
+}
+
+static irqreturn_t max31722_irq_handler(int irq, void *private)
+{
+ struct max31722_data *data = private;
+
+ max31722_update_alarm(data);
+
+ return IRQ_HANDLED;
+}

static int max31722_probe(struct spi_device *spi)
{
@@ -89,11 +218,28 @@ static int max31722_probe(struct spi_device *spi)
* Set SD bit to 0 so we can have continuous measurements.
* Set resolution to 12 bits for maximum precision.
*/
- data->mode = MAX31722_MODE_CONTINUOUS | MAX31722_RESOLUTION_12BIT;
+ data->mode = MAX31722_MODE_CONTINUOUS | MAX31722_RESOLUTION_12BIT
+ | MAX31722_TM_INTERRUPT;
ret = max31722_set_mode(data, MAX31722_MODE_CONTINUOUS);
if (ret < 0)
return ret;

+ data->irq_enabled = false;
+ if (spi->irq > 0) {
+ ret = devm_request_threaded_irq(&spi->dev, spi->irq,
+ NULL,
+ max31722_irq_handler,
+ IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+ dev_name(&spi->dev), data);
+ if (ret < 0) {
+ dev_err(&spi->dev, "request irq %d failed\n", spi->irq);
+ } else {
+ data->irq_enabled = true;
+ data->alarm_active = false;
+ max31722_update_alarm(data);
+ }
+ }
+
data->hwmon_dev = hwmon_device_register_with_groups(&spi->dev,
spi->modalias,
data,

[Guenter Roeck]

Post by Tiberiu Breana
Add threshold alarm support for the max31722 temperature sensor driver.
---
- addressed Guenter's comments
- chip is now set to "interrupt mode" at probe time
- changed minimum and maximum threshold values according to register limits
- the interrupt handler now compares the current temperature to the thresholds
before (re)setting the alarm
- added sysfs notifications for temp1_alarm
- added the max31722_update_alarm function used by the irq handler as well as
at probing time
---
drivers/hwmon/max31722.c | 156 +++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 151 insertions(+), 5 deletions(-)
diff --git a/drivers/hwmon/max31722.c b/drivers/hwmon/max31722.c
index 30a100e..de3cd18 100644
--- a/drivers/hwmon/max31722.c
+++ b/drivers/hwmon/max31722.c
@@ -12,23 +12,42 @@
#include <linux/acpi.h>
#include <linux/hwmon.h>
#include <linux/hwmon-sysfs.h>
+#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/spi/spi.h>
#define MAX31722_REG_CFG 0x00
#define MAX31722_REG_TEMP_LSB 0x01
+#define MAX31722_REG_THIGH_LSB 0x03
+#define MAX31722_REG_TLOW_LSB 0x05
#define MAX31722_MODE_CONTINUOUS 0x00
#define MAX31722_MODE_STANDBY 0x01
#define MAX31722_MODE_MASK 0xFE
#define MAX31722_RESOLUTION_12BIT 0x06
+#define MAX31722_TM_INTERRUPT 0x08
#define MAX31722_WRITE_MASK 0x80
+/*
+ * The 12-bit scale of 62.5 millidegrees/bit is rounded up instead of down
+ * so we don't exceed the maximum register values when setting thresholds.
+ */
+#define MAX31722_12BIT_STEP 63

That gives you the same rounding error as in the other diretion, though.
It would be better to use
DIV_ROUND_CLOSEST(val * 32, 125);
inetead of
DIV_ROUND_CLOSEST(val, 63) << 4;

below.

Post by Tiberiu Breana
+/*
+ * Minimum temperature value: -2^(12-1) * 62.5 = -128,000
+ * Maximum temperature value: (2^(12-1) - 1) * 62.5 ~= 127,938
+ * Due to the rounding of the scale value, the actual threshold limits,
+ * after re-calculating register values, will be -127,000 and 126,937.
+ */
+#define MAX31722_MIN_TEMP -128000
+#define MAX31722_MAX_TEMP 127938
struct max31722_data {
struct device *hwmon_dev;
struct spi_device *spi_device;
u8 mode;
+ bool irq_enabled;
+ bool alarm_active;
};
static int max31722_set_mode(struct max31722_data *data, u8 mode)
@@ -56,23 +75,133 @@ static ssize_t max31722_show_temp(struct device *dev,
{
ssize_t ret;
struct max31722_data *data = dev_get_drvdata(dev);
+ struct sensor_device_attribute *dev_attr = to_sensor_dev_attr(attr);
- ret = spi_w8r16(data->spi_device, MAX31722_REG_TEMP_LSB);
+ ret = spi_w8r16(data->spi_device, dev_attr->index);
if (ret < 0)
return ret;
/* Keep 12 bits and multiply by the scale of 62.5 millidegrees/bit. */
return sprintf(buf, "%d\n", (s16)le16_to_cpu(ret) * 125 / 32);
}
-static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
- max31722_show_temp, NULL, 0);
+static ssize_t max31722_set_temp(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ret;
+ long val;
+ s16 thresh;
+ u8 tx_buf[3];
+ struct max31722_data *data = dev_get_drvdata(dev);
+ struct sensor_device_attribute *dev_attr = to_sensor_dev_attr(attr);
+
+ ret = kstrtol(buf, 10, &val);
+ if (ret < 0)
+ return ret;
+
+ val = clamp_val(val, MAX31722_MIN_TEMP, MAX31722_MAX_TEMP);
+ thresh = cpu_to_le16(DIV_ROUND_CLOSEST(val, MAX31722_12BIT_STEP) << 4);
+

see above - please try to avoid rounding errors.

Post by Tiberiu Breana
+ tx_buf[0] = dev_attr->index | MAX31722_WRITE_MASK;
+ tx_buf[1] = (thresh << 8) >> 8;

I would suggest "thresh & 0xff", as it is easier to understand. Besides, for
negative values, the upper 8 bits are going to be 0xff, meaning the double shift
doesn't really do what you think it will do.

Post by Tiberiu Breana
+ tx_buf[2] = thresh >> 8;
+ ret = spi_write(data->spi_device, &tx_buf, sizeof(tx_buf));
+ if (ret < 0)
+ return ret;
+
+ return count;
+}
+
+static ssize_t max31722_show_alarm(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct spi_device *spi_device = to_spi_device(dev);
+ struct max31722_data *data = spi_get_drvdata(spi_device);
+
+ return sprintf(buf, "%d\n", data->alarm_active);
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, max31722_show_temp, NULL,
+ MAX31722_REG_TEMP_LSB);
+static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, max31722_show_temp,
+ max31722_set_temp, MAX31722_REG_THIGH_LSB);
+static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO, max31722_show_temp,
+ max31722_set_temp, MAX31722_REG_TLOW_LSB);
+static DEVICE_ATTR(temp1_alarm, S_IRUGO, max31722_show_alarm, NULL);
static struct attribute *max31722_attrs[] = {
&sensor_dev_attr_temp1_input.dev_attr.attr,
+ &sensor_dev_attr_temp1_max.dev_attr.attr,
+ &sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
+ &dev_attr_temp1_alarm.attr,
NULL,
};
-ATTRIBUTE_GROUPS(max31722);
+static umode_t max31722_is_visible(struct kobject *kobj, struct attribute *attr,
+ int index)
+{
+ struct device *dev = container_of(kobj, struct device, kobj);
+ struct max31722_data *data = dev_get_drvdata(dev);
+
+ /* Hide the alarm attribute if interrupts are disabled. */
+ if (!data->irq_enabled &&
+ strncmp(attr->name, "temp1_alarm", sizeof("temp1_alarm")) == 0)

Just use "&& index == 3" here. That is what it is for.

Post by Tiberiu Breana
+ return 0;
+
+ return attr->mode;
+}
+
+static const struct attribute_group max31722_group = {
+ .attrs = max31722_attrs, .is_visible = max31722_is_visible,
+};
+
+__ATTRIBUTE_GROUPS(max31722);
+
+static void max31722_update_alarm(struct max31722_data *data)
+{
+ u16 temp;
+ u16 thigh;
+ u16 tlow;
+ ssize_t ret;
+
+ /*
+ * Do a quick temperature reading and compare it with TLOW/THIGH
+ * so we can tell which threshold has been met.
+ */
+ ret = spi_w8r16(data->spi_device, MAX31722_REG_TEMP_LSB);
+ if (ret < 0)
+ goto exit_err;
+ temp = (s16)(le16_to_cpu(ret) * 125 / 32);
+ ret = spi_w8r16(data->spi_device, MAX31722_REG_TLOW_LSB);
+ if (ret < 0)
+ goto exit_err;
+ tlow = (s16)(le16_to_cpu(ret) * 125 / 32);
+ ret = spi_w8r16(data->spi_device, MAX31722_REG_THIGH_LSB);
+ if (ret < 0)
+ goto exit_err;
+ thigh = (s16)(le16_to_cpu(ret) * 125 / 32);
+

Please consider caching tlow and thigh. Those won't change.

Post by Tiberiu Breana
+ if (temp > thigh || (temp < thigh && temp > tlow))
+ data->alarm_active = true;

If we assume that thigh > tlow, this is equivalent to "temp != thigh && temp >
tlow", which doesn't make much sense.

Post by Tiberiu Breana
+ else
+ data->alarm_active = false;
+

I think something like the following would be better, since it takes
the current alarm status into account.

if (temp > thigh || (!data->alarm_active && temp > tlow))
data->alarm_active = true;
else if (temp <= tlow || (data->alarm_active && temp < thigh))
data->alarm_active = false;

Post by Tiberiu Breana
+ sysfs_notify(&data->spi_device->dev.kobj, NULL, "temp1_alarm");
+ return;
+
+ dev_err(&data->spi_device->dev, "failed to read temperature register\n");
+}
+
+static irqreturn_t max31722_irq_handler(int irq, void *private)
+{
+ struct max31722_data *data = private;
+
+ max31722_update_alarm(data);
+
+ return IRQ_HANDLED;
+}
static int max31722_probe(struct spi_device *spi)
{
@@ -89,11 +218,28 @@ static int max31722_probe(struct spi_device *spi)
* Set SD bit to 0 so we can have continuous measurements.
* Set resolution to 12 bits for maximum precision.
*/
- data->mode = MAX31722_MODE_CONTINUOUS | MAX31722_RESOLUTION_12BIT;
+ data->mode = MAX31722_MODE_CONTINUOUS | MAX31722_RESOLUTION_12BIT
+ | MAX31722_TM_INTERRUPT;
ret = max31722_set_mode(data, MAX31722_MODE_CONTINUOUS);
if (ret < 0)
return ret;
+ data->irq_enabled = false;
+ if (spi->irq > 0) {
+ ret = devm_request_threaded_irq(&spi->dev, spi->irq,
+ NULL,
+ max31722_irq_handler,
+ IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+ dev_name(&spi->dev), data);
+ if (ret < 0) {
+ dev_err(&spi->dev, "request irq %d failed\n", spi->irq);

dev_warn(), please, since the error is ignored.

Post by Tiberiu Breana
+ } else {
+ data->irq_enabled = true;
+ data->alarm_active = false;
+ max31722_update_alarm(data);
+ }
+ }
+
data->hwmon_dev = hwmon_device_register_with_groups(&spi->dev,
spi->modalias,
data,
--
1.9.1

[Guenter Roeck]

Post by Tiberiu Breana
Add threshold alarm support for the max31722 temperature sensor driver.
---

[ ... ]

Post by Tiberiu Breana
+
+ /*
+ * Do a quick temperature reading and compare it with TLOW/THIGH
+ * so we can tell which threshold has been met.
+ */
+ ret = spi_w8r16(data->spi_device, MAX31722_REG_TEMP_LSB);
+ if (ret < 0)
+ goto exit_err;
+ temp = (s16)(le16_to_cpu(ret) * 125 / 32);

Another comment: No need to convert values to milli-degrees here.
Just use raw register values.

Thanks,
Guenter