Re: [PATCH 1/3] hwmon: Add Texas Instruments TMP108 temperature sensor driver.
From: Guenter Roeck
Date: Sun Nov 27 2016 - 07:20:15 EST
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@xxxxxxxxx>
---
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@xxxxxxxxx>
+
+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@xxxxxxxxx>
+ *
+ * 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, ®val);
+ 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, ®val);
+ 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@xxxxxxxxx>");
+MODULE_DESCRIPTION("Texas Instruments TMP108 temperature sensor driver");
+MODULE_LICENSE("GPL");