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

[PATCH 1/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 manual-triggered conversions as well as automatic
conversions. The latter is used when the "event" or "therm" function
is present (declaring the physical wire is attached in the DT).

Thanks-to: LABBE Corentin [for suggestions]
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 | 948 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 960 insertions(+), 2 deletions(-)
 create mode 100644 drivers/hwmon/stts751.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index eaf2f91..8fdd241 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..94b7e2b
--- /dev/null
+++ b/drivers/hwmon/stts751.c
@@ -0,0 +1,948 @@
+/*
+ * 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 the following drivers:
+ * - LM95241 driver, which is:
+ *   Copyright (C) 2008, 2010 Davide Rizzo <elpa.rizzo@gmail.com>
+ * - LM90 driver, which is:
+ *   Copyright (C) 2003-2010  Jean Delvare <jdelvare@suse.de>
+ *
+ * *******************************************************************
+ * NOTE: the STTS751 can reach resolution up to 12 bit. However this
+ * is not always possible/reliable.
+ *
+ * This is because if the device has to generate a thermal/alert
+ * signal, it has to perform continuous conversions. In this case the
+ * max attainable resolution depends by the conversion rate.
+ * Even worse, reading the temperature with resolution better than 8
+ * bits would require reading *two* temperature registers, and this is
+ * exactly what you want NOT to do when the device is running
+ * asynchronously. (looking at the datasheet I couldn't find any trick
+ * to emulate an atomic read: no shadow registers, no any 'update' bit
+ * to set..).
+ *
+ * So, it seems we have three choices here (feel free to suggest any
+ * other..):
+ *
+ * 1) Don't care: once every several conversions you'll get a somewhat
+ *    imprecise value.. I hate it!
+ *    Tricking the user providing him/her super-precise readings that
+ *    sometimes are indeed quite imprecise seems really sneaky to me.
+ *
+ * 2) Stop the device, perform a synchronous conversion, and start it
+ *    again. I'm quite tempted to do this..
+ *
+ * 3) Limit the resolution to 8-bit when the sensor is running. This
+ *    would be both perfectly safe and "correct".
+ *
+ * 4) Try to detect if we went racy, retry the reading few times, and
+ *    return the best we can (eventually falling back to 3).
+ *
+ * Since the thermal/alert signal could be potentially an important
+ * protection needed not to fry the HW, I decided that the option 2
+ * is too risky (what if when we try to re-enable the sensor our smbus
+ * write fails?).
+ *
+ * Obviously I didn't choose the one I hate, so the only remaining
+ * option are 3 and 4.
+ *
+ * 4 is possibly slower than 3 but it seems reasonable; so I choose 4
+ * ********************************************************************
+ *
+ * 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>
+
+#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 */
+
+struct stts751_intervals_t {
+	char str[8];
+	int val;
+};
+
+/* HW index vs ASCII and int times in mS */
+static const struct stts751_intervals_t stts751_intervals[] = {
+	{.str = "16000", .val = 16000},
+	{.str = "8000", .val = 8000},
+	{.str = "4000", .val = 4000},
+	{.str = "2000", .val = 2000},
+	{.str = "1000", .val = 1000},
+	{.str = "500", .val = 500},
+	{.str = "250", .val = 250},
+	{.str = "125", .val = 125},
+	{.str = "62.5", .val = 62},
+	{.str = "31.25", .val = 31}
+};
+
+/* special value to indicate to the SW to use manual mode */
+#define STTS751_INTERVAL_MANUAL 0xFF
+
+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 is always present
+	 * Depending by DT/platdata, therm, event, interval are
+	 * dynamically added.
+	 * There are max 4 entries plus the guard
+	 */
+	const struct attribute_group *groups[5];
+};
+
+static int stts751_manual_conversion(struct stts751_priv *priv)
+{
+	s32 ret;
+	unsigned long timeout;
+
+	/* Any value written to this reg will trigger manual conversion */
+	ret = i2c_smbus_write_byte_data(priv->client,
+				STTS751_REG_ONESHOT, 0xFF);
+	if (ret < 0)
+		return ret;
+
+	timeout = jiffies;
+
+	while (1) {
+		ret = i2c_smbus_read_byte_data(priv->client,
+					STTS751_REG_STATUS);
+		if (ret < 0)
+			return ret;
+		if (!(ret & STTS751_STATUS_BUSY))
+			return 0;
+		if (time_after(jiffies,
+				timeout + STTS751_CONV_TIMEOUT * HZ / 1000)) {
+			dev_warn(&priv->client->dev, "conversion timed out\n");
+			break;
+		}
+	}
+	return -ETIMEDOUT;
+}
+
+/* Converts temperature in C split in integer and fractional parts, as supplied
+ * by the HW, to an integer number in mC
+ */
+static int stts751_to_deg(s32 integer, s32 frac)
+{
+	s32 temp;
+
+	/* frac part is supplied by the HW as a numbert whose bits weight, from
+	 * MSB to LSB, are 2-e1, 2e-2 .. 2e-8; while stored as a regular integer
+	 * it would be interpreted as usual (2e+128, 2e+64 ...), so we basically
+	 * need to divide it by 256 to ajust the bits' weight.
+	 * However this would squash it to zero, so let's convert in in mC (mul
+	 * by 1000) right before divide.
+	 */
+	frac = frac * 1000 / 256;
+	temp = sign_extend32(integer, 7) * 1000L + frac;
+
+	return temp;
+}
+
+/* Converts temperature in mC to value in C split in integer and fractional
+ * parts, as the HW wants.
+ */
+static int stts751_to_hw(int val, u8 *integer, u8 *frac)
+{
+	/* HW works in range -64C to +127C */
+	if ((val > 127000) || (val < -64000))
+		return -EINVAL;
+
+	*integer = val / 1000;
+	/* *frac = 256 * (val % 1000) */
+	*frac = 256 * (long)(val - *integer * 1000) / 1000;
+
+	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;
+	unsigned long sample1, sample2, timeout;
+	int i;
+	int ret = 0;
+
+	mutex_lock(&priv->access_lock);
+
+	if (priv->interval == STTS751_INTERVAL_MANUAL) {
+		/* perform a one-shot on-demand conversion */
+		ret = stts751_manual_conversion(priv);
+		if (ret) {
+			dev_warn(&priv->client->dev,
+				"failed to shot conversion %x\n", ret);
+			goto exit;
+		}
+	}
+
+	for (i = 0; i < STTS751_RACE_RETRY; i++) {
+		sample1 = jiffies;
+		integer1 = i2c_smbus_read_byte_data(priv->client,
+						STTS751_REG_TEMP_H);
+
+		if (integer1 < 0) {
+			ret = integer1;
+			dev_warn(&priv->client->dev,
+				"failed to read H reg %x\n", ret);
+			goto exit;
+		}
+
+		frac = i2c_smbus_read_byte_data(priv->client,
+						STTS751_REG_TEMP_L);
+
+		if (frac < 0) {
+			ret = frac;
+			dev_warn(&priv->client->dev,
+				"failed to read L reg %x\n", ret);
+			goto exit;
+		}
+
+		if (priv->interval == STTS751_INTERVAL_MANUAL) {
+			/* we'll look at integer2 later.. */
+			integer2 = integer1;
+			break;
+		}
+
+		integer2 = i2c_smbus_read_byte_data(priv->client,
+						STTS751_REG_TEMP_H);
+		sample2 = jiffies;
+
+		if (integer2 < 0) {
+			dev_warn(&priv->client->dev,
+				"failed to read H reg (2nd time) %x\n", ret);
+			ret = integer2;
+			goto exit;
+		}
+
+		timeout = stts751_intervals[priv->interval].val * HZ / 1000;
+		timeout -= ((timeout < 10) && (timeout > 1)) ? 1 : timeout / 10;
+		if ((integer1 == integer2) &&
+			time_after(sample1 + timeout, sample2))
+			break;
+
+		/* if we are going on with a racy read, don't pretend to be
+		 * super-precise, just use the MSBs ..
+		 */
+		frac = 0;
+	}
+
+exit:
+	mutex_unlock(&priv->access_lock);
+	if (ret)
+		return ret;
+
+	/* use integer2, because when we fallback to the "MSB-only" compromise
+	 * this is the more recent one
+	 */
+	priv->temp = stts751_to_deg(integer2, frac);
+	return ret;
+}
+
+static int stts751_set_temp_reg(struct stts751_priv *priv, int temp,
+				bool is_frac, u8 hreg, u8 lreg)
+{
+	u8 integer, frac;
+	int ret;
+
+	if (stts751_to_hw(temp, &integer, &frac))
+		return -EINVAL;
+
+	mutex_lock(&priv->access_lock);
+	ret = i2c_smbus_write_byte_data(priv->client, hreg, integer);
+	if (ret)
+		goto exit;
+	if (is_frac)
+		ret = i2c_smbus_write_byte_data(priv->client, lreg, frac);
+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);
+
+	/* If we are in auto conversion mode 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).
+	 */
+	if (priv->interval != STTS751_INTERVAL_MANUAL)
+		cache_time = stts751_intervals[priv->interval].val /
+			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, "%s\n",
+			stts751_intervals[priv->interval].str);
+}
+
+static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	unsigned long val;
+	int i;
+	int ret = 0;
+	const int len = sizeof(stts751_intervals) /
+		sizeof(stts751_intervals[0]) - 1;
+	struct stts751_priv *priv = dev_get_drvdata(dev);
+
+	if (kstrtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+
+	for (i = 0; i < len; i++) {
+		if (val >= stts751_intervals[i].val)
+			break;
+	}
+
+	dev_dbg(dev, "setting interval. req:%lu, idx: %d, val: %d", val, i,
+		stts751_intervals[i].val);
+
+	if (priv->interval == i)
+		return count;
+
+	mutex_lock(&priv->access_lock);
+
+	/* speed up, lower the resolution, then modify convrate */
+	if (priv->interval < i) {
+		priv->interval = i;
+		ret = stts751_adjust_resolution(priv);
+		if (ret)
+			goto exit;
+	}
+
+	ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_RATE, i);
+	if (ret)
+		goto exit;
+
+	/* slow down, modify convrate, then raise resolution */
+	if (priv->interval != i) {
+		priv->interval = i;
+		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;
+	u8 tmp;
+
+	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;
+
+	/* We always need to write a value consistent wrt to the resolution,
+	 * otherwise the sensor does not work.
+	 * If we are in manual mode, we use any value for which all resolutions
+	 * are admitted. 4 is fine.
+	 */
+	tmp = (priv->interval == STTS751_INTERVAL_MANUAL) ? 4 : priv->interval;
+	ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_RATE, tmp);
+	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->interval != STTS751_INTERVAL_MANUAL) {
+		/* user input will not wait for status bit, and we just
+		 * provide the last read value. Make sure we really have one
+		 * before claiming we are ready..
+		 */
+		ret = stts751_manual_conversion(priv);
+		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 *of_node = client->dev.of_node;
+
+	priv = devm_kzalloc(&client->dev,
+			sizeof(struct stts751_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 (of_node) {
+		priv->gen_therm = of_property_read_bool(of_node, "has-therm");
+		priv->gen_event = of_property_read_bool(of_node, "has-event");
+		priv->smbus_timeout = !of_property_read_bool(of_node,
+						"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;
+
+	if (priv->gen_therm || priv->gen_event)
+		priv->groups[groups_idx++] = &stts751_interval_group;
+	else
+		priv->interval = STTS751_INTERVAL_MANUAL;
+
+	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

[PATCH 2/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

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

[LABBE Corentin]

Hello

I have some coments below

On Wed, Sep 07, 2016 at 02:33:36PM +0200, Andrea Merello wrote:
> This patch adds a HWMON driver for ST Microelectronics STTS751
> temperature sensors.
> 
> It does support manual-triggered conversions as well as automatic
> conversions. The latter is used when the "event" or "therm" function
> is present (declaring the physical wire is attached in the DT).
> 
> Thanks-to: LABBE Corentin [for suggestions]
> 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 | 948 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 960 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/hwmon/stts751.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index eaf2f91..8fdd241 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

Two space instead of one

> +	  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

This change need to be in another set of patch

>  	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..94b7e2b
> --- /dev/null
> +++ b/drivers/hwmon/stts751.c
> @@ -0,0 +1,948 @@
> +/*
> + * 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 the following drivers:
> + * - LM95241 driver, which is:
> + *   Copyright (C) 2008, 2010 Davide Rizzo <elpa.rizzo@gmail.com>
> + * - LM90 driver, which is:
> + *   Copyright (C) 2003-2010  Jean Delvare <jdelvare@suse.de>

No need to recopy copyright, we can find them in their driver.

> +
> +/* HW index vs ASCII and int times in mS */
> +static const struct stts751_intervals_t stts751_intervals[] = {
> +	{.str = "16000", .val = 16000},
> +	{.str = "8000", .val = 8000},
> +	{.str = "4000", .val = 4000},
> +	{.str = "2000", .val = 2000},
> +	{.str = "1000", .val = 1000},
> +	{.str = "500", .val = 500},
> +	{.str = "250", .val = 250},
> +	{.str = "125", .val = 125},
> +	{.str = "62.5", .val = 62},
> +	{.str = "31.25", .val = 31}
> +};

So you are lying about 62.5 and 31.25 :) ?

> +
> +/* special value to indicate to the SW to use manual mode */
> +#define STTS751_INTERVAL_MANUAL 0xFF
> +
> +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 is always present
> +	 * Depending by DT/platdata, therm, event, interval are
> +	 * dynamically added.
> +	 * There are max 4 entries plus the guard
> +	 */

This type of comment is wrong, you need /* only at the first line

> +	const struct attribute_group *groups[5];
> +};
> +
> +static int stts751_manual_conversion(struct stts751_priv *priv)
> +{
> +	s32 ret;
> +	unsigned long timeout;
> +
> +	/* Any value written to this reg will trigger manual conversion */
> +	ret = i2c_smbus_write_byte_data(priv->client,
> +				STTS751_REG_ONESHOT, 0xFF);
> +	if (ret < 0)
> +		return ret;
> +
> +	timeout = jiffies;
> +
> +	while (1) {
> +		ret = i2c_smbus_read_byte_data(priv->client,
> +					STTS751_REG_STATUS);
> +		if (ret < 0)
> +			return ret;
> +		if (!(ret & STTS751_STATUS_BUSY))
> +			return 0;
> +		if (time_after(jiffies,
> +				timeout + STTS751_CONV_TIMEOUT * HZ / 1000)) {
> +			dev_warn(&priv->client->dev, "conversion timed out\n");
> +			break;
> +		}
> +	}

Perhaps you could rewrite it as:
do {
...
} while (!time_after(xxx))
dev_warn()

> +	return -ETIMEDOUT;
> +}
> +
> +/* Converts temperature in C split in integer and fractional parts, as supplied
> + * by the HW, to an integer number in mC
> + */

Same problem for comment (and all comentw below)

> +static int stts751_update_temp(struct stts751_priv *priv)
> +{
> +	s32 integer1, integer2, frac;
> +	unsigned long sample1, sample2, timeout;
> +	int i;
> +	int ret = 0;
> +
> +	mutex_lock(&priv->access_lock);
> +
> +	if (priv->interval == STTS751_INTERVAL_MANUAL) {
> +		/* perform a one-shot on-demand conversion */
> +		ret = stts751_manual_conversion(priv);
> +		if (ret) {
> +			dev_warn(&priv->client->dev,
> +				"failed to shot conversion %x\n", ret);
> +			goto exit;
> +		}
> +	}
> +
> +	for (i = 0; i < STTS751_RACE_RETRY; i++) {
> +		sample1 = jiffies;
> +		integer1 = i2c_smbus_read_byte_data(priv->client,
> +						STTS751_REG_TEMP_H);
> +
> +		if (integer1 < 0) {
> +			ret = integer1;
> +			dev_warn(&priv->client->dev,
> +				"failed to read H reg %x\n", ret);
> +			goto exit;
> +		}
> +
> +		frac = i2c_smbus_read_byte_data(priv->client,
> +						STTS751_REG_TEMP_L);
> +
> +		if (frac < 0) {
> +			ret = frac;
> +			dev_warn(&priv->client->dev,
> +				"failed to read L reg %x\n", ret);
> +			goto exit;
> +		}
> +
> +		if (priv->interval == STTS751_INTERVAL_MANUAL) {
> +			/* we'll look at integer2 later.. */
> +			integer2 = integer1;
> +			break;
> +		}
> +
> +		integer2 = i2c_smbus_read_byte_data(priv->client,
> +						STTS751_REG_TEMP_H);
> +		sample2 = jiffies;
> +
> +		if (integer2 < 0) {
> +			dev_warn(&priv->client->dev,
> +				"failed to read H reg (2nd time) %x\n", ret);

Hard to find why you print ret and what exactly it is when you print it.

> +			ret = integer2;
> +			goto exit;
> +		}
> +
> +		timeout = stts751_intervals[priv->interval].val * HZ / 1000;
> +		timeout -= ((timeout < 10) && (timeout > 1)) ? 1 : timeout / 10;

What it is the purpose of this decrease ?

> +		if ((integer1 == integer2) &&
> +			time_after(sample1 + timeout, sample2))
> +			break;
> +
> +		/* if we are going on with a racy read, don't pretend to be
> +		 * super-precise, just use the MSBs ..
> +		 */
> +		frac = 0;
> +	}
> +
> +exit:
> +	mutex_unlock(&priv->access_lock);
> +	if (ret)
> +		return ret;
> +
> +	/* use integer2, because when we fallback to the "MSB-only" compromise
> +	 * this is the more recent one
> +	 */
> +	priv->temp = stts751_to_deg(integer2, frac);
> +	return ret;
> +}
> +
> +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 *of_node = client->dev.of_node;
> +
> +	priv = devm_kzalloc(&client->dev,
> +			sizeof(struct stts751_priv), GFP_KERNEL);

Could be rewrite to sizeof(*priv)

Regards

Labbe Corentin

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

[Guenter Roeck]

On 09/07/2016 06:16 AM, LABBE Corentin wrote:
> Hello
>
> I have some coments below
>
Some more comments below.

> On Wed, Sep 07, 2016 at 02:33:36PM +0200, Andrea Merello wrote:
>> This patch adds a HWMON driver for ST Microelectronics STTS751
>> temperature sensors.
>>
>> It does support manual-triggered conversions as well as automatic
>> conversions. The latter is used when the "event" or "therm" function
>> is present (declaring the physical wire is attached in the DT).
>>

All others are penaltized and have to wait for manual mode results when
reading the temperature ? Why ?

>> Thanks-to: LABBE Corentin [for suggestions]
>> 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 | 948 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 960 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/hwmon/stts751.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index eaf2f91..8fdd241 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
>
> Two space instead of one
>
>> +	  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
>
> This change need to be in another set of patch
>
>>  	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..94b7e2b
>> --- /dev/null
>> +++ b/drivers/hwmon/stts751.c
>> @@ -0,0 +1,948 @@
>> +/*
>> + * 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 the following drivers:
>> + * - LM95241 driver, which is:
>> + *   Copyright (C) 2008, 2010 Davide Rizzo <elpa.rizzo@gmail.com>
>> + * - LM90 driver, which is:
>> + *   Copyright (C) 2003-2010  Jean Delvare <jdelvare@suse.de>
>
> No need to recopy copyright, we can find them in their driver.
>
>> +
>> +/* HW index vs ASCII and int times in mS */
>> +static const struct stts751_intervals_t stts751_intervals[] = {
>> +	{.str = "16000", .val = 16000},
>> +	{.str = "8000", .val = 8000},
>> +	{.str = "4000", .val = 4000},
>> +	{.str = "2000", .val = 2000},
>> +	{.str = "1000", .val = 1000},
>> +	{.str = "500", .val = 500},
>> +	{.str = "250", .val = 250},
>> +	{.str = "125", .val = 125},
>> +	{.str = "62.5", .val = 62},
>> +	{.str = "31.25", .val = 31}
>> +};
>
> So you are lying about 62.5 and 31.25 :) ?
>

I don't see what the table (or rather, the string -> integer conversion) is used for anyway.
kstrtol() and sprintf() (or similar) seem to be doing a good job there. Also, there
is an API function to find the closest matching index in a value table.

>> +
>> +/* special value to indicate to the SW to use manual mode */
>> +#define STTS751_INTERVAL_MANUAL 0xFF
>> +
>> +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 is always present
>> +	 * Depending by DT/platdata, therm, event, interval are
>> +	 * dynamically added.
>> +	 * There are max 4 entries plus the guard
>> +	 */
>
> This type of comment is wrong, you need /* only at the first line
>
>> +	const struct attribute_group *groups[5];
>> +};
>> +
>> +static int stts751_manual_conversion(struct stts751_priv *priv)
>> +{
>> +	s32 ret;
>> +	unsigned long timeout;
>> +
>> +	/* Any value written to this reg will trigger manual conversion */
>> +	ret = i2c_smbus_write_byte_data(priv->client,
>> +				STTS751_REG_ONESHOT, 0xFF);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	timeout = jiffies;
>> +
>> +	while (1) {
>> +		ret = i2c_smbus_read_byte_data(priv->client,
>> +					STTS751_REG_STATUS);
>> +		if (ret < 0)
>> +			return ret;
>> +		if (!(ret & STTS751_STATUS_BUSY))
>> +			return 0;
>> +		if (time_after(jiffies,
>> +				timeout + STTS751_CONV_TIMEOUT * HZ / 1000)) {
>> +			dev_warn(&priv->client->dev, "conversion timed out\n");
>> +			break;
>> +		}
>> +	}
>
> Perhaps you could rewrite it as:
> do {
> ...
> } while (!time_after(xxx))
> dev_warn()
>

It is unacceptable anyway, it being a hot loop, and I am not a friend
of noisy warnings. I'd also like to see an explanation why manual mode
has to be supported in the first place.

>> +	return -ETIMEDOUT;
>> +}
>> +
>> +/* Converts temperature in C split in integer and fractional parts, as supplied
>> + * by the HW, to an integer number in mC
>> + */
>
> Same problem for comment (and all comentw below)
>
>> +static int stts751_update_temp(struct stts751_priv *priv)
>> +{
>> +	s32 integer1, integer2, frac;
>> +	unsigned long sample1, sample2, timeout;
>> +	int i;
>> +	int ret = 0;
>> +
>> +	mutex_lock(&priv->access_lock);
>> +
>> +	if (priv->interval == STTS751_INTERVAL_MANUAL) {
>> +		/* perform a one-shot on-demand conversion */
>> +		ret = stts751_manual_conversion(priv);
>> +		if (ret) {
>> +			dev_warn(&priv->client->dev,
>> +				"failed to shot conversion %x\n", ret);

If those are warnings, they are not errors and should not result in an abort.
If they are errors, they should be displayed as errors. But, as mentioned
before, I am not a friend of noisy drivers.

>> +			goto exit;
>> +		}
>> +	}
>> +
>> +	for (i = 0; i < STTS751_RACE_RETRY; i++) {
>> +		sample1 = jiffies;
>> +		integer1 = i2c_smbus_read_byte_data(priv->client,
>> +						STTS751_REG_TEMP_H);
>> +
>> +		if (integer1 < 0) {
>> +			ret = integer1;
>> +			dev_warn(&priv->client->dev,
>> +				"failed to read H reg %x\n", ret);
>> +			goto exit;
>> +		}
>> +
>> +		frac = i2c_smbus_read_byte_data(priv->client,
>> +						STTS751_REG_TEMP_L);
>> +
>> +		if (frac < 0) {
>> +			ret = frac;
>> +			dev_warn(&priv->client->dev,
>> +				"failed to read L reg %x\n", ret);
>> +			goto exit;
>> +		}
>> +
>> +		if (priv->interval == STTS751_INTERVAL_MANUAL) {
>> +			/* we'll look at integer2 later.. */
>> +			integer2 = integer1;
>> +			break;
>> +		}
>> +
>> +		integer2 = i2c_smbus_read_byte_data(priv->client,
>> +						STTS751_REG_TEMP_H);
>> +		sample2 = jiffies;
>> +
>> +		if (integer2 < 0) {
>> +			dev_warn(&priv->client->dev,
>> +				"failed to read H reg (2nd time) %x\n", ret);
>
> Hard to find why you print ret and what exactly it is when you print it.
>
Plus the check should be right after the assignment.

>> +			ret = integer2;
>> +			goto exit;
>> +		}
>> +
>> +		timeout = stts751_intervals[priv->interval].val * HZ / 1000;
>> +		timeout -= ((timeout < 10) && (timeout > 1)) ? 1 : timeout / 10;
>
> What it is the purpose of this decrease ?
>
>> +		if ((integer1 == integer2) &&
>> +			time_after(sample1 + timeout, sample2))

And what is the purpose of this time_after() ? Other drivers have the
same problem and a much simpler solution that doesn't require a timeout
like this. If it is in fact needed, I would want to see a comment explaining it.

>> +			break;
>> +
>> +		/* if we are going on with a racy read, don't pretend to be
>> +		 * super-precise, just use the MSBs ..
>> +		 */
>> +		frac = 0;
>> +	}
>> +
>> +exit:
>> +	mutex_unlock(&priv->access_lock);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* use integer2, because when we fallback to the "MSB-only" compromise
>> +	 * this is the more recent one
>> +	 */
>> +	priv->temp = stts751_to_deg(integer2, frac);

All other drivers I am aware of manage to handle conversions from 16 bit chip
values to temperatures and from temperatures to chip values with a single parameter.

>> +	return ret;
>> +}
>> +
>> +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 *of_node = client->dev.of_node;
>> +

np is the commonly accepted variable name.

>> +	priv = devm_kzalloc(&client->dev,
>> +			sizeof(struct stts751_priv), GFP_KERNEL);
>
> Could be rewrite to sizeof(*priv)
>
> Regards
>
> Labbe Corentin
>
>

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

[Guenter Roeck]

On 09/07/2016 08:03 AM, Andrea Merello wrote:
>
>
> On Wed, Sep 7, 2016 at 4:07 PM, Guenter Roeck <linux@roeck-us.net <mailto:linux@roeck-us.net>> wrote:
>
>     On 09/07/2016 06:16 AM, LABBE Corentin wrote:
>
>         Hello
>
>         I have some coments below
>
>     Some more comments below.
>
>         On Wed, Sep 07, 2016 at 02:33:36PM +0200, Andrea Merello wrote:
>
>             This patch adds a HWMON driver for ST Microelectronics STTS751
>             temperature sensors.
>
>             It does support manual-triggered conversions as well as automatic
>             conversions. The latter is used when the "event" or "therm" function
>             is present (declaring the physical wire is attached in the DT).
>
>
>     All others are penaltized and have to wait for manual mode results when
>     reading the temperature ? Why ?
>
>
> The reason was that with this HW reading a precise value when automatic conversion is running is a pain (as explained in the long comment at the file beginning).
>

As I said, other drivers / chips have the same problem of "non-atomic" reads across
multiple registers. Maybe there is something else different with this chip, but from
the explanation there is no indication what it might be.

> BTW if we want to avoid waiting, I could try letting the converter running in auto mode, and disable it just before reading, perform the read, and enable again (but IMHO only when we have no therm/event, for the reasons explained in the comment)..
>
>
>
>             Thanks-to: LABBE Corentin [for suggestions]
>             Signed-off-by: Andrea Merello <andrea.merello@gmail.com <mailto:andrea.merello@gmail.com>>
>             Cc: LABBE Corentin <clabbe.montjoie@gmail.com <mailto:clabbe.montjoie@gmail.com>>
>             Cc: Guenter Roeck <linux@roeck-us.net <mailto:linux@roeck-us.net>>
>             Cc: Jean Delvare <jdelvare@suse.com <mailto:jdelvare@suse.com>>
>             ---
>              drivers/hwmon/Kconfig   |  12 +-
>              drivers/hwmon/Makefile  |   2 +-
>              drivers/hwmon/stts751.c | 948 ++++++++++++++++++++++++++++++++++++++++++++++++
>              3 files changed, 960 insertions(+), 2 deletions(-)
>              create mode 100644 drivers/hwmon/stts751.c
>
>             diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>             index eaf2f91..8fdd241 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
>
>
>         Two space instead of one
>
>             +         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
>
>
>         This change need to be in another set of patch
>
>                     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..94b7e2b
>             --- /dev/null
>             +++ b/drivers/hwmon/stts751.c
>             @@ -0,0 +1,948 @@
>             +/*
>             + * 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 <mailto:andrea.merello@gmail.com>>
>             + *
>             + * Based on the following drivers:
>             + * - LM95241 driver, which is:
>             + *   Copyright (C) 2008, 2010 Davide Rizzo <elpa.rizzo@gmail.com <mailto:elpa.rizzo@gmail.com>>
>             + * - LM90 driver, which is:
>             + *   Copyright (C) 2003-2010  Jean Delvare <jdelvare@suse.de <mailto:jdelvare@suse.de>>
>
>
>         No need to recopy copyright, we can find them in their driver.
>
>             +
>             +/* HW index vs ASCII and int times in mS */
>             +static const struct stts751_intervals_t stts751_intervals[] = {
>             +       {.str = "16000", .val = 16000},
>             +       {.str = "8000", .val = 8000},
>             +       {.str = "4000", .val = 4000},
>             +       {.str = "2000", .val = 2000},
>             +       {.str = "1000", .val = 1000},
>             +       {.str = "500", .val = 500},
>             +       {.str = "250", .val = 250},
>             +       {.str = "125", .val = 125},
>             +       {.str = "62.5", .val = 62},
>             +       {.str = "31.25", .val = 31}
>             +};
>
>
>         So you are lying about 62.5 and 31.25 :) ?
>
>
>     I don't see what the table (or rather, the string -> integer conversion) is used for anyway.
>     kstrtol() and sprintf() (or similar) seem to be doing a good job there. Also, there
>     is an API function to find the closest matching index in a value table.
>
>
>
> Well, the table does not only relates strings to integer (that would be really natural doing with kstrtol() and friends), but it also relates the conversion rate with the HW index for that conversion rate (str vs table index). So, given that I would have an array anyway, keeping also the pre-converted values seemed reasonable to me
>
> But I could store just the strings in the table, use kstrtol, and demote the table to be a single-dimensional array, if this is preferred.
>
> I'll look at the API you are suggesting. I wasn't aware of it...
>
>
Try find_nearest().

>
>             +
>             +/* special value to indicate to the SW to use manual mode */
>             +#define STTS751_INTERVAL_MANUAL 0xFF
>             +
>             +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 is always present
>             +        * Depending by DT/platdata, therm, event, interval are
>             +        * dynamically added.
>             +        * There are max 4 entries plus the guard
>             +        */
>
>
>         This type of comment is wrong, you need /* only at the first line
>
>             +       const struct attribute_group *groups[5];
>             +};
>             +
>             +static int stts751_manual_conversion(struct stts751_priv *priv)
>             +{
>             +       s32 ret;
>             +       unsigned long timeout;
>             +
>             +       /* Any value written to this reg will trigger manual conversion */
>             +       ret = i2c_smbus_write_byte_data(priv->client,
>             +                               STTS751_REG_ONESHOT, 0xFF);
>             +       if (ret < 0)
>             +               return ret;
>             +
>             +       timeout = jiffies;
>             +
>             +       while (1) {
>             +               ret = i2c_smbus_read_byte_data(priv->client,
>             +                                       STTS751_REG_STATUS);
>             +               if (ret < 0)
>             +                       return ret;
>             +               if (!(ret & STTS751_STATUS_BUSY))
>             +                       return 0;
>             +               if (time_after(jiffies,
>             +                               timeout + STTS751_CONV_TIMEOUT * HZ / 1000)) {
>             +                       dev_warn(&priv->client->dev, "conversion timed out\n");
>             +                       break;
>             +               }
>             +       }
>
>
>         Perhaps you could rewrite it as:
>         do {
>         ...
>         } while (!time_after(xxx))
>         dev_warn()
>
>
>     It is unacceptable anyway, it being a hot loop, and I am not a friend
>     of noisy warnings. I'd also like to see an explanation why manual mode
>     has to be supported in the first place.
>
>
> I see your point, but It's supposed not to happen unless there is a problem. And if there is, then  I thought you might prefer to have extra noise rather than silence.. Anyway, I can either pull the warn off the loop (or rip it out completely) or try avoiding manual mode (see above for explanation).
>
Just keep in mind that I won't accept a hot loop (ie one that keeps
burning CPU without sleeping).

The only reason for possibly accepting manual mode might be to reduce power consumption.
Since power consumption for continuous mode can be reduced to a max of 35uA, I don't
really see the point. Unless there is more explanation how this chip is different to
other chips in the same situation, I don't really accept the explanation for all the
complexity either.

>
>
>             +       return -ETIMEDOUT;
>             +}
>             +
>             +/* Converts temperature in C split in integer and fractional parts, as supplied
>             + * by the HW, to an integer number in mC
>             + */
>
>
>         Same problem for comment (and all comentw below)
>
>             +static int stts751_update_temp(struct stts751_priv *priv)
>             +{
>             +       s32 integer1, integer2, frac;
>             +       unsigned long sample1, sample2, timeout;
>             +       int i;
>             +       int ret = 0;
>             +
>             +       mutex_lock(&priv->access_lock);
>             +
>             +       if (priv->interval == STTS751_INTERVAL_MANUAL) {
>             +               /* perform a one-shot on-demand conversion */
>             +               ret = stts751_manual_conversion(priv);
>             +               if (ret) {
>             +                       dev_warn(&priv->client->dev,
>             +                               "failed to shot conversion %x\n", ret);
>
>
>     If those are warnings, they are not errors and should not result in an abort.
>     If they are errors, they should be displayed as errors. But, as mentioned
>     before, I am not a friend of noisy drivers.
>
> You're right. It's an error. I might change this is either an error, or given that you don't like it, in a dev_dbg..
>
>
>
>             +                       goto exit;
>             +               }
>             +       }
>             +
>             +       for (i = 0; i < STTS751_RACE_RETRY; i++) {
>             +               sample1 = jiffies;
>             +               integer1 = i2c_smbus_read_byte_data(priv->client,
>             +                                               STTS751_REG_TEMP_H);
>             +
>             +               if (integer1 < 0) {
>             +                       ret = integer1;
>             +                       dev_warn(&priv->client->dev,
>             +                               "failed to read H reg %x\n", ret);
>             +                       goto exit;
>             +               }
>             +
>             +               frac = i2c_smbus_read_byte_data(priv->client,
>             +                                               STTS751_REG_TEMP_L);
>             +
>             +               if (frac < 0) {
>             +                       ret = frac;
>             +                       dev_warn(&priv->client->dev,
>             +                               "failed to read L reg %x\n", ret);
>             +                       goto exit;
>             +               }
>             +
>             +               if (priv->interval == STTS751_INTERVAL_MANUAL) {
>             +                       /* we'll look at integer2 later.. */
>             +                       integer2 = integer1;
>             +                       break;
>             +               }
>             +
>             +               integer2 = i2c_smbus_read_byte_data(priv->client,
>             +                                               STTS751_REG_TEMP_H);
>             +               sample2 = jiffies;
>             +
>             +               if (integer2 < 0) {
>             +                       dev_warn(&priv->client->dev,
>             +                               "failed to read H reg (2nd time) %x\n", ret);
>
>
>         Hard to find why you print ret and what exactly it is when you print it.
>
>     Plus the check should be right after the assignment.
>
>
> Just sample the time ASAP, that is "critical", then check if we have to throw the error.
>
>
>
>
>             +                       ret = integer2;
>             +                       goto exit;
>             +               }
>             +
>             +               timeout = stts751_intervals[priv->interval].val * HZ / 1000;
>             +               timeout -= ((timeout < 10) && (timeout > 1)) ? 1 : timeout / 10;
>
>
>         What it is the purpose of this decrease ?
>
>             +               if ((integer1 == integer2) &&
>             +                       time_after(sample1 + timeout, sample2))
>
>
>     And what is the purpose of this time_after() ? Other drivers have the
>     same problem and a much simpler solution that doesn't require a timeout
>     like this. If it is in fact needed, I would want to see a comment explaining it.
>
>
>
> Will look if I can find another driver with similar issue to snoop at..
>
Try lm90.c:lm90_read16().

>
>
>             +                       break;
>             +
>             +               /* if we are going on with a racy read, don't pretend to be
>             +                * super-precise, just use the MSBs ..
>             +                */
>             +               frac = 0;
>             +       }
>             +
>             +exit:
>             +       mutex_unlock(&priv->access_lock);
>             +       if (ret)
>             +               return ret;
>             +
>             +       /* use integer2, because when we fallback to the "MSB-only" compromise
>             +        * this is the more recent one
>             +        */
>             +       priv->temp = stts751_to_deg(integer2, frac);
>
>
>     All other drivers I am aware of manage to handle conversions from 16 bit chip
>     values to temperatures and from temperatures to chip values with a single parameter.
>
>
> I'll check this.. BTW This chip wants two separated reads from two registers, one for the integer and one for the fractional part. The purpose of this function is interpreting these two raw HW values to convert in a number. Is this so bad ?
>
Again, have a look at lm90.c:lm90_read16() and the conversion functions
in drivers which have to read two registers to read the temperature.

Is it bad ? It makes the code more complicated than it has to be and
increases code size, so, yes, I consider it bad.

To some degree I understand your concerns, but complex problems don't
always mean that there has to be a complex solution. Unless you have a good
reason for the complexity, please keep the driver as simple as possible.
That not only reduces code size, it also reduces the probability to
introduce bugs, and it reduces amount of time we have to spend reviewing
the code.

Thanks,
Guenter

>
>
>             +       return ret;
>             +}
>             +
>             +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 *of_node = client->dev.of_node;
>             +
>
>
>     np is the commonly accepted variable name.
>
>
> Will follow the convention..
>
>
>
>
>             +       priv = devm_kzalloc(&client->dev,
>             +                       sizeof(struct stts751_priv), GFP_KERNEL);
>
>
>         Could be rewrite to sizeof(*priv)
>
>         Regards
>
>         Labbe Corentin
>
>
>
>

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

[Andrea Merello]

[-- Attachment #1: Type: text/plain, Size: 20866 bytes --]

On Wed, Sep 7, 2016 at 5:41 PM, Guenter Roeck <linux@roeck-us.net> wrote:

> On 09/07/2016 08:03 AM, Andrea Merello wrote:
>
>>
>>
>> On Wed, Sep 7, 2016 at 4:07 PM, Guenter Roeck <linux@roeck-us.net
>> <mailto:linux@roeck-us.net>> wrote:
>>
>>     On 09/07/2016 06:16 AM, LABBE Corentin wrote:
>>
>>         Hello
>>
>>         I have some coments below
>>
>>     Some more comments below.
>>
>>         On Wed, Sep 07, 2016 at 02:33:36PM +0200, Andrea Merello wrote:
>>
>>             This patch adds a HWMON driver for ST Microelectronics STTS751
>>             temperature sensors.
>>
>>             It does support manual-triggered conversions as well as
>> automatic
>>             conversions. The latter is used when the "event" or "therm"
>> function
>>             is present (declaring the physical wire is attached in the
>> DT).
>>
>>
>>     All others are penaltized and have to wait for manual mode results
>> when
>>     reading the temperature ? Why ?
>>
>>
>> The reason was that with this HW reading a precise value when automatic
>> conversion is running is a pain (as explained in the long comment at the
>> file beginning).
>>
>>
> As I said, other drivers / chips have the same problem of "non-atomic"
> reads across
> multiple registers. Maybe there is something else different with this
> chip, but from
> the explanation there is no indication what it might be.
>
> BTW if we want to avoid waiting, I could try letting the converter running
>> in auto mode, and disable it just before reading, perform the read, and
>> enable again (but IMHO only when we have no therm/event, for the reasons
>> explained in the comment)..
>>
>>
>>
>>             Thanks-to: LABBE Corentin [for suggestions]
>>             Signed-off-by: Andrea Merello <andrea.merello@gmail.com
>> <mailto:andrea.merello@gmail.com>>
>>             Cc: LABBE Corentin <clabbe.montjoie@gmail.com <mailto:
>> clabbe.montjoie@gmail.com>>
>>             Cc: Guenter Roeck <linux@roeck-us.net <mailto:
>> linux@roeck-us.net>>
>>             Cc: Jean Delvare <jdelvare@suse.com <mailto:jdelvare@suse.com
>> >>
>>
>>             ---
>>              drivers/hwmon/Kconfig   |  12 +-
>>              drivers/hwmon/Makefile  |   2 +-
>>              drivers/hwmon/stts751.c | 948 ++++++++++++++++++++++++++++++
>> ++++++++++++++++++
>>              3 files changed, 960 insertions(+), 2 deletions(-)
>>              create mode 100644 drivers/hwmon/stts751.c
>>
>>             diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>             index eaf2f91..8fdd241 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
>>
>>
>>         Two space instead of one
>>
>>             +         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
>>
>>
>>         This change need to be in another set of patch
>>
>>                     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..94b7e2b
>>             --- /dev/null
>>             +++ b/drivers/hwmon/stts751.c
>>             @@ -0,0 +1,948 @@
>>             +/*
>>             + * 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
>> <mailto:andrea.merello@gmail.com>>
>>             + *
>>             + * Based on the following drivers:
>>             + * - LM95241 driver, which is:
>>             + *   Copyright (C) 2008, 2010 Davide Rizzo <
>> elpa.rizzo@gmail.com <mailto:elpa.rizzo@gmail.com>>
>>             + * - LM90 driver, which is:
>>             + *   Copyright (C) 2003-2010  Jean Delvare <jdelvare@suse.de
>> <mailto:jdelvare@suse.de>>
>>
>>
>>         No need to recopy copyright, we can find them in their driver.
>>
>>             +
>>             +/* HW index vs ASCII and int times in mS */
>>             +static const struct stts751_intervals_t stts751_intervals[]
>> = {
>>             +       {.str = "16000", .val = 16000},
>>             +       {.str = "8000", .val = 8000},
>>             +       {.str = "4000", .val = 4000},
>>             +       {.str = "2000", .val = 2000},
>>             +       {.str = "1000", .val = 1000},
>>             +       {.str = "500", .val = 500},
>>             +       {.str = "250", .val = 250},
>>             +       {.str = "125", .val = 125},
>>             +       {.str = "62.5", .val = 62},
>>             +       {.str = "31.25", .val = 31}
>>             +};
>>
>>
>>         So you are lying about 62.5 and 31.25 :) ?
>>
>>
>>     I don't see what the table (or rather, the string -> integer
>> conversion) is used for anyway.
>>     kstrtol() and sprintf() (or similar) seem to be doing a good job
>> there. Also, there
>>     is an API function to find the closest matching index in a value
>> table.
>>
>>
>>
>> Well, the table does not only relates strings to integer (that would be
>> really natural doing with kstrtol() and friends), but it also relates the
>> conversion rate with the HW index for that conversion rate (str vs table
>> index). So, given that I would have an array anyway, keeping also the
>> pre-converted values seemed reasonable to me
>>
>> But I could store just the strings in the table, use kstrtol, and demote
>> the table to be a single-dimensional array, if this is preferred.
>>
>> I'll look at the API you are suggesting. I wasn't aware of it...
>>
>>
>> Try find_nearest().
>
>
Ok


>
>
>>             +
>>             +/* special value to indicate to the SW to use manual mode */
>>             +#define STTS751_INTERVAL_MANUAL 0xFF
>>             +
>>             +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 is always present
>>             +        * Depending by DT/platdata, therm, event, interval
>> are
>>             +        * dynamically added.
>>             +        * There are max 4 entries plus the guard
>>             +        */
>>
>>
>>         This type of comment is wrong, you need /* only at the first line
>>
>>             +       const struct attribute_group *groups[5];
>>             +};
>>             +
>>             +static int stts751_manual_conversion(struct stts751_priv
>> *priv)
>>             +{
>>             +       s32 ret;
>>             +       unsigned long timeout;
>>             +
>>             +       /* Any value written to this reg will trigger manual
>> conversion */
>>             +       ret = i2c_smbus_write_byte_data(priv->client,
>>             +                               STTS751_REG_ONESHOT, 0xFF);
>>             +       if (ret < 0)
>>             +               return ret;
>>             +
>>             +       timeout = jiffies;
>>             +
>>             +       while (1) {
>>             +               ret = i2c_smbus_read_byte_data(priv->client,
>>             +                                       STTS751_REG_STATUS);
>>             +               if (ret < 0)
>>             +                       return ret;
>>             +               if (!(ret & STTS751_STATUS_BUSY))
>>             +                       return 0;
>>             +               if (time_after(jiffies,
>>             +                               timeout +
>> STTS751_CONV_TIMEOUT * HZ / 1000)) {
>>             +                       dev_warn(&priv->client->dev,
>> "conversion timed out\n");
>>             +                       break;
>>             +               }
>>             +       }
>>
>>
>>         Perhaps you could rewrite it as:
>>         do {
>>         ...
>>         } while (!time_after(xxx))
>>         dev_warn()
>>
>>
>>     It is unacceptable anyway, it being a hot loop, and I am not a friend
>>     of noisy warnings. I'd also like to see an explanation why manual mode
>>     has to be supported in the first place.
>>
>>
>> I see your point, but It's supposed not to happen unless there is a
>> problem. And if there is, then  I thought you might prefer to have extra
>> noise rather than silence.. Anyway, I can either pull the warn off the loop
>> (or rip it out completely) or try avoiding manual mode (see above for
>> explanation).
>>
>> Just keep in mind that I won't accept a hot loop (ie one that keeps
> burning CPU without sleeping).
>
>
OK


> The only reason for possibly accepting manual mode might be to reduce
> power consumption.
> Since power consumption for continuous mode can be reduced to a max of
> 35uA, I don't
> really see the point. Unless there is more explanation how this chip is
> different to
> other chips in the same situation, I don't really accept the explanation
> for all the
> complexity either.
>
>
>
OK, I'll try to make it always working in automatic mode.


>
>>
>>             +       return -ETIMEDOUT;
>>             +}
>>             +
>>             +/* Converts temperature in C split in integer and fractional
>> parts, as supplied
>>             + * by the HW, to an integer number in mC
>>             + */
>>
>>
>>         Same problem for comment (and all comentw below)
>>
>>             +static int stts751_update_temp(struct stts751_priv *priv)
>>             +{
>>             +       s32 integer1, integer2, frac;
>>             +       unsigned long sample1, sample2, timeout;
>>             +       int i;
>>             +       int ret = 0;
>>             +
>>             +       mutex_lock(&priv->access_lock);
>>             +
>>             +       if (priv->interval == STTS751_INTERVAL_MANUAL) {
>>             +               /* perform a one-shot on-demand conversion */
>>             +               ret = stts751_manual_conversion(priv);
>>             +               if (ret) {
>>             +                       dev_warn(&priv->client->dev,
>>             +                               "failed to shot conversion
>> %x\n", ret);
>>
>>
>>     If those are warnings, they are not errors and should not result in
>> an abort.
>>     If they are errors, they should be displayed as errors. But, as
>> mentioned
>>     before, I am not a friend of noisy drivers.
>>
>> You're right. It's an error. I might change this is either an error, or
>> given that you don't like it, in a dev_dbg..
>>
>>
>>
>>             +                       goto exit;
>>             +               }
>>             +       }
>>             +
>>             +       for (i = 0; i < STTS751_RACE_RETRY; i++) {
>>             +               sample1 = jiffies;
>>             +               integer1 = i2c_smbus_read_byte_data(priv-
>> >client,
>>             +
>>  STTS751_REG_TEMP_H);
>>             +
>>             +               if (integer1 < 0) {
>>             +                       ret = integer1;
>>             +                       dev_warn(&priv->client->dev,
>>             +                               "failed to read H reg %x\n",
>> ret);
>>             +                       goto exit;
>>             +               }
>>             +
>>             +               frac = i2c_smbus_read_byte_data(priv->client,
>>             +
>>  STTS751_REG_TEMP_L);
>>             +
>>             +               if (frac < 0) {
>>             +                       ret = frac;
>>             +                       dev_warn(&priv->client->dev,
>>             +                               "failed to read L reg %x\n",
>> ret);
>>             +                       goto exit;
>>             +               }
>>             +
>>             +               if (priv->interval ==
>> STTS751_INTERVAL_MANUAL) {
>>             +                       /* we'll look at integer2 later.. */
>>             +                       integer2 = integer1;
>>             +                       break;
>>             +               }
>>             +
>>             +               integer2 = i2c_smbus_read_byte_data(priv-
>> >client,
>>             +
>>  STTS751_REG_TEMP_H);
>>             +               sample2 = jiffies;
>>             +
>>             +               if (integer2 < 0) {
>>             +                       dev_warn(&priv->client->dev,
>>             +                               "failed to read H reg (2nd
>> time) %x\n", ret);
>>
>>
>>         Hard to find why you print ret and what exactly it is when you
>> print it.
>>
>>     Plus the check should be right after the assignment.
>>
>>
>> Just sample the time ASAP, that is "critical", then check if we have to
>> throw the error.
>>
>>
>>
>>
>>             +                       ret = integer2;
>>             +                       goto exit;
>>             +               }
>>             +
>>             +               timeout = stts751_intervals[priv->interval].val
>> * HZ / 1000;
>>             +               timeout -= ((timeout < 10) && (timeout > 1))
>> ? 1 : timeout / 10;
>>
>>
>>         What it is the purpose of this decrease ?
>>
>>             +               if ((integer1 == integer2) &&
>>             +                       time_after(sample1 + timeout,
>> sample2))
>>
>>
>>     And what is the purpose of this time_after() ? Other drivers have the
>>     same problem and a much simpler solution that doesn't require a
>> timeout
>>     like this. If it is in fact needed, I would want to see a comment
>> explaining it.
>>
>>
>>
>> Will look if I can find another driver with similar issue to snoop at..
>>
>> Try lm90.c:lm90_read16().
>


I see.. This is reasonable, and I will implement this trick in stts751
driver.. This seems to have good chances to read correctly, but it can
still go racy for example if the converter ends a conversion between the
first two reads, and we get preempted in between the last two reads (so
that the HW ends another conversion). IMHO looking _also_  at time, and
eventually retry, makes things even more robust..

I'd say we could add time check and retry (with proper cpu_relax() to avoid
those hot loops) also to lm90 rather than removing it from stts651  :)


>
>
>>
>>             +                       break;
>>             +
>>             +               /* if we are going on with a racy read, don't
>> pretend to be
>>             +                * super-precise, just use the MSBs ..
>>             +                */
>>             +               frac = 0;
>>             +       }
>>             +
>>             +exit:
>>             +       mutex_unlock(&priv->access_lock);
>>             +       if (ret)
>>             +               return ret;
>>             +
>>             +       /* use integer2, because when we fallback to the
>> "MSB-only" compromise
>>             +        * this is the more recent one
>>             +        */
>>             +       priv->temp = stts751_to_deg(integer2, frac);
>>
>>
>>     All other drivers I am aware of manage to handle conversions from 16
>> bit chip
>>     values to temperatures and from temperatures to chip values with a
>> single parameter.
>>
>>
>> I'll check this.. BTW This chip wants two separated reads from two
>> registers, one for the integer and one for the fractional part. The purpose
>> of this function is interpreting these two raw HW values to convert in a
>> number. Is this so bad ?
>>
>> Again, have a look at lm90.c:lm90_read16() and the conversion functions
> in drivers which have to read two registers to read the temperature.
>
> Is it bad ? It makes the code more complicated than it has to be and
> increases code size, so, yes, I consider it bad.
>

It does not look like that the simple register combination that lm90 does
could also work for the integer/fractional format of the stts751.

I'll check again more carefully to see if for some math trick the two
things eventually end up to produce the same result, but right now it seems
to me that we need to do some math over the two read values in stts751. And
the same for the reverse direction conversion.

If the point above turns out to be true, then I honestly can't see how
dividing the two-registers read (with its trick to avoid races) and the
math calculations in two functions, as it is now,  could make things
appearing so complicated. Honestly I would say the opposite :) .. However
if it's better for you, I can merge the two things in one single function..


>
> To some degree I understand your concerns, but complex problems don't
> always mean that there has to be a complex solution. Unless you have a good
> reason for the complexity, please keep the driver as simple as possible.
> That not only reduces code size, it also reduces the probability to
> introduce bugs, and it reduces amount of time we have to spend reviewing
> the code.
>

In general that's absolutely true :) But IMHO here we have neither
complicated or simple solutions; we just have a certain degree of
probability to do a (slightly) wrong read (in lm90 also), and we can put
less or more effort (code) in trying to reduce this probability.

If you feel that what lm90 does is enough, and that is the "standard" way,
I'll do the same.. Although I still would prefer to be more paranoid about
races..


>
> Thanks,
> Guenter


BTW I will come back with a V2 as per your requirements, but unfortunately
it will not happen very soon ;(

Thanks
Andrea

>
>
>
>>
>>             +       return ret;
>>             +}
>>             +
>>             +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 *of_node = client->dev.of_node;
>>             +
>>
>>
>>     np is the commonly accepted variable name.
>>
>>
>> Will follow the convention..
>>
>>
>>
>>
>>             +       priv = devm_kzalloc(&client->dev,
>>             +                       sizeof(struct stts751_priv),
>> GFP_KERNEL);
>>
>>
>>         Could be rewrite to sizeof(*priv)
>>
>>         Regards
>>
>>         Labbe Corentin
>>
>>
>>
>>
>>
>

[-- Attachment #2: Type: text/html, Size: 28907 bytes --]

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

[Guenter Roeck]

On 09/08/2016 12:42 AM, Andrea Merello wrote:
>
[ ... ]

>
>         Will look if I can find another driver with similar issue to snoop at..
>
>     Try lm90.c:lm90_read16().
>
>
>
> I see.. This is reasonable, and I will implement this trick in stts751 driver.. This seems to have good chances to read correctly, but it can still go racy for example if the converter ends a conversion between the first two reads, and we get preempted in between the last two reads (so that the HW ends another conversion). IMHO looking _also_  at time, and eventually retry, makes things even more robust..
>
As a general comment, please use a mailer which splits lines at 80 columns or less.

While I understand your concern, the time check is complete overkill.
What is the probability for a double bad reading, and on top of that one that
includes a change in the high temperature register ? One in a Million ?
With a downside of a wrong fraction in the "error" case ? And what does that
really mean, on a chip with a specified accuracy of +/- 1 degrees C ?

Even your solution to drop the fraction in some cases doesn't really solve anything.
It now reports, say, 30.0 degrees C instead of 30.875 degrees C. How is that better
than a "wrong" 30.X degrees C, where X is any other value it might report ?

> I'd say we could add time check and retry (with proper cpu_relax() to avoid those hot loops) also to lm90 rather than removing it from stts651  :)
>
Only if you can show that the problem is seen in the real world.

>
>
>
>
>                     +                       break;
>                     +
>                     +               /* if we are going on with a racy read, don't pretend to be
>                     +                * super-precise, just use the MSBs ..
>                     +                */
>                     +               frac = 0;
>                     +       }
>                     +
>                     +exit:
>                     +       mutex_unlock(&priv->access_lock);
>                     +       if (ret)
>                     +               return ret;
>                     +
>                     +       /* use integer2, because when we fallback to the "MSB-only" compromise
>                     +        * this is the more recent one
>                     +        */
>                     +       priv->temp = stts751_to_deg(integer2, frac);
>
>
>             All other drivers I am aware of manage to handle conversions from 16 bit chip
>             values to temperatures and from temperatures to chip values with a single parameter.
>
>
>         I'll check this.. BTW This chip wants two separated reads from two registers, one for the integer and one for the fractional part. The purpose of this function is interpreting these two raw HW values to convert in a number. Is this so bad ?
>
>     Again, have a look at lm90.c:lm90_read16() and the conversion functions
>     in drivers which have to read two registers to read the temperature.
>
>     Is it bad ? It makes the code more complicated than it has to be and
>     increases code size, so, yes, I consider it bad.
>
>
> It does not look like that the simple register combination that lm90 does could also work for the integer/fractional format of the stts751.
>
Why ? The register formats are exactly the same.

> I'll check again more carefully to see if for some math trick the two things eventually end up to produce the same result, but right now it seems to me that we need to do some math over the two read values in stts751. And the same for the reverse direction conversion.
>
> If the point above turns out to be true, then I honestly can't see how dividing the two-registers read (with its trick to avoid races) and the math calculations in two functions, as it is now,  could make things appearing so complicated. Honestly I would say the opposite :) .. However if it's better for you, I can merge the two things in one single function..
>
Two vs. one function isn't the question here. The lm90 driver combines the two register
values into a single variable. Using a single variable is what I asked for.

>
>
>     To some degree I understand your concerns, but complex problems don't
>     always mean that there has to be a complex solution. Unless you have a good
>     reason for the complexity, please keep the driver as simple as possible.
>     That not only reduces code size, it also reduces the probability to
>     introduce bugs, and it reduces amount of time we have to spend reviewing
>     the code.
>
>
> In general that's absolutely true :) But IMHO here we have neither complicated or simple solutions; we just have a certain degree of probability to do a (slightly) wrong read (in lm90 also), and we can put less or more effort (code) in trying to reduce this probability.
>
> If you feel that what lm90 does is enough, and that is the "standard" way, I'll do the same.. Although I still would prefer to be more paranoid about races..
>
I would say it is sufficient. Many sensors have the same problem, and it would
really be up to the chip vendors to define better (ie 16 bit) register sets.
Until then, we have to find a trade-off between code complexity and avoiding
races like this.

>
> BTW I will come back with a V2 as per your requirements, but unfortunately it will not happen very soon ;(
>
No problem.

Guenter

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

[Andrea Merello]

On Thu, Sep 8, 2016 at 3:35 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 09/08/2016 12:42 AM, Andrea Merello wrote:
>>
>>
> [ ... ]
>
>>
>>         Will look if I can find another driver with similar issue to snoop at..
>>
>>     Try lm90.c:lm90_read16().
>>
>>
>>
>> I see.. This is reasonable, and I will implement this trick in stts751 driver.. This seems to have good chances to read correctly, but it can still go racy for example if the converter ends a conversion between the first two reads, and we get preempted in between the last two reads (so that the HW ends another conversion). IMHO looking _also_  at time, and eventually retry, makes things even more robust..
>>
> As a general comment, please use a mailer which splits lines at 80 columns or less.

Thanks for pointing this out. (Probably I just need to remember to
switch gmail in plain text mode..)

>
>
> While I understand your concern, the time check is complete overkill.
> What is the probability for a double bad reading, and on top of that one that
> includes a change in the high temperature register ? One in a Million ?
> With a downside of a wrong fraction in the "error" case ? And what does that
> really mean, on a chip with a specified accuracy of +/- 1 degrees C ?

Yes, I see your point, and this is indeed true. Probably I have been
too paranoid on this...

> Even your solution to drop the fraction in some cases doesn't really solve anything.
> It now reports, say, 30.0 degrees C instead of 30.875 degrees C. How is that better
> than a "wrong" 30.X degrees C, where X is any other value it might report ?

You're right, I could let the "wrong" fraction data. (but the point of
my solution was not this indeed).

>> I'd say we could add time check and retry (with proper cpu_relax() to avoid those hot loops) also to lm90 rather than removing it from stts651  :)
>>
> Only if you can show that the problem is seen in the real world.
>
>
>>
>>
>>
>>
>>                     +                       break;
>>                     +
>>                     +               /* if we are going on with a racy read, don't pretend to be
>>                     +                * super-precise, just use the MSBs ..
>>                     +                */
>>                     +               frac = 0;
>>                     +       }
>>                     +
>>                     +exit:
>>                     +       mutex_unlock(&priv->access_lock);
>>                     +       if (ret)
>>                     +               return ret;
>>                     +
>>                     +       /* use integer2, because when we fallback to the "MSB-only" compromise
>>                     +        * this is the more recent one
>>                     +        */
>>                     +       priv->temp = stts751_to_deg(integer2, frac);
>>
>>
>>             All other drivers I am aware of manage to handle conversions from 16 bit chip
>>             values to temperatures and from temperatures to chip values with a single parameter.
>>
>>
>>         I'll check this.. BTW This chip wants two separated reads from two registers, one for the integer and one for the fractional part. The purpose of this function is interpreting these two raw HW values to convert in a number. Is this so bad ?
>>
>>     Again, have a look at lm90.c:lm90_read16() and the conversion functions
>>     in drivers which have to read two registers to read the temperature.
>>
>>     Is it bad ? It makes the code more complicated than it has to be and
>>     increases code size, so, yes, I consider it bad.
>>
>>
>> It does not look like that the simple register combination that lm90 does could also work for the integer/fractional format of the stts751.
>>
> Why ? The register formats are exactly the same.

Aah, I didn't saw temp_from_s16() in lm90.c that does the conversion
afterwards, thus I assumed the data read from lm90 didn't need any
conversion and that the reg format was different.. sorry.. Now I got
what you meant :)

>> I'll check again more carefully to see if for some math trick the two things eventually end up to produce the same result, but right now it seems to me that we need to do some math over the two read values in stts751. And the same for the reverse direction conversion.
>>
>> If the point above turns out to be true, then I honestly can't see how dividing the two-registers read (with its trick to avoid races) and the math calculations in two functions, as it is now,  could make things appearing so complicated. Honestly I would say the opposite :) .. However if it's better for you, I can merge the two things in one single function..
>>
> Two vs. one function isn't the question here. The lm90 driver combines the two register
> values into a single variable. Using a single variable is what I asked for.

OK, I'll do the lm90 way :)

>>
>>
>>     To some degree I understand your concerns, but complex problems don't
>>     always mean that there has to be a complex solution. Unless you have a good
>>     reason for the complexity, please keep the driver as simple as possible.
>>     That not only reduces code size, it also reduces the probability to
>>     introduce bugs, and it reduces amount of time we have to spend reviewing
>>     the code.
>>
>>
>> In general that's absolutely true :) But IMHO here we have neither complicated or simple solutions; we just have a certain degree of probability to do a (slightly) wrong read (in lm90 also), and we can put less or more effort (code) in trying to reduce this probability.
>>
>> If you feel that what lm90 does is enough, and that is the "standard" way, I'll do the same.. Although I still would prefer to be more paranoid about races..
>>
> I would say it is sufficient. Many sensors have the same problem, and it would
> really be up to the chip vendors to define better (ie 16 bit) register sets.
> Until then, we have to find a trade-off between code complexity and avoiding
> races like this.

I see..

Thank you very much for discussion and explanations :)

Andrea

>>
>> BTW I will come back with a V2 as per your requirements, but unfortunately it will not happen very soon ;(
>>
> No problem.
>
> Guenter
>

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

[Andrea Merello]

[-- Attachment #1: Type: text/plain, Size: 10955 bytes --]

On Wed, Sep 7, 2016 at 3:16 PM, LABBE Corentin <clabbe.montjoie@gmail.com>
wrote:

> Hello
>
> I have some coments below
>
> On Wed, Sep 07, 2016 at 02:33:36PM +0200, Andrea Merello wrote:
> > This patch adds a HWMON driver for ST Microelectronics STTS751
> > temperature sensors.
> >
> > It does support manual-triggered conversions as well as automatic
> > conversions. The latter is used when the "event" or "therm" function
> > is present (declaring the physical wire is attached in the DT).
> >
> > Thanks-to: LABBE Corentin [for suggestions]
> > 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 | 948 ++++++++++++++++++++++++++++++
> ++++++++++++++++++
> >  3 files changed, 960 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/hwmon/stts751.c
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index eaf2f91..8fdd241 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
>
> Two space instead of one
>

OK


>
> > +       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
>
> This change need to be in another set of patch
>

OK


>
> >       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..94b7e2b
> > --- /dev/null
> > +++ b/drivers/hwmon/stts751.c
> > @@ -0,0 +1,948 @@
> > +/*
> > + * 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 the following drivers:
> > + * - LM95241 driver, which is:
> > + *   Copyright (C) 2008, 2010 Davide Rizzo <elpa.rizzo@gmail.com>
> > + * - LM90 driver, which is:
> > + *   Copyright (C) 2003-2010  Jean Delvare <jdelvare@suse.de>
>
> No need to recopy copyright, we can find them in their driver.
>

OK


>
> > +
> > +/* HW index vs ASCII and int times in mS */
> > +static const struct stts751_intervals_t stts751_intervals[] = {
> > +     {.str = "16000", .val = 16000},
> > +     {.str = "8000", .val = 8000},
> > +     {.str = "4000", .val = 4000},
> > +     {.str = "2000", .val = 2000},
> > +     {.str = "1000", .val = 1000},
> > +     {.str = "500", .val = 500},
> > +     {.str = "250", .val = 250},
> > +     {.str = "125", .val = 125},
> > +     {.str = "62.5", .val = 62},
> > +     {.str = "31.25", .val = 31}
> > +};
>
> So you are lying about 62.5 and 31.25 :) ?
>

No, this is not my intention :)
The .val data is used only to find out reasonable (rounded) values for
things like timeout and cache time, as well as finding the closest walue
the HW can do. The user will see the (correct) string and the HW will use
the right value..


>
> > +
> > +/* special value to indicate to the SW to use manual mode */
> > +#define STTS751_INTERVAL_MANUAL 0xFF
> > +
> > +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 is always present
> > +      * Depending by DT/platdata, therm, event, interval are
> > +      * dynamically added.
> > +      * There are max 4 entries plus the guard
> > +      */
>
> This type of comment is wrong, you need /* only at the first line
>

OK


>
> > +     const struct attribute_group *groups[5];
> > +};
> > +
> > +static int stts751_manual_conversion(struct stts751_priv *priv)
> > +{
> > +     s32 ret;
> > +     unsigned long timeout;
> > +
> > +     /* Any value written to this reg will trigger manual conversion */
> > +     ret = i2c_smbus_write_byte_data(priv->client,
> > +                             STTS751_REG_ONESHOT, 0xFF);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     timeout = jiffies;
> > +
> > +     while (1) {
> > +             ret = i2c_smbus_read_byte_data(priv->client,
> > +                                     STTS751_REG_STATUS);
> > +             if (ret < 0)
> > +                     return ret;
> > +             if (!(ret & STTS751_STATUS_BUSY))
> > +                     return 0;
> > +             if (time_after(jiffies,
> > +                             timeout + STTS751_CONV_TIMEOUT * HZ /
> 1000)) {
> > +                     dev_warn(&priv->client->dev, "conversion timed
> out\n");
> > +                     break;
> > +             }
> > +     }
>
> Perhaps you could rewrite it as:
> do {
> ...
> } while (!time_after(xxx))
> dev_warn()
>

OK


>
> > +     return -ETIMEDOUT;
> > +}
> > +
> > +/* Converts temperature in C split in integer and fractional parts, as
> supplied
> > + * by the HW, to an integer number in mC
> > + */
>
> Same problem for comment (and all comentw below)
>

OK


>
> > +static int stts751_update_temp(struct stts751_priv *priv)
> > +{
> > +     s32 integer1, integer2, frac;
> > +     unsigned long sample1, sample2, timeout;
> > +     int i;
> > +     int ret = 0;
> > +
> > +     mutex_lock(&priv->access_lock);
> > +
> > +     if (priv->interval == STTS751_INTERVAL_MANUAL) {
> > +             /* perform a one-shot on-demand conversion */
> > +             ret = stts751_manual_conversion(priv);
> > +             if (ret) {
> > +                     dev_warn(&priv->client->dev,
> > +                             "failed to shot conversion %x\n", ret);
> > +                     goto exit;
> > +             }
> > +     }
> > +
> > +     for (i = 0; i < STTS751_RACE_RETRY; i++) {
> > +             sample1 = jiffies;
> > +             integer1 = i2c_smbus_read_byte_data(priv->client,
> > +                                             STTS751_REG_TEMP_H);
> > +
> > +             if (integer1 < 0) {
> > +                     ret = integer1;
> > +                     dev_warn(&priv->client->dev,
> > +                             "failed to read H reg %x\n", ret);
> > +                     goto exit;
> > +             }
> > +
> > +             frac = i2c_smbus_read_byte_data(priv->client,
> > +                                             STTS751_REG_TEMP_L);
> > +
> > +             if (frac < 0) {
> > +                     ret = frac;
> > +                     dev_warn(&priv->client->dev,
> > +                             "failed to read L reg %x\n", ret);
> > +                     goto exit;
> > +             }
> > +
> > +             if (priv->interval == STTS751_INTERVAL_MANUAL) {
> > +                     /* we'll look at integer2 later.. */
> > +                     integer2 = integer1;
> > +                     break;
> > +             }
> > +
> > +             integer2 = i2c_smbus_read_byte_data(priv->client,
> > +                                             STTS751_REG_TEMP_H);
> > +             sample2 = jiffies;
> > +
> > +             if (integer2 < 0) {
> > +                     dev_warn(&priv->client->dev,
> > +                             "failed to read H reg (2nd time) %x\n",
> ret);
>
> Hard to find why you print ret and what exactly it is when you print it.
>
> OK, I will change this someway


> > +                     ret = integer2;
> > +                     goto exit;
> > +             }
> > +
> > +             timeout = stts751_intervals[priv->interval].val * HZ /
> 1000;
> > +             timeout -= ((timeout < 10) && (timeout > 1)) ? 1 : timeout
> / 10;
>
> What it is the purpose of this decrease ?
>

Now that I'm looking at it again, I admit I'm not completely convinced of
it me too. BTW it was meant for being conservative, checking for a time
interval slightly less than the real conversion rate.

Anyway I will add a comment and eventually rewrite/fix it.

>
> > +             if ((integer1 == integer2) &&
> > +                     time_after(sample1 + timeout, sample2))
> > +                     break;
> > +
> > +             /* if we are going on with a racy read, don't pretend to be
> > +              * super-precise, just use the MSBs ..
> > +              */
> > +             frac = 0;
> > +     }
> > +
> > +exit:
> > +     mutex_unlock(&priv->access_lock);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* use integer2, because when we fallback to the "MSB-only"
> compromise
> > +      * this is the more recent one
> > +      */
> > +     priv->temp = stts751_to_deg(integer2, frac);
> > +     return ret;
> > +}
> > +
> > +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 *of_node = client->dev.of_node;
> > +
> > +     priv = devm_kzalloc(&client->dev,
> > +                     sizeof(struct stts751_priv), GFP_KERNEL);
>
> Could be rewrite to sizeof(*priv)
>

OK


>
> Regards
>
> Labbe Corentin
>
>

[-- Attachment #2: Type: text/html, Size: 16475 bytes --]