* [PATCH v5 1/2] hwmon: new driver for ST stts751 thermal sensor
@ 2017-01-30 9:40 Andrea Merello
2017-01-30 9:40 ` [PATCH v5 2/2] DT: add binding documentation for STTS751 Andrea Merello
2017-01-30 14:24 ` [PATCH v5 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-30 9:40 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.
Changes since V3:
- fix sysfs_notify() filename
- simplify error exit path in stts751_update_temp(): kill useless goto
- fix set function were racy due to lock incorrectly placed
- use msecs_to_jiffies instead of manual calculation
- fix jiffies and msecs were mixed up
- don't mark cache as good unless reads really succeded to fill up it
- when updating therm, hyst keeps relative value rather than abs value
- don't should in dev_dbg when chip is detected
- don't ever generate two uevents
- clarify clear-on-read behaviour of status reg alarm flags
- simplify mDEG to HW conversion function
- fix chip stops due to incorrect resolution configuration
- fix incorrectly reading negative numbers from HW
Changes since V4:
- fix continuation lines (style, remove uncecessary, ..)
- shorten a unnecessary verbose dev_warn() message
- fix set_interval() exitpath didn't return error
- change uncecesary neg-logic
- change file symbolic permissions with octal
- move chip revision test in probe(), change dev_notice() in dev_dbg()
- use "return 0" on success (non functional change)
- add further checks in i2c detect function
- fix i2c detect function read wrong reg while checking for manufacturer ID
Thanks-to: LABBE Corentin [for suggestions]
Thanks-to: Guenter Roeck [for suggestion and discussions]
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 | 819 ++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 830 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..4154294
--- /dev/null
+++ b/drivers/hwmon/stts751.c
@@ -0,0 +1,819 @@
+/*
+ * 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(s16 hw_val)
+{
+ return hw_val * 125 / 32;
+}
+
+static s32 stts751_to_hw(int val)
+{
+ return DIV_ROUND_CLOSEST(val, 125) * 32;
+}
+
+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;
+ dev_dbg(priv->dev, "setting res %d. config %x", res, priv->config);
+ priv->res = res;
+
+ 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;
+
+ /*
+ * 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", integer1);
+ return integer1;
+ }
+
+ 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", frac);
+ return frac;
+ }
+
+ 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", integer2);
+ return integer2;
+ }
+
+ 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",
+ frac);
+ return frac;
+ }
+ }
+
+ priv->temp = stts751_to_deg((integer1 << 8) | frac);
+ return 0;
+}
+
+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);
+
+ ret = i2c_smbus_write_byte_data(priv->client, hreg, hwval >> 8);
+ if (ret)
+ return ret;
+
+ return i2c_smbus_write_byte_data(priv->client, lreg, hwval & 0xff);
+}
+
+static int stts751_set_temp_reg8(struct stts751_priv *priv, int temp, u8 reg)
+{
+ s32 hwval;
+
+ hwval = stts751_to_hw(temp);
+ return i2c_smbus_write_byte_data(priv->client, reg, hwval >> 8);
+}
+
+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 alarm flags int the status register,
+ * despite what the datasheet claims, gets always cleared on read.
+ */
+static int stts751_update_alert(struct stts751_priv *priv)
+{
+ int ret;
+ bool conv_done;
+ int cache_time = msecs_to_jiffies(stts751_intervals[priv->interval]);
+
+ /*
+ * 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. 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_max_alarm");
+ }
+
+ if (priv->min_alert) {
+ dev_notice(&client->dev, "got alert for LOW temperature");
+
+ /* unblock alert poll */
+ sysfs_notify(&priv->dev->kobj, NULL, "temp1_min_alarm");
+ }
+
+ if (priv->min_alert || priv->max_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 = msecs_to_jiffies(stts751_intervals[priv->interval]);
+
+ mutex_lock(&priv->access_lock);
+ if (time_after(jiffies, priv->last_update + cache_time) ||
+ !priv->data_valid) {
+ ret = stts751_update_temp(priv);
+ if (ret)
+ goto exit;
+
+ ret = stts751_update_alert(priv);
+ if (ret)
+ goto exit;
+ priv->data_valid = true;
+ priv->last_update = jiffies;
+ }
+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 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);
+ mutex_lock(&priv->access_lock);
+ ret = stts751_set_temp_reg8(priv, temp, STTS751_REG_TLIM);
+ if (ret)
+ goto exit;
+
+ dev_dbg(dev, "setting therm %ld", temp);
+
+ /*
+ * hysteresis reg is relative to therm, so the HW does not need to be
+ * adjusted, we need to update our local copy only.
+ */
+ priv->hyst = temp - (priv->therm - priv->hyst);
+ priv->therm = temp;
+
+exit:
+ mutex_unlock(&priv->access_lock);
+ 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;
+
+ mutex_lock(&priv->access_lock);
+ /* HW works in range -64C to +127.937C */
+ temp = clamp_val(temp, -64000, priv->therm);
+ priv->hyst = temp;
+ dev_dbg(priv->dev, "setting hyst %ld", temp);
+ temp = priv->therm - temp;
+ ret = stts751_set_temp_reg8(priv, temp, STTS751_REG_HYST);
+ mutex_unlock(&priv->access_lock);
+ 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;
+
+ mutex_lock(&priv->access_lock);
+ /* 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)
+ goto exit;
+
+ dev_dbg(dev, "setting event max %ld", temp);
+ priv->event_max = temp;
+ ret = count;
+exit:
+ mutex_unlock(&priv->access_lock);
+ return ret;
+}
+
+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;
+
+ mutex_lock(&priv->access_lock);
+ /* 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)
+ goto exit;
+
+ dev_dbg(dev, "setting event min %ld", temp);
+ priv->event_min = temp;
+ ret = count;
+exit:
+ mutex_unlock(&priv->access_lock);
+ return ret;
+}
+
+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 = count;
+ 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]);
+
+ mutex_lock(&priv->access_lock);
+ if (priv->interval == idx)
+ goto exit;
+
+ /*
+ * 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) {
+ dev_dbg(priv->dev, "lower resolution, then modify convrate");
+ 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) {
+ dev_dbg(priv->dev, "modify convrate, then raise resolution");
+ priv->interval = idx;
+ ret = stts751_adjust_resolution(priv);
+ if (ret)
+ goto exit;
+ }
+ ret = count;
+exit:
+ mutex_unlock(&priv->access_lock);
+
+ return ret;
+}
+
+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 tmp;
+
+ if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+ return -ENODEV;
+
+ tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_MAN_ID);
+ if (tmp != ST_MAN_ID)
+ return -ENODEV;
+
+ /* lower temperaure registers always have bits 0-3 set to zero */
+ tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_TEMP_L);
+ if (tmp & 0xf)
+ return -ENODEV;
+
+ tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_HLIM_L);
+ if (tmp & 0xf)
+ return -ENODEV;
+
+ tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_LLIM_L);
+ if (tmp & 0xf)
+ return -ENODEV;
+
+ /* smbus timeout register always have bits 0-7 set to zero */
+ tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_SMBUS_TO);
+ if (tmp & 0x7f)
+ return -ENODEV;
+
+ tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_PROD_ID);
+
+ switch (tmp) {
+ 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);
+
+ 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 0;
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, 0444, show_input, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_min, 0644, show_min, set_min, 0);
+static SENSOR_DEVICE_ATTR(temp1_max, 0644, show_max, set_max, 0);
+static SENSOR_DEVICE_ATTR(temp1_min_alarm, 0444, show_min_alarm, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_max_alarm, 0444, show_max_alarm, NULL, 0);
+static SENSOR_DEVICE_ATTR(temp1_crit, 0644, show_therm, set_therm, 0);
+static SENSOR_DEVICE_ATTR(temp1_crit_hyst, 0644, show_hyst, set_hyst, 0);
+static SENSOR_DEVICE_ATTR(temp1_crit_alarm, 0444, show_therm_trip, NULL, 0);
+static SENSOR_DEVICE_ATTR(update_interval, 0644,
+ 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_nto;
+ int rev_id;
+
+ 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_nto = device_property_read_bool(&client->dev,
+ "smbus-timeout-disable");
+
+ ret = i2c_smbus_write_byte_data(priv->client,
+ STTS751_REG_SMBUS_TO,
+ smbus_nto ? 0 : 0x80);
+ if (ret)
+ return ret;
+ }
+
+ rev_id = i2c_smbus_read_byte_data(priv->client, STTS751_REG_REV_ID);
+ if (rev_id < 0)
+ return -ENODEV;
+ if (rev_id != 0x1) {
+ dev_dbg(&priv->client->dev, "Chip revision 0x%x is untested\n",
+ rev_id);
+ }
+
+ 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 v5 2/2] DT: add binding documentation for STTS751
2017-01-30 9:40 [PATCH v5 1/2] hwmon: new driver for ST stts751 thermal sensor Andrea Merello
@ 2017-01-30 9:40 ` Andrea Merello
2017-01-30 14:24 ` [PATCH v5 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-30 9:40 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 v5 1/2] hwmon: new driver for ST stts751 thermal sensor
2017-01-30 9:40 [PATCH v5 1/2] hwmon: new driver for ST stts751 thermal sensor Andrea Merello
2017-01-30 9:40 ` [PATCH v5 2/2] DT: add binding documentation for STTS751 Andrea Merello
@ 2017-01-30 14:24 ` Guenter Roeck
2017-01-31 7:11 ` Andrea Merello
1 sibling, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2017-01-30 14:24 UTC (permalink / raw)
To: Andrea Merello, linux-hwmon; +Cc: LABBE Corentin, Jean Delvare
On 01/30/2017 01:40 AM, Andrea Merello wrote:
> This patch adds a HWMON driver for ST Microelectronics STTS751
> temperature sensors.
>
Overall looks good. One thing I noticed, though: In the log function calls,
you use a mix of priv->dev and &client->dev as log device. Also, you sometimes
use &priv->client->dev even when a local client variable is available.
I can fix up the latter (no need to resend for that), but please have a
look at the overall use and either send a fixed up version or let me know
if the log devices are all intentional as-is.
Thanks,
Guenter
> Changes since V3:
> - fix sysfs_notify() filename
> - simplify error exit path in stts751_update_temp(): kill useless goto
> - fix set function were racy due to lock incorrectly placed
> - use msecs_to_jiffies instead of manual calculation
> - fix jiffies and msecs were mixed up
> - don't mark cache as good unless reads really succeded to fill up it
> - when updating therm, hyst keeps relative value rather than abs value
> - don't should in dev_dbg when chip is detected
> - don't ever generate two uevents
> - clarify clear-on-read behaviour of status reg alarm flags
> - simplify mDEG to HW conversion function
> - fix chip stops due to incorrect resolution configuration
> - fix incorrectly reading negative numbers from HW
>
> Changes since V4:
> - fix continuation lines (style, remove uncecessary, ..)
> - shorten a unnecessary verbose dev_warn() message
> - fix set_interval() exitpath didn't return error
> - change uncecesary neg-logic
> - change file symbolic permissions with octal
> - move chip revision test in probe(), change dev_notice() in dev_dbg()
> - use "return 0" on success (non functional change)
> - add further checks in i2c detect function
> - fix i2c detect function read wrong reg while checking for manufacturer ID
>
> Thanks-to: LABBE Corentin [for suggestions]
> Thanks-to: Guenter Roeck [for suggestion and discussions]
> 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 | 819 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 830 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..4154294
> --- /dev/null
> +++ b/drivers/hwmon/stts751.c
> @@ -0,0 +1,819 @@
> +/*
> + * 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(s16 hw_val)
> +{
> + return hw_val * 125 / 32;
> +}
> +
> +static s32 stts751_to_hw(int val)
> +{
> + return DIV_ROUND_CLOSEST(val, 125) * 32;
> +}
> +
> +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;
> + dev_dbg(priv->dev, "setting res %d. config %x", res, priv->config);
> + priv->res = res;
> +
> + 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;
> +
> + /*
> + * 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", integer1);
> + return integer1;
> + }
> +
> + 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", frac);
> + return frac;
> + }
> +
> + 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", integer2);
> + return integer2;
> + }
> +
> + 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",
> + frac);
> + return frac;
> + }
> + }
> +
> + priv->temp = stts751_to_deg((integer1 << 8) | frac);
> + return 0;
> +}
> +
> +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);
> +
> + ret = i2c_smbus_write_byte_data(priv->client, hreg, hwval >> 8);
> + if (ret)
> + return ret;
> +
> + return i2c_smbus_write_byte_data(priv->client, lreg, hwval & 0xff);
> +}
> +
> +static int stts751_set_temp_reg8(struct stts751_priv *priv, int temp, u8 reg)
> +{
> + s32 hwval;
> +
> + hwval = stts751_to_hw(temp);
> + return i2c_smbus_write_byte_data(priv->client, reg, hwval >> 8);
> +}
> +
> +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 alarm flags int the status register,
> + * despite what the datasheet claims, gets always cleared on read.
> + */
> +static int stts751_update_alert(struct stts751_priv *priv)
> +{
> + int ret;
> + bool conv_done;
> + int cache_time = msecs_to_jiffies(stts751_intervals[priv->interval]);
> +
> + /*
> + * 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. 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_max_alarm");
> + }
> +
> + if (priv->min_alert) {
> + dev_notice(&client->dev, "got alert for LOW temperature");
> +
> + /* unblock alert poll */
> + sysfs_notify(&priv->dev->kobj, NULL, "temp1_min_alarm");
> + }
> +
> + if (priv->min_alert || priv->max_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 = msecs_to_jiffies(stts751_intervals[priv->interval]);
> +
> + mutex_lock(&priv->access_lock);
> + if (time_after(jiffies, priv->last_update + cache_time) ||
> + !priv->data_valid) {
> + ret = stts751_update_temp(priv);
> + if (ret)
> + goto exit;
> +
> + ret = stts751_update_alert(priv);
> + if (ret)
> + goto exit;
> + priv->data_valid = true;
> + priv->last_update = jiffies;
> + }
> +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 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);
> + mutex_lock(&priv->access_lock);
> + ret = stts751_set_temp_reg8(priv, temp, STTS751_REG_TLIM);
> + if (ret)
> + goto exit;
> +
> + dev_dbg(dev, "setting therm %ld", temp);
> +
> + /*
> + * hysteresis reg is relative to therm, so the HW does not need to be
> + * adjusted, we need to update our local copy only.
> + */
> + priv->hyst = temp - (priv->therm - priv->hyst);
> + priv->therm = temp;
> +
> +exit:
> + mutex_unlock(&priv->access_lock);
> + 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;
> +
> + mutex_lock(&priv->access_lock);
> + /* HW works in range -64C to +127.937C */
> + temp = clamp_val(temp, -64000, priv->therm);
> + priv->hyst = temp;
> + dev_dbg(priv->dev, "setting hyst %ld", temp);
> + temp = priv->therm - temp;
> + ret = stts751_set_temp_reg8(priv, temp, STTS751_REG_HYST);
> + mutex_unlock(&priv->access_lock);
> + 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;
> +
> + mutex_lock(&priv->access_lock);
> + /* 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)
> + goto exit;
> +
> + dev_dbg(dev, "setting event max %ld", temp);
> + priv->event_max = temp;
> + ret = count;
> +exit:
> + mutex_unlock(&priv->access_lock);
> + return ret;
> +}
> +
> +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;
> +
> + mutex_lock(&priv->access_lock);
> + /* 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)
> + goto exit;
> +
> + dev_dbg(dev, "setting event min %ld", temp);
> + priv->event_min = temp;
> + ret = count;
> +exit:
> + mutex_unlock(&priv->access_lock);
> + return ret;
> +}
> +
> +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 = count;
> + 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]);
> +
> + mutex_lock(&priv->access_lock);
> + if (priv->interval == idx)
> + goto exit;
> +
> + /*
> + * 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) {
> + dev_dbg(priv->dev, "lower resolution, then modify convrate");
> + 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) {
> + dev_dbg(priv->dev, "modify convrate, then raise resolution");
> + priv->interval = idx;
> + ret = stts751_adjust_resolution(priv);
> + if (ret)
> + goto exit;
> + }
> + ret = count;
> +exit:
> + mutex_unlock(&priv->access_lock);
> +
> + return ret;
> +}
> +
> +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 tmp;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> + return -ENODEV;
> +
> + tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_MAN_ID);
> + if (tmp != ST_MAN_ID)
> + return -ENODEV;
> +
> + /* lower temperaure registers always have bits 0-3 set to zero */
> + tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_TEMP_L);
> + if (tmp & 0xf)
> + return -ENODEV;
> +
> + tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_HLIM_L);
> + if (tmp & 0xf)
> + return -ENODEV;
> +
> + tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_LLIM_L);
> + if (tmp & 0xf)
> + return -ENODEV;
> +
> + /* smbus timeout register always have bits 0-7 set to zero */
> + tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_SMBUS_TO);
> + if (tmp & 0x7f)
> + return -ENODEV;
> +
> + tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_PROD_ID);
> +
> + switch (tmp) {
> + 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);
> +
> + 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 0;
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, 0444, show_input, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_min, 0644, show_min, set_min, 0);
> +static SENSOR_DEVICE_ATTR(temp1_max, 0644, show_max, set_max, 0);
> +static SENSOR_DEVICE_ATTR(temp1_min_alarm, 0444, show_min_alarm, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_max_alarm, 0444, show_max_alarm, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit, 0644, show_therm, set_therm, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit_hyst, 0644, show_hyst, set_hyst, 0);
> +static SENSOR_DEVICE_ATTR(temp1_crit_alarm, 0444, show_therm_trip, NULL, 0);
> +static SENSOR_DEVICE_ATTR(update_interval, 0644,
> + 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_nto;
> + int rev_id;
> +
> + 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_nto = device_property_read_bool(&client->dev,
> + "smbus-timeout-disable");
> +
> + ret = i2c_smbus_write_byte_data(priv->client,
> + STTS751_REG_SMBUS_TO,
> + smbus_nto ? 0 : 0x80);
> + if (ret)
> + return ret;
> + }
> +
> + rev_id = i2c_smbus_read_byte_data(priv->client, STTS751_REG_REV_ID);
> + if (rev_id < 0)
> + return -ENODEV;
> + if (rev_id != 0x1) {
> + dev_dbg(&priv->client->dev, "Chip revision 0x%x is untested\n",
> + rev_id);
> + }
> +
> + 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");
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 1/2] hwmon: new driver for ST stts751 thermal sensor
2017-01-30 14:24 ` [PATCH v5 1/2] hwmon: new driver for ST stts751 thermal sensor Guenter Roeck
@ 2017-01-31 7:11 ` Andrea Merello
2017-01-31 11:47 ` Guenter Roeck
0 siblings, 1 reply; 7+ messages in thread
From: Andrea Merello @ 2017-01-31 7:11 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, LABBE Corentin, Jean Delvare
On Mon, Jan 30, 2017 at 3:24 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 01/30/2017 01:40 AM, Andrea Merello wrote:
>>
>> This patch adds a HWMON driver for ST Microelectronics STTS751
>> temperature sensors.
>>
>
> Overall looks good. One thing I noticed, though: In the log function calls,
> you use a mix of priv->dev and &client->dev as log device. Also, you
> sometimes
> use &priv->client->dev even when a local client variable is available.
> I can fix up the latter (no need to resend for that), but please have a
> look at the overall use and either send a fixed up version or let me know
> if the log devices are all intentional as-is.
Hmm, you are right: there is a bit of mess here. It's not intentional indeed..
I gave a look to lm90.c and lm63.c; it seems no one uses the hwmon
device for this purpose. We can probably stick to &client->dev or
&priv->client->dev when a local client variable is not already
available.
Also, it looks like we can change some &priv->client->dev in
&client->dev in probe function when calling i2c read/write functions.
If you ACK, I'll fix and resend.
> Thanks,
> Guenter
>
>
>> Changes since V3:
>> - fix sysfs_notify() filename
>> - simplify error exit path in stts751_update_temp(): kill useless goto
>> - fix set function were racy due to lock incorrectly placed
>> - use msecs_to_jiffies instead of manual calculation
>> - fix jiffies and msecs were mixed up
>> - don't mark cache as good unless reads really succeded to fill up it
>> - when updating therm, hyst keeps relative value rather than abs value
>> - don't should in dev_dbg when chip is detected
>> - don't ever generate two uevents
>> - clarify clear-on-read behaviour of status reg alarm flags
>> - simplify mDEG to HW conversion function
>> - fix chip stops due to incorrect resolution configuration
>> - fix incorrectly reading negative numbers from HW
>>
>> Changes since V4:
>> - fix continuation lines (style, remove uncecessary, ..)
>> - shorten a unnecessary verbose dev_warn() message
>> - fix set_interval() exitpath didn't return error
>> - change uncecesary neg-logic
>> - change file symbolic permissions with octal
>> - move chip revision test in probe(), change dev_notice() in dev_dbg()
>> - use "return 0" on success (non functional change)
>> - add further checks in i2c detect function
>> - fix i2c detect function read wrong reg while checking for manufacturer
>> ID
>>
>> Thanks-to: LABBE Corentin [for suggestions]
>> Thanks-to: Guenter Roeck [for suggestion and discussions]
>> 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 | 819
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 830 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..4154294
>> --- /dev/null
>> +++ b/drivers/hwmon/stts751.c
>> @@ -0,0 +1,819 @@
>> +/*
>> + * 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(s16 hw_val)
>> +{
>> + return hw_val * 125 / 32;
>> +}
>> +
>> +static s32 stts751_to_hw(int val)
>> +{
>> + return DIV_ROUND_CLOSEST(val, 125) * 32;
>> +}
>> +
>> +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;
>> + dev_dbg(priv->dev, "setting res %d. config %x", res,
>> priv->config);
>> + priv->res = res;
>> +
>> + 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;
>> +
>> + /*
>> + * 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", integer1);
>> + return integer1;
>> + }
>> +
>> + 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", frac);
>> + return frac;
>> + }
>> +
>> + 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",
>> integer2);
>> + return integer2;
>> + }
>> +
>> + 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",
>> + frac);
>> + return frac;
>> + }
>> + }
>> +
>> + priv->temp = stts751_to_deg((integer1 << 8) | frac);
>> + return 0;
>> +}
>> +
>> +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);
>> +
>> + ret = i2c_smbus_write_byte_data(priv->client, hreg, hwval >> 8);
>> + if (ret)
>> + return ret;
>> +
>> + return i2c_smbus_write_byte_data(priv->client, lreg, hwval &
>> 0xff);
>> +}
>> +
>> +static int stts751_set_temp_reg8(struct stts751_priv *priv, int temp, u8
>> reg)
>> +{
>> + s32 hwval;
>> +
>> + hwval = stts751_to_hw(temp);
>> + return i2c_smbus_write_byte_data(priv->client, reg, hwval >> 8);
>> +}
>> +
>> +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 alarm flags int the status
>> register,
>> + * despite what the datasheet claims, gets always cleared on read.
>> + */
>> +static int stts751_update_alert(struct stts751_priv *priv)
>> +{
>> + int ret;
>> + bool conv_done;
>> + int cache_time =
>> msecs_to_jiffies(stts751_intervals[priv->interval]);
>> +
>> + /*
>> + * 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. 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_max_alarm");
>> + }
>> +
>> + if (priv->min_alert) {
>> + dev_notice(&client->dev, "got alert for LOW temperature");
>> +
>> + /* unblock alert poll */
>> + sysfs_notify(&priv->dev->kobj, NULL, "temp1_min_alarm");
>> + }
>> +
>> + if (priv->min_alert || priv->max_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 =
>> msecs_to_jiffies(stts751_intervals[priv->interval]);
>> +
>> + mutex_lock(&priv->access_lock);
>> + if (time_after(jiffies, priv->last_update + cache_time) ||
>> + !priv->data_valid) {
>> + ret = stts751_update_temp(priv);
>> + if (ret)
>> + goto exit;
>> +
>> + ret = stts751_update_alert(priv);
>> + if (ret)
>> + goto exit;
>> + priv->data_valid = true;
>> + priv->last_update = jiffies;
>> + }
>> +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 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);
>> + mutex_lock(&priv->access_lock);
>> + ret = stts751_set_temp_reg8(priv, temp, STTS751_REG_TLIM);
>> + if (ret)
>> + goto exit;
>> +
>> + dev_dbg(dev, "setting therm %ld", temp);
>> +
>> + /*
>> + * hysteresis reg is relative to therm, so the HW does not need to
>> be
>> + * adjusted, we need to update our local copy only.
>> + */
>> + priv->hyst = temp - (priv->therm - priv->hyst);
>> + priv->therm = temp;
>> +
>> +exit:
>> + mutex_unlock(&priv->access_lock);
>> + 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;
>> +
>> + mutex_lock(&priv->access_lock);
>> + /* HW works in range -64C to +127.937C */
>> + temp = clamp_val(temp, -64000, priv->therm);
>> + priv->hyst = temp;
>> + dev_dbg(priv->dev, "setting hyst %ld", temp);
>> + temp = priv->therm - temp;
>> + ret = stts751_set_temp_reg8(priv, temp, STTS751_REG_HYST);
>> + mutex_unlock(&priv->access_lock);
>> + 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;
>> +
>> + mutex_lock(&priv->access_lock);
>> + /* 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)
>> + goto exit;
>> +
>> + dev_dbg(dev, "setting event max %ld", temp);
>> + priv->event_max = temp;
>> + ret = count;
>> +exit:
>> + mutex_unlock(&priv->access_lock);
>> + return ret;
>> +}
>> +
>> +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;
>> +
>> + mutex_lock(&priv->access_lock);
>> + /* 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)
>> + goto exit;
>> +
>> + dev_dbg(dev, "setting event min %ld", temp);
>> + priv->event_min = temp;
>> + ret = count;
>> +exit:
>> + mutex_unlock(&priv->access_lock);
>> + return ret;
>> +}
>> +
>> +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 = count;
>> + 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]);
>> +
>> + mutex_lock(&priv->access_lock);
>> + if (priv->interval == idx)
>> + goto exit;
>> +
>> + /*
>> + * 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) {
>> + dev_dbg(priv->dev, "lower resolution, then modify
>> convrate");
>> + 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) {
>> + dev_dbg(priv->dev, "modify convrate, then raise
>> resolution");
>> + priv->interval = idx;
>> + ret = stts751_adjust_resolution(priv);
>> + if (ret)
>> + goto exit;
>> + }
>> + ret = count;
>> +exit:
>> + mutex_unlock(&priv->access_lock);
>> +
>> + return ret;
>> +}
>> +
>> +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 tmp;
>> +
>> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
>> + return -ENODEV;
>> +
>> + tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_MAN_ID);
>> + if (tmp != ST_MAN_ID)
>> + return -ENODEV;
>> +
>> + /* lower temperaure registers always have bits 0-3 set to zero */
>> + tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_TEMP_L);
>> + if (tmp & 0xf)
>> + return -ENODEV;
>> +
>> + tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_HLIM_L);
>> + if (tmp & 0xf)
>> + return -ENODEV;
>> +
>> + tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_LLIM_L);
>> + if (tmp & 0xf)
>> + return -ENODEV;
>> +
>> + /* smbus timeout register always have bits 0-7 set to zero */
>> + tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_SMBUS_TO);
>> + if (tmp & 0x7f)
>> + return -ENODEV;
>> +
>> + tmp = i2c_smbus_read_byte_data(new_client, STTS751_REG_PROD_ID);
>> +
>> + switch (tmp) {
>> + 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);
>> +
>> + 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 0;
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(temp1_input, 0444, show_input, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_min, 0644, show_min, set_min, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_max, 0644, show_max, set_max, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_min_alarm, 0444, show_min_alarm, NULL,
>> 0);
>> +static SENSOR_DEVICE_ATTR(temp1_max_alarm, 0444, show_max_alarm, NULL,
>> 0);
>> +static SENSOR_DEVICE_ATTR(temp1_crit, 0644, show_therm, set_therm,
>> 0);
>> +static SENSOR_DEVICE_ATTR(temp1_crit_hyst, 0644, show_hyst, set_hyst, 0);
>> +static SENSOR_DEVICE_ATTR(temp1_crit_alarm, 0444, show_therm_trip, NULL,
>> 0);
>> +static SENSOR_DEVICE_ATTR(update_interval, 0644,
>> + 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_nto;
>> + int rev_id;
>> +
>> + 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_nto = device_property_read_bool(&client->dev,
>> +
>> "smbus-timeout-disable");
>> +
>> + ret = i2c_smbus_write_byte_data(priv->client,
>> + STTS751_REG_SMBUS_TO,
>> + smbus_nto ? 0 : 0x80);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + rev_id = i2c_smbus_read_byte_data(priv->client,
>> STTS751_REG_REV_ID);
>> + if (rev_id < 0)
>> + return -ENODEV;
>> + if (rev_id != 0x1) {
>> + dev_dbg(&priv->client->dev, "Chip revision 0x%x is
>> untested\n",
>> + rev_id);
>> + }
>> +
>> + 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");
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 1/2] hwmon: new driver for ST stts751 thermal sensor
2017-01-31 7:11 ` Andrea Merello
@ 2017-01-31 11:47 ` Guenter Roeck
2017-02-01 7:31 ` Andrea Merello
0 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2017-01-31 11:47 UTC (permalink / raw)
To: andrea.merello; +Cc: linux-hwmon, LABBE Corentin, Jean Delvare
On 01/30/2017 11:11 PM, Andrea Merello wrote:
> On Mon, Jan 30, 2017 at 3:24 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 01/30/2017 01:40 AM, Andrea Merello wrote:
>>>
>>> This patch adds a HWMON driver for ST Microelectronics STTS751
>>> temperature sensors.
>>>
>>
>> Overall looks good. One thing I noticed, though: In the log function calls,
>> you use a mix of priv->dev and &client->dev as log device. Also, you
>> sometimes
>> use &priv->client->dev even when a local client variable is available.
>> I can fix up the latter (no need to resend for that), but please have a
>> look at the overall use and either send a fixed up version or let me know
>> if the log devices are all intentional as-is.
>
> Hmm, you are right: there is a bit of mess here. It's not intentional indeed..
>
> I gave a look to lm90.c and lm63.c; it seems no one uses the hwmon
> device for this purpose. We can probably stick to &client->dev or
> &priv->client->dev when a local client variable is not already
> available.
It is really your call as the driver's author to make if you want to use
the hwmon device or &client->dev.
>
> Also, it looks like we can change some &priv->client->dev in
> &client->dev in probe function when calling i2c read/write functions.
>
> If you ACK, I'll fix and resend.
>
SGTM.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 1/2] hwmon: new driver for ST stts751 thermal sensor
2017-01-31 11:47 ` Guenter Roeck
@ 2017-02-01 7:31 ` Andrea Merello
2017-02-01 14:11 ` Guenter Roeck
0 siblings, 1 reply; 7+ messages in thread
From: Andrea Merello @ 2017-02-01 7:31 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, LABBE Corentin, Jean Delvare
On Tue, Jan 31, 2017 at 12:47 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 01/30/2017 11:11 PM, Andrea Merello wrote:
>>
>> On Mon, Jan 30, 2017 at 3:24 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> On 01/30/2017 01:40 AM, Andrea Merello wrote:
>>>>
>>>>
>>>> This patch adds a HWMON driver for ST Microelectronics STTS751
>>>> temperature sensors.
>>>>
>>>
>>> Overall looks good. One thing I noticed, though: In the log function
>>> calls,
>>> you use a mix of priv->dev and &client->dev as log device. Also, you
>>> sometimes
>>> use &priv->client->dev even when a local client variable is available.
>>> I can fix up the latter (no need to resend for that), but please have a
>>> look at the overall use and either send a fixed up version or let me know
>>> if the log devices are all intentional as-is.
>>
>>
>> Hmm, you are right: there is a bit of mess here. It's not intentional
>> indeed..
>>
>> I gave a look to lm90.c and lm63.c; it seems no one uses the hwmon
>> device for this purpose. We can probably stick to &client->dev or
>> &priv->client->dev when a local client variable is not already
>> available.
>
>
> It is really your call as the driver's author to make if you want to use
> the hwmon device or &client->dev.
I've decided to stick with &client->dev for all dev_dbg(), while I
would use the hwmon device for dev_notice() and dev_warn(), that are
supposed to be eventually read by the user.
However, while keeping an eye on the log messages I've realized that
if the temperature remains out-of-limit for a lot of time, especially
if the conversion rate is set fast, we tend to spam the log with alert
messages.. I learned that you are usually concerned by noisy drivers,
so I guess you might want to mitigate this..
I could use dev_notice_once(), but it seems inappropriate here, or I
can silence the message at least until one reads the chip. Lm90 does
even disable the alarm until that happen, but it has only one alarm;
here we have two, so it seems not correct here.
Any suggestion?
>>
>>
>> Also, it looks like we can change some &priv->client->dev in
>> &client->dev in probe function when calling i2c read/write functions.
>>
>> If you ACK, I'll fix and resend.
>>
> SGTM.
>
> Thanks,
> Guenter
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5 1/2] hwmon: new driver for ST stts751 thermal sensor
2017-02-01 7:31 ` Andrea Merello
@ 2017-02-01 14:11 ` Guenter Roeck
0 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2017-02-01 14:11 UTC (permalink / raw)
To: andrea.merello; +Cc: linux-hwmon, LABBE Corentin, Jean Delvare
On 01/31/2017 11:31 PM, Andrea Merello wrote:
> On Tue, Jan 31, 2017 at 12:47 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 01/30/2017 11:11 PM, Andrea Merello wrote:
>>>
>>> On Mon, Jan 30, 2017 at 3:24 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On 01/30/2017 01:40 AM, Andrea Merello wrote:
>>>>>
>>>>>
>>>>> This patch adds a HWMON driver for ST Microelectronics STTS751
>>>>> temperature sensors.
>>>>>
>>>>
>>>> Overall looks good. One thing I noticed, though: In the log function
>>>> calls,
>>>> you use a mix of priv->dev and &client->dev as log device. Also, you
>>>> sometimes
>>>> use &priv->client->dev even when a local client variable is available.
>>>> I can fix up the latter (no need to resend for that), but please have a
>>>> look at the overall use and either send a fixed up version or let me know
>>>> if the log devices are all intentional as-is.
>>>
>>>
>>> Hmm, you are right: there is a bit of mess here. It's not intentional
>>> indeed..
>>>
>>> I gave a look to lm90.c and lm63.c; it seems no one uses the hwmon
>>> device for this purpose. We can probably stick to &client->dev or
>>> &priv->client->dev when a local client variable is not already
>>> available.
>>
>>
>> It is really your call as the driver's author to make if you want to use
>> the hwmon device or &client->dev.
>
> I've decided to stick with &client->dev for all dev_dbg(), while I
> would use the hwmon device for dev_notice() and dev_warn(), that are
> supposed to be eventually read by the user.
>
> However, while keeping an eye on the log messages I've realized that
> if the temperature remains out-of-limit for a lot of time, especially
> if the conversion rate is set fast, we tend to spam the log with alert
> messages.. I learned that you are usually concerned by noisy drivers,
> so I guess you might want to mitigate this..
>
> I could use dev_notice_once(), but it seems inappropriate here, or I
> can silence the message at least until one reads the chip. Lm90 does
> even disable the alarm until that happen, but it has only one alarm;
> here we have two, so it seems not correct here.
>
> Any suggestion?
>
Use a flag that shows if the attribute in question was read ?
Guenter
>
>>>
>>>
>>> Also, it looks like we can change some &priv->client->dev in
>>> &client->dev in probe function when calling i2c read/write functions.
>>>
>>> If you ACK, I'll fix and resend.
>>>
>> SGTM.
>>
>> Thanks,
>> Guenter
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-02-01 14:11 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-30 9:40 [PATCH v5 1/2] hwmon: new driver for ST stts751 thermal sensor Andrea Merello
2017-01-30 9:40 ` [PATCH v5 2/2] DT: add binding documentation for STTS751 Andrea Merello
2017-01-30 14:24 ` [PATCH v5 1/2] hwmon: new driver for ST stts751 thermal sensor Guenter Roeck
2017-01-31 7:11 ` Andrea Merello
2017-01-31 11:47 ` Guenter Roeck
2017-02-01 7:31 ` Andrea Merello
2017-02-01 14:11 ` Guenter Roeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox