[PATCH 1/3] hwmon: Add Texas Instruments TMP108 temperature sensor driver.

[PATCH 1/3] hwmon: Add Texas Instruments TMP108 temperature sensor driver.

[John Muir]

Add support for the TI TMP108 temperature sensor with some device
configuration parameters.

Signed-off-by: John Muir <john@jmuir.com>
---
 Documentation/devicetree/bindings/hwmon/tmp108.txt |  27 ++
 Documentation/hwmon/tmp108                         |  38 ++
 drivers/hwmon/Kconfig                              |  11 +
 drivers/hwmon/Makefile                             |   1 +
 drivers/hwmon/tmp108.c                             | 429 +++++++++++++++++++++
 5 files changed, 506 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/tmp108.txt
 create mode 100644 Documentation/hwmon/tmp108
 create mode 100644 drivers/hwmon/tmp108.c

diff --git a/Documentation/devicetree/bindings/hwmon/tmp108.txt b/Documentation/devicetree/bindings/hwmon/tmp108.txt
new file mode 100644
index 0000000..210af63
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/tmp108.txt
@@ -0,0 +1,27 @@
+TMP108 temperature sensor
+-------------------------
+
+This device supports I2C only.
+
+Requires node properties:
+- compatible : "ti,tmp108"
+- reg : the I2C address of the device. This is 0x48, 0x49, 0x4a, or 0x4b.
+
+Optional node properties:
+- ti,thermostat-mode-comparitor : (boolean) select the comparitor mode for the
+      thermostat rather than the default interrupt-mode.
+- ti,alert-active-high : (boolean) make the alert pin active-high instead of
+      the default active-low.
+- ti,conversion-rate-cHz : (integer, cHz) select the conversion frequency for
+      continuous mode, in centi-Hz: 25, 100 (default), 400, or 1600.
+- ti,hysteresis : (integer, C) select the hysteresis value: 0, 1, 2, or 4
+      (celcius).
+
+Example:
+	tmp108@48 {
+		compatible = "ti,tmp108";
+		reg = <0x48>;
+		ti,alert-active-high;
+		ti,hysteresis = <2>;
+		ti,conversion-rate-cHz = <25>;
+	};
diff --git a/Documentation/hwmon/tmp108 b/Documentation/hwmon/tmp108
new file mode 100644
index 0000000..ef2e9a3
--- /dev/null
+++ b/Documentation/hwmon/tmp108
@@ -0,0 +1,38 @@
+Kernel driver tmp108
+====================
+
+Supported chips:
+  * Texas Instruments TMP108
+    Prefix: 'tmp108'
+    Addresses scanned: none
+    Datasheet: http://www.ti.com/product/tmp108
+
+Author:
+	John Muir <john@jmuir.com>
+
+Description
+-----------
+
+The Texas Instruments TMP108 implements one temperature sensor. An alert pin
+can be set when temperatures exceed minimum or maximum values plus or minus a
+hysteresis value.
+
+The sensor is accurate to 0.75C over the range of -25 to +85 C, and to 1.0
+degree from -40 to +125 C. Resolution of the sensor is 0.0625 degree. The
+operating temperature has a minimum of -55 C and a maximum of +150 C.
+Hysteresis values can be set to 0, 1, 2, or 4C.
+
+The TMP108 has a programmable update rate that can select between 8, 4, 1, and
+0.5 Hz.
+
+By default the TMP108 reads the temperature continuously. To conserve power,
+the TMP108 has a one-shot mode where the device is normally shut-down. When a
+one shot is requested the temperature is read, the result can be retrieved,
+and then the device is shut down automatically. (This driver only supports
+continuous mode.)
+
+The driver provides the common sysfs-interface for temperatures (see
+Documentation/hwmon/sysfs-interface under Temperatures).
+
+See Documentation/devicetree/bindings/hwmon/tmp108.txt for configuration
+properties.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 45cef3d..4c173de 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1591,6 +1591,17 @@ config SENSORS_TMP103
 	  This driver can also be built as a module.  If so, the module
 	  will be called tmp103.
 
+config SENSORS_TMP108
+	tristate "Texas Instruments TMP108"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for Texas Instruments TMP108
+	  sensor chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called tmp108.
+
 config SENSORS_TMP401
 	tristate "Texas Instruments TMP401 and compatibles"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index aecf4ba..51e5256 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -152,6 +152,7 @@ obj-$(CONFIG_SENSORS_TC74)	+= tc74.o
 obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
 obj-$(CONFIG_SENSORS_TMP102)	+= tmp102.o
 obj-$(CONFIG_SENSORS_TMP103)	+= tmp103.o
+obj-$(CONFIG_SENSORS_TMP108)	+= tmp108.o
 obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
 obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
 obj-$(CONFIG_SENSORS_TWL4030_MADC)+= twl4030-madc-hwmon.o
diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
new file mode 100644
index 0000000..da64517
--- /dev/null
+++ b/drivers/hwmon/tmp108.c
@@ -0,0 +1,429 @@
+/* Texas Instruments TMP108 SMBus temperature sensor driver
+ *
+ * Copyright (C) 2016 John Muir <john@jmuir.com>
+ *
+ * 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/delay.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/device.h>
+#include <linux/jiffies.h>
+#include <linux/regmap.h>
+#include <linux/thermal.h>
+#include <linux/of.h>
+
+#define	DRIVER_NAME "tmp108"
+
+#define	TMP108_REG_TEMP		0x00
+#define	TMP108_REG_CONF		0x01
+#define	TMP108_REG_TLOW		0x02
+#define	TMP108_REG_THIGH	0x03
+
+#define TMP108_TEMP_REG_COUNT	3
+
+#define TMP108_TEMP_MIN_MC	-50000 /* Minimum millicelcius. */
+#define TMP108_TEMP_MAX_MC	127937 /* Maximum millicelcius. */
+
+/* Configuration register bits.
+ * Note: these bit definitions are byte swapped.
+ */
+#define TMP108_CONF_M0		0x0100 /* Sensor mode. */
+#define TMP108_CONF_M1		0x0200
+#define TMP108_CONF_TM		0x0400 /* Thermostat mode. */
+#define TMP108_CONF_FL		0x0800 /* Watchdog flag - TLOW */
+#define TMP108_CONF_FH		0x1000 /* Watchdog flag - THIGH */
+#define TMP108_CONF_CR0		0x2000 /* Conversion rate. */
+#define TMP108_CONF_CR1		0x4000
+#define TMP108_CONF_ID		0x8000
+#define TMP108_CONF_HYS0	0x0010 /* Hysteresis. */
+#define TMP108_CONF_HYS1	0x0020
+#define TMP108_CONF_POL		0x0080 /* Polarity of alert. */
+
+/* Defaults set by the hardware upon reset. */
+#define TMP108_CONF_DEFAULTS		(TMP108_CONF_CR0 | TMP108_CONF_TM |\
+					 TMP108_CONF_HYS0 | TMP108_CONF_M1)
+/* These bits are read-only. */
+#define TMP108_CONF_READ_ONLY		(TMP108_CONF_FL | TMP108_CONF_FH |\
+					 TMP108_CONF_ID)
+
+#define TMP108_CONF_MODE_MASK		(TMP108_CONF_M0|TMP108_CONF_M1)
+#define TMP108_MODE_SHUTDOWN		0x0000
+#define TMP108_MODE_ONE_SHOT		TMP108_CONF_M0
+#define TMP108_MODE_CONTINUOUS		TMP108_CONF_M1		/* Default */
+
+#define TMP108_CONF_CONVRATE_MASK	(TMP108_CONF_CR0|TMP108_CONF_CR1)
+#define TMP108_CONVRATE_0P25HZ		0x0000
+#define TMP108_CONVRATE_1HZ		TMP108_CONF_CR0		/* Default */
+#define TMP108_CONVRATE_4HZ		TMP108_CONF_CR1
+#define TMP108_CONVRATE_16HZ		(TMP108_CONF_CR0|TMP108_CONF_CR1)
+
+#define TMP108_CONF_HYSTERESIS_MASK	(TMP108_CONF_HYS0|TMP108_CONF_HYS1)
+#define TMP108_HYSTERESIS_0C		0x0000
+#define TMP108_HYSTERESIS_1C		TMP108_CONF_HYS0	/* Default */
+#define TMP108_HYSTERESIS_2C		TMP108_CONF_HYS1
+#define TMP108_HYSTERESIS_4C		(TMP108_CONF_HYS0|TMP108_CONF_HYS1)
+
+#define TMP108_CONVERSION_TIME_MS	30	/* in milli-seconds */
+
+struct tmp108 {
+	struct regmap *regmap;
+	struct device *hwmon_dev;
+	struct thermal_zone_device *tz;
+	u16 config;
+	unsigned long ready_time;
+};
+
+static const u8 tmp108_temp_reg[TMP108_TEMP_REG_COUNT] = {
+	TMP108_REG_TEMP,
+	TMP108_REG_TLOW,
+	TMP108_REG_THIGH,
+};
+
+/* convert 12-bit TMP108 register value to milliCelsius */
+static inline int tmp108_temp_reg_to_mC(s16 val)
+{
+	return (val & ~0x01) * 1000 / 256;
+}
+
+/* convert milliCelsius to left adjusted 12-bit TMP108 register value */
+static inline u16 tmp108_mC_to_temp_reg(int val)
+{
+	return (val * 256) / 1000;
+}
+
+static int tmp108_read_reg_temp(struct device *dev, int reg, int *temp)
+{
+	struct tmp108 *tmp108 = dev_get_drvdata(dev);
+	unsigned int regval;
+	int err;
+
+	switch (reg) {
+	case TMP108_REG_TEMP:
+		/* Is it too early to return a conversion ? */
+		if (time_before(jiffies, tmp108->ready_time)) {
+			dev_dbg(dev, "%s: Conversion not ready yet..\n",
+				__func__);
+			return -EAGAIN;
+		}
+		break;
+	case TMP108_REG_TLOW:
+	case TMP108_REG_THIGH:
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	err = regmap_read(tmp108->regmap, reg, &regval);
+	if (err < 0)
+		return err;
+	*temp = tmp108_temp_reg_to_mC(regval);
+
+	return 0;
+}
+
+static ssize_t tmp108_show_temp(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
+	int temp;
+	int err;
+
+	if (sda->index >= ARRAY_SIZE(tmp108_temp_reg))
+		return -EINVAL;
+
+	err = tmp108_read_reg_temp(dev, tmp108_temp_reg[sda->index], &temp);
+	if (err)
+		return err;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", temp);
+}
+
+static ssize_t tmp108_set_temp(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
+	struct tmp108 *tmp108 = dev_get_drvdata(dev);
+	long temp;
+	int err;
+
+	if (sda->index >= ARRAY_SIZE(tmp108_temp_reg))
+		return -EINVAL;
+
+	if (kstrtol(buf, 10, &temp) < 0)
+		return -EINVAL;
+
+	temp = clamp_val(temp, TMP108_TEMP_MIN_MC, TMP108_TEMP_MAX_MC);
+	err = regmap_write(tmp108->regmap, tmp108_temp_reg[sda->index],
+			   tmp108_mC_to_temp_reg(temp));
+	if (err)
+		return err;
+	return count;
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, tmp108_show_temp, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, tmp108_show_temp,
+			  tmp108_set_temp, 1);
+static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, tmp108_show_temp,
+			  tmp108_set_temp, 2);
+
+static struct attribute *tmp108_attrs[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(tmp108);
+
+static int tmp108_get_temp(void *dev, int *temp)
+{
+	return tmp108_read_reg_temp(dev, TMP108_REG_TEMP, temp);
+}
+
+static const struct thermal_zone_of_device_ops tmp108_of_thermal_ops = {
+	.get_temp = tmp108_get_temp,
+};
+
+static void tmp108_update_ready_time(struct tmp108 *tmp108)
+{
+	tmp108->ready_time = jiffies;
+	if ((tmp108->config & TMP108_CONF_MODE_MASK)
+	    == TMP108_MODE_CONTINUOUS) {
+		tmp108->ready_time +=
+			msecs_to_jiffies(TMP108_CONVERSION_TIME_MS);
+	}
+}
+
+static void tmp108_restore_config(void *data)
+{
+	struct tmp108 *tmp108 = data;
+
+	regmap_write(tmp108->regmap, TMP108_REG_CONF, tmp108->config);
+	tmp108_update_ready_time(tmp108);
+}
+
+static bool tmp108_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	return reg != TMP108_REG_TEMP;
+}
+
+static bool tmp108_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	return reg == TMP108_REG_TEMP;
+}
+
+static const struct regmap_config tmp108_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = TMP108_REG_THIGH,
+	.writeable_reg = tmp108_is_writeable_reg,
+	.volatile_reg = tmp108_is_volatile_reg,
+	.val_format_endian = REGMAP_ENDIAN_BIG,
+	.cache_type = REGCACHE_RBTREE,
+	.use_single_rw = true,
+};
+
+static int tmp108_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct device *hwmon_dev;
+	struct tmp108 *tmp108;
+	unsigned int regval;
+	int err;
+	u16 config;
+	u32 convrate = 100;
+	u32 hysteresis = 1;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_WORD_DATA)) {
+		dev_err(dev,
+			"adapter doesn't support SMBus word transactions\n");
+		return -ENODEV;
+	}
+
+	tmp108 = devm_kzalloc(dev, sizeof(*tmp108), GFP_KERNEL);
+	if (!tmp108)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, tmp108);
+
+	tmp108->regmap = devm_regmap_init_i2c(client, &tmp108_regmap_config);
+	if (IS_ERR(tmp108->regmap))
+		return PTR_ERR(tmp108->regmap);
+
+	err = regmap_read(tmp108->regmap, TMP108_REG_CONF, &regval);
+	if (err < 0) {
+		dev_err(dev, "error reading config register\n");
+		return err;
+	}
+	tmp108->config = regval;
+	config = regval;
+
+	/* At this time, only continuous mode is supported. */
+	config &= ~TMP108_CONF_MODE_MASK;
+	config |= TMP108_MODE_CONTINUOUS;
+
+	if (device_property_read_bool(dev, "ti,thermostat-mode-comparitor"))
+		config &= ~TMP108_CONF_TM;
+	else
+		config |= TMP108_CONF_TM;
+
+	if (device_property_read_bool(dev, "ti,alert-active-high"))
+		config |= TMP108_CONF_POL;
+	else
+		config &= ~TMP108_CONF_POL;
+
+	if (device_property_read_u32(dev, "ti,conversion-rate-cHz", &convrate)
+	    >= 0) {
+		config &= ~TMP108_CONF_CONVRATE_MASK;
+		switch (convrate) {
+		case 25:
+			config |= TMP108_CONVRATE_0P25HZ;
+			break;
+		case 100:
+			config |= TMP108_CONVRATE_1HZ;
+			break;
+		case 400:
+			config |= TMP108_CONVRATE_4HZ;
+			break;
+		case 1600:
+			config |= TMP108_CONVRATE_16HZ;
+			break;
+		default:
+			dev_err(dev, "conversion rate %u invalid: defaulting to 1Hz.\n",
+				convrate);
+			convrate = 100;
+			config |= TMP108_CONVRATE_1HZ;
+			break;
+		}
+	}
+
+	if (device_property_read_u32(dev, "ti,hysteresis", &hysteresis) >= 0) {
+		config &= ~TMP108_CONF_HYSTERESIS_MASK;
+		switch (hysteresis) {
+		case 0:
+			config |= TMP108_HYSTERESIS_0C;
+			break;
+		case 1:
+			config |= TMP108_HYSTERESIS_1C;
+			break;
+		case 2:
+			config |= TMP108_HYSTERESIS_2C;
+			break;
+		case 4:
+			config |= TMP108_HYSTERESIS_4C;
+			break;
+		default:
+			dev_err(dev, "hysteresis value %u invalid: defaulting to 1C.\n",
+				hysteresis);
+			hysteresis = 1;
+			config |= TMP108_HYSTERESIS_1C;
+			break;
+		}
+	}
+
+	err = regmap_write(tmp108->regmap, TMP108_REG_CONF, config);
+	if (err < 0) {
+		dev_err(dev, "error writing config register\n");
+		return err;
+	}
+
+	tmp108->config = config;
+	tmp108_update_ready_time(tmp108);
+
+	err = devm_add_action_or_reset(dev, tmp108_restore_config, tmp108);
+	if (err)
+		return err;
+
+	hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
+						      tmp108, tmp108_groups);
+	if (IS_ERR(hwmon_dev)) {
+		dev_dbg(dev, "unable to register hwmon device\n");
+		return PTR_ERR(hwmon_dev);
+	}
+
+	tmp108->hwmon_dev = hwmon_dev;
+	tmp108->tz = thermal_zone_of_sensor_register(hwmon_dev, 0, hwmon_dev,
+						     &tmp108_of_thermal_ops);
+	if (IS_ERR(tmp108->tz))
+		return PTR_ERR(tmp108->tz);
+
+	dev_info(dev, "%s, alert: active %s, hyst: %uC, conv: %ucHz\n",
+		 (tmp108->config & TMP108_CONF_TM) != 0 ?
+			"interrupt" : "comparator",
+		 (tmp108->config & TMP108_CONF_POL) != 0 ? "high" : "low",
+		 hysteresis, convrate);
+	return 0;
+}
+
+static int tmp108_remove(struct i2c_client *client)
+{
+	struct tmp108 *tmp108 = i2c_get_clientdata(client);
+
+	thermal_zone_of_sensor_unregister(tmp108->hwmon_dev, tmp108->tz);
+	hwmon_device_unregister(tmp108->hwmon_dev);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int tmp108_suspend(struct device *dev)
+{
+	struct tmp108 *tmp108 = dev_get_drvdata(dev);
+
+	return regmap_update_bits(tmp108->regmap, TMP108_REG_CONF,
+				  TMP108_CONF_MODE_MASK, TMP108_MODE_SHUTDOWN);
+}
+
+static int tmp108_resume(struct device *dev)
+{
+	struct tmp108 *tmp108 = dev_get_drvdata(dev);
+	int err;
+
+	err = regmap_write(tmp108->regmap, TMP108_REG_CONF, tmp108->config);
+
+	tmp108_update_ready_time(tmp108);
+
+	return err;
+}
+#endif /* CONFIG_PM */
+
+static SIMPLE_DEV_PM_OPS(tmp108_dev_pm_ops, tmp108_suspend, tmp108_resume);
+
+static const struct i2c_device_id tmp108_id[] = {
+	{ "tmp108", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tmp108_id);
+
+static struct i2c_driver tmp108_driver = {
+	.driver.name	= DRIVER_NAME,
+	.driver.pm	= &tmp108_dev_pm_ops,
+	.probe		= tmp108_probe,
+	.remove		= tmp108_remove,
+	.id_table	= tmp108_id,
+};
+
+module_i2c_driver(tmp108_driver);
+
+MODULE_AUTHOR("John Muir <john@jmuir.com>");
+MODULE_DESCRIPTION("Texas Instruments TMP108 temperature sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.8.0.rc3.226.g39d4020

[PATCH 2/3] hwmon: tmp108: Use devm variants of registration interfaces.

[John Muir]

From: John Muir <muirj@google.com>

Use the devm hwmon and thermal zone registration functions to
clean up the code and remove the need for an i2c_driver.remove
callback.

Signed-off-by: John Muir <john@jmuir.com>
---
 drivers/hwmon/tmp108.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
index da64517..b13d652 100644
--- a/drivers/hwmon/tmp108.c
+++ b/drivers/hwmon/tmp108.c
@@ -83,8 +83,6 @@
 
 struct tmp108 {
 	struct regmap *regmap;
-	struct device *hwmon_dev;
-	struct thermal_zone_device *tz;
 	u16 config;
 	unsigned long ready_time;
 };
@@ -245,6 +243,7 @@ static int tmp108_probe(struct i2c_client *client,
 {
 	struct device *dev = &client->dev;
 	struct device *hwmon_dev;
+	struct thermal_zone_device *tz;
 	struct tmp108 *tmp108;
 	unsigned int regval;
 	int err;
@@ -353,18 +352,18 @@ static int tmp108_probe(struct i2c_client *client,
 	if (err)
 		return err;
 
-	hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
-						      tmp108, tmp108_groups);
+	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
+							   tmp108,
+							   tmp108_groups);
 	if (IS_ERR(hwmon_dev)) {
 		dev_dbg(dev, "unable to register hwmon device\n");
 		return PTR_ERR(hwmon_dev);
 	}
 
-	tmp108->hwmon_dev = hwmon_dev;
-	tmp108->tz = thermal_zone_of_sensor_register(hwmon_dev, 0, hwmon_dev,
-						     &tmp108_of_thermal_ops);
-	if (IS_ERR(tmp108->tz))
-		return PTR_ERR(tmp108->tz);
+	tz = devm_thermal_zone_of_sensor_register(hwmon_dev, 0, hwmon_dev,
+						  &tmp108_of_thermal_ops);
+	if (IS_ERR(tz))
+		return PTR_ERR(tz);
 
 	dev_info(dev, "%s, alert: active %s, hyst: %uC, conv: %ucHz\n",
 		 (tmp108->config & TMP108_CONF_TM) != 0 ?
@@ -374,16 +373,6 @@ static int tmp108_probe(struct i2c_client *client,
 	return 0;
 }
 
-static int tmp108_remove(struct i2c_client *client)
-{
-	struct tmp108 *tmp108 = i2c_get_clientdata(client);
-
-	thermal_zone_of_sensor_unregister(tmp108->hwmon_dev, tmp108->tz);
-	hwmon_device_unregister(tmp108->hwmon_dev);
-
-	return 0;
-}
-
 #ifdef CONFIG_PM_SLEEP
 static int tmp108_suspend(struct device *dev)
 {
@@ -418,7 +407,6 @@ static struct i2c_driver tmp108_driver = {
 	.driver.name	= DRIVER_NAME,
 	.driver.pm	= &tmp108_dev_pm_ops,
 	.probe		= tmp108_probe,
-	.remove		= tmp108_remove,
 	.id_table	= tmp108_id,
 };
 
-- 
2.8.0.rc3.226.g39d4020

[PATCH 3/3] hwmon: tmp108: Update driver to use hwmon_chip_info.

[John Muir]

Move the tmp108 driver from hwmon attribute groups to
hwmon_chip_info.

Signed-off-by: John Muir <john@jmuir.com>
---
 drivers/hwmon/tmp108.c | 142 +++++++++++++++++++++++++------------------------
 1 file changed, 73 insertions(+), 69 deletions(-)

diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
index b13d652..29ddc2e 100644
--- a/drivers/hwmon/tmp108.c
+++ b/drivers/hwmon/tmp108.c
@@ -25,7 +25,6 @@
 #include <linux/device.h>
 #include <linux/jiffies.h>
 #include <linux/regmap.h>
-#include <linux/thermal.h>
 #include <linux/of.h>
 
 #define	DRIVER_NAME "tmp108"
@@ -87,12 +86,6 @@ struct tmp108 {
 	unsigned long ready_time;
 };
 
-static const u8 tmp108_temp_reg[TMP108_TEMP_REG_COUNT] = {
-	TMP108_REG_TEMP,
-	TMP108_REG_TLOW,
-	TMP108_REG_THIGH,
-};
-
 /* convert 12-bit TMP108 register value to milliCelsius */
 static inline int tmp108_temp_reg_to_mC(s16 val)
 {
@@ -105,23 +98,28 @@ static inline u16 tmp108_mC_to_temp_reg(int val)
 	return (val * 256) / 1000;
 }
 
-static int tmp108_read_reg_temp(struct device *dev, int reg, int *temp)
+static int tmp108_read(struct device *dev, enum hwmon_sensor_types type,
+		       u32 attr, int channel, long *temp)
 {
 	struct tmp108 *tmp108 = dev_get_drvdata(dev);
 	unsigned int regval;
-	int err;
+	int err, reg;
 
-	switch (reg) {
-	case TMP108_REG_TEMP:
+	switch (attr) {
+	case hwmon_temp_input:
 		/* Is it too early to return a conversion ? */
 		if (time_before(jiffies, tmp108->ready_time)) {
 			dev_dbg(dev, "%s: Conversion not ready yet..\n",
 				__func__);
 			return -EAGAIN;
 		}
+		reg = TMP108_REG_TEMP;
+		break;
+	case hwmon_temp_min:
+		reg = TMP108_REG_TLOW;
 		break;
-	case TMP108_REG_TLOW:
-	case TMP108_REG_THIGH:
+	case hwmon_temp_max:
+		reg = TMP108_REG_THIGH;
 		break;
 	default:
 		return -EOPNOTSUPP;
@@ -135,68 +133,79 @@ static int tmp108_read_reg_temp(struct device *dev, int reg, int *temp)
 	return 0;
 }
 
-static ssize_t tmp108_show_temp(struct device *dev,
-				struct device_attribute *attr,
-				char *buf)
+static int tmp108_write(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long temp)
 {
-	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
-	int temp;
-	int err;
-
-	if (sda->index >= ARRAY_SIZE(tmp108_temp_reg))
-		return -EINVAL;
+	struct tmp108 *tmp108 = dev_get_drvdata(dev);
+	int reg;
 
-	err = tmp108_read_reg_temp(dev, tmp108_temp_reg[sda->index], &temp);
-	if (err)
-		return err;
+	switch (attr) {
+	case hwmon_temp_min:
+		reg = TMP108_REG_TLOW;
+		break;
+	case hwmon_temp_max:
+		reg = TMP108_REG_THIGH;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
 
-	return snprintf(buf, PAGE_SIZE, "%d\n", temp);
+	temp = clamp_val(temp, TMP108_TEMP_MIN_MC, TMP108_TEMP_MAX_MC);
+	return regmap_write(tmp108->regmap, reg, tmp108_mC_to_temp_reg(temp));
 }
 
-static ssize_t tmp108_set_temp(struct device *dev,
-			       struct device_attribute *attr,
-			       const char *buf, size_t count)
+static umode_t tmp108_is_visible(const void *data, enum hwmon_sensor_types type,
+				 u32 attr, int channel)
 {
-	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
-	struct tmp108 *tmp108 = dev_get_drvdata(dev);
-	long temp;
-	int err;
+	if (type != hwmon_temp)
+		return 0;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		return S_IRUGO;
+	case hwmon_temp_min:
+	case hwmon_temp_max:
+		return S_IRUGO | S_IWUSR;
+	default:
+		return 0;
+	}
+}
 
-	if (sda->index >= ARRAY_SIZE(tmp108_temp_reg))
-		return -EINVAL;
+static u32 tmp108_chip_config[] = {
+	HWMON_C_REGISTER_TZ,
+	0
+};
 
-	if (kstrtol(buf, 10, &temp) < 0)
-		return -EINVAL;
+static const struct hwmon_channel_info tmp108_chip = {
+	.type = hwmon_chip,
+	.config = tmp108_chip_config,
+};
 
-	temp = clamp_val(temp, TMP108_TEMP_MIN_MC, TMP108_TEMP_MAX_MC);
-	err = regmap_write(tmp108->regmap, tmp108_temp_reg[sda->index],
-			   tmp108_mC_to_temp_reg(temp));
-	if (err)
-		return err;
-	return count;
-}
+static u32 tmp108_temp_config[] = {
+	HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MIN,
+	0
+};
 
-static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, tmp108_show_temp, NULL, 0);
-static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, tmp108_show_temp,
-			  tmp108_set_temp, 1);
-static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, tmp108_show_temp,
-			  tmp108_set_temp, 2);
+static const struct hwmon_channel_info tmp108_temp = {
+	.type = hwmon_temp,
+	.config = tmp108_temp_config,
+};
 
-static struct attribute *tmp108_attrs[] = {
-	&sensor_dev_attr_temp1_input.dev_attr.attr,
-	&sensor_dev_attr_temp1_min.dev_attr.attr,
-	&sensor_dev_attr_temp1_max.dev_attr.attr,
+static const struct hwmon_channel_info *tmp108_info[] = {
+	&tmp108_chip,
+	&tmp108_temp,
 	NULL
 };
-ATTRIBUTE_GROUPS(tmp108);
 
-static int tmp108_get_temp(void *dev, int *temp)
-{
-	return tmp108_read_reg_temp(dev, TMP108_REG_TEMP, temp);
-}
+static const struct hwmon_ops tmp108_hwmon_ops = {
+	.is_visible = tmp108_is_visible,
+	.read = tmp108_read,
+	.write = tmp108_write,
+};
 
-static const struct thermal_zone_of_device_ops tmp108_of_thermal_ops = {
-	.get_temp = tmp108_get_temp,
+static const struct hwmon_chip_info tmp108_chip_info = {
+	.ops = &tmp108_hwmon_ops,
+	.info = tmp108_info,
 };
 
 static void tmp108_update_ready_time(struct tmp108 *tmp108)
@@ -243,7 +252,6 @@ static int tmp108_probe(struct i2c_client *client,
 {
 	struct device *dev = &client->dev;
 	struct device *hwmon_dev;
-	struct thermal_zone_device *tz;
 	struct tmp108 *tmp108;
 	unsigned int regval;
 	int err;
@@ -352,19 +360,15 @@ static int tmp108_probe(struct i2c_client *client,
 	if (err)
 		return err;
 
-	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
-							   tmp108,
-							   tmp108_groups);
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+							 tmp108,
+							 &tmp108_chip_info,
+							 NULL);
 	if (IS_ERR(hwmon_dev)) {
 		dev_dbg(dev, "unable to register hwmon device\n");
 		return PTR_ERR(hwmon_dev);
 	}
 
-	tz = devm_thermal_zone_of_sensor_register(hwmon_dev, 0, hwmon_dev,
-						  &tmp108_of_thermal_ops);
-	if (IS_ERR(tz))
-		return PTR_ERR(tz);
-
 	dev_info(dev, "%s, alert: active %s, hyst: %uC, conv: %ucHz\n",
 		 (tmp108->config & TMP108_CONF_TM) != 0 ?
 			"interrupt" : "comparator",
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 1/3] hwmon: Add Texas Instruments TMP108 temperature sensor driver.

[Guenter Roeck]

On 11/26/2016 09:15 PM, John Muir wrote:
> Add support for the TI TMP108 temperature sensor with some device
> configuration parameters.
>
> Signed-off-by: John Muir <john@jmuir.com>
> ---
>  Documentation/devicetree/bindings/hwmon/tmp108.txt |  27 ++
>  Documentation/hwmon/tmp108                         |  38 ++
>  drivers/hwmon/Kconfig                              |  11 +
>  drivers/hwmon/Makefile                             |   1 +
>  drivers/hwmon/tmp108.c                             | 429 +++++++++++++++++++++
>  5 files changed, 506 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/tmp108.txt
>  create mode 100644 Documentation/hwmon/tmp108
>  create mode 100644 drivers/hwmon/tmp108.c
>
> diff --git a/Documentation/devicetree/bindings/hwmon/tmp108.txt b/Documentation/devicetree/bindings/hwmon/tmp108.txt

This should be provided in a separate patch.

> new file mode 100644
> index 0000000..210af63
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/tmp108.txt
> @@ -0,0 +1,27 @@
> +TMP108 temperature sensor
> +-------------------------
> +
> +This device supports I2C only.
> +
> +Requires node properties:
> +- compatible : "ti,tmp108"
> +- reg : the I2C address of the device. This is 0x48, 0x49, 0x4a, or 0x4b.
> +
> +Optional node properties:
> +- ti,thermostat-mode-comparitor : (boolean) select the comparitor mode for the

s/comparitor/comparator/g

> +      thermostat rather than the default interrupt-mode.
> +- ti,alert-active-high : (boolean) make the alert pin active-high instead of
> +      the default active-low.

The driver doesn't support interrupts/alerts. Do those properties really add value ?

> +- ti,conversion-rate-cHz : (integer, cHz) select the conversion frequency for
> +      continuous mode, in centi-Hz: 25, 100 (default), 400, or 1600.
> +- ti,hysteresis : (integer, C) select the hysteresis value: 0, 1, 2, or 4
> +      (celcius).
> +
We would nor mally make the conversion rate and hysteresis user configurable,
with the update_interval and temp1_{min,max}_hyst attributes. Those are not really
system configuration properties.

> +Example:
> +	tmp108@48 {
> +		compatible = "ti,tmp108";
> +		reg = <0x48>;
> +		ti,alert-active-high;
> +		ti,hysteresis = <2>;
> +		ti,conversion-rate-cHz = <25>;
> +	};
> diff --git a/Documentation/hwmon/tmp108 b/Documentation/hwmon/tmp108
> new file mode 100644
> index 0000000..ef2e9a3
> --- /dev/null
> +++ b/Documentation/hwmon/tmp108
> @@ -0,0 +1,38 @@
> +Kernel driver tmp108
> +====================
> +
> +Supported chips:
> +  * Texas Instruments TMP108
> +    Prefix: 'tmp108'
> +    Addresses scanned: none
> +    Datasheet: http://www.ti.com/product/tmp108
> +
> +Author:
> +	John Muir <john@jmuir.com>
> +
> +Description
> +-----------
> +
> +The Texas Instruments TMP108 implements one temperature sensor. An alert pin
> +can be set when temperatures exceed minimum or maximum values plus or minus a
> +hysteresis value.
> +
> +The sensor is accurate to 0.75C over the range of -25 to +85 C, and to 1.0
> +degree from -40 to +125 C. Resolution of the sensor is 0.0625 degree. The
> +operating temperature has a minimum of -55 C and a maximum of +150 C.
> +Hysteresis values can be set to 0, 1, 2, or 4C.
> +
> +The TMP108 has a programmable update rate that can select between 8, 4, 1, and
> +0.5 Hz.
> +
> +By default the TMP108 reads the temperature continuously. To conserve power,
> +the TMP108 has a one-shot mode where the device is normally shut-down. When a
> +one shot is requested the temperature is read, the result can be retrieved,
> +and then the device is shut down automatically. (This driver only supports
> +continuous mode.)
> +
> +The driver provides the common sysfs-interface for temperatures (see
> +Documentation/hwmon/sysfs-interface under Temperatures).
> +
> +See Documentation/devicetree/bindings/hwmon/tmp108.txt for configuration
> +properties.
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 45cef3d..4c173de 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1591,6 +1591,17 @@ config SENSORS_TMP103
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called tmp103.
>
> +config SENSORS_TMP108
> +	tristate "Texas Instruments TMP108"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  If you say yes here you get support for Texas Instruments TMP108
> +	  sensor chips.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called tmp108.
> +
>  config SENSORS_TMP401
>  	tristate "Texas Instruments TMP401 and compatibles"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index aecf4ba..51e5256 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -152,6 +152,7 @@ obj-$(CONFIG_SENSORS_TC74)	+= tc74.o
>  obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
>  obj-$(CONFIG_SENSORS_TMP102)	+= tmp102.o
>  obj-$(CONFIG_SENSORS_TMP103)	+= tmp103.o
> +obj-$(CONFIG_SENSORS_TMP108)	+= tmp108.o
>  obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
>  obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
>  obj-$(CONFIG_SENSORS_TWL4030_MADC)+= twl4030-madc-hwmon.o
> diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
> new file mode 100644
> index 0000000..da64517
> --- /dev/null
> +++ b/drivers/hwmon/tmp108.c
> @@ -0,0 +1,429 @@
> +/* Texas Instruments TMP108 SMBus temperature sensor driver
> + *
> + * Copyright (C) 2016 John Muir <john@jmuir.com>
> + *
> + * 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/delay.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/device.h>
> +#include <linux/jiffies.h>
> +#include <linux/regmap.h>
> +#include <linux/thermal.h>
> +#include <linux/of.h>
> +
Alphabetic order, please.

> +#define	DRIVER_NAME "tmp108"
> +
> +#define	TMP108_REG_TEMP		0x00
> +#define	TMP108_REG_CONF		0x01
> +#define	TMP108_REG_TLOW		0x02
> +#define	TMP108_REG_THIGH	0x03
> +
> +#define TMP108_TEMP_REG_COUNT	3
> +
> +#define TMP108_TEMP_MIN_MC	-50000 /* Minimum millicelcius. */
> +#define TMP108_TEMP_MAX_MC	127937 /* Maximum millicelcius. */
> +
> +/* Configuration register bits.
> + * Note: these bit definitions are byte swapped.
> + */
> +#define TMP108_CONF_M0		0x0100 /* Sensor mode. */
> +#define TMP108_CONF_M1		0x0200
> +#define TMP108_CONF_TM		0x0400 /* Thermostat mode. */
> +#define TMP108_CONF_FL		0x0800 /* Watchdog flag - TLOW */
> +#define TMP108_CONF_FH		0x1000 /* Watchdog flag - THIGH */
> +#define TMP108_CONF_CR0		0x2000 /* Conversion rate. */
> +#define TMP108_CONF_CR1		0x4000
> +#define TMP108_CONF_ID		0x8000
> +#define TMP108_CONF_HYS0	0x0010 /* Hysteresis. */
> +#define TMP108_CONF_HYS1	0x0020
> +#define TMP108_CONF_POL		0x0080 /* Polarity of alert. */
> +
> +/* Defaults set by the hardware upon reset. */
> +#define TMP108_CONF_DEFAULTS		(TMP108_CONF_CR0 | TMP108_CONF_TM |\
> +					 TMP108_CONF_HYS0 | TMP108_CONF_M1)
> +/* These bits are read-only. */
> +#define TMP108_CONF_READ_ONLY		(TMP108_CONF_FL | TMP108_CONF_FH |\
> +					 TMP108_CONF_ID)
> +
> +#define TMP108_CONF_MODE_MASK		(TMP108_CONF_M0|TMP108_CONF_M1)
> +#define TMP108_MODE_SHUTDOWN		0x0000
> +#define TMP108_MODE_ONE_SHOT		TMP108_CONF_M0
> +#define TMP108_MODE_CONTINUOUS		TMP108_CONF_M1		/* Default */
> +
> +#define TMP108_CONF_CONVRATE_MASK	(TMP108_CONF_CR0|TMP108_CONF_CR1)
> +#define TMP108_CONVRATE_0P25HZ		0x0000
> +#define TMP108_CONVRATE_1HZ		TMP108_CONF_CR0		/* Default */
> +#define TMP108_CONVRATE_4HZ		TMP108_CONF_CR1
> +#define TMP108_CONVRATE_16HZ		(TMP108_CONF_CR0|TMP108_CONF_CR1)
> +
> +#define TMP108_CONF_HYSTERESIS_MASK	(TMP108_CONF_HYS0|TMP108_CONF_HYS1)
> +#define TMP108_HYSTERESIS_0C		0x0000
> +#define TMP108_HYSTERESIS_1C		TMP108_CONF_HYS0	/* Default */
> +#define TMP108_HYSTERESIS_2C		TMP108_CONF_HYS1
> +#define TMP108_HYSTERESIS_4C		(TMP108_CONF_HYS0|TMP108_CONF_HYS1)
> +
> +#define TMP108_CONVERSION_TIME_MS	30	/* in milli-seconds */
> +
> +struct tmp108 {
> +	struct regmap *regmap;
> +	struct device *hwmon_dev;
> +	struct thermal_zone_device *tz;
> +	u16 config;
> +	unsigned long ready_time;
> +};
> +
> +static const u8 tmp108_temp_reg[TMP108_TEMP_REG_COUNT] = {
> +	TMP108_REG_TEMP,
> +	TMP108_REG_TLOW,
> +	TMP108_REG_THIGH,
> +};
> +
> +/* convert 12-bit TMP108 register value to milliCelsius */
> +static inline int tmp108_temp_reg_to_mC(s16 val)
> +{
> +	return (val & ~0x01) * 1000 / 256;
> +}
> +
> +/* convert milliCelsius to left adjusted 12-bit TMP108 register value */
> +static inline u16 tmp108_mC_to_temp_reg(int val)
> +{
> +	return (val * 256) / 1000;
> +}
> +
> +static int tmp108_read_reg_temp(struct device *dev, int reg, int *temp)
> +{
> +	struct tmp108 *tmp108 = dev_get_drvdata(dev);
> +	unsigned int regval;
> +	int err;
> +
> +	switch (reg) {
> +	case TMP108_REG_TEMP:
> +		/* Is it too early to return a conversion ? */
> +		if (time_before(jiffies, tmp108->ready_time)) {
> +			dev_dbg(dev, "%s: Conversion not ready yet..\n",
> +				__func__);
> +			return -EAGAIN;
> +		}
> +		break;
> +	case TMP108_REG_TLOW:
> +	case TMP108_REG_THIGH:
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	err = regmap_read(tmp108->regmap, reg, &regval);
> +	if (err < 0)
> +		return err;
> +	*temp = tmp108_temp_reg_to_mC(regval);
> +
> +	return 0;
> +}
> +
> +static ssize_t tmp108_show_temp(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
> +	int temp;
> +	int err;
> +
> +	if (sda->index >= ARRAY_SIZE(tmp108_temp_reg))
> +		return -EINVAL;
> +
Unnecessary. That would be a programming error, which we don't check in the
kernel. You could also provide the register directly in sda->index.

> +	err = tmp108_read_reg_temp(dev, tmp108_temp_reg[sda->index], &temp);
> +	if (err)
> +		return err;
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", temp);
> +}
> +
> +static ssize_t tmp108_set_temp(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
> +	struct tmp108 *tmp108 = dev_get_drvdata(dev);
> +	long temp;
> +	int err;
> +
> +	if (sda->index >= ARRAY_SIZE(tmp108_temp_reg))
> +		return -EINVAL;
> +
> +	if (kstrtol(buf, 10, &temp) < 0)
> +		return -EINVAL;
> +
> +	temp = clamp_val(temp, TMP108_TEMP_MIN_MC, TMP108_TEMP_MAX_MC);
> +	err = regmap_write(tmp108->regmap, tmp108_temp_reg[sda->index],
> +			   tmp108_mC_to_temp_reg(temp));
> +	if (err)
> +		return err;
> +	return count;
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, tmp108_show_temp, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, tmp108_show_temp,
> +			  tmp108_set_temp, 1);
> +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, tmp108_show_temp,
> +			  tmp108_set_temp, 2);
> +

You could also provide temp1_{min,max}_alarm.

> +static struct attribute *tmp108_attrs[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_temp1_min.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max.dev_attr.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(tmp108);
> +
> +static int tmp108_get_temp(void *dev, int *temp)
> +{
> +	return tmp108_read_reg_temp(dev, TMP108_REG_TEMP, temp);
> +}
> +
> +static const struct thermal_zone_of_device_ops tmp108_of_thermal_ops = {
> +	.get_temp = tmp108_get_temp,
> +};
> +
> +static void tmp108_update_ready_time(struct tmp108 *tmp108)
> +{
> +	tmp108->ready_time = jiffies;
> +	if ((tmp108->config & TMP108_CONF_MODE_MASK)
> +	    == TMP108_MODE_CONTINUOUS) {

Don't you want a "!" here ? Presumably the delay is only needed
if the original configuration was not for continuous mode.

> +		tmp108->ready_time +=
> +			msecs_to_jiffies(TMP108_CONVERSION_TIME_MS);
> +	}
> +}
> +
> +static void tmp108_restore_config(void *data)
> +{
> +	struct tmp108 *tmp108 = data;
> +
> +	regmap_write(tmp108->regmap, TMP108_REG_CONF, tmp108->config);
> +	tmp108_update_ready_time(tmp108);

This is called when the driver is unloaded, meaning the call to
tmp108_update_ready_time() does not really add any value.

> +}
> +
> +static bool tmp108_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	return reg != TMP108_REG_TEMP;
> +}
> +
> +static bool tmp108_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	return reg == TMP108_REG_TEMP;
> +}
> +
> +static const struct regmap_config tmp108_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.max_register = TMP108_REG_THIGH,
> +	.writeable_reg = tmp108_is_writeable_reg,
> +	.volatile_reg = tmp108_is_volatile_reg,
> +	.val_format_endian = REGMAP_ENDIAN_BIG,
> +	.cache_type = REGCACHE_RBTREE,
> +	.use_single_rw = true,
> +};
> +
> +static int tmp108_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct device *hwmon_dev;
> +	struct tmp108 *tmp108;
> +	unsigned int regval;
> +	int err;
> +	u16 config;
> +	u32 convrate = 100;
> +	u32 hysteresis = 1;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_WORD_DATA)) {
> +		dev_err(dev,
> +			"adapter doesn't support SMBus word transactions\n");
> +		return -ENODEV;
> +	}
> +
> +	tmp108 = devm_kzalloc(dev, sizeof(*tmp108), GFP_KERNEL);
> +	if (!tmp108)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, tmp108);
> +
> +	tmp108->regmap = devm_regmap_init_i2c(client, &tmp108_regmap_config);
> +	if (IS_ERR(tmp108->regmap))
> +		return PTR_ERR(tmp108->regmap);
> +
> +	err = regmap_read(tmp108->regmap, TMP108_REG_CONF, &regval);
> +	if (err < 0) {
> +		dev_err(dev, "error reading config register\n");
> +		return err;
> +	}
> +	tmp108->config = regval;
> +	config = regval;
> +
> +	/* At this time, only continuous mode is supported. */
> +	config &= ~TMP108_CONF_MODE_MASK;
> +	config |= TMP108_MODE_CONTINUOUS;
> +
> +	if (device_property_read_bool(dev, "ti,thermostat-mode-comparitor"))
> +		config &= ~TMP108_CONF_TM;
> +	else
> +		config |= TMP108_CONF_TM;
> +
> +	if (device_property_read_bool(dev, "ti,alert-active-high"))
> +		config |= TMP108_CONF_POL;
> +	else
> +		config &= ~TMP108_CONF_POL;
> +
> +	if (device_property_read_u32(dev, "ti,conversion-rate-cHz", &convrate)
> +	    >= 0) {
> +		config &= ~TMP108_CONF_CONVRATE_MASK;
> +		switch (convrate) {
> +		case 25:
> +			config |= TMP108_CONVRATE_0P25HZ;
> +			break;
> +		case 100:
> +			config |= TMP108_CONVRATE_1HZ;
> +			break;
> +		case 400:
> +			config |= TMP108_CONVRATE_4HZ;
> +			break;
> +		case 1600:
> +			config |= TMP108_CONVRATE_16HZ;
> +			break;
> +		default:
> +			dev_err(dev, "conversion rate %u invalid: defaulting to 1Hz.\n",
> +				convrate);
> +			convrate = 100;
> +			config |= TMP108_CONVRATE_1HZ;
> +			break;
> +		}
> +	}
> +
> +	if (device_property_read_u32(dev, "ti,hysteresis", &hysteresis) >= 0) {
> +		config &= ~TMP108_CONF_HYSTERESIS_MASK;
> +		switch (hysteresis) {
> +		case 0:
> +			config |= TMP108_HYSTERESIS_0C;
> +			break;
> +		case 1:
> +			config |= TMP108_HYSTERESIS_1C;
> +			break;
> +		case 2:
> +			config |= TMP108_HYSTERESIS_2C;
> +			break;
> +		case 4:
> +			config |= TMP108_HYSTERESIS_4C;
> +			break;
> +		default:
> +			dev_err(dev, "hysteresis value %u invalid: defaulting to 1C.\n",
> +				hysteresis);
> +			hysteresis = 1;
> +			config |= TMP108_HYSTERESIS_1C;
> +			break;
> +		}
> +	}
> +
> +	err = regmap_write(tmp108->regmap, TMP108_REG_CONF, config);
> +	if (err < 0) {
> +		dev_err(dev, "error writing config register\n");
> +		return err;
> +	}
> +
> +	tmp108->config = config;
> +	tmp108_update_ready_time(tmp108);
> +
> +	err = devm_add_action_or_reset(dev, tmp108_restore_config, tmp108);
> +	if (err)
> +		return err;
> +
> +	hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
> +						      tmp108, tmp108_groups);

Please consider using the devm_ function here.

> +	if (IS_ERR(hwmon_dev)) {
> +		dev_dbg(dev, "unable to register hwmon device\n");
> +		return PTR_ERR(hwmon_dev);
> +	}
> +
> +	tmp108->hwmon_dev = hwmon_dev;
> +	tmp108->tz = thermal_zone_of_sensor_register(hwmon_dev, 0, hwmon_dev,
> +						     &tmp108_of_thermal_ops);
> +	if (IS_ERR(tmp108->tz))
> +		return PTR_ERR(tmp108->tz);

hwmon device not unregistered here. That would be fixed by using the devm
function above.

> +
> +	dev_info(dev, "%s, alert: active %s, hyst: %uC, conv: %ucHz\n",
> +		 (tmp108->config & TMP108_CONF_TM) != 0 ?
> +			"interrupt" : "comparator",
> +		 (tmp108->config & TMP108_CONF_POL) != 0 ? "high" : "low",
> +		 hysteresis, convrate);
> +	return 0;
> +}
> +
> +static int tmp108_remove(struct i2c_client *client)
> +{
> +	struct tmp108 *tmp108 = i2c_get_clientdata(client);
> +
> +	thermal_zone_of_sensor_unregister(tmp108->hwmon_dev, tmp108->tz);
> +	hwmon_device_unregister(tmp108->hwmon_dev);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int tmp108_suspend(struct device *dev)

I prefer __maybe_unused without #ifdef to ensure that the functions can be compiled
even if unused.

> +{
> +	struct tmp108 *tmp108 = dev_get_drvdata(dev);
> +
> +	return regmap_update_bits(tmp108->regmap, TMP108_REG_CONF,
> +				  TMP108_CONF_MODE_MASK, TMP108_MODE_SHUTDOWN);
> +}
> +
> +static int tmp108_resume(struct device *dev)
> +{
> +	struct tmp108 *tmp108 = dev_get_drvdata(dev);
> +	int err;
> +
> +	err = regmap_write(tmp108->regmap, TMP108_REG_CONF, tmp108->config);
> +
> +	tmp108_update_ready_time(tmp108);
> +
> +	return err;
> +}
> +#endif /* CONFIG_PM */
> +
> +static SIMPLE_DEV_PM_OPS(tmp108_dev_pm_ops, tmp108_suspend, tmp108_resume);
> +
> +static const struct i2c_device_id tmp108_id[] = {
> +	{ "tmp108", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, tmp108_id);
> +
> +static struct i2c_driver tmp108_driver = {
> +	.driver.name	= DRIVER_NAME,
> +	.driver.pm	= &tmp108_dev_pm_ops,
> +	.probe		= tmp108_probe,
> +	.remove		= tmp108_remove,
> +	.id_table	= tmp108_id,
> +};
> +
> +module_i2c_driver(tmp108_driver);
> +
> +MODULE_AUTHOR("John Muir <john@jmuir.com>");
> +MODULE_DESCRIPTION("Texas Instruments TMP108 temperature sensor driver");
> +MODULE_LICENSE("GPL");
>

Re: [PATCH 2/3] hwmon: tmp108: Use devm variants of registration interfaces.

[Guenter Roeck]

On 11/26/2016 09:15 PM, John Muir wrote:
> From: John Muir <muirj@google.com>
>
> Use the devm hwmon and thermal zone registration functions to
> clean up the code and remove the need for an i2c_driver.remove
> callback.
>
> Signed-off-by: John Muir <john@jmuir.com>
> ---
>  drivers/hwmon/tmp108.c | 28 ++++++++--------------------
>  1 file changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
> index da64517..b13d652 100644
> --- a/drivers/hwmon/tmp108.c
> +++ b/drivers/hwmon/tmp108.c
> @@ -83,8 +83,6 @@
>
>  struct tmp108 {
>  	struct regmap *regmap;
> -	struct device *hwmon_dev;
> -	struct thermal_zone_device *tz;
>  	u16 config;
>  	unsigned long ready_time;
>  };
> @@ -245,6 +243,7 @@ static int tmp108_probe(struct i2c_client *client,
>  {
>  	struct device *dev = &client->dev;
>  	struct device *hwmon_dev;
> +	struct thermal_zone_device *tz;
>  	struct tmp108 *tmp108;
>  	unsigned int regval;
>  	int err;
> @@ -353,18 +352,18 @@ static int tmp108_probe(struct i2c_client *client,
>  	if (err)
>  		return err;
>
> -	hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
> -						      tmp108, tmp108_groups);
> +	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
> +							   tmp108,
> +							   tmp108_groups);
>  	if (IS_ERR(hwmon_dev)) {
>  		dev_dbg(dev, "unable to register hwmon device\n");
>  		return PTR_ERR(hwmon_dev);
>  	}
>
> -	tmp108->hwmon_dev = hwmon_dev;
> -	tmp108->tz = thermal_zone_of_sensor_register(hwmon_dev, 0, hwmon_dev,
> -						     &tmp108_of_thermal_ops);
> -	if (IS_ERR(tmp108->tz))
> -		return PTR_ERR(tmp108->tz);
> +	tz = devm_thermal_zone_of_sensor_register(hwmon_dev, 0, hwmon_dev,
> +						  &tmp108_of_thermal_ops);
> +	if (IS_ERR(tz))
> +		return PTR_ERR(tz);
>
>  	dev_info(dev, "%s, alert: active %s, hyst: %uC, conv: %ucHz\n",
>  		 (tmp108->config & TMP108_CONF_TM) != 0 ?
> @@ -374,16 +373,6 @@ static int tmp108_probe(struct i2c_client *client,
>  	return 0;
>  }
>
> -static int tmp108_remove(struct i2c_client *client)
> -{
> -	struct tmp108 *tmp108 = i2c_get_clientdata(client);
> -
With this, i2c_set_clientdata() in the probe function is no longer needed.

> -	thermal_zone_of_sensor_unregister(tmp108->hwmon_dev, tmp108->tz);
> -	hwmon_device_unregister(tmp108->hwmon_dev);
> -
> -	return 0;
> -}
> -
>  #ifdef CONFIG_PM_SLEEP
>  static int tmp108_suspend(struct device *dev)
>  {
> @@ -418,7 +407,6 @@ static struct i2c_driver tmp108_driver = {
>  	.driver.name	= DRIVER_NAME,
>  	.driver.pm	= &tmp108_dev_pm_ops,
>  	.probe		= tmp108_probe,
> -	.remove		= tmp108_remove,
>  	.id_table	= tmp108_id,
>  };
>
>

Re: [PATCH 3/3] hwmon: tmp108: Update driver to use hwmon_chip_info.

[Guenter Roeck]

On Sat, Nov 26, 2016 at 09:15:37PM -0800, John Muir wrote:
> Move the tmp108 driver from hwmon attribute groups to
> hwmon_chip_info.
> 
> Signed-off-by: John Muir <john@jmuir.com>
> ---

Hi John,

please have a look at the following patch.

Something else: Symbolic permissions are out of favor nowadays.
You might instead just use 0644 / 0444. Not that I really care,
but it prevents the inevitable follow-up patches.

Thanks,
Guenter

---
From: Guenter Roeck <linux@roeck-us.net>
Date: Sun, 27 Nov 2016 14:54:19 -0800
Subject: [PATCH] hwmon: (tmp108) Add support for various attributes

Add support for temp1_{min,max}_hyst, temp1_{min,max}_alarm,
and update_interval.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/tmp108.c | 144 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 124 insertions(+), 20 deletions(-)

diff --git a/drivers/hwmon/tmp108.c b/drivers/hwmon/tmp108.c
index 29ddc2e124c6..4cc2adee069a 100644
--- a/drivers/hwmon/tmp108.c
+++ b/drivers/hwmon/tmp108.c
@@ -103,7 +103,33 @@ static int tmp108_read(struct device *dev, enum hwmon_sensor_types type,
 {
 	struct tmp108 *tmp108 = dev_get_drvdata(dev);
 	unsigned int regval;
-	int err, reg;
+	int err, hyst;
+
+	if (type == hwmon_chip) {
+		if (attr == hwmon_chip_update_interval) {
+			err = regmap_read(tmp108->regmap, TMP108_REG_CONF,
+					  &regval);
+			if (err < 0)
+				return err;
+			switch (regval & TMP108_CONF_CONVRATE_MASK) {
+			case TMP108_CONVRATE_0P25HZ:
+			default:
+				*temp = 4000;
+				break;
+			case TMP108_CONVRATE_1HZ:
+				*temp = 1000;
+				break;
+			case TMP108_CONVRATE_4HZ:
+				*temp = 250;
+				break;
+			case TMP108_CONVRATE_16HZ:
+				*temp = 63;
+				break;
+			}
+			return 0;
+		}
+		return -EOPNOTSUPP;
+	}
 
 	switch (attr) {
 	case hwmon_temp_input:
@@ -113,23 +139,57 @@ static int tmp108_read(struct device *dev, enum hwmon_sensor_types type,
 				__func__);
 			return -EAGAIN;
 		}
-		reg = TMP108_REG_TEMP;
+		err = regmap_read(tmp108->regmap, TMP108_REG_TEMP, &regval);
+		if (err < 0)
+			return err;
+		*temp = tmp108_temp_reg_to_mC(regval);
 		break;
 	case hwmon_temp_min:
-		reg = TMP108_REG_TLOW;
-		break;
 	case hwmon_temp_max:
-		reg = TMP108_REG_THIGH;
+		err = regmap_read(tmp108->regmap, attr == hwmon_temp_min ?
+				  TMP108_REG_TLOW : TMP108_REG_THIGH, &regval);
+		if (err < 0)
+			return err;
+		*temp = tmp108_temp_reg_to_mC(regval);
+		break;
+	case hwmon_temp_min_alarm:
+	case hwmon_temp_max_alarm:
+		err = regmap_read(tmp108->regmap, TMP108_REG_CONF, &regval);
+		if (err < 0)
+			return err;
+		*temp = !!(regval & (attr == hwmon_temp_min_alarm ?
+				     TMP108_CONF_FL : TMP108_CONF_FH));
+		break;
+	case hwmon_temp_min_hyst:
+	case hwmon_temp_max_hyst:
+		err = regmap_read(tmp108->regmap, TMP108_REG_CONF, &regval);
+		if (err < 0)
+			return err;
+		switch (regval & TMP108_CONF_HYSTERESIS_MASK) {
+		case TMP108_HYSTERESIS_0C:
+		default:
+			hyst = 0;
+			break;
+		case TMP108_HYSTERESIS_1C:
+			hyst = 1000;
+			break;
+		case TMP108_HYSTERESIS_2C:
+			hyst = 2000;
+			break;
+		case TMP108_HYSTERESIS_4C:
+			hyst = 4000;
+			break;
+		}
+		err = regmap_read(tmp108->regmap, attr == hwmon_temp_min_hyst ?
+				  TMP108_REG_TLOW : TMP108_REG_THIGH, &regval);
+		if (err < 0)
+			return err;
+		*temp = tmp108_temp_reg_to_mC(regval) - hyst;
 		break;
 	default:
 		return -EOPNOTSUPP;
 	}
 
-	err = regmap_read(tmp108->regmap, reg, &regval);
-	if (err < 0)
-		return err;
-	*temp = tmp108_temp_reg_to_mC(regval);
-
 	return 0;
 }
 
@@ -137,33 +197,76 @@ static int tmp108_write(struct device *dev, enum hwmon_sensor_types type,
 			u32 attr, int channel, long temp)
 {
 	struct tmp108 *tmp108 = dev_get_drvdata(dev);
-	int reg;
+	u32 regval, mask;
+	int err;
+
+	if (type == hwmon_chip) {
+		if (attr == hwmon_chip_update_interval) {
+			if (temp < 156)
+				mask = TMP108_CONVRATE_16HZ;
+			else if (temp < 625)
+				mask = TMP108_CONVRATE_4HZ;
+			else if (temp < 2500)
+				mask = TMP108_CONVRATE_1HZ;
+			else
+				mask = TMP108_CONVRATE_0P25HZ;
+
+			return regmap_update_bits(tmp108->regmap,
+						  TMP108_REG_CONF,
+						  TMP108_CONF_CONVRATE_MASK,
+						  mask);
+		}
+		return -EOPNOTSUPP;
+	}
 
 	switch (attr) {
 	case hwmon_temp_min:
-		reg = TMP108_REG_TLOW;
-		break;
 	case hwmon_temp_max:
-		reg = TMP108_REG_THIGH;
-		break;
+		temp = clamp_val(temp, TMP108_TEMP_MIN_MC, TMP108_TEMP_MAX_MC);
+		return regmap_write(tmp108->regmap,
+				    attr == hwmon_temp_min ?
+					TMP108_REG_TLOW : TMP108_REG_THIGH,
+				    tmp108_mC_to_temp_reg(temp));
+	case hwmon_temp_min_hyst:
+		/* limit value range to prevent overflow */
+		temp = clamp_val(temp, -300000, 300000);
+		err = regmap_read(tmp108->regmap, TMP108_REG_TLOW, &regval);
+		if (err < 0)
+			return err;
+		temp = tmp108_temp_reg_to_mC(regval) - temp;
+		if (temp < 500)
+			mask = TMP108_HYSTERESIS_0C;
+		else if (temp < 1500)
+			mask = TMP108_HYSTERESIS_1C;
+		else if (temp < 3000)
+			mask = TMP108_HYSTERESIS_2C;
+		else
+			mask = TMP108_HYSTERESIS_4C;
+		return regmap_update_bits(tmp108->regmap, TMP108_REG_CONF,
+					  TMP108_CONF_HYSTERESIS_MASK,
+					  mask);
 	default:
 		return -EOPNOTSUPP;
 	}
-
-	temp = clamp_val(temp, TMP108_TEMP_MIN_MC, TMP108_TEMP_MAX_MC);
-	return regmap_write(tmp108->regmap, reg, tmp108_mC_to_temp_reg(temp));
 }
 
 static umode_t tmp108_is_visible(const void *data, enum hwmon_sensor_types type,
 				 u32 attr, int channel)
 {
+	if (type == hwmon_chip && attr == hwmon_chip_update_interval)
+		return S_IRUGO | S_IWUSR;
+
 	if (type != hwmon_temp)
 		return 0;
 
 	switch (attr) {
 	case hwmon_temp_input:
+	case hwmon_temp_max_hyst:
+	case hwmon_temp_min_alarm:
+	case hwmon_temp_max_alarm:
 		return S_IRUGO;
 	case hwmon_temp_min:
+	case hwmon_temp_min_hyst:
 	case hwmon_temp_max:
 		return S_IRUGO | S_IWUSR;
 	default:
@@ -172,7 +275,7 @@ static umode_t tmp108_is_visible(const void *data, enum hwmon_sensor_types type,
 }
 
 static u32 tmp108_chip_config[] = {
-	HWMON_C_REGISTER_TZ,
+	HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL,
 	0
 };
 
@@ -182,7 +285,8 @@ static const struct hwmon_channel_info tmp108_chip = {
 };
 
 static u32 tmp108_temp_config[] = {
-	HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MIN,
+	HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_MIN | HWMON_T_MIN_HYST
+	  | HWMON_T_MAX_HYST | HWMON_T_MIN_ALARM | HWMON_T_MAX_ALARM,
 	0
 };
 
-- 
2.5.0

Re: [PATCH 3/3] hwmon: tmp108: Update driver to use hwmon_chip_info.

[John Muir]

> On 2016.11.27, at 15:00 , Guenter Roeck <linux@roeck-us.net> wrote:
> 
> On Sat, Nov 26, 2016 at 09:15:37PM -0800, John Muir wrote:
>> Move the tmp108 driver from hwmon attribute groups to
>> hwmon_chip_info.
>> 
>> Signed-off-by: John Muir <john@jmuir.com>
>> ---
> 
> Hi John,
> 
> please have a look at the following patch.
> 
> Something else: Symbolic permissions are out of favor nowadays.
> You might instead just use 0644 / 0444. Not that I really care,
> but it prevents the inevitable follow-up patches.
> 

Hi Guenter,

Thank you for this patch. I will attempt to implement the same changes using the older interfaces in my 4.4 implementation. I will incorporate your changes and other comments as soon as I can (early this week) and re-send my patch series.

Cheers,

John.

Re: [PATCH 3/3] hwmon: tmp108: Update driver to use hwmon_chip_info.

[Guenter Roeck]

On 11/27/2016 06:10 PM, John Muir wrote:
>> On 2016.11.27, at 15:00 , Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Sat, Nov 26, 2016 at 09:15:37PM -0800, John Muir wrote:
>>> Move the tmp108 driver from hwmon attribute groups to
>>> hwmon_chip_info.
>>>
>>> Signed-off-by: John Muir <john@jmuir.com>
>>> ---
>>
>> Hi John,
>>
>> please have a look at the following patch.
>>
>> Something else: Symbolic permissions are out of favor nowadays.
>> You might instead just use 0644 / 0444. Not that I really care,
>> but it prevents the inevitable follow-up patches.
>>
>
> Hi Guenter,
>
> Thank you for this patch. I will attempt to implement the same changes using the older interfaces in my 4.4 implementation. I will incorporate your changes and other comments as soon as I can (early this week) and re-send my patch series.
>
Hi John,

My pleasure.

I also wrote a module test for the driver (with my patch applied):

https://github.com/groeck/module-tests/blob/master/scripts/tmp108.sh

Just in case you want to give it a try.

Guenter

Re: [PATCH 1/3] hwmon: Add Texas Instruments TMP108 temperature sensor driver.

[John Muir]

> On Nov 27, 2016, at 4:19 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> On 11/26/2016 09:15 PM, John Muir wrote:
>> Add support for the TI TMP108 temperature sensor with some device
>> configuration parameters.
>> +- ti,alert-active-high : (boolean) make the alert pin active-high instead of
>> +      the default active-low.
> 
> The driver doesn't support interrupts/alerts. Do those properties really add value ?
Getting ahead of myself. I will create a patch for this in the future.

>> +static void tmp108_update_ready_time(struct tmp108 *tmp108)
>> +{
>> +	tmp108->ready_time = jiffies;
>> +	if ((tmp108->config & TMP108_CONF_MODE_MASK)
>> +	    == TMP108_MODE_CONTINUOUS) {
> 
> Don't you want a "!" here ? Presumably the delay is only needed
> if the original configuration was not for continuous mode.
> 
>> +		tmp108->ready_time +=
>> +			msecs_to_jiffies(TMP108_CONVERSION_TIME_MS);
>> +	}
>> +}
The delay is required for both really. When the device is set into continuous mode it starts converting and the first temperature is ready (just under) 30ms later.

For the (unsupported at this time) one-shot mode, there would likewise be a delay, but I was envisioning having the requesting task sleep while waiting for the data to be ready, rather than get an -EAGAIN until the data is ready. 

>> +	hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
>> +						      tmp108, tmp108_groups);
> 
> Please consider using the devm_ function here.
> 
>> +	if (IS_ERR(hwmon_dev)) {
>> +		dev_dbg(dev, "unable to register hwmon device\n");
>> +		return PTR_ERR(hwmon_dev);
>> +	}
>> +
>> +	tmp108->hwmon_dev = hwmon_dev;
>> +	tmp108->tz = thermal_zone_of_sensor_register(hwmon_dev, 0, hwmon_dev,
>> +						     &tmp108_of_thermal_ops);
>> +	if (IS_ERR(tmp108->tz))
>> +		return PTR_ERR(tmp108->tz);
> 
> hwmon device not unregistered here. That would be fixed by using the devm
> function above.
> 
For the above two registrations and .remove function, I was worried that there would be an order problem between the i2c->remove and the device-managed cleanup. I’ll get deeper into that code to determine if that is a problem.

For your other comments, I will make the necessary changes.

Thanks!

John.

Re: [PATCH 1/3] hwmon: Add Texas Instruments TMP108 temperature sensor driver.

[Guenter Roeck]

On Mon, Nov 28, 2016 at 11:40:42AM -0800, John Muir wrote:
> 
> > On Nov 27, 2016, at 4:19 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> > On 11/26/2016 09:15 PM, John Muir wrote:
> >> Add support for the TI TMP108 temperature sensor with some device
> >> configuration parameters.
> >> +- ti,alert-active-high : (boolean) make the alert pin active-high instead of
> >> +      the default active-low.
> > 
> > The driver doesn't support interrupts/alerts. Do those properties really add value ?
> Getting ahead of myself. I will create a patch for this in the future.
> 
> >> +static void tmp108_update_ready_time(struct tmp108 *tmp108)
> >> +{
> >> +	tmp108->ready_time = jiffies;
> >> +	if ((tmp108->config & TMP108_CONF_MODE_MASK)
> >> +	    == TMP108_MODE_CONTINUOUS) {
> > 
> > Don't you want a "!" here ? Presumably the delay is only needed
> > if the original configuration was not for continuous mode.
> > 
> >> +		tmp108->ready_time +=
> >> +			msecs_to_jiffies(TMP108_CONVERSION_TIME_MS);
> >> +	}
> >> +}
> The delay is required for both really. When the device is set into continuous mode it starts converting and the first temperature is ready (just under) 30ms later.
> 

The datasheet states, though, that the chip will start converting as soon
as the device powers up. The kernel would have to start really fast after that
to have to wait another 30 ms.

The current code ends up waiting if it doesn't have to (because it waits
if the chip was originally configured for continuous mode), and not waiting
if it has to (because the chip was not configured for continuous mode),
which doesn't seem to be such a good idea.

> For the (unsupported at this time) one-shot mode, there would likewise be a delay, but I was envisioning having the requesting task sleep while waiting for the data to be ready, rather than get an -EAGAIN until the data is ready. 
> 

We had that discussion before; the thermal subsystem doesn't like to be kept
waiting. You would end up adding a lot of complexity for very little gain.
It might make more sense to consider implementing runtime idle support
instead of one-shot mode if power consumption is a concern.

> >> +	hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
> >> +						      tmp108, tmp108_groups);
> > 
> > Please consider using the devm_ function here.
> > 
> >> +	if (IS_ERR(hwmon_dev)) {
> >> +		dev_dbg(dev, "unable to register hwmon device\n");
> >> +		return PTR_ERR(hwmon_dev);
> >> +	}
> >> +
> >> +	tmp108->hwmon_dev = hwmon_dev;
> >> +	tmp108->tz = thermal_zone_of_sensor_register(hwmon_dev, 0, hwmon_dev,
> >> +						     &tmp108_of_thermal_ops);
> >> +	if (IS_ERR(tmp108->tz))
> >> +		return PTR_ERR(tmp108->tz);
> > 
> > hwmon device not unregistered here. That would be fixed by using the devm
> > function above.
> > 
> For the above two registrations and .remove function, I was worried that there would be an order problem between the i2c->remove and the device-managed cleanup. I’ll get deeper into that code to determine if that is a problem.
> 

The thermal sensor registration will be removed first (in the remove function).
devm_ functions are all unregistered / removed after the remove function was
called (in the order of the devm_ call). Otherwise you would have trouble
with devm_kzalloc() as well.

Note that my followup-patch had a problem with the minimum hysteresis
temperature; it needs to be higher than the lower limit, not lower.

Thanks,
Guenter

Re: [PATCH 1/3] hwmon: Add Texas Instruments TMP108 temperature sensor driver.

[John Muir]

> On Nov 28, 2016, at 11:58 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> On Mon, Nov 28, 2016 at 11:40:42AM -0800, John Muir wrote:
>>>> +static void tmp108_update_ready_time(struct tmp108 *tmp108)
>>>> +{
>>>> +	tmp108->ready_time = jiffies;
>>>> +	if ((tmp108->config & TMP108_CONF_MODE_MASK)
>>>> +	    == TMP108_MODE_CONTINUOUS) {
>>> 
>>> Don't you want a "!" here ? Presumably the delay is only needed
>>> if the original configuration was not for continuous mode.
>>> 
>>>> +		tmp108->ready_time +=
>>>> +			msecs_to_jiffies(TMP108_CONVERSION_TIME_MS);
>>>> +	}
>>>> +}
>> The delay is required for both really. When the device is set into continuous mode it starts converting and the first temperature is ready (just under) 30ms later.
>> 
> 
> The datasheet states, though, that the chip will start converting as soon
> as the device powers up. The kernel would have to start really fast after that
> to have to wait another 30 ms.
> 
> The current code ends up waiting if it doesn't have to (because it waits
> if the chip was originally configured for continuous mode), and not waiting
> if it has to (because the chip was not configured for continuous mode),
> which doesn't seem to be such a good idea.

You are referring to the statement under “Conversion Rate” in the data sheet?

"After power-up or a general-call reset, the TMP108 immediately starts a conversion, as shown in Figure 9. The first result is available after 27 ms (typical).”

The datasheet also states that in ’shutdown’ mode, the device is only powering the serial interface (see "Mode Bits”). So, I assumed that the conversions weren’t happening during in that state, and that there would be the delay after moving from the ‘shutdown' state (as is described by the one-shot mode as well where the device state returns to ’shutdown’ when the conversion has taken place).

My intention was that the delay is enforced after PM resume, and for convenience I re-used the same function during probe to initialize the ready_time. I was assuming that the device could be in the shutdown state - for example if the module had previously been unloaded. I agree that this is not required at system startup time. Perhaps I should check to see if the previous configuration had the device in the ‘shutdown’ state, and in that instance apply the delay? In my opinion it doesn’t really matter either way.

FYI, this same logic exists in the TMP102 device driver. The TMP102 device is very similar.

> 
>> For the (unsupported at this time) one-shot mode, there would likewise be a delay, but I was envisioning having the requesting task sleep while waiting for the data to be ready, rather than get an -EAGAIN until the data is ready. 
>> 
> 
> We had that discussion before; the thermal subsystem doesn't like to be kept
> waiting. You would end up adding a lot of complexity for very little gain.
> It might make more sense to consider implementing runtime idle support
> instead of one-shot mode if power consumption is a concern.

OK thanks. I’ll leave this as unsupported for now.

> 
>>>> +	hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
>>>> +						      tmp108, tmp108_groups);
>>> 
>>> Please consider using the devm_ function here.
>>> 
>>>> +	if (IS_ERR(hwmon_dev)) {
>>>> +		dev_dbg(dev, "unable to register hwmon device\n");
>>>> +		return PTR_ERR(hwmon_dev);
>>>> +	}
>>>> +
>>>> +	tmp108->hwmon_dev = hwmon_dev;
>>>> +	tmp108->tz = thermal_zone_of_sensor_register(hwmon_dev, 0, hwmon_dev,
>>>> +						     &tmp108_of_thermal_ops);
>>>> +	if (IS_ERR(tmp108->tz))
>>>> +		return PTR_ERR(tmp108->tz);
>>> 
>>> hwmon device not unregistered here. That would be fixed by using the devm
>>> function above.
>>> 
>> For the above two registrations and .remove function, I was worried that there would be an order problem between the i2c->remove and the device-managed cleanup. I’ll get deeper into that code to determine if that is a problem.
>> 
> 
> The thermal sensor registration will be removed first (in the remove function).
> devm_ functions are all unregistered / removed after the remove function was
> called (in the order of the devm_ call). Otherwise you would have trouble
> with devm_kzalloc() as well.

Great.

> Note that my followup-patch had a problem with the minimum hysteresis
> temperature; it needs to be higher than the lower limit, not lower.

I’ll keep that in mind.

Cheers,

John.

Re: [PATCH 1/3] hwmon: Add Texas Instruments TMP108 temperature sensor driver.

[Guenter Roeck]

On Mon, Nov 28, 2016 at 01:25:37PM -0800, John Muir wrote:
> 
> > On Nov 28, 2016, at 11:58 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> > On Mon, Nov 28, 2016 at 11:40:42AM -0800, John Muir wrote:
> >>>> +static void tmp108_update_ready_time(struct tmp108 *tmp108)
> >>>> +{
> >>>> +	tmp108->ready_time = jiffies;
> >>>> +	if ((tmp108->config & TMP108_CONF_MODE_MASK)
> >>>> +	    == TMP108_MODE_CONTINUOUS) {
> >>> 
> >>> Don't you want a "!" here ? Presumably the delay is only needed
> >>> if the original configuration was not for continuous mode.
> >>> 
> >>>> +		tmp108->ready_time +=
> >>>> +			msecs_to_jiffies(TMP108_CONVERSION_TIME_MS);
> >>>> +	}
> >>>> +}
> >> The delay is required for both really. When the device is set into continuous mode it starts converting and the first temperature is ready (just under) 30ms later.
> >> 
> > 
> > The datasheet states, though, that the chip will start converting as soon
> > as the device powers up. The kernel would have to start really fast after that
> > to have to wait another 30 ms.
> > 
> > The current code ends up waiting if it doesn't have to (because it waits
> > if the chip was originally configured for continuous mode), and not waiting
> > if it has to (because the chip was not configured for continuous mode),
> > which doesn't seem to be such a good idea.
> 
> You are referring to the statement under “Conversion Rate” in the data sheet?
> 
> "After power-up or a general-call reset, the TMP108 immediately starts a conversion, as shown in Figure 9. The first result is available after 27 ms (typical).”
> 
> The datasheet also states that in ’shutdown’ mode, the device is only powering the serial interface (see "Mode Bits”). So, I assumed that the conversions weren’t happening during in that state, and that there would be the delay after moving from the ‘shutdown' state (as is described by the one-shot mode as well where the device state returns to ’shutdown’ when the conversion has taken place).

The device is, after power-on, not in shutdown mode.

> 
> My intention was that the delay is enforced after PM resume, and for convenience I re-used the same function during probe to initialize the ready_time. I was assuming that the device could be in the shutdown state - for example if the module had previously been unloaded. I agree that this is not required at system startup time. Perhaps I should check to see if the previous configuration had the device in the ‘shutdown’ state, and in that instance apply the delay? In my opinion it doesn’t really matter either way.
> 
> FYI, this same logic exists in the TMP102 device driver. The TMP102 device is very similar.
> 

The tmp102 driver adds the timeout if the device was in shutdown mode (SD=1).

	if (tmp102->config_orig & TMP102_CONF_SD) {
		...
		tmp102->ready_time += msecs_to_jiffies(CONVERSION_TIME_MS);
	}

Your code adds the timeout if the device was in continuous operation mode (M1=1).

	if ((tmp108->config & TMP108_CONF_MODE_MASK)
	    == TMP108_MODE_CONTINUOUS) {
		tmp108->ready_time +=
		msecs_to_jiffies(TMP108_CONVERSION_TIME_MS);
	}

Unless I am missing something, that is exactly the opposite.

Side note: Per datasheet, M0 is irrelevant if M1=1. The above check does not
match M1=1, M0=1, but that condition would still reflect continuous mode.

Guenter

Re: [PATCH 1/3] hwmon: Add Texas Instruments TMP108 temperature sensor driver.

[John Muir]

On Nov 28, 2016, at 2:19 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> The tmp102 driver adds the timeout if the device was in shutdown mode (SD=1).
> 
> 	if (tmp102->config_orig & TMP102_CONF_SD) {
> 		...
> 		tmp102->ready_time += msecs_to_jiffies(CONVERSION_TIME_MS);
> 	}
> 
> Your code adds the timeout if the device was in continuous operation mode (M1=1).
> 
> 	if ((tmp108->config & TMP108_CONF_MODE_MASK)
> 	    == TMP108_MODE_CONTINUOUS) {
> 		tmp108->ready_time +=
> 		msecs_to_jiffies(TMP108_CONVERSION_TIME_MS);
> 	}
> 
> Unless I am missing something, that is exactly the opposite.

Note that the TMP102 code is looking at ‘config_orig’ which was the initial device state, whereas in my proposed driver the code looks at the current configuration.

The update_ready_time function was only intended to be called AFTER the device has moved to a new mode. For the probe case I simply ignored the fact that it was already in continuous mode at startup and lazily re-used the update_ready_time function causing the code to run through a few extra instructions. I was attempting to re-use the logic in multiple cases.

I’ll update the code in my next patch series and you can re-review.

> Side note: Per datasheet, M0 is irrelevant if M1=1. The above check does not
> match M1=1, M0=1, but that condition would still reflect continuous mode.
> 

OK. Noted.

Thanks,

John.

Re: [PATCH 1/3] hwmon: Add Texas Instruments TMP108 temperature sensor driver.

[Guenter Roeck]

On Mon, Nov 28, 2016 at 03:10:38PM -0800, John Muir wrote:
> On Nov 28, 2016, at 2:19 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> > The tmp102 driver adds the timeout if the device was in shutdown mode (SD=1).
> > 
> > 	if (tmp102->config_orig & TMP102_CONF_SD) {
> > 		...
> > 		tmp102->ready_time += msecs_to_jiffies(CONVERSION_TIME_MS);
> > 	}
> > 
> > Your code adds the timeout if the device was in continuous operation mode (M1=1).
> > 
> > 	if ((tmp108->config & TMP108_CONF_MODE_MASK)
> > 	    == TMP108_MODE_CONTINUOUS) {
> > 		tmp108->ready_time +=
> > 		msecs_to_jiffies(TMP108_CONVERSION_TIME_MS);
> > 	}
> > 
> > Unless I am missing something, that is exactly the opposite.
> 
> Note that the TMP102 code is looking at ‘config_orig’ which was the initial device state, whereas in my proposed driver the code looks at the current configuration.
> 
Ah, yes. Your code also uses tmp108->config in tmp108_restore_config(),
which I guess confused me.

Not sure I understand why you would want to add the wait time even if M1
was set before (and the tmp102 driver doesn't do that). Really, if that
is what you want, it would be much simpler if you just unconditionally
set ready_time to 30 ms after the current time after setting the M1 bit,
no matter if it was set before or not. I personally don't think that would
be such a good idea, but it would at least be less obfuscating than the
current code. Also, I would suggest to either drop tmp108_restore_config(),
or to have it restore the real original state.

Thanks,
Guenter