[PATCH v2 0/2] Add driver for ST stts751 thermal sensor

[PATCH v2 0/2] Add driver for ST stts751 thermal sensor

[Andrea Merello]

This series adds new driver for ST stts751 thermal sensor

Changes since v1:
- removed the "manual mode" logic; always use automatic mode
- changed the way 16-bit data is read from two 8-bit regs (now similar to lm90)
- code simplifications
- comment style fixes

Andrea Merello (2):
  DT: add binding documentation for STTS751
  hwmon: new driver for ST stts751 thermal sensor

 .../devicetree/bindings/hwmon/stts751.txt          |  19 +
 drivers/hwmon/Kconfig                              |  12 +-
 drivers/hwmon/Makefile                             |   2 +-
 drivers/hwmon/stts751.c                            | 805 +++++++++++++++++++++
 4 files changed, 836 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/stts751.txt
 create mode 100644 drivers/hwmon/stts751.c

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: LABBE Corentin <clabbe.montjoie@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Jean Delvare <jdelvare@suse.com>


--
2.7.4

[PATCH v2 1/2] DT: add binding documentation for STTS751

[Andrea Merello]

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 Documentation/devicetree/bindings/hwmon/stts751.txt | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/stts751.txt

diff --git a/Documentation/devicetree/bindings/hwmon/stts751.txt b/Documentation/devicetree/bindings/hwmon/stts751.txt
new file mode 100644
index 0000000..277ad46
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/stts751.txt
@@ -0,0 +1,19 @@
+* STTS751 thermometer.
+
+Required node properties:
+- compatible: "stts751"
+- reg: I2C bus address of the device
+
+Optional properties:
+- has-therm: specifies that the "therm" pin is wired
+- has-event: specifies that the "event" pin is wired
+- smbus-timeout-disable: when set, the smbus timeout function will be disabled
+
+Example stts751 node:
+
+temp-sensor {
+	compatible = "stts751";
+	reg = <0x48>;
+	has-therm;
+	has-event;
+}
-- 
2.7.4

[PATCH v2 2/2] hwmon: new driver for ST stts751 thermal sensor

[Andrea Merello]

This patch adds a HWMON driver for ST Microelectronics STTS751
temperature sensors.

It does support temperature reading (of course), SMBUS alert
functionality and "therm" output.

Thanks-to: LABBE Corentin [for suggestions]
Thanks-to: Guenter Roeck [for suggestions/discussions]
Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
Cc: LABBE Corentin <clabbe.montjoie@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Jean Delvare <jdelvare@suse.com>
---
 drivers/hwmon/Kconfig   |  12 +-
 drivers/hwmon/Makefile  |   2 +-
 drivers/hwmon/stts751.c | 805 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 817 insertions(+), 2 deletions(-)
 create mode 100644 drivers/hwmon/stts751.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index eaf2f91..4e70b42 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1448,6 +1448,16 @@ config SENSORS_SCH5636
 	  This driver can also be built as a module.  If so, the module
 	  will be called sch5636.
 
+config SENSORS_STTS751
+	tristate "ST Microelectronics STTS751"
+	depends on I2C
+	help
+	  If you say yes here you get support for STTS751
+	  temperature sensor chips.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called stts751.
+
 config SENSORS_SMM665
 	tristate "Summit Microelectronics SMM665"
 	depends on I2C
@@ -1506,7 +1516,7 @@ config SENSORS_ADS7871
 
 config SENSORS_AMC6821
 	tristate "Texas Instruments AMC6821"
-	depends on I2C 
+	depends on I2C
 	help
 	  If you say yes here you get support for the Texas Instruments
 	  AMC6821 hardware monitoring chips.
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index fe87d28..1114130 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -147,6 +147,7 @@ obj-$(CONFIG_SENSORS_SMM665)	+= smm665.o
 obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
 obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
 obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
+obj-$(CONFIG_SENSORS_STTS751)	+= stts751.o
 obj-$(CONFIG_SENSORS_AMC6821)	+= amc6821.o
 obj-$(CONFIG_SENSORS_TC74)	+= tc74.o
 obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
@@ -169,4 +170,3 @@ obj-$(CONFIG_SENSORS_WM8350)	+= wm8350-hwmon.o
 obj-$(CONFIG_PMBUS)		+= pmbus/
 
 ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
-
diff --git a/drivers/hwmon/stts751.c b/drivers/hwmon/stts751.c
new file mode 100644
index 0000000..8358e04
--- /dev/null
+++ b/drivers/hwmon/stts751.c
@@ -0,0 +1,805 @@
+/*
+ * STTS751 sensor driver
+ *
+ * Copyright (C) 2016 Istituto Italiano di Tecnologia - RBCS - EDL
+ * Robotics, Brain and Cognitive Sciences department
+ * Electronic Design Laboratory
+ *
+ * Written by Andrea Merello <andrea.merello@gmail.com>
+ *
+ * Based on  LM95241 driver and LM90 driver
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+
+#include <linux/bitops.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_irq.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/util_macros.h>
+
+#define DEVNAME "stts751"
+
+static const unsigned short normal_i2c[] = {
+	0x48, 0x49, 0x38, 0x39,  /* STTS751-0 */
+	0x4A, 0x4B, 0x3A, 0x3B,  /* STTS751-1 */
+	I2C_CLIENT_END };
+
+#define STTS751_REG_TEMP_H	0x00
+#define STTS751_REG_STATUS	0x01
+#define STTS751_STATUS_TRIPL	BIT(5)
+#define STTS751_STATUS_TRIPH	BIT(6)
+#define STTS751_STATUS_BUSY	BIT(8)
+#define STTS751_REG_TEMP_L	0x02
+#define STTS751_REG_CONF	0x03
+#define STTS751_CONF_RES_MASK	0x0C
+#define STTS751_CONF_RES_SHIFT  2
+#define STTS751_CONF_EVENT_DIS  BIT(7)
+#define STTS751_CONF_STOP	BIT(6)
+#define STTS751_REG_RATE	0x04
+#define STTS751_REG_HLIM_H	0x05
+#define STTS751_REG_HLIM_L	0x06
+#define STTS751_REG_LLIM_H	0x07
+#define STTS751_REG_LLIM_L	0x08
+#define STTS751_REG_ONESHOT	0x0F
+#define STTS751_REG_TLIM	0x20
+#define STTS751_REG_HYST	0x21
+#define STTS751_REG_SMBUS_TO	0x22
+
+#define STTS751_REG_PROD_ID	0xFD
+#define STTS751_REG_MAN_ID	0xFE
+#define STTS751_REG_REV_ID	0xFF
+
+#define STTS751_0_PROD_ID	0x00
+#define STTS751_1_PROD_ID	0x01
+#define ST_MAN_ID		0x53
+
+/* stick with HW defaults */
+#define STTS751_THERM_DEFAULT	85000
+#define STTS751_HYST_DEFAULT	10000
+#define STTS751_EVENT_MAX_DEFAULT 85000
+#define STTS751_EVENT_MIN_DEFAULT 0
+
+#define STTS751_RACE_RETRY	5
+#define STTS751_CONV_TIMEOUT	100 /* mS */
+#define STTS751_CACHE_TIME	100 /* mS */
+
+/*
+ * Possible update intervals are (in mS):
+ * 16000, 8000, 4000, 2000, 1000, 500, 250, 125, 62.5, 31.25
+ * However we are not going to complicate things too much and we stick to the
+ * approx value in mS.
+ */
+static const int stts751_intervals[] = {
+	16000, 8000, 4000, 2000, 1000, 500, 250, 125, 63, 31
+};
+
+struct stts751_priv {
+	struct device *dev;
+	struct i2c_client *client;
+	struct mutex access_lock;
+	unsigned long interval;
+	int res;
+	bool gen_therm, gen_event;
+	int event_max, event_min;
+	int therm;
+	int hyst;
+	bool smbus_timeout;
+	int temp;
+	unsigned long last_update;
+	u8 config;
+	bool min_alert, max_alert;
+	bool data_valid;
+
+	/*
+	 * Temperature/interval is always present.
+	 * Depending by DT therm and event are dynamically added.
+	 * There are max 4 entries plus the guard
+	 */
+	const struct attribute_group *groups[5];
+};
+
+/*
+ * These functions converts temperature from HW format to integer format and
+ * vice-vers. They are (mostly) taken from lm90 driver. Unit is in mC.
+ */
+static int stts751_to_deg(s32 hw_val)
+{
+	return hw_val * 125 / 32;
+}
+
+static s16 stts751_to_hw(int val, s32 *hw_val)
+{
+	/* HW works in range -64C to +127C */
+	if ((val > 127000) || (val < -64000))
+		return -EINVAL;
+
+	if (val < 0)
+		*hw_val = (val - 62) / 125 * 32;
+	else
+		*hw_val = (val + 62) / 125 * 32;
+
+	return 0;
+}
+
+static int stts751_adjust_resolution(struct stts751_priv *priv)
+{
+	u8 res;
+
+	switch (priv->interval) {
+	case 9:
+		/* 10 bits */
+		res = 0;
+		break;
+	case 8:
+		/* 11 bits */
+		res = 1;
+		break;
+	default:
+		/* 12 bits */
+		res = 3;
+		break;
+	}
+
+	if (priv->res == res)
+		return 0;
+
+	priv->config &= ~STTS751_CONF_RES_MASK;
+	priv->config |= res << STTS751_CONF_RES_SHIFT;
+
+	return i2c_smbus_write_byte_data(priv->client,
+				STTS751_REG_CONF, priv->config);
+}
+
+static int stts751_update_temp(struct stts751_priv *priv)
+{
+	s32 integer1, integer2, frac;
+	int ret = 0;
+
+	mutex_lock(&priv->access_lock);
+
+	/*
+	 * There is a trick here, like in the lm90 driver. We have to read two
+	 * registers to get the sensor temperature, but we have to beware a
+	 * conversion could occur between the readings. We could use the
+	 * one-shot conversion register, but we don't want to do this (disables
+	 * hardware monitoring). So the solution used here is to read the high
+	 * byte once, then the low byte, then the high byte again. If the new
+	 * high byte matches the old one, then we have a valid reading. Else we
+	 * have to read the low byte again, and now we believe we have a correct
+	 * reading.
+	 */
+	integer1 = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_H);
+	if (integer1 < 0) {
+		dev_dbg(&priv->client->dev,
+			"I2C read failed (temp H). ret: %x\n", ret);
+		ret = integer1;
+		goto exit;
+	}
+
+	frac = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_L);
+	if (frac < 0) {
+		dev_dbg(&priv->client->dev,
+			"I2C read failed (temp L). ret: %x\n", ret);
+		ret = frac;
+		goto exit;
+	}
+
+	integer2 = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_H);
+	if (integer2 < 0) {
+		dev_dbg(&priv->client->dev,
+			"I2C 2nd read failed (temp H). ret: %x\n", ret);
+		ret = integer2;
+		goto exit;
+	}
+
+	if (integer1 != integer2) {
+		frac = i2c_smbus_read_byte_data(priv->client,
+						STTS751_REG_TEMP_L);
+		if (frac < 0) {
+			dev_dbg(&priv->client->dev,
+				"I2C 2nd read failed (temp L). ret: %x\n", ret);
+			ret = frac;
+			goto exit;
+		}
+	}
+
+exit:
+	mutex_unlock(&priv->access_lock);
+	if (ret)
+		return ret;
+
+	priv->temp = stts751_to_deg((integer1 << 8) | frac);
+	return ret;
+}
+
+static int stts751_set_temp_reg(struct stts751_priv *priv, int temp,
+				bool is_frac, u8 hreg, u8 lreg)
+{
+	s32 hwval;
+	int ret;
+
+	if (stts751_to_hw(temp, &hwval))
+		return -EINVAL;
+
+	mutex_lock(&priv->access_lock);
+	ret = i2c_smbus_write_byte_data(priv->client, hreg, hwval >> 8);
+	if (ret)
+		goto exit;
+	if (is_frac)
+		ret = i2c_smbus_write_byte_data(priv->client,
+						lreg, hwval & 0xff);
+exit:
+	mutex_unlock(&priv->access_lock);
+
+	return ret;
+}
+
+static int stts751_update_alert(struct stts751_priv *priv)
+{
+	int ret;
+
+	/* not for us.. */
+	if (!priv->gen_event)
+		return 0;
+
+	ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_STATUS);
+
+	if (ret < 0)
+		return ret;
+
+	priv->max_alert = priv->max_alert || !!(ret & STTS751_STATUS_TRIPH);
+	priv->min_alert = priv->min_alert || !!(ret & STTS751_STATUS_TRIPL);
+
+	return 0;
+}
+
+static void stts751_alert(struct i2c_client *client,
+			enum i2c_alert_protocol type, unsigned int data)
+{
+	int ret;
+	struct stts751_priv *priv = i2c_get_clientdata(client);
+	bool prev_max = priv->max_alert;
+	bool prev_min = priv->min_alert;
+
+	if (type != I2C_PROTOCOL_SMBUS_ALERT)
+		return;
+
+	dev_dbg(&client->dev, "alert!");
+
+	mutex_lock(&priv->access_lock);
+	ret = stts751_update_alert(priv);
+	if (ret < 0) {
+		/* default to worst case */
+		priv->max_alert = true;
+		priv->min_alert = true;
+
+		if (!(prev_max && prev_min)) {
+			dev_warn(&priv->client->dev,
+				"Alert received, but can't communicate to the device. Something bad happening? Triggering all alarms!");
+		}
+	}
+
+	if (!prev_max && priv->max_alert) {
+		dev_notice(&client->dev, "got alert for HIGH temperature");
+
+		/* unblock alert poll */
+		sysfs_notify(&priv->dev->kobj, NULL, "temp1_event_max_alert");
+		kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
+	}
+
+	if (!prev_min && priv->min_alert) {
+		dev_notice(&client->dev, "got alert for LOW temperature");
+
+		/* unblock alert poll */
+		sysfs_notify(&priv->dev->kobj, NULL, "temp1_event_min_alert");
+		kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
+	}
+	mutex_unlock(&priv->access_lock);
+}
+
+static ssize_t show_max_alert(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct stts751_priv *priv = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->max_alert);
+}
+
+static ssize_t set_max_alert(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
+{
+	struct stts751_priv *priv = dev_get_drvdata(dev);
+
+	mutex_lock(&priv->access_lock);
+	priv->max_alert = false;
+	mutex_unlock(&priv->access_lock);
+
+	return count;
+}
+
+static ssize_t show_min_alert(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct stts751_priv *priv = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->min_alert);
+}
+
+static ssize_t set_min_alert(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
+{
+	struct stts751_priv *priv = dev_get_drvdata(dev);
+
+	mutex_lock(&priv->access_lock);
+	priv->min_alert = false;
+	mutex_unlock(&priv->access_lock);
+
+	return count;
+}
+
+static ssize_t show_input(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	int ret;
+	int cache_time = STTS751_CACHE_TIME * HZ / 1000;
+	struct stts751_priv *priv = dev_get_drvdata(dev);
+
+	/*
+	 * Adjust the cache time wrt the sample rate. We do 4X in order to get
+	 * a new measure in no more than 1/4 of the sample time (that seemed
+	 * reasonable to me).
+	 */
+	cache_time = stts751_intervals[priv->interval] / 4 * HZ / 1000;
+
+	if (time_after(jiffies,	priv->last_update + cache_time) ||
+		!priv->data_valid) {
+		ret = stts751_update_temp(priv);
+		if (ret)
+			return ret;
+		priv->last_update = jiffies;
+		priv->data_valid = true;
+	}
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->temp);
+}
+
+static ssize_t show_therm(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct stts751_priv *priv = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->therm);
+}
+
+
+static ssize_t set_therm(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
+{
+	int ret;
+	long temp;
+	struct stts751_priv *priv = dev_get_drvdata(dev);
+
+	if (kstrtol(buf, 10, &temp) < 0)
+		return -EINVAL;
+
+	ret = stts751_set_temp_reg(priv, temp, false, STTS751_REG_TLIM, 0);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev, "setting therm %ld", temp);
+
+	priv->therm = temp;
+	return count;
+}
+
+static ssize_t show_hyst(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct stts751_priv *priv = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->hyst);
+}
+
+
+static ssize_t set_hyst(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
+{
+	int ret;
+	long temp;
+	struct stts751_priv *priv = dev_get_drvdata(dev);
+
+	if (kstrtol(buf, 10, &temp) < 0)
+		return -EINVAL;
+
+	ret = stts751_set_temp_reg(priv, temp, false, STTS751_REG_HYST, 0);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev, "setting hyst %ld", temp);
+
+	priv->hyst = temp;
+	return count;
+}
+
+static ssize_t show_max(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct stts751_priv *priv = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_max);
+}
+
+static ssize_t set_max(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
+{
+	int ret;
+	long temp;
+	struct stts751_priv *priv = dev_get_drvdata(dev);
+
+	if (kstrtol(buf, 10, &temp) < 0)
+		return -EINVAL;
+
+	ret = stts751_set_temp_reg(priv, temp, true,
+				STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev, "setting event max %ld", temp);
+	priv->event_max = temp;
+	return count;
+}
+
+static ssize_t show_min(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	struct stts751_priv *priv = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_min);
+}
+
+
+static ssize_t set_min(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
+{
+	int ret;
+	long temp;
+	struct stts751_priv *priv = dev_get_drvdata(dev);
+
+	if (kstrtol(buf, 10, &temp) < 0)
+		return -EINVAL;
+
+	ret = stts751_set_temp_reg(priv, temp, true,
+				STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev, "setting event min %ld", temp);
+
+	priv->event_min = temp;
+	return count;
+}
+
+static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct stts751_priv *priv = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n",
+			stts751_intervals[priv->interval]);
+}
+
+static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	unsigned long val;
+	int idx;
+	int ret = 0;
+	struct stts751_priv *priv = dev_get_drvdata(dev);
+
+	if (kstrtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	idx = find_closest_descending(val, stts751_intervals,
+				ARRAY_SIZE(stts751_intervals));
+
+	dev_dbg(dev, "setting interval. req:%lu, idx: %d, val: %d", val, idx,
+		stts751_intervals[idx]);
+
+	if (priv->interval == idx)
+		return count;
+
+	mutex_lock(&priv->access_lock);
+
+	/* speed up, lower the resolution, then modify convrate */
+	if (priv->interval < idx) {
+		priv->interval = idx;
+		ret = stts751_adjust_resolution(priv);
+		if (ret)
+			goto exit;
+	}
+
+	ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_RATE, idx);
+	if (ret)
+		goto exit;
+
+	/* slow down, modify convrate, then raise resolution */
+	if (priv->interval != idx) {
+		priv->interval = idx;
+		ret = stts751_adjust_resolution(priv);
+		if (ret)
+			goto exit;
+
+	}
+exit:
+	mutex_unlock(&priv->access_lock);
+
+	return count;
+}
+
+static int stts751_detect(struct i2c_client *new_client,
+			  struct i2c_board_info *info)
+{
+	struct i2c_adapter *adapter = new_client->adapter;
+	const char *name;
+	int mfg_id, prod_id, rev_id;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -ENODEV;
+
+	mfg_id = i2c_smbus_read_byte_data(new_client, ST_MAN_ID);
+	if (mfg_id != ST_MAN_ID)
+		return -ENODEV;
+
+	prod_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_PROD_ID);
+
+	switch (prod_id) {
+	case STTS751_0_PROD_ID:
+		name = "STTS751-0";
+		break;
+	case STTS751_1_PROD_ID:
+		name = "STTS751-1";
+		break;
+	default:
+		return -ENODEV;
+	}
+	dev_info(&new_client->dev, "Chip %s detected!", name);
+
+	rev_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_REV_ID);
+
+	if (rev_id != 0x1) {
+		dev_notice(&new_client->dev,
+			"Chip revision 0x%x is untested\nPlease report whether it works to andrea.merello@gmail.com",
+			rev_id);
+	}
+
+	strlcpy(info->type, name, I2C_NAME_SIZE);
+	return 0;
+}
+
+static int stts751_init_chip(struct stts751_priv *priv)
+{
+	int ret;
+
+	priv->config = STTS751_CONF_EVENT_DIS | STTS751_CONF_STOP;
+	ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_CONF,
+					priv->config);
+	if (ret)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_RATE,
+					priv->interval);
+	if (ret)
+		return ret;
+
+	/* invalid, to force update */
+	priv->res = -1;
+	ret = stts751_adjust_resolution(priv);
+	if (ret)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(priv->client,
+					STTS751_REG_SMBUS_TO,
+					priv->smbus_timeout ? 0x80 : 0);
+	if (ret)
+		return ret;
+
+
+	if (priv->gen_event) {
+		ret = stts751_set_temp_reg(priv, priv->event_max, true,
+					STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
+		if (ret)
+			return ret;
+
+		ret = stts751_set_temp_reg(priv, priv->event_min, true,
+					STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);
+		if (ret)
+			return ret;
+		priv->config &= ~STTS751_CONF_EVENT_DIS;
+	}
+
+	if (priv->gen_therm) {
+		ret = stts751_set_temp_reg(priv, priv->therm, false,
+					STTS751_REG_TLIM, 0);
+		if (ret)
+			return ret;
+
+		ret = stts751_set_temp_reg(priv, priv->hyst, false,
+					STTS751_REG_HYST, 0);
+		if (ret)
+			return ret;
+	}
+
+	priv->config &= ~STTS751_CONF_STOP;
+	ret = i2c_smbus_write_byte_data(priv->client,
+					STTS751_REG_CONF, priv->config);
+
+	return ret;
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO,
+			show_min, set_min, 0);
+static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO,
+			show_max, set_max, 0);
+static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO,
+			show_min_alert, set_min_alert, 0);
+static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO,
+			show_max_alert, set_max_alert, 0);
+static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,
+			set_therm, 0);
+static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO, show_hyst,
+			set_hyst, 0);
+static SENSOR_DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO,
+			show_interval, set_interval, 0);
+
+/* always present */
+static struct attribute *stts751_temp_attrs[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	NULL
+};
+
+static struct attribute_group stts751_temp_group = {
+	.attrs = stts751_temp_attrs,
+};
+
+/* present when therm pin or event pin are connected */
+static struct attribute *stts751_interval_attrs[] = {
+	&sensor_dev_attr_update_interval.dev_attr.attr,
+	NULL
+};
+
+static struct attribute_group stts751_interval_group = {
+	.attrs = stts751_interval_attrs,
+};
+
+/* present when event pin is connected */
+static struct attribute *stts751_event_attrs[] = {
+	&sensor_dev_attr_temp1_event_min.dev_attr.attr,
+	&sensor_dev_attr_temp1_event_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_event_min_alert.dev_attr.attr,
+	&sensor_dev_attr_temp1_event_max_alert.dev_attr.attr,
+	NULL
+};
+
+static struct attribute_group stts751_event_group = {
+	.attrs = stts751_event_attrs,
+};
+
+/* present when therm pin is connected */
+static struct attribute *stts751_therm_attrs[] = {
+	&sensor_dev_attr_temp1_therm.dev_attr.attr,
+	&sensor_dev_attr_temp1_therm_hyst.dev_attr.attr,
+	NULL
+};
+
+static struct attribute_group stts751_therm_group = {
+	.attrs = stts751_therm_attrs,
+};
+
+static int stts751_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct stts751_priv *priv;
+	int ret;
+	int groups_idx = 0;
+	struct device_node *np = client->dev.of_node;
+
+	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->client = client;
+	i2c_set_clientdata(client, priv);
+	mutex_init(&priv->access_lock);
+
+	/* default to 2 samples per second */
+	priv->interval = 5;
+	/* default to timeout enable, as per chip default */
+	priv->smbus_timeout = true;
+	priv->last_update = 0;
+	priv->data_valid = false;
+	priv->max_alert = false;
+	priv->min_alert = false;
+	priv->gen_therm = false;
+	priv->gen_event = false;
+	priv->therm = STTS751_THERM_DEFAULT;
+	priv->hyst = STTS751_HYST_DEFAULT;
+	priv->event_max = STTS751_EVENT_MAX_DEFAULT;
+	priv->event_min = STTS751_EVENT_MIN_DEFAULT;
+
+	if (np) {
+		priv->gen_therm = of_property_read_bool(np, "has-therm");
+		priv->gen_event = of_property_read_bool(np, "has-event");
+		priv->smbus_timeout = !of_property_read_bool(np,
+						"smbus-timeout-disable");
+	} else {
+		dev_notice(&client->dev, "No DT data. Event/therm disabled\n");
+	}
+
+	dev_dbg(&client->dev, "gen_event: %s, gen_therm: %s",
+		priv->gen_event ? "YES" : "NO",
+		priv->gen_therm ? "YES" : "NO");
+
+	priv->groups[groups_idx++] = &stts751_temp_group;
+	priv->groups[groups_idx++] = &stts751_interval_group;
+
+	if (priv->gen_therm)
+		priv->groups[groups_idx++] = &stts751_therm_group;
+
+	if (priv->gen_event)
+		priv->groups[groups_idx++] = &stts751_event_group;
+
+	priv->groups[groups_idx] = NULL;
+
+	ret = stts751_init_chip(priv);
+	if (ret)
+		return ret;
+
+	priv->dev = devm_hwmon_device_register_with_groups(&client->dev,
+							client->name, priv,
+							priv->groups);
+	return PTR_ERR_OR_ZERO(priv->dev);
+}
+
+static const struct i2c_device_id stts751_id[] = {
+	{ "stts751", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, stts751_id);
+
+static struct i2c_driver stts751_driver = {
+	.class		= I2C_CLASS_HWMON,
+	.driver = {
+		.name	= DEVNAME,
+	},
+	.probe		= stts751_probe,
+	.id_table	= stts751_id,
+	.detect		= stts751_detect,
+	.alert		= stts751_alert,
+	.address_list	= normal_i2c,
+};
+
+module_i2c_driver(stts751_driver);
+
+MODULE_AUTHOR("Andrea Merello <andrea.merello@gmail.com>");
+MODULE_DESCRIPTION("STTS751 sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4

Re: [PATCH v2 1/2] DT: add binding documentation for STTS751

[Guenter Roeck]

On Tue, Sep 27, 2016 at 02:07:26PM +0200, Andrea Merello wrote:
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  Documentation/devicetree/bindings/hwmon/stts751.txt | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/stts751.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/stts751.txt b/Documentation/devicetree/bindings/hwmon/stts751.txt
> new file mode 100644
> index 0000000..277ad46
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/stts751.txt
> @@ -0,0 +1,19 @@
> +* STTS751 thermometer.
> +
> +Required node properties:
> +- compatible: "stts751"
> +- reg: I2C bus address of the device
> +
> +Optional properties:
> +- has-therm: specifies that the "therm" pin is wired
> +- has-event: specifies that the "event" pin is wired

As mentioned in the code patch, I don't think those properties are actually
needed.

Guenter

> +- smbus-timeout-disable: when set, the smbus timeout function will be disabled
> +
> +Example stts751 node:
> +
> +temp-sensor {
> +	compatible = "stts751";
> +	reg = <0x48>;
> +	has-therm;
> +	has-event;
> +}
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] hwmon: new driver for ST stts751 thermal sensor

[Andrea Merello]

On Fri, Nov 4, 2016 at 5:01 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Fri, Nov 04, 2016 at 04:43:46PM +0100, Andrea Merello wrote:
>> On Fri, Nov 4, 2016 at 4:09 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> > On Fri, Nov 04, 2016 at 09:24:25AM +0100, Andrea Merello wrote:
>> >> On Thu, Nov 3, 2016 at 10:16 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> >> > On Thu, Nov 03, 2016 at 08:35:27AM +0100, Andrea Merello wrote:
>> >> >> On Wed, Nov 2, 2016 at 11:48 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> >> >> > On Wed, Nov 02, 2016 at 09:11:35AM +0100, Andrea Merello wrote:
>> >> >> >> On Mon, Oct 31, 2016 at 5:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> >> >> >> > On 10/31/2016 08:27 AM, Andrea Merello wrote:
>> >> >> >> >
>> >> >> >> >>>>
>> >> >> >> >>>>>> +     if (ret < 0)
>> >> >> >> >>>>>> +             return ret;
>> >> >> >> >>>>>> +
>> >> >> >> >>>>>> +     priv->max_alert = priv->max_alert || !!(ret &
>> >> >> >> >>>>>> STTS751_STATUS_TRIPH);
>> >> >> >> >>>>>> +     priv->min_alert = priv->min_alert || !!(ret &
>> >> >> >> >>>>>> STTS751_STATUS_TRIPL);
>> >> >> >> >>>>>> +
>> >> >> >> >>>>>
>> >> >> >> >>>>>
>> >> >> >> >>>>> I am not really happy about the caching of alert values. This isn't
>> >> >> >> >>>>> really
>> >> >> >> >>>>> common. Usually we report current alert values when reading. I don't
>> >> >> >> >>>>> really
>> >> >> >> >>>>> see why this driver should be different to all the others.
>> >> >> >> >>>>
>> >> >> >> >>>>
>> >> >> >> >>>>
>> >> >> >> >>>> OK
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> I've tried to avoid caching, but as soon as I read the status register
>> >> >> >> >> in the alert handler, the device clears that reg, and it will remain
>> >> >> >> >> cleared up to the next conversion.
>> >> >> >> >>
>> >> >> >> >> So for example if the reader is blocked on poll over the alert
>> >> >> >> >> attribute, it gets unblocked on alert, and it will very likely read
>> >> >> >> >> zero..
>> >> >> >> >>
>> >> >> >> >> The only way I can see for avoiding caching, and for changing the API
>> >> >> >> >> about clearing events as you asked below, is performing a temperature
>> >> >> >> >> read from the temperature registers and doing software compare with
>> >> >> >> >> the limits in show_[max/min]_alert.
>> >> >> >> >>
>> >> >> >> >> Does this sounds OK for you ?
>> >> >> >> >>
>> >> >> >> >
>> >> >> >> > Not really. That could be done from user space by just reading the
>> >> >> >> > temperature
>> >> >> >> > and comparing it against limits.
>> >> >> >>
>> >> >> >> Sorry, I don't see the point here.. Why the fact that something could
>> >> >> >> be done in userspace should suggest we don't want to do it in the
>> >> >> >> driver? We do, for example, a simple calculation to give an absolute
>> >> >> >> value for the hysteresis; we could do this also in userspace, but we
>> >> >> >> do this in the driver in order to support the proper API..
>> >> >> >>
>> >> >> > Datasheet says:
>> >> >> >
>> >> >> > T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A >
>> >> >> > high limit). T HIGH is cleared when the status register is read, provided
>> >> >> > the condition no longer exists.
>> >> >> > T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low
>> >> >> > limit). T LOW is cleared when the status register is read, provided the
>> >> >> > condition no longer exists.
>> >> >> >
>> >> >> > This isn't exactly what you are claiming, since it says "cleared ... provided
>> >> >> > that the condition no longer exists", which would be more in line with other
>> >> >> > chips.
>> >> >>
>> >> >> Yes, I know.. I've also read that on the datasheet.. And I tried
>> >> >> reading the status register in both the alert handler and the show
>> >> >> function, but it didn't worked :/
>> >> >>
>> >> >> So I started to suspect the status register got actually clear after
>> >> >> reading up to the next conversion, and to test this, I simply read it
>> >> >> twice in the alert handler. The first read always read 1, as expected;
>> >> >> the second time I always got zero, even if the temperature was still
>> >> >> over-limit.. And I soon got another event, that behaved in the same
>> >> >> manner.
>> >> >>
>> >> > Question is if the event bit is set again after the next conversion; this would
>> >> > be relatively easy to handle (cache the status register for one conversion
>> >> > time).
>> >>
>> >> Yes, I meant the HW does exactly this. But it seems to me that caching
>> >> could end up in subtle races here..
>> >>
>> >> For example suppose that user is reading the lower limit alarm file,
>> >> or therm, and this will trigger read of the status reg; but a
>> >> conversion is just ended and set the high limit alarm just before
>> >> reading the status reg. Then the event handler is invoked but it will
>> >> find out the status reg to be (incorrectly) zero, (or rely on a value
>> >> cached when reading the lower limit, that was zero); thus a thread
>> >> blocked on poll on the high alarm would not be woken up. (ok, this
>> >> probably will happen in next conversions, but this doesn't sound quite
>> >> sane to me..).
>> >>
>> > Good catch and thinking, but there is a workaround. If the cache time
>> > has not expired, just logically or the cached status value with the current
>> > value in the alert function, and update the time stamp. This way nothing
>> > gets lost.
>>
>> Hmm, it seems OK; thanks for suggesting this :) I'll try to go this way..
>>
>> BTW I would say that the cache time for the alert should be
>> coservatively a bit longer than the conversion time (to avoid early
>> expiration when the status register has been not restored yet). On the
>> other hand for the temperature reading I'm using a cache time that is
>> shorter (I think it is 1/4) than the conversion interval, so that the
>> user gets a reasonably fresh value (becasue the chip sampling freq is
>> asyncrhronous wrt the reads we do). Do you think it's worth to keep
>> doing this (so we have two different cache time for alarms and temp
>> value), or do you think that it's pointless, and better to unify it
>> (at the longer one, I would say)?
>>
>
> I would suggest to use a single cache time, in line with the real cache time,
> using the maximum as per the datasheet and possibly rounded up. If the user
> wants faster readings, that can be made configurable using the 'update-interval'
> attribute. If the chip is configured to only read the temperature once per,
> say, 16 seconds, this is what the user wanted, and real-time accuracy is not
> guaranteed anyway.

Ok, I'll come back with a new version of the patch changed as discussed.

Thanks
Andrea

>
>> >>
>> >> >> (also I'm thinking about a way to mask events once we know that we are
>> >> >> in a out-of-limit condition, to avoid having a lot of them, but simply
>> >> >> disabling events and re-enabling them after userspace reads the alarm
>> >> >> and find it to be zero, like lm90 driver does, IMHO wouldn't be good
>> >> >> here, because we have a single bit to disable both high and low
>> >> >> limits..)
>> >> >>
>> >> >> > The THRM bit doesn't seem to self-clean either, at least not according to the
>> >> >> > datasheet.
>> >> >>
>> >> >> I need to test this.. But this doesn't generate any event, it seems
>> >> >> only inteded to drive an external circuit, say a fan, and not to
>> >> >> notify SW.. And currently I even have no show function to read it's
>> >> >> status (now that I'm on it, I think I should add it...)..
>> >> >>
>> >> > Yes. Note that this pin might be wired to an interrupt.
>> >>
>> >> IMHO it isn't straightforward to handle this..
>> >>
>> >> AFAIK what is done by the smbus code to allow handling smbus events
>> >> lines with regular interrupt lines is to briefly disabling the IRQ
>> >> line until the event can be acknowledged with the ARA (in non atomic
>> >> context).
>> >>
>> >> But here is even worse because there is no ARA; it seems that the
>> >> therm line can't be "cleared" at all.
>> >>
>> >> Maybe it's possible to do hacks similar to what the smbus code does,
>> >> but keeping the IRQ line disabled (given that it must not be shared)..
>> >> Maybe if the IRQ controller could be set in edge-triggered mode it
>> >> would be easier..
>> >>
>> >> But does it really worth struggling supporting this scenario? There's
>> >> the event pin explicitly designed for events, and there is code that
>> >> make it possible to use this even with regular IRQs.. Using also the
>> >> therm pin as interrupt seems honestly a bit pointless and
>> >> inappropriate..
>> >>
>> > Correct, one would use an edge triggered interrupt in this case. But
>> > you are right, this should only be implemented if that use case is
>> > actually encountered.
>> >
>> > Thanks,
>> > Guenter

Re: [PATCH v2 2/2] hwmon: new driver for ST stts751 thermal sensor

[Guenter Roeck]

On Fri, Nov 04, 2016 at 04:43:46PM +0100, Andrea Merello wrote:
> On Fri, Nov 4, 2016 at 4:09 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On Fri, Nov 04, 2016 at 09:24:25AM +0100, Andrea Merello wrote:
> >> On Thu, Nov 3, 2016 at 10:16 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> >> > On Thu, Nov 03, 2016 at 08:35:27AM +0100, Andrea Merello wrote:
> >> >> On Wed, Nov 2, 2016 at 11:48 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> >> >> > On Wed, Nov 02, 2016 at 09:11:35AM +0100, Andrea Merello wrote:
> >> >> >> On Mon, Oct 31, 2016 at 5:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> >> >> >> > On 10/31/2016 08:27 AM, Andrea Merello wrote:
> >> >> >> >
> >> >> >> >>>>
> >> >> >> >>>>>> +     if (ret < 0)
> >> >> >> >>>>>> +             return ret;
> >> >> >> >>>>>> +
> >> >> >> >>>>>> +     priv->max_alert = priv->max_alert || !!(ret &
> >> >> >> >>>>>> STTS751_STATUS_TRIPH);
> >> >> >> >>>>>> +     priv->min_alert = priv->min_alert || !!(ret &
> >> >> >> >>>>>> STTS751_STATUS_TRIPL);
> >> >> >> >>>>>> +
> >> >> >> >>>>>
> >> >> >> >>>>>
> >> >> >> >>>>> I am not really happy about the caching of alert values. This isn't
> >> >> >> >>>>> really
> >> >> >> >>>>> common. Usually we report current alert values when reading. I don't
> >> >> >> >>>>> really
> >> >> >> >>>>> see why this driver should be different to all the others.
> >> >> >> >>>>
> >> >> >> >>>>
> >> >> >> >>>>
> >> >> >> >>>> OK
> >> >> >> >>
> >> >> >> >>
> >> >> >> >> I've tried to avoid caching, but as soon as I read the status register
> >> >> >> >> in the alert handler, the device clears that reg, and it will remain
> >> >> >> >> cleared up to the next conversion.
> >> >> >> >>
> >> >> >> >> So for example if the reader is blocked on poll over the alert
> >> >> >> >> attribute, it gets unblocked on alert, and it will very likely read
> >> >> >> >> zero..
> >> >> >> >>
> >> >> >> >> The only way I can see for avoiding caching, and for changing the API
> >> >> >> >> about clearing events as you asked below, is performing a temperature
> >> >> >> >> read from the temperature registers and doing software compare with
> >> >> >> >> the limits in show_[max/min]_alert.
> >> >> >> >>
> >> >> >> >> Does this sounds OK for you ?
> >> >> >> >>
> >> >> >> >
> >> >> >> > Not really. That could be done from user space by just reading the
> >> >> >> > temperature
> >> >> >> > and comparing it against limits.
> >> >> >>
> >> >> >> Sorry, I don't see the point here.. Why the fact that something could
> >> >> >> be done in userspace should suggest we don't want to do it in the
> >> >> >> driver? We do, for example, a simple calculation to give an absolute
> >> >> >> value for the hysteresis; we could do this also in userspace, but we
> >> >> >> do this in the driver in order to support the proper API..
> >> >> >>
> >> >> > Datasheet says:
> >> >> >
> >> >> > T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A >
> >> >> > high limit). T HIGH is cleared when the status register is read, provided
> >> >> > the condition no longer exists.
> >> >> > T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low
> >> >> > limit). T LOW is cleared when the status register is read, provided the
> >> >> > condition no longer exists.
> >> >> >
> >> >> > This isn't exactly what you are claiming, since it says "cleared ... provided
> >> >> > that the condition no longer exists", which would be more in line with other
> >> >> > chips.
> >> >>
> >> >> Yes, I know.. I've also read that on the datasheet.. And I tried
> >> >> reading the status register in both the alert handler and the show
> >> >> function, but it didn't worked :/
> >> >>
> >> >> So I started to suspect the status register got actually clear after
> >> >> reading up to the next conversion, and to test this, I simply read it
> >> >> twice in the alert handler. The first read always read 1, as expected;
> >> >> the second time I always got zero, even if the temperature was still
> >> >> over-limit.. And I soon got another event, that behaved in the same
> >> >> manner.
> >> >>
> >> > Question is if the event bit is set again after the next conversion; this would
> >> > be relatively easy to handle (cache the status register for one conversion
> >> > time).
> >>
> >> Yes, I meant the HW does exactly this. But it seems to me that caching
> >> could end up in subtle races here..
> >>
> >> For example suppose that user is reading the lower limit alarm file,
> >> or therm, and this will trigger read of the status reg; but a
> >> conversion is just ended and set the high limit alarm just before
> >> reading the status reg. Then the event handler is invoked but it will
> >> find out the status reg to be (incorrectly) zero, (or rely on a value
> >> cached when reading the lower limit, that was zero); thus a thread
> >> blocked on poll on the high alarm would not be woken up. (ok, this
> >> probably will happen in next conversions, but this doesn't sound quite
> >> sane to me..).
> >>
> > Good catch and thinking, but there is a workaround. If the cache time
> > has not expired, just logically or the cached status value with the current
> > value in the alert function, and update the time stamp. This way nothing
> > gets lost.
> 
> Hmm, it seems OK; thanks for suggesting this :) I'll try to go this way..
> 
> BTW I would say that the cache time for the alert should be
> coservatively a bit longer than the conversion time (to avoid early
> expiration when the status register has been not restored yet). On the
> other hand for the temperature reading I'm using a cache time that is
> shorter (I think it is 1/4) than the conversion interval, so that the
> user gets a reasonably fresh value (becasue the chip sampling freq is
> asyncrhronous wrt the reads we do). Do you think it's worth to keep
> doing this (so we have two different cache time for alarms and temp
> value), or do you think that it's pointless, and better to unify it
> (at the longer one, I would say)?
> 

I would suggest to use a single cache time, in line with the real cache time,
using the maximum as per the datasheet and possibly rounded up. If the user
wants faster readings, that can be made configurable using the 'update-interval'
attribute. If the chip is configured to only read the temperature once per,
say, 16 seconds, this is what the user wanted, and real-time accuracy is not
guaranteed anyway.

> >>
> >> >> (also I'm thinking about a way to mask events once we know that we are
> >> >> in a out-of-limit condition, to avoid having a lot of them, but simply
> >> >> disabling events and re-enabling them after userspace reads the alarm
> >> >> and find it to be zero, like lm90 driver does, IMHO wouldn't be good
> >> >> here, because we have a single bit to disable both high and low
> >> >> limits..)
> >> >>
> >> >> > The THRM bit doesn't seem to self-clean either, at least not according to the
> >> >> > datasheet.
> >> >>
> >> >> I need to test this.. But this doesn't generate any event, it seems
> >> >> only inteded to drive an external circuit, say a fan, and not to
> >> >> notify SW.. And currently I even have no show function to read it's
> >> >> status (now that I'm on it, I think I should add it...)..
> >> >>
> >> > Yes. Note that this pin might be wired to an interrupt.
> >>
> >> IMHO it isn't straightforward to handle this..
> >>
> >> AFAIK what is done by the smbus code to allow handling smbus events
> >> lines with regular interrupt lines is to briefly disabling the IRQ
> >> line until the event can be acknowledged with the ARA (in non atomic
> >> context).
> >>
> >> But here is even worse because there is no ARA; it seems that the
> >> therm line can't be "cleared" at all.
> >>
> >> Maybe it's possible to do hacks similar to what the smbus code does,
> >> but keeping the IRQ line disabled (given that it must not be shared)..
> >> Maybe if the IRQ controller could be set in edge-triggered mode it
> >> would be easier..
> >>
> >> But does it really worth struggling supporting this scenario? There's
> >> the event pin explicitly designed for events, and there is code that
> >> make it possible to use this even with regular IRQs.. Using also the
> >> therm pin as interrupt seems honestly a bit pointless and
> >> inappropriate..
> >>
> > Correct, one would use an edge triggered interrupt in this case. But
> > you are right, this should only be implemented if that use case is
> > actually encountered.
> >
> > Thanks,
> > Guenter

Re: [PATCH v2 2/2] hwmon: new driver for ST stts751 thermal sensor

[Andrea Merello]

On Fri, Nov 4, 2016 at 4:09 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Fri, Nov 04, 2016 at 09:24:25AM +0100, Andrea Merello wrote:
>> On Thu, Nov 3, 2016 at 10:16 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> > On Thu, Nov 03, 2016 at 08:35:27AM +0100, Andrea Merello wrote:
>> >> On Wed, Nov 2, 2016 at 11:48 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> >> > On Wed, Nov 02, 2016 at 09:11:35AM +0100, Andrea Merello wrote:
>> >> >> On Mon, Oct 31, 2016 at 5:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> >> >> > On 10/31/2016 08:27 AM, Andrea Merello wrote:
>> >> >> >
>> >> >> >>>>
>> >> >> >>>>>> +     if (ret < 0)
>> >> >> >>>>>> +             return ret;
>> >> >> >>>>>> +
>> >> >> >>>>>> +     priv->max_alert = priv->max_alert || !!(ret &
>> >> >> >>>>>> STTS751_STATUS_TRIPH);
>> >> >> >>>>>> +     priv->min_alert = priv->min_alert || !!(ret &
>> >> >> >>>>>> STTS751_STATUS_TRIPL);
>> >> >> >>>>>> +
>> >> >> >>>>>
>> >> >> >>>>>
>> >> >> >>>>> I am not really happy about the caching of alert values. This isn't
>> >> >> >>>>> really
>> >> >> >>>>> common. Usually we report current alert values when reading. I don't
>> >> >> >>>>> really
>> >> >> >>>>> see why this driver should be different to all the others.
>> >> >> >>>>
>> >> >> >>>>
>> >> >> >>>>
>> >> >> >>>> OK
>> >> >> >>
>> >> >> >>
>> >> >> >> I've tried to avoid caching, but as soon as I read the status register
>> >> >> >> in the alert handler, the device clears that reg, and it will remain
>> >> >> >> cleared up to the next conversion.
>> >> >> >>
>> >> >> >> So for example if the reader is blocked on poll over the alert
>> >> >> >> attribute, it gets unblocked on alert, and it will very likely read
>> >> >> >> zero..
>> >> >> >>
>> >> >> >> The only way I can see for avoiding caching, and for changing the API
>> >> >> >> about clearing events as you asked below, is performing a temperature
>> >> >> >> read from the temperature registers and doing software compare with
>> >> >> >> the limits in show_[max/min]_alert.
>> >> >> >>
>> >> >> >> Does this sounds OK for you ?
>> >> >> >>
>> >> >> >
>> >> >> > Not really. That could be done from user space by just reading the
>> >> >> > temperature
>> >> >> > and comparing it against limits.
>> >> >>
>> >> >> Sorry, I don't see the point here.. Why the fact that something could
>> >> >> be done in userspace should suggest we don't want to do it in the
>> >> >> driver? We do, for example, a simple calculation to give an absolute
>> >> >> value for the hysteresis; we could do this also in userspace, but we
>> >> >> do this in the driver in order to support the proper API..
>> >> >>
>> >> > Datasheet says:
>> >> >
>> >> > T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A >
>> >> > high limit). T HIGH is cleared when the status register is read, provided
>> >> > the condition no longer exists.
>> >> > T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low
>> >> > limit). T LOW is cleared when the status register is read, provided the
>> >> > condition no longer exists.
>> >> >
>> >> > This isn't exactly what you are claiming, since it says "cleared ... provided
>> >> > that the condition no longer exists", which would be more in line with other
>> >> > chips.
>> >>
>> >> Yes, I know.. I've also read that on the datasheet.. And I tried
>> >> reading the status register in both the alert handler and the show
>> >> function, but it didn't worked :/
>> >>
>> >> So I started to suspect the status register got actually clear after
>> >> reading up to the next conversion, and to test this, I simply read it
>> >> twice in the alert handler. The first read always read 1, as expected;
>> >> the second time I always got zero, even if the temperature was still
>> >> over-limit.. And I soon got another event, that behaved in the same
>> >> manner.
>> >>
>> > Question is if the event bit is set again after the next conversion; this would
>> > be relatively easy to handle (cache the status register for one conversion
>> > time).
>>
>> Yes, I meant the HW does exactly this. But it seems to me that caching
>> could end up in subtle races here..
>>
>> For example suppose that user is reading the lower limit alarm file,
>> or therm, and this will trigger read of the status reg; but a
>> conversion is just ended and set the high limit alarm just before
>> reading the status reg. Then the event handler is invoked but it will
>> find out the status reg to be (incorrectly) zero, (or rely on a value
>> cached when reading the lower limit, that was zero); thus a thread
>> blocked on poll on the high alarm would not be woken up. (ok, this
>> probably will happen in next conversions, but this doesn't sound quite
>> sane to me..).
>>
> Good catch and thinking, but there is a workaround. If the cache time
> has not expired, just logically or the cached status value with the current
> value in the alert function, and update the time stamp. This way nothing
> gets lost.

Hmm, it seems OK; thanks for suggesting this :) I'll try to go this way..

BTW I would say that the cache time for the alert should be
coservatively a bit longer than the conversion time (to avoid early
expiration when the status register has been not restored yet). On the
other hand for the temperature reading I'm using a cache time that is
shorter (I think it is 1/4) than the conversion interval, so that the
user gets a reasonably fresh value (becasue the chip sampling freq is
asyncrhronous wrt the reads we do). Do you think it's worth to keep
doing this (so we have two different cache time for alarms and temp
value), or do you think that it's pointless, and better to unify it
(at the longer one, I would say)?

>>
>> >> (also I'm thinking about a way to mask events once we know that we are
>> >> in a out-of-limit condition, to avoid having a lot of them, but simply
>> >> disabling events and re-enabling them after userspace reads the alarm
>> >> and find it to be zero, like lm90 driver does, IMHO wouldn't be good
>> >> here, because we have a single bit to disable both high and low
>> >> limits..)
>> >>
>> >> > The THRM bit doesn't seem to self-clean either, at least not according to the
>> >> > datasheet.
>> >>
>> >> I need to test this.. But this doesn't generate any event, it seems
>> >> only inteded to drive an external circuit, say a fan, and not to
>> >> notify SW.. And currently I even have no show function to read it's
>> >> status (now that I'm on it, I think I should add it...)..
>> >>
>> > Yes. Note that this pin might be wired to an interrupt.
>>
>> IMHO it isn't straightforward to handle this..
>>
>> AFAIK what is done by the smbus code to allow handling smbus events
>> lines with regular interrupt lines is to briefly disabling the IRQ
>> line until the event can be acknowledged with the ARA (in non atomic
>> context).
>>
>> But here is even worse because there is no ARA; it seems that the
>> therm line can't be "cleared" at all.
>>
>> Maybe it's possible to do hacks similar to what the smbus code does,
>> but keeping the IRQ line disabled (given that it must not be shared)..
>> Maybe if the IRQ controller could be set in edge-triggered mode it
>> would be easier..
>>
>> But does it really worth struggling supporting this scenario? There's
>> the event pin explicitly designed for events, and there is code that
>> make it possible to use this even with regular IRQs.. Using also the
>> therm pin as interrupt seems honestly a bit pointless and
>> inappropriate..
>>
> Correct, one would use an edge triggered interrupt in this case. But
> you are right, this should only be implemented if that use case is
> actually encountered.
>
> Thanks,
> Guenter

Re: [PATCH v2 2/2] hwmon: new driver for ST stts751 thermal sensor

[Guenter Roeck]

On Fri, Nov 04, 2016 at 09:24:25AM +0100, Andrea Merello wrote:
> On Thu, Nov 3, 2016 at 10:16 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On Thu, Nov 03, 2016 at 08:35:27AM +0100, Andrea Merello wrote:
> >> On Wed, Nov 2, 2016 at 11:48 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> >> > On Wed, Nov 02, 2016 at 09:11:35AM +0100, Andrea Merello wrote:
> >> >> On Mon, Oct 31, 2016 at 5:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> >> >> > On 10/31/2016 08:27 AM, Andrea Merello wrote:
> >> >> >
> >> >> >>>>
> >> >> >>>>>> +     if (ret < 0)
> >> >> >>>>>> +             return ret;
> >> >> >>>>>> +
> >> >> >>>>>> +     priv->max_alert = priv->max_alert || !!(ret &
> >> >> >>>>>> STTS751_STATUS_TRIPH);
> >> >> >>>>>> +     priv->min_alert = priv->min_alert || !!(ret &
> >> >> >>>>>> STTS751_STATUS_TRIPL);
> >> >> >>>>>> +
> >> >> >>>>>
> >> >> >>>>>
> >> >> >>>>> I am not really happy about the caching of alert values. This isn't
> >> >> >>>>> really
> >> >> >>>>> common. Usually we report current alert values when reading. I don't
> >> >> >>>>> really
> >> >> >>>>> see why this driver should be different to all the others.
> >> >> >>>>
> >> >> >>>>
> >> >> >>>>
> >> >> >>>> OK
> >> >> >>
> >> >> >>
> >> >> >> I've tried to avoid caching, but as soon as I read the status register
> >> >> >> in the alert handler, the device clears that reg, and it will remain
> >> >> >> cleared up to the next conversion.
> >> >> >>
> >> >> >> So for example if the reader is blocked on poll over the alert
> >> >> >> attribute, it gets unblocked on alert, and it will very likely read
> >> >> >> zero..
> >> >> >>
> >> >> >> The only way I can see for avoiding caching, and for changing the API
> >> >> >> about clearing events as you asked below, is performing a temperature
> >> >> >> read from the temperature registers and doing software compare with
> >> >> >> the limits in show_[max/min]_alert.
> >> >> >>
> >> >> >> Does this sounds OK for you ?
> >> >> >>
> >> >> >
> >> >> > Not really. That could be done from user space by just reading the
> >> >> > temperature
> >> >> > and comparing it against limits.
> >> >>
> >> >> Sorry, I don't see the point here.. Why the fact that something could
> >> >> be done in userspace should suggest we don't want to do it in the
> >> >> driver? We do, for example, a simple calculation to give an absolute
> >> >> value for the hysteresis; we could do this also in userspace, but we
> >> >> do this in the driver in order to support the proper API..
> >> >>
> >> > Datasheet says:
> >> >
> >> > T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A >
> >> > high limit). T HIGH is cleared when the status register is read, provided
> >> > the condition no longer exists.
> >> > T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low
> >> > limit). T LOW is cleared when the status register is read, provided the
> >> > condition no longer exists.
> >> >
> >> > This isn't exactly what you are claiming, since it says "cleared ... provided
> >> > that the condition no longer exists", which would be more in line with other
> >> > chips.
> >>
> >> Yes, I know.. I've also read that on the datasheet.. And I tried
> >> reading the status register in both the alert handler and the show
> >> function, but it didn't worked :/
> >>
> >> So I started to suspect the status register got actually clear after
> >> reading up to the next conversion, and to test this, I simply read it
> >> twice in the alert handler. The first read always read 1, as expected;
> >> the second time I always got zero, even if the temperature was still
> >> over-limit.. And I soon got another event, that behaved in the same
> >> manner.
> >>
> > Question is if the event bit is set again after the next conversion; this would
> > be relatively easy to handle (cache the status register for one conversion
> > time).
> 
> Yes, I meant the HW does exactly this. But it seems to me that caching
> could end up in subtle races here..
> 
> For example suppose that user is reading the lower limit alarm file,
> or therm, and this will trigger read of the status reg; but a
> conversion is just ended and set the high limit alarm just before
> reading the status reg. Then the event handler is invoked but it will
> find out the status reg to be (incorrectly) zero, (or rely on a value
> cached when reading the lower limit, that was zero); thus a thread
> blocked on poll on the high alarm would not be woken up. (ok, this
> probably will happen in next conversions, but this doesn't sound quite
> sane to me..).
> 
Good catch and thinking, but there is a workaround. If the cache time
has not expired, just logically or the cached status value with the current
value in the alert function, and update the time stamp. This way nothing
gets lost.

> 
> >> (also I'm thinking about a way to mask events once we know that we are
> >> in a out-of-limit condition, to avoid having a lot of them, but simply
> >> disabling events and re-enabling them after userspace reads the alarm
> >> and find it to be zero, like lm90 driver does, IMHO wouldn't be good
> >> here, because we have a single bit to disable both high and low
> >> limits..)
> >>
> >> > The THRM bit doesn't seem to self-clean either, at least not according to the
> >> > datasheet.
> >>
> >> I need to test this.. But this doesn't generate any event, it seems
> >> only inteded to drive an external circuit, say a fan, and not to
> >> notify SW.. And currently I even have no show function to read it's
> >> status (now that I'm on it, I think I should add it...)..
> >>
> > Yes. Note that this pin might be wired to an interrupt.
> 
> IMHO it isn't straightforward to handle this..
> 
> AFAIK what is done by the smbus code to allow handling smbus events
> lines with regular interrupt lines is to briefly disabling the IRQ
> line until the event can be acknowledged with the ARA (in non atomic
> context).
> 
> But here is even worse because there is no ARA; it seems that the
> therm line can't be "cleared" at all.
> 
> Maybe it's possible to do hacks similar to what the smbus code does,
> but keeping the IRQ line disabled (given that it must not be shared)..
> Maybe if the IRQ controller could be set in edge-triggered mode it
> would be easier..
> 
> But does it really worth struggling supporting this scenario? There's
> the event pin explicitly designed for events, and there is code that
> make it possible to use this even with regular IRQs.. Using also the
> therm pin as interrupt seems honestly a bit pointless and
> inappropriate..
> 
Correct, one would use an edge triggered interrupt in this case. But
you are right, this should only be implemented if that use case is
actually encountered.

Thanks,
Guenter

Re: [PATCH v2 2/2] hwmon: new driver for ST stts751 thermal sensor

[Andrea Merello]

On Thu, Nov 3, 2016 at 10:16 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Thu, Nov 03, 2016 at 08:35:27AM +0100, Andrea Merello wrote:
>> On Wed, Nov 2, 2016 at 11:48 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> > On Wed, Nov 02, 2016 at 09:11:35AM +0100, Andrea Merello wrote:
>> >> On Mon, Oct 31, 2016 at 5:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> >> > On 10/31/2016 08:27 AM, Andrea Merello wrote:
>> >> >
>> >> >>>>
>> >> >>>>>> +     if (ret < 0)
>> >> >>>>>> +             return ret;
>> >> >>>>>> +
>> >> >>>>>> +     priv->max_alert = priv->max_alert || !!(ret &
>> >> >>>>>> STTS751_STATUS_TRIPH);
>> >> >>>>>> +     priv->min_alert = priv->min_alert || !!(ret &
>> >> >>>>>> STTS751_STATUS_TRIPL);
>> >> >>>>>> +
>> >> >>>>>
>> >> >>>>>
>> >> >>>>> I am not really happy about the caching of alert values. This isn't
>> >> >>>>> really
>> >> >>>>> common. Usually we report current alert values when reading. I don't
>> >> >>>>> really
>> >> >>>>> see why this driver should be different to all the others.
>> >> >>>>
>> >> >>>>
>> >> >>>>
>> >> >>>> OK
>> >> >>
>> >> >>
>> >> >> I've tried to avoid caching, but as soon as I read the status register
>> >> >> in the alert handler, the device clears that reg, and it will remain
>> >> >> cleared up to the next conversion.
>> >> >>
>> >> >> So for example if the reader is blocked on poll over the alert
>> >> >> attribute, it gets unblocked on alert, and it will very likely read
>> >> >> zero..
>> >> >>
>> >> >> The only way I can see for avoiding caching, and for changing the API
>> >> >> about clearing events as you asked below, is performing a temperature
>> >> >> read from the temperature registers and doing software compare with
>> >> >> the limits in show_[max/min]_alert.
>> >> >>
>> >> >> Does this sounds OK for you ?
>> >> >>
>> >> >
>> >> > Not really. That could be done from user space by just reading the
>> >> > temperature
>> >> > and comparing it against limits.
>> >>
>> >> Sorry, I don't see the point here.. Why the fact that something could
>> >> be done in userspace should suggest we don't want to do it in the
>> >> driver? We do, for example, a simple calculation to give an absolute
>> >> value for the hysteresis; we could do this also in userspace, but we
>> >> do this in the driver in order to support the proper API..
>> >>
>> > Datasheet says:
>> >
>> > T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A >
>> > high limit). T HIGH is cleared when the status register is read, provided
>> > the condition no longer exists.
>> > T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low
>> > limit). T LOW is cleared when the status register is read, provided the
>> > condition no longer exists.
>> >
>> > This isn't exactly what you are claiming, since it says "cleared ... provided
>> > that the condition no longer exists", which would be more in line with other
>> > chips.
>>
>> Yes, I know.. I've also read that on the datasheet.. And I tried
>> reading the status register in both the alert handler and the show
>> function, but it didn't worked :/
>>
>> So I started to suspect the status register got actually clear after
>> reading up to the next conversion, and to test this, I simply read it
>> twice in the alert handler. The first read always read 1, as expected;
>> the second time I always got zero, even if the temperature was still
>> over-limit.. And I soon got another event, that behaved in the same
>> manner.
>>
> Question is if the event bit is set again after the next conversion; this would
> be relatively easy to handle (cache the status register for one conversion
> time).

Yes, I meant the HW does exactly this. But it seems to me that caching
could end up in subtle races here..

For example suppose that user is reading the lower limit alarm file,
or therm, and this will trigger read of the status reg; but a
conversion is just ended and set the high limit alarm just before
reading the status reg. Then the event handler is invoked but it will
find out the status reg to be (incorrectly) zero, (or rely on a value
cached when reading the lower limit, that was zero); thus a thread
blocked on poll on the high alarm would not be woken up. (ok, this
probably will happen in next conversions, but this doesn't sound quite
sane to me..).


>> (also I'm thinking about a way to mask events once we know that we are
>> in a out-of-limit condition, to avoid having a lot of them, but simply
>> disabling events and re-enabling them after userspace reads the alarm
>> and find it to be zero, like lm90 driver does, IMHO wouldn't be good
>> here, because we have a single bit to disable both high and low
>> limits..)
>>
>> > The THRM bit doesn't seem to self-clean either, at least not according to the
>> > datasheet.
>>
>> I need to test this.. But this doesn't generate any event, it seems
>> only inteded to drive an external circuit, say a fan, and not to
>> notify SW.. And currently I even have no show function to read it's
>> status (now that I'm on it, I think I should add it...)..
>>
> Yes. Note that this pin might be wired to an interrupt.

IMHO it isn't straightforward to handle this..

AFAIK what is done by the smbus code to allow handling smbus events
lines with regular interrupt lines is to briefly disabling the IRQ
line until the event can be acknowledged with the ARA (in non atomic
context).

But here is even worse because there is no ARA; it seems that the
therm line can't be "cleared" at all.

Maybe it's possible to do hacks similar to what the smbus code does,
but keeping the IRQ line disabled (given that it must not be shared)..
Maybe if the IRQ controller could be set in edge-triggered mode it
would be easier..

But does it really worth struggling supporting this scenario? There's
the event pin explicitly designed for events, and there is code that
make it possible to use this even with regular IRQs.. Using also the
therm pin as interrupt seems honestly a bit pointless and
inappropriate..

>
>> If I add it, and this func reads the status reg, then it will end up
>> in racy clears of the other high/low event bits anyway, since it's all
>> in the same reg..
>>
>> Indeed it seems to me that we can rely on chip automatism only for
>> alarm event generation. And I don't see all that advantage in relying
>> on HW comparators for reporting alarms in the show functions; IMHO
>> there we can do our comparisons in SW without any real issue..
>>
>> >
>> > This means I'll need to get a sample and/or eval board to test this myself.
>>
>> If you want to write your own tests, I will be happy to make them run
>
> That isn't the point; I'll want to see how the chip really behaves.
> Reading through the datasheet, the most likely behavior is that the status
> bits are cleared on read and set again with the next measurement cycle, as
> already mentioned above. Per datasheet, the alert is supposed to be raised
> again after each measurement cycle if the condition still exists. This behavior
> is actually quite common with other chips. If so, all we would need to do is
> to cache the status register value for one measurement cycle.

Yes, that's what the HW seems to do. I didn't evince that from the
datasheet, but it seems to behave that way :)

> Regarding testing, I'll ultimately write a module test for the driver; see
> https://github.com/groeck/module-tests for details. For that, all I'll need
> is a register dump.

Ok, next time I will be working on that board I'll try this...

> Thanks,
> Guenter
>
>> on my board and give you the results back. Just pass me the test code
>> :)
>>
>> > Guenter
>> >
>> >> > If the chip clears the alert bit on read, why would you need a set attribute
>> >> > to clear it ?
>> >>
>> >> That was used to clear the cached value, not the chip reg.
>> >>
>> >> > From what you are saying, the next alert should update all
>> >> > alert bits, meaning they would be auto-updated at that time. User space
>> >> > would then always read the current state.
>> >> >
>> >> > Sure, you would cache the value
>> >> > in the alert function to be able to read it in the read function, but I
>> >> > don't see why all the complexity around it in your code would be needed.
>> >>
>> >> The next alert is, by definition, an alert, so it will always update
>> >> the cached value with a '1'. We don't have anything that tell us when
>> >> the alert is gone. No one would clear the cached value with '0' when
>> >> the alert is gone. (ok, we have two alert bits, so the _other_ one of
>> >> them will be set to zero, but that's not the point)..
>> >>
>> >> So, in order to report the current alarm value, without relying to the
>> >> user to clear it, as you asked for, caching seems not an option..
>> >>
>> >>
>> >> > You might want to update the alert status in case there is no interrupt
>> >> > support, though (maybe you do already; I didn't re-check the code).
>> >>
>> >> OK, good point, I'll make sure it works event in this case.
>> >>
>> >> > The 'sensors' code doesn't know that it is supposed to write into the
>> >> > alert attribute to clear the status, thus unless you have a special
>> >> > application, an alert would never be cleared. That doesn't sound very
>> >> > appealing to me.
>> >>
>> >> So, OK to change the API as you said, however I don't see how this can
>> >> be done except for doing the manual temperature/limit comparison in
>> >> the driver:
>> >>
>> >> When you get into show_[max/min]_alert function you can't rely on
>> >> reading the status register (because as I said in my prev mail it
>> >> could have been just cleared by reading it in the event handler), but
>> >> as I explained above it seems to me that you cannot also rely on a
>> >> value cached in the event handler with this API, because no one will
>> >> ever clear it..
>> >>
>> >> In other words you will see a status register that could be zero even
>> >> when we have the alarm, or a cached value that is still 1 even if the
>> >> alarm has gone time ago.. I could clear the cached value in the
>> >> show_[max/min]_alert function itself, but this doesn't seems enough to
>> >> me for reporting to the userspace the _current_ alarm value, as you
>> >> asked for.
>> >>
>> >> Or am I missing something ?
>> >>
>> >> >
>> >> >>>>
>> >> >>>>>> +     return 0;
>> >> >>>>>> +}
>> >> >>>>>> +
>> >> >>>>>> +static void stts751_alert(struct i2c_client *client,
>> >> >>>>>> +                     enum i2c_alert_protocol type, unsigned int data)
>> >> >>>>>> +{
>> >> >>>>>> +     int ret;
>> >> >>>>>> +     struct stts751_priv *priv = i2c_get_clientdata(client);
>> >> >>>>>> +     bool prev_max = priv->max_alert;
>> >> >>>>>> +     bool prev_min = priv->min_alert;
>> >> >>>>>> +
>> >> >>>>>> +     if (type != I2C_PROTOCOL_SMBUS_ALERT)
>> >> >>>>>> +             return;
>> >> >>>>>> +
>> >> >>>>>> +     dev_dbg(&client->dev, "alert!");
>> >> >>>>>> +
>> >> >>>>>
>> >> >>>>>
>> >> >>>>>
>> >> >>>>> Any chance to make this a bit more generic and also support
>> >> >>>>> interrupts ?
>> >> >>>>
>> >> >>>>
>> >> >>>>
>> >> >>>> May you please elaborate?
>> >> >>>>
>> >> >>>> IMHO adding the logic for handling SMBus alerts using IRQs to any
>> >> >>>> HWmon driver is redundant, because it is already implemented in the
>> >> >>>> SMBus layer, just right now you need patching SMBus layer for enabling
>> >> >>>> it from the DT world (a couple of patches for this are floating
>> >> >>>> around, but they seems not ready for merge yet)... Using this chip
>> >> >>>> with a generic interrupt is indeed exactly what I'm doing..
>> >> >>>>
>> >> >>>> .. Or am I completely missing your point ?
>> >> >>>>
>> >> >>> "just right now you need patching SMBus layer for enabling it from the DT
>> >> >>> world"
>> >> >>>
>> >> >>> So it _doesn't_ work, correct ?
>> >> >>
>> >> >>
>> >> >> I mean, the generic smbus code works for stts751, and the chip seems
>> >> >> happy. So, in this case, no need to reimplement it elsewhere. It would
>> >> >> just end up to be a copy-paste from smbus.c.... Just AFAICT we need
>> >> >> some mechanism to hook smbus code from DT world..
>> >> >>
>> >> >
>> >> > Ok, then let's just go with it. If it doesn't work in some application we'll
>> >> > have to fix it up later.
>> >> >
>> >> >
>> >> >>>>>> +
>> >> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
>> >> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO,
>> >> >>>>>> +                     show_min, set_min, 0);
>> >> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO,
>> >> >>>>>> +                     show_max, set_max, 0);
>> >> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO,
>> >> >>>>>> +                     show_min_alert, set_min_alert, 0);
>> >> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO,
>> >> >>>>>> +                     show_max_alert, set_max_alert, 0);
>> >> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,
>> >> >>>>>> +                     set_therm, 0);
>> >> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO,
>> >> >>>>>> show_hyst,
>> >> >>>>>> +                     set_hyst, 0);
>> >> >>>>>
>> >> >>>>>
>> >> >>>>>
>> >> >>>>> Please use standard attribute names.
>> >> >>>>
>> >> >>>>
>> >> >>>>
>> >> >>>> I can convert the event stuff in temp1_[min/max]_alarm for limits and
>> >> >>>> temp1_alarm for the flag, but it's not obvious to be what should I use
>> >> >>>> for the therm stuff.. May you please give me advices about this?
>> >> >>>>
>> >> >>>> I'd propose temp1_therm_[max/min]_hyst, but they seems not standard
>> >> >>>> anyway...
>> >> >>>>
>> >> >>>
>> >> >>> From the chip datasheet, this is supposed to reflect overtemperature
>> >> >>> conditions,
>> >> >>> stronger than high/low, thus 'crit' seems appropriate.
>> >> >>
>> >> >>
>> >> >> I see.. I can go with 'crit' :)
>> >> >>
>> >> >> Just is there any place where I can document this? Despite the
>> >> >> datasheet, I can easily imagine real-world usage scenarios, for
>> >> >> example driving a fan from that signal, where 'crit' might be not so
>> >> >> intuitive.. Indeed I would like to make it visible the 'crit'
>> >> >> <->'therm' binding to users. Maybe an additional attribute
>> >> >> 'temp1_crit_label' or something like that ?
>> >> >>
>> >> > You are supposed to provide Documentation/hwmon/stts751, which would
>> >> > include such information.
>> >> >
>> >> > A label applies to a sensor, not to a sensor attribute, so that would
>> >> > not be appropriate.
>> >>
>> >> OK to provide it, but it would be in kernel source tree. Any further
>> >> hint that we could give to someone that looks at in /sys ?
>> >>
>> >> > Thanks,
>> >> > Guenter
>> >> >
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] hwmon: new driver for ST stts751 thermal sensor

[Guenter Roeck]

On Thu, Nov 03, 2016 at 08:35:27AM +0100, Andrea Merello wrote:
> On Wed, Nov 2, 2016 at 11:48 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On Wed, Nov 02, 2016 at 09:11:35AM +0100, Andrea Merello wrote:
> >> On Mon, Oct 31, 2016 at 5:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> >> > On 10/31/2016 08:27 AM, Andrea Merello wrote:
> >> >
> >> >>>>
> >> >>>>>> +     if (ret < 0)
> >> >>>>>> +             return ret;
> >> >>>>>> +
> >> >>>>>> +     priv->max_alert = priv->max_alert || !!(ret &
> >> >>>>>> STTS751_STATUS_TRIPH);
> >> >>>>>> +     priv->min_alert = priv->min_alert || !!(ret &
> >> >>>>>> STTS751_STATUS_TRIPL);
> >> >>>>>> +
> >> >>>>>
> >> >>>>>
> >> >>>>> I am not really happy about the caching of alert values. This isn't
> >> >>>>> really
> >> >>>>> common. Usually we report current alert values when reading. I don't
> >> >>>>> really
> >> >>>>> see why this driver should be different to all the others.
> >> >>>>
> >> >>>>
> >> >>>>
> >> >>>> OK
> >> >>
> >> >>
> >> >> I've tried to avoid caching, but as soon as I read the status register
> >> >> in the alert handler, the device clears that reg, and it will remain
> >> >> cleared up to the next conversion.
> >> >>
> >> >> So for example if the reader is blocked on poll over the alert
> >> >> attribute, it gets unblocked on alert, and it will very likely read
> >> >> zero..
> >> >>
> >> >> The only way I can see for avoiding caching, and for changing the API
> >> >> about clearing events as you asked below, is performing a temperature
> >> >> read from the temperature registers and doing software compare with
> >> >> the limits in show_[max/min]_alert.
> >> >>
> >> >> Does this sounds OK for you ?
> >> >>
> >> >
> >> > Not really. That could be done from user space by just reading the
> >> > temperature
> >> > and comparing it against limits.
> >>
> >> Sorry, I don't see the point here.. Why the fact that something could
> >> be done in userspace should suggest we don't want to do it in the
> >> driver? We do, for example, a simple calculation to give an absolute
> >> value for the hysteresis; we could do this also in userspace, but we
> >> do this in the driver in order to support the proper API..
> >>
> > Datasheet says:
> >
> > T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A >
> > high limit). T HIGH is cleared when the status register is read, provided
> > the condition no longer exists.
> > T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low
> > limit). T LOW is cleared when the status register is read, provided the
> > condition no longer exists.
> >
> > This isn't exactly what you are claiming, since it says "cleared ... provided
> > that the condition no longer exists", which would be more in line with other
> > chips.
> 
> Yes, I know.. I've also read that on the datasheet.. And I tried
> reading the status register in both the alert handler and the show
> function, but it didn't worked :/
> 
> So I started to suspect the status register got actually clear after
> reading up to the next conversion, and to test this, I simply read it
> twice in the alert handler. The first read always read 1, as expected;
> the second time I always got zero, even if the temperature was still
> over-limit.. And I soon got another event, that behaved in the same
> manner.
> 
Question is if the event bit is set again after the next conversion; this would
be relatively easy to handle (cache the status register for one conversion
time).

> (also I'm thinking about a way to mask events once we know that we are
> in a out-of-limit condition, to avoid having a lot of them, but simply
> disabling events and re-enabling them after userspace reads the alarm
> and find it to be zero, like lm90 driver does, IMHO wouldn't be good
> here, because we have a single bit to disable both high and low
> limits..)
> 
> > The THRM bit doesn't seem to self-clean either, at least not according to the
> > datasheet.
> 
> I need to test this.. But this doesn't generate any event, it seems
> only inteded to drive an external circuit, say a fan, and not to
> notify SW.. And currently I even have no show function to read it's
> status (now that I'm on it, I think I should add it...)..
> 
Yes. Note that this pin might be wired to an interrupt.

> If I add it, and this func reads the status reg, then it will end up
> in racy clears of the other high/low event bits anyway, since it's all
> in the same reg..
> 
> Indeed it seems to me that we can rely on chip automatism only for
> alarm event generation. And I don't see all that advantage in relying
> on HW comparators for reporting alarms in the show functions; IMHO
> there we can do our comparisons in SW without any real issue..
> 
> >
> > This means I'll need to get a sample and/or eval board to test this myself.
> 
> If you want to write your own tests, I will be happy to make them run

That isn't the point; I'll want to see how the chip really behaves. 
Reading through the datasheet, the most likely behavior is that the status
bits are cleared on read and set again with the next measurement cycle, as
already mentioned above. Per datasheet, the alert is supposed to be raised
again after each measurement cycle if the condition still exists. This behavior
is actually quite common with other chips. If so, all we would need to do is
to cache the status register value for one measurement cycle.

Regarding testing, I'll ultimately write a module test for the driver; see
https://github.com/groeck/module-tests for details. For that, all I'll need
is a register dump.

Thanks,
Guenter

> on my board and give you the results back. Just pass me the test code
> :)
> 
> > Guenter
> >
> >> > If the chip clears the alert bit on read, why would you need a set attribute
> >> > to clear it ?
> >>
> >> That was used to clear the cached value, not the chip reg.
> >>
> >> > From what you are saying, the next alert should update all
> >> > alert bits, meaning they would be auto-updated at that time. User space
> >> > would then always read the current state.
> >> >
> >> > Sure, you would cache the value
> >> > in the alert function to be able to read it in the read function, but I
> >> > don't see why all the complexity around it in your code would be needed.
> >>
> >> The next alert is, by definition, an alert, so it will always update
> >> the cached value with a '1'. We don't have anything that tell us when
> >> the alert is gone. No one would clear the cached value with '0' when
> >> the alert is gone. (ok, we have two alert bits, so the _other_ one of
> >> them will be set to zero, but that's not the point)..
> >>
> >> So, in order to report the current alarm value, without relying to the
> >> user to clear it, as you asked for, caching seems not an option..
> >>
> >>
> >> > You might want to update the alert status in case there is no interrupt
> >> > support, though (maybe you do already; I didn't re-check the code).
> >>
> >> OK, good point, I'll make sure it works event in this case.
> >>
> >> > The 'sensors' code doesn't know that it is supposed to write into the
> >> > alert attribute to clear the status, thus unless you have a special
> >> > application, an alert would never be cleared. That doesn't sound very
> >> > appealing to me.
> >>
> >> So, OK to change the API as you said, however I don't see how this can
> >> be done except for doing the manual temperature/limit comparison in
> >> the driver:
> >>
> >> When you get into show_[max/min]_alert function you can't rely on
> >> reading the status register (because as I said in my prev mail it
> >> could have been just cleared by reading it in the event handler), but
> >> as I explained above it seems to me that you cannot also rely on a
> >> value cached in the event handler with this API, because no one will
> >> ever clear it..
> >>
> >> In other words you will see a status register that could be zero even
> >> when we have the alarm, or a cached value that is still 1 even if the
> >> alarm has gone time ago.. I could clear the cached value in the
> >> show_[max/min]_alert function itself, but this doesn't seems enough to
> >> me for reporting to the userspace the _current_ alarm value, as you
> >> asked for.
> >>
> >> Or am I missing something ?
> >>
> >> >
> >> >>>>
> >> >>>>>> +     return 0;
> >> >>>>>> +}
> >> >>>>>> +
> >> >>>>>> +static void stts751_alert(struct i2c_client *client,
> >> >>>>>> +                     enum i2c_alert_protocol type, unsigned int data)
> >> >>>>>> +{
> >> >>>>>> +     int ret;
> >> >>>>>> +     struct stts751_priv *priv = i2c_get_clientdata(client);
> >> >>>>>> +     bool prev_max = priv->max_alert;
> >> >>>>>> +     bool prev_min = priv->min_alert;
> >> >>>>>> +
> >> >>>>>> +     if (type != I2C_PROTOCOL_SMBUS_ALERT)
> >> >>>>>> +             return;
> >> >>>>>> +
> >> >>>>>> +     dev_dbg(&client->dev, "alert!");
> >> >>>>>> +
> >> >>>>>
> >> >>>>>
> >> >>>>>
> >> >>>>> Any chance to make this a bit more generic and also support
> >> >>>>> interrupts ?
> >> >>>>
> >> >>>>
> >> >>>>
> >> >>>> May you please elaborate?
> >> >>>>
> >> >>>> IMHO adding the logic for handling SMBus alerts using IRQs to any
> >> >>>> HWmon driver is redundant, because it is already implemented in the
> >> >>>> SMBus layer, just right now you need patching SMBus layer for enabling
> >> >>>> it from the DT world (a couple of patches for this are floating
> >> >>>> around, but they seems not ready for merge yet)... Using this chip
> >> >>>> with a generic interrupt is indeed exactly what I'm doing..
> >> >>>>
> >> >>>> .. Or am I completely missing your point ?
> >> >>>>
> >> >>> "just right now you need patching SMBus layer for enabling it from the DT
> >> >>> world"
> >> >>>
> >> >>> So it _doesn't_ work, correct ?
> >> >>
> >> >>
> >> >> I mean, the generic smbus code works for stts751, and the chip seems
> >> >> happy. So, in this case, no need to reimplement it elsewhere. It would
> >> >> just end up to be a copy-paste from smbus.c.... Just AFAICT we need
> >> >> some mechanism to hook smbus code from DT world..
> >> >>
> >> >
> >> > Ok, then let's just go with it. If it doesn't work in some application we'll
> >> > have to fix it up later.
> >> >
> >> >
> >> >>>>>> +
> >> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
> >> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO,
> >> >>>>>> +                     show_min, set_min, 0);
> >> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO,
> >> >>>>>> +                     show_max, set_max, 0);
> >> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO,
> >> >>>>>> +                     show_min_alert, set_min_alert, 0);
> >> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO,
> >> >>>>>> +                     show_max_alert, set_max_alert, 0);
> >> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,
> >> >>>>>> +                     set_therm, 0);
> >> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO,
> >> >>>>>> show_hyst,
> >> >>>>>> +                     set_hyst, 0);
> >> >>>>>
> >> >>>>>
> >> >>>>>
> >> >>>>> Please use standard attribute names.
> >> >>>>
> >> >>>>
> >> >>>>
> >> >>>> I can convert the event stuff in temp1_[min/max]_alarm for limits and
> >> >>>> temp1_alarm for the flag, but it's not obvious to be what should I use
> >> >>>> for the therm stuff.. May you please give me advices about this?
> >> >>>>
> >> >>>> I'd propose temp1_therm_[max/min]_hyst, but they seems not standard
> >> >>>> anyway...
> >> >>>>
> >> >>>
> >> >>> From the chip datasheet, this is supposed to reflect overtemperature
> >> >>> conditions,
> >> >>> stronger than high/low, thus 'crit' seems appropriate.
> >> >>
> >> >>
> >> >> I see.. I can go with 'crit' :)
> >> >>
> >> >> Just is there any place where I can document this? Despite the
> >> >> datasheet, I can easily imagine real-world usage scenarios, for
> >> >> example driving a fan from that signal, where 'crit' might be not so
> >> >> intuitive.. Indeed I would like to make it visible the 'crit'
> >> >> <->'therm' binding to users. Maybe an additional attribute
> >> >> 'temp1_crit_label' or something like that ?
> >> >>
> >> > You are supposed to provide Documentation/hwmon/stts751, which would
> >> > include such information.
> >> >
> >> > A label applies to a sensor, not to a sensor attribute, so that would
> >> > not be appropriate.
> >>
> >> OK to provide it, but it would be in kernel source tree. Any further
> >> hint that we could give to someone that looks at in /sys ?
> >>
> >> > Thanks,
> >> > Guenter
> >> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] hwmon: new driver for ST stts751 thermal sensor

[Andrea Merello]

On Wed, Nov 2, 2016 at 11:48 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Wed, Nov 02, 2016 at 09:11:35AM +0100, Andrea Merello wrote:
>> On Mon, Oct 31, 2016 at 5:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> > On 10/31/2016 08:27 AM, Andrea Merello wrote:
>> >
>> >>>>
>> >>>>>> +     if (ret < 0)
>> >>>>>> +             return ret;
>> >>>>>> +
>> >>>>>> +     priv->max_alert = priv->max_alert || !!(ret &
>> >>>>>> STTS751_STATUS_TRIPH);
>> >>>>>> +     priv->min_alert = priv->min_alert || !!(ret &
>> >>>>>> STTS751_STATUS_TRIPL);
>> >>>>>> +
>> >>>>>
>> >>>>>
>> >>>>> I am not really happy about the caching of alert values. This isn't
>> >>>>> really
>> >>>>> common. Usually we report current alert values when reading. I don't
>> >>>>> really
>> >>>>> see why this driver should be different to all the others.
>> >>>>
>> >>>>
>> >>>>
>> >>>> OK
>> >>
>> >>
>> >> I've tried to avoid caching, but as soon as I read the status register
>> >> in the alert handler, the device clears that reg, and it will remain
>> >> cleared up to the next conversion.
>> >>
>> >> So for example if the reader is blocked on poll over the alert
>> >> attribute, it gets unblocked on alert, and it will very likely read
>> >> zero..
>> >>
>> >> The only way I can see for avoiding caching, and for changing the API
>> >> about clearing events as you asked below, is performing a temperature
>> >> read from the temperature registers and doing software compare with
>> >> the limits in show_[max/min]_alert.
>> >>
>> >> Does this sounds OK for you ?
>> >>
>> >
>> > Not really. That could be done from user space by just reading the
>> > temperature
>> > and comparing it against limits.
>>
>> Sorry, I don't see the point here.. Why the fact that something could
>> be done in userspace should suggest we don't want to do it in the
>> driver? We do, for example, a simple calculation to give an absolute
>> value for the hysteresis; we could do this also in userspace, but we
>> do this in the driver in order to support the proper API..
>>
> Datasheet says:
>
> T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A >
> high limit). T HIGH is cleared when the status register is read, provided
> the condition no longer exists.
> T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low
> limit). T LOW is cleared when the status register is read, provided the
> condition no longer exists.
>
> This isn't exactly what you are claiming, since it says "cleared ... provided
> that the condition no longer exists", which would be more in line with other
> chips.

Yes, I know.. I've also read that on the datasheet.. And I tried
reading the status register in both the alert handler and the show
function, but it didn't worked :/

So I started to suspect the status register got actually clear after
reading up to the next conversion, and to test this, I simply read it
twice in the alert handler. The first read always read 1, as expected;
the second time I always got zero, even if the temperature was still
over-limit.. And I soon got another event, that behaved in the same
manner.

(also I'm thinking about a way to mask events once we know that we are
in a out-of-limit condition, to avoid having a lot of them, but simply
disabling events and re-enabling them after userspace reads the alarm
and find it to be zero, like lm90 driver does, IMHO wouldn't be good
here, because we have a single bit to disable both high and low
limits..)

> The THRM bit doesn't seem to self-clean either, at least not according to the
> datasheet.

I need to test this.. But this doesn't generate any event, it seems
only inteded to drive an external circuit, say a fan, and not to
notify SW.. And currently I even have no show function to read it's
status (now that I'm on it, I think I should add it...)..

If I add it, and this func reads the status reg, then it will end up
in racy clears of the other high/low event bits anyway, since it's all
in the same reg..

Indeed it seems to me that we can rely on chip automatism only for
alarm event generation. And I don't see all that advantage in relying
on HW comparators for reporting alarms in the show functions; IMHO
there we can do our comparisons in SW without any real issue..

>
> This means I'll need to get a sample and/or eval board to test this myself.

If you want to write your own tests, I will be happy to make them run
on my board and give you the results back. Just pass me the test code
:)

> Guenter
>
>> > If the chip clears the alert bit on read, why would you need a set attribute
>> > to clear it ?
>>
>> That was used to clear the cached value, not the chip reg.
>>
>> > From what you are saying, the next alert should update all
>> > alert bits, meaning they would be auto-updated at that time. User space
>> > would then always read the current state.
>> >
>> > Sure, you would cache the value
>> > in the alert function to be able to read it in the read function, but I
>> > don't see why all the complexity around it in your code would be needed.
>>
>> The next alert is, by definition, an alert, so it will always update
>> the cached value with a '1'. We don't have anything that tell us when
>> the alert is gone. No one would clear the cached value with '0' when
>> the alert is gone. (ok, we have two alert bits, so the _other_ one of
>> them will be set to zero, but that's not the point)..
>>
>> So, in order to report the current alarm value, without relying to the
>> user to clear it, as you asked for, caching seems not an option..
>>
>>
>> > You might want to update the alert status in case there is no interrupt
>> > support, though (maybe you do already; I didn't re-check the code).
>>
>> OK, good point, I'll make sure it works event in this case.
>>
>> > The 'sensors' code doesn't know that it is supposed to write into the
>> > alert attribute to clear the status, thus unless you have a special
>> > application, an alert would never be cleared. That doesn't sound very
>> > appealing to me.
>>
>> So, OK to change the API as you said, however I don't see how this can
>> be done except for doing the manual temperature/limit comparison in
>> the driver:
>>
>> When you get into show_[max/min]_alert function you can't rely on
>> reading the status register (because as I said in my prev mail it
>> could have been just cleared by reading it in the event handler), but
>> as I explained above it seems to me that you cannot also rely on a
>> value cached in the event handler with this API, because no one will
>> ever clear it..
>>
>> In other words you will see a status register that could be zero even
>> when we have the alarm, or a cached value that is still 1 even if the
>> alarm has gone time ago.. I could clear the cached value in the
>> show_[max/min]_alert function itself, but this doesn't seems enough to
>> me for reporting to the userspace the _current_ alarm value, as you
>> asked for.
>>
>> Or am I missing something ?
>>
>> >
>> >>>>
>> >>>>>> +     return 0;
>> >>>>>> +}
>> >>>>>> +
>> >>>>>> +static void stts751_alert(struct i2c_client *client,
>> >>>>>> +                     enum i2c_alert_protocol type, unsigned int data)
>> >>>>>> +{
>> >>>>>> +     int ret;
>> >>>>>> +     struct stts751_priv *priv = i2c_get_clientdata(client);
>> >>>>>> +     bool prev_max = priv->max_alert;
>> >>>>>> +     bool prev_min = priv->min_alert;
>> >>>>>> +
>> >>>>>> +     if (type != I2C_PROTOCOL_SMBUS_ALERT)
>> >>>>>> +             return;
>> >>>>>> +
>> >>>>>> +     dev_dbg(&client->dev, "alert!");
>> >>>>>> +
>> >>>>>
>> >>>>>
>> >>>>>
>> >>>>> Any chance to make this a bit more generic and also support
>> >>>>> interrupts ?
>> >>>>
>> >>>>
>> >>>>
>> >>>> May you please elaborate?
>> >>>>
>> >>>> IMHO adding the logic for handling SMBus alerts using IRQs to any
>> >>>> HWmon driver is redundant, because it is already implemented in the
>> >>>> SMBus layer, just right now you need patching SMBus layer for enabling
>> >>>> it from the DT world (a couple of patches for this are floating
>> >>>> around, but they seems not ready for merge yet)... Using this chip
>> >>>> with a generic interrupt is indeed exactly what I'm doing..
>> >>>>
>> >>>> .. Or am I completely missing your point ?
>> >>>>
>> >>> "just right now you need patching SMBus layer for enabling it from the DT
>> >>> world"
>> >>>
>> >>> So it _doesn't_ work, correct ?
>> >>
>> >>
>> >> I mean, the generic smbus code works for stts751, and the chip seems
>> >> happy. So, in this case, no need to reimplement it elsewhere. It would
>> >> just end up to be a copy-paste from smbus.c.... Just AFAICT we need
>> >> some mechanism to hook smbus code from DT world..
>> >>
>> >
>> > Ok, then let's just go with it. If it doesn't work in some application we'll
>> > have to fix it up later.
>> >
>> >
>> >>>>>> +
>> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
>> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO,
>> >>>>>> +                     show_min, set_min, 0);
>> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO,
>> >>>>>> +                     show_max, set_max, 0);
>> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO,
>> >>>>>> +                     show_min_alert, set_min_alert, 0);
>> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO,
>> >>>>>> +                     show_max_alert, set_max_alert, 0);
>> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,
>> >>>>>> +                     set_therm, 0);
>> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO,
>> >>>>>> show_hyst,
>> >>>>>> +                     set_hyst, 0);
>> >>>>>
>> >>>>>
>> >>>>>
>> >>>>> Please use standard attribute names.
>> >>>>
>> >>>>
>> >>>>
>> >>>> I can convert the event stuff in temp1_[min/max]_alarm for limits and
>> >>>> temp1_alarm for the flag, but it's not obvious to be what should I use
>> >>>> for the therm stuff.. May you please give me advices about this?
>> >>>>
>> >>>> I'd propose temp1_therm_[max/min]_hyst, but they seems not standard
>> >>>> anyway...
>> >>>>
>> >>>
>> >>> From the chip datasheet, this is supposed to reflect overtemperature
>> >>> conditions,
>> >>> stronger than high/low, thus 'crit' seems appropriate.
>> >>
>> >>
>> >> I see.. I can go with 'crit' :)
>> >>
>> >> Just is there any place where I can document this? Despite the
>> >> datasheet, I can easily imagine real-world usage scenarios, for
>> >> example driving a fan from that signal, where 'crit' might be not so
>> >> intuitive.. Indeed I would like to make it visible the 'crit'
>> >> <->'therm' binding to users. Maybe an additional attribute
>> >> 'temp1_crit_label' or something like that ?
>> >>
>> > You are supposed to provide Documentation/hwmon/stts751, which would
>> > include such information.
>> >
>> > A label applies to a sensor, not to a sensor attribute, so that would
>> > not be appropriate.
>>
>> OK to provide it, but it would be in kernel source tree. Any further
>> hint that we could give to someone that looks at in /sys ?
>>
>> > Thanks,
>> > Guenter
>> >

Re: [PATCH v2 2/2] hwmon: new driver for ST stts751 thermal sensor

[Guenter Roeck]

On Wed, Nov 02, 2016 at 09:11:35AM +0100, Andrea Merello wrote:
> On Mon, Oct 31, 2016 at 5:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 10/31/2016 08:27 AM, Andrea Merello wrote:
> >
> >>>>
> >>>>>> +     if (ret < 0)
> >>>>>> +             return ret;
> >>>>>> +
> >>>>>> +     priv->max_alert = priv->max_alert || !!(ret &
> >>>>>> STTS751_STATUS_TRIPH);
> >>>>>> +     priv->min_alert = priv->min_alert || !!(ret &
> >>>>>> STTS751_STATUS_TRIPL);
> >>>>>> +
> >>>>>
> >>>>>
> >>>>> I am not really happy about the caching of alert values. This isn't
> >>>>> really
> >>>>> common. Usually we report current alert values when reading. I don't
> >>>>> really
> >>>>> see why this driver should be different to all the others.
> >>>>
> >>>>
> >>>>
> >>>> OK
> >>
> >>
> >> I've tried to avoid caching, but as soon as I read the status register
> >> in the alert handler, the device clears that reg, and it will remain
> >> cleared up to the next conversion.
> >>
> >> So for example if the reader is blocked on poll over the alert
> >> attribute, it gets unblocked on alert, and it will very likely read
> >> zero..
> >>
> >> The only way I can see for avoiding caching, and for changing the API
> >> about clearing events as you asked below, is performing a temperature
> >> read from the temperature registers and doing software compare with
> >> the limits in show_[max/min]_alert.
> >>
> >> Does this sounds OK for you ?
> >>
> >
> > Not really. That could be done from user space by just reading the
> > temperature
> > and comparing it against limits.
> 
> Sorry, I don't see the point here.. Why the fact that something could
> be done in userspace should suggest we don't want to do it in the
> driver? We do, for example, a simple calculation to give an absolute
> value for the hysteresis; we could do this also in userspace, but we
> do this in the driver in order to support the proper API..
> 
Datasheet says:

T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A >
high limit). T HIGH is cleared when the status register is read, provided
the condition no longer exists.
T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low
limit). T LOW is cleared when the status register is read, provided the
condition no longer exists.

This isn't exactly what you are claiming, since it says "cleared ... provided
that the condition no longer exists", which would be more in line with other
chips.

The THRM bit doesn't seem to self-clean either, at least not according to the
datasheet.

This means I'll need to get a sample and/or eval board to test this myself.

Guenter

> > If the chip clears the alert bit on read, why would you need a set attribute
> > to clear it ?
> 
> That was used to clear the cached value, not the chip reg.
> 
> > From what you are saying, the next alert should update all
> > alert bits, meaning they would be auto-updated at that time. User space
> > would then always read the current state.
> >
> > Sure, you would cache the value
> > in the alert function to be able to read it in the read function, but I
> > don't see why all the complexity around it in your code would be needed.
> 
> The next alert is, by definition, an alert, so it will always update
> the cached value with a '1'. We don't have anything that tell us when
> the alert is gone. No one would clear the cached value with '0' when
> the alert is gone. (ok, we have two alert bits, so the _other_ one of
> them will be set to zero, but that's not the point)..
> 
> So, in order to report the current alarm value, without relying to the
> user to clear it, as you asked for, caching seems not an option..
> 
> 
> > You might want to update the alert status in case there is no interrupt
> > support, though (maybe you do already; I didn't re-check the code).
> 
> OK, good point, I'll make sure it works event in this case.
> 
> > The 'sensors' code doesn't know that it is supposed to write into the
> > alert attribute to clear the status, thus unless you have a special
> > application, an alert would never be cleared. That doesn't sound very
> > appealing to me.
> 
> So, OK to change the API as you said, however I don't see how this can
> be done except for doing the manual temperature/limit comparison in
> the driver:
> 
> When you get into show_[max/min]_alert function you can't rely on
> reading the status register (because as I said in my prev mail it
> could have been just cleared by reading it in the event handler), but
> as I explained above it seems to me that you cannot also rely on a
> value cached in the event handler with this API, because no one will
> ever clear it..
> 
> In other words you will see a status register that could be zero even
> when we have the alarm, or a cached value that is still 1 even if the
> alarm has gone time ago.. I could clear the cached value in the
> show_[max/min]_alert function itself, but this doesn't seems enough to
> me for reporting to the userspace the _current_ alarm value, as you
> asked for.
> 
> Or am I missing something ?
> 
> >
> >>>>
> >>>>>> +     return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void stts751_alert(struct i2c_client *client,
> >>>>>> +                     enum i2c_alert_protocol type, unsigned int data)
> >>>>>> +{
> >>>>>> +     int ret;
> >>>>>> +     struct stts751_priv *priv = i2c_get_clientdata(client);
> >>>>>> +     bool prev_max = priv->max_alert;
> >>>>>> +     bool prev_min = priv->min_alert;
> >>>>>> +
> >>>>>> +     if (type != I2C_PROTOCOL_SMBUS_ALERT)
> >>>>>> +             return;
> >>>>>> +
> >>>>>> +     dev_dbg(&client->dev, "alert!");
> >>>>>> +
> >>>>>
> >>>>>
> >>>>>
> >>>>> Any chance to make this a bit more generic and also support
> >>>>> interrupts ?
> >>>>
> >>>>
> >>>>
> >>>> May you please elaborate?
> >>>>
> >>>> IMHO adding the logic for handling SMBus alerts using IRQs to any
> >>>> HWmon driver is redundant, because it is already implemented in the
> >>>> SMBus layer, just right now you need patching SMBus layer for enabling
> >>>> it from the DT world (a couple of patches for this are floating
> >>>> around, but they seems not ready for merge yet)... Using this chip
> >>>> with a generic interrupt is indeed exactly what I'm doing..
> >>>>
> >>>> .. Or am I completely missing your point ?
> >>>>
> >>> "just right now you need patching SMBus layer for enabling it from the DT
> >>> world"
> >>>
> >>> So it _doesn't_ work, correct ?
> >>
> >>
> >> I mean, the generic smbus code works for stts751, and the chip seems
> >> happy. So, in this case, no need to reimplement it elsewhere. It would
> >> just end up to be a copy-paste from smbus.c.... Just AFAICT we need
> >> some mechanism to hook smbus code from DT world..
> >>
> >
> > Ok, then let's just go with it. If it doesn't work in some application we'll
> > have to fix it up later.
> >
> >
> >>>>>> +
> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO,
> >>>>>> +                     show_min, set_min, 0);
> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO,
> >>>>>> +                     show_max, set_max, 0);
> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO,
> >>>>>> +                     show_min_alert, set_min_alert, 0);
> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO,
> >>>>>> +                     show_max_alert, set_max_alert, 0);
> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,
> >>>>>> +                     set_therm, 0);
> >>>>>> +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO,
> >>>>>> show_hyst,
> >>>>>> +                     set_hyst, 0);
> >>>>>
> >>>>>
> >>>>>
> >>>>> Please use standard attribute names.
> >>>>
> >>>>
> >>>>
> >>>> I can convert the event stuff in temp1_[min/max]_alarm for limits and
> >>>> temp1_alarm for the flag, but it's not obvious to be what should I use
> >>>> for the therm stuff.. May you please give me advices about this?
> >>>>
> >>>> I'd propose temp1_therm_[max/min]_hyst, but they seems not standard
> >>>> anyway...
> >>>>
> >>>
> >>> From the chip datasheet, this is supposed to reflect overtemperature
> >>> conditions,
> >>> stronger than high/low, thus 'crit' seems appropriate.
> >>
> >>
> >> I see.. I can go with 'crit' :)
> >>
> >> Just is there any place where I can document this? Despite the
> >> datasheet, I can easily imagine real-world usage scenarios, for
> >> example driving a fan from that signal, where 'crit' might be not so
> >> intuitive.. Indeed I would like to make it visible the 'crit'
> >> <->'therm' binding to users. Maybe an additional attribute
> >> 'temp1_crit_label' or something like that ?
> >>
> > You are supposed to provide Documentation/hwmon/stts751, which would
> > include such information.
> >
> > A label applies to a sensor, not to a sensor attribute, so that would
> > not be appropriate.
> 
> OK to provide it, but it would be in kernel source tree. Any further
> hint that we could give to someone that looks at in /sys ?
> 
> > Thanks,
> > Guenter
> >

Re: [PATCH v2 2/2] hwmon: new driver for ST stts751 thermal sensor

[Andrea Merello]

On Mon, Oct 31, 2016 at 5:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 10/31/2016 08:27 AM, Andrea Merello wrote:
>
>>>>
>>>>>> +     if (ret < 0)
>>>>>> +             return ret;
>>>>>> +
>>>>>> +     priv->max_alert = priv->max_alert || !!(ret &
>>>>>> STTS751_STATUS_TRIPH);
>>>>>> +     priv->min_alert = priv->min_alert || !!(ret &
>>>>>> STTS751_STATUS_TRIPL);
>>>>>> +
>>>>>
>>>>>
>>>>> I am not really happy about the caching of alert values. This isn't
>>>>> really
>>>>> common. Usually we report current alert values when reading. I don't
>>>>> really
>>>>> see why this driver should be different to all the others.
>>>>
>>>>
>>>>
>>>> OK
>>
>>
>> I've tried to avoid caching, but as soon as I read the status register
>> in the alert handler, the device clears that reg, and it will remain
>> cleared up to the next conversion.
>>
>> So for example if the reader is blocked on poll over the alert
>> attribute, it gets unblocked on alert, and it will very likely read
>> zero..
>>
>> The only way I can see for avoiding caching, and for changing the API
>> about clearing events as you asked below, is performing a temperature
>> read from the temperature registers and doing software compare with
>> the limits in show_[max/min]_alert.
>>
>> Does this sounds OK for you ?
>>
>
> Not really. That could be done from user space by just reading the
> temperature
> and comparing it against limits.

Sorry, I don't see the point here.. Why the fact that something could
be done in userspace should suggest we don't want to do it in the
driver? We do, for example, a simple calculation to give an absolute
value for the hysteresis; we could do this also in userspace, but we
do this in the driver in order to support the proper API..

> If the chip clears the alert bit on read, why would you need a set attribute
> to clear it ?

That was used to clear the cached value, not the chip reg.

> From what you are saying, the next alert should update all
> alert bits, meaning they would be auto-updated at that time. User space
> would then always read the current state.
>
> Sure, you would cache the value
> in the alert function to be able to read it in the read function, but I
> don't see why all the complexity around it in your code would be needed.

The next alert is, by definition, an alert, so it will always update
the cached value with a '1'. We don't have anything that tell us when
the alert is gone. No one would clear the cached value with '0' when
the alert is gone. (ok, we have two alert bits, so the _other_ one of
them will be set to zero, but that's not the point)..

So, in order to report the current alarm value, without relying to the
user to clear it, as you asked for, caching seems not an option..


> You might want to update the alert status in case there is no interrupt
> support, though (maybe you do already; I didn't re-check the code).

OK, good point, I'll make sure it works event in this case.

> The 'sensors' code doesn't know that it is supposed to write into the
> alert attribute to clear the status, thus unless you have a special
> application, an alert would never be cleared. That doesn't sound very
> appealing to me.

So, OK to change the API as you said, however I don't see how this can
be done except for doing the manual temperature/limit comparison in
the driver:

When you get into show_[max/min]_alert function you can't rely on
reading the status register (because as I said in my prev mail it
could have been just cleared by reading it in the event handler), but
as I explained above it seems to me that you cannot also rely on a
value cached in the event handler with this API, because no one will
ever clear it..

In other words you will see a status register that could be zero even
when we have the alarm, or a cached value that is still 1 even if the
alarm has gone time ago.. I could clear the cached value in the
show_[max/min]_alert function itself, but this doesn't seems enough to
me for reporting to the userspace the _current_ alarm value, as you
asked for.

Or am I missing something ?

>
>>>>
>>>>>> +     return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static void stts751_alert(struct i2c_client *client,
>>>>>> +                     enum i2c_alert_protocol type, unsigned int data)
>>>>>> +{
>>>>>> +     int ret;
>>>>>> +     struct stts751_priv *priv = i2c_get_clientdata(client);
>>>>>> +     bool prev_max = priv->max_alert;
>>>>>> +     bool prev_min = priv->min_alert;
>>>>>> +
>>>>>> +     if (type != I2C_PROTOCOL_SMBUS_ALERT)
>>>>>> +             return;
>>>>>> +
>>>>>> +     dev_dbg(&client->dev, "alert!");
>>>>>> +
>>>>>
>>>>>
>>>>>
>>>>> Any chance to make this a bit more generic and also support
>>>>> interrupts ?
>>>>
>>>>
>>>>
>>>> May you please elaborate?
>>>>
>>>> IMHO adding the logic for handling SMBus alerts using IRQs to any
>>>> HWmon driver is redundant, because it is already implemented in the
>>>> SMBus layer, just right now you need patching SMBus layer for enabling
>>>> it from the DT world (a couple of patches for this are floating
>>>> around, but they seems not ready for merge yet)... Using this chip
>>>> with a generic interrupt is indeed exactly what I'm doing..
>>>>
>>>> .. Or am I completely missing your point ?
>>>>
>>> "just right now you need patching SMBus layer for enabling it from the DT
>>> world"
>>>
>>> So it _doesn't_ work, correct ?
>>
>>
>> I mean, the generic smbus code works for stts751, and the chip seems
>> happy. So, in this case, no need to reimplement it elsewhere. It would
>> just end up to be a copy-paste from smbus.c.... Just AFAICT we need
>> some mechanism to hook smbus code from DT world..
>>
>
> Ok, then let's just go with it. If it doesn't work in some application we'll
> have to fix it up later.
>
>
>>>>>> +
>>>>>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
>>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO,
>>>>>> +                     show_min, set_min, 0);
>>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO,
>>>>>> +                     show_max, set_max, 0);
>>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO,
>>>>>> +                     show_min_alert, set_min_alert, 0);
>>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO,
>>>>>> +                     show_max_alert, set_max_alert, 0);
>>>>>> +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,
>>>>>> +                     set_therm, 0);
>>>>>> +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO,
>>>>>> show_hyst,
>>>>>> +                     set_hyst, 0);
>>>>>
>>>>>
>>>>>
>>>>> Please use standard attribute names.
>>>>
>>>>
>>>>
>>>> I can convert the event stuff in temp1_[min/max]_alarm for limits and
>>>> temp1_alarm for the flag, but it's not obvious to be what should I use
>>>> for the therm stuff.. May you please give me advices about this?
>>>>
>>>> I'd propose temp1_therm_[max/min]_hyst, but they seems not standard
>>>> anyway...
>>>>
>>>
>>> From the chip datasheet, this is supposed to reflect overtemperature
>>> conditions,
>>> stronger than high/low, thus 'crit' seems appropriate.
>>
>>
>> I see.. I can go with 'crit' :)
>>
>> Just is there any place where I can document this? Despite the
>> datasheet, I can easily imagine real-world usage scenarios, for
>> example driving a fan from that signal, where 'crit' might be not so
>> intuitive.. Indeed I would like to make it visible the 'crit'
>> <->'therm' binding to users. Maybe an additional attribute
>> 'temp1_crit_label' or something like that ?
>>
> You are supposed to provide Documentation/hwmon/stts751, which would
> include such information.
>
> A label applies to a sensor, not to a sensor attribute, so that would
> not be appropriate.

OK to provide it, but it would be in kernel source tree. Any further
hint that we could give to someone that looks at in /sys ?

> Thanks,
> Guenter
>

Re: [PATCH v2 2/2] hwmon: new driver for ST stts751 thermal sensor

[Guenter Roeck]

On 10/31/2016 08:27 AM, Andrea Merello wrote:

>>>
>>>>> +     if (ret < 0)
>>>>> +             return ret;
>>>>> +
>>>>> +     priv->max_alert = priv->max_alert || !!(ret &
>>>>> STTS751_STATUS_TRIPH);
>>>>> +     priv->min_alert = priv->min_alert || !!(ret &
>>>>> STTS751_STATUS_TRIPL);
>>>>> +
>>>>
>>>> I am not really happy about the caching of alert values. This isn't
>>>> really
>>>> common. Usually we report current alert values when reading. I don't
>>>> really
>>>> see why this driver should be different to all the others.
>>>
>>>
>>> OK
>
> I've tried to avoid caching, but as soon as I read the status register
> in the alert handler, the device clears that reg, and it will remain
> cleared up to the next conversion.
>
> So for example if the reader is blocked on poll over the alert
> attribute, it gets unblocked on alert, and it will very likely read
> zero..
>
> The only way I can see for avoiding caching, and for changing the API
> about clearing events as you asked below, is performing a temperature
> read from the temperature registers and doing software compare with
> the limits in show_[max/min]_alert.
>
> Does this sounds OK for you ?
>

Not really. That could be done from user space by just reading the temperature
and comparing it against limits.

If the chip clears the alert bit on read, why would you need a set attribute
to clear it ? From what you are saying, the next alert should update all
alert bits, meaning they would be auto-updated at that time. User space
would then always read the current state. Sure, you would cache the value
in the alert function to be able to read it in the read function, but I
don't see why all the complexity around it in your code would be needed.

You might want to update the alert status in case there is no interrupt
support, though (maybe you do already; I didn't re-check the code).

The 'sensors' code doesn't know that it is supposed to write into the
alert attribute to clear the status, thus unless you have a special
application, an alert would never be cleared. That doesn't sound very
appealing to me.

>>>
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static void stts751_alert(struct i2c_client *client,
>>>>> +                     enum i2c_alert_protocol type, unsigned int data)
>>>>> +{
>>>>> +     int ret;
>>>>> +     struct stts751_priv *priv = i2c_get_clientdata(client);
>>>>> +     bool prev_max = priv->max_alert;
>>>>> +     bool prev_min = priv->min_alert;
>>>>> +
>>>>> +     if (type != I2C_PROTOCOL_SMBUS_ALERT)
>>>>> +             return;
>>>>> +
>>>>> +     dev_dbg(&client->dev, "alert!");
>>>>> +
>>>>
>>>>
>>>> Any chance to make this a bit more generic and also support
>>>> interrupts ?
>>>
>>>
>>> May you please elaborate?
>>>
>>> IMHO adding the logic for handling SMBus alerts using IRQs to any
>>> HWmon driver is redundant, because it is already implemented in the
>>> SMBus layer, just right now you need patching SMBus layer for enabling
>>> it from the DT world (a couple of patches for this are floating
>>> around, but they seems not ready for merge yet)... Using this chip
>>> with a generic interrupt is indeed exactly what I'm doing..
>>>
>>> .. Or am I completely missing your point ?
>>>
>> "just right now you need patching SMBus layer for enabling it from the DT
>> world"
>>
>> So it _doesn't_ work, correct ?
>
> I mean, the generic smbus code works for stts751, and the chip seems
> happy. So, in this case, no need to reimplement it elsewhere. It would
> just end up to be a copy-paste from smbus.c.... Just AFAICT we need
> some mechanism to hook smbus code from DT world..
>

Ok, then let's just go with it. If it doesn't work in some application we'll
have to fix it up later.

>>>>> +
>>>>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO,
>>>>> +                     show_min, set_min, 0);
>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO,
>>>>> +                     show_max, set_max, 0);
>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO,
>>>>> +                     show_min_alert, set_min_alert, 0);
>>>>> +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO,
>>>>> +                     show_max_alert, set_max_alert, 0);
>>>>> +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,
>>>>> +                     set_therm, 0);
>>>>> +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO,
>>>>> show_hyst,
>>>>> +                     set_hyst, 0);
>>>>
>>>>
>>>> Please use standard attribute names.
>>>
>>>
>>> I can convert the event stuff in temp1_[min/max]_alarm for limits and
>>> temp1_alarm for the flag, but it's not obvious to be what should I use
>>> for the therm stuff.. May you please give me advices about this?
>>>
>>> I'd propose temp1_therm_[max/min]_hyst, but they seems not standard
>>> anyway...
>>>
>>
>> From the chip datasheet, this is supposed to reflect overtemperature
>> conditions,
>> stronger than high/low, thus 'crit' seems appropriate.
>
> I see.. I can go with 'crit' :)
>
> Just is there any place where I can document this? Despite the
> datasheet, I can easily imagine real-world usage scenarios, for
> example driving a fan from that signal, where 'crit' might be not so
> intuitive.. Indeed I would like to make it visible the 'crit'
> <->'therm' binding to users. Maybe an additional attribute
> 'temp1_crit_label' or something like that ?
>
You are supposed to provide Documentation/hwmon/stts751, which would
include such information.

A label applies to a sensor, not to a sensor attribute, so that would
not be appropriate.

Thanks,
Guenter

Re: [PATCH v2 2/2] hwmon: new driver for ST stts751 thermal sensor

[Andrea Merello]

On Wed, Oct 19, 2016 at 4:50 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 10/19/2016 05:16 AM, Andrea Merello wrote:
>>
>> On Wed, Oct 5, 2016 at 1:51 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> On Tue, Sep 27, 2016 at 02:07:27PM +0200, Andrea Merello wrote:
>>>>
>>>> This patch adds a HWMON driver for ST Microelectronics STTS751
>>>> temperature sensors.
>>>>
>>>> It does support temperature reading (of course), SMBUS alert
>>>> functionality and "therm" output.
>>>>
>>>> Thanks-to: LABBE Corentin [for suggestions]
>>>> Thanks-to: Guenter Roeck [for suggestions/discussions]
>>>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>>>> Cc: LABBE Corentin <clabbe.montjoie@gmail.com>
>>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>>> Cc: Jean Delvare <jdelvare@suse.com>
>>>> ---
>>>>  drivers/hwmon/Kconfig   |  12 +-
>>>>  drivers/hwmon/Makefile  |   2 +-
>>>>  drivers/hwmon/stts751.c | 805
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 817 insertions(+), 2 deletions(-)
>>>>  create mode 100644 drivers/hwmon/stts751.c
>>>>
>>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>>> index eaf2f91..4e70b42 100644
>>>> --- a/drivers/hwmon/Kconfig
>>>> +++ b/drivers/hwmon/Kconfig
>>>> @@ -1448,6 +1448,16 @@ config SENSORS_SCH5636
>>>>         This driver can also be built as a module.  If so, the module
>>>>         will be called sch5636.
>>>>
>>>> +config SENSORS_STTS751
>>>> +     tristate "ST Microelectronics STTS751"
>>>> +     depends on I2C
>>>> +     help
>>>> +       If you say yes here you get support for STTS751
>>>> +       temperature sensor chips.
>>>> +
>>>> +       This driver can also be built as a module. If so, the module
>>>> +       will be called stts751.
>>>> +
>>>>  config SENSORS_SMM665
>>>>       tristate "Summit Microelectronics SMM665"
>>>>       depends on I2C
>>>> @@ -1506,7 +1516,7 @@ config SENSORS_ADS7871
>>>>
>>>>  config SENSORS_AMC6821
>>>>       tristate "Texas Instruments AMC6821"
>>>> -     depends on I2C
>>>> +     depends on I2C
>>>
>>>
>>> ???
>>
>>
>> My editor is kind enough to automatically kill trailing spaces.. I'll
>> take care of moving this one in another patch in next series version..
>>
>>>
>>>>       help
>>>>         If you say yes here you get support for the Texas Instruments
>>>>         AMC6821 hardware monitoring chips.
>>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>>> index fe87d28..1114130 100644
>>>> --- a/drivers/hwmon/Makefile
>>>> +++ b/drivers/hwmon/Makefile
>>>> @@ -147,6 +147,7 @@ obj-$(CONFIG_SENSORS_SMM665)      += smm665.o
>>>>  obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
>>>>  obj-$(CONFIG_SENSORS_SMSC47M1)       += smsc47m1.o
>>>>  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>>>> +obj-$(CONFIG_SENSORS_STTS751)        += stts751.o
>>>>  obj-$(CONFIG_SENSORS_AMC6821)        += amc6821.o
>>>>  obj-$(CONFIG_SENSORS_TC74)   += tc74.o
>>>>  obj-$(CONFIG_SENSORS_THMC50) += thmc50.o
>>>> @@ -169,4 +170,3 @@ obj-$(CONFIG_SENSORS_WM8350)      += wm8350-hwmon.o
>>>>  obj-$(CONFIG_PMBUS)          += pmbus/
>>>>
>>>>  ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
>>>> -
>>>
>>>
>>> Looks like an unnecessary whitespace change.
>>
>>
>> Like above..
>>
>>>
>>>> diff --git a/drivers/hwmon/stts751.c b/drivers/hwmon/stts751.c
>>>> new file mode 100644
>>>> index 0000000..8358e04
>>>> --- /dev/null
>>>> +++ b/drivers/hwmon/stts751.c
>>>> @@ -0,0 +1,805 @@
>>>> +/*
>>>> + * STTS751 sensor driver
>>>> + *
>>>> + * Copyright (C) 2016 Istituto Italiano di Tecnologia - RBCS - EDL
>>>> + * Robotics, Brain and Cognitive Sciences department
>>>> + * Electronic Design Laboratory
>>>> + *
>>>> + * Written by Andrea Merello <andrea.merello@gmail.com>
>>>> + *
>>>> + * Based on  LM95241 driver and LM90 driver
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License as published by
>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>> + * (at your option) any later version.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + */
>>>> +
>>>> +
>>>
>>> No douple empty lines please.
>>
>>
>> OK. BTW do you use some enhanced "checkpatch" script in order to catch
>> issues like these?
>>
>
> In this case my brain, but checkpatch --strict reports it as well.

OK, Thanks.

>
>>>
>>>> +#include <linux/bitops.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/hwmon.h>
>>>> +#include <linux/hwmon-sysfs.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/jiffies.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mutex.h>
>>>> +#include <linux/of_irq.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/sysfs.h>
>>>> +#include <linux/util_macros.h>
>>>> +
>>>> +#define DEVNAME "stts751"
>>>> +
>>>> +static const unsigned short normal_i2c[] = {
>>>> +     0x48, 0x49, 0x38, 0x39,  /* STTS751-0 */
>>>> +     0x4A, 0x4B, 0x3A, 0x3B,  /* STTS751-1 */
>>>> +     I2C_CLIENT_END };
>>>> +
>>>> +#define STTS751_REG_TEMP_H   0x00
>>>> +#define STTS751_REG_STATUS   0x01
>>>> +#define STTS751_STATUS_TRIPL BIT(5)
>>>> +#define STTS751_STATUS_TRIPH BIT(6)
>>>> +#define STTS751_STATUS_BUSY  BIT(8)
>>>> +#define STTS751_REG_TEMP_L   0x02
>>>> +#define STTS751_REG_CONF     0x03
>>>> +#define STTS751_CONF_RES_MASK        0x0C
>>>> +#define STTS751_CONF_RES_SHIFT  2
>>>> +#define STTS751_CONF_EVENT_DIS  BIT(7)
>>>> +#define STTS751_CONF_STOP    BIT(6)
>>>> +#define STTS751_REG_RATE     0x04
>>>> +#define STTS751_REG_HLIM_H   0x05
>>>> +#define STTS751_REG_HLIM_L   0x06
>>>> +#define STTS751_REG_LLIM_H   0x07
>>>> +#define STTS751_REG_LLIM_L   0x08
>>>> +#define STTS751_REG_ONESHOT  0x0F
>>>> +#define STTS751_REG_TLIM     0x20
>>>> +#define STTS751_REG_HYST     0x21
>>>> +#define STTS751_REG_SMBUS_TO 0x22
>>>> +
>>>> +#define STTS751_REG_PROD_ID  0xFD
>>>> +#define STTS751_REG_MAN_ID   0xFE
>>>> +#define STTS751_REG_REV_ID   0xFF
>>>> +
>>>> +#define STTS751_0_PROD_ID    0x00
>>>> +#define STTS751_1_PROD_ID    0x01
>>>> +#define ST_MAN_ID            0x53
>>>> +
>>>> +/* stick with HW defaults */
>>>> +#define STTS751_THERM_DEFAULT        85000
>>>> +#define STTS751_HYST_DEFAULT 10000
>>>> +#define STTS751_EVENT_MAX_DEFAULT 85000
>>>> +#define STTS751_EVENT_MIN_DEFAULT 0
>>>> +
>>>> +#define STTS751_RACE_RETRY   5
>>>> +#define STTS751_CONV_TIMEOUT 100 /* mS */
>>>> +#define STTS751_CACHE_TIME   100 /* mS */
>>>> +
>>>> +/*
>>>> + * Possible update intervals are (in mS):
>>>> + * 16000, 8000, 4000, 2000, 1000, 500, 250, 125, 62.5, 31.25
>>>> + * However we are not going to complicate things too much and we stick
>>>> to the
>>>> + * approx value in mS.
>>>> + */
>>>> +static const int stts751_intervals[] = {
>>>> +     16000, 8000, 4000, 2000, 1000, 500, 250, 125, 63, 31
>>>> +};
>>>> +
>>>> +struct stts751_priv {
>>>> +     struct device *dev;
>>>> +     struct i2c_client *client;
>>>> +     struct mutex access_lock;
>>>> +     unsigned long interval;
>>>> +     int res;
>>>> +     bool gen_therm, gen_event;
>>>> +     int event_max, event_min;
>>>> +     int therm;
>>>> +     int hyst;
>>>> +     bool smbus_timeout;
>>>> +     int temp;
>>>> +     unsigned long last_update;
>>>> +     u8 config;
>>>> +     bool min_alert, max_alert;
>>>> +     bool data_valid;
>>>> +
>>>> +     /*
>>>> +      * Temperature/interval is always present.
>>>> +      * Depending by DT therm and event are dynamically added.
>>>> +      * There are max 4 entries plus the guard
>>>> +      */
>>>> +     const struct attribute_group *groups[5];
>>>> +};
>>>> +
>>>> +/*
>>>> + * These functions converts temperature from HW format to integer
>>>> format and
>>>> + * vice-vers. They are (mostly) taken from lm90 driver. Unit is in mC.
>>>> + */
>>>> +static int stts751_to_deg(s32 hw_val)
>>>> +{
>>>> +     return hw_val * 125 / 32;
>>>> +}
>>>> +
>>>> +static s16 stts751_to_hw(int val, s32 *hw_val)
>>>> +{
>>>> +     /* HW works in range -64C to +127C */
>>>> +     if ((val > 127000) || (val < -64000))
>>>
>>>
>>> Extra ( ). However, clamp_val(val, -64000, 127000) is preferred here
>>> instead of returning an error (we don't want to force users to guess
>>> valid ranges).
>>>
>>> The upper limit should probably be 127937, though, per datasheet.
>>
>>
>>
>> OK
>>
>>>> +             return -EINVAL;
>>>> +
>>>> +     if (val < 0)
>>>> +             *hw_val = (val - 62) / 125 * 32;
>>>> +     else
>>>> +             *hw_val = (val + 62) / 125 * 32;
>>>> +
>>>
>>> By having this function return an int, you could combine the 16-bit
>>> return
>>> value with the error code, and the extra parameter would not be necessary
>>> (even more so since this function should not return an error in the first
>>> place).
>>
>>
>> OK, now it's never failing.
>>>
>>>
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int stts751_adjust_resolution(struct stts751_priv *priv)
>>>> +{
>>>> +     u8 res;
>>>> +
>>>> +     switch (priv->interval) {
>>>> +     case 9:
>>>> +             /* 10 bits */
>>>> +             res = 0;
>>>> +             break;
>>>> +     case 8:
>>>> +             /* 11 bits */
>>>> +             res = 1;
>>>> +             break;
>>>> +     default:
>>>> +             /* 12 bits */
>>>> +             res = 3;
>>>> +             break;
>>>> +     }
>>>> +
>>>> +     if (priv->res == res)
>>>> +             return 0;
>>>> +
>>>> +     priv->config &= ~STTS751_CONF_RES_MASK;
>>>> +     priv->config |= res << STTS751_CONF_RES_SHIFT;
>>>> +
>>>> +     return i2c_smbus_write_byte_data(priv->client,
>>>> +                             STTS751_REG_CONF, priv->config);
>>>> +}
>>>> +
>>>> +static int stts751_update_temp(struct stts751_priv *priv)
>>>> +{
>>>> +     s32 integer1, integer2, frac;
>>>> +     int ret = 0;
>>>> +
>>>> +     mutex_lock(&priv->access_lock);
>>>> +
>>>> +     /*
>>>> +      * There is a trick here, like in the lm90 driver. We have to read
>>>> two
>>>> +      * registers to get the sensor temperature, but we have to beware
>>>> a
>>>> +      * conversion could occur between the readings. We could use the
>>>> +      * one-shot conversion register, but we don't want to do this
>>>> (disables
>>>> +      * hardware monitoring). So the solution used here is to read the
>>>> high
>>>> +      * byte once, then the low byte, then the high byte again. If the
>>>> new
>>>> +      * high byte matches the old one, then we have a valid reading.
>>>> Else we
>>>> +      * have to read the low byte again, and now we believe we have a
>>>> correct
>>>> +      * reading.
>>>> +      */
>>>> +     integer1 = i2c_smbus_read_byte_data(priv->client,
>>>> STTS751_REG_TEMP_H);
>>>> +     if (integer1 < 0) {
>>>> +             dev_dbg(&priv->client->dev,
>>>> +                     "I2C read failed (temp H). ret: %x\n", ret);
>>>> +             ret = integer1;
>>>> +             goto exit;
>>>> +     }
>>>> +
>>>> +     frac = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_L);
>>>> +     if (frac < 0) {
>>>> +             dev_dbg(&priv->client->dev,
>>>> +                     "I2C read failed (temp L). ret: %x\n", ret);
>>>> +             ret = frac;
>>>> +             goto exit;
>>>> +     }
>>>> +
>>>> +     integer2 = i2c_smbus_read_byte_data(priv->client,
>>>> STTS751_REG_TEMP_H);
>>>> +     if (integer2 < 0) {
>>>> +             dev_dbg(&priv->client->dev,
>>>> +                     "I2C 2nd read failed (temp H). ret: %x\n", ret);
>>>> +             ret = integer2;
>>>> +             goto exit;
>>>> +     }
>>>> +
>>>> +     if (integer1 != integer2) {
>>>> +             frac = i2c_smbus_read_byte_data(priv->client,
>>>> +                                             STTS751_REG_TEMP_L);
>>>> +             if (frac < 0) {
>>>> +                     dev_dbg(&priv->client->dev,
>>>> +                             "I2C 2nd read failed (temp L). ret: %x\n",
>>>> ret);
>>>> +                     ret = frac;
>>>> +                     goto exit;
>>>> +             }
>>>> +     }
>>>> +
>>>> +exit:
>>>> +     mutex_unlock(&priv->access_lock);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +     priv->temp = stts751_to_deg((integer1 << 8) | frac);
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +static int stts751_set_temp_reg(struct stts751_priv *priv, int temp,
>>>> +                             bool is_frac, u8 hreg, u8 lreg)
>>>> +{
>>>> +     s32 hwval;
>>>> +     int ret;
>>>> +
>>>> +     if (stts751_to_hw(temp, &hwval))
>>>> +             return -EINVAL;
>>>> +
>>>> +     mutex_lock(&priv->access_lock);
>>>> +     ret = i2c_smbus_write_byte_data(priv->client, hreg, hwval >> 8);
>>>> +     if (ret)
>>>> +             goto exit;
>>>> +     if (is_frac)
>>>> +             ret = i2c_smbus_write_byte_data(priv->client,
>>>> +                                             lreg, hwval & 0xff);
>>>> +exit:
>>>> +     mutex_unlock(&priv->access_lock);
>>>> +
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +static int stts751_update_alert(struct stts751_priv *priv)
>>>> +{
>>>> +     int ret;
>>>> +
>>>> +     /* not for us.. */
>>>> +     if (!priv->gen_event)
>>>> +             return 0;
>>>> +
>>>> +     ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_STATUS);
>>>> +
>>>
>>> Please no empty line between functions and checking of return values.
>>
>>
>> OK
>>
>>>> +     if (ret < 0)
>>>> +             return ret;
>>>> +
>>>> +     priv->max_alert = priv->max_alert || !!(ret &
>>>> STTS751_STATUS_TRIPH);
>>>> +     priv->min_alert = priv->min_alert || !!(ret &
>>>> STTS751_STATUS_TRIPL);
>>>> +
>>>
>>> I am not really happy about the caching of alert values. This isn't
>>> really
>>> common. Usually we report current alert values when reading. I don't
>>> really
>>> see why this driver should be different to all the others.
>>
>>
>> OK

I've tried to avoid caching, but as soon as I read the status register
in the alert handler, the device clears that reg, and it will remain
cleared up to the next conversion.

So for example if the reader is blocked on poll over the alert
attribute, it gets unblocked on alert, and it will very likely read
zero..

The only way I can see for avoiding caching, and for changing the API
about clearing events as you asked below, is performing a temperature
read from the temperature registers and doing software compare with
the limits in show_[max/min]_alert.

Does this sounds OK for you ?

>>
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static void stts751_alert(struct i2c_client *client,
>>>> +                     enum i2c_alert_protocol type, unsigned int data)
>>>> +{
>>>> +     int ret;
>>>> +     struct stts751_priv *priv = i2c_get_clientdata(client);
>>>> +     bool prev_max = priv->max_alert;
>>>> +     bool prev_min = priv->min_alert;
>>>> +
>>>> +     if (type != I2C_PROTOCOL_SMBUS_ALERT)
>>>> +             return;
>>>> +
>>>> +     dev_dbg(&client->dev, "alert!");
>>>> +
>>>
>>>
>>> Any chance to make this a bit more generic and also support
>>> interrupts ?
>>
>>
>> May you please elaborate?
>>
>> IMHO adding the logic for handling SMBus alerts using IRQs to any
>> HWmon driver is redundant, because it is already implemented in the
>> SMBus layer, just right now you need patching SMBus layer for enabling
>> it from the DT world (a couple of patches for this are floating
>> around, but they seems not ready for merge yet)... Using this chip
>> with a generic interrupt is indeed exactly what I'm doing..
>>
>> .. Or am I completely missing your point ?
>>
> "just right now you need patching SMBus layer for enabling it from the DT
> world"
>
> So it _doesn't_ work, correct ?

I mean, the generic smbus code works for stts751, and the chip seems
happy. So, in this case, no need to reimplement it elsewhere. It would
just end up to be a copy-paste from smbus.c.... Just AFAICT we need
some mechanism to hook smbus code from DT world..

> The problem with SMBus alert is that each chip implements it differently,
> to the point where it is difficult to implement it such that it supports
> more than one chip on the same bus. For the most part, chip interrupt
> lines are thus not connected to the i2c controller but to, for example,
> a gpio pin, and each chip on the same bus has a separate interrupt.
> I am looking forward to see how this is going to be supported in the
> i2c infrastructure. For my part, I long since gave up on it, but I'll
> be happy to stand corrected.

Hmm, I didn't know about all those weirdness in devices :(

BTW this sounds like saying that we need to make the smbus code
device-bound, rather bus-bound, but even in this case the smbus code
can still be generic rather than being buried in each slave driver..
At least for those device like stts751 for with the generic code
works..

Consider also that indeed we still need to send the ARAs over the bus,
and AFAIK those will hit all devices on the bus.. So this thing seems
that needs to be somehow centralized anyway.. I'm honestly unsure if I
could support alerts in, say, stts751 driver, handling by myself the
IRQ line and sending ARAs over the bus without impacting other devices
on the same bus, even if they have their own IRQs..

I mean even if I do implement all this stuff in my driver,  I'm not
sure it will help much when you have multiple devices with different
IRQs on the same bus.. I even suspect this can complicate things..  :/

... Please correct me if I'm wrong; any discussion on this topic would
be appreciated :)

BTW it seems that other hwmon drivers, like lm90, are doing the same,
relying on generic SMBus support.. For example lm90 doing
if (type != I2C_PROTOCOL_SMBUS_ALERT)
    return;

>
>>>
>>>> +     mutex_lock(&priv->access_lock);
>>>> +     ret = stts751_update_alert(priv);
>>>> +     if (ret < 0) {
>>>> +             /* default to worst case */
>>>> +             priv->max_alert = true;
>>>> +             priv->min_alert = true;
>>>> +
>>>> +             if (!(prev_max && prev_min)) {
>>>> +                     dev_warn(&priv->client->dev,
>>>> +                             "Alert received, but can't communicate to
>>>> the device. Something bad happening? Triggering all alarms!");
>>>> +             }
>>>> +     }
>>>> +
>>>> +     if (!prev_max && priv->max_alert) {
>>>> +             dev_notice(&client->dev, "got alert for HIGH
>>>> temperature");
>>>> +
>>>> +             /* unblock alert poll */
>>>> +             sysfs_notify(&priv->dev->kobj, NULL,
>>>> "temp1_event_max_alert");
>>>> +             kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
>>>> +     }
>>>> +
>>>> +     if (!prev_min && priv->min_alert) {
>>>> +             dev_notice(&client->dev, "got alert for LOW temperature");
>>>> +
>>>> +             /* unblock alert poll */
>>>> +             sysfs_notify(&priv->dev->kobj, NULL,
>>>> "temp1_event_min_alert");
>>>> +             kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
>>>> +     }
>>>> +     mutex_unlock(&priv->access_lock);
>>>> +}
>>>> +
>>>> +static ssize_t show_max_alert(struct device *dev, struct
>>>> device_attribute *attr,
>>>> +                       char *buf)
>>>> +{
>>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->max_alert);
>>>> +}
>>>> +
>>>> +static ssize_t set_max_alert(struct device *dev, struct
>>>> device_attribute *attr,
>>>> +                    const char *buf, size_t count)
>>>> +{
>>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +     mutex_lock(&priv->access_lock);
>>>> +     priv->max_alert = false;
>>>> +     mutex_unlock(&priv->access_lock);
>>>> +
>>>> +     return count;
>>>> +}
>>>> +
>>>> +static ssize_t show_min_alert(struct device *dev, struct
>>>> device_attribute *attr,
>>>> +                       char *buf)
>>>> +{
>>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->min_alert);
>>>> +}
>>>> +
>>>> +static ssize_t set_min_alert(struct device *dev, struct
>>>> device_attribute *attr,
>>>> +                    const char *buf, size_t count)
>>>> +{
>>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +     mutex_lock(&priv->access_lock);
>>>> +     priv->min_alert = false;
>>>> +     mutex_unlock(&priv->access_lock);
>>>> +
>>>
>>> This is highly unusual. If the alert needs to be cleared, please
>>> auto-clear it
>>> when reading.
>>
>>
>> OK
>>
>>>> +     return count;
>>>> +}
>>>> +
>>>> +static ssize_t show_input(struct device *dev, struct device_attribute
>>>> *attr,
>>>> +                       char *buf)
>>>> +{
>>>> +     int ret;
>>>> +     int cache_time = STTS751_CACHE_TIME * HZ / 1000;
>>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +     /*
>>>> +      * Adjust the cache time wrt the sample rate. We do 4X in order to
>>>> get
>>>> +      * a new measure in no more than 1/4 of the sample time (that
>>>> seemed
>>>> +      * reasonable to me).
>>>> +      */
>>>> +     cache_time = stts751_intervals[priv->interval] / 4 * HZ / 1000;
>>>> +
>>>> +     if (time_after(jiffies, priv->last_update + cache_time) ||
>>>> +             !priv->data_valid) {
>>>> +             ret = stts751_update_temp(priv);
>>>> +             if (ret)
>>>> +                     return ret;
>>>> +             priv->last_update = jiffies;
>>>> +             priv->data_valid = true;
>>>> +     }
>>>> +
>>>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->temp);
>>>> +}
>>>> +
>>>> +static ssize_t show_therm(struct device *dev, struct device_attribute
>>>> *attr,
>>>> +                     char *buf)
>>>> +{
>>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->therm);
>>>> +}
>>>> +
>>>> +
>>>> +static ssize_t set_therm(struct device *dev, struct device_attribute
>>>> *attr,
>>>> +                    const char *buf, size_t count)
>>>> +{
>>>> +     int ret;
>>>> +     long temp;
>>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +     if (kstrtol(buf, 10, &temp) < 0)
>>>> +             return -EINVAL;
>>>> +
>>>> +     ret = stts751_set_temp_reg(priv, temp, false, STTS751_REG_TLIM,
>>>> 0);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>
>>> Does it really add value to use the same function for 8- and 16-bit
>>> registers ?
>>> Seems to me it just makes the code more complicated.
>>
>>
>> I haven't a strong opinion about this.. it's this complication vs a
>> slight code duplication.. I've changed this in the way you suggest :)
>>
>>>> +     dev_dbg(dev, "setting therm %ld", temp);
>>>> +
>>>> +     priv->therm = temp;
>>>> +     return count;
>>>> +}
>>>> +
>>>> +static ssize_t show_hyst(struct device *dev, struct device_attribute
>>>> *attr,
>>>> +                     char *buf)
>>>> +{
>>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->hyst);
>>>> +}
>>>> +
>>>> +
>>>> +static ssize_t set_hyst(struct device *dev, struct device_attribute
>>>> *attr,
>>>> +                    const char *buf, size_t count)
>>>> +{
>>>> +     int ret;
>>>> +     long temp;
>>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +     if (kstrtol(buf, 10, &temp) < 0)
>>>> +             return -EINVAL;
>>>> +
>>>
>>> Per ABI, this is supposed to be an absolute temperature. For the chip,
>>> it is a delta. You'll have to do some calculation to convert the absolute
>>> value into a delta. Same for the show command.
>>
>>
>> OK
>>
>>>
>>>> +     ret = stts751_set_temp_reg(priv, temp, false, STTS751_REG_HYST,
>>>> 0);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +     dev_dbg(dev, "setting hyst %ld", temp);
>>>> +
>>>> +     priv->hyst = temp;
>>>> +     return count;
>>>> +}
>>>> +
>>>> +static ssize_t show_max(struct device *dev, struct device_attribute
>>>> *attr,
>>>> +                     char *buf)
>>>> +{
>>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_max);
>>>> +}
>>>> +
>>>> +static ssize_t set_max(struct device *dev, struct device_attribute
>>>> *attr,
>>>> +                    const char *buf, size_t count)
>>>> +{
>>>> +     int ret;
>>>> +     long temp;
>>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +     if (kstrtol(buf, 10, &temp) < 0)
>>>> +             return -EINVAL;
>>>> +
>>>> +     ret = stts751_set_temp_reg(priv, temp, true,
>>>> +                             STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +     dev_dbg(dev, "setting event max %ld", temp);
>>>> +     priv->event_max = temp;
>>>> +     return count;
>>>> +}
>>>> +
>>>> +static ssize_t show_min(struct device *dev, struct device_attribute
>>>> *attr,
>>>> +                     char *buf)
>>>> +{
>>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_min);
>>>> +}
>>>> +
>>>> +
>>>
>>>
>>> Single empty line, please.
>>
>>
>> OK
>>
>>>
>>>> +static ssize_t set_min(struct device *dev, struct device_attribute
>>>> *attr,
>>>> +                    const char *buf, size_t count)
>>>> +{
>>>> +     int ret;
>>>> +     long temp;
>>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +     if (kstrtol(buf, 10, &temp) < 0)
>>>> +             return -EINVAL;
>>>> +
>>>> +     ret = stts751_set_temp_reg(priv, temp, true,
>>>> +                             STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +     dev_dbg(dev, "setting event min %ld", temp);
>>>> +
>>>> +     priv->event_min = temp;
>>>> +     return count;
>>>> +}
>>>> +
>>>> +static ssize_t show_interval(struct device *dev, struct
>>>> device_attribute *attr,
>>>> +                          char *buf)
>>>> +{
>>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n",
>>>> +                     stts751_intervals[priv->interval]);
>>>> +}
>>>> +
>>>> +static ssize_t set_interval(struct device *dev, struct device_attribute
>>>> *attr,
>>>> +                         const char *buf, size_t count)
>>>> +{
>>>> +     unsigned long val;
>>>> +     int idx;
>>>> +     int ret = 0;
>>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>>> +
>>>> +     if (kstrtoul(buf, 10, &val) < 0)
>>>> +             return -EINVAL;
>>>> +
>>>> +     idx = find_closest_descending(val, stts751_intervals,
>>>> +                             ARRAY_SIZE(stts751_intervals));
>>>> +
>>>> +     dev_dbg(dev, "setting interval. req:%lu, idx: %d, val: %d", val,
>>>> idx,
>>>> +             stts751_intervals[idx]);
>>>> +
>>>> +     if (priv->interval == idx)
>>>> +             return count;
>>>> +
>>>> +     mutex_lock(&priv->access_lock);
>>>> +
>>>> +     /* speed up, lower the resolution, then modify convrate */
>>>> +     if (priv->interval < idx) {
>>>> +             priv->interval = idx;
>>>> +             ret = stts751_adjust_resolution(priv);
>>>> +             if (ret)
>>>> +                     goto exit;
>>>> +     }
>>>> +
>>>> +     ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_RATE,
>>>> idx);
>>>> +     if (ret)
>>>> +             goto exit;
>>>> +
>>>> +     /* slow down, modify convrate, then raise resolution */
>>>> +     if (priv->interval != idx) {
>>>> +             priv->interval = idx;
>>>> +             ret = stts751_adjust_resolution(priv);
>>>> +             if (ret)
>>>> +                     goto exit;
>>>> +
>>>
>>> Extra empty line.
>>>
>>> Is it really necessary to have this code twice ? What is the harm in
>>> just doing it once, either before or after setting the conversion rate ?
>>
>>
>> In early development stages I've become suspicious about the chip
>> starting to misbehave if I ever set, even briefly, an invalid
>> configuration.
>>
>> I tried to avoid that by setting rate and resolution in such an order
>> that never ends up in an invalid configuration. The order is thus not
>> fixed and depends by what changes are you doing..
>>
>> (I'm not completely sure: I've hit something strange and I thought it
>> could be because of this, and once I changed my code this way I never
>> hit that again)..
>>
> Please add a respective comment into the code describing the reasons.

OK

>
>>>> +     }
>>>> +exit:
>>>> +     mutex_unlock(&priv->access_lock);
>>>> +
>>>> +     return count;
>>>> +}
>>>> +
>>>> +static int stts751_detect(struct i2c_client *new_client,
>>>> +                       struct i2c_board_info *info)
>>>> +{
>>>> +     struct i2c_adapter *adapter = new_client->adapter;
>>>> +     const char *name;
>>>> +     int mfg_id, prod_id, rev_id;
>>>> +
>>>> +     if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
>>>> +             return -ENODEV;
>>>> +
>>>> +     mfg_id = i2c_smbus_read_byte_data(new_client, ST_MAN_ID);
>>>> +     if (mfg_id != ST_MAN_ID)
>>>> +             return -ENODEV;
>>>> +
>>>> +     prod_id = i2c_smbus_read_byte_data(new_client,
>>>> STTS751_REG_PROD_ID);
>>>> +
>>>> +     switch (prod_id) {
>>>> +     case STTS751_0_PROD_ID:
>>>> +             name = "STTS751-0";
>>>> +             break;
>>>> +     case STTS751_1_PROD_ID:
>>>> +             name = "STTS751-1";
>>>> +             break;
>>>> +     default:
>>>> +             return -ENODEV;
>>>> +     }
>>>> +     dev_info(&new_client->dev, "Chip %s detected!", name);
>>>> +
>>>
>>> Is this noise necessary ?
>>
>>
>> OK, changed in dev_dbg.
>>
>>>
>>>> +     rev_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_REV_ID);
>>>> +
>>>> +     if (rev_id != 0x1) {
>>>
>>>
>>> You might want to check for an error here (and return -ENODEV if you get
>>> one).
>>
>>
>> OK
>>
>>>> +             dev_notice(&new_client->dev,
>>>> +                     "Chip revision 0x%x is untested\nPlease report
>>>> whether it works to andrea.merello@gmail.com",
>>>> +                     rev_id);
>>>> +     }
>>>> +
>>>> +     strlcpy(info->type, name, I2C_NAME_SIZE);
>>>
>>>
>>> This doesn't match the driver name (or, rather, the values in
>>> stts751_id). Does this instantiate the driver ? That would surprise
>>> me.
>>
>>
>> It does, but I'll double check what the heck I've done here :)
>>
>
> Either case this should return a string matching stts751_id[], otherwise
> auto-detection
> and manual or devicetree based instantiation would return different names.

I'm going to stick to the only one string we have in stts751id_[].
Differentiating STTS751-0 from STTS751-1 indeed seems useless for
anything but reporting that name in dev_dbg.

> Does your use case even include the detect function ?

Oh.. I've checked, and it seems that right now no one is ever calling
my detect funcion indeed..

>
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int stts751_init_chip(struct stts751_priv *priv)
>>>> +{
>>>> +     int ret;
>>>> +
>>>> +     priv->config = STTS751_CONF_EVENT_DIS | STTS751_CONF_STOP;
>>>> +     ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_CONF,
>>>> +                                     priv->config);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +     ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_RATE,
>>>> +                                     priv->interval);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +     /* invalid, to force update */
>>>> +     priv->res = -1;
>>>> +     ret = stts751_adjust_resolution(priv);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +     ret = i2c_smbus_write_byte_data(priv->client,
>>>> +                                     STTS751_REG_SMBUS_TO,
>>>> +                                     priv->smbus_timeout ? 0x80 : 0);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +
>>>> +     if (priv->gen_event) {
>>>> +             ret = stts751_set_temp_reg(priv, priv->event_max, true,
>>>> +                                     STTS751_REG_HLIM_H,
>>>> STTS751_REG_HLIM_L);
>>>> +             if (ret)
>>>> +                     return ret;
>>>> +
>>>> +             ret = stts751_set_temp_reg(priv, priv->event_min, true,
>>>> +                                     STTS751_REG_LLIM_H,
>>>> STTS751_REG_LLIM_L);
>>>
>>>
>>> Those registers all have power-up defaults. Unless there is a good
>>> reason,
>>> we should not overwrite them. In some applications, the values may be
>>> initialized from the BIOS/ROMMON.
>>
>>
>> OK. I've changed it for all those regs but the smsbus timeout stuff:
>> I think that this depends by the kind of I2C controller the device is
>> attached to, so I'd still getting that from DT. Even if someone
>> already initialized it (assuming it did it correctly, and the DT is
>> correct also) I'll eventually end up in overwriting it with the same
>> value.
>>
>> Do you agree with this ?
>>
> Ok to overwrite it if you have devicetree data, but otherwise you should
> not touch it.

OK

>
>>>> +             if (ret)
>>>> +                     return ret;
>>>> +             priv->config &= ~STTS751_CONF_EVENT_DIS;
>>>> +     }
>>>> +
>>>> +     if (priv->gen_therm) {
>>>> +             ret = stts751_set_temp_reg(priv, priv->therm, false,
>>>> +                                     STTS751_REG_TLIM, 0);
>>>> +             if (ret)
>>>> +                     return ret;
>>>> +
>>>> +             ret = stts751_set_temp_reg(priv, priv->hyst, false,
>>>> +                                     STTS751_REG_HYST, 0);
>>>> +             if (ret)
>>>> +                     return ret;
>>>> +     }
>>>> +
>>>> +     priv->config &= ~STTS751_CONF_STOP;
>>>> +     ret = i2c_smbus_write_byte_data(priv->client,
>>>> +                                     STTS751_REG_CONF, priv->config);
>>>> +
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
>>>> +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO,
>>>> +                     show_min, set_min, 0);
>>>> +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO,
>>>> +                     show_max, set_max, 0);
>>>> +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO,
>>>> +                     show_min_alert, set_min_alert, 0);
>>>> +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO,
>>>> +                     show_max_alert, set_max_alert, 0);
>>>> +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,
>>>> +                     set_therm, 0);
>>>> +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO,
>>>> show_hyst,
>>>> +                     set_hyst, 0);
>>>
>>>
>>> Please use standard attribute names.
>>
>>
>> I can convert the event stuff in temp1_[min/max]_alarm for limits and
>> temp1_alarm for the flag, but it's not obvious to be what should I use
>> for the therm stuff.. May you please give me advices about this?
>>
>> I'd propose temp1_therm_[max/min]_hyst, but they seems not standard
>> anyway...
>>
>
> From the chip datasheet, this is supposed to reflect overtemperature
> conditions,
> stronger than high/low, thus 'crit' seems appropriate.

I see.. I can go with 'crit' :)

Just is there any place where I can document this? Despite the
datasheet, I can easily imagine real-world usage scenarios, for
example driving a fan from that signal, where 'crit' might be not so
intuitive.. Indeed I would like to make it visible the 'crit'
<->'therm' binding to users. Maybe an additional attribute
'temp1_crit_label' or something like that ?

>
>>>
>>>> +static SENSOR_DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO,
>>>> +                     show_interval, set_interval, 0);
>>>> +
>>>> +/* always present */
>>>> +static struct attribute *stts751_temp_attrs[] = {
>>>> +     &sensor_dev_attr_temp1_input.dev_attr.attr,
>>>> +     NULL
>>>> +};
>>>> +
>>>> +static struct attribute_group stts751_temp_group = {
>>>> +     .attrs = stts751_temp_attrs,
>>>> +};
>>>> +
>>>> +/* present when therm pin or event pin are connected */
>>>> +static struct attribute *stts751_interval_attrs[] = {
>>>> +     &sensor_dev_attr_update_interval.dev_attr.attr,
>>>> +     NULL
>>>> +};
>>>> +
>>>> +static struct attribute_group stts751_interval_group = {
>>>> +     .attrs = stts751_interval_attrs,
>>>> +};
>>>> +
>>>> +/* present when event pin is connected */
>>>> +static struct attribute *stts751_event_attrs[] = {
>>>> +     &sensor_dev_attr_temp1_event_min.dev_attr.attr,
>>>> +     &sensor_dev_attr_temp1_event_max.dev_attr.attr,
>>>> +     &sensor_dev_attr_temp1_event_min_alert.dev_attr.attr,
>>>> +     &sensor_dev_attr_temp1_event_max_alert.dev_attr.attr,
>>>> +     NULL
>>>> +};
>>>> +
>>>> +static struct attribute_group stts751_event_group = {
>>>> +     .attrs = stts751_event_attrs,
>>>> +};
>>>> +
>>>> +/* present when therm pin is connected */
>>>> +static struct attribute *stts751_therm_attrs[] = {
>>>> +     &sensor_dev_attr_temp1_therm.dev_attr.attr,
>>>> +     &sensor_dev_attr_temp1_therm_hyst.dev_attr.attr,
>>>> +     NULL
>>>> +};
>>>> +
>>>> +static struct attribute_group stts751_therm_group = {
>>>> +     .attrs = stts751_therm_attrs,
>>>> +};
>>>> +
>>>> +static int stts751_probe(struct i2c_client *client,
>>>> +                      const struct i2c_device_id *id)
>>>> +{
>>>> +     struct stts751_priv *priv;
>>>> +     int ret;
>>>> +     int groups_idx = 0;
>>>> +     struct device_node *np = client->dev.of_node;
>>>> +
>>>> +     priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>>>> +     if (!priv)
>>>> +             return -ENOMEM;
>>>> +
>>>> +     priv->client = client;
>>>> +     i2c_set_clientdata(client, priv);
>>>> +     mutex_init(&priv->access_lock);
>>>> +
>>>> +     /* default to 2 samples per second */
>>>> +     priv->interval = 5;
>>>> +     /* default to timeout enable, as per chip default */
>>>> +     priv->smbus_timeout = true;
>>>> +     priv->last_update = 0;
>>>> +     priv->data_valid = false;
>>>> +     priv->max_alert = false;
>>>> +     priv->min_alert = false;
>>>> +     priv->gen_therm = false;
>>>> +     priv->gen_event = false;
>>>
>>>
>>> Unnecessary initializations.
>>
>>
>> OK
>>
>>>
>>>> +     priv->therm = STTS751_THERM_DEFAULT;
>>>> +     priv->hyst = STTS751_HYST_DEFAULT;
>>>> +     priv->event_max = STTS751_EVENT_MAX_DEFAULT;
>>>> +     priv->event_min = STTS751_EVENT_MIN_DEFAULT;
>>>> +
>>>
>>> I think it would be better to read the initial values from the chip
>>> (also see above).
>>>
>>
>> OK
>>
>>>> +     if (np) {
>>>> +             priv->gen_therm = of_property_read_bool(np, "has-therm");
>>>> +             priv->gen_event = of_property_read_bool(np, "has-event");
>>>> +             priv->smbus_timeout = !of_property_read_bool(np,
>>>> +                                             "smbus-timeout-disable");
>>>> +     } else {
>>>> +             dev_notice(&client->dev, "No DT data. Event/therm
>>>> disabled\n");
>>>
>>>
>>> Seems to be quite rude to penaltize non-DT systems this way.
>>
>>
>> That's gone since the two props "has-therm" and "has-event" are now
>> gone. Any suggestion please about how to get the
>> "smbus-timeout-disable" stuff in non-DT case ?
>>
> Platform data, or better use device properties for both cases.

OK, I've switched to device properties.

>
>>>> +     }
>>>> +
>>>> +     dev_dbg(&client->dev, "gen_event: %s, gen_therm: %s",
>>>> +             priv->gen_event ? "YES" : "NO",
>>>> +             priv->gen_therm ? "YES" : "NO");
>>>> +
>>>> +     priv->groups[groups_idx++] = &stts751_temp_group;
>>>> +     priv->groups[groups_idx++] = &stts751_interval_group;
>>>
>>>
>>> Any reason for having multiple groups here ?
>>
>>
>> Not anymore, as per your suggesion below, they are gone :)
>>
>>>> +
>>>> +     if (priv->gen_therm)
>>>> +             priv->groups[groups_idx++] = &stts751_therm_group;
>>>> +
>>>> +     if (priv->gen_event)
>>>> +             priv->groups[groups_idx++] = &stts751_event_group;
>>>> +
>>>
>>> Why are those groups optional ? This is quite unusual. Even if the chip
>>> is not
>>> connected to alert or interrupt lines, it still supports limit registers
>>> and
>>> reports alerts. On top of that, at least interrupt support can be
>>> detected
>>> automatically (from client->irq) and would not need a devicetree
>>> property.
>>
>>
>> Props "has-therm" and "has-event" gone; please see above about IRQ stuf..
>>
>>> I think you really only need a single group, and all attributes should
>>> always
>>> be available.
>>
>>
>> OK
>>
>>>
>>>> +     priv->groups[groups_idx] = NULL;
>>>
>>>
>>> Unnecessary initialization. devm_kzalloc() initializes priv with 0.
>>
>>
>> OK
>>
>>>> +
>>>> +     ret = stts751_init_chip(priv);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +     priv->dev = devm_hwmon_device_register_with_groups(&client->dev,
>>>> +                                                     client->name,
>>>> priv,
>>>> +                                                     priv->groups);
>>>> +     return PTR_ERR_OR_ZERO(priv->dev);
>>>> +}
>>>> +
>>>> +static const struct i2c_device_id stts751_id[] = {
>>>> +     { "stts751", 0 },
>>>> +     { }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(i2c, stts751_id);
>>>> +
>>>> +static struct i2c_driver stts751_driver = {
>>>> +     .class          = I2C_CLASS_HWMON,
>>>> +     .driver = {
>>>> +             .name   = DEVNAME,
>>>> +     },
>>>> +     .probe          = stts751_probe,
>>>> +     .id_table       = stts751_id,
>>>> +     .detect         = stts751_detect,
>>>> +     .alert          = stts751_alert,
>>>> +     .address_list   = normal_i2c,
>>>> +};
>>>> +
>>>> +module_i2c_driver(stts751_driver);
>>>> +
>>>> +MODULE_AUTHOR("Andrea Merello <andrea.merello@gmail.com>");
>>>> +MODULE_DESCRIPTION("STTS751 sensor driver");
>>>> +MODULE_LICENSE("GPL");
>>>> --
>>>> 2.7.4
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-hwmon"
>>>> in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>

Re: [PATCH v2 2/2] hwmon: new driver for ST stts751 thermal sensor

[Guenter Roeck]

On 10/19/2016 05:16 AM, Andrea Merello wrote:
> On Wed, Oct 5, 2016 at 1:51 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On Tue, Sep 27, 2016 at 02:07:27PM +0200, Andrea Merello wrote:
>>> This patch adds a HWMON driver for ST Microelectronics STTS751
>>> temperature sensors.
>>>
>>> It does support temperature reading (of course), SMBUS alert
>>> functionality and "therm" output.
>>>
>>> Thanks-to: LABBE Corentin [for suggestions]
>>> Thanks-to: Guenter Roeck [for suggestions/discussions]
>>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>>> Cc: LABBE Corentin <clabbe.montjoie@gmail.com>
>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>> Cc: Jean Delvare <jdelvare@suse.com>
>>> ---
>>>  drivers/hwmon/Kconfig   |  12 +-
>>>  drivers/hwmon/Makefile  |   2 +-
>>>  drivers/hwmon/stts751.c | 805 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 817 insertions(+), 2 deletions(-)
>>>  create mode 100644 drivers/hwmon/stts751.c
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index eaf2f91..4e70b42 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -1448,6 +1448,16 @@ config SENSORS_SCH5636
>>>         This driver can also be built as a module.  If so, the module
>>>         will be called sch5636.
>>>
>>> +config SENSORS_STTS751
>>> +     tristate "ST Microelectronics STTS751"
>>> +     depends on I2C
>>> +     help
>>> +       If you say yes here you get support for STTS751
>>> +       temperature sensor chips.
>>> +
>>> +       This driver can also be built as a module. If so, the module
>>> +       will be called stts751.
>>> +
>>>  config SENSORS_SMM665
>>>       tristate "Summit Microelectronics SMM665"
>>>       depends on I2C
>>> @@ -1506,7 +1516,7 @@ config SENSORS_ADS7871
>>>
>>>  config SENSORS_AMC6821
>>>       tristate "Texas Instruments AMC6821"
>>> -     depends on I2C
>>> +     depends on I2C
>>
>> ???
>
> My editor is kind enough to automatically kill trailing spaces.. I'll
> take care of moving this one in another patch in next series version..
>
>>
>>>       help
>>>         If you say yes here you get support for the Texas Instruments
>>>         AMC6821 hardware monitoring chips.
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index fe87d28..1114130 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -147,6 +147,7 @@ obj-$(CONFIG_SENSORS_SMM665)      += smm665.o
>>>  obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
>>>  obj-$(CONFIG_SENSORS_SMSC47M1)       += smsc47m1.o
>>>  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>>> +obj-$(CONFIG_SENSORS_STTS751)        += stts751.o
>>>  obj-$(CONFIG_SENSORS_AMC6821)        += amc6821.o
>>>  obj-$(CONFIG_SENSORS_TC74)   += tc74.o
>>>  obj-$(CONFIG_SENSORS_THMC50) += thmc50.o
>>> @@ -169,4 +170,3 @@ obj-$(CONFIG_SENSORS_WM8350)      += wm8350-hwmon.o
>>>  obj-$(CONFIG_PMBUS)          += pmbus/
>>>
>>>  ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
>>> -
>>
>> Looks like an unnecessary whitespace change.
>
> Like above..
>
>>
>>> diff --git a/drivers/hwmon/stts751.c b/drivers/hwmon/stts751.c
>>> new file mode 100644
>>> index 0000000..8358e04
>>> --- /dev/null
>>> +++ b/drivers/hwmon/stts751.c
>>> @@ -0,0 +1,805 @@
>>> +/*
>>> + * STTS751 sensor driver
>>> + *
>>> + * Copyright (C) 2016 Istituto Italiano di Tecnologia - RBCS - EDL
>>> + * Robotics, Brain and Cognitive Sciences department
>>> + * Electronic Design Laboratory
>>> + *
>>> + * Written by Andrea Merello <andrea.merello@gmail.com>
>>> + *
>>> + * Based on  LM95241 driver and LM90 driver
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +
>> No douple empty lines please.
>
> OK. BTW do you use some enhanced "checkpatch" script in order to catch
> issues like these?
>

In this case my brain, but checkpatch --strict reports it as well.

>>
>>> +#include <linux/bitops.h>
>>> +#include <linux/err.h>
>>> +#include <linux/hwmon.h>
>>> +#include <linux/hwmon-sysfs.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/init.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/jiffies.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/of_irq.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/sysfs.h>
>>> +#include <linux/util_macros.h>
>>> +
>>> +#define DEVNAME "stts751"
>>> +
>>> +static const unsigned short normal_i2c[] = {
>>> +     0x48, 0x49, 0x38, 0x39,  /* STTS751-0 */
>>> +     0x4A, 0x4B, 0x3A, 0x3B,  /* STTS751-1 */
>>> +     I2C_CLIENT_END };
>>> +
>>> +#define STTS751_REG_TEMP_H   0x00
>>> +#define STTS751_REG_STATUS   0x01
>>> +#define STTS751_STATUS_TRIPL BIT(5)
>>> +#define STTS751_STATUS_TRIPH BIT(6)
>>> +#define STTS751_STATUS_BUSY  BIT(8)
>>> +#define STTS751_REG_TEMP_L   0x02
>>> +#define STTS751_REG_CONF     0x03
>>> +#define STTS751_CONF_RES_MASK        0x0C
>>> +#define STTS751_CONF_RES_SHIFT  2
>>> +#define STTS751_CONF_EVENT_DIS  BIT(7)
>>> +#define STTS751_CONF_STOP    BIT(6)
>>> +#define STTS751_REG_RATE     0x04
>>> +#define STTS751_REG_HLIM_H   0x05
>>> +#define STTS751_REG_HLIM_L   0x06
>>> +#define STTS751_REG_LLIM_H   0x07
>>> +#define STTS751_REG_LLIM_L   0x08
>>> +#define STTS751_REG_ONESHOT  0x0F
>>> +#define STTS751_REG_TLIM     0x20
>>> +#define STTS751_REG_HYST     0x21
>>> +#define STTS751_REG_SMBUS_TO 0x22
>>> +
>>> +#define STTS751_REG_PROD_ID  0xFD
>>> +#define STTS751_REG_MAN_ID   0xFE
>>> +#define STTS751_REG_REV_ID   0xFF
>>> +
>>> +#define STTS751_0_PROD_ID    0x00
>>> +#define STTS751_1_PROD_ID    0x01
>>> +#define ST_MAN_ID            0x53
>>> +
>>> +/* stick with HW defaults */
>>> +#define STTS751_THERM_DEFAULT        85000
>>> +#define STTS751_HYST_DEFAULT 10000
>>> +#define STTS751_EVENT_MAX_DEFAULT 85000
>>> +#define STTS751_EVENT_MIN_DEFAULT 0
>>> +
>>> +#define STTS751_RACE_RETRY   5
>>> +#define STTS751_CONV_TIMEOUT 100 /* mS */
>>> +#define STTS751_CACHE_TIME   100 /* mS */
>>> +
>>> +/*
>>> + * Possible update intervals are (in mS):
>>> + * 16000, 8000, 4000, 2000, 1000, 500, 250, 125, 62.5, 31.25
>>> + * However we are not going to complicate things too much and we stick to the
>>> + * approx value in mS.
>>> + */
>>> +static const int stts751_intervals[] = {
>>> +     16000, 8000, 4000, 2000, 1000, 500, 250, 125, 63, 31
>>> +};
>>> +
>>> +struct stts751_priv {
>>> +     struct device *dev;
>>> +     struct i2c_client *client;
>>> +     struct mutex access_lock;
>>> +     unsigned long interval;
>>> +     int res;
>>> +     bool gen_therm, gen_event;
>>> +     int event_max, event_min;
>>> +     int therm;
>>> +     int hyst;
>>> +     bool smbus_timeout;
>>> +     int temp;
>>> +     unsigned long last_update;
>>> +     u8 config;
>>> +     bool min_alert, max_alert;
>>> +     bool data_valid;
>>> +
>>> +     /*
>>> +      * Temperature/interval is always present.
>>> +      * Depending by DT therm and event are dynamically added.
>>> +      * There are max 4 entries plus the guard
>>> +      */
>>> +     const struct attribute_group *groups[5];
>>> +};
>>> +
>>> +/*
>>> + * These functions converts temperature from HW format to integer format and
>>> + * vice-vers. They are (mostly) taken from lm90 driver. Unit is in mC.
>>> + */
>>> +static int stts751_to_deg(s32 hw_val)
>>> +{
>>> +     return hw_val * 125 / 32;
>>> +}
>>> +
>>> +static s16 stts751_to_hw(int val, s32 *hw_val)
>>> +{
>>> +     /* HW works in range -64C to +127C */
>>> +     if ((val > 127000) || (val < -64000))
>>
>> Extra ( ). However, clamp_val(val, -64000, 127000) is preferred here
>> instead of returning an error (we don't want to force users to guess
>> valid ranges).
>>
>> The upper limit should probably be 127937, though, per datasheet.
>
>
> OK
>
>>> +             return -EINVAL;
>>> +
>>> +     if (val < 0)
>>> +             *hw_val = (val - 62) / 125 * 32;
>>> +     else
>>> +             *hw_val = (val + 62) / 125 * 32;
>>> +
>> By having this function return an int, you could combine the 16-bit return
>> value with the error code, and the extra parameter would not be necessary
>> (even more so since this function should not return an error in the first
>> place).
>
> OK, now it's never failing.
>>
>>> +     return 0;
>>> +}
>>> +
>>> +static int stts751_adjust_resolution(struct stts751_priv *priv)
>>> +{
>>> +     u8 res;
>>> +
>>> +     switch (priv->interval) {
>>> +     case 9:
>>> +             /* 10 bits */
>>> +             res = 0;
>>> +             break;
>>> +     case 8:
>>> +             /* 11 bits */
>>> +             res = 1;
>>> +             break;
>>> +     default:
>>> +             /* 12 bits */
>>> +             res = 3;
>>> +             break;
>>> +     }
>>> +
>>> +     if (priv->res == res)
>>> +             return 0;
>>> +
>>> +     priv->config &= ~STTS751_CONF_RES_MASK;
>>> +     priv->config |= res << STTS751_CONF_RES_SHIFT;
>>> +
>>> +     return i2c_smbus_write_byte_data(priv->client,
>>> +                             STTS751_REG_CONF, priv->config);
>>> +}
>>> +
>>> +static int stts751_update_temp(struct stts751_priv *priv)
>>> +{
>>> +     s32 integer1, integer2, frac;
>>> +     int ret = 0;
>>> +
>>> +     mutex_lock(&priv->access_lock);
>>> +
>>> +     /*
>>> +      * There is a trick here, like in the lm90 driver. We have to read two
>>> +      * registers to get the sensor temperature, but we have to beware a
>>> +      * conversion could occur between the readings. We could use the
>>> +      * one-shot conversion register, but we don't want to do this (disables
>>> +      * hardware monitoring). So the solution used here is to read the high
>>> +      * byte once, then the low byte, then the high byte again. If the new
>>> +      * high byte matches the old one, then we have a valid reading. Else we
>>> +      * have to read the low byte again, and now we believe we have a correct
>>> +      * reading.
>>> +      */
>>> +     integer1 = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_H);
>>> +     if (integer1 < 0) {
>>> +             dev_dbg(&priv->client->dev,
>>> +                     "I2C read failed (temp H). ret: %x\n", ret);
>>> +             ret = integer1;
>>> +             goto exit;
>>> +     }
>>> +
>>> +     frac = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_L);
>>> +     if (frac < 0) {
>>> +             dev_dbg(&priv->client->dev,
>>> +                     "I2C read failed (temp L). ret: %x\n", ret);
>>> +             ret = frac;
>>> +             goto exit;
>>> +     }
>>> +
>>> +     integer2 = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_H);
>>> +     if (integer2 < 0) {
>>> +             dev_dbg(&priv->client->dev,
>>> +                     "I2C 2nd read failed (temp H). ret: %x\n", ret);
>>> +             ret = integer2;
>>> +             goto exit;
>>> +     }
>>> +
>>> +     if (integer1 != integer2) {
>>> +             frac = i2c_smbus_read_byte_data(priv->client,
>>> +                                             STTS751_REG_TEMP_L);
>>> +             if (frac < 0) {
>>> +                     dev_dbg(&priv->client->dev,
>>> +                             "I2C 2nd read failed (temp L). ret: %x\n", ret);
>>> +                     ret = frac;
>>> +                     goto exit;
>>> +             }
>>> +     }
>>> +
>>> +exit:
>>> +     mutex_unlock(&priv->access_lock);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     priv->temp = stts751_to_deg((integer1 << 8) | frac);
>>> +     return ret;
>>> +}
>>> +
>>> +static int stts751_set_temp_reg(struct stts751_priv *priv, int temp,
>>> +                             bool is_frac, u8 hreg, u8 lreg)
>>> +{
>>> +     s32 hwval;
>>> +     int ret;
>>> +
>>> +     if (stts751_to_hw(temp, &hwval))
>>> +             return -EINVAL;
>>> +
>>> +     mutex_lock(&priv->access_lock);
>>> +     ret = i2c_smbus_write_byte_data(priv->client, hreg, hwval >> 8);
>>> +     if (ret)
>>> +             goto exit;
>>> +     if (is_frac)
>>> +             ret = i2c_smbus_write_byte_data(priv->client,
>>> +                                             lreg, hwval & 0xff);
>>> +exit:
>>> +     mutex_unlock(&priv->access_lock);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static int stts751_update_alert(struct stts751_priv *priv)
>>> +{
>>> +     int ret;
>>> +
>>> +     /* not for us.. */
>>> +     if (!priv->gen_event)
>>> +             return 0;
>>> +
>>> +     ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_STATUS);
>>> +
>> Please no empty line between functions and checking of return values.
>
> OK
>
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     priv->max_alert = priv->max_alert || !!(ret & STTS751_STATUS_TRIPH);
>>> +     priv->min_alert = priv->min_alert || !!(ret & STTS751_STATUS_TRIPL);
>>> +
>> I am not really happy about the caching of alert values. This isn't really
>> common. Usually we report current alert values when reading. I don't really
>> see why this driver should be different to all the others.
>
> OK
>
>>> +     return 0;
>>> +}
>>> +
>>> +static void stts751_alert(struct i2c_client *client,
>>> +                     enum i2c_alert_protocol type, unsigned int data)
>>> +{
>>> +     int ret;
>>> +     struct stts751_priv *priv = i2c_get_clientdata(client);
>>> +     bool prev_max = priv->max_alert;
>>> +     bool prev_min = priv->min_alert;
>>> +
>>> +     if (type != I2C_PROTOCOL_SMBUS_ALERT)
>>> +             return;
>>> +
>>> +     dev_dbg(&client->dev, "alert!");
>>> +
>>
>> Any chance to make this a bit more generic and also support
>> interrupts ?
>
> May you please elaborate?
>
> IMHO adding the logic for handling SMBus alerts using IRQs to any
> HWmon driver is redundant, because it is already implemented in the
> SMBus layer, just right now you need patching SMBus layer for enabling
> it from the DT world (a couple of patches for this are floating
> around, but they seems not ready for merge yet)... Using this chip
> with a generic interrupt is indeed exactly what I'm doing..
>
> .. Or am I completely missing your point ?
>
"just right now you need patching SMBus layer for enabling it from the DT world"

So it _doesn't_ work, correct ?

The problem with SMBus alert is that each chip implements it differently,
to the point where it is difficult to implement it such that it supports
more than one chip on the same bus. For the most part, chip interrupt
lines are thus not connected to the i2c controller but to, for example,
a gpio pin, and each chip on the same bus has a separate interrupt.
I am looking forward to see how this is going to be supported in the
i2c infrastructure. For my part, I long since gave up on it, but I'll
be happy to stand corrected.

>>
>>> +     mutex_lock(&priv->access_lock);
>>> +     ret = stts751_update_alert(priv);
>>> +     if (ret < 0) {
>>> +             /* default to worst case */
>>> +             priv->max_alert = true;
>>> +             priv->min_alert = true;
>>> +
>>> +             if (!(prev_max && prev_min)) {
>>> +                     dev_warn(&priv->client->dev,
>>> +                             "Alert received, but can't communicate to the device. Something bad happening? Triggering all alarms!");
>>> +             }
>>> +     }
>>> +
>>> +     if (!prev_max && priv->max_alert) {
>>> +             dev_notice(&client->dev, "got alert for HIGH temperature");
>>> +
>>> +             /* unblock alert poll */
>>> +             sysfs_notify(&priv->dev->kobj, NULL, "temp1_event_max_alert");
>>> +             kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
>>> +     }
>>> +
>>> +     if (!prev_min && priv->min_alert) {
>>> +             dev_notice(&client->dev, "got alert for LOW temperature");
>>> +
>>> +             /* unblock alert poll */
>>> +             sysfs_notify(&priv->dev->kobj, NULL, "temp1_event_min_alert");
>>> +             kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
>>> +     }
>>> +     mutex_unlock(&priv->access_lock);
>>> +}
>>> +
>>> +static ssize_t show_max_alert(struct device *dev, struct device_attribute *attr,
>>> +                       char *buf)
>>> +{
>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->max_alert);
>>> +}
>>> +
>>> +static ssize_t set_max_alert(struct device *dev, struct device_attribute *attr,
>>> +                    const char *buf, size_t count)
>>> +{
>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +     mutex_lock(&priv->access_lock);
>>> +     priv->max_alert = false;
>>> +     mutex_unlock(&priv->access_lock);
>>> +
>>> +     return count;
>>> +}
>>> +
>>> +static ssize_t show_min_alert(struct device *dev, struct device_attribute *attr,
>>> +                       char *buf)
>>> +{
>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->min_alert);
>>> +}
>>> +
>>> +static ssize_t set_min_alert(struct device *dev, struct device_attribute *attr,
>>> +                    const char *buf, size_t count)
>>> +{
>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +     mutex_lock(&priv->access_lock);
>>> +     priv->min_alert = false;
>>> +     mutex_unlock(&priv->access_lock);
>>> +
>> This is highly unusual. If the alert needs to be cleared, please auto-clear it
>> when reading.
>
> OK
>
>>> +     return count;
>>> +}
>>> +
>>> +static ssize_t show_input(struct device *dev, struct device_attribute *attr,
>>> +                       char *buf)
>>> +{
>>> +     int ret;
>>> +     int cache_time = STTS751_CACHE_TIME * HZ / 1000;
>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +     /*
>>> +      * Adjust the cache time wrt the sample rate. We do 4X in order to get
>>> +      * a new measure in no more than 1/4 of the sample time (that seemed
>>> +      * reasonable to me).
>>> +      */
>>> +     cache_time = stts751_intervals[priv->interval] / 4 * HZ / 1000;
>>> +
>>> +     if (time_after(jiffies, priv->last_update + cache_time) ||
>>> +             !priv->data_valid) {
>>> +             ret = stts751_update_temp(priv);
>>> +             if (ret)
>>> +                     return ret;
>>> +             priv->last_update = jiffies;
>>> +             priv->data_valid = true;
>>> +     }
>>> +
>>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->temp);
>>> +}
>>> +
>>> +static ssize_t show_therm(struct device *dev, struct device_attribute *attr,
>>> +                     char *buf)
>>> +{
>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->therm);
>>> +}
>>> +
>>> +
>>> +static ssize_t set_therm(struct device *dev, struct device_attribute *attr,
>>> +                    const char *buf, size_t count)
>>> +{
>>> +     int ret;
>>> +     long temp;
>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +     if (kstrtol(buf, 10, &temp) < 0)
>>> +             return -EINVAL;
>>> +
>>> +     ret = stts751_set_temp_reg(priv, temp, false, STTS751_REG_TLIM, 0);
>>> +     if (ret)
>>> +             return ret;
>>> +
>> Does it really add value to use the same function for 8- and 16-bit registers ?
>> Seems to me it just makes the code more complicated.
>
> I haven't a strong opinion about this.. it's this complication vs a
> slight code duplication.. I've changed this in the way you suggest :)
>
>>> +     dev_dbg(dev, "setting therm %ld", temp);
>>> +
>>> +     priv->therm = temp;
>>> +     return count;
>>> +}
>>> +
>>> +static ssize_t show_hyst(struct device *dev, struct device_attribute *attr,
>>> +                     char *buf)
>>> +{
>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->hyst);
>>> +}
>>> +
>>> +
>>> +static ssize_t set_hyst(struct device *dev, struct device_attribute *attr,
>>> +                    const char *buf, size_t count)
>>> +{
>>> +     int ret;
>>> +     long temp;
>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +     if (kstrtol(buf, 10, &temp) < 0)
>>> +             return -EINVAL;
>>> +
>> Per ABI, this is supposed to be an absolute temperature. For the chip,
>> it is a delta. You'll have to do some calculation to convert the absolute
>> value into a delta. Same for the show command.
>
> OK
>
>>
>>> +     ret = stts751_set_temp_reg(priv, temp, false, STTS751_REG_HYST, 0);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     dev_dbg(dev, "setting hyst %ld", temp);
>>> +
>>> +     priv->hyst = temp;
>>> +     return count;
>>> +}
>>> +
>>> +static ssize_t show_max(struct device *dev, struct device_attribute *attr,
>>> +                     char *buf)
>>> +{
>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_max);
>>> +}
>>> +
>>> +static ssize_t set_max(struct device *dev, struct device_attribute *attr,
>>> +                    const char *buf, size_t count)
>>> +{
>>> +     int ret;
>>> +     long temp;
>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +     if (kstrtol(buf, 10, &temp) < 0)
>>> +             return -EINVAL;
>>> +
>>> +     ret = stts751_set_temp_reg(priv, temp, true,
>>> +                             STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     dev_dbg(dev, "setting event max %ld", temp);
>>> +     priv->event_max = temp;
>>> +     return count;
>>> +}
>>> +
>>> +static ssize_t show_min(struct device *dev, struct device_attribute *attr,
>>> +                     char *buf)
>>> +{
>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_min);
>>> +}
>>> +
>>> +
>>
>> Single empty line, please.
>
> OK
>
>>
>>> +static ssize_t set_min(struct device *dev, struct device_attribute *attr,
>>> +                    const char *buf, size_t count)
>>> +{
>>> +     int ret;
>>> +     long temp;
>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +     if (kstrtol(buf, 10, &temp) < 0)
>>> +             return -EINVAL;
>>> +
>>> +     ret = stts751_set_temp_reg(priv, temp, true,
>>> +                             STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     dev_dbg(dev, "setting event min %ld", temp);
>>> +
>>> +     priv->event_min = temp;
>>> +     return count;
>>> +}
>>> +
>>> +static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
>>> +                          char *buf)
>>> +{
>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n",
>>> +                     stts751_intervals[priv->interval]);
>>> +}
>>> +
>>> +static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
>>> +                         const char *buf, size_t count)
>>> +{
>>> +     unsigned long val;
>>> +     int idx;
>>> +     int ret = 0;
>>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>>> +
>>> +     if (kstrtoul(buf, 10, &val) < 0)
>>> +             return -EINVAL;
>>> +
>>> +     idx = find_closest_descending(val, stts751_intervals,
>>> +                             ARRAY_SIZE(stts751_intervals));
>>> +
>>> +     dev_dbg(dev, "setting interval. req:%lu, idx: %d, val: %d", val, idx,
>>> +             stts751_intervals[idx]);
>>> +
>>> +     if (priv->interval == idx)
>>> +             return count;
>>> +
>>> +     mutex_lock(&priv->access_lock);
>>> +
>>> +     /* speed up, lower the resolution, then modify convrate */
>>> +     if (priv->interval < idx) {
>>> +             priv->interval = idx;
>>> +             ret = stts751_adjust_resolution(priv);
>>> +             if (ret)
>>> +                     goto exit;
>>> +     }
>>> +
>>> +     ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_RATE, idx);
>>> +     if (ret)
>>> +             goto exit;
>>> +
>>> +     /* slow down, modify convrate, then raise resolution */
>>> +     if (priv->interval != idx) {
>>> +             priv->interval = idx;
>>> +             ret = stts751_adjust_resolution(priv);
>>> +             if (ret)
>>> +                     goto exit;
>>> +
>> Extra empty line.
>>
>> Is it really necessary to have this code twice ? What is the harm in
>> just doing it once, either before or after setting the conversion rate ?
>
> In early development stages I've become suspicious about the chip
> starting to misbehave if I ever set, even briefly, an invalid
> configuration.
>
> I tried to avoid that by setting rate and resolution in such an order
> that never ends up in an invalid configuration. The order is thus not
> fixed and depends by what changes are you doing..
>
> (I'm not completely sure: I've hit something strange and I thought it
> could be because of this, and once I changed my code this way I never
> hit that again)..
>
Please add a respective comment into the code describing the reasons.

>>> +     }
>>> +exit:
>>> +     mutex_unlock(&priv->access_lock);
>>> +
>>> +     return count;
>>> +}
>>> +
>>> +static int stts751_detect(struct i2c_client *new_client,
>>> +                       struct i2c_board_info *info)
>>> +{
>>> +     struct i2c_adapter *adapter = new_client->adapter;
>>> +     const char *name;
>>> +     int mfg_id, prod_id, rev_id;
>>> +
>>> +     if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
>>> +             return -ENODEV;
>>> +
>>> +     mfg_id = i2c_smbus_read_byte_data(new_client, ST_MAN_ID);
>>> +     if (mfg_id != ST_MAN_ID)
>>> +             return -ENODEV;
>>> +
>>> +     prod_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_PROD_ID);
>>> +
>>> +     switch (prod_id) {
>>> +     case STTS751_0_PROD_ID:
>>> +             name = "STTS751-0";
>>> +             break;
>>> +     case STTS751_1_PROD_ID:
>>> +             name = "STTS751-1";
>>> +             break;
>>> +     default:
>>> +             return -ENODEV;
>>> +     }
>>> +     dev_info(&new_client->dev, "Chip %s detected!", name);
>>> +
>> Is this noise necessary ?
>
> OK, changed in dev_dbg.
>
>>
>>> +     rev_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_REV_ID);
>>> +
>>> +     if (rev_id != 0x1) {
>>
>> You might want to check for an error here (and return -ENODEV if you get one).
>
> OK
>
>>> +             dev_notice(&new_client->dev,
>>> +                     "Chip revision 0x%x is untested\nPlease report whether it works to andrea.merello@gmail.com",
>>> +                     rev_id);
>>> +     }
>>> +
>>> +     strlcpy(info->type, name, I2C_NAME_SIZE);
>>
>> This doesn't match the driver name (or, rather, the values in
>> stts751_id). Does this instantiate the driver ? That would surprise
>> me.
>
> It does, but I'll double check what the heck I've done here :)
>

Either case this should return a string matching stts751_id[], otherwise auto-detection
and manual or devicetree based instantiation would return different names.

Does your use case even include the detect function ?

>>> +     return 0;
>>> +}
>>> +
>>> +static int stts751_init_chip(struct stts751_priv *priv)
>>> +{
>>> +     int ret;
>>> +
>>> +     priv->config = STTS751_CONF_EVENT_DIS | STTS751_CONF_STOP;
>>> +     ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_CONF,
>>> +                                     priv->config);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_RATE,
>>> +                                     priv->interval);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     /* invalid, to force update */
>>> +     priv->res = -1;
>>> +     ret = stts751_adjust_resolution(priv);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = i2c_smbus_write_byte_data(priv->client,
>>> +                                     STTS751_REG_SMBUS_TO,
>>> +                                     priv->smbus_timeout ? 0x80 : 0);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +
>>> +     if (priv->gen_event) {
>>> +             ret = stts751_set_temp_reg(priv, priv->event_max, true,
>>> +                                     STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
>>> +             if (ret)
>>> +                     return ret;
>>> +
>>> +             ret = stts751_set_temp_reg(priv, priv->event_min, true,
>>> +                                     STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);
>>
>> Those registers all have power-up defaults. Unless there is a good reason,
>> we should not overwrite them. In some applications, the values may be
>> initialized from the BIOS/ROMMON.
>
> OK. I've changed it for all those regs but the smsbus timeout stuff:
> I think that this depends by the kind of I2C controller the device is
> attached to, so I'd still getting that from DT. Even if someone
> already initialized it (assuming it did it correctly, and the DT is
> correct also) I'll eventually end up in overwriting it with the same
> value.
>
> Do you agree with this ?
>
Ok to overwrite it if you have devicetree data, but otherwise you should
not touch it.

>>> +             if (ret)
>>> +                     return ret;
>>> +             priv->config &= ~STTS751_CONF_EVENT_DIS;
>>> +     }
>>> +
>>> +     if (priv->gen_therm) {
>>> +             ret = stts751_set_temp_reg(priv, priv->therm, false,
>>> +                                     STTS751_REG_TLIM, 0);
>>> +             if (ret)
>>> +                     return ret;
>>> +
>>> +             ret = stts751_set_temp_reg(priv, priv->hyst, false,
>>> +                                     STTS751_REG_HYST, 0);
>>> +             if (ret)
>>> +                     return ret;
>>> +     }
>>> +
>>> +     priv->config &= ~STTS751_CONF_STOP;
>>> +     ret = i2c_smbus_write_byte_data(priv->client,
>>> +                                     STTS751_REG_CONF, priv->config);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
>>> +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO,
>>> +                     show_min, set_min, 0);
>>> +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO,
>>> +                     show_max, set_max, 0);
>>> +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO,
>>> +                     show_min_alert, set_min_alert, 0);
>>> +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO,
>>> +                     show_max_alert, set_max_alert, 0);
>>> +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,
>>> +                     set_therm, 0);
>>> +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO, show_hyst,
>>> +                     set_hyst, 0);
>>
>> Please use standard attribute names.
>
> I can convert the event stuff in temp1_[min/max]_alarm for limits and
> temp1_alarm for the flag, but it's not obvious to be what should I use
> for the therm stuff.. May you please give me advices about this?
>
> I'd propose temp1_therm_[max/min]_hyst, but they seems not standard anyway...
>

 From the chip datasheet, this is supposed to reflect overtemperature conditions,
stronger than high/low, thus 'crit' seems appropriate.

>>
>>> +static SENSOR_DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO,
>>> +                     show_interval, set_interval, 0);
>>> +
>>> +/* always present */
>>> +static struct attribute *stts751_temp_attrs[] = {
>>> +     &sensor_dev_attr_temp1_input.dev_attr.attr,
>>> +     NULL
>>> +};
>>> +
>>> +static struct attribute_group stts751_temp_group = {
>>> +     .attrs = stts751_temp_attrs,
>>> +};
>>> +
>>> +/* present when therm pin or event pin are connected */
>>> +static struct attribute *stts751_interval_attrs[] = {
>>> +     &sensor_dev_attr_update_interval.dev_attr.attr,
>>> +     NULL
>>> +};
>>> +
>>> +static struct attribute_group stts751_interval_group = {
>>> +     .attrs = stts751_interval_attrs,
>>> +};
>>> +
>>> +/* present when event pin is connected */
>>> +static struct attribute *stts751_event_attrs[] = {
>>> +     &sensor_dev_attr_temp1_event_min.dev_attr.attr,
>>> +     &sensor_dev_attr_temp1_event_max.dev_attr.attr,
>>> +     &sensor_dev_attr_temp1_event_min_alert.dev_attr.attr,
>>> +     &sensor_dev_attr_temp1_event_max_alert.dev_attr.attr,
>>> +     NULL
>>> +};
>>> +
>>> +static struct attribute_group stts751_event_group = {
>>> +     .attrs = stts751_event_attrs,
>>> +};
>>> +
>>> +/* present when therm pin is connected */
>>> +static struct attribute *stts751_therm_attrs[] = {
>>> +     &sensor_dev_attr_temp1_therm.dev_attr.attr,
>>> +     &sensor_dev_attr_temp1_therm_hyst.dev_attr.attr,
>>> +     NULL
>>> +};
>>> +
>>> +static struct attribute_group stts751_therm_group = {
>>> +     .attrs = stts751_therm_attrs,
>>> +};
>>> +
>>> +static int stts751_probe(struct i2c_client *client,
>>> +                      const struct i2c_device_id *id)
>>> +{
>>> +     struct stts751_priv *priv;
>>> +     int ret;
>>> +     int groups_idx = 0;
>>> +     struct device_node *np = client->dev.of_node;
>>> +
>>> +     priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>>> +     if (!priv)
>>> +             return -ENOMEM;
>>> +
>>> +     priv->client = client;
>>> +     i2c_set_clientdata(client, priv);
>>> +     mutex_init(&priv->access_lock);
>>> +
>>> +     /* default to 2 samples per second */
>>> +     priv->interval = 5;
>>> +     /* default to timeout enable, as per chip default */
>>> +     priv->smbus_timeout = true;
>>> +     priv->last_update = 0;
>>> +     priv->data_valid = false;
>>> +     priv->max_alert = false;
>>> +     priv->min_alert = false;
>>> +     priv->gen_therm = false;
>>> +     priv->gen_event = false;
>>
>> Unnecessary initializations.
>
> OK
>
>>
>>> +     priv->therm = STTS751_THERM_DEFAULT;
>>> +     priv->hyst = STTS751_HYST_DEFAULT;
>>> +     priv->event_max = STTS751_EVENT_MAX_DEFAULT;
>>> +     priv->event_min = STTS751_EVENT_MIN_DEFAULT;
>>> +
>> I think it would be better to read the initial values from the chip
>> (also see above).
>>
>
> OK
>
>>> +     if (np) {
>>> +             priv->gen_therm = of_property_read_bool(np, "has-therm");
>>> +             priv->gen_event = of_property_read_bool(np, "has-event");
>>> +             priv->smbus_timeout = !of_property_read_bool(np,
>>> +                                             "smbus-timeout-disable");
>>> +     } else {
>>> +             dev_notice(&client->dev, "No DT data. Event/therm disabled\n");
>>
>> Seems to be quite rude to penaltize non-DT systems this way.
>
> That's gone since the two props "has-therm" and "has-event" are now
> gone. Any suggestion please about how to get the
> "smbus-timeout-disable" stuff in non-DT case ?
>
Platform data, or better use device properties for both cases.

>>> +     }
>>> +
>>> +     dev_dbg(&client->dev, "gen_event: %s, gen_therm: %s",
>>> +             priv->gen_event ? "YES" : "NO",
>>> +             priv->gen_therm ? "YES" : "NO");
>>> +
>>> +     priv->groups[groups_idx++] = &stts751_temp_group;
>>> +     priv->groups[groups_idx++] = &stts751_interval_group;
>>
>> Any reason for having multiple groups here ?
>
> Not anymore, as per your suggesion below, they are gone :)
>
>>> +
>>> +     if (priv->gen_therm)
>>> +             priv->groups[groups_idx++] = &stts751_therm_group;
>>> +
>>> +     if (priv->gen_event)
>>> +             priv->groups[groups_idx++] = &stts751_event_group;
>>> +
>> Why are those groups optional ? This is quite unusual. Even if the chip is not
>> connected to alert or interrupt lines, it still supports limit registers and
>> reports alerts. On top of that, at least interrupt support can be detected
>> automatically (from client->irq) and would not need a devicetree property.
>
> Props "has-therm" and "has-event" gone; please see above about IRQ stuf..
>
>> I think you really only need a single group, and all attributes should always
>> be available.
>
> OK
>
>>
>>> +     priv->groups[groups_idx] = NULL;
>>
>> Unnecessary initialization. devm_kzalloc() initializes priv with 0.
>
> OK
>
>>> +
>>> +     ret = stts751_init_chip(priv);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     priv->dev = devm_hwmon_device_register_with_groups(&client->dev,
>>> +                                                     client->name, priv,
>>> +                                                     priv->groups);
>>> +     return PTR_ERR_OR_ZERO(priv->dev);
>>> +}
>>> +
>>> +static const struct i2c_device_id stts751_id[] = {
>>> +     { "stts751", 0 },
>>> +     { }
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, stts751_id);
>>> +
>>> +static struct i2c_driver stts751_driver = {
>>> +     .class          = I2C_CLASS_HWMON,
>>> +     .driver = {
>>> +             .name   = DEVNAME,
>>> +     },
>>> +     .probe          = stts751_probe,
>>> +     .id_table       = stts751_id,
>>> +     .detect         = stts751_detect,
>>> +     .alert          = stts751_alert,
>>> +     .address_list   = normal_i2c,
>>> +};
>>> +
>>> +module_i2c_driver(stts751_driver);
>>> +
>>> +MODULE_AUTHOR("Andrea Merello <andrea.merello@gmail.com>");
>>> +MODULE_DESCRIPTION("STTS751 sensor driver");
>>> +MODULE_LICENSE("GPL");
>>> --
>>> 2.7.4
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2 2/2] hwmon: new driver for ST stts751 thermal sensor

[Andrea Merello]

On Wed, Oct 5, 2016 at 1:51 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Tue, Sep 27, 2016 at 02:07:27PM +0200, Andrea Merello wrote:
>> This patch adds a HWMON driver for ST Microelectronics STTS751
>> temperature sensors.
>>
>> It does support temperature reading (of course), SMBUS alert
>> functionality and "therm" output.
>>
>> Thanks-to: LABBE Corentin [for suggestions]
>> Thanks-to: Guenter Roeck [for suggestions/discussions]
>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>> Cc: LABBE Corentin <clabbe.montjoie@gmail.com>
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> Cc: Jean Delvare <jdelvare@suse.com>
>> ---
>>  drivers/hwmon/Kconfig   |  12 +-
>>  drivers/hwmon/Makefile  |   2 +-
>>  drivers/hwmon/stts751.c | 805 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 817 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/hwmon/stts751.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index eaf2f91..4e70b42 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1448,6 +1448,16 @@ config SENSORS_SCH5636
>>         This driver can also be built as a module.  If so, the module
>>         will be called sch5636.
>>
>> +config SENSORS_STTS751
>> +     tristate "ST Microelectronics STTS751"
>> +     depends on I2C
>> +     help
>> +       If you say yes here you get support for STTS751
>> +       temperature sensor chips.
>> +
>> +       This driver can also be built as a module. If so, the module
>> +       will be called stts751.
>> +
>>  config SENSORS_SMM665
>>       tristate "Summit Microelectronics SMM665"
>>       depends on I2C
>> @@ -1506,7 +1516,7 @@ config SENSORS_ADS7871
>>
>>  config SENSORS_AMC6821
>>       tristate "Texas Instruments AMC6821"
>> -     depends on I2C
>> +     depends on I2C
>
> ???

My editor is kind enough to automatically kill trailing spaces.. I'll
take care of moving this one in another patch in next series version..

>
>>       help
>>         If you say yes here you get support for the Texas Instruments
>>         AMC6821 hardware monitoring chips.
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index fe87d28..1114130 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -147,6 +147,7 @@ obj-$(CONFIG_SENSORS_SMM665)      += smm665.o
>>  obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
>>  obj-$(CONFIG_SENSORS_SMSC47M1)       += smsc47m1.o
>>  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>> +obj-$(CONFIG_SENSORS_STTS751)        += stts751.o
>>  obj-$(CONFIG_SENSORS_AMC6821)        += amc6821.o
>>  obj-$(CONFIG_SENSORS_TC74)   += tc74.o
>>  obj-$(CONFIG_SENSORS_THMC50) += thmc50.o
>> @@ -169,4 +170,3 @@ obj-$(CONFIG_SENSORS_WM8350)      += wm8350-hwmon.o
>>  obj-$(CONFIG_PMBUS)          += pmbus/
>>
>>  ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
>> -
>
> Looks like an unnecessary whitespace change.

Like above..

>
>> diff --git a/drivers/hwmon/stts751.c b/drivers/hwmon/stts751.c
>> new file mode 100644
>> index 0000000..8358e04
>> --- /dev/null
>> +++ b/drivers/hwmon/stts751.c
>> @@ -0,0 +1,805 @@
>> +/*
>> + * STTS751 sensor driver
>> + *
>> + * Copyright (C) 2016 Istituto Italiano di Tecnologia - RBCS - EDL
>> + * Robotics, Brain and Cognitive Sciences department
>> + * Electronic Design Laboratory
>> + *
>> + * Written by Andrea Merello <andrea.merello@gmail.com>
>> + *
>> + * Based on  LM95241 driver and LM90 driver
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +
> No douple empty lines please.

OK. BTW do you use some enhanced "checkpatch" script in order to catch
issues like these?

>
>> +#include <linux/bitops.h>
>> +#include <linux/err.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/i2c.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/slab.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/util_macros.h>
>> +
>> +#define DEVNAME "stts751"
>> +
>> +static const unsigned short normal_i2c[] = {
>> +     0x48, 0x49, 0x38, 0x39,  /* STTS751-0 */
>> +     0x4A, 0x4B, 0x3A, 0x3B,  /* STTS751-1 */
>> +     I2C_CLIENT_END };
>> +
>> +#define STTS751_REG_TEMP_H   0x00
>> +#define STTS751_REG_STATUS   0x01
>> +#define STTS751_STATUS_TRIPL BIT(5)
>> +#define STTS751_STATUS_TRIPH BIT(6)
>> +#define STTS751_STATUS_BUSY  BIT(8)
>> +#define STTS751_REG_TEMP_L   0x02
>> +#define STTS751_REG_CONF     0x03
>> +#define STTS751_CONF_RES_MASK        0x0C
>> +#define STTS751_CONF_RES_SHIFT  2
>> +#define STTS751_CONF_EVENT_DIS  BIT(7)
>> +#define STTS751_CONF_STOP    BIT(6)
>> +#define STTS751_REG_RATE     0x04
>> +#define STTS751_REG_HLIM_H   0x05
>> +#define STTS751_REG_HLIM_L   0x06
>> +#define STTS751_REG_LLIM_H   0x07
>> +#define STTS751_REG_LLIM_L   0x08
>> +#define STTS751_REG_ONESHOT  0x0F
>> +#define STTS751_REG_TLIM     0x20
>> +#define STTS751_REG_HYST     0x21
>> +#define STTS751_REG_SMBUS_TO 0x22
>> +
>> +#define STTS751_REG_PROD_ID  0xFD
>> +#define STTS751_REG_MAN_ID   0xFE
>> +#define STTS751_REG_REV_ID   0xFF
>> +
>> +#define STTS751_0_PROD_ID    0x00
>> +#define STTS751_1_PROD_ID    0x01
>> +#define ST_MAN_ID            0x53
>> +
>> +/* stick with HW defaults */
>> +#define STTS751_THERM_DEFAULT        85000
>> +#define STTS751_HYST_DEFAULT 10000
>> +#define STTS751_EVENT_MAX_DEFAULT 85000
>> +#define STTS751_EVENT_MIN_DEFAULT 0
>> +
>> +#define STTS751_RACE_RETRY   5
>> +#define STTS751_CONV_TIMEOUT 100 /* mS */
>> +#define STTS751_CACHE_TIME   100 /* mS */
>> +
>> +/*
>> + * Possible update intervals are (in mS):
>> + * 16000, 8000, 4000, 2000, 1000, 500, 250, 125, 62.5, 31.25
>> + * However we are not going to complicate things too much and we stick to the
>> + * approx value in mS.
>> + */
>> +static const int stts751_intervals[] = {
>> +     16000, 8000, 4000, 2000, 1000, 500, 250, 125, 63, 31
>> +};
>> +
>> +struct stts751_priv {
>> +     struct device *dev;
>> +     struct i2c_client *client;
>> +     struct mutex access_lock;
>> +     unsigned long interval;
>> +     int res;
>> +     bool gen_therm, gen_event;
>> +     int event_max, event_min;
>> +     int therm;
>> +     int hyst;
>> +     bool smbus_timeout;
>> +     int temp;
>> +     unsigned long last_update;
>> +     u8 config;
>> +     bool min_alert, max_alert;
>> +     bool data_valid;
>> +
>> +     /*
>> +      * Temperature/interval is always present.
>> +      * Depending by DT therm and event are dynamically added.
>> +      * There are max 4 entries plus the guard
>> +      */
>> +     const struct attribute_group *groups[5];
>> +};
>> +
>> +/*
>> + * These functions converts temperature from HW format to integer format and
>> + * vice-vers. They are (mostly) taken from lm90 driver. Unit is in mC.
>> + */
>> +static int stts751_to_deg(s32 hw_val)
>> +{
>> +     return hw_val * 125 / 32;
>> +}
>> +
>> +static s16 stts751_to_hw(int val, s32 *hw_val)
>> +{
>> +     /* HW works in range -64C to +127C */
>> +     if ((val > 127000) || (val < -64000))
>
> Extra ( ). However, clamp_val(val, -64000, 127000) is preferred here
> instead of returning an error (we don't want to force users to guess
> valid ranges).
>
> The upper limit should probably be 127937, though, per datasheet.


OK

>> +             return -EINVAL;
>> +
>> +     if (val < 0)
>> +             *hw_val = (val - 62) / 125 * 32;
>> +     else
>> +             *hw_val = (val + 62) / 125 * 32;
>> +
> By having this function return an int, you could combine the 16-bit return
> value with the error code, and the extra parameter would not be necessary
> (even more so since this function should not return an error in the first
> place).

OK, now it's never failing.
>
>> +     return 0;
>> +}
>> +
>> +static int stts751_adjust_resolution(struct stts751_priv *priv)
>> +{
>> +     u8 res;
>> +
>> +     switch (priv->interval) {
>> +     case 9:
>> +             /* 10 bits */
>> +             res = 0;
>> +             break;
>> +     case 8:
>> +             /* 11 bits */
>> +             res = 1;
>> +             break;
>> +     default:
>> +             /* 12 bits */
>> +             res = 3;
>> +             break;
>> +     }
>> +
>> +     if (priv->res == res)
>> +             return 0;
>> +
>> +     priv->config &= ~STTS751_CONF_RES_MASK;
>> +     priv->config |= res << STTS751_CONF_RES_SHIFT;
>> +
>> +     return i2c_smbus_write_byte_data(priv->client,
>> +                             STTS751_REG_CONF, priv->config);
>> +}
>> +
>> +static int stts751_update_temp(struct stts751_priv *priv)
>> +{
>> +     s32 integer1, integer2, frac;
>> +     int ret = 0;
>> +
>> +     mutex_lock(&priv->access_lock);
>> +
>> +     /*
>> +      * There is a trick here, like in the lm90 driver. We have to read two
>> +      * registers to get the sensor temperature, but we have to beware a
>> +      * conversion could occur between the readings. We could use the
>> +      * one-shot conversion register, but we don't want to do this (disables
>> +      * hardware monitoring). So the solution used here is to read the high
>> +      * byte once, then the low byte, then the high byte again. If the new
>> +      * high byte matches the old one, then we have a valid reading. Else we
>> +      * have to read the low byte again, and now we believe we have a correct
>> +      * reading.
>> +      */
>> +     integer1 = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_H);
>> +     if (integer1 < 0) {
>> +             dev_dbg(&priv->client->dev,
>> +                     "I2C read failed (temp H). ret: %x\n", ret);
>> +             ret = integer1;
>> +             goto exit;
>> +     }
>> +
>> +     frac = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_L);
>> +     if (frac < 0) {
>> +             dev_dbg(&priv->client->dev,
>> +                     "I2C read failed (temp L). ret: %x\n", ret);
>> +             ret = frac;
>> +             goto exit;
>> +     }
>> +
>> +     integer2 = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_H);
>> +     if (integer2 < 0) {
>> +             dev_dbg(&priv->client->dev,
>> +                     "I2C 2nd read failed (temp H). ret: %x\n", ret);
>> +             ret = integer2;
>> +             goto exit;
>> +     }
>> +
>> +     if (integer1 != integer2) {
>> +             frac = i2c_smbus_read_byte_data(priv->client,
>> +                                             STTS751_REG_TEMP_L);
>> +             if (frac < 0) {
>> +                     dev_dbg(&priv->client->dev,
>> +                             "I2C 2nd read failed (temp L). ret: %x\n", ret);
>> +                     ret = frac;
>> +                     goto exit;
>> +             }
>> +     }
>> +
>> +exit:
>> +     mutex_unlock(&priv->access_lock);
>> +     if (ret)
>> +             return ret;
>> +
>> +     priv->temp = stts751_to_deg((integer1 << 8) | frac);
>> +     return ret;
>> +}
>> +
>> +static int stts751_set_temp_reg(struct stts751_priv *priv, int temp,
>> +                             bool is_frac, u8 hreg, u8 lreg)
>> +{
>> +     s32 hwval;
>> +     int ret;
>> +
>> +     if (stts751_to_hw(temp, &hwval))
>> +             return -EINVAL;
>> +
>> +     mutex_lock(&priv->access_lock);
>> +     ret = i2c_smbus_write_byte_data(priv->client, hreg, hwval >> 8);
>> +     if (ret)
>> +             goto exit;
>> +     if (is_frac)
>> +             ret = i2c_smbus_write_byte_data(priv->client,
>> +                                             lreg, hwval & 0xff);
>> +exit:
>> +     mutex_unlock(&priv->access_lock);
>> +
>> +     return ret;
>> +}
>> +
>> +static int stts751_update_alert(struct stts751_priv *priv)
>> +{
>> +     int ret;
>> +
>> +     /* not for us.. */
>> +     if (!priv->gen_event)
>> +             return 0;
>> +
>> +     ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_STATUS);
>> +
> Please no empty line between functions and checking of return values.

OK

>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     priv->max_alert = priv->max_alert || !!(ret & STTS751_STATUS_TRIPH);
>> +     priv->min_alert = priv->min_alert || !!(ret & STTS751_STATUS_TRIPL);
>> +
> I am not really happy about the caching of alert values. This isn't really
> common. Usually we report current alert values when reading. I don't really
> see why this driver should be different to all the others.

OK

>> +     return 0;
>> +}
>> +
>> +static void stts751_alert(struct i2c_client *client,
>> +                     enum i2c_alert_protocol type, unsigned int data)
>> +{
>> +     int ret;
>> +     struct stts751_priv *priv = i2c_get_clientdata(client);
>> +     bool prev_max = priv->max_alert;
>> +     bool prev_min = priv->min_alert;
>> +
>> +     if (type != I2C_PROTOCOL_SMBUS_ALERT)
>> +             return;
>> +
>> +     dev_dbg(&client->dev, "alert!");
>> +
>
> Any chance to make this a bit more generic and also support
> interrupts ?

May you please elaborate?

IMHO adding the logic for handling SMBus alerts using IRQs to any
HWmon driver is redundant, because it is already implemented in the
SMBus layer, just right now you need patching SMBus layer for enabling
it from the DT world (a couple of patches for this are floating
around, but they seems not ready for merge yet)... Using this chip
with a generic interrupt is indeed exactly what I'm doing..

.. Or am I completely missing your point ?

>
>> +     mutex_lock(&priv->access_lock);
>> +     ret = stts751_update_alert(priv);
>> +     if (ret < 0) {
>> +             /* default to worst case */
>> +             priv->max_alert = true;
>> +             priv->min_alert = true;
>> +
>> +             if (!(prev_max && prev_min)) {
>> +                     dev_warn(&priv->client->dev,
>> +                             "Alert received, but can't communicate to the device. Something bad happening? Triggering all alarms!");
>> +             }
>> +     }
>> +
>> +     if (!prev_max && priv->max_alert) {
>> +             dev_notice(&client->dev, "got alert for HIGH temperature");
>> +
>> +             /* unblock alert poll */
>> +             sysfs_notify(&priv->dev->kobj, NULL, "temp1_event_max_alert");
>> +             kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
>> +     }
>> +
>> +     if (!prev_min && priv->min_alert) {
>> +             dev_notice(&client->dev, "got alert for LOW temperature");
>> +
>> +             /* unblock alert poll */
>> +             sysfs_notify(&priv->dev->kobj, NULL, "temp1_event_min_alert");
>> +             kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
>> +     }
>> +     mutex_unlock(&priv->access_lock);
>> +}
>> +
>> +static ssize_t show_max_alert(struct device *dev, struct device_attribute *attr,
>> +                       char *buf)
>> +{
>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->max_alert);
>> +}
>> +
>> +static ssize_t set_max_alert(struct device *dev, struct device_attribute *attr,
>> +                    const char *buf, size_t count)
>> +{
>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> +     mutex_lock(&priv->access_lock);
>> +     priv->max_alert = false;
>> +     mutex_unlock(&priv->access_lock);
>> +
>> +     return count;
>> +}
>> +
>> +static ssize_t show_min_alert(struct device *dev, struct device_attribute *attr,
>> +                       char *buf)
>> +{
>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->min_alert);
>> +}
>> +
>> +static ssize_t set_min_alert(struct device *dev, struct device_attribute *attr,
>> +                    const char *buf, size_t count)
>> +{
>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> +     mutex_lock(&priv->access_lock);
>> +     priv->min_alert = false;
>> +     mutex_unlock(&priv->access_lock);
>> +
> This is highly unusual. If the alert needs to be cleared, please auto-clear it
> when reading.

OK

>> +     return count;
>> +}
>> +
>> +static ssize_t show_input(struct device *dev, struct device_attribute *attr,
>> +                       char *buf)
>> +{
>> +     int ret;
>> +     int cache_time = STTS751_CACHE_TIME * HZ / 1000;
>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> +     /*
>> +      * Adjust the cache time wrt the sample rate. We do 4X in order to get
>> +      * a new measure in no more than 1/4 of the sample time (that seemed
>> +      * reasonable to me).
>> +      */
>> +     cache_time = stts751_intervals[priv->interval] / 4 * HZ / 1000;
>> +
>> +     if (time_after(jiffies, priv->last_update + cache_time) ||
>> +             !priv->data_valid) {
>> +             ret = stts751_update_temp(priv);
>> +             if (ret)
>> +                     return ret;
>> +             priv->last_update = jiffies;
>> +             priv->data_valid = true;
>> +     }
>> +
>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->temp);
>> +}
>> +
>> +static ssize_t show_therm(struct device *dev, struct device_attribute *attr,
>> +                     char *buf)
>> +{
>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->therm);
>> +}
>> +
>> +
>> +static ssize_t set_therm(struct device *dev, struct device_attribute *attr,
>> +                    const char *buf, size_t count)
>> +{
>> +     int ret;
>> +     long temp;
>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> +     if (kstrtol(buf, 10, &temp) < 0)
>> +             return -EINVAL;
>> +
>> +     ret = stts751_set_temp_reg(priv, temp, false, STTS751_REG_TLIM, 0);
>> +     if (ret)
>> +             return ret;
>> +
> Does it really add value to use the same function for 8- and 16-bit registers ?
> Seems to me it just makes the code more complicated.

I haven't a strong opinion about this.. it's this complication vs a
slight code duplication.. I've changed this in the way you suggest :)

>> +     dev_dbg(dev, "setting therm %ld", temp);
>> +
>> +     priv->therm = temp;
>> +     return count;
>> +}
>> +
>> +static ssize_t show_hyst(struct device *dev, struct device_attribute *attr,
>> +                     char *buf)
>> +{
>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->hyst);
>> +}
>> +
>> +
>> +static ssize_t set_hyst(struct device *dev, struct device_attribute *attr,
>> +                    const char *buf, size_t count)
>> +{
>> +     int ret;
>> +     long temp;
>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> +     if (kstrtol(buf, 10, &temp) < 0)
>> +             return -EINVAL;
>> +
> Per ABI, this is supposed to be an absolute temperature. For the chip,
> it is a delta. You'll have to do some calculation to convert the absolute
> value into a delta. Same for the show command.

OK

>
>> +     ret = stts751_set_temp_reg(priv, temp, false, STTS751_REG_HYST, 0);
>> +     if (ret)
>> +             return ret;
>> +
>> +     dev_dbg(dev, "setting hyst %ld", temp);
>> +
>> +     priv->hyst = temp;
>> +     return count;
>> +}
>> +
>> +static ssize_t show_max(struct device *dev, struct device_attribute *attr,
>> +                     char *buf)
>> +{
>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_max);
>> +}
>> +
>> +static ssize_t set_max(struct device *dev, struct device_attribute *attr,
>> +                    const char *buf, size_t count)
>> +{
>> +     int ret;
>> +     long temp;
>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> +     if (kstrtol(buf, 10, &temp) < 0)
>> +             return -EINVAL;
>> +
>> +     ret = stts751_set_temp_reg(priv, temp, true,
>> +                             STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
>> +     if (ret)
>> +             return ret;
>> +
>> +     dev_dbg(dev, "setting event max %ld", temp);
>> +     priv->event_max = temp;
>> +     return count;
>> +}
>> +
>> +static ssize_t show_min(struct device *dev, struct device_attribute *attr,
>> +                     char *buf)
>> +{
>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_min);
>> +}
>> +
>> +
>
> Single empty line, please.

OK

>
>> +static ssize_t set_min(struct device *dev, struct device_attribute *attr,
>> +                    const char *buf, size_t count)
>> +{
>> +     int ret;
>> +     long temp;
>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> +     if (kstrtol(buf, 10, &temp) < 0)
>> +             return -EINVAL;
>> +
>> +     ret = stts751_set_temp_reg(priv, temp, true,
>> +                             STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);
>> +     if (ret)
>> +             return ret;
>> +
>> +     dev_dbg(dev, "setting event min %ld", temp);
>> +
>> +     priv->event_min = temp;
>> +     return count;
>> +}
>> +
>> +static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
>> +                          char *buf)
>> +{
>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> +     return snprintf(buf, PAGE_SIZE - 1, "%d\n",
>> +                     stts751_intervals[priv->interval]);
>> +}
>> +
>> +static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
>> +                         const char *buf, size_t count)
>> +{
>> +     unsigned long val;
>> +     int idx;
>> +     int ret = 0;
>> +     struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> +     if (kstrtoul(buf, 10, &val) < 0)
>> +             return -EINVAL;
>> +
>> +     idx = find_closest_descending(val, stts751_intervals,
>> +                             ARRAY_SIZE(stts751_intervals));
>> +
>> +     dev_dbg(dev, "setting interval. req:%lu, idx: %d, val: %d", val, idx,
>> +             stts751_intervals[idx]);
>> +
>> +     if (priv->interval == idx)
>> +             return count;
>> +
>> +     mutex_lock(&priv->access_lock);
>> +
>> +     /* speed up, lower the resolution, then modify convrate */
>> +     if (priv->interval < idx) {
>> +             priv->interval = idx;
>> +             ret = stts751_adjust_resolution(priv);
>> +             if (ret)
>> +                     goto exit;
>> +     }
>> +
>> +     ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_RATE, idx);
>> +     if (ret)
>> +             goto exit;
>> +
>> +     /* slow down, modify convrate, then raise resolution */
>> +     if (priv->interval != idx) {
>> +             priv->interval = idx;
>> +             ret = stts751_adjust_resolution(priv);
>> +             if (ret)
>> +                     goto exit;
>> +
> Extra empty line.
>
> Is it really necessary to have this code twice ? What is the harm in
> just doing it once, either before or after setting the conversion rate ?

In early development stages I've become suspicious about the chip
starting to misbehave if I ever set, even briefly, an invalid
configuration.

I tried to avoid that by setting rate and resolution in such an order
that never ends up in an invalid configuration. The order is thus not
fixed and depends by what changes are you doing..

(I'm not completely sure: I've hit something strange and I thought it
could be because of this, and once I changed my code this way I never
hit that again)..

>> +     }
>> +exit:
>> +     mutex_unlock(&priv->access_lock);
>> +
>> +     return count;
>> +}
>> +
>> +static int stts751_detect(struct i2c_client *new_client,
>> +                       struct i2c_board_info *info)
>> +{
>> +     struct i2c_adapter *adapter = new_client->adapter;
>> +     const char *name;
>> +     int mfg_id, prod_id, rev_id;
>> +
>> +     if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
>> +             return -ENODEV;
>> +
>> +     mfg_id = i2c_smbus_read_byte_data(new_client, ST_MAN_ID);
>> +     if (mfg_id != ST_MAN_ID)
>> +             return -ENODEV;
>> +
>> +     prod_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_PROD_ID);
>> +
>> +     switch (prod_id) {
>> +     case STTS751_0_PROD_ID:
>> +             name = "STTS751-0";
>> +             break;
>> +     case STTS751_1_PROD_ID:
>> +             name = "STTS751-1";
>> +             break;
>> +     default:
>> +             return -ENODEV;
>> +     }
>> +     dev_info(&new_client->dev, "Chip %s detected!", name);
>> +
> Is this noise necessary ?

OK, changed in dev_dbg.

>
>> +     rev_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_REV_ID);
>> +
>> +     if (rev_id != 0x1) {
>
> You might want to check for an error here (and return -ENODEV if you get one).

OK

>> +             dev_notice(&new_client->dev,
>> +                     "Chip revision 0x%x is untested\nPlease report whether it works to andrea.merello@gmail.com",
>> +                     rev_id);
>> +     }
>> +
>> +     strlcpy(info->type, name, I2C_NAME_SIZE);
>
> This doesn't match the driver name (or, rather, the values in
> stts751_id). Does this instantiate the driver ? That would surprise
> me.

It does, but I'll double check what the heck I've done here :)

>> +     return 0;
>> +}
>> +
>> +static int stts751_init_chip(struct stts751_priv *priv)
>> +{
>> +     int ret;
>> +
>> +     priv->config = STTS751_CONF_EVENT_DIS | STTS751_CONF_STOP;
>> +     ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_CONF,
>> +                                     priv->config);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_RATE,
>> +                                     priv->interval);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /* invalid, to force update */
>> +     priv->res = -1;
>> +     ret = stts751_adjust_resolution(priv);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = i2c_smbus_write_byte_data(priv->client,
>> +                                     STTS751_REG_SMBUS_TO,
>> +                                     priv->smbus_timeout ? 0x80 : 0);
>> +     if (ret)
>> +             return ret;
>> +
>> +
>> +     if (priv->gen_event) {
>> +             ret = stts751_set_temp_reg(priv, priv->event_max, true,
>> +                                     STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             ret = stts751_set_temp_reg(priv, priv->event_min, true,
>> +                                     STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);
>
> Those registers all have power-up defaults. Unless there is a good reason,
> we should not overwrite them. In some applications, the values may be
> initialized from the BIOS/ROMMON.

OK. I've changed it for all those regs but the smsbus timeout stuff:
I think that this depends by the kind of I2C controller the device is
attached to, so I'd still getting that from DT. Even if someone
already initialized it (assuming it did it correctly, and the DT is
correct also) I'll eventually end up in overwriting it with the same
value.

Do you agree with this ?

>> +             if (ret)
>> +                     return ret;
>> +             priv->config &= ~STTS751_CONF_EVENT_DIS;
>> +     }
>> +
>> +     if (priv->gen_therm) {
>> +             ret = stts751_set_temp_reg(priv, priv->therm, false,
>> +                                     STTS751_REG_TLIM, 0);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             ret = stts751_set_temp_reg(priv, priv->hyst, false,
>> +                                     STTS751_REG_HYST, 0);
>> +             if (ret)
>> +                     return ret;
>> +     }
>> +
>> +     priv->config &= ~STTS751_CONF_STOP;
>> +     ret = i2c_smbus_write_byte_data(priv->client,
>> +                                     STTS751_REG_CONF, priv->config);
>> +
>> +     return ret;
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO,
>> +                     show_min, set_min, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO,
>> +                     show_max, set_max, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO,
>> +                     show_min_alert, set_min_alert, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO,
>> +                     show_max_alert, set_max_alert, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,
>> +                     set_therm, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO, show_hyst,
>> +                     set_hyst, 0);
>
> Please use standard attribute names.

I can convert the event stuff in temp1_[min/max]_alarm for limits and
temp1_alarm for the flag, but it's not obvious to be what should I use
for the therm stuff.. May you please give me advices about this?

I'd propose temp1_therm_[max/min]_hyst, but they seems not standard anyway...

>
>> +static SENSOR_DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO,
>> +                     show_interval, set_interval, 0);
>> +
>> +/* always present */
>> +static struct attribute *stts751_temp_attrs[] = {
>> +     &sensor_dev_attr_temp1_input.dev_attr.attr,
>> +     NULL
>> +};
>> +
>> +static struct attribute_group stts751_temp_group = {
>> +     .attrs = stts751_temp_attrs,
>> +};
>> +
>> +/* present when therm pin or event pin are connected */
>> +static struct attribute *stts751_interval_attrs[] = {
>> +     &sensor_dev_attr_update_interval.dev_attr.attr,
>> +     NULL
>> +};
>> +
>> +static struct attribute_group stts751_interval_group = {
>> +     .attrs = stts751_interval_attrs,
>> +};
>> +
>> +/* present when event pin is connected */
>> +static struct attribute *stts751_event_attrs[] = {
>> +     &sensor_dev_attr_temp1_event_min.dev_attr.attr,
>> +     &sensor_dev_attr_temp1_event_max.dev_attr.attr,
>> +     &sensor_dev_attr_temp1_event_min_alert.dev_attr.attr,
>> +     &sensor_dev_attr_temp1_event_max_alert.dev_attr.attr,
>> +     NULL
>> +};
>> +
>> +static struct attribute_group stts751_event_group = {
>> +     .attrs = stts751_event_attrs,
>> +};
>> +
>> +/* present when therm pin is connected */
>> +static struct attribute *stts751_therm_attrs[] = {
>> +     &sensor_dev_attr_temp1_therm.dev_attr.attr,
>> +     &sensor_dev_attr_temp1_therm_hyst.dev_attr.attr,
>> +     NULL
>> +};
>> +
>> +static struct attribute_group stts751_therm_group = {
>> +     .attrs = stts751_therm_attrs,
>> +};
>> +
>> +static int stts751_probe(struct i2c_client *client,
>> +                      const struct i2c_device_id *id)
>> +{
>> +     struct stts751_priv *priv;
>> +     int ret;
>> +     int groups_idx = 0;
>> +     struct device_node *np = client->dev.of_node;
>> +
>> +     priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>> +     if (!priv)
>> +             return -ENOMEM;
>> +
>> +     priv->client = client;
>> +     i2c_set_clientdata(client, priv);
>> +     mutex_init(&priv->access_lock);
>> +
>> +     /* default to 2 samples per second */
>> +     priv->interval = 5;
>> +     /* default to timeout enable, as per chip default */
>> +     priv->smbus_timeout = true;
>> +     priv->last_update = 0;
>> +     priv->data_valid = false;
>> +     priv->max_alert = false;
>> +     priv->min_alert = false;
>> +     priv->gen_therm = false;
>> +     priv->gen_event = false;
>
> Unnecessary initializations.

OK

>
>> +     priv->therm = STTS751_THERM_DEFAULT;
>> +     priv->hyst = STTS751_HYST_DEFAULT;
>> +     priv->event_max = STTS751_EVENT_MAX_DEFAULT;
>> +     priv->event_min = STTS751_EVENT_MIN_DEFAULT;
>> +
> I think it would be better to read the initial values from the chip
> (also see above).
>

OK

>> +     if (np) {
>> +             priv->gen_therm = of_property_read_bool(np, "has-therm");
>> +             priv->gen_event = of_property_read_bool(np, "has-event");
>> +             priv->smbus_timeout = !of_property_read_bool(np,
>> +                                             "smbus-timeout-disable");
>> +     } else {
>> +             dev_notice(&client->dev, "No DT data. Event/therm disabled\n");
>
> Seems to be quite rude to penaltize non-DT systems this way.

That's gone since the two props "has-therm" and "has-event" are now
gone. Any suggestion please about how to get the
"smbus-timeout-disable" stuff in non-DT case ?

>> +     }
>> +
>> +     dev_dbg(&client->dev, "gen_event: %s, gen_therm: %s",
>> +             priv->gen_event ? "YES" : "NO",
>> +             priv->gen_therm ? "YES" : "NO");
>> +
>> +     priv->groups[groups_idx++] = &stts751_temp_group;
>> +     priv->groups[groups_idx++] = &stts751_interval_group;
>
> Any reason for having multiple groups here ?

Not anymore, as per your suggesion below, they are gone :)

>> +
>> +     if (priv->gen_therm)
>> +             priv->groups[groups_idx++] = &stts751_therm_group;
>> +
>> +     if (priv->gen_event)
>> +             priv->groups[groups_idx++] = &stts751_event_group;
>> +
> Why are those groups optional ? This is quite unusual. Even if the chip is not
> connected to alert or interrupt lines, it still supports limit registers and
> reports alerts. On top of that, at least interrupt support can be detected
> automatically (from client->irq) and would not need a devicetree property.

Props "has-therm" and "has-event" gone; please see above about IRQ stuf..

> I think you really only need a single group, and all attributes should always
> be available.

OK

>
>> +     priv->groups[groups_idx] = NULL;
>
> Unnecessary initialization. devm_kzalloc() initializes priv with 0.

OK

>> +
>> +     ret = stts751_init_chip(priv);
>> +     if (ret)
>> +             return ret;
>> +
>> +     priv->dev = devm_hwmon_device_register_with_groups(&client->dev,
>> +                                                     client->name, priv,
>> +                                                     priv->groups);
>> +     return PTR_ERR_OR_ZERO(priv->dev);
>> +}
>> +
>> +static const struct i2c_device_id stts751_id[] = {
>> +     { "stts751", 0 },
>> +     { }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, stts751_id);
>> +
>> +static struct i2c_driver stts751_driver = {
>> +     .class          = I2C_CLASS_HWMON,
>> +     .driver = {
>> +             .name   = DEVNAME,
>> +     },
>> +     .probe          = stts751_probe,
>> +     .id_table       = stts751_id,
>> +     .detect         = stts751_detect,
>> +     .alert          = stts751_alert,
>> +     .address_list   = normal_i2c,
>> +};
>> +
>> +module_i2c_driver(stts751_driver);
>> +
>> +MODULE_AUTHOR("Andrea Merello <andrea.merello@gmail.com>");
>> +MODULE_DESCRIPTION("STTS751 sensor driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.7.4
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] hwmon: new driver for ST stts751 thermal sensor

[Guenter Roeck]

On Tue, Sep 27, 2016 at 02:07:27PM +0200, Andrea Merello wrote:
> This patch adds a HWMON driver for ST Microelectronics STTS751
> temperature sensors.
> 
> It does support temperature reading (of course), SMBUS alert
> functionality and "therm" output.
> 
> Thanks-to: LABBE Corentin [for suggestions]
> Thanks-to: Guenter Roeck [for suggestions/discussions]
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> Cc: LABBE Corentin <clabbe.montjoie@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Jean Delvare <jdelvare@suse.com>
> ---
>  drivers/hwmon/Kconfig   |  12 +-
>  drivers/hwmon/Makefile  |   2 +-
>  drivers/hwmon/stts751.c | 805 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 817 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/hwmon/stts751.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index eaf2f91..4e70b42 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1448,6 +1448,16 @@ config SENSORS_SCH5636
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called sch5636.
>  
> +config SENSORS_STTS751
> +	tristate "ST Microelectronics STTS751"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for STTS751
> +	  temperature sensor chips.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called stts751.
> +
>  config SENSORS_SMM665
>  	tristate "Summit Microelectronics SMM665"
>  	depends on I2C
> @@ -1506,7 +1516,7 @@ config SENSORS_ADS7871
>  
>  config SENSORS_AMC6821
>  	tristate "Texas Instruments AMC6821"
> -	depends on I2C 
> +	depends on I2C

???

>  	help
>  	  If you say yes here you get support for the Texas Instruments
>  	  AMC6821 hardware monitoring chips.
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index fe87d28..1114130 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -147,6 +147,7 @@ obj-$(CONFIG_SENSORS_SMM665)	+= smm665.o
>  obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
>  obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
>  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
> +obj-$(CONFIG_SENSORS_STTS751)	+= stts751.o
>  obj-$(CONFIG_SENSORS_AMC6821)	+= amc6821.o
>  obj-$(CONFIG_SENSORS_TC74)	+= tc74.o
>  obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
> @@ -169,4 +170,3 @@ obj-$(CONFIG_SENSORS_WM8350)	+= wm8350-hwmon.o
>  obj-$(CONFIG_PMBUS)		+= pmbus/
>  
>  ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
> -

Looks like an unnecessary whitespace change.

> diff --git a/drivers/hwmon/stts751.c b/drivers/hwmon/stts751.c
> new file mode 100644
> index 0000000..8358e04
> --- /dev/null
> +++ b/drivers/hwmon/stts751.c
> @@ -0,0 +1,805 @@
> +/*
> + * STTS751 sensor driver
> + *
> + * Copyright (C) 2016 Istituto Italiano di Tecnologia - RBCS - EDL
> + * Robotics, Brain and Cognitive Sciences department
> + * Electronic Design Laboratory
> + *
> + * Written by Andrea Merello <andrea.merello@gmail.com>
> + *
> + * Based on  LM95241 driver and LM90 driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +
No douple empty lines please.

> +#include <linux/bitops.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_irq.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/util_macros.h>
> +
> +#define DEVNAME "stts751"
> +
> +static const unsigned short normal_i2c[] = {
> +	0x48, 0x49, 0x38, 0x39,  /* STTS751-0 */
> +	0x4A, 0x4B, 0x3A, 0x3B,  /* STTS751-1 */
> +	I2C_CLIENT_END };
> +
> +#define STTS751_REG_TEMP_H	0x00
> +#define STTS751_REG_STATUS	0x01
> +#define STTS751_STATUS_TRIPL	BIT(5)
> +#define STTS751_STATUS_TRIPH	BIT(6)
> +#define STTS751_STATUS_BUSY	BIT(8)
> +#define STTS751_REG_TEMP_L	0x02
> +#define STTS751_REG_CONF	0x03
> +#define STTS751_CONF_RES_MASK	0x0C
> +#define STTS751_CONF_RES_SHIFT  2
> +#define STTS751_CONF_EVENT_DIS  BIT(7)
> +#define STTS751_CONF_STOP	BIT(6)
> +#define STTS751_REG_RATE	0x04
> +#define STTS751_REG_HLIM_H	0x05
> +#define STTS751_REG_HLIM_L	0x06
> +#define STTS751_REG_LLIM_H	0x07
> +#define STTS751_REG_LLIM_L	0x08
> +#define STTS751_REG_ONESHOT	0x0F
> +#define STTS751_REG_TLIM	0x20
> +#define STTS751_REG_HYST	0x21
> +#define STTS751_REG_SMBUS_TO	0x22
> +
> +#define STTS751_REG_PROD_ID	0xFD
> +#define STTS751_REG_MAN_ID	0xFE
> +#define STTS751_REG_REV_ID	0xFF
> +
> +#define STTS751_0_PROD_ID	0x00
> +#define STTS751_1_PROD_ID	0x01
> +#define ST_MAN_ID		0x53
> +
> +/* stick with HW defaults */
> +#define STTS751_THERM_DEFAULT	85000
> +#define STTS751_HYST_DEFAULT	10000
> +#define STTS751_EVENT_MAX_DEFAULT 85000
> +#define STTS751_EVENT_MIN_DEFAULT 0
> +
> +#define STTS751_RACE_RETRY	5
> +#define STTS751_CONV_TIMEOUT	100 /* mS */
> +#define STTS751_CACHE_TIME	100 /* mS */
> +
> +/*
> + * Possible update intervals are (in mS):
> + * 16000, 8000, 4000, 2000, 1000, 500, 250, 125, 62.5, 31.25
> + * However we are not going to complicate things too much and we stick to the
> + * approx value in mS.
> + */
> +static const int stts751_intervals[] = {
> +	16000, 8000, 4000, 2000, 1000, 500, 250, 125, 63, 31
> +};
> +
> +struct stts751_priv {
> +	struct device *dev;
> +	struct i2c_client *client;
> +	struct mutex access_lock;
> +	unsigned long interval;
> +	int res;
> +	bool gen_therm, gen_event;
> +	int event_max, event_min;
> +	int therm;
> +	int hyst;
> +	bool smbus_timeout;
> +	int temp;
> +	unsigned long last_update;
> +	u8 config;
> +	bool min_alert, max_alert;
> +	bool data_valid;
> +
> +	/*
> +	 * Temperature/interval is always present.
> +	 * Depending by DT therm and event are dynamically added.
> +	 * There are max 4 entries plus the guard
> +	 */
> +	const struct attribute_group *groups[5];
> +};
> +
> +/*
> + * These functions converts temperature from HW format to integer format and
> + * vice-vers. They are (mostly) taken from lm90 driver. Unit is in mC.
> + */
> +static int stts751_to_deg(s32 hw_val)
> +{
> +	return hw_val * 125 / 32;
> +}
> +
> +static s16 stts751_to_hw(int val, s32 *hw_val)
> +{
> +	/* HW works in range -64C to +127C */
> +	if ((val > 127000) || (val < -64000))

Extra ( ). However, clamp_val(val, -64000, 127000) is preferred here
instead of returning an error (we don't want to force users to guess
valid ranges).

The upper limit should probably be 127937, though, per datasheet.

> +		return -EINVAL;
> +
> +	if (val < 0)
> +		*hw_val = (val - 62) / 125 * 32;
> +	else
> +		*hw_val = (val + 62) / 125 * 32;
> +
By having this function return an int, you could combine the 16-bit return
value with the error code, and the extra parameter would not be necessary
(even more so since this function should not return an error in the first
place).

> +	return 0;
> +}
> +
> +static int stts751_adjust_resolution(struct stts751_priv *priv)
> +{
> +	u8 res;
> +
> +	switch (priv->interval) {
> +	case 9:
> +		/* 10 bits */
> +		res = 0;
> +		break;
> +	case 8:
> +		/* 11 bits */
> +		res = 1;
> +		break;
> +	default:
> +		/* 12 bits */
> +		res = 3;
> +		break;
> +	}
> +
> +	if (priv->res == res)
> +		return 0;
> +
> +	priv->config &= ~STTS751_CONF_RES_MASK;
> +	priv->config |= res << STTS751_CONF_RES_SHIFT;
> +
> +	return i2c_smbus_write_byte_data(priv->client,
> +				STTS751_REG_CONF, priv->config);
> +}
> +
> +static int stts751_update_temp(struct stts751_priv *priv)
> +{
> +	s32 integer1, integer2, frac;
> +	int ret = 0;
> +
> +	mutex_lock(&priv->access_lock);
> +
> +	/*
> +	 * There is a trick here, like in the lm90 driver. We have to read two
> +	 * registers to get the sensor temperature, but we have to beware a
> +	 * conversion could occur between the readings. We could use the
> +	 * one-shot conversion register, but we don't want to do this (disables
> +	 * hardware monitoring). So the solution used here is to read the high
> +	 * byte once, then the low byte, then the high byte again. If the new
> +	 * high byte matches the old one, then we have a valid reading. Else we
> +	 * have to read the low byte again, and now we believe we have a correct
> +	 * reading.
> +	 */
> +	integer1 = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_H);
> +	if (integer1 < 0) {
> +		dev_dbg(&priv->client->dev,
> +			"I2C read failed (temp H). ret: %x\n", ret);
> +		ret = integer1;
> +		goto exit;
> +	}
> +
> +	frac = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_L);
> +	if (frac < 0) {
> +		dev_dbg(&priv->client->dev,
> +			"I2C read failed (temp L). ret: %x\n", ret);
> +		ret = frac;
> +		goto exit;
> +	}
> +
> +	integer2 = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_H);
> +	if (integer2 < 0) {
> +		dev_dbg(&priv->client->dev,
> +			"I2C 2nd read failed (temp H). ret: %x\n", ret);
> +		ret = integer2;
> +		goto exit;
> +	}
> +
> +	if (integer1 != integer2) {
> +		frac = i2c_smbus_read_byte_data(priv->client,
> +						STTS751_REG_TEMP_L);
> +		if (frac < 0) {
> +			dev_dbg(&priv->client->dev,
> +				"I2C 2nd read failed (temp L). ret: %x\n", ret);
> +			ret = frac;
> +			goto exit;
> +		}
> +	}
> +
> +exit:
> +	mutex_unlock(&priv->access_lock);
> +	if (ret)
> +		return ret;
> +
> +	priv->temp = stts751_to_deg((integer1 << 8) | frac);
> +	return ret;
> +}
> +
> +static int stts751_set_temp_reg(struct stts751_priv *priv, int temp,
> +				bool is_frac, u8 hreg, u8 lreg)
> +{
> +	s32 hwval;
> +	int ret;
> +
> +	if (stts751_to_hw(temp, &hwval))
> +		return -EINVAL;
> +
> +	mutex_lock(&priv->access_lock);
> +	ret = i2c_smbus_write_byte_data(priv->client, hreg, hwval >> 8);
> +	if (ret)
> +		goto exit;
> +	if (is_frac)
> +		ret = i2c_smbus_write_byte_data(priv->client,
> +						lreg, hwval & 0xff);
> +exit:
> +	mutex_unlock(&priv->access_lock);
> +
> +	return ret;
> +}
> +
> +static int stts751_update_alert(struct stts751_priv *priv)
> +{
> +	int ret;
> +
> +	/* not for us.. */
> +	if (!priv->gen_event)
> +		return 0;
> +
> +	ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_STATUS);
> +
Please no empty line between functions and checking of return values.

> +	if (ret < 0)
> +		return ret;
> +
> +	priv->max_alert = priv->max_alert || !!(ret & STTS751_STATUS_TRIPH);
> +	priv->min_alert = priv->min_alert || !!(ret & STTS751_STATUS_TRIPL);
> +
I am not really happy about the caching of alert values. This isn't really
common. Usually we report current alert values when reading. I don't really
see why this driver should be different to all the others.

> +	return 0;
> +}
> +
> +static void stts751_alert(struct i2c_client *client,
> +			enum i2c_alert_protocol type, unsigned int data)
> +{
> +	int ret;
> +	struct stts751_priv *priv = i2c_get_clientdata(client);
> +	bool prev_max = priv->max_alert;
> +	bool prev_min = priv->min_alert;
> +
> +	if (type != I2C_PROTOCOL_SMBUS_ALERT)
> +		return;
> +
> +	dev_dbg(&client->dev, "alert!");
> +

Any chance to make this a bit more generic and also support
interrupts ?

> +	mutex_lock(&priv->access_lock);
> +	ret = stts751_update_alert(priv);
> +	if (ret < 0) {
> +		/* default to worst case */
> +		priv->max_alert = true;
> +		priv->min_alert = true;
> +
> +		if (!(prev_max && prev_min)) {
> +			dev_warn(&priv->client->dev,
> +				"Alert received, but can't communicate to the device. Something bad happening? Triggering all alarms!");
> +		}
> +	}
> +
> +	if (!prev_max && priv->max_alert) {
> +		dev_notice(&client->dev, "got alert for HIGH temperature");
> +
> +		/* unblock alert poll */
> +		sysfs_notify(&priv->dev->kobj, NULL, "temp1_event_max_alert");
> +		kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
> +	}
> +
> +	if (!prev_min && priv->min_alert) {
> +		dev_notice(&client->dev, "got alert for LOW temperature");
> +
> +		/* unblock alert poll */
> +		sysfs_notify(&priv->dev->kobj, NULL, "temp1_event_min_alert");
> +		kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
> +	}
> +	mutex_unlock(&priv->access_lock);
> +}
> +
> +static ssize_t show_max_alert(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->max_alert);
> +}
> +
> +static ssize_t set_max_alert(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	mutex_lock(&priv->access_lock);
> +	priv->max_alert = false;
> +	mutex_unlock(&priv->access_lock);
> +
> +	return count;
> +}
> +
> +static ssize_t show_min_alert(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->min_alert);
> +}
> +
> +static ssize_t set_min_alert(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	mutex_lock(&priv->access_lock);
> +	priv->min_alert = false;
> +	mutex_unlock(&priv->access_lock);
> +
This is highly unusual. If the alert needs to be cleared, please auto-clear it
when reading.

> +	return count;
> +}
> +
> +static ssize_t show_input(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	int ret;
> +	int cache_time = STTS751_CACHE_TIME * HZ / 1000;
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	/*
> +	 * Adjust the cache time wrt the sample rate. We do 4X in order to get
> +	 * a new measure in no more than 1/4 of the sample time (that seemed
> +	 * reasonable to me).
> +	 */
> +	cache_time = stts751_intervals[priv->interval] / 4 * HZ / 1000;
> +
> +	if (time_after(jiffies,	priv->last_update + cache_time) ||
> +		!priv->data_valid) {
> +		ret = stts751_update_temp(priv);
> +		if (ret)
> +			return ret;
> +		priv->last_update = jiffies;
> +		priv->data_valid = true;
> +	}
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->temp);
> +}
> +
> +static ssize_t show_therm(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->therm);
> +}
> +
> +
> +static ssize_t set_therm(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	int ret;
> +	long temp;
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	if (kstrtol(buf, 10, &temp) < 0)
> +		return -EINVAL;
> +
> +	ret = stts751_set_temp_reg(priv, temp, false, STTS751_REG_TLIM, 0);
> +	if (ret)
> +		return ret;
> +
Does it really add value to use the same function for 8- and 16-bit registers ?
Seems to me it just makes the code more complicated.

> +	dev_dbg(dev, "setting therm %ld", temp);
> +
> +	priv->therm = temp;
> +	return count;
> +}
> +
> +static ssize_t show_hyst(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->hyst);
> +}
> +
> +
> +static ssize_t set_hyst(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	int ret;
> +	long temp;
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	if (kstrtol(buf, 10, &temp) < 0)
> +		return -EINVAL;
> +
Per ABI, this is supposed to be an absolute temperature. For the chip,
it is a delta. You'll have to do some calculation to convert the absolute
value into a delta. Same for the show command.

> +	ret = stts751_set_temp_reg(priv, temp, false, STTS751_REG_HYST, 0);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(dev, "setting hyst %ld", temp);
> +
> +	priv->hyst = temp;
> +	return count;
> +}
> +
> +static ssize_t show_max(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_max);
> +}
> +
> +static ssize_t set_max(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	int ret;
> +	long temp;
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	if (kstrtol(buf, 10, &temp) < 0)
> +		return -EINVAL;
> +
> +	ret = stts751_set_temp_reg(priv, temp, true,
> +				STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(dev, "setting event max %ld", temp);
> +	priv->event_max = temp;
> +	return count;
> +}
> +
> +static ssize_t show_min(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_min);
> +}
> +
> +

Single empty line, please.

> +static ssize_t set_min(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	int ret;
> +	long temp;
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	if (kstrtol(buf, 10, &temp) < 0)
> +		return -EINVAL;
> +
> +	ret = stts751_set_temp_reg(priv, temp, true,
> +				STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(dev, "setting event min %ld", temp);
> +
> +	priv->event_min = temp;
> +	return count;
> +}
> +
> +static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n",
> +			stts751_intervals[priv->interval]);
> +}
> +
> +static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	unsigned long val;
> +	int idx;
> +	int ret = 0;
> +	struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> +	if (kstrtoul(buf, 10, &val) < 0)
> +		return -EINVAL;
> +
> +	idx = find_closest_descending(val, stts751_intervals,
> +				ARRAY_SIZE(stts751_intervals));
> +
> +	dev_dbg(dev, "setting interval. req:%lu, idx: %d, val: %d", val, idx,
> +		stts751_intervals[idx]);
> +
> +	if (priv->interval == idx)
> +		return count;
> +
> +	mutex_lock(&priv->access_lock);
> +
> +	/* speed up, lower the resolution, then modify convrate */
> +	if (priv->interval < idx) {
> +		priv->interval = idx;
> +		ret = stts751_adjust_resolution(priv);
> +		if (ret)
> +			goto exit;
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_RATE, idx);
> +	if (ret)
> +		goto exit;
> +
> +	/* slow down, modify convrate, then raise resolution */
> +	if (priv->interval != idx) {
> +		priv->interval = idx;
> +		ret = stts751_adjust_resolution(priv);
> +		if (ret)
> +			goto exit;
> +
Extra empty line. 

Is it really necessary to have this code twice ? What is the harm in
just doing it once, either before or after setting the conversion rate ?

> +	}
> +exit:
> +	mutex_unlock(&priv->access_lock);
> +
> +	return count;
> +}
> +
> +static int stts751_detect(struct i2c_client *new_client,
> +			  struct i2c_board_info *info)
> +{
> +	struct i2c_adapter *adapter = new_client->adapter;
> +	const char *name;
> +	int mfg_id, prod_id, rev_id;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +		return -ENODEV;
> +
> +	mfg_id = i2c_smbus_read_byte_data(new_client, ST_MAN_ID);
> +	if (mfg_id != ST_MAN_ID)
> +		return -ENODEV;
> +
> +	prod_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_PROD_ID);
> +
> +	switch (prod_id) {
> +	case STTS751_0_PROD_ID:
> +		name = "STTS751-0";
> +		break;
> +	case STTS751_1_PROD_ID:
> +		name = "STTS751-1";
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +	dev_info(&new_client->dev, "Chip %s detected!", name);
> +
Is this noise necessary ?

> +	rev_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_REV_ID);
> +
> +	if (rev_id != 0x1) {

You might want to check for an error here (and return -ENODEV if you get one).

> +		dev_notice(&new_client->dev,
> +			"Chip revision 0x%x is untested\nPlease report whether it works to andrea.merello@gmail.com",
> +			rev_id);
> +	}
> +
> +	strlcpy(info->type, name, I2C_NAME_SIZE);

This doesn't match the driver name (or, rather, the values in
stts751_id). Does this instantiate the driver ? That would surprise
me.

> +	return 0;
> +}
> +
> +static int stts751_init_chip(struct stts751_priv *priv)
> +{
> +	int ret;
> +
> +	priv->config = STTS751_CONF_EVENT_DIS | STTS751_CONF_STOP;
> +	ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_CONF,
> +					priv->config);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_RATE,
> +					priv->interval);
> +	if (ret)
> +		return ret;
> +
> +	/* invalid, to force update */
> +	priv->res = -1;
> +	ret = stts751_adjust_resolution(priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(priv->client,
> +					STTS751_REG_SMBUS_TO,
> +					priv->smbus_timeout ? 0x80 : 0);
> +	if (ret)
> +		return ret;
> +
> +
> +	if (priv->gen_event) {
> +		ret = stts751_set_temp_reg(priv, priv->event_max, true,
> +					STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
> +		if (ret)
> +			return ret;
> +
> +		ret = stts751_set_temp_reg(priv, priv->event_min, true,
> +					STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);

Those registers all have power-up defaults. Unless there is a good reason,
we should not overwrite them. In some applications, the values may be
initialized from the BIOS/ROMMON.

> +		if (ret)
> +			return ret;
> +		priv->config &= ~STTS751_CONF_EVENT_DIS;
> +	}
> +
> +	if (priv->gen_therm) {
> +		ret = stts751_set_temp_reg(priv, priv->therm, false,
> +					STTS751_REG_TLIM, 0);
> +		if (ret)
> +			return ret;
> +
> +		ret = stts751_set_temp_reg(priv, priv->hyst, false,
> +					STTS751_REG_HYST, 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	priv->config &= ~STTS751_CONF_STOP;
> +	ret = i2c_smbus_write_byte_data(priv->client,
> +					STTS751_REG_CONF, priv->config);
> +
> +	return ret;
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO,
> +			show_min, set_min, 0);
> +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO,
> +			show_max, set_max, 0);
> +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO,
> +			show_min_alert, set_min_alert, 0);
> +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO,
> +			show_max_alert, set_max_alert, 0);
> +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,
> +			set_therm, 0);
> +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO, show_hyst,
> +			set_hyst, 0);

Please use standard attribute names.

> +static SENSOR_DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO,
> +			show_interval, set_interval, 0);
> +
> +/* always present */
> +static struct attribute *stts751_temp_attrs[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute_group stts751_temp_group = {
> +	.attrs = stts751_temp_attrs,
> +};
> +
> +/* present when therm pin or event pin are connected */
> +static struct attribute *stts751_interval_attrs[] = {
> +	&sensor_dev_attr_update_interval.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute_group stts751_interval_group = {
> +	.attrs = stts751_interval_attrs,
> +};
> +
> +/* present when event pin is connected */
> +static struct attribute *stts751_event_attrs[] = {
> +	&sensor_dev_attr_temp1_event_min.dev_attr.attr,
> +	&sensor_dev_attr_temp1_event_max.dev_attr.attr,
> +	&sensor_dev_attr_temp1_event_min_alert.dev_attr.attr,
> +	&sensor_dev_attr_temp1_event_max_alert.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute_group stts751_event_group = {
> +	.attrs = stts751_event_attrs,
> +};
> +
> +/* present when therm pin is connected */
> +static struct attribute *stts751_therm_attrs[] = {
> +	&sensor_dev_attr_temp1_therm.dev_attr.attr,
> +	&sensor_dev_attr_temp1_therm_hyst.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute_group stts751_therm_group = {
> +	.attrs = stts751_therm_attrs,
> +};
> +
> +static int stts751_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct stts751_priv *priv;
> +	int ret;
> +	int groups_idx = 0;
> +	struct device_node *np = client->dev.of_node;
> +
> +	priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->client = client;
> +	i2c_set_clientdata(client, priv);
> +	mutex_init(&priv->access_lock);
> +
> +	/* default to 2 samples per second */
> +	priv->interval = 5;
> +	/* default to timeout enable, as per chip default */
> +	priv->smbus_timeout = true;
> +	priv->last_update = 0;
> +	priv->data_valid = false;
> +	priv->max_alert = false;
> +	priv->min_alert = false;
> +	priv->gen_therm = false;
> +	priv->gen_event = false;

Unnecessary initializations.

> +	priv->therm = STTS751_THERM_DEFAULT;
> +	priv->hyst = STTS751_HYST_DEFAULT;
> +	priv->event_max = STTS751_EVENT_MAX_DEFAULT;
> +	priv->event_min = STTS751_EVENT_MIN_DEFAULT;
> +
I think it would be better to read the initial values from the chip
(also see above).

> +	if (np) {
> +		priv->gen_therm = of_property_read_bool(np, "has-therm");
> +		priv->gen_event = of_property_read_bool(np, "has-event");
> +		priv->smbus_timeout = !of_property_read_bool(np,
> +						"smbus-timeout-disable");
> +	} else {
> +		dev_notice(&client->dev, "No DT data. Event/therm disabled\n");

Seems to be quite rude to penaltize non-DT systems this way.

> +	}
> +
> +	dev_dbg(&client->dev, "gen_event: %s, gen_therm: %s",
> +		priv->gen_event ? "YES" : "NO",
> +		priv->gen_therm ? "YES" : "NO");
> +
> +	priv->groups[groups_idx++] = &stts751_temp_group;
> +	priv->groups[groups_idx++] = &stts751_interval_group;

Any reason for having multiple groups here ?

> +
> +	if (priv->gen_therm)
> +		priv->groups[groups_idx++] = &stts751_therm_group;
> +
> +	if (priv->gen_event)
> +		priv->groups[groups_idx++] = &stts751_event_group;
> +
Why are those groups optional ? This is quite unusual. Even if the chip is not
connected to alert or interrupt lines, it still supports limit registers and
reports alerts. On top of that, at least interrupt support can be detected
automatically (from client->irq) and would not need a devicetree property.

I think you really only need a single group, and all attributes should always
be available.

> +	priv->groups[groups_idx] = NULL;

Unnecessary initialization. devm_kzalloc() initializes priv with 0.

> +
> +	ret = stts751_init_chip(priv);
> +	if (ret)
> +		return ret;
> +
> +	priv->dev = devm_hwmon_device_register_with_groups(&client->dev,
> +							client->name, priv,
> +							priv->groups);
> +	return PTR_ERR_OR_ZERO(priv->dev);
> +}
> +
> +static const struct i2c_device_id stts751_id[] = {
> +	{ "stts751", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, stts751_id);
> +
> +static struct i2c_driver stts751_driver = {
> +	.class		= I2C_CLASS_HWMON,
> +	.driver = {
> +		.name	= DEVNAME,
> +	},
> +	.probe		= stts751_probe,
> +	.id_table	= stts751_id,
> +	.detect		= stts751_detect,
> +	.alert		= stts751_alert,
> +	.address_list	= normal_i2c,
> +};
> +
> +module_i2c_driver(stts751_driver);
> +
> +MODULE_AUTHOR("Andrea Merello <andrea.merello@gmail.com>");
> +MODULE_DESCRIPTION("STTS751 sensor driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html