* [PATCH v3 1/2] hwmon: new driver for ST stts751 thermal sensor
@ 2017-01-24 15:02 Andrea Merello
2017-01-24 15:02 ` [PATCH v3 2/2] DT: add binding documentation for STTS751 Andrea Merello
2017-01-24 20:18 ` [PATCH v3 1/2] hwmon: new driver for ST stts751 thermal sensor Guenter Roeck
0 siblings, 2 replies; 7+ messages in thread
From: Andrea Merello @ 2017-01-24 15:02 UTC (permalink / raw)
To: linux-hwmon; +Cc: Andrea Merello, LABBE Corentin, Guenter Roeck, Jean Delvare
This patch adds a HWMON driver for ST Microelectronics STTS751
temperature sensors.
It does support manual-triggered conversions as well as automatic
conversions. The latter is used when the "event" or "therm" function
is present (declaring the physical wire is attached in the DT).
Thanks-to: LABBE Corentin [for suggestions]
Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
Cc: LABBE Corentin <clabbe.montjoie@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Jean Delvare <jdelvare@suse.com>
---
drivers/hwmon/Kconfig | 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/stts751.c | 809 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 820 insertions(+)
create mode 100644 drivers/hwmon/stts751.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index d1f82f2..8fdd241 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1448,6 +1448,16 @@ config SENSORS_SCH5636
This driver can also be built as a module. If so, the module
will be called sch5636.
+config SENSORS_STTS751
+ tristate "ST Microelectronics STTS751"
+ depends on I2C
+ help
+ If you say yes here you get support for STTS751
+ temperature sensor chips.
+
+ This driver can also be built as a module. If so, the module
+ will be called stts751.
+
config SENSORS_SMM665
tristate "Summit Microelectronics SMM665"
depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 7476b22..1114130 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -147,6 +147,7 @@ obj-$(CONFIG_SENSORS_SMM665) += smm665.o
obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o
obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
+obj-$(CONFIG_SENSORS_STTS751) += stts751.o
obj-$(CONFIG_SENSORS_AMC6821) += amc6821.o
obj-$(CONFIG_SENSORS_TC74) += tc74.o
obj-$(CONFIG_SENSORS_THMC50) += thmc50.o
diff --git a/drivers/hwmon/stts751.c b/drivers/hwmon/stts751.c
new file mode 100644
index 0000000..0d4716f
--- /dev/null
+++ b/drivers/hwmon/stts751.c
@@ -0,0 +1,809 @@
+/*
+ * STTS751 sensor driver
+ *
+ * Copyright (C) 2016-2017 Istituto Italiano di Tecnologia - RBCS - EDL
+ * Robotics, Brain and Cognitive Sciences department
+ * Electronic Design Laboratory
+ *
+ * Written by Andrea Merello <andrea.merello@gmail.com>
+ *
+ * Based on LM95241 driver and LM90 driver
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/bitops.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/util_macros.h>
+
+#define DEVNAME "stts751"
+
+static const unsigned short normal_i2c[] = {
+ 0x48, 0x49, 0x38, 0x39, /* STTS751-0 */
+ 0x4A, 0x4B, 0x3A, 0x3B, /* STTS751-1 */
+ I2C_CLIENT_END };
+
+#define STTS751_REG_TEMP_H 0x00
+#define STTS751_REG_STATUS 0x01
+#define STTS751_STATUS_TRIPT BIT(0)
+#define STTS751_STATUS_TRIPL BIT(5)
+#define STTS751_STATUS_TRIPH BIT(6)
+#define STTS751_REG_TEMP_L 0x02
+#define STTS751_REG_CONF 0x03
+#define STTS751_CONF_RES_MASK 0x0C
+#define STTS751_CONF_RES_SHIFT 2
+#define STTS751_CONF_EVENT_DIS BIT(7)
+#define STTS751_CONF_STOP BIT(6)
+#define STTS751_REG_RATE 0x04
+#define STTS751_REG_HLIM_H 0x05
+#define STTS751_REG_HLIM_L 0x06
+#define STTS751_REG_LLIM_H 0x07
+#define STTS751_REG_LLIM_L 0x08
+#define STTS751_REG_TLIM 0x20
+#define STTS751_REG_HYST 0x21
+#define STTS751_REG_SMBUS_TO 0x22
+
+#define STTS751_REG_PROD_ID 0xFD
+#define STTS751_REG_MAN_ID 0xFE
+#define STTS751_REG_REV_ID 0xFF
+
+#define STTS751_0_PROD_ID 0x00
+#define STTS751_1_PROD_ID 0x01
+#define ST_MAN_ID 0x53
+
+/*
+ * Possible update intervals are (in mS):
+ * 16000, 8000, 4000, 2000, 1000, 500, 250, 125, 62.5, 31.25
+ * However we are not going to complicate things too much and we stick to the
+ * approx value in mS.
+ */
+static const int stts751_intervals[] = {
+ 16000, 8000, 4000, 2000, 1000, 500, 250, 125, 63, 31
+};
+
+static const struct i2c_device_id stts751_id[] = {
+ { "stts751", 0 },
+ { }
+};
+
+struct stts751_priv {
+ struct device *dev;
+ struct i2c_client *client;
+ struct mutex access_lock;
+ u8 interval;
+ int res;
+ int event_max, event_min;
+ int therm;
+ int hyst;
+ bool smbus_timeout;
+ int temp;
+ unsigned long last_update, last_alert_update;
+ u8 config;
+ bool min_alert, max_alert, therm_trip;
+ bool data_valid, alert_valid;
+};
+
+/*
+ * These functions converts temperature from HW format to integer format and
+ * vice-vers. They are (mostly) taken from lm90 driver. Unit is in mC.
+ */
+static int stts751_to_deg(s32 hw_val)
+{
+ return hw_val * 125 / 32;
+}
+
+static s32 stts751_to_hw(int val)
+{
+ s32 hw_val;
+
+ if (val < 0)
+ hw_val = (val - 62) / 125 * 32;
+ else
+ hw_val = (val + 62) / 125 * 32;
+
+ return hw_val;
+}
+
+static int stts751_adjust_resolution(struct stts751_priv *priv)
+{
+ u8 res;
+
+ switch (priv->interval) {
+ case 9:
+ /* 10 bits */
+ res = 0;
+ break;
+ case 8:
+ /* 11 bits */
+ res = 1;
+ break;
+ default:
+ /* 12 bits */
+ res = 3;
+ break;
+ }
+
+ if (priv->res == res)
+ return 0;
+
+ priv->config &= ~STTS751_CONF_RES_MASK;
+ priv->config |= res << STTS751_CONF_RES_SHIFT;
+
+ return i2c_smbus_write_byte_data(priv->client,
+ STTS751_REG_CONF, priv->config);
+}
+
+static int stts751_update_temp(struct stts751_priv *priv)
+{
+ s32 integer1, integer2, frac;
+ int ret = 0;
+
+ /*
+ * There is a trick here, like in the lm90 driver. We have to read two
+ * registers to get the sensor temperature, but we have to beware a
+ * conversion could occur between the readings. We could use the
+ * one-shot conversion register, but we don't want to do this (disables
+ * hardware monitoring). So the solution used here is to read the high
+ * byte once, then the low byte, then the high byte again. If the new
+ * high byte matches the old one, then we have a valid reading. Else we
+ * have to read the low byte again, and now we believe we have a correct
+ * reading.
+ */
+ integer1 = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_H);
+ if (integer1 < 0) {
+ dev_dbg(&priv->client->dev,
+ "I2C read failed (temp H). ret: %x\n", ret);
+ ret = integer1;
+ goto exit;
+ }
+
+ frac = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_L);
+ if (frac < 0) {
+ dev_dbg(&priv->client->dev,
+ "I2C read failed (temp L). ret: %x\n", ret);
+ ret = frac;
+ goto exit;
+ }
+
+ integer2 = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_H);
+ if (integer2 < 0) {
+ dev_dbg(&priv->client->dev,
+ "I2C 2nd read failed (temp H). ret: %x\n", ret);
+ ret = integer2;
+ goto exit;
+ }
+
+ if (integer1 != integer2) {
+ frac = i2c_smbus_read_byte_data(priv->client,
+ STTS751_REG_TEMP_L);
+ if (frac < 0) {
+ dev_dbg(&priv->client->dev,
+ "I2C 2nd read failed (temp L). ret: %x\n", ret);
+ ret = frac;
+ goto exit;
+ }
+ }
+
+exit:
+ if (ret)
+ return ret;
+
+ priv->temp = stts751_to_deg((integer1 << 8) | frac);
+ return ret;
+}
+
+static int stts751_set_temp_reg16(struct stts751_priv *priv, int temp,
+ u8 hreg, u8 lreg)
+{
+ s32 hwval;
+ int ret;
+
+ hwval = stts751_to_hw(temp);
+
+ mutex_lock(&priv->access_lock);
+ ret = i2c_smbus_write_byte_data(priv->client, hreg, hwval >> 8);
+ if (!ret)
+ ret = i2c_smbus_write_byte_data(priv->client, lreg,
+ hwval & 0xff);
+ mutex_unlock(&priv->access_lock);
+
+ return ret;
+}
+
+static int stts751_set_temp_reg8(struct stts751_priv *priv, int temp, u8 reg)
+{
+ s32 hwval;
+ int ret;
+
+ hwval = stts751_to_hw(temp);
+
+ mutex_lock(&priv->access_lock);
+ ret = i2c_smbus_write_byte_data(priv->client, reg, hwval >> 8);
+ mutex_unlock(&priv->access_lock);
+
+ return ret;
+}
+
+static int stts751_read_reg16(struct stts751_priv *priv, int *temp,
+ u8 hreg, u8 lreg)
+{
+ int integer, frac;
+
+ integer = i2c_smbus_read_byte_data(priv->client, hreg);
+ if (integer < 0)
+ return integer;
+
+ frac = i2c_smbus_read_byte_data(priv->client, lreg);
+ if (frac < 0)
+ return frac;
+
+ *temp = stts751_to_deg((integer << 8) | frac);
+
+ return 0;
+}
+
+static int stts751_read_reg8(struct stts751_priv *priv, int *temp, u8 reg)
+{
+ int integer;
+
+ integer = i2c_smbus_read_byte_data(priv->client, reg);
+ if (integer < 0)
+ return integer;
+
+ *temp = stts751_to_deg(integer << 8);
+
+ return 0;
+}
+
+/*
+ * Update alert flags without waiting for cache to expire. We detects alerts
+ * immediately for the sake of the alert handler; we still need to deal with
+ * caching to workaround the fact that the status register gets cleared when
+ * reading it.
+ */
+static int stts751_update_alert(struct stts751_priv *priv)
+{
+ int ret;
+ bool conv_done;
+ int cache_time =
+ DIV_ROUND_UP(stts751_intervals[priv->interval] * HZ, 1000);
+
+ /*
+ * Add another 10% because if we run faster than the HW conversion
+ * rate we will end up in reporting incorrectly alarms.
+ */
+ cache_time += cache_time / 10;
+
+ ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_STATUS);
+ if (ret < 0)
+ return ret;
+
+ dev_dbg(priv->dev, "status reg %x\n", ret);
+ conv_done = ret & (STTS751_STATUS_TRIPH | STTS751_STATUS_TRIPL);
+ /*
+ * Reset the cache if the cache time expired, or if we are sure
+ * we have valid data from a device conversion, or if we know
+ * our cache has been never written.
+ *
+ * Note that when the cache has been never written the point is
+ * to correctly initialize the timestamp, rather than clearing
+ * the cache values.
+ *
+ * Note that updating the cache timestamp when we get an alarm flag
+ * is required, otherwise we could incorrectly report alarms to be zero.
+ */
+ if (time_after(jiffies, priv->last_alert_update + cache_time) ||
+ conv_done || !priv->alert_valid) {
+ priv->max_alert = false;
+ priv->min_alert = false;
+ priv->alert_valid = true;
+ priv->last_alert_update = jiffies;
+ dev_dbg(priv->dev, "invalidating alert cache\n");
+ }
+
+ priv->max_alert |= !!(ret & STTS751_STATUS_TRIPH);
+ priv->min_alert |= !!(ret & STTS751_STATUS_TRIPL);
+ priv->therm_trip = !!(ret & STTS751_STATUS_TRIPT);
+
+ dev_dbg(priv->dev, "max_alert: %d, min_alert: %d, therm_trip: %d\n",
+ priv->max_alert, priv->min_alert, priv->therm_trip);
+
+ return 0;
+}
+
+static void stts751_alert(struct i2c_client *client,
+ enum i2c_alert_protocol type, unsigned int data)
+{
+ int ret;
+ struct stts751_priv *priv = i2c_get_clientdata(client);
+
+ if (type != I2C_PROTOCOL_SMBUS_ALERT)
+ return;
+
+ dev_dbg(&client->dev, "alert!");
+
+ mutex_lock(&priv->access_lock);
+ ret = stts751_update_alert(priv);
+ if (ret < 0) {
+ /* default to worst case */
+ priv->max_alert = true;
+ priv->min_alert = true;
+
+ dev_warn(&priv->client->dev,
+ "Alert received, but can't communicate to the device. Something bad happening? Triggering all alarms!");
+ }
+
+ if (priv->max_alert) {
+ dev_notice(&client->dev, "got alert for HIGH temperature");
+
+ /* unblock alert poll */
+ sysfs_notify(&priv->dev->kobj, NULL, "temp1_event_max_alert");
+ kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
+ }
+
+ if (priv->min_alert) {
+ dev_notice(&client->dev, "got alert for LOW temperature");
+
+ /* unblock alert poll */
+ sysfs_notify(&priv->dev->kobj, NULL, "temp1_event_min_alert");
+ kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
+ }
+ mutex_unlock(&priv->access_lock);
+}
+
+static int stts751_update(struct stts751_priv *priv)
+{
+ int ret = 0;
+ int cache_time = stts751_intervals[priv->interval] / 1000;
+
+ mutex_lock(&priv->access_lock);
+ if (time_after(jiffies, priv->last_update + cache_time) ||
+ !priv->data_valid) {
+ priv->data_valid = true;
+ priv->last_update = jiffies;
+
+ ret = stts751_update_temp(priv);
+ if (ret)
+ goto exit;
+
+ ret = stts751_update_alert(priv);
+ }
+exit:
+ mutex_unlock(&priv->access_lock);
+
+ return ret;
+}
+
+static ssize_t show_max_alarm(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ int ret;
+ struct stts751_priv *priv = dev_get_drvdata(dev);
+
+ ret = stts751_update(priv);
+ if (ret < 0)
+ return ret;
+
+ return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->max_alert);
+}
+
+static ssize_t show_min_alarm(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ int ret;
+ struct stts751_priv *priv = dev_get_drvdata(dev);
+
+ ret = stts751_update(priv);
+ if (ret < 0)
+ return ret;
+
+ return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->min_alert);
+}
+
+static ssize_t show_input(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ int ret;
+ struct stts751_priv *priv = dev_get_drvdata(dev);
+
+ ret = stts751_update(priv);
+ if (ret < 0)
+ return ret;
+
+ return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->temp);
+}
+
+static ssize_t show_therm(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct stts751_priv *priv = dev_get_drvdata(dev);
+
+ return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->therm);
+}
+
+static int do_set_hyst(struct stts751_priv *priv, int temp)
+{
+ /* HW works in range -64C to +127.937C */
+ temp = clamp_val(temp, -64000, priv->therm);
+ priv->hyst = temp;
+ dev_dbg(priv->dev, "setting hyst %d", temp);
+ temp = priv->therm - temp;
+
+ return stts751_set_temp_reg8(priv, temp, STTS751_REG_HYST);
+}
+
+static ssize_t set_therm(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ret;
+ long temp;
+ struct stts751_priv *priv = dev_get_drvdata(dev);
+
+ if (kstrtol(buf, 10, &temp) < 0)
+ return -EINVAL;
+ /* HW works in range -64C to +127.937C */
+ temp = clamp_val(temp, -64000, 127937);
+ ret = stts751_set_temp_reg8(priv, temp, STTS751_REG_TLIM);
+ if (ret)
+ return ret;
+
+ dev_dbg(dev, "setting therm %ld", temp);
+ priv->therm = temp;
+
+ /*
+ * hysteresis reg is relative to therm, so we need to update
+ * it as well.
+ */
+ ret = do_set_hyst(priv, priv->hyst);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+static ssize_t show_hyst(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct stts751_priv *priv = dev_get_drvdata(dev);
+
+ return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->hyst);
+}
+
+static ssize_t set_hyst(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ret;
+ long temp;
+
+ struct stts751_priv *priv = dev_get_drvdata(dev);
+
+ if (kstrtol(buf, 10, &temp) < 0)
+ return -EINVAL;
+ ret = do_set_hyst(priv, temp);
+ if (ret)
+ return ret;
+ return count;
+}
+
+static ssize_t show_therm_trip(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int ret;
+ struct stts751_priv *priv = dev_get_drvdata(dev);
+
+ ret = stts751_update(priv);
+ if (ret < 0)
+ return ret;
+
+ return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->therm_trip);
+}
+
+static ssize_t show_max(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct stts751_priv *priv = dev_get_drvdata(dev);
+
+ return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_max);
+}
+
+static ssize_t set_max(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ret;
+ long temp;
+ struct stts751_priv *priv = dev_get_drvdata(dev);
+
+ if (kstrtol(buf, 10, &temp) < 0)
+ return -EINVAL;
+ /* HW works in range -64C to +127.937C */
+ temp = clamp_val(temp, priv->event_min, 127937);
+ ret = stts751_set_temp_reg16(priv, temp,
+ STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
+ if (ret)
+ return ret;
+
+ dev_dbg(dev, "setting event max %ld", temp);
+ priv->event_max = temp;
+ return count;
+}
+
+static ssize_t show_min(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct stts751_priv *priv = dev_get_drvdata(dev);
+
+ return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_min);
+}
+
+static ssize_t set_min(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ret;
+ long temp;
+ struct stts751_priv *priv = dev_get_drvdata(dev);
+
+ if (kstrtol(buf, 10, &temp) < 0)
+ return -EINVAL;
+ /* HW works in range -64C to +127.937C */
+ temp = clamp_val(temp, -64000, priv->event_max);
+ ret = stts751_set_temp_reg16(priv, temp,
+ STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);
+ if (ret)
+ return ret;
+
+ dev_dbg(dev, "setting event min %ld", temp);
+
+ priv->event_min = temp;
+ return count;
+}
+
+static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct stts751_priv *priv = dev_get_drvdata(dev);
+
+ return snprintf(buf, PAGE_SIZE - 1, "%d\n",
+ stts751_intervals[priv->interval]);
+}
+
+static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ unsigned long val;
+ int idx;
+ int ret = 0;
+ struct stts751_priv *priv = dev_get_drvdata(dev);
+
+ if (kstrtoul(buf, 10, &val) < 0)
+ return -EINVAL;
+
+ idx = find_closest_descending(val, stts751_intervals,
+ ARRAY_SIZE(stts751_intervals));
+
+ dev_dbg(dev, "setting interval. req:%lu, idx: %d, val: %d", val, idx,
+ stts751_intervals[idx]);
+
+ if (priv->interval == idx)
+ return count;
+
+ mutex_lock(&priv->access_lock);
+
+ /*
+ * In early development stages I've become suspicious about the chip
+ * starting to misbehave if I ever set, even briefly, an invalid
+ * configuration. While I'm not sure this is really needed, be
+ * conservative and set rate/resolution in such an order that avoids
+ * passing through an invalid configuration.
+ */
+
+ /* speed up: lower the resolution, then modify convrate */
+ if (priv->interval < idx) {
+ priv->interval = idx;
+ ret = stts751_adjust_resolution(priv);
+ if (ret)
+ goto exit;
+ }
+
+ ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_RATE, idx);
+ if (ret)
+ goto exit;
+ /* slow down: modify convrate, then raise resolution */
+ if (priv->interval != idx) {
+ priv->interval = idx;
+ ret = stts751_adjust_resolution(priv);
+ if (ret)
+ goto exit;
+ }
+exit:
+ mutex_unlock(&priv->access_lock);
+
+ return count;
+}
+
+static int stts751_detect(struct i2c_client *new_client,
+ struct i2c_board_info *info)
+{
+ struct i2c_adapter *adapter = new_client->adapter;
+ const char *name;
+ int mfg_id, prod_id, rev_id;
+
+ if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+ return -ENODEV;
+
+ mfg_id = i2c_smbus_read_byte_data(new_client, ST_MAN_ID);
+ if (mfg_id != ST_MAN_ID)
+ return -ENODEV;
+
+ prod_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_PROD_ID);
+
+ switch (prod_id) {
+ case STTS751_0_PROD_ID:
+ name = "STTS751-0";
+ break;
+ case STTS751_1_PROD_ID:
+ name = "STTS751-1";
+ break;
+ default:
+ return -ENODEV;
+ }
+ dev_dbg(&new_client->dev, "Chip %s detected!", name);
+
+ rev_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_REV_ID);
+ if (rev_id < 0)
+ return -ENODEV;
+ if (rev_id != 0x1) {
+ dev_notice(&new_client->dev,
+ "Chip revision 0x%x is untested\nPlease report whether it works to andrea.merello@gmail.com",
+ rev_id);
+ }
+
+ strlcpy(info->type, stts751_id[0].name, I2C_NAME_SIZE);
+ return 0;
+}
+
+static int stts751_read_chip_config(struct stts751_priv *priv)
+{
+ int ret;
+ int tmp;
+
+ ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_CONF);
+ if (ret < 0)
+ return ret;
+ priv->config = ret;
+ priv->res = (ret & STTS751_CONF_RES_MASK) >> STTS751_CONF_RES_SHIFT;
+
+ ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_RATE);
+ if (ret < 0)
+ return ret;
+ priv->interval = ret;
+
+ ret = stts751_read_reg16(priv, &priv->event_max,
+ STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
+ if (ret)
+ return ret;
+
+ ret = stts751_read_reg16(priv, &priv->event_min,
+ STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);
+ if (ret)
+ return ret;
+
+ ret = stts751_read_reg8(priv, &priv->therm, STTS751_REG_TLIM);
+ if (ret)
+ return ret;
+
+ ret = stts751_read_reg8(priv, &tmp, STTS751_REG_HYST);
+ if (ret)
+ return ret;
+ priv->hyst = priv->therm - tmp;
+
+ return ret;
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_min, set_min, 0);
+static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_max, set_max, 0);
+static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IRUGO, show_min_alarm, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, show_max_alarm, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_therm,
+ set_therm, 0);
+static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_hyst,
+ set_hyst, 0);
+static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_therm_trip, NULL, 0);
+static SENSOR_DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO,
+ show_interval, set_interval, 0);
+
+static struct attribute *stts751_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,
+ &sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
+ &sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
+ &sensor_dev_attr_temp1_crit.dev_attr.attr,
+ &sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
+ &sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
+ &sensor_dev_attr_update_interval.dev_attr.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(stts751);
+
+static int stts751_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct stts751_priv *priv;
+ int ret;
+ bool smbus_to;
+
+ priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->client = client;
+ i2c_set_clientdata(client, priv);
+ mutex_init(&priv->access_lock);
+
+ if (device_property_present(&client->dev,
+ "smbus-timeout-disable")) {
+ smbus_to = !device_property_read_bool(&client->dev,
+ "smbus-timeout-disable");
+
+ ret = i2c_smbus_write_byte_data(priv->client,
+ STTS751_REG_SMBUS_TO,
+ smbus_to ? 0x80 : 0);
+ if (ret)
+ return ret;
+ }
+
+ ret = stts751_read_chip_config(priv);
+ if (ret)
+ return ret;
+
+ priv->config &= ~(STTS751_CONF_STOP | STTS751_CONF_EVENT_DIS);
+ ret = i2c_smbus_write_byte_data(priv->client,
+ STTS751_REG_CONF, priv->config);
+ if (ret)
+ return ret;
+
+ priv->dev = devm_hwmon_device_register_with_groups(&client->dev,
+ client->name, priv,
+ stts751_groups);
+ return PTR_ERR_OR_ZERO(priv->dev);
+}
+
+MODULE_DEVICE_TABLE(i2c, stts751_id);
+
+static struct i2c_driver stts751_driver = {
+ .class = I2C_CLASS_HWMON,
+ .driver = {
+ .name = DEVNAME,
+ },
+ .probe = stts751_probe,
+ .id_table = stts751_id,
+ .detect = stts751_detect,
+ .alert = stts751_alert,
+ .address_list = normal_i2c,
+};
+
+module_i2c_driver(stts751_driver);
+
+MODULE_AUTHOR("Andrea Merello <andrea.merello@gmail.com>");
+MODULE_DESCRIPTION("STTS751 sensor driver");
+MODULE_LICENSE("GPL");
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 2/2] DT: add binding documentation for STTS751
2017-01-24 15:02 [PATCH v3 1/2] hwmon: new driver for ST stts751 thermal sensor Andrea Merello
@ 2017-01-24 15:02 ` Andrea Merello
2017-01-24 20:18 ` [PATCH v3 1/2] hwmon: new driver for ST stts751 thermal sensor Guenter Roeck
1 sibling, 0 replies; 7+ messages in thread
From: Andrea Merello @ 2017-01-24 15:02 UTC (permalink / raw)
To: linux-hwmon; +Cc: Andrea Merello, Rob Herring, Mark Rutland
Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
---
Documentation/devicetree/bindings/hwmon/stts751.txt | 15 +++++++++++++++
1 file changed, 15 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/stts751.txt
diff --git a/Documentation/devicetree/bindings/hwmon/stts751.txt b/Documentation/devicetree/bindings/hwmon/stts751.txt
new file mode 100644
index 0000000..3ee1dc3
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/stts751.txt
@@ -0,0 +1,15 @@
+* STTS751 thermometer.
+
+Required node properties:
+- compatible: "stts751"
+- reg: I2C bus address of the device
+
+Optional properties:
+- smbus-timeout-disable: when set, the smbus timeout function will be disabled
+
+Example stts751 node:
+
+temp-sensor {
+ compatible = "stts751";
+ reg = <0x48>;
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] hwmon: new driver for ST stts751 thermal sensor
2017-01-24 15:02 [PATCH v3 1/2] hwmon: new driver for ST stts751 thermal sensor Andrea Merello
2017-01-24 15:02 ` [PATCH v3 2/2] DT: add binding documentation for STTS751 Andrea Merello
@ 2017-01-24 20:18 ` Guenter Roeck
2017-01-25 9:51 ` Andrea Merello
1 sibling, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2017-01-24 20:18 UTC (permalink / raw)
To: Andrea Merello; +Cc: linux-hwmon, LABBE Corentin, Jean Delvare
On Tue, Jan 24, 2017 at 04:02:20PM +0100, Andrea Merello wrote:
> This patch adds a HWMON driver for ST Microelectronics STTS751
> temperature sensors.
>
> It does support manual-triggered conversions as well as automatic
> conversions. The latter is used when the "event" or "therm" function
> is present (declaring the physical wire is attached in the DT).
>
> Thanks-to: LABBE Corentin [for suggestions]
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> Cc: LABBE Corentin <clabbe.montjoie@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Jean Delvare <jdelvare@suse.com>
> ---
No change log, meaning we have to review the entire driver again or look up
older versions and comments ourselves. This will take a while. For now, just a
few quick commnents. For the future, please consider providing a changelog.
> drivers/hwmon/Kconfig | 10 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/stts751.c | 809 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 820 insertions(+)
> create mode 100644 drivers/hwmon/stts751.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index d1f82f2..8fdd241 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1448,6 +1448,16 @@ config SENSORS_SCH5636
> This driver can also be built as a module. If so, the module
> will be called sch5636.
>
> +config SENSORS_STTS751
> + tristate "ST Microelectronics STTS751"
> + depends on I2C
> + help
> + If you say yes here you get support for STTS751
> + temperature sensor chips.
> +
> + This driver can also be built as a module. If so, the module
> + will be called stts751.
> +
> config SENSORS_SMM665
> tristate "Summit Microelectronics SMM665"
> depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 7476b22..1114130 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -147,6 +147,7 @@ obj-$(CONFIG_SENSORS_SMM665) += smm665.o
> obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
> obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o
> obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
> +obj-$(CONFIG_SENSORS_STTS751) += stts751.o
> obj-$(CONFIG_SENSORS_AMC6821) += amc6821.o
> obj-$(CONFIG_SENSORS_TC74) += tc74.o
> obj-$(CONFIG_SENSORS_THMC50) += thmc50.o
> diff --git a/drivers/hwmon/stts751.c b/drivers/hwmon/stts751.c
> new file mode 100644
> index 0000000..0d4716f
> --- /dev/null
> +++ b/drivers/hwmon/stts751.c
> @@ -0,0 +1,809 @@
> +/*
> + * STTS751 sensor driver
> + *
> + * Copyright (C) 2016-2017 Istituto Italiano di Tecnologia - RBCS - EDL
> + * Robotics, Brain and Cognitive Sciences department
> + * Electronic Design Laboratory
> + *
> + * Written by Andrea Merello <andrea.merello@gmail.com>
> + *
> + * Based on LM95241 driver and LM90 driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/util_macros.h>
> +
> +#define DEVNAME "stts751"
> +
> +static const unsigned short normal_i2c[] = {
> + 0x48, 0x49, 0x38, 0x39, /* STTS751-0 */
> + 0x4A, 0x4B, 0x3A, 0x3B, /* STTS751-1 */
> + I2C_CLIENT_END };
> +
> +#define STTS751_REG_TEMP_H 0x00
> +#define STTS751_REG_STATUS 0x01
> +#define STTS751_STATUS_TRIPT BIT(0)
> +#define STTS751_STATUS_TRIPL BIT(5)
> +#define STTS751_STATUS_TRIPH BIT(6)
> +#define STTS751_REG_TEMP_L 0x02
> +#define STTS751_REG_CONF 0x03
> +#define STTS751_CONF_RES_MASK 0x0C
> +#define STTS751_CONF_RES_SHIFT 2
> +#define STTS751_CONF_EVENT_DIS BIT(7)
> +#define STTS751_CONF_STOP BIT(6)
> +#define STTS751_REG_RATE 0x04
> +#define STTS751_REG_HLIM_H 0x05
> +#define STTS751_REG_HLIM_L 0x06
> +#define STTS751_REG_LLIM_H 0x07
> +#define STTS751_REG_LLIM_L 0x08
> +#define STTS751_REG_TLIM 0x20
> +#define STTS751_REG_HYST 0x21
> +#define STTS751_REG_SMBUS_TO 0x22
> +
> +#define STTS751_REG_PROD_ID 0xFD
> +#define STTS751_REG_MAN_ID 0xFE
> +#define STTS751_REG_REV_ID 0xFF
> +
> +#define STTS751_0_PROD_ID 0x00
> +#define STTS751_1_PROD_ID 0x01
> +#define ST_MAN_ID 0x53
> +
> +/*
> + * Possible update intervals are (in mS):
> + * 16000, 8000, 4000, 2000, 1000, 500, 250, 125, 62.5, 31.25
> + * However we are not going to complicate things too much and we stick to the
> + * approx value in mS.
> + */
> +static const int stts751_intervals[] = {
> + 16000, 8000, 4000, 2000, 1000, 500, 250, 125, 63, 31
> +};
> +
> +static const struct i2c_device_id stts751_id[] = {
> + { "stts751", 0 },
> + { }
> +};
> +
> +struct stts751_priv {
> + struct device *dev;
> + struct i2c_client *client;
> + struct mutex access_lock;
> + u8 interval;
> + int res;
> + int event_max, event_min;
> + int therm;
> + int hyst;
> + bool smbus_timeout;
> + int temp;
> + unsigned long last_update, last_alert_update;
> + u8 config;
> + bool min_alert, max_alert, therm_trip;
> + bool data_valid, alert_valid;
> +};
> +
> +/*
> + * These functions converts temperature from HW format to integer format and
> + * vice-vers. They are (mostly) taken from lm90 driver. Unit is in mC.
> + */
> +static int stts751_to_deg(s32 hw_val)
> +{
> + return hw_val * 125 / 32;
> +}
> +
> +static s32 stts751_to_hw(int val)
> +{
> + s32 hw_val;
> +
> + if (val < 0)
> + hw_val = (val - 62) / 125 * 32;
> + else
> + hw_val = (val + 62) / 125 * 32;
> +
How about "return DIV_ROUND_CLOSEST(val, 125) * 32;" ?
> + return hw_val;
> +}
> +
> +static int stts751_adjust_resolution(struct stts751_priv *priv)
> +{
> + u8 res;
> +
> + switch (priv->interval) {
> + case 9:
> + /* 10 bits */
> + res = 0;
> + break;
> + case 8:
> + /* 11 bits */
> + res = 1;
> + break;
> + default:
> + /* 12 bits */
> + res = 3;
> + break;
> + }
> +
> + if (priv->res == res)
> + return 0;
> +
> + priv->config &= ~STTS751_CONF_RES_MASK;
> + priv->config |= res << STTS751_CONF_RES_SHIFT;
> +
> + return i2c_smbus_write_byte_data(priv->client,
> + STTS751_REG_CONF, priv->config);
> +}
> +
> +static int stts751_update_temp(struct stts751_priv *priv)
> +{
> + s32 integer1, integer2, frac;
> + int ret = 0;
> +
> + /*
> + * There is a trick here, like in the lm90 driver. We have to read two
> + * registers to get the sensor temperature, but we have to beware a
> + * conversion could occur between the readings. We could use the
> + * one-shot conversion register, but we don't want to do this (disables
> + * hardware monitoring). So the solution used here is to read the high
> + * byte once, then the low byte, then the high byte again. If the new
> + * high byte matches the old one, then we have a valid reading. Else we
> + * have to read the low byte again, and now we believe we have a correct
> + * reading.
> + */
> + integer1 = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_H);
> + if (integer1 < 0) {
> + dev_dbg(&priv->client->dev,
> + "I2C read failed (temp H). ret: %x\n", ret);
> + ret = integer1;
> + goto exit;
> + }
> +
> + frac = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_L);
> + if (frac < 0) {
> + dev_dbg(&priv->client->dev,
> + "I2C read failed (temp L). ret: %x\n", ret);
> + ret = frac;
> + goto exit;
> + }
> +
> + integer2 = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_H);
> + if (integer2 < 0) {
> + dev_dbg(&priv->client->dev,
> + "I2C 2nd read failed (temp H). ret: %x\n", ret);
> + ret = integer2;
> + goto exit;
> + }
> +
> + if (integer1 != integer2) {
> + frac = i2c_smbus_read_byte_data(priv->client,
> + STTS751_REG_TEMP_L);
> + if (frac < 0) {
> + dev_dbg(&priv->client->dev,
> + "I2C 2nd read failed (temp L). ret: %x\n", ret);
> + ret = frac;
> + goto exit;
> + }
> + }
> +
> +exit:
> + if (ret)
> + return ret;
If the code can return immediately, please do so.
> +
> + priv->temp = stts751_to_deg((integer1 << 8) | frac);
> + return ret;
> +}
> +
> +static int stts751_set_temp_reg16(struct stts751_priv *priv, int temp,
> + u8 hreg, u8 lreg)
> +{
> + s32 hwval;
> + int ret;
> +
> + hwval = stts751_to_hw(temp);
> +
> + mutex_lock(&priv->access_lock);
> + ret = i2c_smbus_write_byte_data(priv->client, hreg, hwval >> 8);
> + if (!ret)
> + ret = i2c_smbus_write_byte_data(priv->client, lreg,
> + hwval & 0xff);
> + mutex_unlock(&priv->access_lock);
> +
> + return ret;
> +}
> +
> +static int stts751_set_temp_reg8(struct stts751_priv *priv, int temp, u8 reg)
> +{
> + s32 hwval;
> + int ret;
> +
> + hwval = stts751_to_hw(temp);
> +
> + mutex_lock(&priv->access_lock);
> + ret = i2c_smbus_write_byte_data(priv->client, reg, hwval >> 8);
> + mutex_unlock(&priv->access_lock);
The lock is in the wrong location. It needs to be in the calling code,
since the calling code updates context variables. The same is true for
stts751_set_temp_reg16().
> +
> + return ret;
> +}
> +
> +static int stts751_read_reg16(struct stts751_priv *priv, int *temp,
> + u8 hreg, u8 lreg)
> +{
> + int integer, frac;
> +
> + integer = i2c_smbus_read_byte_data(priv->client, hreg);
> + if (integer < 0)
> + return integer;
> +
> + frac = i2c_smbus_read_byte_data(priv->client, lreg);
> + if (frac < 0)
> + return frac;
> +
> + *temp = stts751_to_deg((integer << 8) | frac);
> +
> + return 0;
> +}
> +
> +static int stts751_read_reg8(struct stts751_priv *priv, int *temp, u8 reg)
> +{
> + int integer;
> +
> + integer = i2c_smbus_read_byte_data(priv->client, reg);
> + if (integer < 0)
> + return integer;
> +
> + *temp = stts751_to_deg(integer << 8);
> +
> + return 0;
> +}
> +
> +/*
> + * Update alert flags without waiting for cache to expire. We detects alerts
> + * immediately for the sake of the alert handler; we still need to deal with
> + * caching to workaround the fact that the status register gets cleared when
Did we have this before ? The datasheet claims that the status is only cleared
if the condition no longer exists. If that is incorrect, please explain here,
or it will come up again.
> + * reading it.
> + */
> +static int stts751_update_alert(struct stts751_priv *priv)
> +{
> + int ret;
> + bool conv_done;
> + int cache_time =
> + DIV_ROUND_UP(stts751_intervals[priv->interval] * HZ, 1000);
How about using msecs_to_jiffies() ?
> +
> + /*
> + * Add another 10% because if we run faster than the HW conversion
> + * rate we will end up in reporting incorrectly alarms.
> + */
> + cache_time += cache_time / 10;
> +
> + ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_STATUS);
> + if (ret < 0)
> + return ret;
> +
> + dev_dbg(priv->dev, "status reg %x\n", ret);
> + conv_done = ret & (STTS751_STATUS_TRIPH | STTS751_STATUS_TRIPL);
> + /*
> + * Reset the cache if the cache time expired, or if we are sure
> + * we have valid data from a device conversion, or if we know
> + * our cache has been never written.
> + *
> + * Note that when the cache has been never written the point is
> + * to correctly initialize the timestamp, rather than clearing
> + * the cache values.
> + *
> + * Note that updating the cache timestamp when we get an alarm flag
> + * is required, otherwise we could incorrectly report alarms to be zero.
> + */
> + if (time_after(jiffies, priv->last_alert_update + cache_time) ||
> + conv_done || !priv->alert_valid) {
> + priv->max_alert = false;
> + priv->min_alert = false;
> + priv->alert_valid = true;
> + priv->last_alert_update = jiffies;
> + dev_dbg(priv->dev, "invalidating alert cache\n");
> + }
> +
> + priv->max_alert |= !!(ret & STTS751_STATUS_TRIPH);
> + priv->min_alert |= !!(ret & STTS751_STATUS_TRIPL);
> + priv->therm_trip = !!(ret & STTS751_STATUS_TRIPT);
> +
> + dev_dbg(priv->dev, "max_alert: %d, min_alert: %d, therm_trip: %d\n",
> + priv->max_alert, priv->min_alert, priv->therm_trip);
> +
> + return 0;
> +}
> +
> +static void stts751_alert(struct i2c_client *client,
> + enum i2c_alert_protocol type, unsigned int data)
> +{
> + int ret;
> + struct stts751_priv *priv = i2c_get_clientdata(client);
> +
> + if (type != I2C_PROTOCOL_SMBUS_ALERT)
> + return;
> +
> + dev_dbg(&client->dev, "alert!");
> +
> + mutex_lock(&priv->access_lock);
> + ret = stts751_update_alert(priv);
> + if (ret < 0) {
> + /* default to worst case */
> + priv->max_alert = true;
> + priv->min_alert = true;
> +
> + dev_warn(&priv->client->dev,
> + "Alert received, but can't communicate to the device. Something bad happening? Triggering all alarms!");
> + }
> +
> + if (priv->max_alert) {
> + dev_notice(&client->dev, "got alert for HIGH temperature");
> +
> + /* unblock alert poll */
> + sysfs_notify(&priv->dev->kobj, NULL, "temp1_event_max_alert");
> + kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
> + }
> +
> + if (priv->min_alert) {
> + dev_notice(&client->dev, "got alert for LOW temperature");
> +
> + /* unblock alert poll */
> + sysfs_notify(&priv->dev->kobj, NULL, "temp1_event_min_alert");
> + kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
This way you can get two uevents, which is really undesirable.
Maybe move uevent creation into a separate if().
> + }
> + mutex_unlock(&priv->access_lock);
> +}
> +
> +static int stts751_update(struct stts751_priv *priv)
> +{
> + int ret = 0;
> + int cache_time = stts751_intervals[priv->interval] / 1000;
jiffies and the parameter for time_after() are in HZ. Does this
assume that HZ=1000 ? msecs_to_jiffies() might be more appropriate.
> +
> + mutex_lock(&priv->access_lock);
> + if (time_after(jiffies, priv->last_update + cache_time) ||
> + !priv->data_valid) {
> + priv->data_valid = true;
Not really, only after stts751_update_temp() succeeded.
> + priv->last_update = jiffies;
Same here.
> +
> + ret = stts751_update_temp(priv);
> + if (ret)
> + goto exit;
> +
> + ret = stts751_update_alert(priv);
if (!ret)
ret = stts751_update_alert(priv);
would avoid the goto.
> + }
> +exit:
> + mutex_unlock(&priv->access_lock);
> +
> + return ret;
> +}
> +
> +static ssize_t show_max_alarm(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + int ret;
> + struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> + ret = stts751_update(priv);
> + if (ret < 0)
> + return ret;
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->max_alert);
> +}
> +
> +static ssize_t show_min_alarm(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + int ret;
> + struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> + ret = stts751_update(priv);
> + if (ret < 0)
> + return ret;
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->min_alert);
> +}
> +
> +static ssize_t show_input(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + int ret;
> + struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> + ret = stts751_update(priv);
> + if (ret < 0)
> + return ret;
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->temp);
> +}
> +
> +static ssize_t show_therm(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->therm);
> +}
> +
> +static int do_set_hyst(struct stts751_priv *priv, int temp)
> +{
> + /* HW works in range -64C to +127.937C */
> + temp = clamp_val(temp, -64000, priv->therm);
> + priv->hyst = temp;
> + dev_dbg(priv->dev, "setting hyst %d", temp);
> + temp = priv->therm - temp;
> +
> + return stts751_set_temp_reg8(priv, temp, STTS751_REG_HYST);
> +}
> +
> +static ssize_t set_therm(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + int ret;
> + long temp;
> + struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> + if (kstrtol(buf, 10, &temp) < 0)
> + return -EINVAL;
> + /* HW works in range -64C to +127.937C */
> + temp = clamp_val(temp, -64000, 127937);
> + ret = stts751_set_temp_reg8(priv, temp, STTS751_REG_TLIM);
> + if (ret)
> + return ret;
> +
> + dev_dbg(dev, "setting therm %ld", temp);
> + priv->therm = temp;
> +
> + /*
> + * hysteresis reg is relative to therm, so we need to update
> + * it as well.
> + */
> + ret = do_set_hyst(priv, priv->hyst);
The basic expectation here is that the hysteresis value changes automatically.
If the old hysteresis was 8 degrees below the limit, it should still be 8
degrees below the new limit. This code tries to restore the old (absolute)
value, which hardly ever makes any sense.
> + if (ret)
> + return ret;
> +
> + return count;
> +}
> +
> +static ssize_t show_hyst(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->hyst);
> +}
> +
> +static ssize_t set_hyst(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + int ret;
> + long temp;
> +
> + struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> + if (kstrtol(buf, 10, &temp) < 0)
> + return -EINVAL;
> + ret = do_set_hyst(priv, temp);
> + if (ret)
> + return ret;
> + return count;
> +}
> +
> +static ssize_t show_therm_trip(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int ret;
> + struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> + ret = stts751_update(priv);
> + if (ret < 0)
> + return ret;
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->therm_trip);
> +}
> +
> +static ssize_t show_max(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_max);
> +}
> +
> +static ssize_t set_max(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + int ret;
> + long temp;
> + struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> + if (kstrtol(buf, 10, &temp) < 0)
> + return -EINVAL;
> + /* HW works in range -64C to +127.937C */
> + temp = clamp_val(temp, priv->event_min, 127937);
> + ret = stts751_set_temp_reg16(priv, temp,
> + STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
> + if (ret)
> + return ret;
> +
> + dev_dbg(dev, "setting event max %ld", temp);
> + priv->event_max = temp;
As mentioned above, the writes are all racy. Two writes in parallel may
result in inconsistent register values vs. values written into event_max etc.
> + return count;
> +}
> +
> +static ssize_t show_min(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_min);
> +}
> +
> +static ssize_t set_min(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + int ret;
> + long temp;
> + struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> + if (kstrtol(buf, 10, &temp) < 0)
> + return -EINVAL;
> + /* HW works in range -64C to +127.937C */
> + temp = clamp_val(temp, -64000, priv->event_max);
> + ret = stts751_set_temp_reg16(priv, temp,
> + STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);
> + if (ret)
> + return ret;
> +
> + dev_dbg(dev, "setting event min %ld", temp);
> +
> + priv->event_min = temp;
> + return count;
> +}
> +
> +static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> + return snprintf(buf, PAGE_SIZE - 1, "%d\n",
> + stts751_intervals[priv->interval]);
> +}
> +
> +static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + unsigned long val;
> + int idx;
> + int ret = 0;
> + struct stts751_priv *priv = dev_get_drvdata(dev);
> +
> + if (kstrtoul(buf, 10, &val) < 0)
> + return -EINVAL;
> +
> + idx = find_closest_descending(val, stts751_intervals,
> + ARRAY_SIZE(stts751_intervals));
> +
> + dev_dbg(dev, "setting interval. req:%lu, idx: %d, val: %d", val, idx,
> + stts751_intervals[idx]);
> +
> + if (priv->interval == idx)
> + return count;
> +
> + mutex_lock(&priv->access_lock);
> +
> + /*
> + * In early development stages I've become suspicious about the chip
> + * starting to misbehave if I ever set, even briefly, an invalid
> + * configuration. While I'm not sure this is really needed, be
> + * conservative and set rate/resolution in such an order that avoids
> + * passing through an invalid configuration.
> + */
> +
> + /* speed up: lower the resolution, then modify convrate */
> + if (priv->interval < idx) {
> + priv->interval = idx;
> + ret = stts751_adjust_resolution(priv);
> + if (ret)
> + goto exit;
> + }
> +
> + ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_RATE, idx);
> + if (ret)
> + goto exit;
> + /* slow down: modify convrate, then raise resolution */
> + if (priv->interval != idx) {
> + priv->interval = idx;
> + ret = stts751_adjust_resolution(priv);
> + if (ret)
> + goto exit;
> + }
> +exit:
> + mutex_unlock(&priv->access_lock);
> +
> + return count;
> +}
> +
> +static int stts751_detect(struct i2c_client *new_client,
> + struct i2c_board_info *info)
> +{
> + struct i2c_adapter *adapter = new_client->adapter;
> + const char *name;
> + int mfg_id, prod_id, rev_id;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> + return -ENODEV;
> +
> + mfg_id = i2c_smbus_read_byte_data(new_client, ST_MAN_ID);
> + if (mfg_id != ST_MAN_ID)
> + return -ENODEV;
> +
> + prod_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_PROD_ID);
> +
> + switch (prod_id) {
> + case STTS751_0_PROD_ID:
> + name = "STTS751-0";
> + break;
> + case STTS751_1_PROD_ID:
> + name = "STTS751-1";
> + break;
> + default:
> + return -ENODEV;
> + }
> + dev_dbg(&new_client->dev, "Chip %s detected!", name);
I'll never understand what the "!" in such messages is for ;-).
> +
> + rev_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_REV_ID);
> + if (rev_id < 0)
> + return -ENODEV;
> + if (rev_id != 0x1) {
> + dev_notice(&new_client->dev,
> + "Chip revision 0x%x is untested\nPlease report whether it works to andrea.merello@gmail.com",
> + rev_id);
> + }
> +
> + strlcpy(info->type, stts751_id[0].name, I2C_NAME_SIZE);
> + return 0;
> +}
> +
> +static int stts751_read_chip_config(struct stts751_priv *priv)
> +{
> + int ret;
> + int tmp;
> +
> + ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_CONF);
> + if (ret < 0)
> + return ret;
> + priv->config = ret;
> + priv->res = (ret & STTS751_CONF_RES_MASK) >> STTS751_CONF_RES_SHIFT;
> +
> + ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_RATE);
> + if (ret < 0)
> + return ret;
> + priv->interval = ret;
> +
> + ret = stts751_read_reg16(priv, &priv->event_max,
> + STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
> + if (ret)
> + return ret;
> +
> + ret = stts751_read_reg16(priv, &priv->event_min,
> + STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);
> + if (ret)
> + return ret;
> +
> + ret = stts751_read_reg8(priv, &priv->therm, STTS751_REG_TLIM);
> + if (ret)
> + return ret;
> +
> + ret = stts751_read_reg8(priv, &tmp, STTS751_REG_HYST);
> + if (ret)
> + return ret;
> + priv->hyst = priv->therm - tmp;
> +
> + return ret;
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_min, set_min, 0);
> +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_max, set_max, 0);
> +static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IRUGO, show_min_alarm, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, show_max_alarm, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_therm,
> + set_therm, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_hyst,
> + set_hyst, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_therm_trip, NULL, 0);
> +static SENSOR_DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO,
> + show_interval, set_interval, 0);
> +
> +static struct attribute *stts751_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,
> + &sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp1_crit.dev_attr.attr,
> + &sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
> + &sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
> + &sensor_dev_attr_update_interval.dev_attr.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(stts751);
> +
> +static int stts751_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct stts751_priv *priv;
> + int ret;
> + bool smbus_to;
> +
> + priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->client = client;
> + i2c_set_clientdata(client, priv);
> + mutex_init(&priv->access_lock);
> +
> + if (device_property_present(&client->dev,
> + "smbus-timeout-disable")) {
> + smbus_to = !device_property_read_bool(&client->dev,
> + "smbus-timeout-disable");
> +
> + ret = i2c_smbus_write_byte_data(priv->client,
> + STTS751_REG_SMBUS_TO,
> + smbus_to ? 0x80 : 0);
> + if (ret)
> + return ret;
> + }
> +
> + ret = stts751_read_chip_config(priv);
> + if (ret)
> + return ret;
> +
> + priv->config &= ~(STTS751_CONF_STOP | STTS751_CONF_EVENT_DIS);
> + ret = i2c_smbus_write_byte_data(priv->client,
> + STTS751_REG_CONF, priv->config);
> + if (ret)
> + return ret;
> +
> + priv->dev = devm_hwmon_device_register_with_groups(&client->dev,
> + client->name, priv,
> + stts751_groups);
> + return PTR_ERR_OR_ZERO(priv->dev);
> +}
> +
> +MODULE_DEVICE_TABLE(i2c, stts751_id);
> +
> +static struct i2c_driver stts751_driver = {
> + .class = I2C_CLASS_HWMON,
> + .driver = {
> + .name = DEVNAME,
> + },
> + .probe = stts751_probe,
> + .id_table = stts751_id,
> + .detect = stts751_detect,
> + .alert = stts751_alert,
> + .address_list = normal_i2c,
> +};
> +
> +module_i2c_driver(stts751_driver);
> +
> +MODULE_AUTHOR("Andrea Merello <andrea.merello@gmail.com>");
> +MODULE_DESCRIPTION("STTS751 sensor driver");
> +MODULE_LICENSE("GPL");
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] hwmon: new driver for ST stts751 thermal sensor
2017-01-24 20:18 ` [PATCH v3 1/2] hwmon: new driver for ST stts751 thermal sensor Guenter Roeck
@ 2017-01-25 9:51 ` Andrea Merello
2017-01-25 10:53 ` Guenter Roeck
0 siblings, 1 reply; 7+ messages in thread
From: Andrea Merello @ 2017-01-25 9:51 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, LABBE Corentin, Jean Delvare
On Tue, Jan 24, 2017 at 9:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Tue, Jan 24, 2017 at 04:02:20PM +0100, Andrea Merello wrote:
>> This patch adds a HWMON driver for ST Microelectronics STTS751
>> temperature sensors.
>>
>> It does support manual-triggered conversions as well as automatic
>> conversions. The latter is used when the "event" or "therm" function
>> is present (declaring the physical wire is attached in the DT).
>>
>> Thanks-to: LABBE Corentin [for suggestions]
>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>> Cc: LABBE Corentin <clabbe.montjoie@gmail.com>
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> Cc: Jean Delvare <jdelvare@suse.com>
>> ---
>
> No change log, meaning we have to review the entire driver again or look up
> older versions and comments ourselves. This will take a while. For now, just a
> few quick commnents. For the future, please consider providing a changelog.
Ok, of course. I'll do.
BTW most I've done is basically what we discussed; If you want I can
provide you the list of commits (before squashing/rebase), that should
be basically similar to a changelog.
>> drivers/hwmon/Kconfig | 10 +
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/stts751.c | 809 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 820 insertions(+)
>> create mode 100644 drivers/hwmon/stts751.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index d1f82f2..8fdd241 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1448,6 +1448,16 @@ config SENSORS_SCH5636
>> This driver can also be built as a module. If so, the module
>> will be called sch5636.
>>
>> +config SENSORS_STTS751
>> + tristate "ST Microelectronics STTS751"
>> + depends on I2C
>> + help
>> + If you say yes here you get support for STTS751
>> + temperature sensor chips.
>> +
>> + This driver can also be built as a module. If so, the module
>> + will be called stts751.
>> +
>> config SENSORS_SMM665
>> tristate "Summit Microelectronics SMM665"
>> depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 7476b22..1114130 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -147,6 +147,7 @@ obj-$(CONFIG_SENSORS_SMM665) += smm665.o
>> obj-$(CONFIG_SENSORS_SMSC47B397)+= smsc47b397.o
>> obj-$(CONFIG_SENSORS_SMSC47M1) += smsc47m1.o
>> obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>> +obj-$(CONFIG_SENSORS_STTS751) += stts751.o
>> obj-$(CONFIG_SENSORS_AMC6821) += amc6821.o
>> obj-$(CONFIG_SENSORS_TC74) += tc74.o
>> obj-$(CONFIG_SENSORS_THMC50) += thmc50.o
>> diff --git a/drivers/hwmon/stts751.c b/drivers/hwmon/stts751.c
>> new file mode 100644
>> index 0000000..0d4716f
>> --- /dev/null
>> +++ b/drivers/hwmon/stts751.c
>> @@ -0,0 +1,809 @@
>> +/*
>> + * STTS751 sensor driver
>> + *
>> + * Copyright (C) 2016-2017 Istituto Italiano di Tecnologia - RBCS - EDL
>> + * Robotics, Brain and Cognitive Sciences department
>> + * Electronic Design Laboratory
>> + *
>> + * Written by Andrea Merello <andrea.merello@gmail.com>
>> + *
>> + * Based on LM95241 driver and LM90 driver
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/err.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/i2c.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/property.h>
>> +#include <linux/slab.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/util_macros.h>
>> +
>> +#define DEVNAME "stts751"
>> +
>> +static const unsigned short normal_i2c[] = {
>> + 0x48, 0x49, 0x38, 0x39, /* STTS751-0 */
>> + 0x4A, 0x4B, 0x3A, 0x3B, /* STTS751-1 */
>> + I2C_CLIENT_END };
>> +
>> +#define STTS751_REG_TEMP_H 0x00
>> +#define STTS751_REG_STATUS 0x01
>> +#define STTS751_STATUS_TRIPT BIT(0)
>> +#define STTS751_STATUS_TRIPL BIT(5)
>> +#define STTS751_STATUS_TRIPH BIT(6)
>> +#define STTS751_REG_TEMP_L 0x02
>> +#define STTS751_REG_CONF 0x03
>> +#define STTS751_CONF_RES_MASK 0x0C
>> +#define STTS751_CONF_RES_SHIFT 2
>> +#define STTS751_CONF_EVENT_DIS BIT(7)
>> +#define STTS751_CONF_STOP BIT(6)
>> +#define STTS751_REG_RATE 0x04
>> +#define STTS751_REG_HLIM_H 0x05
>> +#define STTS751_REG_HLIM_L 0x06
>> +#define STTS751_REG_LLIM_H 0x07
>> +#define STTS751_REG_LLIM_L 0x08
>> +#define STTS751_REG_TLIM 0x20
>> +#define STTS751_REG_HYST 0x21
>> +#define STTS751_REG_SMBUS_TO 0x22
>> +
>> +#define STTS751_REG_PROD_ID 0xFD
>> +#define STTS751_REG_MAN_ID 0xFE
>> +#define STTS751_REG_REV_ID 0xFF
>> +
>> +#define STTS751_0_PROD_ID 0x00
>> +#define STTS751_1_PROD_ID 0x01
>> +#define ST_MAN_ID 0x53
>> +
>> +/*
>> + * Possible update intervals are (in mS):
>> + * 16000, 8000, 4000, 2000, 1000, 500, 250, 125, 62.5, 31.25
>> + * However we are not going to complicate things too much and we stick to the
>> + * approx value in mS.
>> + */
>> +static const int stts751_intervals[] = {
>> + 16000, 8000, 4000, 2000, 1000, 500, 250, 125, 63, 31
>> +};
>> +
>> +static const struct i2c_device_id stts751_id[] = {
>> + { "stts751", 0 },
>> + { }
>> +};
>> +
>> +struct stts751_priv {
>> + struct device *dev;
>> + struct i2c_client *client;
>> + struct mutex access_lock;
>> + u8 interval;
>> + int res;
>> + int event_max, event_min;
>> + int therm;
>> + int hyst;
>> + bool smbus_timeout;
>> + int temp;
>> + unsigned long last_update, last_alert_update;
>> + u8 config;
>> + bool min_alert, max_alert, therm_trip;
>> + bool data_valid, alert_valid;
>> +};
>> +
>> +/*
>> + * These functions converts temperature from HW format to integer format and
>> + * vice-vers. They are (mostly) taken from lm90 driver. Unit is in mC.
>> + */
>> +static int stts751_to_deg(s32 hw_val)
>> +{
>> + return hw_val * 125 / 32;
>> +}
>> +
>> +static s32 stts751_to_hw(int val)
>> +{
>> + s32 hw_val;
>> +
>> + if (val < 0)
>> + hw_val = (val - 62) / 125 * 32;
>> + else
>> + hw_val = (val + 62) / 125 * 32;
>> +
>
> How about "return DIV_ROUND_CLOSEST(val, 125) * 32;" ?
I admit that I stale this from lm90 without caring too much.. Now,
looking at it, I realized that we could also improve the calculation
precision by performing the multiplication before the division:
DIV_ROUND_CLOSEST(val * 32, 125);
(I think doing in this way makes rounding almost useless. Maybe it has
a slight effect.)
>> +}
>> +
>> +static int stts751_adjust_resolution(struct stts751_priv *priv)
>> +{
>> + u8 res;
>> +
>> + switch (priv->interval) {
>> + case 9:
>> + /* 10 bits */
>> + res = 0;
>> + break;
>> + case 8:
>> + /* 11 bits */
>> + res = 1;
>> + break;
>> + default:
>> + /* 12 bits */
>> + res = 3;
>> + break;
>> + }
>> +
>> + if (priv->res == res)
>> + return 0;
>> +
>> + priv->config &= ~STTS751_CONF_RES_MASK;
>> + priv->config |= res << STTS751_CONF_RES_SHIFT;
>> +
>> + return i2c_smbus_write_byte_data(priv->client,
>> + STTS751_REG_CONF, priv->config);
>> +}
>> +
>> +static int stts751_update_temp(struct stts751_priv *priv)
>> +{
>> + s32 integer1, integer2, frac;
>> + int ret = 0;
>> +
>> + /*
>> + * There is a trick here, like in the lm90 driver. We have to read two
>> + * registers to get the sensor temperature, but we have to beware a
>> + * conversion could occur between the readings. We could use the
>> + * one-shot conversion register, but we don't want to do this (disables
>> + * hardware monitoring). So the solution used here is to read the high
>> + * byte once, then the low byte, then the high byte again. If the new
>> + * high byte matches the old one, then we have a valid reading. Else we
>> + * have to read the low byte again, and now we believe we have a correct
>> + * reading.
>> + */
>> + integer1 = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_H);
>> + if (integer1 < 0) {
>> + dev_dbg(&priv->client->dev,
>> + "I2C read failed (temp H). ret: %x\n", ret);
>> + ret = integer1;
>> + goto exit;
>> + }
>> +
>> + frac = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_L);
>> + if (frac < 0) {
>> + dev_dbg(&priv->client->dev,
>> + "I2C read failed (temp L). ret: %x\n", ret);
>> + ret = frac;
>> + goto exit;
>> + }
>> +
>> + integer2 = i2c_smbus_read_byte_data(priv->client, STTS751_REG_TEMP_H);
>> + if (integer2 < 0) {
>> + dev_dbg(&priv->client->dev,
>> + "I2C 2nd read failed (temp H). ret: %x\n", ret);
>> + ret = integer2;
>> + goto exit;
>> + }
>> +
>> + if (integer1 != integer2) {
>> + frac = i2c_smbus_read_byte_data(priv->client,
>> + STTS751_REG_TEMP_L);
>> + if (frac < 0) {
>> + dev_dbg(&priv->client->dev,
>> + "I2C 2nd read failed (temp L). ret: %x\n", ret);
>> + ret = frac;
>> + goto exit;
>> + }
>> + }
>> +
>> +exit:
>> + if (ret)
>> + return ret;
>
> If the code can return immediately, please do so.
OK
>> +
>> + priv->temp = stts751_to_deg((integer1 << 8) | frac);
>> + return ret;
>> +}
>> +
>> +static int stts751_set_temp_reg16(struct stts751_priv *priv, int temp,
>> + u8 hreg, u8 lreg)
>> +{
>> + s32 hwval;
>> + int ret;
>> +
>> + hwval = stts751_to_hw(temp);
>> +
>> + mutex_lock(&priv->access_lock);
>> + ret = i2c_smbus_write_byte_data(priv->client, hreg, hwval >> 8);
>> + if (!ret)
>> + ret = i2c_smbus_write_byte_data(priv->client, lreg,
>> + hwval & 0xff);
>> + mutex_unlock(&priv->access_lock);
>> +
>> + return ret;
>> +}
>> +
>> +static int stts751_set_temp_reg8(struct stts751_priv *priv, int temp, u8 reg)
>> +{
>> + s32 hwval;
>> + int ret;
>> +
>> + hwval = stts751_to_hw(temp);
>> +
>> + mutex_lock(&priv->access_lock);
>> + ret = i2c_smbus_write_byte_data(priv->client, reg, hwval >> 8);
>> + mutex_unlock(&priv->access_lock);
>
> The lock is in the wrong location. It needs to be in the calling code,
> since the calling code updates context variables. The same is true for
> stts751_set_temp_reg16().
Oops, you are right.. will fix.
>> +
>> + return ret;
>> +}
>> +
>> +static int stts751_read_reg16(struct stts751_priv *priv, int *temp,
>> + u8 hreg, u8 lreg)
>> +{
>> + int integer, frac;
>> +
>> + integer = i2c_smbus_read_byte_data(priv->client, hreg);
>> + if (integer < 0)
>> + return integer;
>> +
>> + frac = i2c_smbus_read_byte_data(priv->client, lreg);
>> + if (frac < 0)
>> + return frac;
>> +
>> + *temp = stts751_to_deg((integer << 8) | frac);
>> +
>> + return 0;
>> +}
>> +
>> +static int stts751_read_reg8(struct stts751_priv *priv, int *temp, u8 reg)
>> +{
>> + int integer;
>> +
>> + integer = i2c_smbus_read_byte_data(priv->client, reg);
>> + if (integer < 0)
>> + return integer;
>> +
>> + *temp = stts751_to_deg(integer << 8);
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Update alert flags without waiting for cache to expire. We detects alerts
>> + * immediately for the sake of the alert handler; we still need to deal with
>> + * caching to workaround the fact that the status register gets cleared when
>
> Did we have this before ? The datasheet claims that the status is only cleared
> if the condition no longer exists. If that is incorrect, please explain here,
> or it will come up again.
Yes, we already discussed this. The bit gets always cleared on read;
it will be eventually re-asserted on next conversion. The comment is
here exactly to clarify this. I'll add something like "despite what
the datasheet claims"..
>> + * reading it.
>> + */
>> +static int stts751_update_alert(struct stts751_priv *priv)
>> +{
>> + int ret;
>> + bool conv_done;
>> + int cache_time =
>> + DIV_ROUND_UP(stts751_intervals[priv->interval] * HZ, 1000);
>
> How about using msecs_to_jiffies() ?
OK
>
>> +
>> + /*
>> + * Add another 10% because if we run faster than the HW conversion
>> + * rate we will end up in reporting incorrectly alarms.
>> + */
>> + cache_time += cache_time / 10;
>> +
>> + ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_STATUS);
>> + if (ret < 0)
>> + return ret;
>> +
>> + dev_dbg(priv->dev, "status reg %x\n", ret);
>> + conv_done = ret & (STTS751_STATUS_TRIPH | STTS751_STATUS_TRIPL);
>> + /*
>> + * Reset the cache if the cache time expired, or if we are sure
>> + * we have valid data from a device conversion, or if we know
>> + * our cache has been never written.
>> + *
>> + * Note that when the cache has been never written the point is
>> + * to correctly initialize the timestamp, rather than clearing
>> + * the cache values.
>> + *
>> + * Note that updating the cache timestamp when we get an alarm flag
>> + * is required, otherwise we could incorrectly report alarms to be zero.
>> + */
>> + if (time_after(jiffies, priv->last_alert_update + cache_time) ||
>> + conv_done || !priv->alert_valid) {
>> + priv->max_alert = false;
>> + priv->min_alert = false;
>> + priv->alert_valid = true;
>> + priv->last_alert_update = jiffies;
>> + dev_dbg(priv->dev, "invalidating alert cache\n");
>> + }
>> +
>> + priv->max_alert |= !!(ret & STTS751_STATUS_TRIPH);
>> + priv->min_alert |= !!(ret & STTS751_STATUS_TRIPL);
>> + priv->therm_trip = !!(ret & STTS751_STATUS_TRIPT);
>> +
>> + dev_dbg(priv->dev, "max_alert: %d, min_alert: %d, therm_trip: %d\n",
>> + priv->max_alert, priv->min_alert, priv->therm_trip);
>> +
>> + return 0;
>> +}
>> +
>> +static void stts751_alert(struct i2c_client *client,
>> + enum i2c_alert_protocol type, unsigned int data)
>> +{
>> + int ret;
>> + struct stts751_priv *priv = i2c_get_clientdata(client);
>> +
>> + if (type != I2C_PROTOCOL_SMBUS_ALERT)
>> + return;
>> +
>> + dev_dbg(&client->dev, "alert!");
>> +
>> + mutex_lock(&priv->access_lock);
>> + ret = stts751_update_alert(priv);
>> + if (ret < 0) {
>> + /* default to worst case */
>> + priv->max_alert = true;
>> + priv->min_alert = true;
>> +
>> + dev_warn(&priv->client->dev,
>> + "Alert received, but can't communicate to the device. Something bad happening? Triggering all alarms!");
>> + }
>> +
>> + if (priv->max_alert) {
>> + dev_notice(&client->dev, "got alert for HIGH temperature");
>> +
>> + /* unblock alert poll */
>> + sysfs_notify(&priv->dev->kobj, NULL, "temp1_event_max_alert");
>> + kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
>> + }
>> +
>> + if (priv->min_alert) {
>> + dev_notice(&client->dev, "got alert for LOW temperature");
>> +
>> + /* unblock alert poll */
>> + sysfs_notify(&priv->dev->kobj, NULL, "temp1_event_min_alert");
>> + kobject_uevent(&priv->dev->kobj, KOBJ_CHANGE);
>
> This way you can get two uevents, which is really undesirable.
> Maybe move uevent creation into a separate if().
OK
>> + }
>> + mutex_unlock(&priv->access_lock);
>> +}
>> +
>> +static int stts751_update(struct stts751_priv *priv)
>> +{
>> + int ret = 0;
>> + int cache_time = stts751_intervals[priv->interval] / 1000;
>
> jiffies and the parameter for time_after() are in HZ. Does this
> assume that HZ=1000 ? msecs_to_jiffies() might be more appropriate.
Oops, this is wrong. msecs_to_jiffies() is needed, as you said.
>> +
>> + mutex_lock(&priv->access_lock);
>> + if (time_after(jiffies, priv->last_update + cache_time) ||
>> + !priv->data_valid) {
>> + priv->data_valid = true;
>
> Not really, only after stts751_update_temp() succeeded.
>
>> + priv->last_update = jiffies;
>
> Same here.
OK. We could even move them after stts751_update_alert()
>> +
>> + ret = stts751_update_temp(priv);
>> + if (ret)
>> + goto exit;
>> +
>> + ret = stts751_update_alert(priv);
>
> if (!ret)
> ret = stts751_update_alert(priv);
>
> would avoid the goto.
OK
>> + }
>> +exit:
>> + mutex_unlock(&priv->access_lock);
>> +
>> + return ret;
>> +}
>> +
>> +static ssize_t show_max_alarm(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + int ret;
>> + struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> + ret = stts751_update(priv);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->max_alert);
>> +}
>> +
>> +static ssize_t show_min_alarm(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + int ret;
>> + struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> + ret = stts751_update(priv);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->min_alert);
>> +}
>> +
>> +static ssize_t show_input(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + int ret;
>> + struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> + ret = stts751_update(priv);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->temp);
>> +}
>> +
>> +static ssize_t show_therm(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->therm);
>> +}
>> +
>> +static int do_set_hyst(struct stts751_priv *priv, int temp)
>> +{
>> + /* HW works in range -64C to +127.937C */
>> + temp = clamp_val(temp, -64000, priv->therm);
>> + priv->hyst = temp;
>> + dev_dbg(priv->dev, "setting hyst %d", temp);
>> + temp = priv->therm - temp;
>> +
>> + return stts751_set_temp_reg8(priv, temp, STTS751_REG_HYST);
>> +}
>> +
>> +static ssize_t set_therm(struct device *dev, struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + int ret;
>> + long temp;
>> + struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> + if (kstrtol(buf, 10, &temp) < 0)
>> + return -EINVAL;
>> + /* HW works in range -64C to +127.937C */
>> + temp = clamp_val(temp, -64000, 127937);
>> + ret = stts751_set_temp_reg8(priv, temp, STTS751_REG_TLIM);
>> + if (ret)
>> + return ret;
>> +
>> + dev_dbg(dev, "setting therm %ld", temp);
>> + priv->therm = temp;
>> +
>> + /*
>> + * hysteresis reg is relative to therm, so we need to update
>> + * it as well.
>> + */
>> + ret = do_set_hyst(priv, priv->hyst);
>
> The basic expectation here is that the hysteresis value changes automatically.
> If the old hysteresis was 8 degrees below the limit, it should still be 8
> degrees below the new limit. This code tries to restore the old (absolute)
> value, which hardly ever makes any sense.
Ah, OK.
>> + if (ret)
>> + return ret;
>> +
>> + return count;
>> +}
>> +
>> +static ssize_t show_hyst(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->hyst);
>> +}
>> +
>> +static ssize_t set_hyst(struct device *dev, struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + int ret;
>> + long temp;
>> +
>> + struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> + if (kstrtol(buf, 10, &temp) < 0)
>> + return -EINVAL;
>> + ret = do_set_hyst(priv, temp);
>> + if (ret)
>> + return ret;
>> + return count;
>> +}
>> +
>> +static ssize_t show_therm_trip(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + int ret;
>> + struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> + ret = stts751_update(priv);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->therm_trip);
>> +}
>> +
>> +static ssize_t show_max(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_max);
>> +}
>> +
>> +static ssize_t set_max(struct device *dev, struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + int ret;
>> + long temp;
>> + struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> + if (kstrtol(buf, 10, &temp) < 0)
>> + return -EINVAL;
>> + /* HW works in range -64C to +127.937C */
>> + temp = clamp_val(temp, priv->event_min, 127937);
>> + ret = stts751_set_temp_reg16(priv, temp,
>> + STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
>> + if (ret)
>> + return ret;
>> +
>> + dev_dbg(dev, "setting event max %ld", temp);
>> + priv->event_max = temp;
>
> As mentioned above, the writes are all racy. Two writes in parallel may
> result in inconsistent register values vs. values written into event_max etc.
OK
>> + return count;
>> +}
>> +
>> +static ssize_t show_min(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> + return snprintf(buf, PAGE_SIZE - 1, "%d\n", priv->event_min);
>> +}
>> +
>> +static ssize_t set_min(struct device *dev, struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + int ret;
>> + long temp;
>> + struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> + if (kstrtol(buf, 10, &temp) < 0)
>> + return -EINVAL;
>> + /* HW works in range -64C to +127.937C */
>> + temp = clamp_val(temp, -64000, priv->event_max);
>> + ret = stts751_set_temp_reg16(priv, temp,
>> + STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);
>> + if (ret)
>> + return ret;
>> +
>> + dev_dbg(dev, "setting event min %ld", temp);
>> +
>> + priv->event_min = temp;
>> + return count;
>> +}
>> +
>> +static ssize_t show_interval(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> + return snprintf(buf, PAGE_SIZE - 1, "%d\n",
>> + stts751_intervals[priv->interval]);
>> +}
>> +
>> +static ssize_t set_interval(struct device *dev, struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + unsigned long val;
>> + int idx;
>> + int ret = 0;
>> + struct stts751_priv *priv = dev_get_drvdata(dev);
>> +
>> + if (kstrtoul(buf, 10, &val) < 0)
>> + return -EINVAL;
>> +
>> + idx = find_closest_descending(val, stts751_intervals,
>> + ARRAY_SIZE(stts751_intervals));
>> +
>> + dev_dbg(dev, "setting interval. req:%lu, idx: %d, val: %d", val, idx,
>> + stts751_intervals[idx]);
>> +
>> + if (priv->interval == idx)
>> + return count;
>> +
>> + mutex_lock(&priv->access_lock);
>> +
>> + /*
>> + * In early development stages I've become suspicious about the chip
>> + * starting to misbehave if I ever set, even briefly, an invalid
>> + * configuration. While I'm not sure this is really needed, be
>> + * conservative and set rate/resolution in such an order that avoids
>> + * passing through an invalid configuration.
>> + */
>> +
>> + /* speed up: lower the resolution, then modify convrate */
>> + if (priv->interval < idx) {
>> + priv->interval = idx;
>> + ret = stts751_adjust_resolution(priv);
>> + if (ret)
>> + goto exit;
>> + }
>> +
>> + ret = i2c_smbus_write_byte_data(priv->client, STTS751_REG_RATE, idx);
>> + if (ret)
>> + goto exit;
>> + /* slow down: modify convrate, then raise resolution */
>> + if (priv->interval != idx) {
>> + priv->interval = idx;
>> + ret = stts751_adjust_resolution(priv);
>> + if (ret)
>> + goto exit;
>> + }
>> +exit:
>> + mutex_unlock(&priv->access_lock);
>> +
>> + return count;
>> +}
>> +
>> +static int stts751_detect(struct i2c_client *new_client,
>> + struct i2c_board_info *info)
>> +{
>> + struct i2c_adapter *adapter = new_client->adapter;
>> + const char *name;
>> + int mfg_id, prod_id, rev_id;
>> +
>> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
>> + return -ENODEV;
>> +
>> + mfg_id = i2c_smbus_read_byte_data(new_client, ST_MAN_ID);
>> + if (mfg_id != ST_MAN_ID)
>> + return -ENODEV;
>> +
>> + prod_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_PROD_ID);
>> +
>> + switch (prod_id) {
>> + case STTS751_0_PROD_ID:
>> + name = "STTS751-0";
>> + break;
>> + case STTS751_1_PROD_ID:
>> + name = "STTS751-1";
>> + break;
>> + default:
>> + return -ENODEV;
>> + }
>> + dev_dbg(&new_client->dev, "Chip %s detected!", name);
>
> I'll never understand what the "!" in such messages is for ;-).
It's the driver in early debug stages that gets excited because it
just found it's beloved hardware ;-) ..But I'm going to drop it :)
>> +
>> + rev_id = i2c_smbus_read_byte_data(new_client, STTS751_REG_REV_ID);
>> + if (rev_id < 0)
>> + return -ENODEV;
>> + if (rev_id != 0x1) {
>> + dev_notice(&new_client->dev,
>> + "Chip revision 0x%x is untested\nPlease report whether it works to andrea.merello@gmail.com",
>> + rev_id);
>> + }
>> +
>> + strlcpy(info->type, stts751_id[0].name, I2C_NAME_SIZE);
>> + return 0;
>> +}
>> +
>> +static int stts751_read_chip_config(struct stts751_priv *priv)
>> +{
>> + int ret;
>> + int tmp;
>> +
>> + ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_CONF);
>> + if (ret < 0)
>> + return ret;
>> + priv->config = ret;
>> + priv->res = (ret & STTS751_CONF_RES_MASK) >> STTS751_CONF_RES_SHIFT;
>> +
>> + ret = i2c_smbus_read_byte_data(priv->client, STTS751_REG_RATE);
>> + if (ret < 0)
>> + return ret;
>> + priv->interval = ret;
>> +
>> + ret = stts751_read_reg16(priv, &priv->event_max,
>> + STTS751_REG_HLIM_H, STTS751_REG_HLIM_L);
>> + if (ret)
>> + return ret;
>> +
>> + ret = stts751_read_reg16(priv, &priv->event_min,
>> + STTS751_REG_LLIM_H, STTS751_REG_LLIM_L);
>> + if (ret)
>> + return ret;
>> +
>> + ret = stts751_read_reg8(priv, &priv->therm, STTS751_REG_TLIM);
>> + if (ret)
>> + return ret;
>> +
>> + ret = stts751_read_reg8(priv, &tmp, STTS751_REG_HYST);
>> + if (ret)
>> + return ret;
>> + priv->hyst = priv->therm - tmp;
>> +
>> + return ret;
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_min, set_min, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_max, set_max, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IRUGO, show_min_alarm, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, show_max_alarm, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_therm,
>> + set_therm, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_hyst,
>> + set_hyst, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_therm_trip, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO,
>> + show_interval, set_interval, 0);
>> +
>> +static struct attribute *stts751_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,
>> + &sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
>> + &sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
>> + &sensor_dev_attr_temp1_crit.dev_attr.attr,
>> + &sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
>> + &sensor_dev_attr_temp1_crit_alarm.dev_attr.attr,
>> + &sensor_dev_attr_update_interval.dev_attr.attr,
>> + NULL
>> +};
>> +ATTRIBUTE_GROUPS(stts751);
>> +
>> +static int stts751_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + struct stts751_priv *priv;
>> + int ret;
>> + bool smbus_to;
>> +
>> + priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + priv->client = client;
>> + i2c_set_clientdata(client, priv);
>> + mutex_init(&priv->access_lock);
>> +
>> + if (device_property_present(&client->dev,
>> + "smbus-timeout-disable")) {
>> + smbus_to = !device_property_read_bool(&client->dev,
>> + "smbus-timeout-disable");
>> +
>> + ret = i2c_smbus_write_byte_data(priv->client,
>> + STTS751_REG_SMBUS_TO,
>> + smbus_to ? 0x80 : 0);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + ret = stts751_read_chip_config(priv);
>> + if (ret)
>> + return ret;
>> +
>> + priv->config &= ~(STTS751_CONF_STOP | STTS751_CONF_EVENT_DIS);
>> + ret = i2c_smbus_write_byte_data(priv->client,
>> + STTS751_REG_CONF, priv->config);
>> + if (ret)
>> + return ret;
>> +
>> + priv->dev = devm_hwmon_device_register_with_groups(&client->dev,
>> + client->name, priv,
>> + stts751_groups);
>> + return PTR_ERR_OR_ZERO(priv->dev);
>> +}
>> +
>> +MODULE_DEVICE_TABLE(i2c, stts751_id);
>> +
>> +static struct i2c_driver stts751_driver = {
>> + .class = I2C_CLASS_HWMON,
>> + .driver = {
>> + .name = DEVNAME,
>> + },
>> + .probe = stts751_probe,
>> + .id_table = stts751_id,
>> + .detect = stts751_detect,
>> + .alert = stts751_alert,
>> + .address_list = normal_i2c,
>> +};
>> +
>> +module_i2c_driver(stts751_driver);
>> +
>> +MODULE_AUTHOR("Andrea Merello <andrea.merello@gmail.com>");
>> +MODULE_DESCRIPTION("STTS751 sensor driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] hwmon: new driver for ST stts751 thermal sensor
2017-01-25 9:51 ` Andrea Merello
@ 2017-01-25 10:53 ` Guenter Roeck
2017-01-25 11:00 ` Andrea Merello
0 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2017-01-25 10:53 UTC (permalink / raw)
To: andrea.merello; +Cc: linux-hwmon, LABBE Corentin, Jean Delvare
On 01/25/2017 01:51 AM, Andrea Merello wrote:
> On Tue, Jan 24, 2017 at 9:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On Tue, Jan 24, 2017 at 04:02:20PM +0100, Andrea Merello wrote:
>>> This patch adds a HWMON driver for ST Microelectronics STTS751
>>> temperature sensors.
>>>
>>> It does support manual-triggered conversions as well as automatic
>>> conversions. The latter is used when the "event" or "therm" function
>>> is present (declaring the physical wire is attached in the DT).
>>>
>>> Thanks-to: LABBE Corentin [for suggestions]
>>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>>> Cc: LABBE Corentin <clabbe.montjoie@gmail.com>
>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>> Cc: Jean Delvare <jdelvare@suse.com>
>>> ---
>>
[ ... ]
>>> +
>>> +static s32 stts751_to_hw(int val)
>>> +{
>>> + s32 hw_val;
>>> +
>>> + if (val < 0)
>>> + hw_val = (val - 62) / 125 * 32;
>>> + else
>>> + hw_val = (val + 62) / 125 * 32;
>>> +
>>
>> How about "return DIV_ROUND_CLOSEST(val, 125) * 32;" ?
>
> I admit that I stale this from lm90 without caring too much.. Now,
> looking at it, I realized that we could also improve the calculation
> precision by performing the multiplication before the division:
>
> DIV_ROUND_CLOSEST(val * 32, 125);
>
> (I think doing in this way makes rounding almost useless. Maybe it has
> a slight effect.)
>
The purpose of doing the multiplication afterwards is to make sure
that the lower 5 bits are 0 (or, rather, to shift the result 5 bits
to the left). Doing it first defeats that purpose, and would actually
mess up the rounding (because the lower bits will be ignored by the
hardware). For example, writing 0xff would yield 0xe0, while what
you would probably really want is to write 0x1e0 in that situation.
Guenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] hwmon: new driver for ST stts751 thermal sensor
2017-01-25 10:53 ` Guenter Roeck
@ 2017-01-25 11:00 ` Andrea Merello
2017-01-25 11:16 ` Guenter Roeck
0 siblings, 1 reply; 7+ messages in thread
From: Andrea Merello @ 2017-01-25 11:00 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, LABBE Corentin, Jean Delvare
On Wed, Jan 25, 2017 at 11:53 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 01/25/2017 01:51 AM, Andrea Merello wrote:
>>
>> On Tue, Jan 24, 2017 at 9:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> On Tue, Jan 24, 2017 at 04:02:20PM +0100, Andrea Merello wrote:
>>>>
>>>> This patch adds a HWMON driver for ST Microelectronics STTS751
>>>> temperature sensors.
>>>>
>>>> It does support manual-triggered conversions as well as automatic
>>>> conversions. The latter is used when the "event" or "therm" function
>>>> is present (declaring the physical wire is attached in the DT).
>>>>
>>>> Thanks-to: LABBE Corentin [for suggestions]
>>>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>>>> Cc: LABBE Corentin <clabbe.montjoie@gmail.com>
>>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>>> Cc: Jean Delvare <jdelvare@suse.com>
>>>> ---
>>>
>>>
>
> [ ... ]
>
>>>> +
>>>> +static s32 stts751_to_hw(int val)
>>>> +{
>>>> + s32 hw_val;
>>>> +
>>>> + if (val < 0)
>>>> + hw_val = (val - 62) / 125 * 32;
>>>> + else
>>>> + hw_val = (val + 62) / 125 * 32;
>>>> +
>>>
>>>
>>> How about "return DIV_ROUND_CLOSEST(val, 125) * 32;" ?
>>
>>
>> I admit that I stale this from lm90 without caring too much.. Now,
>> looking at it, I realized that we could also improve the calculation
>> precision by performing the multiplication before the division:
>>
>> DIV_ROUND_CLOSEST(val * 32, 125);
>>
>> (I think doing in this way makes rounding almost useless. Maybe it has
>> a slight effect.)
>>
>
> The purpose of doing the multiplication afterwards is to make sure
> that the lower 5 bits are 0 (or, rather, to shift the result 5 bits
> to the left). Doing it first defeats that purpose, and would actually
> mess up the rounding (because the lower bits will be ignored by the
> hardware). For example, writing 0xff would yield 0xe0, while what
> you would probably really want is to write 0x1e0 in that situation.
Ah, right. Sorry, while looking at the calculations I lost focus on
the specific context (hw details).
> Guenter
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] hwmon: new driver for ST stts751 thermal sensor
2017-01-25 11:00 ` Andrea Merello
@ 2017-01-25 11:16 ` Guenter Roeck
0 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2017-01-25 11:16 UTC (permalink / raw)
To: andrea.merello; +Cc: linux-hwmon, LABBE Corentin, Jean Delvare
On 01/25/2017 03:00 AM, Andrea Merello wrote:
> On Wed, Jan 25, 2017 at 11:53 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 01/25/2017 01:51 AM, Andrea Merello wrote:
>>>
>>> On Tue, Jan 24, 2017 at 9:18 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On Tue, Jan 24, 2017 at 04:02:20PM +0100, Andrea Merello wrote:
>>>>>
>>>>> This patch adds a HWMON driver for ST Microelectronics STTS751
>>>>> temperature sensors.
>>>>>
>>>>> It does support manual-triggered conversions as well as automatic
>>>>> conversions. The latter is used when the "event" or "therm" function
>>>>> is present (declaring the physical wire is attached in the DT).
>>>>>
>>>>> Thanks-to: LABBE Corentin [for suggestions]
>>>>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>>>>> Cc: LABBE Corentin <clabbe.montjoie@gmail.com>
>>>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>>>> Cc: Jean Delvare <jdelvare@suse.com>
>>>>> ---
>>>>
>>>>
>>
>> [ ... ]
>>
>>>>> +
>>>>> +static s32 stts751_to_hw(int val)
>>>>> +{
>>>>> + s32 hw_val;
>>>>> +
>>>>> + if (val < 0)
>>>>> + hw_val = (val - 62) / 125 * 32;
>>>>> + else
>>>>> + hw_val = (val + 62) / 125 * 32;
>>>>> +
>>>>
>>>>
>>>> How about "return DIV_ROUND_CLOSEST(val, 125) * 32;" ?
>>>
>>>
>>> I admit that I stale this from lm90 without caring too much.. Now,
>>> looking at it, I realized that we could also improve the calculation
>>> precision by performing the multiplication before the division:
>>>
>>> DIV_ROUND_CLOSEST(val * 32, 125);
>>>
>>> (I think doing in this way makes rounding almost useless. Maybe it has
>>> a slight effect.)
>>>
>>
>> The purpose of doing the multiplication afterwards is to make sure
>> that the lower 5 bits are 0 (or, rather, to shift the result 5 bits
>> to the left). Doing it first defeats that purpose, and would actually
>> mess up the rounding (because the lower bits will be ignored by the
>> hardware). For example, writing 0xff would yield 0xe0, while what
>> you would probably really want is to write 0x1e0 in that situation.
>
> Ah, right. Sorry, while looking at the calculations I lost focus on
> the specific context (hw details).
>
No problem, I fell into that same trap myself at some point :-)
Guenter
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-01-25 11:16 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-24 15:02 [PATCH v3 1/2] hwmon: new driver for ST stts751 thermal sensor Andrea Merello
2017-01-24 15:02 ` [PATCH v3 2/2] DT: add binding documentation for STTS751 Andrea Merello
2017-01-24 20:18 ` [PATCH v3 1/2] hwmon: new driver for ST stts751 thermal sensor Guenter Roeck
2017-01-25 9:51 ` Andrea Merello
2017-01-25 10:53 ` Guenter Roeck
2017-01-25 11:00 ` Andrea Merello
2017-01-25 11:16 ` Guenter Roeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.