Re: [PATCH v5] hwmon: (max31722) Add threshold alarm support

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

 



On 04/08/2016 05:54 AM, Breana, Tiberiu A wrote:
-----Original Message-----
From: Guenter Roeck [mailto:linux@xxxxxxxxxxxx]
Sent: Friday, April 8, 2016 3:24 PM
To: Breana, Tiberiu A <tiberiu.a.breana@xxxxxxxxx>; lm-sensors@lm-
sensors.org
Cc: Baluta, Daniel <daniel.baluta@xxxxxxxxx>
Subject: Re: [PATCH v5] hwmon: (max31722) Add threshold alarm support

On 04/08/2016 02:12 AM, Tiberiu Breana wrote:
Add threshold alarm support for the max31722 temperature sensor driver.

Signed-off-by: Tiberiu Breana <tiberiu.a.breana@xxxxxxxxx>
---
Changes from v4:
- addressed Guenter's comments

As specified in the v3 change log, threshold values, as well as the
alarm attribute, are only visible if interrupts are enabled.

I missed that part. Why ? The thresholds are still supported by the chip, even
without interrupts.

Thanks,
Guenter

I don't see the point of exposing the threshold values if they can't be used
to generate interrupts.

Looking into the datasheet again, there are at least two possible other
use cases, one of which does not require interrupt support.

First, by putting the chip into comparator mode and selecting an edge
triggered interrupt, the interrupt line itself would indicate the
temperature status. This would be more beneficial than the current
implementation, since the interrupt line status would indicate
when/if the chip exceeded temperature limits, and the reading the current
temperature as well as the comparisons would not be necessary (assuming
the interrupt line status can be reported).

Second, also in comparator more, the TOUT line could be used
to directly trigger some action, such as turning on a fan. This
use case does not require any interrupt in the first place,
but still makes use of thigh/tlow.

The second use case makes it questionable if interrupt mode should
be selected automatically. The default operating mode is comparator
mode, and that use case requires it. Given that, and if your use case
requires interrupt mode, I think interrupt mode should only be selected
if interrupt support is enabled.

Thanks,
Guenter

Thank you as well for your feedback.

Tiberiu


---
   drivers/hwmon/max31722.c | 181
+++++++++++++++++++++++++++++++++++++++++++++--
   1 file changed, 177 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/max31722.c b/drivers/hwmon/max31722.c
index
30a100e..f557c36 100644
--- a/drivers/hwmon/max31722.c
+++ b/drivers/hwmon/max31722.c
@@ -12,23 +12,38 @@
   #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
+/*
+ * Minimum temperature value: -2^(12-1)      * 62.5  = -128,000
+ * Maximum temperature value: (2^(12-1) - 1) * 62.5 ~=  127,937  */
+#define MAX31722_MIN_TEMP			       -128000
+#define MAX31722_MAX_TEMP				127937

   struct max31722_data {
   	struct device *hwmon_dev;
   	struct spi_device *spi_device;
+	struct mutex update_lock;
   	u8 mode;
+	s32 thigh;
+	s32 tlow;
+	bool irq_enabled;
+	bool alarm_active;
   };

   static int max31722_set_mode(struct max31722_data *data, u8 mode)
@@
-56,6 +71,12 @@ 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);
+
+	if (dev_attr->index == MAX31722_REG_THIGH_LSB)
+		return sprintf(buf, "%d\n", data->thigh);
+	else if (dev_attr->index == MAX31722_REG_TLOW_LSB)
+		return sprintf(buf, "%d\n", data->tlow);

   	ret = spi_w8r16(data->spi_device, MAX31722_REG_TEMP_LSB);
   	if (ret < 0)
@@ -64,15 +85,145 @@ static ssize_t max31722_show_temp(struct device
*dev,
   	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 * 32, 125));
+
+	tx_buf[0] = dev_attr->index | MAX31722_WRITE_MASK;
+	tx_buf[1] = thresh & 0xff;
+	tx_buf[2] = thresh >> 8;
+
+	mutex_lock(&data->update_lock);
+
+	ret = spi_write(data->spi_device, &tx_buf, sizeof(tx_buf));
+	if (ret < 0) {
+		mutex_unlock(&data->update_lock);
+		return ret;
+	}
+
+	if (dev_attr->index == MAX31722_REG_THIGH_LSB)
+		data->thigh = thresh * 125 / 32;
+	else
+		data->tlow = thresh * 125 / 32;
+
+	mutex_unlock(&data->update_lock);
+
+	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 alarm and threshold attributes if interrupts are disabled. */
+	if (!data->irq_enabled && index > 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 ssize_t max31722_update_alarm(struct max31722_data *data) {
+	s16 temp;
+	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)
+		return ret;
+	temp = (s16)le16_to_cpu(ret) * 125 / 32;
+
+	if (temp > data->thigh || (!data->alarm_active && temp > data-
tlow))
+		data->alarm_active = true;
+	else if (temp <= data->tlow ||
+			(data->alarm_active && temp < data->thigh))
+		data->alarm_active = false;
+
+	return 0;
+}
+
+static irqreturn_t max31722_irq_handler(int irq, void *private) {
+	struct max31722_data *data = private;
+
+	max31722_update_alarm(data);
+	sysfs_notify(&data->spi_device->dev.kobj, NULL, "temp1_alarm");
+
+	return IRQ_HANDLED;
+}
+
+static ssize_t max31722_init_thresh(struct max31722_data *data) {
+	ssize_t ret;
+
+	/* Cache threshold values. */
+	ret = spi_w8r16(data->spi_device, MAX31722_REG_TLOW_LSB);
+	if (ret < 0)
+		goto exit_err;
+	data->tlow = (s16)le16_to_cpu(ret) * 125 / 32;
+
+	ret = spi_w8r16(data->spi_device, MAX31722_REG_THIGH_LSB);
+	if (ret < 0)
+		goto exit_err;
+	data->thigh = (s16)le16_to_cpu(ret) * 125 / 32;
+
+	return 0;
+
+exit_err:
+	dev_err(&data->spi_device->dev,
+		"failed to read temperature threshold\n");
+	return ret;
+}

   static int max31722_probe(struct spi_device *spi)
   {
@@ -83,17 +234,39 @@ static int max31722_probe(struct spi_device *spi)
   	if (!data)
   		return -ENOMEM;

+	mutex_init(&data->update_lock);
   	spi_set_drvdata(spi, data);
   	data->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;

+	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_warn(&spi->dev, "request irq %d failed\n",
+				  spi->irq);
+		} else {
+			data->irq_enabled = true;
+			ret = max31722_init_thresh(data);
+			if (ret < 0)
+				return ret;
+			ret = max31722_update_alarm(data);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
   	data->hwmon_dev = hwmon_device_register_with_groups(&spi-
dev,
   							    spi->modalias,
   							    data,





_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors



[Index of Archives]     [Linux Kernel]     [Linux Hardware Monitoring]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux