Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754032AbcK0MUD (ORCPT ); Sun, 27 Nov 2016 07:20:03 -0500 Received: from bh-25.webhostbox.net ([208.91.199.152]:42528 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753917AbcK0MT5 (ORCPT ); Sun, 27 Nov 2016 07:19:57 -0500 Subject: Re: [PATCH 1/3] hwmon: Add Texas Instruments TMP108 temperature sensor driver. To: John Muir , Jean Delvare , Jonathan Corbet , Mark Rutland , Rob Herring , linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org References: <1480223737-82080-2-git-send-email-john@jmuir.com> From: Guenter Roeck Message-ID: Date: Sun, 27 Nov 2016 04:19:52 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1480223737-82080-2-git-send-email-john@jmuir.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Authenticated_sender: linux@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: authenticated_id: linux@roeck-us.net X-Authenticated-Sender: bh-25.webhostbox.net: linux@roeck-us.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 18770 Lines: 603 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 > --- > 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 > + > +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 > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + 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 "); > +MODULE_DESCRIPTION("Texas Instruments TMP108 temperature sensor driver"); > +MODULE_LICENSE("GPL"); >