hwmon: new driver for ST stts751 thermal sensor
Guenter Roeck
maintainerQuoted 41 lines · Andrea Merello
This patch adds a HWMON driver for ST Microelectronics STTS751 temperature sensors.
It does support temperature reading (of course), SMBUS alert functionality and "therm" output.
Thanks-to: LABBE Corentin [for suggestions]
Thanks-to: Guenter Roeck [for suggestions/discussions]Signed-off-by Andrea Merello <[email protected]>Cc LABBE Corentin <[email protected]>Cc Guenter Roeck <[email protected]>Cc Jean Delvare <[email protected]>--- drivers/hwmon/Kconfig | 12 +- drivers/hwmon/Makefile | 2 +- drivers/hwmon/stts751.c | 805 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 817 insertions(+), 2 deletions(-) create mode 100644 drivers/hwmon/stts751.cdrivers/hwmon/Kconfig +10
-0@@ -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
drivers/hwmon/Kconfig config SENSORS_ADS7871 · line 1519
Quoted 5 lines
drivers/hwmon/Kconfig +1
-1@@ -1506,7 +1516,7 @@ config SENSORS_ADS7871 config SENSORS_AMC6821 tristate "Texas Instruments AMC6821" - depends on I2C + depends on I2C
???
Quoted 16 lines
drivers/hwmon/Kconfig +0
-0help If you say yes here you get support for the Texas Instruments AMC6821 hardware monitoring chips.drivers/hwmon/Makefile +1
-0@@ -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
drivers/hwmon/Makefile obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o · line 172
Quoted 4 lines
drivers/hwmon/Makefile +0
-1@@ -169,4 +170,3 @@ obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o obj-$(CONFIG_PMBUS) += pmbus/ ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG -
Looks like an unnecessary whitespace change.
Quoted 19 lines
drivers/hwmon/stts751.c new file +13
-0@@ -0,0 +1,805 @@ +/* +* STTS751 sensor driver +* +* Copyright (C) 2016 Istituto Italiano di Tecnologia - RBCS - EDL +* Robotics, Brain and Cognitive Sciences department +* Electronic Design Laboratory +* +* Written by Andrea Merello <[email protected]> +* +* 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
drivers/hwmon/stts751.c line 23
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+* 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. +*/ + +
No douple empty lines please.
Quoted 99 lines
drivers/hwmon/stts751.c new file +99
-0+#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/of_irq.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_TRIPL BIT(5) +#define STTS751_STATUS_TRIPH BIT(6) +#define STTS751_STATUS_BUSY BIT(8) +#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_ONESHOT 0x0F +#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 + +/* stick with HW defaults */ +#define STTS751_THERM_DEFAULT 85000 +#define STTS751_HYST_DEFAULT 10000 +#define STTS751_EVENT_MAX_DEFAULT 85000 +#define STTS751_EVENT_MIN_DEFAULT 0 + +#define STTS751_RACE_RETRY 5 +#define STTS751_CONV_TIMEOUT 100/* mS */ +#define STTS751_CACHE_TIME 100/* mS */ + +/* +* 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 +}; + +struct stts751_priv { +struct device *dev; +struct i2c_client *client; +struct mutex access_lock; +unsigned long interval; +int res; +bool gen_therm, gen_event; +int event_max, event_min; +int therm; +int hyst; +bool smbus_timeout; +int temp; +unsigned long last_update; +u8 config; +bool min_alert, max_alert; +bool data_valid; + +/* +* Temperature/interval is always present. +* Depending by DT therm and event are dynamically added. +* There are max 4 entries plus the guard +*/ +const struct attribute_group *groups[5 ]; +}; + +/* +* These functions converts temperature from HW format to integer format and +* vice-vers. They are (mostly) taken from lm90 driver. Unit is in mC.
drivers/hwmon/stts751.c line 132
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+*/ +static int stts751_to_deg (s32 hw_val) +{ +return hw_val *125 /32 ; +} + +static s16 stts751_to_hw (int val,s32 *hw_val) +{ +/* HW works in range -64C to +127C */ +if ((val >127000 ) || (val <-64000 ))
Extra ( ). However, clamp_val(val, -64000, 127000) is preferred here instead of returning an error (we don't want to force users to guess valid ranges).
The upper limit should probably be 127937, though, per datasheet.
drivers/hwmon/stts751.c line 139
Quoted 7 lines
drivers/hwmon/stts751.c new file +7
-0+return -EINVAL ; + +if (val <0 ) + *hw_val = (val -62 ) /125 *32 ; +else + *hw_val = (val +62 ) /125 *32 ; +
By having this function return an int, you could combine the 16-bit return value with the error code, and the extra parameter would not be necessary (even more so since this function should not return an error in the first place).
Quoted 116 lines
drivers/hwmon/stts751.c new file +116
-0+return 0 ; +} + +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 ; + +mutex_lock (&priv->access_lock); + +/* +* 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: +mutex_unlock (&priv->access_lock); +if (ret) +return ret; + + priv->temp =stts751_to_deg ((integer1 <<8 ) | frac); +return ret; +} + +static int stts751_set_temp_reg (struct stts751_priv *priv,int temp, +bool is_frac,u8 hreg,u8 lreg) +{ +s32 hwval; +int ret; + +if (stts751_to_hw (temp, &hwval)) +return -EINVAL ; + +mutex_lock (&priv->access_lock); + ret =i2c_smbus_write_byte_data (priv->client, hreg, hwval >>8 ); +if (ret) + goto exit; +if (is_frac) + ret =i2c_smbus_write_byte_data (priv->client, + lreg, hwval &0xff ); +exit: +mutex_unlock (&priv->access_lock); + +return ret; +} +
drivers/hwmon/stts751.c line 265
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+static int stts751_update_alert (struct stts751_priv *priv) +{ +int ret; + +/* not for us.. */ +if (!priv->gen_event) +return 0 ; + + ret =i2c_smbus_read_byte_data (priv->client,STTS751_REG_STATUS ); +
Please no empty line between functions and checking of return values.
drivers/hwmon/stts751.c line 271
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+if (ret <0 ) +return ret; + + priv->max_alert = priv->max_alert || !!(ret &STTS751_STATUS_TRIPH ); + priv->min_alert = priv->min_alert || !!(ret &STTS751_STATUS_TRIPL ); +
I am not really happy about the caching of alert values. This isn't really common. Usually we report current alert values when reading. I don't really see why this driver should be different to all the others.
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+return 0 ; +} + +static void stts751_alert (struct i2c_client *client, +enum i2c_alert_protocol type,unsigned int data) +{
drivers/hwmon/stts751.c line 287
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+int ret; +struct stts751_priv *priv =i2c_get_clientdata (client); +bool prev_max = priv->max_alert; +bool prev_min = priv->min_alert; + +if (type !=I2C_PROTOCOL_SMBUS_ALERT ) +return ; + +dev_dbg (&client->dev,"alert!" ); +
Any chance to make this a bit more generic and also support interrupts ?
Quoted 58 lines
drivers/hwmon/stts751.c new file +58
-0+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; + +if (!(prev_max && prev_min)) { +dev_warn (&priv->client->dev, +"Alert received, but can't communicate to the device. Something bad happening? Triggering all alarms!" ); + } + } + +if (!prev_max && 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 (!prev_min && 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 ssize_t show_max_alert (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->max_alert); +} + +static ssize_t set_max_alert (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) +{ +struct stts751_priv *priv =dev_get_drvdata (dev); + +mutex_lock (&priv->access_lock); + priv->max_alert = false; +mutex_unlock (&priv->access_lock); + +return count; +} + +static ssize_t show_min_alert (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->min_alert); +}
drivers/hwmon/stts751.c line 355
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+ +static ssize_t set_min_alert (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) +{ +struct stts751_priv *priv =dev_get_drvdata (dev); + +mutex_lock (&priv->access_lock); + priv->min_alert = false; +mutex_unlock (&priv->access_lock); +
This is highly unusual. If the alert needs to be cleared, please auto-clear it when reading.
Quoted 42 lines
drivers/hwmon/stts751.c new file +42
-0+return count; +} + +static ssize_t show_input (struct device *dev,struct device_attribute *attr, +char *buf) +{ +int ret; +int cache_time =STTS751_CACHE_TIME *HZ /1000 ; +struct stts751_priv *priv =dev_get_drvdata (dev); + +/* +* Adjust the cache time wrt the sample rate. We do 4X in order to get +* a new measure in no more than 1/4 of the sample time (that seemed +* reasonable to me). +*/ + cache_time = stts751_intervals[priv->interval] /4 *HZ /1000 ; + +if (time_after (jiffies, priv->last_update + cache_time) || + !priv->data_valid) { + ret =stts751_update_temp (priv); +if (ret) +return ret; + priv->last_update = jiffies; + priv->data_valid = true; + } + +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;
drivers/hwmon/stts751.c line 407
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+long temp; +struct stts751_priv *priv =dev_get_drvdata (dev); + +if (kstrtol (buf,10 , &temp) <0 ) +return -EINVAL ; + + ret =stts751_set_temp_reg (priv, temp, false,STTS751_REG_TLIM ,0 ); +if (ret) +return ret; +
Does it really add value to use the same function for 8- and 16-bit registers ? Seems to me it just makes the code more complicated.
Quoted 15 lines
drivers/hwmon/stts751.c new file +15
-0+dev_dbg (dev,"setting therm %ld" , temp); + + priv->therm = temp; +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); +} + +
drivers/hwmon/stts751.c line 432
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+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 ; +
Per ABI, this is supposed to be an absolute temperature. For the chip, it is a delta. You'll have to do some calculation to convert the absolute value into a delta. Same for the show command.
Quoted 37 lines
drivers/hwmon/stts751.c new file +37
-0+ ret =stts751_set_temp_reg (priv, temp, false,STTS751_REG_HYST ,0 ); +if (ret) +return ret; + +dev_dbg (dev,"setting hyst %ld" , temp); + + priv->hyst = temp; +return count; +} + +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 ; + + ret =stts751_set_temp_reg (priv, temp, true, +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; +}
drivers/hwmon/stts751.c line 479
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+ +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); +} + +
Single empty line, please.
Quoted 61 lines
drivers/hwmon/stts751.c new file +61
-0+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 ; + + ret =stts751_set_temp_reg (priv, temp, true, +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); + +/* 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);
drivers/hwmon/stts751.c line 550
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+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; +
Extra empty line.
Is it really necessary to have this code twice ? What is the harm in just doing it once, either before or after setting the conversion rate ?
Quoted 25 lines
drivers/hwmon/stts751.c new file +25
-0+ } +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 :
drivers/hwmon/stts751.c line 585
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+ name ="STTS751-0" ; +break ; +case STTS751_1_PROD_ID : + name ="STTS751-1" ; +break ; +default : +return -ENODEV ; + } +dev_info (&new_client->dev,"Chip %s detected!" , name); +
Is this noise necessary ?
drivers/hwmon/stts751.c line 588
Quoted 3 lines
drivers/hwmon/stts751.c new file +3
-0+ rev_id =i2c_smbus_read_byte_data (new_client,STTS751_REG_REV_ID ); + +if (rev_id !=0x1 ) {
You might want to check for an error here (and return -ENODEV if you get one).
drivers/hwmon/stts751.c line 594
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+dev_notice (&new_client->dev, +"Chip revision 0x%x is untested\nPlease report whether it works to [email protected]" , + rev_id); + } + +strlcpy (info->type, name,I2C_NAME_SIZE );
This doesn't match the driver name (or, rather, the values in stts751_id). Does this instantiate the driver ? That would surprise me.
Quoted 29 lines
drivers/hwmon/stts751.c new file +29
-0+return 0 ; +} + +static int stts751_init_chip (struct stts751_priv *priv) +{ +int ret; + + priv->config =STTS751_CONF_EVENT_DIS |STTS751_CONF_STOP ; + ret =i2c_smbus_write_byte_data (priv->client,STTS751_REG_CONF , + priv->config); +if (ret) +return ret; + + ret =i2c_smbus_write_byte_data (priv->client,STTS751_REG_RATE , + priv->interval); +if (ret) +return ret; + +/* invalid, to force update */ + priv->res =-1 ; + ret =stts751_adjust_resolution (priv); +if (ret) +return ret; + + ret =i2c_smbus_write_byte_data (priv->client, +STTS751_REG_SMBUS_TO , + priv->smbus_timeout ?0x80 :0 ); +if (ret) +return ret;
drivers/hwmon/stts751.c line 633
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+ + +if (priv->gen_event) { + ret =stts751_set_temp_reg (priv, priv->event_max, true, +STTS751_REG_HLIM_H ,STTS751_REG_HLIM_L ); +if (ret) +return ret; + + ret =stts751_set_temp_reg (priv, priv->event_min, true, +STTS751_REG_LLIM_H ,STTS751_REG_LLIM_L );
Those registers all have power-up defaults. Unless there is a good reason, we should not overwrite them. In some applications, the values may be initialized from the BIOS/ROMMON.
Quoted 27 lines
drivers/hwmon/stts751.c new file +27
-0+if (ret) +return ret; + priv->config &= ~STTS751_CONF_EVENT_DIS ; + } + +if (priv->gen_therm) { + ret =stts751_set_temp_reg (priv, priv->therm, false, +STTS751_REG_TLIM ,0 ); +if (ret) +return ret; + + ret =stts751_set_temp_reg (priv, priv->hyst, false, +STTS751_REG_HYST ,0 ); +if (ret) +return ret; + } + + priv->config &= ~STTS751_CONF_STOP ; + ret =i2c_smbus_write_byte_data (priv->client, +STTS751_REG_CONF , priv->config); + +return ret; +} + +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO, + show_min, set_min, 0);
drivers/hwmon/stts751.c line 670
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO, + show_max, set_max, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO, + show_min_alert, set_min_alert, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO, + show_max_alert, set_max_alert, 0); +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm, + set_therm, 0); +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO, show_hyst, + set_hyst, 0);
Please use standard attribute names.
Quoted 63 lines
drivers/hwmon/stts751.c new file +63
-0+static SENSOR_DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, + show_interval, set_interval, 0); + +/* always present */ +static struct attribute *stts751_temp_attrs[] = { + &sensor_dev_attr_temp1_input.dev_attr.attr, +NULL +}; + +static struct attribute_group stts751_temp_group = { + .attrs = stts751_temp_attrs, +}; + +/* present when therm pin or event pin are connected */ +static struct attribute *stts751_interval_attrs[] = { + &sensor_dev_attr_update_interval.dev_attr.attr, +NULL +}; + +static struct attribute_group stts751_interval_group = { + .attrs = stts751_interval_attrs, +}; + +/* present when event pin is connected */ +static struct attribute *stts751_event_attrs[] = { + &sensor_dev_attr_temp1_event_min.dev_attr.attr, + &sensor_dev_attr_temp1_event_max.dev_attr.attr, + &sensor_dev_attr_temp1_event_min_alert.dev_attr.attr, + &sensor_dev_attr_temp1_event_max_alert.dev_attr.attr, +NULL +}; + +static struct attribute_group stts751_event_group = { + .attrs = stts751_event_attrs, +}; + +/* present when therm pin is connected */ +static struct attribute *stts751_therm_attrs[] = { + &sensor_dev_attr_temp1_therm.dev_attr.attr, + &sensor_dev_attr_temp1_therm_hyst.dev_attr.attr, +NULL +}; + +static struct attribute_group stts751_therm_group = { + .attrs = stts751_therm_attrs, +}; + +static int stts751_probe (struct i2c_client *client, +const struct i2c_device_id *id) +{ +struct stts751_priv *priv; +int ret; +int groups_idx =0 ; +struct device_node *np = client->dev.of_node; + + 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); +
drivers/hwmon/stts751.c line 743
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+/* default to 2 samples per second */ + priv->interval =5 ; +/* default to timeout enable, as per chip default */ + priv->smbus_timeout = true; + priv->last_update =0 ; + priv->data_valid = false; + priv->max_alert = false; + priv->min_alert = false; + priv->gen_therm = false; + priv->gen_event = false;
Unnecessary initializations.
drivers/hwmon/stts751.c line 748
Quoted 5 lines
drivers/hwmon/stts751.c new file +5
-0+ priv->therm =STTS751_THERM_DEFAULT ; + priv->hyst =STTS751_HYST_DEFAULT ; + priv->event_max =STTS751_EVENT_MAX_DEFAULT ; + priv->event_min =STTS751_EVENT_MIN_DEFAULT ; +
I think it would be better to read the initial values from the chip (also see above).
drivers/hwmon/stts751.c line 755
Quoted 7 lines
drivers/hwmon/stts751.c new file +7
-0+if (np) { + priv->gen_therm =of_property_read_bool (np,"has-therm" ); + priv->gen_event =of_property_read_bool (np,"has-event" ); + priv->smbus_timeout = !of_property_read_bool (np, +"smbus-timeout-disable" ); + }else { +dev_notice (&client->dev,"No DT data. Event/therm disabled\n" );
Seems to be quite rude to penaltize non-DT systems this way.
drivers/hwmon/stts751.c line 763
Quoted 8 lines
drivers/hwmon/stts751.c new file +8
-0+ } + +dev_dbg (&client->dev,"gen_event: %s, gen_therm: %s" , + priv->gen_event ?"YES" :"NO" , + priv->gen_therm ?"YES" :"NO" ); + + priv->groups[groups_idx++] = &stts751_temp_group; + priv->groups[groups_idx++] = &stts751_interval_group;
Any reason for having multiple groups here ?
drivers/hwmon/stts751.c line 770
Quoted 7 lines
drivers/hwmon/stts751.c new file +7
-0+ +if (priv->gen_therm) + priv->groups[groups_idx++] = &stts751_therm_group; + +if (priv->gen_event) + priv->groups[groups_idx++] = &stts751_event_group; +
Why are those groups optional ? This is quite unusual. Even if the chip is not connected to alert or interrupt lines, it still supports limit registers and reports alerts. On top of that, at least interrupt support can be detected automatically (from client->irq) and would not need a devicetree property.
I think you really only need a single group, and all attributes should always be available.
drivers/hwmon/stts751.c line 771
Quoted 1 line
drivers/hwmon/stts751.c new file +1
-0+ priv->groups[groups_idx] =NULL ;
Unnecessary initialization. devm_kzalloc() initializes priv with 0.
Quoted 41 lines
drivers/hwmon/stts751.c new file +34
-0+ + ret =stts751_init_chip (priv); +if (ret) +return ret; + + priv->dev =devm_hwmon_device_register_with_groups (&client->dev, + client->name, priv, + priv->groups); +return PTR_ERR_OR_ZERO (priv->dev); +} + +static const struct i2c_device_id stts751_id[] = { + {"stts751" ,0 }, + { } +}; +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 <[email protected]>" ); +MODULE_DESCRIPTION ("STTS751 sensor driver" ); +MODULE_LICENSE ("GPL" );Signature · 6 lines
2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 43 lines · Guenter Roeck
Quoted 42 lines · Andrea Merello
This patch adds a HWMON driver for ST Microelectronics STTS751 temperature sensors.
It does support temperature reading (of course), SMBUS alert functionality and "therm" output.
Thanks-to: LABBE Corentin [for suggestions]
Thanks-to: Guenter Roeck [for suggestions/discussions]Signed-off-by Andrea Merello <[email protected]>Cc LABBE Corentin <[email protected]>Cc Guenter Roeck <[email protected]>Cc Jean Delvare <[email protected]>--- drivers/hwmon/Kconfig | 12 +- drivers/hwmon/Makefile | 2 +- drivers/hwmon/stts751.c | 805 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 817 insertions(+), 2 deletions(-) create mode 100644 drivers/hwmon/stts751.cdrivers/hwmon/Kconfig +10
-0@@ -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@@ -1506,7 +1516,7 @@ config SENSORS_ADS7871
Quoted 6 lines
drivers/hwmon/Kconfig config SENSORS_ADS7871 · line 1519
Quoted 4 lines
drivers/hwmon/Kconfig +1
-1config SENSORS_AMC6821 tristate "Texas Instruments AMC6821" - depends on I2C + depends on I2C???
My editor is kind enough to automatically kill trailing spaces.. I'll take care of moving this one in another patch in next series version..
Quoted 19 lines
Quoted 19 lines
drivers/hwmon/Kconfig +0
-0help If you say yes here you get support for the Texas Instruments AMC6821 hardware monitoring chips.drivers/hwmon/Makefile +1
-0@@ -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@@ -169,4 +170,3 @@ obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o obj-$(CONFIG_PMBUS) += pmbus/
Quoted 4 lines
drivers/hwmon/Makefile obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o · line 172
Quoted 2 lines
drivers/hwmon/Makefile +0
-1ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG -Looks like an unnecessary whitespace change.
Like above..
Quoted 23 lines
Quoted 23 lines
drivers/hwmon/stts751.c new file +16
-0@@ -0,0 +1,805 @@ +/* +* STTS751 sensor driver +* +* Copyright (C) 2016 Istituto Italiano di Tecnologia - RBCS - EDL +* Robotics, Brain and Cognitive Sciences department +* Electronic Design Laboratory +* +* Written by Andrea Merello <[email protected]> +* +* 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. +*
Quoted 8 lines
drivers/hwmon/stts751.c line 23
Quoted 7 lines
drivers/hwmon/stts751.c new file +7
-0+* 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. +*/ + +No douple empty lines please.
OK. BTW do you use some enhanced "checkpatch" script in order to catch issues like these?
Quoted 115 lines
Quoted 100 lines
drivers/hwmon/stts751.c new file +99
-0+#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/of_irq.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_TRIPL BIT(5) +#define STTS751_STATUS_TRIPH BIT(6) +#define STTS751_STATUS_BUSY BIT(8) +#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_ONESHOT 0x0F +#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 + +/* stick with HW defaults */ +#define STTS751_THERM_DEFAULT 85000 +#define STTS751_HYST_DEFAULT 10000 +#define STTS751_EVENT_MAX_DEFAULT 85000 +#define STTS751_EVENT_MIN_DEFAULT 0 + +#define STTS751_RACE_RETRY 5 +#define STTS751_CONV_TIMEOUT 100/* mS */ +#define STTS751_CACHE_TIME 100/* mS */ + +/* +* 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 +}; + +struct stts751_priv { +struct device *dev; +struct i2c_client *client; +struct mutex access_lock; +unsigned long interval; +int res; +bool gen_therm, gen_event; +int event_max, event_min; +int therm; +int hyst; +bool smbus_timeout; +int temp; +unsigned long last_update; +u8 config; +bool min_alert, max_alert; +bool data_valid; + +/* +* Temperature/interval is always present. +* Depending by DT therm and event are dynamically added. +* There are max 4 entries plus the guard +*/ +const struct attribute_group *groups[5 ]; +}; + +/* +* These functions converts temperature from HW format to integer format and +* vice-vers. They are (mostly) taken from lm90 driver. Unit is in mC. drivers/hwmon/stts751.c line 132
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+*/ +static int stts751_to_deg (s32 hw_val) +{ +return hw_val *125 /32 ; +} + +static s16 stts751_to_hw (int val,s32 *hw_val) +{ +/* HW works in range -64C to +127C */ +if ((val >127000 ) || (val <-64000 ))Extra ( ). However,
clamp_val(val, -64000, 127000)is preferred here instead of returning an error (we don't want to force users to guess valid ranges).
Quoted 1 line
The upper limit should probably be 127937, though, per datasheet.
OK
Quoted 11 lines
drivers/hwmon/stts751.c line 139
Quoted 7 lines
drivers/hwmon/stts751.c new file +7
-0+return -EINVAL ; + +if (val <0 ) + *hw_val = (val -62 ) /125 *32 ; +else + *hw_val = (val +62 ) /125 *32 ; +By having this function return an int, you could combine the 16-bit return value with the error code, and the extra parameter would not be necessary (even more so since this function should not return an error in the first place).
OK, now it's never failing.
Quoted 120 lines
Quoted 120 lines
drivers/hwmon/stts751.c new file +119
-0+return 0 ; +} + +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 ; + +mutex_lock (&priv->access_lock); + +/* +* 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: +mutex_unlock (&priv->access_lock); +if (ret) +return ret; + + priv->temp =stts751_to_deg ((integer1 <<8 ) | frac); +return ret; +} + +static int stts751_set_temp_reg (struct stts751_priv *priv,int temp, +bool is_frac,u8 hreg,u8 lreg) +{ +s32 hwval; +int ret; + +if (stts751_to_hw (temp, &hwval)) +return -EINVAL ; + +mutex_lock (&priv->access_lock); + ret =i2c_smbus_write_byte_data (priv->client, hreg, hwval >>8 ); +if (ret) + goto exit; +if (is_frac) + ret =i2c_smbus_write_byte_data (priv->client, + lreg, hwval &0xff ); +exit: +mutex_unlock (&priv->access_lock); + +return ret; +} + +static int stts751_update_alert (struct stts751_priv *priv) +{ +int ret;
Quoted 8 lines
drivers/hwmon/stts751.c line 265
Quoted 7 lines
drivers/hwmon/stts751.c new file +7
-0+ +/* not for us.. */ +if (!priv->gen_event) +return 0 ; + + ret =i2c_smbus_read_byte_data (priv->client,STTS751_REG_STATUS ); +Please no empty line between functions and checking of return values.
OK
Quoted 9 lines
drivers/hwmon/stts751.c line 271
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+if (ret <0 ) +return ret; + + priv->max_alert = priv->max_alert || !!(ret &STTS751_STATUS_TRIPH ); + priv->min_alert = priv->min_alert || !!(ret &STTS751_STATUS_TRIPL ); +I am not really happy about the caching of alert values. This isn't really common. Usually we report current alert values when reading. I don't really see why this driver should be different to all the others.
OK
Quoted 11 lines
Quoted 11 lines
drivers/hwmon/stts751.c new file +11
-0+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); +bool prev_max = priv->max_alert; +bool prev_min = priv->min_alert; +
Quoted 8 lines
drivers/hwmon/stts751.c line 287
Quoted 5 lines
drivers/hwmon/stts751.c new file +5
-0+if (type !=I2C_PROTOCOL_SMBUS_ALERT ) +return ; + +dev_dbg (&client->dev,"alert!" ); +Any chance to make this a bit more generic and also support interrupts ?
May you please elaborate?
IMHO adding the logic for handling SMBus alerts using IRQs to any HWmon driver is redundant, because it is already implemented in the SMBus layer, just right now you need patching SMBus layer for enabling it from the DT world (a couple of patches for this are floating around, but they seems not ready for merge yet)... Using this chip with a generic interrupt is indeed exactly what I'm doing..
.. Or am I completely missing your point ?
Quoted 63 lines
Quoted 63 lines
drivers/hwmon/stts751.c new file +62
-0+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; + +if (!(prev_max && prev_min)) { +dev_warn (&priv->client->dev, +"Alert received, but can't communicate to the device. Something bad happening? Triggering all alarms!" ); + } + } + +if (!prev_max && 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 (!prev_min && 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 ssize_t show_max_alert (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->max_alert); +} + +static ssize_t set_max_alert (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) +{ +struct stts751_priv *priv =dev_get_drvdata (dev); + +mutex_lock (&priv->access_lock); + priv->max_alert = false; +mutex_unlock (&priv->access_lock); + +return count; +} + +static ssize_t show_min_alert (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->min_alert); +} + +static ssize_t set_min_alert (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) +{
Quoted 8 lines
drivers/hwmon/stts751.c line 355
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+struct stts751_priv *priv =dev_get_drvdata (dev); + +mutex_lock (&priv->access_lock); + priv->min_alert = false; +mutex_unlock (&priv->access_lock); +This is highly unusual. If the alert needs to be cleared, please auto-clear it when reading.
OK
Quoted 46 lines
Quoted 46 lines
drivers/hwmon/stts751.c new file +46
-0+return count; +} + +static ssize_t show_input (struct device *dev,struct device_attribute *attr, +char *buf) +{ +int ret; +int cache_time =STTS751_CACHE_TIME *HZ /1000 ; +struct stts751_priv *priv =dev_get_drvdata (dev); + +/* +* Adjust the cache time wrt the sample rate. We do 4X in order to get +* a new measure in no more than 1/4 of the sample time (that seemed +* reasonable to me). +*/ + cache_time = stts751_intervals[priv->interval] /4 *HZ /1000 ; + +if (time_after (jiffies, priv->last_update + cache_time) || + !priv->data_valid) { + ret =stts751_update_temp (priv); +if (ret) +return ret; + priv->last_update = jiffies; + priv->data_valid = true; + } + +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 )
Quoted 8 lines
drivers/hwmon/stts751.c line 407
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+return -EINVAL ; + + ret =stts751_set_temp_reg (priv, temp, false,STTS751_REG_TLIM ,0 ); +if (ret) +return ret; +Does it really add value to use the same function for 8- and 16-bit registers ? Seems to me it just makes the code more complicated.
I haven't a strong opinion about this.. it's this complication vs a slight code duplication.. I've changed this in the way you suggest :)
Quoted 20 lines
Quoted 20 lines
drivers/hwmon/stts751.c new file +20
-0+dev_dbg (dev,"setting therm %ld" , temp); + + priv->therm = temp; +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;
Quoted 8 lines
drivers/hwmon/stts751.c line 432
Quoted 5 lines
drivers/hwmon/stts751.c new file +5
-0+struct stts751_priv *priv =dev_get_drvdata (dev); + +if (kstrtol (buf,10 , &temp) <0 ) +return -EINVAL ; +Per ABI, this is supposed to be an absolute temperature. For the chip, it is a delta. You'll have to do some calculation to convert the absolute value into a delta. Same for the show command.
OK
Quoted 42 lines
Quoted 42 lines
drivers/hwmon/stts751.c new file +41
-0+ ret =stts751_set_temp_reg (priv, temp, false,STTS751_REG_HYST ,0 ); +if (ret) +return ret; + +dev_dbg (dev,"setting hyst %ld" , temp); + + priv->hyst = temp; +return count; +} + +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 ; + + ret =stts751_set_temp_reg (priv, temp, true, +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) +{
Quoted 8 lines
drivers/hwmon/stts751.c line 479
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+struct stts751_priv *priv =dev_get_drvdata (dev); + +return snprintf (buf,PAGE_SIZE -1 ,"%d\n" , priv->event_min); +} + +Single empty line, please.
OK
Quoted 74 lines
Quoted 62 lines
drivers/hwmon/stts751.c new file +61
-0+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 ; + + ret =stts751_set_temp_reg (priv, temp, true, +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); + +/* 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);drivers/hwmon/stts751.c line 550
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+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; +Extra empty line.
Quoted 2 lines
Is it really necessary to have this code twice ? What is the harm in just doing it once, either before or after setting the conversion rate ?
In early development stages I've become suspicious about the chip starting to misbehave if I ever set, even briefly, an invalid configuration.
I tried to avoid that by setting rate and resolution in such an order that never ends up in an invalid configuration. The order is thus not fixed and depends by what changes are you doing..
(I'm not completely sure: I've hit something strange and I thought it could be because of this, and once I changed my code this way I never hit that again)..
Quoted 28 lines
Quoted 28 lines
drivers/hwmon/stts751.c new file +28
-0+ } +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 :
Quoted 8 lines
drivers/hwmon/stts751.c line 585
Quoted 7 lines
drivers/hwmon/stts751.c new file +7
-0+ name ="STTS751-1" ; +break ; +default : +return -ENODEV ; + } +dev_info (&new_client->dev,"Chip %s detected!" , name); +Is this noise necessary ?
OK, changed in dev_dbg.
Quoted 6 lines
drivers/hwmon/stts751.c line 588
Quoted 4 lines
drivers/hwmon/stts751.c new file +3
-0+ rev_id =i2c_smbus_read_byte_data (new_client,STTS751_REG_REV_ID ); + +if (rev_id !=0x1 ) {You might want to check for an error here (and return
-ENODEVif you get one).
OK
Quoted 10 lines
drivers/hwmon/stts751.c line 594
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+dev_notice (&new_client->dev, +"Chip revision 0x%x is untested\nPlease report whether it works to [email protected]" , + rev_id); + } + +strlcpy (info->type, name,I2C_NAME_SIZE );This doesn't match the driver name (or, rather, the values in
stts751_id). Does this instantiate the driver ? That would surprise me.
It does, but I'll double check what the heck I've done here :)
Quoted 35 lines
Quoted 35 lines
drivers/hwmon/stts751.c new file +35
-0+return 0 ; +} + +static int stts751_init_chip (struct stts751_priv *priv) +{ +int ret; + + priv->config =STTS751_CONF_EVENT_DIS |STTS751_CONF_STOP ; + ret =i2c_smbus_write_byte_data (priv->client,STTS751_REG_CONF , + priv->config); +if (ret) +return ret; + + ret =i2c_smbus_write_byte_data (priv->client,STTS751_REG_RATE , + priv->interval); +if (ret) +return ret; + +/* invalid, to force update */ + priv->res =-1 ; + ret =stts751_adjust_resolution (priv); +if (ret) +return ret; + + ret =i2c_smbus_write_byte_data (priv->client, +STTS751_REG_SMBUS_TO , + priv->smbus_timeout ?0x80 :0 ); +if (ret) +return ret; + + +if (priv->gen_event) { + ret =stts751_set_temp_reg (priv, priv->event_max, true, +STTS751_REG_HLIM_H ,STTS751_REG_HLIM_L ); +if (ret)
Quoted 8 lines
drivers/hwmon/stts751.c line 633
Quoted 4 lines
drivers/hwmon/stts751.c new file +4
-0+return ret; + + ret =stts751_set_temp_reg (priv, priv->event_min, true, +STTS751_REG_LLIM_H ,STTS751_REG_LLIM_L );Those registers all have power-up defaults. Unless there is a good reason, we should not overwrite them. In some applications, the values may be initialized from the BIOS/ROMMON.
OK. I've changed it for all those regs but the smsbus timeout stuff:
I think that this depends by the kind of I2C controller the device is attached to, so I'd still getting that from DT. Even if someone already initialized it (assuming it did it correctly, and the DT is correct also) I'll eventually end up in overwriting it with the same value.
Do you agree with this ?
Quoted 31 lines
Quoted 31 lines
drivers/hwmon/stts751.c new file +31
-0+if (ret) +return ret; + priv->config &= ~STTS751_CONF_EVENT_DIS ; + } + +if (priv->gen_therm) { + ret =stts751_set_temp_reg (priv, priv->therm, false, +STTS751_REG_TLIM ,0 ); +if (ret) +return ret; + + ret =stts751_set_temp_reg (priv, priv->hyst, false, +STTS751_REG_HYST ,0 ); +if (ret) +return ret; + } + + priv->config &= ~STTS751_CONF_STOP ; + ret =i2c_smbus_write_byte_data (priv->client, +STTS751_REG_CONF , priv->config); + +return ret; +} + +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO, + show_min, set_min, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO, + show_max, set_max, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO, + show_min_alert, set_min_alert, 0);
Quoted 8 lines
drivers/hwmon/stts751.c line 670
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO, + show_max_alert, set_max_alert, 0); +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm, + set_therm, 0); +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO, show_hyst, + set_hyst, 0);Please use standard attribute names.
I can convert the event stuff in temp1_[min/max]_alarm for limits and temp1_alarm for the flag, but it's not obvious to be what should I use for the therm stuff.. May you please give me advices about this?
I'd propose temp1_therm_[max/min]_hyst, but they seems not standard anyway...
Quoted 68 lines
Quoted 68 lines
drivers/hwmon/stts751.c new file +67
-0+static SENSOR_DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, + show_interval, set_interval, 0); + +/* always present */ +static struct attribute *stts751_temp_attrs[] = { + &sensor_dev_attr_temp1_input.dev_attr.attr, +NULL +}; + +static struct attribute_group stts751_temp_group = { + .attrs = stts751_temp_attrs, +}; + +/* present when therm pin or event pin are connected */ +static struct attribute *stts751_interval_attrs[] = { + &sensor_dev_attr_update_interval.dev_attr.attr, +NULL +}; + +static struct attribute_group stts751_interval_group = { + .attrs = stts751_interval_attrs, +}; + +/* present when event pin is connected */ +static struct attribute *stts751_event_attrs[] = { + &sensor_dev_attr_temp1_event_min.dev_attr.attr, + &sensor_dev_attr_temp1_event_max.dev_attr.attr, + &sensor_dev_attr_temp1_event_min_alert.dev_attr.attr, + &sensor_dev_attr_temp1_event_max_alert.dev_attr.attr, +NULL +}; + +static struct attribute_group stts751_event_group = { + .attrs = stts751_event_attrs, +}; + +/* present when therm pin is connected */ +static struct attribute *stts751_therm_attrs[] = { + &sensor_dev_attr_temp1_therm.dev_attr.attr, + &sensor_dev_attr_temp1_therm_hyst.dev_attr.attr, +NULL +}; + +static struct attribute_group stts751_therm_group = { + .attrs = stts751_therm_attrs, +}; + +static int stts751_probe (struct i2c_client *client, +const struct i2c_device_id *id) +{ +struct stts751_priv *priv; +int ret; +int groups_idx =0 ; +struct device_node *np = client->dev.of_node; + + 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); + +/* default to 2 samples per second */ + priv->interval =5 ; +/* default to timeout enable, as per chip default */ + priv->smbus_timeout = true;
Quoted 8 lines
drivers/hwmon/stts751.c line 743
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+ priv->last_update =0 ; + priv->data_valid = false; + priv->max_alert = false; + priv->min_alert = false; + priv->gen_therm = false; + priv->gen_event = false;Unnecessary initializations.
OK
Quoted 9 lines
drivers/hwmon/stts751.c line 748
Quoted 6 lines
drivers/hwmon/stts751.c new file +5
-0+ priv->therm =STTS751_THERM_DEFAULT ; + priv->hyst =STTS751_HYST_DEFAULT ; + priv->event_max =STTS751_EVENT_MAX_DEFAULT ; + priv->event_min =STTS751_EVENT_MIN_DEFAULT ; +I think it would be better to read the initial values from the chip (also see above).
OK
Quoted 9 lines
drivers/hwmon/stts751.c line 755
Quoted 7 lines
drivers/hwmon/stts751.c new file +7
-0+if (np) { + priv->gen_therm =of_property_read_bool (np,"has-therm" ); + priv->gen_event =of_property_read_bool (np,"has-event" ); + priv->smbus_timeout = !of_property_read_bool (np, +"smbus-timeout-disable" ); + }else { +dev_notice (&client->dev,"No DT data. Event/therm disabled\n" );Seems to be quite rude to penaltize non-DT systems this way.
That's gone since the two props "has-therm" and "has-event" are now gone. Any suggestion please about how to get the "smbus-timeout-disable" stuff in non-DT case ?
Quoted 10 lines
drivers/hwmon/stts751.c line 763
Quoted 8 lines
drivers/hwmon/stts751.c new file +8
-0+ } + +dev_dbg (&client->dev,"gen_event: %s, gen_therm: %s" , + priv->gen_event ?"YES" :"NO" , + priv->gen_therm ?"YES" :"NO" ); + + priv->groups[groups_idx++] = &stts751_temp_group; + priv->groups[groups_idx++] = &stts751_interval_group;Any reason for having multiple groups here ?
Not anymore, as per your suggesion below, they are gone :)
Quoted 11 lines
drivers/hwmon/stts751.c line 770
Quoted 7 lines
drivers/hwmon/stts751.c new file +7
-0+ +if (priv->gen_therm) + priv->groups[groups_idx++] = &stts751_therm_group; + +if (priv->gen_event) + priv->groups[groups_idx++] = &stts751_event_group; +Why are those groups optional ? This is quite unusual. Even if the chip is not connected to alert or interrupt lines, it still supports limit registers and reports alerts. On top of that, at least interrupt support can be detected automatically (from
client->irq) and would not need a devicetree property.
Props "has-therm" and "has-event" gone; please see above about IRQ stuf..
Quoted 2 lines
I think you really only need a single group, and all attributes should always be available.
OK
Quoted 4 lines
drivers/hwmon/stts751.c line 771
Quoted 2 lines
drivers/hwmon/stts751.c new file +1
-0+ priv->groups[groups_idx] =NULL ;Unnecessary initialization.
devm_kzalloc()initializes priv with 0.
OK
Quoted 40 lines
Quoted 40 lines
drivers/hwmon/stts751.c new file +34
-1+ + ret =stts751_init_chip (priv); +if (ret) +return ret; + + priv->dev =devm_hwmon_device_register_with_groups (&client->dev, + client->name, priv, + priv->groups); +return PTR_ERR_OR_ZERO (priv->dev); +} + +static const struct i2c_device_id stts751_id[] = { + {"stts751" ,0 }, + { } +}; +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 <[email protected]>" ); +MODULE_DESCRIPTION ("STTS751 sensor driver" ); +MODULE_LICENSE ("GPL" ); --2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Guenter Roeck
maintainerQuoted 112 lines · Andrea Merello
Quoted 43 lines · Guenter Roeck
Quoted 42 lines · Andrea Merello
This patch adds a HWMON driver for ST Microelectronics STTS751 temperature sensors.
It does support temperature reading (of course), SMBUS alert functionality and "therm" output.
Thanks-to: LABBE Corentin [for suggestions]
Thanks-to: Guenter Roeck [for suggestions/discussions]Signed-off-by Andrea Merello <[email protected]>Cc LABBE Corentin <[email protected]>Cc Guenter Roeck <[email protected]>Cc Jean Delvare <[email protected]>--- drivers/hwmon/Kconfig | 12 +- drivers/hwmon/Makefile | 2 +- drivers/hwmon/stts751.c | 805 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 817 insertions(+), 2 deletions(-) create mode 100644 drivers/hwmon/stts751.cdrivers/hwmon/Kconfig +10
-0@@ -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@@ -1506,7 +1516,7 @@ config SENSORS_ADS7871Quoted 6 lines
drivers/hwmon/Kconfig config SENSORS_ADS7871 · line 1519
Quoted 4 lines
drivers/hwmon/Kconfig +1
-1config SENSORS_AMC6821 tristate "Texas Instruments AMC6821" - depends on I2C + depends on I2C???
My editor is kind enough to automatically kill trailing spaces.. I'll take care of moving this one in another patch in next series version..
Quoted 19 lines
Quoted 19 lines
drivers/hwmon/Kconfig +0
-0help If you say yes here you get support for the Texas Instruments AMC6821 hardware monitoring chips.drivers/hwmon/Makefile +1
-0@@ -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@@ -169,4 +170,3 @@ obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o obj-$(CONFIG_PMBUS) += pmbus/Quoted 4 lines
drivers/hwmon/Makefile obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o · line 172
Quoted 2 lines
drivers/hwmon/Makefile +0
-1ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG -Looks like an unnecessary whitespace change.
Like above..
Quoted 23 lines
Quoted 23 lines
drivers/hwmon/stts751.c new file +16
-0@@ -0,0 +1,805 @@ +/* +* STTS751 sensor driver +* +* Copyright (C) 2016 Istituto Italiano di Tecnologia - RBCS - EDL +* Robotics, Brain and Cognitive Sciences department +* Electronic Design Laboratory +* +* Written by Andrea Merello <[email protected]> +* +* 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. +* Quoted 8 lines
drivers/hwmon/stts751.c line 23
Quoted 7 lines
drivers/hwmon/stts751.c new file +7
-0+* 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. +*/ + +No douple empty lines please.
Quoted 3 lines
OK. BTW do you use some enhanced "checkpatch" script in order to catch issues like these?
In this case my brain, but checkpatch --strict reports it as well.
Quoted 298 lines
Quoted 115 lines
Quoted 100 lines
drivers/hwmon/stts751.c new file +99
-0+#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/of_irq.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_TRIPL BIT(5) +#define STTS751_STATUS_TRIPH BIT(6) +#define STTS751_STATUS_BUSY BIT(8) +#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_ONESHOT 0x0F +#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 + +/* stick with HW defaults */ +#define STTS751_THERM_DEFAULT 85000 +#define STTS751_HYST_DEFAULT 10000 +#define STTS751_EVENT_MAX_DEFAULT 85000 +#define STTS751_EVENT_MIN_DEFAULT 0 + +#define STTS751_RACE_RETRY 5 +#define STTS751_CONV_TIMEOUT 100/* mS */ +#define STTS751_CACHE_TIME 100/* mS */ + +/* +* 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 +}; + +struct stts751_priv { +struct device *dev; +struct i2c_client *client; +struct mutex access_lock; +unsigned long interval; +int res; +bool gen_therm, gen_event; +int event_max, event_min; +int therm; +int hyst; +bool smbus_timeout; +int temp; +unsigned long last_update; +u8 config; +bool min_alert, max_alert; +bool data_valid; + +/* +* Temperature/interval is always present. +* Depending by DT therm and event are dynamically added. +* There are max 4 entries plus the guard +*/ +const struct attribute_group *groups[5 ]; +}; + +/* +* These functions converts temperature from HW format to integer format and +* vice-vers. They are (mostly) taken from lm90 driver. Unit is in mC. drivers/hwmon/stts751.c line 132
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+*/ +static int stts751_to_deg (s32 hw_val) +{ +return hw_val *125 /32 ; +} + +static s16 stts751_to_hw (int val,s32 *hw_val) +{ +/* HW works in range -64C to +127C */ +if ((val >127000 ) || (val <-64000 ))Extra ( ). However,
clamp_val(val, -64000, 127000)is preferred here instead of returning an error (we don't want to force users to guess valid ranges).Quoted 1 line
The upper limit should probably be 127937, though, per datasheet.
OK
Quoted 11 lines
drivers/hwmon/stts751.c line 139
Quoted 7 lines
drivers/hwmon/stts751.c new file +7
-0+return -EINVAL ; + +if (val <0 ) + *hw_val = (val -62 ) /125 *32 ; +else + *hw_val = (val +62 ) /125 *32 ; +By having this function return an int, you could combine the 16-bit return value with the error code, and the extra parameter would not be necessary (even more so since this function should not return an error in the first place).
OK, now it's never failing.
Quoted 120 lines
Quoted 120 lines
drivers/hwmon/stts751.c new file +119
-0+return 0 ; +} + +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 ; + +mutex_lock (&priv->access_lock); + +/* +* 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: +mutex_unlock (&priv->access_lock); +if (ret) +return ret; + + priv->temp =stts751_to_deg ((integer1 <<8 ) | frac); +return ret; +} + +static int stts751_set_temp_reg (struct stts751_priv *priv,int temp, +bool is_frac,u8 hreg,u8 lreg) +{ +s32 hwval; +int ret; + +if (stts751_to_hw (temp, &hwval)) +return -EINVAL ; + +mutex_lock (&priv->access_lock); + ret =i2c_smbus_write_byte_data (priv->client, hreg, hwval >>8 ); +if (ret) + goto exit; +if (is_frac) + ret =i2c_smbus_write_byte_data (priv->client, + lreg, hwval &0xff ); +exit: +mutex_unlock (&priv->access_lock); + +return ret; +} + +static int stts751_update_alert (struct stts751_priv *priv) +{ +int ret;Quoted 8 lines
drivers/hwmon/stts751.c line 265
Quoted 7 lines
drivers/hwmon/stts751.c new file +7
-0+ +/* not for us.. */ +if (!priv->gen_event) +return 0 ; + + ret =i2c_smbus_read_byte_data (priv->client,STTS751_REG_STATUS ); +Please no empty line between functions and checking of return values.
OK
Quoted 9 lines
drivers/hwmon/stts751.c line 271
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+if (ret <0 ) +return ret; + + priv->max_alert = priv->max_alert || !!(ret &STTS751_STATUS_TRIPH ); + priv->min_alert = priv->min_alert || !!(ret &STTS751_STATUS_TRIPL ); +I am not really happy about the caching of alert values. This isn't really common. Usually we report current alert values when reading. I don't really see why this driver should be different to all the others.
OK
Quoted 11 lines
Quoted 11 lines
drivers/hwmon/stts751.c new file +11
-0+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); +bool prev_max = priv->max_alert; +bool prev_min = priv->min_alert; +Quoted 8 lines
drivers/hwmon/stts751.c line 287
Quoted 5 lines
drivers/hwmon/stts751.c new file +5
-0+if (type !=I2C_PROTOCOL_SMBUS_ALERT ) +return ; + +dev_dbg (&client->dev,"alert!" ); +Any chance to make this a bit more generic and also support interrupts ?
May you please elaborate?
Quoted 9 lines
IMHO adding the logic for handling SMBus alerts using IRQs to any HWmon driver is redundant, because it is already implemented in the SMBus layer, just right now you need patching SMBus layer for enabling it from the DT world (a couple of patches for this are floating around, but they seems not ready for merge yet)... Using this chip with a generic interrupt is indeed exactly what I'm doing..
.. Or am I completely missing your point ?
"just right now you need patching SMBus layer for enabling it from the DT world"
So it _doesn't_ work, correct ?
The problem with SMBus alert is that each chip implements it differently, to the point where it is difficult to implement it such that it supports more than one chip on the same bus. For the most part, chip interrupt lines are thus not connected to the i2c controller but to, for example, a gpio pin, and each chip on the same bus has a separate interrupt. I am looking forward to see how this is going to be supported in the i2c infrastructure. For my part, I long since gave up on it, but I'll be happy to stand corrected.
Quoted 297 lines
Quoted 63 lines
Quoted 63 lines
drivers/hwmon/stts751.c new file +62
-0+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; + +if (!(prev_max && prev_min)) { +dev_warn (&priv->client->dev, +"Alert received, but can't communicate to the device. Something bad happening? Triggering all alarms!" ); + } + } + +if (!prev_max && 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 (!prev_min && 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 ssize_t show_max_alert (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->max_alert); +} + +static ssize_t set_max_alert (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) +{ +struct stts751_priv *priv =dev_get_drvdata (dev); + +mutex_lock (&priv->access_lock); + priv->max_alert = false; +mutex_unlock (&priv->access_lock); + +return count; +} + +static ssize_t show_min_alert (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->min_alert); +} + +static ssize_t set_min_alert (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) +{Quoted 8 lines
drivers/hwmon/stts751.c line 355
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+struct stts751_priv *priv =dev_get_drvdata (dev); + +mutex_lock (&priv->access_lock); + priv->min_alert = false; +mutex_unlock (&priv->access_lock); +This is highly unusual. If the alert needs to be cleared, please auto-clear it when reading.
OK
Quoted 46 lines
Quoted 46 lines
drivers/hwmon/stts751.c new file +46
-0+return count; +} + +static ssize_t show_input (struct device *dev,struct device_attribute *attr, +char *buf) +{ +int ret; +int cache_time =STTS751_CACHE_TIME *HZ /1000 ; +struct stts751_priv *priv =dev_get_drvdata (dev); + +/* +* Adjust the cache time wrt the sample rate. We do 4X in order to get +* a new measure in no more than 1/4 of the sample time (that seemed +* reasonable to me). +*/ + cache_time = stts751_intervals[priv->interval] /4 *HZ /1000 ; + +if (time_after (jiffies, priv->last_update + cache_time) || + !priv->data_valid) { + ret =stts751_update_temp (priv); +if (ret) +return ret; + priv->last_update = jiffies; + priv->data_valid = true; + } + +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 )Quoted 8 lines
drivers/hwmon/stts751.c line 407
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+return -EINVAL ; + + ret =stts751_set_temp_reg (priv, temp, false,STTS751_REG_TLIM ,0 ); +if (ret) +return ret; +Does it really add value to use the same function for 8- and 16-bit registers ? Seems to me it just makes the code more complicated.
I haven't a strong opinion about this.. it's this complication vs a slight code duplication.. I've changed this in the way you suggest :)
Quoted 20 lines
Quoted 20 lines
drivers/hwmon/stts751.c new file +20
-0+dev_dbg (dev,"setting therm %ld" , temp); + + priv->therm = temp; +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;Quoted 8 lines
drivers/hwmon/stts751.c line 432
Quoted 5 lines
drivers/hwmon/stts751.c new file +5
-0+struct stts751_priv *priv =dev_get_drvdata (dev); + +if (kstrtol (buf,10 , &temp) <0 ) +return -EINVAL ; +Per ABI, this is supposed to be an absolute temperature. For the chip, it is a delta. You'll have to do some calculation to convert the absolute value into a delta. Same for the show command.
OK
Quoted 42 lines
Quoted 42 lines
drivers/hwmon/stts751.c new file +41
-0+ ret =stts751_set_temp_reg (priv, temp, false,STTS751_REG_HYST ,0 ); +if (ret) +return ret; + +dev_dbg (dev,"setting hyst %ld" , temp); + + priv->hyst = temp; +return count; +} + +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 ; + + ret =stts751_set_temp_reg (priv, temp, true, +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) +{Quoted 8 lines
drivers/hwmon/stts751.c line 479
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+struct stts751_priv *priv =dev_get_drvdata (dev); + +return snprintf (buf,PAGE_SIZE -1 ,"%d\n" , priv->event_min); +} + +Single empty line, please.
OK
Quoted 74 lines
Quoted 62 lines
drivers/hwmon/stts751.c new file +61
-0+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 ; + + ret =stts751_set_temp_reg (priv, temp, true, +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); + +/* 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);drivers/hwmon/stts751.c line 550
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+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; +Extra empty line.
Quoted 2 lines
Is it really necessary to have this code twice ? What is the harm in just doing it once, either before or after setting the conversion rate ?
In early development stages I've become suspicious about the chip starting to misbehave if I ever set, even briefly, an invalid configuration.
Quoted 8 lines
I tried to avoid that by setting rate and resolution in such an order that never ends up in an invalid configuration. The order is thus not fixed and depends by what changes are you doing..
(I'm not completely sure: I've hit something strange and I thought it could be because of this, and once I changed my code this way I never hit that again)..
Please add a respective comment into the code describing the reasons.
Quoted 55 lines
Quoted 28 lines
Quoted 28 lines
drivers/hwmon/stts751.c new file +28
-0+ } +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 :Quoted 8 lines
drivers/hwmon/stts751.c line 585
Quoted 7 lines
drivers/hwmon/stts751.c new file +7
-0+ name ="STTS751-1" ; +break ; +default : +return -ENODEV ; + } +dev_info (&new_client->dev,"Chip %s detected!" , name); +Is this noise necessary ?
OK, changed in
dev_dbg.Quoted 6 lines
drivers/hwmon/stts751.c line 588
Quoted 4 lines
drivers/hwmon/stts751.c new file +3
-0+ rev_id =i2c_smbus_read_byte_data (new_client,STTS751_REG_REV_ID ); + +if (rev_id !=0x1 ) {You might want to check for an error here (and return
-ENODEVif you get one).OK
Quoted 7 lines
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+dev_notice (&new_client->dev, +"Chip revision 0x%x is untested\nPlease report whether it works to [email protected]" , + rev_id); + } + +strlcpy (info->type, name,I2C_NAME_SIZE );
Quoted 6 lines
Quoted 3 lines
This doesn't match the driver name (or, rather, the values in
stts751_id). Does this instantiate the driver ? That would surprise me.It does, but I'll double check what the heck I've done here :)
Either case this should return a string matching stts751_id[], otherwise auto-detection and manual or devicetree based instantiation would return different names.
Does your use case even include the detect function ?
Quoted 44 lines
Quoted 35 lines
Quoted 35 lines
drivers/hwmon/stts751.c new file +35
-0+return 0 ; +} + +static int stts751_init_chip (struct stts751_priv *priv) +{ +int ret; + + priv->config =STTS751_CONF_EVENT_DIS |STTS751_CONF_STOP ; + ret =i2c_smbus_write_byte_data (priv->client,STTS751_REG_CONF , + priv->config); +if (ret) +return ret; + + ret =i2c_smbus_write_byte_data (priv->client,STTS751_REG_RATE , + priv->interval); +if (ret) +return ret; + +/* invalid, to force update */ + priv->res =-1 ; + ret =stts751_adjust_resolution (priv); +if (ret) +return ret; + + ret =i2c_smbus_write_byte_data (priv->client, +STTS751_REG_SMBUS_TO , + priv->smbus_timeout ?0x80 :0 ); +if (ret) +return ret; + + +if (priv->gen_event) { + ret =stts751_set_temp_reg (priv, priv->event_max, true, +STTS751_REG_HLIM_H ,STTS751_REG_HLIM_L ); +if (ret)Quoted 8 lines
drivers/hwmon/stts751.c line 633
Quoted 4 lines
drivers/hwmon/stts751.c new file +4
-0+return ret; + + ret =stts751_set_temp_reg (priv, priv->event_min, true, +STTS751_REG_LLIM_H ,STTS751_REG_LLIM_L );Those registers all have power-up defaults. Unless there is a good reason, we should not overwrite them. In some applications, the values may be initialized from the BIOS/ROMMON.
Quoted 9 lines
OK. I've changed it for all those regs but the smsbus timeout stuff:
I think that this depends by the kind of I2C controller the device is attached to, so I'd still getting that from DT. Even if someone already initialized it (assuming it did it correctly, and the DT is correct also) I'll eventually end up in overwriting it with the same value.Do you agree with this ?
Ok to overwrite it if you have devicetree data, but otherwise you should not touch it.
Quoted 40 lines
Quoted 31 lines
Quoted 31 lines
drivers/hwmon/stts751.c new file +31
-0+if (ret) +return ret; + priv->config &= ~STTS751_CONF_EVENT_DIS ; + } + +if (priv->gen_therm) { + ret =stts751_set_temp_reg (priv, priv->therm, false, +STTS751_REG_TLIM ,0 ); +if (ret) +return ret; + + ret =stts751_set_temp_reg (priv, priv->hyst, false, +STTS751_REG_HYST ,0 ); +if (ret) +return ret; + } + + priv->config &= ~STTS751_CONF_STOP ; + ret =i2c_smbus_write_byte_data (priv->client, +STTS751_REG_CONF , priv->config); + +return ret; +} + +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO, + show_min, set_min, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO, + show_max, set_max, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO, + show_min_alert, set_min_alert, 0);Quoted 8 lines
drivers/hwmon/stts751.c line 670
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO, + show_max_alert, set_max_alert, 0); +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm, + set_therm, 0); +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO, show_hyst, + set_hyst, 0);Please use standard attribute names.
Quoted 6 lines
I can convert the event stuff in temp1_[min/max]_alarm for limits and
temp1_alarmfor the flag, but it's not obvious to be what should I use for the therm stuff.. May you please give me advices about this?I'd propose temp1_therm_[max/min]_hyst, but they seems not standard anyway...
From the chip datasheet, this is supposed to reflect overtemperature conditions, stronger than high/low, thus 'crit' seems appropriate.
Quoted 99 lines
Quoted 68 lines
Quoted 68 lines
drivers/hwmon/stts751.c new file +67
-0+static SENSOR_DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, + show_interval, set_interval, 0); + +/* always present */ +static struct attribute *stts751_temp_attrs[] = { + &sensor_dev_attr_temp1_input.dev_attr.attr, +NULL +}; + +static struct attribute_group stts751_temp_group = { + .attrs = stts751_temp_attrs, +}; + +/* present when therm pin or event pin are connected */ +static struct attribute *stts751_interval_attrs[] = { + &sensor_dev_attr_update_interval.dev_attr.attr, +NULL +}; + +static struct attribute_group stts751_interval_group = { + .attrs = stts751_interval_attrs, +}; + +/* present when event pin is connected */ +static struct attribute *stts751_event_attrs[] = { + &sensor_dev_attr_temp1_event_min.dev_attr.attr, + &sensor_dev_attr_temp1_event_max.dev_attr.attr, + &sensor_dev_attr_temp1_event_min_alert.dev_attr.attr, + &sensor_dev_attr_temp1_event_max_alert.dev_attr.attr, +NULL +}; + +static struct attribute_group stts751_event_group = { + .attrs = stts751_event_attrs, +}; + +/* present when therm pin is connected */ +static struct attribute *stts751_therm_attrs[] = { + &sensor_dev_attr_temp1_therm.dev_attr.attr, + &sensor_dev_attr_temp1_therm_hyst.dev_attr.attr, +NULL +}; + +static struct attribute_group stts751_therm_group = { + .attrs = stts751_therm_attrs, +}; + +static int stts751_probe (struct i2c_client *client, +const struct i2c_device_id *id) +{ +struct stts751_priv *priv; +int ret; +int groups_idx =0 ; +struct device_node *np = client->dev.of_node; + + 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); + +/* default to 2 samples per second */ + priv->interval =5 ; +/* default to timeout enable, as per chip default */ + priv->smbus_timeout = true;Quoted 8 lines
drivers/hwmon/stts751.c line 743
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+ priv->last_update =0 ; + priv->data_valid = false; + priv->max_alert = false; + priv->min_alert = false; + priv->gen_therm = false; + priv->gen_event = false;Unnecessary initializations.
OK
Quoted 9 lines
drivers/hwmon/stts751.c line 748
Quoted 6 lines
drivers/hwmon/stts751.c new file +5
-0+ priv->therm =STTS751_THERM_DEFAULT ; + priv->hyst =STTS751_HYST_DEFAULT ; + priv->event_max =STTS751_EVENT_MAX_DEFAULT ; + priv->event_min =STTS751_EVENT_MIN_DEFAULT ; +I think it would be better to read the initial values from the chip (also see above).
OK
Quoted 8 lines
Quoted 7 lines
drivers/hwmon/stts751.c new file +7
-0+if (np) { + priv->gen_therm =of_property_read_bool (np,"has-therm" ); + priv->gen_event =of_property_read_bool (np,"has-event" ); + priv->smbus_timeout = !of_property_read_bool (np, +"smbus-timeout-disable" ); + }else { +dev_notice (&client->dev,"No DT data. Event/therm disabled\n" );
Quoted 6 lines
Quoted 1 line
Seems to be quite rude to penaltize non-DT systems this way.
That's gone since the two props "has-therm" and "has-event" are now gone. Any suggestion please about how to get the "smbus-timeout-disable" stuff in non-DT case ?
Platform data, or better use device properties for both cases.
Quoted 80 lines
Quoted 10 lines
drivers/hwmon/stts751.c line 763
Quoted 8 lines
drivers/hwmon/stts751.c new file +8
-0+ } + +dev_dbg (&client->dev,"gen_event: %s, gen_therm: %s" , + priv->gen_event ?"YES" :"NO" , + priv->gen_therm ?"YES" :"NO" ); + + priv->groups[groups_idx++] = &stts751_temp_group; + priv->groups[groups_idx++] = &stts751_interval_group;Any reason for having multiple groups here ?
Not anymore, as per your suggesion below, they are gone :)
Quoted 11 lines
drivers/hwmon/stts751.c line 770
Quoted 7 lines
drivers/hwmon/stts751.c new file +7
-0+ +if (priv->gen_therm) + priv->groups[groups_idx++] = &stts751_therm_group; + +if (priv->gen_event) + priv->groups[groups_idx++] = &stts751_event_group; +Why are those groups optional ? This is quite unusual. Even if the chip is not connected to alert or interrupt lines, it still supports limit registers and reports alerts. On top of that, at least interrupt support can be detected automatically (from
client->irq) and would not need a devicetree property.Props "has-therm" and "has-event" gone; please see above about IRQ stuf..
Quoted 2 lines
I think you really only need a single group, and all attributes should always be available.
OK
Quoted 4 lines
drivers/hwmon/stts751.c line 771
Quoted 2 lines
drivers/hwmon/stts751.c new file +1
-0+ priv->groups[groups_idx] =NULL ;Unnecessary initialization.
devm_kzalloc()initializes priv with 0.OK
Quoted 40 lines
Quoted 40 lines
drivers/hwmon/stts751.c new file +34
-1+ + ret =stts751_init_chip (priv); +if (ret) +return ret; + + priv->dev =devm_hwmon_device_register_with_groups (&client->dev, + client->name, priv, + priv->groups); +return PTR_ERR_OR_ZERO (priv->dev); +} + +static const struct i2c_device_id stts751_id[] = { + {"stts751" ,0 }, + { } +}; +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 <[email protected]>" ); +MODULE_DESCRIPTION ("STTS751 sensor driver" ); +MODULE_LICENSE ("GPL" ); --2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 123 lines · Guenter Roeck
Quoted 122 lines · Andrea Merello
Quoted 46 lines · Guenter Roeck
Quoted 44 lines · Andrea Merello
This patch adds a HWMON driver for ST Microelectronics STTS751 temperature sensors.
It does support temperature reading (of course), SMBUS alert functionality and "therm" output.
Thanks-to: LABBE Corentin [for suggestions]
Thanks-to: Guenter Roeck [for suggestions/discussions]Signed-off-by Andrea Merello <[email protected]>Cc LABBE Corentin <[email protected]>Cc Guenter Roeck <[email protected]>Cc Jean Delvare <[email protected]>--- drivers/hwmon/Kconfig | 12 +- drivers/hwmon/Makefile | 2 +- drivers/hwmon/stts751.c | 805 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 817 insertions(+), 2 deletions(-) create mode 100644 drivers/hwmon/stts751.cdrivers/hwmon/Kconfig +10
-0@@ -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@@ -1506,7 +1516,7 @@ config SENSORS_ADS7871Quoted 7 lines
drivers/hwmon/Kconfig config SENSORS_ADS7871 · line 1519
Quoted 4 lines
drivers/hwmon/Kconfig +1
-1config SENSORS_AMC6821 tristate "Texas Instruments AMC6821" - depends on I2C + depends on I2C???
My editor is kind enough to automatically kill trailing spaces.. I'll take care of moving this one in another patch in next series version..
Quoted 19 lines
Quoted 19 lines
drivers/hwmon/Kconfig +0
-0help If you say yes here you get support for the Texas Instruments AMC6821 hardware monitoring chips.drivers/hwmon/Makefile +1
-0@@ -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@@ -169,4 +170,3 @@ obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o obj-$(CONFIG_PMBUS) += pmbus/Quoted 5 lines
drivers/hwmon/Makefile obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o · line 172
Quoted 2 lines
drivers/hwmon/Makefile +0
-1ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG -Looks like an unnecessary whitespace change.
Like above..
Quoted 32 lines
Quoted 20 lines
drivers/hwmon/stts751.c new file +13
-0@@ -0,0 +1,805 @@ +/* +* STTS751 sensor driver +* +* Copyright (C) 2016 Istituto Italiano di Tecnologia - RBCS - EDL +* Robotics, Brain and Cognitive Sciences department +* Electronic Design Laboratory +* +* Written by Andrea Merello <[email protected]> +* +* 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 drivers/hwmon/stts751.c line 23
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+* 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. +*/ + +No douple empty lines please.
Quoted 5 lines
Quoted 3 lines
OK. BTW do you use some enhanced "checkpatch" script in order to catch issues like these?
In this case my brain, but checkpatch --strict reports it as well.
OK, Thanks.
Quoted 301 lines
Quoted 301 lines
Quoted 119 lines
Quoted 104 lines
drivers/hwmon/stts751.c new file +65
-0+#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/of_irq.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_TRIPL BIT(5) +#define STTS751_STATUS_TRIPH BIT(6) +#define STTS751_STATUS_BUSY BIT(8) +#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_ONESHOT 0x0F +#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 + +/* stick with HW defaults */ +#define STTS751_THERM_DEFAULT 85000 +#define STTS751_HYST_DEFAULT 10000 +#define STTS751_EVENT_MAX_DEFAULT 85000 +#define STTS751_EVENT_MIN_DEFAULT 0 + +#define STTS751_RACE_RETRY 5 +#define STTS751_CONV_TIMEOUT 100/* mS */ +#define STTS751_CACHE_TIME 100/* mS */ + +/* +* 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.
+ */drivers/hwmon/stts751.c new file +31
-0+static const int stts751_intervals[] = { +16000 ,8000 ,4000 ,2000 ,1000 ,500 ,250 ,125 ,63 ,31 +}; + +struct stts751_priv { +struct device *dev; +struct i2c_client *client; +struct mutex access_lock; +unsigned long interval; +int res; +bool gen_therm, gen_event; +int event_max, event_min; +int therm; +int hyst; +bool smbus_timeout; +int temp; +unsigned long last_update; +u8 config; +bool min_alert, max_alert; +bool data_valid; + +/* +* Temperature/interval is always present. +* Depending by DT therm and event are dynamically added. +* There are max 4 entries plus the guard +*/ +const struct attribute_group *groups[5 ]; +}; + +/* +* These functions converts temperature from HW format to integer format and
+ * vice-vers. They are (mostly) taken from lm90 driver. Unit is in mC.
+ */drivers/hwmon/stts751.c
Quoted 9 lines
drivers/hwmon/stts751.c new file +9
-0+static int stts751_to_deg (s32 hw_val) +{ +return hw_val *125 /32 ; +} + +static s16 stts751_to_hw (int val,s32 *hw_val) +{ +/* HW works in range -64C to +127C */ +if ((val >127000 ) || (val <-64000 ))Extra ( ). However,
clamp_val(val, -64000, 127000)is preferred here instead of returning an error (we don't want to force users to guess valid ranges).Quoted 1 line
The upper limit should probably be 127937, though, per datasheet.
OK
Quoted 5 lines
Quoted 5 lines
drivers/hwmon/stts751.c new file +5
-0+return -EINVAL ; + +if (val <0 ) + *hw_val = (val -62 ) /125 *32 ; +else Quoted 8 lines
drivers/hwmon/stts751.c
Quoted 2 lines
drivers/hwmon/stts751.c new file +2
-0+ *hw_val = (val +62 ) /125 *32 ; +By having this function return an int, you could combine the 16-bit return
value with the error code, and the extra parameter would not be necessary (even more so since this function should not return an error in the first place).OK, now it's never failing.
Quoted 132 lines
Quoted 132 lines
drivers/hwmon/stts751.c new file +41
-0+return 0 ; +} + +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 ; + +mutex_lock (&priv->access_lock); + +/* +* 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
adrivers/hwmon/stts751.c new file +2
-0+* 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
correctdrivers/hwmon/stts751.c new file +3
-0+* reading. +*/ + integer1 = i2c_smbus_read_byte_data(priv->client,
STTS751_REG_TEMP_H);drivers/hwmon/stts751.c new file +16
-0+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);drivers/hwmon/stts751.c new file +13
-0+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);
drivers/hwmon/stts751.c new file +40
-0+ ret = frac; + goto exit; + } + } + +exit: +mutex_unlock (&priv->access_lock); +if (ret) +return ret; + + priv->temp =stts751_to_deg ((integer1 <<8 ) | frac); +return ret; +} + +static int stts751_set_temp_reg (struct stts751_priv *priv,int temp, +bool is_frac,u8 hreg,u8 lreg) +{ +s32 hwval; +int ret; + +if (stts751_to_hw (temp, &hwval)) +return -EINVAL ; + +mutex_lock (&priv->access_lock); + ret =i2c_smbus_write_byte_data (priv->client, hreg, hwval >>8 ); +if (ret) + goto exit; +if (is_frac) + ret =i2c_smbus_write_byte_data (priv->client, + lreg, hwval &0xff ); +exit: +mutex_unlock (&priv->access_lock); + +return ret; +} + +static int stts751_update_alert (struct stts751_priv *priv) +{ +int ret; +Quoted 8 lines
drivers/hwmon/stts751.c
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+/* not for us.. */ +if (!priv->gen_event) +return 0 ; + + ret =i2c_smbus_read_byte_data (priv->client,STTS751_REG_STATUS ); +Please no empty line between functions and checking of return values.
OK
Quoted 14 lines
Quoted 8 lines
drivers/hwmon/stts751.c new file +4
-0+if (ret <0 ) +return ret; + + priv->max_alert = priv->max_alert || !!(ret &
STTS751_STATUS_TRIPH);
+priv->min_alert=priv->min_alert|| !!(ret &STTS751_STATUS_TRIPL);
+I am not really happy about the caching of alert values. This isn't really
common. Usually we report current alert values when reading. I don't really
see why this driver should be different to all the others.
Quoted 1 line
Quoted 1 line
OK
I've tried to avoid caching, but as soon as I read the status register in the alert handler, the device clears that reg, and it will remain cleared up to the next conversion.
So for example if the reader is blocked on poll over the alert attribute, it gets unblocked on alert, and it will very likely read zero..
The only way I can see for avoiding caching, and for changing the API about clearing events as you asked below, is performing a temperature read from the temperature registers and doing software compare with the limits in show_[max/min]_alert.
Does this sounds OK for you ?
Quoted 32 lines
Quoted 32 lines
Quoted 13 lines
Quoted 13 lines
drivers/hwmon/stts751.c new file +12
-0+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); +bool prev_max = priv->max_alert; +bool prev_min = priv->min_alert; + +if (type !=I2C_PROTOCOL_SMBUS_ALERT )Quoted 8 lines
drivers/hwmon/stts751.c
Quoted 4 lines
drivers/hwmon/stts751.c new file +4
-0+return ; + +dev_dbg (&client->dev,"alert!" ); +Any chance to make this a bit more generic and also support interrupts ?
May you please elaborate?
IMHO adding the logic for handling SMBus alerts using IRQs to any HWmon driver is redundant, because it is already implemented in the SMBus layer, just right now you need patching SMBus layer for enabling it from the DT world (a couple of patches for this are floating around, but they seems not ready for merge yet)... Using this chip with a generic interrupt is indeed exactly what I'm doing..
Quoted 6 lines
Quoted 2 lines
.. Or am I completely missing your point ?
"just right now you need patching SMBus layer for enabling it from the DT world"
So it _doesn't_ work, correct ?
I mean, the generic smbus code works for stts751, and the chip seems happy. So, in this case, no need to reimplement it elsewhere. It would just end up to be a copy-paste from smbus.c.... Just AFAICT we need some mechanism to hook smbus code from DT world..
Quoted 8 lines
The problem with SMBus alert is that each chip implements it differently, to the point where it is difficult to implement it such that it supports more than one chip on the same bus. For the most part, chip interrupt lines are thus not connected to the i2c controller but to, for example, a gpio pin, and each chip on the same bus has a separate interrupt. I am looking forward to see how this is going to be supported in the i2c infrastructure. For my part, I long since gave up on it, but I'll be happy to stand corrected.
Hmm, I didn't know about all those weirdness in devices :(
BTW this sounds like saying that we need to make the smbus code device-bound, rather bus-bound, but even in this case the smbus code can still be generic rather than being buried in each slave driver.. At least for those device like stts751 for with the generic code works..
Consider also that indeed we still need to send the ARAs over the bus, and AFAIK those will hit all devices on the bus.. So this thing seems that needs to be somehow centralized anyway.. I'm honestly unsure if I could support alerts in, say, stts751 driver, handling by myself the IRQ line and sending ARAs over the bus without impacting other devices on the same bus, even if they have their own IRQs..
I mean even if I do implement all this stuff in my driver, I'm not sure it will help much when you have multiple devices with different IRQs on the same bus.. I even suspect this can complicate things.. :/
... Please correct me if I'm wrong; any discussion on this topic would be appreciated :)
BTW it seems that other hwmon drivers, like lm90, are doing the same, relying on generic SMBus support.. For example lm90 doing if (type != I2C_PROTOCOL_SMBUS_ALERT)
return;
Quoted 339 lines
Quoted 339 lines
Quoted 74 lines
Quoted 74 lines
drivers/hwmon/stts751.c new file +10
-0+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; + +if (!(prev_max && prev_min)) { + dev_warn(&priv->client->dev, + "Alert received, but can't communicate tothe device. Something bad happening? Triggering all alarms!");
drivers/hwmon/stts751.c new file +5
-0+ } + } + +if (!prev_max && priv->max_alert) { + dev_notice(&client->dev, "got alert for HIGHtemperature");
drivers/hwmon/stts751.c new file +3
-0+ +/* unblock alert poll */ + sysfs_notify(&priv->dev->kobj, NULL,"
temp1_event_max_alert");drivers/hwmon/stts751.c new file +8
-0+kobject_uevent (&priv->dev->kobj,KOBJ_CHANGE ); + } + +if (!prev_min && 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");drivers/hwmon/stts751.c new file +6
-0+kobject_uevent (&priv->dev->kobj,KOBJ_CHANGE ); + } +mutex_unlock (&priv->access_lock); +} + +static ssize_t show_max_alert(struct device *dev, struct
device_attribute*attr,drivers/hwmon/stts751.c new file +8
-0+ char *buf) +{ +struct stts751_priv *priv =dev_get_drvdata (dev); + +return snprintf (buf,PAGE_SIZE -1 ,"%d\n" , priv->max_alert); +} + +static ssize_t set_max_alert(struct device *dev, struct
device_attribute*attr,drivers/hwmon/stts751.c new file +12
-0+ const char *buf, size_t count) +{ +struct stts751_priv *priv =dev_get_drvdata (dev); + +mutex_lock (&priv->access_lock); + priv->max_alert = false; +mutex_unlock (&priv->access_lock); + +return count; +} + +static ssize_t show_min_alert(struct device *dev, struct
device_attribute*attr,drivers/hwmon/stts751.c new file +8
-0+ char *buf) +{ +struct stts751_priv *priv =dev_get_drvdata (dev); + +return snprintf (buf,PAGE_SIZE -1 ,"%d\n" , priv->min_alert); +} + +static ssize_t set_min_alert(struct device *dev, struct
device_attribute*attr,drivers/hwmon/stts751.c new file +4
-0+ const char *buf, size_t count) +{ +struct stts751_priv *priv =dev_get_drvdata (dev); +Quoted 8 lines
drivers/hwmon/stts751.c
Quoted 4 lines
drivers/hwmon/stts751.c new file +4
-0+mutex_lock (&priv->access_lock); + priv->min_alert = false; +mutex_unlock (&priv->access_lock); +This is highly unusual. If the alert needs to be cleared, please auto-clear it
when reading.OK
Quoted 54 lines
Quoted 54 lines
drivers/hwmon/stts751.c new file +4
-0+return count; +} + +static ssize_t show_input(struct device *dev, struct device_attribute*attr,
drivers/hwmon/stts751.c new file +8
-0+ char *buf) +{ +int ret; +int cache_time =STTS751_CACHE_TIME *HZ /1000 ; +struct stts751_priv *priv =dev_get_drvdata (dev); + +/* +* Adjust the cache time wrt the sample rate. We do 4X in order to get
+ * a new measure in no more than 1/4 of the sample time (that
seemeddrivers/hwmon/stts751.c new file +17
-0+* reasonable to me). +*/ + cache_time = stts751_intervals[priv->interval] /4 *HZ /1000 ; + +if (time_after (jiffies, priv->last_update + cache_time) || + !priv->data_valid) { + ret =stts751_update_temp (priv); +if (ret) +return ret; + priv->last_update = jiffies; + priv->data_valid = true; + } + +return snprintf (buf,PAGE_SIZE -1 ,"%d\n" , priv->temp); +} + +static ssize_t show_therm(struct device *dev, struct device_attribute*attr,
drivers/hwmon/stts751.c new file +9
-0+ 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,
drivers/hwmon/stts751.c new file +10
-0+ 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 = stts751_set_temp_reg(priv, temp, false, STTS751_REG_TLIM,Quoted 8 lines
drivers/hwmon/stts751.c
Quoted 4 lines
0);
drivers/hwmon/stts751.c new file +3
-0+if (ret) +return ret; +Does it really add value to use the same function for 8- and 16-bit registers ?
Seems to me it just makes the code more complicated.I haven't a strong opinion about this.. it's this complication vs a slight code duplication.. I've changed this in the way you suggest :)
Quoted 23 lines
Quoted 23 lines
drivers/hwmon/stts751.c new file +7
-0+dev_dbg (dev,"setting therm %ld" , temp); + + priv->therm = temp; +return count; +} + +static ssize_t show_hyst(struct device *dev, struct device_attribute*attr,
drivers/hwmon/stts751.c new file +9
-0+ 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,
drivers/hwmon/stts751.c new file +5
-0+ const char *buf, size_t count) +{ + int ret; +long temp; +struct stts751_priv *priv =dev_get_drvdata (dev);Quoted 8 lines
drivers/hwmon/stts751.c
Quoted 4 lines
drivers/hwmon/stts751.c new file +4
-0+ +if (kstrtol (buf,10 , &temp) <0 ) +return -EINVAL ; +Per ABI, this is supposed to be an absolute temperature. For the chip, it is a delta. You'll have to do some calculation to convert the absolute value into a delta. Same for the show command.
OK
Quoted 47 lines
Quoted 47 lines
+ ret = stts751_set_temp_reg(priv, temp, false,
STTS751_REG_HYST,
0);drivers/hwmon/stts751.c new file +10
-0+if (ret) +return ret; + +dev_dbg (dev,"setting hyst %ld" , temp); + + priv->hyst = temp; +return count; +} + +static ssize_t show_max(struct device *dev, struct device_attribute*attr,
drivers/hwmon/stts751.c new file +8
-0+ 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,
drivers/hwmon/stts751.c new file +20
-0+ 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 =stts751_set_temp_reg (priv, temp, true, +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,
drivers/hwmon/stts751.c new file +3
-0+ char *buf) +{ +struct stts751_priv *priv =dev_get_drvdata (dev);Quoted 8 lines
drivers/hwmon/stts751.c
Quoted 5 lines
drivers/hwmon/stts751.c new file +5
-0+ +return snprintf (buf,PAGE_SIZE -1 ,"%d\n" , priv->event_min); +} + +Single empty line, please.
OK
Quoted 80 lines
Quoted 67 lines
+static
ssize_tset_min(struct device *dev, structdevice_attribute*attr,drivers/hwmon/stts751.c new file +21
-0+ 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 =stts751_set_temp_reg (priv, temp, true, +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,drivers/hwmon/stts751.c new file +9
-0+ 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,
drivers/hwmon/stts751.c new file +14
-0+ 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,
drivers/hwmon/stts751.c new file +16
-0+ stts751_intervals[idx]); + +if (priv->interval == idx) +return count; + +mutex_lock (&priv->access_lock); + +/* 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);
drivers/hwmon/stts751.c
Quoted 10 lines
drivers/hwmon/stts751.c new file +10
-0+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; +Extra empty line.
Quoted 2 lines
Is it really necessary to have this code twice ? What is the harm in just doing it once, either before or after setting the conversion rate ?
In early development stages I've become suspicious about the chip starting to misbehave if I ever set, even briefly, an invalid configuration.
I tried to avoid that by setting rate and resolution in such an order that never ends up in an invalid configuration. The order is thus not fixed and depends by what changes are you doing..
Quoted 5 lines
Quoted 4 lines
(I'm not completely sure: I've hit something strange and I thought it could be because of this, and once I changed my code this way I never hit that again)..
Please add a respective comment into the code describing the reasons.
OK
Quoted 69 lines
Quoted 69 lines
Quoted 31 lines
Quoted 31 lines
drivers/hwmon/stts751.c new file +22
-0+ } +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);drivers/hwmon/stts751.c new file +7
-0+ +switch (prod_id) { +case STTS751_0_PROD_ID : + name ="STTS751-0" ; +break ; +case STTS751_1_PROD_ID : + name ="STTS751-1" ;Quoted 8 lines
drivers/hwmon/stts751.c
Quoted 6 lines
drivers/hwmon/stts751.c new file +6
-0+break ; +default : +return -ENODEV ; + } +dev_info (&new_client->dev,"Chip %s detected!" , name); +Is this noise necessary ?
OK, changed in
dev_dbg.Quoted 8 lines
drivers/hwmon/stts751.c
Quoted 4 lines
drivers/hwmon/stts751.c new file +3
-0+ rev_id =i2c_smbus_read_byte_data (new_client,STTS751_REG_REV_ID ); + +if (rev_id !=0x1 ) {You might want to check for an error here (and return
-ENODEVif you get one).OK
Quoted 12 lines
drivers/hwmon/stts751.c
Quoted 7 lines
drivers/hwmon/stts751.c new file +2
-0+ dev_notice(&new_client->dev, + "Chip revision 0x%x is untested\nPlease reportwhether it works to [email protected]",
drivers/hwmon/stts751.c new file +4
-0+ rev_id); + } + +strlcpy (info->type, name,I2C_NAME_SIZE );This doesn't match the driver name (or, rather, the values in
stts751_id). Does this instantiate the driver ? That would surprise me.
Quoted 6 lines
Quoted 2 lines
It does, but I'll double check what the heck I've done here :)
Either case this should return a string matching stts751_id[], otherwise auto-detection
and manual or devicetree based instantiation would return different names.
I'm going to stick to the only one string we have in stts751id_[]. Differentiating STTS751-0 from STTS751-1 indeed seems useless for anything but reporting that name in dev_dbg.
Quoted 1 line
Does your use case even include the detect function ?
Oh.. I've checked, and it seems that right now no one is ever calling my detect funcion indeed..
Quoted 57 lines
Quoted 57 lines
Quoted 40 lines
Quoted 40 lines
drivers/hwmon/stts751.c new file +34
-0+return 0 ; +} + +static int stts751_init_chip (struct stts751_priv *priv) +{ +int ret; + + priv->config =STTS751_CONF_EVENT_DIS |STTS751_CONF_STOP ; + ret =i2c_smbus_write_byte_data (priv->client,STTS751_REG_CONF , + priv->config); +if (ret) +return ret; + + ret =i2c_smbus_write_byte_data (priv->client,STTS751_REG_RATE , + priv->interval); +if (ret) +return ret; + +/* invalid, to force update */ + priv->res =-1 ; + ret =stts751_adjust_resolution (priv); +if (ret) +return ret; + + ret =i2c_smbus_write_byte_data (priv->client, +STTS751_REG_SMBUS_TO , + priv->smbus_timeout ?0x80 :0 ); +if (ret) +return ret; + + +if (priv->gen_event) { + ret = stts751_set_temp_reg(priv, priv->event_max, true, + STTS751_REG_HLIM_H,
STTS751_REG_HLIM_L);drivers/hwmon/stts751.c new file +4
-0+if (ret) +return ret; + + ret = stts751_set_temp_reg(priv, priv->event_min, true,Quoted 8 lines
Quoted 2 lines
+
STTS751_REG_LLIM_H,STTS751_REG_LLIM_L);Those registers all have power-up defaults. Unless there is a good reason,
we should not overwrite them. In some applications, the values may be initialized from the BIOS/ROMMON.OK. I've changed it for all those regs but the smsbus timeout stuff:
I think that this depends by the kind of I2C controller the device is attached to, so I'd still getting that from DT. Even if someone already initialized it (assuming it did it correctly, and the DT is correct also) I'll eventually end up in overwriting it with the same value.
Quoted 4 lines
Quoted 2 lines
Do you agree with this ?
Ok to overwrite it if you have devicetree data, but otherwise you should not touch it.
OK
Quoted 48 lines
Quoted 48 lines
Quoted 34 lines
Quoted 34 lines
drivers/hwmon/stts751.c new file +33
-0+if (ret) +return ret; + priv->config &= ~STTS751_CONF_EVENT_DIS ; + } + +if (priv->gen_therm) { + ret =stts751_set_temp_reg (priv, priv->therm, false, +STTS751_REG_TLIM ,0 ); +if (ret) +return ret; + + ret =stts751_set_temp_reg (priv, priv->hyst, false, +STTS751_REG_HYST ,0 ); +if (ret) +return ret; + } + + priv->config &= ~STTS751_CONF_STOP ; + ret =i2c_smbus_write_byte_data (priv->client, +STTS751_REG_CONF , priv->config); + +return ret; +} + +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO, + show_min, set_min, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO, + show_max, set_max, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO, + show_min_alert, set_min_alert, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO, + show_max_alert, set_max_alert, 0);Quoted 8 lines
Quoted 5 lines
drivers/hwmon/stts751.c new file +3
-0+static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm, + set_therm, 0); +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO,
show_hyst,
+set_hyst, 0);Please use standard attribute names.
I can convert the event stuff in temp1_[min/max]_alarm for limits and
temp1_alarmfor the flag, but it's not obvious to be what should I use for the therm stuff.. May you please give me advices about this?
Quoted 7 lines
Quoted 3 lines
I'd propose temp1_therm_[max/min]_hyst, but they seems not standard anyway...
From the chip datasheet, this is supposed to reflect overtemperature conditions,
stronger than high/low, thus 'crit' seems appropriate.
I see.. I can go with 'crit' :)
Just is there any place where I can document this? Despite the datasheet, I can easily imagine real-world usage scenarios, for example driving a fan from that signal, where 'crit' might be not so intuitive.. Indeed I would like to make it visible the 'crit' <->'therm' binding to users. Maybe an additional attribute 'temp1_crit_label' or something like that ?
Quoted 105 lines
Quoted 105 lines
Quoted 70 lines
Quoted 70 lines
drivers/hwmon/stts751.c new file +68
-0+static SENSOR_DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO, + show_interval, set_interval, 0); + +/* always present */ +static struct attribute *stts751_temp_attrs[] = { + &sensor_dev_attr_temp1_input.dev_attr.attr, +NULL +}; + +static struct attribute_group stts751_temp_group = { + .attrs = stts751_temp_attrs, +}; + +/* present when therm pin or event pin are connected */ +static struct attribute *stts751_interval_attrs[] = { + &sensor_dev_attr_update_interval.dev_attr.attr, +NULL +}; + +static struct attribute_group stts751_interval_group = { + .attrs = stts751_interval_attrs, +}; + +/* present when event pin is connected */ +static struct attribute *stts751_event_attrs[] = { + &sensor_dev_attr_temp1_event_min.dev_attr.attr, + &sensor_dev_attr_temp1_event_max.dev_attr.attr, + &sensor_dev_attr_temp1_event_min_alert.dev_attr.attr, + &sensor_dev_attr_temp1_event_max_alert.dev_attr.attr, +NULL +}; + +static struct attribute_group stts751_event_group = { + .attrs = stts751_event_attrs, +}; + +/* present when therm pin is connected */ +static struct attribute *stts751_therm_attrs[] = { + &sensor_dev_attr_temp1_therm.dev_attr.attr, + &sensor_dev_attr_temp1_therm_hyst.dev_attr.attr, +NULL +}; + +static struct attribute_group stts751_therm_group = { + .attrs = stts751_therm_attrs, +}; + +static int stts751_probe (struct i2c_client *client, +const struct i2c_device_id *id) +{ +struct stts751_priv *priv; +int ret; +int groups_idx =0 ; +struct device_node *np = client->dev.of_node; + + 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); + +/* default to 2 samples per second */ + priv->interval =5 ; +/* default to timeout enable, as per chip default */ + priv->smbus_timeout = true; + priv->last_update =0 ;Quoted 8 lines
drivers/hwmon/stts751.c
Quoted 5 lines
drivers/hwmon/stts751.c new file +5
-0+ priv->data_valid = false; + priv->max_alert = false; + priv->min_alert = false; + priv->gen_therm = false; + priv->gen_event = false;Unnecessary initializations.
OK
Quoted 10 lines
drivers/hwmon/stts751.c
Quoted 6 lines
drivers/hwmon/stts751.c new file +5
-0+ priv->therm =STTS751_THERM_DEFAULT ; + priv->hyst =STTS751_HYST_DEFAULT ; + priv->event_max =STTS751_EVENT_MAX_DEFAULT ; + priv->event_min =STTS751_EVENT_MIN_DEFAULT ; +I think it would be better to read the initial values from the chip (also see above).
OK
Quoted 10 lines
Quoted 8 lines
drivers/hwmon/stts751.c new file +7
-0+if (np) { + priv->gen_therm =of_property_read_bool (np,"has-therm" ); + priv->gen_event =of_property_read_bool (np,"has-event" ); + priv->smbus_timeout = !of_property_read_bool (np, +"smbus-timeout-disable" ); + }else { + dev_notice(&client->dev, "No DT data. Event/thermdisabled\n");
Quoted 8 lines
Quoted 7 lines
Quoted 1 line
Seems to be quite rude to penaltize non-DT systems this way.
That's gone since the two props "has-therm" and "has-event" are now gone. Any suggestion please about how to get the "smbus-timeout-disable" stuff in non-DT case ?
Platform data, or better use device properties for both cases.
OK, I've switched to device properties.
Quoted 97 lines
Quoted 96 lines
Quoted 12 lines
drivers/hwmon/stts751.c
Quoted 9 lines
drivers/hwmon/stts751.c new file +8
-0+ } + +dev_dbg (&client->dev,"gen_event: %s, gen_therm: %s" , + priv->gen_event ?"YES" :"NO" , + priv->gen_therm ?"YES" :"NO" ); + + priv->groups[groups_idx++] = &stts751_temp_group; + priv->groups[groups_idx++] = &stts751_interval_group;Any reason for having multiple groups here ?
Not anymore, as per your suggesion below, they are gone :)
Quoted 8 lines
drivers/hwmon/stts751.c
Quoted 7 lines
drivers/hwmon/stts751.c new file +7
-0+ +if (priv->gen_therm) + priv->groups[groups_idx++] = &stts751_therm_group; + +if (priv->gen_event) + priv->groups[groups_idx++] = &stts751_event_group; +Quoted 8 lines
Why are those groups optional ? This is quite unusual. Even if the chip is not
connected to alert or interrupt lines, it still supports limit registers and
reports alerts. On top of that, at least interrupt support can be detected
automatically (fromclient->irq) and would not need a devicetree property.Props "has-therm" and "has-event" gone; please see above about IRQ stuf..
Quoted 3 lines
I think you really only need a single group, and all attributes should always
be available.OK
Quoted 5 lines
Quoted 2 lines
+ priv->groups[groups_idx] = NULL;
Unnecessary initialization.
devm_kzalloc()initializes priv with 0.OK
Quoted 42 lines
Quoted 42 lines
drivers/hwmon/stts751.c new file +7
-0+ + ret =stts751_init_chip (priv); +if (ret) +return ret; + + priv->dev = devm_hwmon_device_register_with_groups(&client->dev, + client->name,priv,
drivers/hwmon/stts751.c new file +27
-1+ priv->groups); +return PTR_ERR_OR_ZERO (priv->dev); +} + +static const struct i2c_device_id stts751_id[] = { + {"stts751" ,0 }, + { } +}; +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 <[email protected]>" ); +MODULE_DESCRIPTION ("STTS751 sensor driver" ); +MODULE_LICENSE ("GPL" ); --2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Guenter Roeck
maintainerQuoted 27 lines · Andrea Merello
Quoted 17 lines
Quoted 17 lines
Quoted 15 lines
Quoted 9 lines
(quoted diff) +4
-0+ if (ret < 0) + return ret; + + priv->max_alert = priv->max_alert || !!(ret &
STTS751_STATUS_TRIPH);
+priv->min_alert=priv->min_alert|| !!(ret &STTS751_STATUS_TRIPL);
+I am not really happy about the caching of alert values. This isn't really
common. Usually we report current alert values when reading. I don't really
see why this driver should be different to all the others.Quoted 1 line
Quoted 1 line
OK
I've tried to avoid caching, but as soon as I read the status register in the alert handler, the device clears that reg, and it will remain cleared up to the next conversion.
So for example if the reader is blocked on poll over the alert attribute, it gets unblocked on alert, and it will very likely read zero..
Quoted 7 lines
The only way I can see for avoiding caching, and for changing the API about clearing events as you asked below, is performing a temperature read from the temperature registers and doing software compare with the limits in show_[max/min]_alert.
Does this sounds OK for you ?
Not really. That could be done from user space by just reading the temperature and comparing it against limits.
If the chip clears the alert bit on read, why would you need a set attribute to clear it ? From what you are saying, the next alert should update all alert bits, meaning they would be auto-updated at that time. User space would then always read the current state. Sure, you would cache the value in the alert function to be able to read it in the read function, but I don't see why all the complexity around it in your code would be needed.
You might want to update the alert status in case there is no interrupt support, though (maybe you do already; I didn't re-check the code).
The 'sensors' code doesn't know that it is supposed to write into the alert attribute to clear the status, thus unless you have a special application, an alert would never be cleared. That doesn't sound very appealing to me.
Quoted 37 lines
Quoted 37 lines
Quoted 25 lines
Quoted 13 lines
Quoted 13 lines
(quoted diff) +12
-0+ 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); + bool prev_max = priv->max_alert; + bool prev_min = priv->min_alert; + + if (type != I2C_PROTOCOL_SMBUS_ALERT)Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ return; + + dev_dbg(&client->dev, "alert!"); +Any chance to make this a bit more generic and also support interrupts ?
May you please elaborate?
Quoted 9 lines
IMHO adding the logic for handling SMBus alerts using IRQs to any HWmon driver is redundant, because it is already implemented in the SMBus layer, just right now you need patching SMBus layer for enabling it from the DT world (a couple of patches for this are floating around, but they seems not ready for merge yet)... Using this chip with a generic interrupt is indeed exactly what I'm doing..
.. Or am I completely missing your point ?
"just right now you need patching SMBus layer for enabling it from the DT world"
Quoted 7 lines
Quoted 1 line
So it _doesn't_ work, correct ?
I mean, the generic smbus code works for stts751, and the chip seems happy. So, in this case, no need to reimplement it elsewhere. It would just end up to be a copy-paste from
smbus.c.... Just AFAICT we need some mechanism to hook smbus code from DT world..
Ok, then let's just go with it. If it doesn't work in some application we'll have to fix it up later.
Quoted 32 lines
Quoted 24 lines
Quoted 24 lines
Quoted 10 lines
Quoted 10 lines
(quoted diff) +10
-0+ +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO, + show_min, set_min, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO, + show_max, set_max, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO, + show_min_alert, set_min_alert, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO, + show_max_alert, set_max_alert, 0);Quoted 8 lines
Quoted 5 lines
(quoted diff) +3
-0+static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm, + set_therm, 0); +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO,
show_hyst,
+set_hyst, 0);Please use standard attribute names.
I can convert the event stuff in temp1_[min/max]_alarm for limits and
temp1_alarmfor the flag, but it's not obvious to be what should I use for the therm stuff.. May you please give me advices about this?Quoted 7 lines
Quoted 3 lines
I'd propose temp1_therm_[max/min]_hyst, but they seems not standard anyway...
From the chip datasheet, this is supposed to reflect overtemperature conditions,
stronger than high/low, thus 'crit' seems appropriate.
Quoted 9 lines
I see.. I can go with 'crit' :)
Just is there any place where I can document this? Despite the datasheet, I can easily imagine real-world usage scenarios, for example driving a fan from that signal, where 'crit' might be not so intuitive.. Indeed I would like to make it visible the 'crit' <->'therm' binding to users. Maybe an additional attribute '
temp1_crit_label' or something like that ?
You are supposed to provide Documentation/hwmon/stts751, which would include such information.
A label applies to a sensor, not to a sensor attribute, so that would not be appropriate.
Thanks,
Guenter
Quoted 37 lines · Guenter Roeck
Quoted 35 lines · Andrea Merello
Quoted 19 lines
Quoted 19 lines
Quoted 16 lines
Quoted 9 lines
(quoted diff) +4
-0+ if (ret < 0) + return ret; + + priv->max_alert = priv->max_alert || !!(ret &
STTS751_STATUS_TRIPH);
+priv->min_alert=priv->min_alert|| !!(ret &STTS751_STATUS_TRIPL);
+I am not really happy about the caching of alert values. This isn't really
common. Usually we report current alert values when reading. I don't really
see why this driver should be different to all the others.Quoted 1 line
Quoted 1 line
OK
I've tried to avoid caching, but as soon as I read the status register in the alert handler, the device clears that reg, and it will remain cleared up to the next conversion.
So for example if the reader is blocked on poll over the alert attribute, it gets unblocked on alert, and it will very likely read zero..
The only way I can see for avoiding caching, and for changing the API about clearing events as you asked below, is performing a temperature read from the temperature registers and doing software compare with the limits in show_[max/min]_alert.
Quoted 6 lines
Quoted 2 lines
Does this sounds OK for you ?
Not really. That could be done from user space by just reading the temperature
and comparing it against limits.
Sorry, I don't see the point here.. Why the fact that something could be done in userspace should suggest we don't want to do it in the driver? We do, for example, a simple calculation to give an absolute value for the hysteresis; we could do this also in userspace, but we do this in the driver in order to support the proper API..
Quoted 2 lines
If the chip clears the alert bit on read, why would you need a set attribute to clear it ?
That was used to clear the cached value, not the chip reg.
Quoted 7 lines
From what you are saying, the next alert should update all alert bits, meaning they would be auto-updated at that time. User space would then always read the current state.
Sure, you would cache the value
in the alert function to be able to read it in the read function, but I don't see why all the complexity around it in your code would be needed.
The next alert is, by definition, an alert, so it will always update the cached value with a '1'. We don't have anything that tell us when the alert is gone. No one would clear the cached value with '0' when the alert is gone. (ok, we have two alert bits, so the _other_ one of them will be set to zero, but that's not the point)..
So, in order to report the current alarm value, without relying to the user to clear it, as you asked for, caching seems not an option..
Quoted 2 lines
You might want to update the alert status in case there is no interrupt support, though (maybe you do already; I didn't re-check the code).
OK, good point, I'll make sure it works event in this case.
Quoted 4 lines
The 'sensors' code doesn't know that it is supposed to write into the alert attribute to clear the status, thus unless you have a special application, an alert would never be cleared. That doesn't sound very appealing to me.
So, OK to change the API as you said, however I don't see how this can be done except for doing the manual temperature/limit comparison in the driver:
When you get into show_[max/min]_alert function you can't rely on reading the status register (because as I said in my prev mail it could have been just cleared by reading it in the event handler), but as I explained above it seems to me that you cannot also rely on a value cached in the event handler with this API, because no one will ever clear it..
In other words you will see a status register that could be zero even when we have the alarm, or a cached value that is still 1 even if the alarm has gone time ago.. I could clear the cached value in the show_[max/min]_alert function itself, but this doesn't seems enough to me for reporting to the userspace the _current_ alarm value, as you asked for.
Or am I missing something ?
Quoted 97 lines
Quoted 40 lines
Quoted 40 lines
Quoted 28 lines
Quoted 15 lines
Quoted 15 lines
(quoted diff) +13
-0+ 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); + bool prev_max = priv->max_alert; + bool prev_min = priv->min_alert; + + if (type != I2C_PROTOCOL_SMBUS_ALERT) + return;Quoted 8 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ + dev_dbg(&client->dev, "alert!"); +Any chance to make this a bit more generic and also support interrupts ?
May you please elaborate?
Quoted 9 lines
IMHO adding the logic for handling SMBus alerts using IRQs to any HWmon driver is redundant, because it is already implemented in the SMBus layer, just right now you need patching SMBus layer for enabling it from the DT world (a couple of patches for this are floating around, but they seems not ready for merge yet)... Using this chip with a generic interrupt is indeed exactly what I'm doing..
.. Or am I completely missing your point ?
"just right now you need patching SMBus layer for enabling it from the DT world"
Quoted 8 lines
Quoted 1 line
So it _doesn't_ work, correct ?
I mean, the generic smbus code works for stts751, and the chip seems happy. So, in this case, no need to reimplement it elsewhere. It would just end up to be a copy-paste from
smbus.c.... Just AFAICT we need some mechanism to hook smbus code from DT world..Ok, then let's just go with it. If it doesn't work in some application we'll have to fix it up later.
Quoted 35 lines
Quoted 26 lines
Quoted 26 lines
Quoted 11 lines
Quoted 11 lines
(quoted diff) +11
-0+ +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO, + show_min, set_min, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO, + show_max, set_max, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO, + show_min_alert, set_min_alert, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO, + show_max_alert, set_max_alert, 0); +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,Quoted 8 lines
Quoted 4 lines
(quoted diff) +2
-0+ set_therm, 0); +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO,
show_hyst,
+set_hyst, 0);Please use standard attribute names.
I can convert the event stuff in temp1_[min/max]_alarm for limits and
temp1_alarmfor the flag, but it's not obvious to be what should I use for the therm stuff.. May you please give me advices about this?Quoted 7 lines
Quoted 3 lines
I'd propose temp1_therm_[max/min]_hyst, but they seems not standard anyway...
From the chip datasheet, this is supposed to reflect overtemperature conditions,
stronger than high/low, thus 'crit' seems appropriate.Quoted 9 lines
I see.. I can go with 'crit' :)
Just is there any place where I can document this? Despite the datasheet, I can easily imagine real-world usage scenarios, for example driving a fan from that signal, where 'crit' might be not so intuitive.. Indeed I would like to make it visible the 'crit' <->'therm' binding to users. Maybe an additional attribute '
temp1_crit_label' or something like that ?
Quoted 5 lines
You are supposed to provide
Documentation/hwmon/stts751, which would include such information.A label applies to a sensor, not to a sensor attribute, so that would not be appropriate.
OK to provide it, but it would be in kernel source tree. Any further hint that we could give to someone that looks at in /sys ?
Quoted 3 lines
Thanks,
Guenter
Guenter Roeck
maintainerQuoted 45 lines · Andrea Merello
Quoted 37 lines · Guenter Roeck
Quoted 35 lines · Andrea Merello
Quoted 19 lines
Quoted 19 lines
Quoted 16 lines
Quoted 9 lines
(quoted diff) +4
-0+ if (ret < 0) + return ret; + + priv->max_alert = priv->max_alert || !!(ret &
STTS751_STATUS_TRIPH);
+priv->min_alert=priv->min_alert|| !!(ret &STTS751_STATUS_TRIPL);
+I am not really happy about the caching of alert values. This isn't really
common. Usually we report current alert values when reading. I don't really
see why this driver should be different to all the others.Quoted 1 line
Quoted 1 line
OK
I've tried to avoid caching, but as soon as I read the status register in the alert handler, the device clears that reg, and it will remain cleared up to the next conversion.
So for example if the reader is blocked on poll over the alert attribute, it gets unblocked on alert, and it will very likely read zero..
The only way I can see for avoiding caching, and for changing the API about clearing events as you asked below, is performing a temperature read from the temperature registers and doing software compare with the limits in show_[max/min]_alert.
Quoted 6 lines
Quoted 2 lines
Does this sounds OK for you ?
Not really. That could be done from user space by just reading the temperature
and comparing it against limits.
Quoted 6 lines
Sorry, I don't see the point here.. Why the fact that something could be done in userspace should suggest we don't want to do it in the driver? We do, for example, a simple calculation to give an absolute value for the hysteresis; we could do this also in userspace, but we do this in the driver in order to support the proper API..
Datasheet says:
T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A > high limit). T HIGH is cleared when the status register is read, provided the condition no longer exists.
T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low limit). T LOW is cleared when the status register is read, provided the condition no longer exists.
This isn't exactly what you are claiming, since it says "cleared ... provided that the condition no longer exists", which would be more in line with other chips.
The THRM bit doesn't seem to self-clean either, at least not according to the datasheet.
This means I'll need to get a sample and/or eval board to test this myself.
Guenter
Quoted 162 lines
Quoted 2 lines
If the chip clears the alert bit on read, why would you need a set attribute to clear it ?
That was used to clear the cached value, not the chip reg.
Quoted 7 lines
From what you are saying, the next alert should update all alert bits, meaning they would be auto-updated at that time. User space would then always read the current state.
Sure, you would cache the value
in the alert function to be able to read it in the read function, but I don't see why all the complexity around it in your code would be needed.The next alert is, by definition, an alert, so it will always update the cached value with a '1'. We don't have anything that tell us when the alert is gone. No one would clear the cached value with '0' when the alert is gone. (ok, we have two alert bits, so the _other_ one of them will be set to zero, but that's not the point)..
So, in order to report the current alarm value, without relying to the user to clear it, as you asked for, caching seems not an option..
Quoted 2 lines
You might want to update the alert status in case there is no interrupt support, though (maybe you do already; I didn't re-check the code).
OK, good point, I'll make sure it works event in this case.
Quoted 4 lines
The 'sensors' code doesn't know that it is supposed to write into the alert attribute to clear the status, thus unless you have a special application, an alert would never be cleared. That doesn't sound very appealing to me.
So, OK to change the API as you said, however I don't see how this can be done except for doing the manual temperature/limit comparison in the driver:
When you get into show_[max/min]_alert function you can't rely on reading the status register (because as I said in my prev mail it could have been just cleared by reading it in the event handler), but as I explained above it seems to me that you cannot also rely on a value cached in the event handler with this API, because no one will ever clear it..
In other words you will see a status register that could be zero even when we have the alarm, or a cached value that is still 1 even if the alarm has gone time ago.. I could clear the cached value in the show_[max/min]_alert function itself, but this doesn't seems enough to me for reporting to the userspace the _current_ alarm value, as you asked for.
Or am I missing something ?
Quoted 97 lines
Quoted 40 lines
Quoted 40 lines
Quoted 28 lines
Quoted 15 lines
Quoted 15 lines
(quoted diff) +13
-0+ 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); + bool prev_max = priv->max_alert; + bool prev_min = priv->min_alert; + + if (type != I2C_PROTOCOL_SMBUS_ALERT) + return;Quoted 8 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ + dev_dbg(&client->dev, "alert!"); +Any chance to make this a bit more generic and also support interrupts ?
May you please elaborate?
Quoted 9 lines
IMHO adding the logic for handling SMBus alerts using IRQs to any HWmon driver is redundant, because it is already implemented in the SMBus layer, just right now you need patching SMBus layer for enabling it from the DT world (a couple of patches for this are floating around, but they seems not ready for merge yet)... Using this chip with a generic interrupt is indeed exactly what I'm doing..
.. Or am I completely missing your point ?
"just right now you need patching SMBus layer for enabling it from the DT world"
Quoted 8 lines
Quoted 1 line
So it _doesn't_ work, correct ?
I mean, the generic smbus code works for stts751, and the chip seems happy. So, in this case, no need to reimplement it elsewhere. It would just end up to be a copy-paste from
smbus.c.... Just AFAICT we need some mechanism to hook smbus code from DT world..Ok, then let's just go with it. If it doesn't work in some application we'll have to fix it up later.
Quoted 35 lines
Quoted 26 lines
Quoted 26 lines
Quoted 11 lines
Quoted 11 lines
(quoted diff) +11
-0+ +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO, + show_min, set_min, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO, + show_max, set_max, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO, + show_min_alert, set_min_alert, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO, + show_max_alert, set_max_alert, 0); +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,Quoted 8 lines
Quoted 4 lines
(quoted diff) +2
-0+ set_therm, 0); +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO,
show_hyst,
+set_hyst, 0);Please use standard attribute names.
I can convert the event stuff in temp1_[min/max]_alarm for limits and
temp1_alarmfor the flag, but it's not obvious to be what should I use for the therm stuff.. May you please give me advices about this?Quoted 7 lines
Quoted 3 lines
I'd propose temp1_therm_[max/min]_hyst, but they seems not standard anyway...
From the chip datasheet, this is supposed to reflect overtemperature conditions,
stronger than high/low, thus 'crit' seems appropriate.Quoted 9 lines
I see.. I can go with 'crit' :)
Just is there any place where I can document this? Despite the datasheet, I can easily imagine real-world usage scenarios, for example driving a fan from that signal, where 'crit' might be not so intuitive.. Indeed I would like to make it visible the 'crit' <->'therm' binding to users. Maybe an additional attribute '
temp1_crit_label' or something like that ?Quoted 5 lines
You are supposed to provide
Documentation/hwmon/stts751, which would include such information.A label applies to a sensor, not to a sensor attribute, so that would not be appropriate.
OK to provide it, but it would be in kernel source tree. Any further hint that we could give to someone that looks at in /sys ?
Quoted 3 lines
Thanks,
Guenter
Quoted 61 lines · Guenter Roeck
Quoted 45 lines · Andrea Merello
Quoted 37 lines · Guenter Roeck
Quoted 35 lines · Andrea Merello
Quoted 19 lines
Quoted 19 lines
Quoted 16 lines
Quoted 9 lines
(quoted diff) +4
-0+ if (ret < 0) + return ret; + + priv->max_alert = priv->max_alert || !!(ret &
STTS751_STATUS_TRIPH);
+priv->min_alert=priv->min_alert|| !!(ret &STTS751_STATUS_TRIPL);
+I am not really happy about the caching of alert values. This isn't really
common. Usually we report current alert values when reading. I don't really
see why this driver should be different to all the others.Quoted 1 line
Quoted 1 line
OK
I've tried to avoid caching, but as soon as I read the status register in the alert handler, the device clears that reg, and it will remain cleared up to the next conversion.
So for example if the reader is blocked on poll over the alert attribute, it gets unblocked on alert, and it will very likely read zero..
The only way I can see for avoiding caching, and for changing the API about clearing events as you asked below, is performing a temperature read from the temperature registers and doing software compare with the limits in show_[max/min]_alert.
Quoted 6 lines
Quoted 2 lines
Does this sounds OK for you ?
Not really. That could be done from user space by just reading the temperature
and comparing it against limits.Quoted 6 lines
Sorry, I don't see the point here.. Why the fact that something could be done in userspace should suggest we don't want to do it in the driver? We do, for example, a simple calculation to give an absolute value for the hysteresis; we could do this also in userspace, but we do this in the driver in order to support the proper API..
Datasheet says:
T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A > high limit). T HIGH is cleared when the status register is read, provided the condition no longer exists.
T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low limit). T LOW is cleared when the status register is read, provided the condition no longer exists.
Quoted 3 lines
This isn't exactly what you are claiming, since it says "cleared ... provided that the condition no longer exists", which would be more in line with other chips.
Yes, I know.. I've also read that on the datasheet.. And I tried reading the status register in both the alert handler and the show function, but it didn't worked :/
So I started to suspect the status register got actually clear after reading up to the next conversion, and to test this, I simply read it twice in the alert handler. The first read always read 1, as expected; the second time I always got zero, even if the temperature was still over-limit.. And I soon got another event, that behaved in the same manner.
(also I'm thinking about a way to mask events once we know that we are in a out-of-limit condition, to avoid having a lot of them, but simply disabling events and re-enabling them after userspace reads the alarm and find it to be zero, like lm90 driver does, IMHO wouldn't be good here, because we have a single bit to disable both high and low limits..)
Quoted 2 lines
The THRM bit doesn't seem to self-clean either, at least not according to the datasheet.
I need to test this.. But this doesn't generate any event, it seems only inteded to drive an external circuit, say a fan, and not to notify SW.. And currently I even have no show function to read it's status (now that I'm on it, I think I should add it...)..
If I add it, and this func reads the status reg, then it will end up in racy clears of the other high/low event bits anyway, since it's all in the same reg..
Indeed it seems to me that we can rely on chip automatism only for alarm event generation. And I don't see all that advantage in relying on HW comparators for reporting alarms in the show functions; IMHO there we can do our comparisons in SW without any real issue..
Quoted 2 lines
This means I'll need to get a sample and/or eval board to test this myself.
If you want to write your own tests, I will be happy to make them run on my board and give you the results back. Just pass me the test code :)
Quoted 164 lines
Guenter
Quoted 162 lines
Quoted 2 lines
If the chip clears the alert bit on read, why would you need a set attribute to clear it ?
That was used to clear the cached value, not the chip reg.
Quoted 7 lines
From what you are saying, the next alert should update all alert bits, meaning they would be auto-updated at that time. User space would then always read the current state.
Sure, you would cache the value
in the alert function to be able to read it in the read function, but I don't see why all the complexity around it in your code would be needed.The next alert is, by definition, an alert, so it will always update the cached value with a '1'. We don't have anything that tell us when the alert is gone. No one would clear the cached value with '0' when the alert is gone. (ok, we have two alert bits, so the _other_ one of them will be set to zero, but that's not the point)..
So, in order to report the current alarm value, without relying to the user to clear it, as you asked for, caching seems not an option..
Quoted 2 lines
You might want to update the alert status in case there is no interrupt support, though (maybe you do already; I didn't re-check the code).
OK, good point, I'll make sure it works event in this case.
Quoted 4 lines
The 'sensors' code doesn't know that it is supposed to write into the alert attribute to clear the status, thus unless you have a special application, an alert would never be cleared. That doesn't sound very appealing to me.
So, OK to change the API as you said, however I don't see how this can be done except for doing the manual temperature/limit comparison in the driver:
When you get into show_[max/min]_alert function you can't rely on reading the status register (because as I said in my prev mail it could have been just cleared by reading it in the event handler), but as I explained above it seems to me that you cannot also rely on a value cached in the event handler with this API, because no one will ever clear it..
In other words you will see a status register that could be zero even when we have the alarm, or a cached value that is still 1 even if the alarm has gone time ago.. I could clear the cached value in the show_[max/min]_alert function itself, but this doesn't seems enough to me for reporting to the userspace the _current_ alarm value, as you asked for.
Or am I missing something ?
Quoted 97 lines
Quoted 40 lines
Quoted 40 lines
Quoted 28 lines
Quoted 15 lines
Quoted 15 lines
(quoted diff) +13
-0+ 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); + bool prev_max = priv->max_alert; + bool prev_min = priv->min_alert; + + if (type != I2C_PROTOCOL_SMBUS_ALERT) + return;Quoted 8 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ + dev_dbg(&client->dev, "alert!"); +Any chance to make this a bit more generic and also support interrupts ?
May you please elaborate?
Quoted 9 lines
IMHO adding the logic for handling SMBus alerts using IRQs to any HWmon driver is redundant, because it is already implemented in the SMBus layer, just right now you need patching SMBus layer for enabling it from the DT world (a couple of patches for this are floating around, but they seems not ready for merge yet)... Using this chip with a generic interrupt is indeed exactly what I'm doing..
.. Or am I completely missing your point ?
"just right now you need patching SMBus layer for enabling it from the DT world"
Quoted 8 lines
Quoted 1 line
So it _doesn't_ work, correct ?
I mean, the generic smbus code works for stts751, and the chip seems happy. So, in this case, no need to reimplement it elsewhere. It would just end up to be a copy-paste from
smbus.c.... Just AFAICT we need some mechanism to hook smbus code from DT world..Ok, then let's just go with it. If it doesn't work in some application we'll have to fix it up later.
Quoted 35 lines
Quoted 26 lines
Quoted 26 lines
Quoted 11 lines
Quoted 11 lines
(quoted diff) +11
-0+ +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO, + show_min, set_min, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO, + show_max, set_max, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO, + show_min_alert, set_min_alert, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO, + show_max_alert, set_max_alert, 0); +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,Quoted 8 lines
Quoted 4 lines
(quoted diff) +2
-0+ set_therm, 0); +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO,
show_hyst,
+set_hyst, 0);Please use standard attribute names.
I can convert the event stuff in temp1_[min/max]_alarm for limits and
temp1_alarmfor the flag, but it's not obvious to be what should I use for the therm stuff.. May you please give me advices about this?Quoted 7 lines
Quoted 3 lines
I'd propose temp1_therm_[max/min]_hyst, but they seems not standard anyway...
From the chip datasheet, this is supposed to reflect overtemperature conditions,
stronger than high/low, thus 'crit' seems appropriate.Quoted 9 lines
I see.. I can go with 'crit' :)
Just is there any place where I can document this? Despite the datasheet, I can easily imagine real-world usage scenarios, for example driving a fan from that signal, where 'crit' might be not so intuitive.. Indeed I would like to make it visible the 'crit' <->'therm' binding to users. Maybe an additional attribute '
temp1_crit_label' or something like that ?Quoted 5 lines
You are supposed to provide
Documentation/hwmon/stts751, which would include such information.A label applies to a sensor, not to a sensor attribute, so that would not be appropriate.
OK to provide it, but it would be in kernel source tree. Any further hint that we could give to someone that looks at in /sys ?
Quoted 3 lines
Thanks,
Guenter
Guenter Roeck
maintainerQuoted 70 lines · Andrea Merello
Quoted 61 lines · Guenter Roeck
Quoted 45 lines · Andrea Merello
Quoted 37 lines · Guenter Roeck
Quoted 35 lines · Andrea Merello
Quoted 19 lines
Quoted 19 lines
Quoted 16 lines
Quoted 9 lines
(quoted diff) +4
-0+ if (ret < 0) + return ret; + + priv->max_alert = priv->max_alert || !!(ret &
STTS751_STATUS_TRIPH);
+priv->min_alert=priv->min_alert|| !!(ret &STTS751_STATUS_TRIPL);
+I am not really happy about the caching of alert values. This isn't really
common. Usually we report current alert values when reading. I don't really
see why this driver should be different to all the others.Quoted 1 line
Quoted 1 line
OK
I've tried to avoid caching, but as soon as I read the status register in the alert handler, the device clears that reg, and it will remain cleared up to the next conversion.
So for example if the reader is blocked on poll over the alert attribute, it gets unblocked on alert, and it will very likely read zero..
The only way I can see for avoiding caching, and for changing the API about clearing events as you asked below, is performing a temperature read from the temperature registers and doing software compare with the limits in show_[max/min]_alert.
Quoted 6 lines
Quoted 2 lines
Does this sounds OK for you ?
Not really. That could be done from user space by just reading the temperature
and comparing it against limits.Quoted 6 lines
Sorry, I don't see the point here.. Why the fact that something could be done in userspace should suggest we don't want to do it in the driver? We do, for example, a simple calculation to give an absolute value for the hysteresis; we could do this also in userspace, but we do this in the driver in order to support the proper API..
Datasheet says:
T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A > high limit). T HIGH is cleared when the status register is read, provided the condition no longer exists.
T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low limit). T LOW is cleared when the status register is read, provided the condition no longer exists.Quoted 3 lines
This isn't exactly what you are claiming, since it says "cleared ... provided that the condition no longer exists", which would be more in line with other chips.
Yes, I know.. I've also read that on the datasheet.. And I tried reading the status register in both the alert handler and the show function, but it didn't worked :/
Quoted 7 lines
So I started to suspect the status register got actually clear after reading up to the next conversion, and to test this, I simply read it twice in the alert handler. The first read always read 1, as expected; the second time I always got zero, even if the temperature was still over-limit.. And I soon got another event, that behaved in the same manner.
Question is if the event bit is set again after the next conversion; this would be relatively easy to handle (cache the status register for one conversion time).
Quoted 7 lines
(also I'm thinking about a way to mask events once we know that we are in a out-of-limit condition, to avoid having a lot of them, but simply disabling events and re-enabling them after userspace reads the alarm and find it to be zero, like lm90 driver does, IMHO wouldn't be good here, because we have a single bit to disable both high and low limits..)
Quoted 8 lines
Quoted 2 lines
The THRM bit doesn't seem to self-clean either, at least not according to the datasheet.
I need to test this.. But this doesn't generate any event, it seems only inteded to drive an external circuit, say a fan, and not to notify SW.. And currently I even have no show function to read it's status (now that I'm on it, I think I should add it...)..
Yes. Note that this pin might be wired to an interrupt.
Quoted 10 lines
If I add it, and this func reads the status reg, then it will end up in racy clears of the other high/low event bits anyway, since it's all in the same reg..
Indeed it seems to me that we can rely on chip automatism only for alarm event generation. And I don't see all that advantage in relying on HW comparators for reporting alarms in the show functions; IMHO there we can do our comparisons in SW without any real issue..
Quoted 1 line
Quoted 3 lines
Quoted 1 line
This means I'll need to get a sample and/or eval board to test this myself.
If you want to write your own tests, I will be happy to make them run
That isn't the point; I'll want to see how the chip really behaves. Reading through the datasheet, the most likely behavior is that the status bits are cleared on read and set again with the next measurement cycle, as already mentioned above. Per datasheet, the alert is supposed to be raised again after each measurement cycle if the condition still exists. This behavior is actually quite common with other chips. If so, all we would need to do is to cache the status register value for one measurement cycle.
Regarding testing, I'll ultimately write a module test for the driver; see
https://github.com/groeck/module-tests for details. For that, all I'll need is a register dump.
Thanks,
Guenter
Quoted 171 lines
on my board and give you the results back. Just pass me the test code :)
Quoted 164 lines
Guenter
Quoted 162 lines
Quoted 2 lines
If the chip clears the alert bit on read, why would you need a set attribute to clear it ?
That was used to clear the cached value, not the chip reg.
Quoted 7 lines
From what you are saying, the next alert should update all alert bits, meaning they would be auto-updated at that time. User space would then always read the current state.
Sure, you would cache the value
in the alert function to be able to read it in the read function, but I don't see why all the complexity around it in your code would be needed.The next alert is, by definition, an alert, so it will always update the cached value with a '1'. We don't have anything that tell us when the alert is gone. No one would clear the cached value with '0' when the alert is gone. (ok, we have two alert bits, so the _other_ one of them will be set to zero, but that's not the point)..
So, in order to report the current alarm value, without relying to the user to clear it, as you asked for, caching seems not an option..
Quoted 2 lines
You might want to update the alert status in case there is no interrupt support, though (maybe you do already; I didn't re-check the code).
OK, good point, I'll make sure it works event in this case.
Quoted 4 lines
The 'sensors' code doesn't know that it is supposed to write into the alert attribute to clear the status, thus unless you have a special application, an alert would never be cleared. That doesn't sound very appealing to me.
So, OK to change the API as you said, however I don't see how this can be done except for doing the manual temperature/limit comparison in the driver:
When you get into show_[max/min]_alert function you can't rely on reading the status register (because as I said in my prev mail it could have been just cleared by reading it in the event handler), but as I explained above it seems to me that you cannot also rely on a value cached in the event handler with this API, because no one will ever clear it..
In other words you will see a status register that could be zero even when we have the alarm, or a cached value that is still 1 even if the alarm has gone time ago.. I could clear the cached value in the show_[max/min]_alert function itself, but this doesn't seems enough to me for reporting to the userspace the _current_ alarm value, as you asked for.
Or am I missing something ?
Quoted 97 lines
Quoted 40 lines
Quoted 40 lines
Quoted 28 lines
Quoted 15 lines
Quoted 15 lines
(quoted diff) +13
-0+ 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); + bool prev_max = priv->max_alert; + bool prev_min = priv->min_alert; + + if (type != I2C_PROTOCOL_SMBUS_ALERT) + return;Quoted 8 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ + dev_dbg(&client->dev, "alert!"); +Any chance to make this a bit more generic and also support interrupts ?
May you please elaborate?
Quoted 9 lines
IMHO adding the logic for handling SMBus alerts using IRQs to any HWmon driver is redundant, because it is already implemented in the SMBus layer, just right now you need patching SMBus layer for enabling it from the DT world (a couple of patches for this are floating around, but they seems not ready for merge yet)... Using this chip with a generic interrupt is indeed exactly what I'm doing..
.. Or am I completely missing your point ?
"just right now you need patching SMBus layer for enabling it from the DT world"
Quoted 8 lines
Quoted 1 line
So it _doesn't_ work, correct ?
I mean, the generic smbus code works for stts751, and the chip seems happy. So, in this case, no need to reimplement it elsewhere. It would just end up to be a copy-paste from
smbus.c.... Just AFAICT we need some mechanism to hook smbus code from DT world..Ok, then let's just go with it. If it doesn't work in some application we'll have to fix it up later.
Quoted 35 lines
Quoted 26 lines
Quoted 26 lines
Quoted 11 lines
Quoted 11 lines
(quoted diff) +11
-0+ +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO, + show_min, set_min, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO, + show_max, set_max, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO, + show_min_alert, set_min_alert, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO, + show_max_alert, set_max_alert, 0); +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,Quoted 8 lines
Quoted 4 lines
(quoted diff) +2
-0+ set_therm, 0); +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO,
show_hyst,
+set_hyst, 0);Please use standard attribute names.
I can convert the event stuff in temp1_[min/max]_alarm for limits and
temp1_alarmfor the flag, but it's not obvious to be what should I use for the therm stuff.. May you please give me advices about this?Quoted 7 lines
Quoted 3 lines
I'd propose temp1_therm_[max/min]_hyst, but they seems not standard anyway...
From the chip datasheet, this is supposed to reflect overtemperature conditions,
stronger than high/low, thus 'crit' seems appropriate.Quoted 9 lines
I see.. I can go with 'crit' :)
Just is there any place where I can document this? Despite the datasheet, I can easily imagine real-world usage scenarios, for example driving a fan from that signal, where 'crit' might be not so intuitive.. Indeed I would like to make it visible the 'crit' <->'therm' binding to users. Maybe an additional attribute '
temp1_crit_label' or something like that ?Quoted 5 lines
You are supposed to provide
Documentation/hwmon/stts751, which would include such information.A label applies to a sensor, not to a sensor attribute, so that would not be appropriate.
OK to provide it, but it would be in kernel source tree. Any further hint that we could give to someone that looks at in /sys ?
Quoted 3 lines
Thanks,
Guenter--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 78 lines · Guenter Roeck
Quoted 70 lines · Andrea Merello
Quoted 61 lines · Guenter Roeck
Quoted 45 lines · Andrea Merello
Quoted 37 lines · Guenter Roeck
Quoted 35 lines · Andrea Merello
Quoted 19 lines
Quoted 19 lines
Quoted 16 lines
Quoted 9 lines
(quoted diff) +4
-0+ if (ret < 0) + return ret; + + priv->max_alert = priv->max_alert || !!(ret &
STTS751_STATUS_TRIPH);
+priv->min_alert=priv->min_alert|| !!(ret &STTS751_STATUS_TRIPL);
+I am not really happy about the caching of alert values. This isn't really
common. Usually we report current alert values when reading. I don't really
see why this driver should be different to all the others.Quoted 1 line
Quoted 1 line
OK
I've tried to avoid caching, but as soon as I read the status register in the alert handler, the device clears that reg, and it will remain cleared up to the next conversion.
So for example if the reader is blocked on poll over the alert attribute, it gets unblocked on alert, and it will very likely read zero..
The only way I can see for avoiding caching, and for changing the API about clearing events as you asked below, is performing a temperature read from the temperature registers and doing software compare with the limits in show_[max/min]_alert.
Quoted 6 lines
Quoted 2 lines
Does this sounds OK for you ?
Not really. That could be done from user space by just reading the temperature
and comparing it against limits.Quoted 6 lines
Sorry, I don't see the point here.. Why the fact that something could be done in userspace should suggest we don't want to do it in the driver? We do, for example, a simple calculation to give an absolute value for the hysteresis; we could do this also in userspace, but we do this in the driver in order to support the proper API..
Datasheet says:
T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A > high limit). T HIGH is cleared when the status register is read, provided the condition no longer exists.
T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low limit). T LOW is cleared when the status register is read, provided the condition no longer exists.Quoted 3 lines
This isn't exactly what you are claiming, since it says "cleared ... provided that the condition no longer exists", which would be more in line with other chips.
Yes, I know.. I've also read that on the datasheet.. And I tried reading the status register in both the alert handler and the show function, but it didn't worked :/
Quoted 7 lines
So I started to suspect the status register got actually clear after reading up to the next conversion, and to test this, I simply read it twice in the alert handler. The first read always read 1, as expected; the second time I always got zero, even if the temperature was still over-limit.. And I soon got another event, that behaved in the same manner.
Quoted 3 lines
Question is if the event bit is set again after the next conversion; this would be relatively easy to handle (cache the status register for one conversion time).
Yes, I meant the HW does exactly this. But it seems to me that caching could end up in subtle races here..
For example suppose that user is reading the lower limit alarm file, or therm, and this will trigger read of the status reg; but a conversion is just ended and set the high limit alarm just before reading the status reg. Then the event handler is invoked but it will find out the status reg to be (incorrectly) zero, (or rely on a value cached when reading the lower limit, that was zero); thus a thread blocked on poll on the high alarm would not be woken up. (ok, this probably will happen in next conversions, but this doesn't sound quite sane to me..).
Quoted 10 lines
Quoted 10 lines
(also I'm thinking about a way to mask events once we know that we are in a out-of-limit condition, to avoid having a lot of them, but simply disabling events and re-enabling them after userspace reads the alarm and find it to be zero, like lm90 driver does, IMHO wouldn't be good here, because we have a single bit to disable both high and low limits..)
Quoted 2 lines
The THRM bit doesn't seem to self-clean either, at least not according to the datasheet.
Quoted 6 lines
Quoted 5 lines
I need to test this.. But this doesn't generate any event, it seems only inteded to drive an external circuit, say a fan, and not to notify SW.. And currently I even have no show function to read it's status (now that I'm on it, I think I should add it...)..
Yes. Note that this pin might be wired to an interrupt.
IMHO it isn't straightforward to handle this..
AFAIK what is done by the smbus code to allow handling smbus events lines with regular interrupt lines is to briefly disabling the IRQ line until the event can be acknowledged with the ARA (in non atomic context).
But here is even worse because there is no ARA; it seems that the therm line can't be "cleared" at all.
Maybe it's possible to do hacks similar to what the smbus code does, but keeping the IRQ line disabled (given that it must not be shared).. Maybe if the IRQ controller could be set in edge-triggered mode it would be easier..
But does it really worth struggling supporting this scenario? There's the event pin explicitly designed for events, and there is code that make it possible to use this even with regular IRQs.. Using also the therm pin as interrupt seems honestly a bit pointless and inappropriate..
Quoted 15 lines
Quoted 11 lines
If I add it, and this func reads the status reg, then it will end up in racy clears of the other high/low event bits anyway, since it's all in the same reg..
Indeed it seems to me that we can rely on chip automatism only for alarm event generation. And I don't see all that advantage in relying on HW comparators for reporting alarms in the show functions; IMHO there we can do our comparisons in SW without any real issue..
Quoted 1 line
Quoted 3 lines
Quoted 1 line
This means I'll need to get a sample and/or eval board to test this myself.
If you want to write your own tests, I will be happy to make them run
Quoted 7 lines
That isn't the point; I'll want to see how the chip really behaves. Reading through the datasheet, the most likely behavior is that the status bits are cleared on read and set again with the next measurement cycle, as already mentioned above. Per datasheet, the alert is supposed to be raised again after each measurement cycle if the condition still exists. This behavior is actually quite common with other chips. If so, all we would need to do is to cache the status register value for one measurement cycle.
Yes, that's what the HW seems to do. I didn't evince that from the datasheet, but it seems to behave that way :)
Quoted 3 lines
Regarding testing, I'll ultimately write a module test for the driver; see
https://github.com/groeck/module-tests for details. For that, all I'll need is a register dump.
Ok, next time I will be working on that board I'll try this...
Quoted 174 lines
Thanks,
GuenterQuoted 171 lines
on my board and give you the results back. Just pass me the test code :)
Quoted 164 lines
Guenter
Quoted 162 lines
Quoted 2 lines
If the chip clears the alert bit on read, why would you need a set attribute to clear it ?
That was used to clear the cached value, not the chip reg.
Quoted 7 lines
From what you are saying, the next alert should update all alert bits, meaning they would be auto-updated at that time. User space would then always read the current state.
Sure, you would cache the value
in the alert function to be able to read it in the read function, but I don't see why all the complexity around it in your code would be needed.The next alert is, by definition, an alert, so it will always update the cached value with a '1'. We don't have anything that tell us when the alert is gone. No one would clear the cached value with '0' when the alert is gone. (ok, we have two alert bits, so the _other_ one of them will be set to zero, but that's not the point)..
So, in order to report the current alarm value, without relying to the user to clear it, as you asked for, caching seems not an option..
Quoted 2 lines
You might want to update the alert status in case there is no interrupt support, though (maybe you do already; I didn't re-check the code).
OK, good point, I'll make sure it works event in this case.
Quoted 4 lines
The 'sensors' code doesn't know that it is supposed to write into the alert attribute to clear the status, thus unless you have a special application, an alert would never be cleared. That doesn't sound very appealing to me.
So, OK to change the API as you said, however I don't see how this can be done except for doing the manual temperature/limit comparison in the driver:
When you get into show_[max/min]_alert function you can't rely on reading the status register (because as I said in my prev mail it could have been just cleared by reading it in the event handler), but as I explained above it seems to me that you cannot also rely on a value cached in the event handler with this API, because no one will ever clear it..
In other words you will see a status register that could be zero even when we have the alarm, or a cached value that is still 1 even if the alarm has gone time ago.. I could clear the cached value in the show_[max/min]_alert function itself, but this doesn't seems enough to me for reporting to the userspace the _current_ alarm value, as you asked for.
Or am I missing something ?
Quoted 97 lines
Quoted 40 lines
Quoted 40 lines
Quoted 28 lines
Quoted 15 lines
Quoted 15 lines
(quoted diff) +13
-0+ 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); + bool prev_max = priv->max_alert; + bool prev_min = priv->min_alert; + + if (type != I2C_PROTOCOL_SMBUS_ALERT) + return;Quoted 8 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ + dev_dbg(&client->dev, "alert!"); +Any chance to make this a bit more generic and also support interrupts ?
May you please elaborate?
Quoted 9 lines
IMHO adding the logic for handling SMBus alerts using IRQs to any HWmon driver is redundant, because it is already implemented in the SMBus layer, just right now you need patching SMBus layer for enabling it from the DT world (a couple of patches for this are floating around, but they seems not ready for merge yet)... Using this chip with a generic interrupt is indeed exactly what I'm doing..
.. Or am I completely missing your point ?
"just right now you need patching SMBus layer for enabling it from the DT world"
Quoted 8 lines
Quoted 1 line
So it _doesn't_ work, correct ?
I mean, the generic smbus code works for stts751, and the chip seems happy. So, in this case, no need to reimplement it elsewhere. It would just end up to be a copy-paste from
smbus.c.... Just AFAICT we need some mechanism to hook smbus code from DT world..Ok, then let's just go with it. If it doesn't work in some application we'll have to fix it up later.
Quoted 35 lines
Quoted 26 lines
Quoted 26 lines
Quoted 11 lines
Quoted 11 lines
(quoted diff) +11
-0+ +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min, S_IWUSR | S_IRUGO, + show_min, set_min, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max, S_IWUSR | S_IRUGO, + show_max, set_max, 0); +static SENSOR_DEVICE_ATTR(temp1_event_min_alert, S_IWUSR | S_IRUGO, + show_min_alert, set_min_alert, 0); +static SENSOR_DEVICE_ATTR(temp1_event_max_alert, S_IWUSR | S_IRUGO, + show_max_alert, set_max_alert, 0); +static SENSOR_DEVICE_ATTR(temp1_therm, S_IWUSR | S_IRUGO, show_therm,Quoted 8 lines
Quoted 4 lines
(quoted diff) +2
-0+ set_therm, 0); +static SENSOR_DEVICE_ATTR(temp1_therm_hyst, S_IWUSR | S_IRUGO,
show_hyst,
+set_hyst, 0);Please use standard attribute names.
I can convert the event stuff in temp1_[min/max]_alarm for limits and
temp1_alarmfor the flag, but it's not obvious to be what should I use for the therm stuff.. May you please give me advices about this?Quoted 7 lines
Quoted 3 lines
I'd propose temp1_therm_[max/min]_hyst, but they seems not standard anyway...
From the chip datasheet, this is supposed to reflect overtemperature conditions,
stronger than high/low, thus 'crit' seems appropriate.Quoted 9 lines
I see.. I can go with 'crit' :)
Just is there any place where I can document this? Despite the datasheet, I can easily imagine real-world usage scenarios, for example driving a fan from that signal, where 'crit' might be not so intuitive.. Indeed I would like to make it visible the 'crit' <->'therm' binding to users. Maybe an additional attribute '
temp1_crit_label' or something like that ?Quoted 5 lines
You are supposed to provide
Documentation/hwmon/stts751, which would include such information.A label applies to a sensor, not to a sensor attribute, so that would not be appropriate.
OK to provide it, but it would be in kernel source tree. Any further hint that we could give to someone that looks at in /sys ?
Quoted 3 lines
Thanks,
Guenter--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Guenter Roeck
maintainerQuoted 86 lines · Andrea Merello
Quoted 78 lines · Guenter Roeck
Quoted 70 lines · Andrea Merello
Quoted 61 lines · Guenter Roeck
Quoted 45 lines · Andrea Merello
Quoted 37 lines · Guenter Roeck
Quoted 35 lines · Andrea Merello
Quoted 19 lines
Quoted 19 lines
Quoted 16 lines
Quoted 9 lines
(quoted diff) +4
-0+ if (ret < 0) + return ret; + + priv->max_alert = priv->max_alert || !!(ret &
STTS751_STATUS_TRIPH);
+priv->min_alert=priv->min_alert|| !!(ret &STTS751_STATUS_TRIPL);
+I am not really happy about the caching of alert values. This isn't really
common. Usually we report current alert values when reading. I don't really
see why this driver should be different to all the others.Quoted 1 line
Quoted 1 line
OK
I've tried to avoid caching, but as soon as I read the status register in the alert handler, the device clears that reg, and it will remain cleared up to the next conversion.
So for example if the reader is blocked on poll over the alert attribute, it gets unblocked on alert, and it will very likely read zero..
The only way I can see for avoiding caching, and for changing the API about clearing events as you asked below, is performing a temperature read from the temperature registers and doing software compare with the limits in show_[max/min]_alert.
Quoted 6 lines
Quoted 2 lines
Does this sounds OK for you ?
Not really. That could be done from user space by just reading the temperature
and comparing it against limits.Quoted 6 lines
Sorry, I don't see the point here.. Why the fact that something could be done in userspace should suggest we don't want to do it in the driver? We do, for example, a simple calculation to give an absolute value for the hysteresis; we could do this also in userspace, but we do this in the driver in order to support the proper API..
Datasheet says:
T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A > high limit). T HIGH is cleared when the status register is read, provided the condition no longer exists.
T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low limit). T LOW is cleared when the status register is read, provided the condition no longer exists.Quoted 3 lines
This isn't exactly what you are claiming, since it says "cleared ... provided that the condition no longer exists", which would be more in line with other chips.
Yes, I know.. I've also read that on the datasheet.. And I tried reading the status register in both the alert handler and the show function, but it didn't worked :/
Quoted 7 lines
So I started to suspect the status register got actually clear after reading up to the next conversion, and to test this, I simply read it twice in the alert handler. The first read always read 1, as expected; the second time I always got zero, even if the temperature was still over-limit.. And I soon got another event, that behaved in the same manner.
Quoted 3 lines
Question is if the event bit is set again after the next conversion; this would be relatively easy to handle (cache the status register for one conversion time).
Yes, I meant the HW does exactly this. But it seems to me that caching could end up in subtle races here..
Quoted 10 lines
For example suppose that user is reading the lower limit alarm file, or therm, and this will trigger read of the status reg; but a conversion is just ended and set the high limit alarm just before reading the status reg. Then the event handler is invoked but it will find out the status reg to be (incorrectly) zero, (or rely on a value cached when reading the lower limit, that was zero); thus a thread blocked on poll on the high alarm would not be woken up. (ok, this probably will happen in next conversions, but this doesn't sound quite sane to me..).
Good catch and thinking, but there is a workaround. If the cache time has not expired, just logically or the cached status value with the current value in the alert function, and update the time stamp. This way nothing gets lost.
Quoted 33 lines
Quoted 11 lines
Quoted 11 lines
(also I'm thinking about a way to mask events once we know that we are in a out-of-limit condition, to avoid having a lot of them, but simply disabling events and re-enabling them after userspace reads the alarm and find it to be zero, like lm90 driver does, IMHO wouldn't be good here, because we have a single bit to disable both high and low limits..)
Quoted 2 lines
The THRM bit doesn't seem to self-clean either, at least not according to the datasheet.
Quoted 6 lines
Quoted 5 lines
I need to test this.. But this doesn't generate any event, it seems only inteded to drive an external circuit, say a fan, and not to notify SW.. And currently I even have no show function to read it's status (now that I'm on it, I think I should add it...)..
Yes. Note that this pin might be wired to an interrupt.
IMHO it isn't straightforward to handle this..
AFAIK what is done by the smbus code to allow handling smbus events lines with regular interrupt lines is to briefly disabling the IRQ line until the event can be acknowledged with the ARA (in non atomic context).
But here is even worse because there is no ARA; it seems that the therm line can't be "cleared" at all.
Maybe it's possible to do hacks similar to what the smbus code does, but keeping the IRQ line disabled (given that it must not be shared).. Maybe if the IRQ controller could be set in edge-triggered mode it would be easier..
Quoted 6 lines
But does it really worth struggling supporting this scenario? There's the event pin explicitly designed for events, and there is code that make it possible to use this even with regular IRQs.. Using also the therm pin as interrupt seems honestly a bit pointless and inappropriate..
Correct, one would use an edge triggered interrupt in this case. But you are right, this should only be implemented if that use case is actually encountered.
Thanks,
Guenter
Quoted 97 lines · Guenter Roeck
Quoted 86 lines · Andrea Merello
Quoted 78 lines · Guenter Roeck
Quoted 70 lines · Andrea Merello
Quoted 61 lines · Guenter Roeck
Quoted 45 lines · Andrea Merello
Quoted 37 lines · Guenter Roeck
Quoted 35 lines · Andrea Merello
Quoted 19 lines
Quoted 19 lines
Quoted 16 lines
Quoted 9 lines
(quoted diff) +4
-0+ if (ret < 0) + return ret; + + priv->max_alert = priv->max_alert || !!(ret &
STTS751_STATUS_TRIPH);
+priv->min_alert=priv->min_alert|| !!(ret &STTS751_STATUS_TRIPL);
+I am not really happy about the caching of alert values. This isn't really
common. Usually we report current alert values when reading. I don't really
see why this driver should be different to all the others.Quoted 1 line
Quoted 1 line
OK
I've tried to avoid caching, but as soon as I read the status register in the alert handler, the device clears that reg, and it will remain cleared up to the next conversion.
So for example if the reader is blocked on poll over the alert attribute, it gets unblocked on alert, and it will very likely read zero..
The only way I can see for avoiding caching, and for changing the API about clearing events as you asked below, is performing a temperature read from the temperature registers and doing software compare with the limits in show_[max/min]_alert.
Quoted 6 lines
Quoted 2 lines
Does this sounds OK for you ?
Not really. That could be done from user space by just reading the temperature
and comparing it against limits.Quoted 6 lines
Sorry, I don't see the point here.. Why the fact that something could be done in userspace should suggest we don't want to do it in the driver? We do, for example, a simple calculation to give an absolute value for the hysteresis; we could do this also in userspace, but we do this in the driver in order to support the proper API..
Datasheet says:
T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A > high limit). T HIGH is cleared when the status register is read, provided the condition no longer exists.
T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low limit). T LOW is cleared when the status register is read, provided the condition no longer exists.Quoted 3 lines
This isn't exactly what you are claiming, since it says "cleared ... provided that the condition no longer exists", which would be more in line with other chips.
Yes, I know.. I've also read that on the datasheet.. And I tried reading the status register in both the alert handler and the show function, but it didn't worked :/
Quoted 7 lines
So I started to suspect the status register got actually clear after reading up to the next conversion, and to test this, I simply read it twice in the alert handler. The first read always read 1, as expected; the second time I always got zero, even if the temperature was still over-limit.. And I soon got another event, that behaved in the same manner.
Quoted 3 lines
Question is if the event bit is set again after the next conversion; this would be relatively easy to handle (cache the status register for one conversion time).
Yes, I meant the HW does exactly this. But it seems to me that caching could end up in subtle races here..
Quoted 10 lines
For example suppose that user is reading the lower limit alarm file, or therm, and this will trigger read of the status reg; but a conversion is just ended and set the high limit alarm just before reading the status reg. Then the event handler is invoked but it will find out the status reg to be (incorrectly) zero, (or rely on a value cached when reading the lower limit, that was zero); thus a thread blocked on poll on the high alarm would not be woken up. (ok, this probably will happen in next conversions, but this doesn't sound quite sane to me..).
Quoted 4 lines
Good catch and thinking, but there is a workaround. If the cache time has not expired, just logically or the cached status value with the current value in the alert function, and update the time stamp. This way nothing gets lost.
Hmm, it seems OK; thanks for suggesting this :) I'll try to go this way..
BTW I would say that the cache time for the alert should be coservatively a bit longer than the conversion time (to avoid early expiration when the status register has been not restored yet). On the other hand for the temperature reading I'm using a cache time that is shorter (I think it is 1/4) than the conversion interval, so that the user gets a reasonably fresh value (becasue the chip sampling freq is asyncrhronous wrt the reads we do). Do you think it's worth to keep doing this (so we have two different cache time for alarms and temp value), or do you think that it's pointless, and better to unify it (at the longer one, I would say)?
Quoted 45 lines
Quoted 33 lines
Quoted 11 lines
Quoted 11 lines
(also I'm thinking about a way to mask events once we know that we are in a out-of-limit condition, to avoid having a lot of them, but simply disabling events and re-enabling them after userspace reads the alarm and find it to be zero, like lm90 driver does, IMHO wouldn't be good here, because we have a single bit to disable both high and low limits..)
Quoted 2 lines
The THRM bit doesn't seem to self-clean either, at least not according to the datasheet.
Quoted 6 lines
Quoted 5 lines
I need to test this.. But this doesn't generate any event, it seems only inteded to drive an external circuit, say a fan, and not to notify SW.. And currently I even have no show function to read it's status (now that I'm on it, I think I should add it...)..
Yes. Note that this pin might be wired to an interrupt.
IMHO it isn't straightforward to handle this..
AFAIK what is done by the smbus code to allow handling smbus events lines with regular interrupt lines is to briefly disabling the IRQ line until the event can be acknowledged with the ARA (in non atomic context).
But here is even worse because there is no ARA; it seems that the therm line can't be "cleared" at all.
Maybe it's possible to do hacks similar to what the smbus code does, but keeping the IRQ line disabled (given that it must not be shared).. Maybe if the IRQ controller could be set in edge-triggered mode it would be easier..
Quoted 6 lines
But does it really worth struggling supporting this scenario? There's the event pin explicitly designed for events, and there is code that make it possible to use this even with regular IRQs.. Using also the therm pin as interrupt seems honestly a bit pointless and inappropriate..
Correct, one would use an edge triggered interrupt in this case. But you are right, this should only be implemented if that use case is actually encountered.
Thanks,
Guenter
Guenter Roeck
maintainerQuoted 105 lines · Andrea Merello
Quoted 97 lines · Guenter Roeck
Quoted 86 lines · Andrea Merello
Quoted 78 lines · Guenter Roeck
Quoted 70 lines · Andrea Merello
Quoted 61 lines · Guenter Roeck
Quoted 45 lines · Andrea Merello
Quoted 37 lines · Guenter Roeck
Quoted 35 lines · Andrea Merello
Quoted 19 lines
Quoted 19 lines
Quoted 16 lines
Quoted 9 lines
+ if (ret < 0)
+ return ret;
+
+ priv->max_alert = priv->max_alert || !!(ret &
STTS751_STATUS_TRIPH);
+ priv->min_alert = priv->min_alert || !!(ret &
STTS751_STATUS_TRIPL);
+I am not really happy about the caching of alert values. This isn't really
common. Usually we report current alert values when reading. I don't really
see why this driver should be different to all the others.Quoted 1 line
Quoted 1 line
OK
I've tried to avoid caching, but as soon as I read the status register in the alert handler, the device clears that reg, and it will remain cleared up to the next conversion.
So for example if the reader is blocked on poll over the alert attribute, it gets unblocked on alert, and it will very likely read zero..
The only way I can see for avoiding caching, and for changing the API about clearing events as you asked below, is performing a temperature read from the temperature registers and doing software compare with the limits in show_[max/min]_alert.
Quoted 6 lines
Quoted 2 lines
Does this sounds OK for you ?
Not really. That could be done from user space by just reading the temperature
and comparing it against limits.Quoted 6 lines
Sorry, I don't see the point here.. Why the fact that something could be done in userspace should suggest we don't want to do it in the driver? We do, for example, a simple calculation to give an absolute value for the hysteresis; we could do this also in userspace, but we do this in the driver in order to support the proper API..
Datasheet says:
T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A > high limit). T HIGH is cleared when the status register is read, provided the condition no longer exists.
T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low limit). T LOW is cleared when the status register is read, provided the condition no longer exists.Quoted 3 lines
This isn't exactly what you are claiming, since it says "cleared ... provided that the condition no longer exists", which would be more in line with other chips.
Yes, I know.. I've also read that on the datasheet.. And I tried reading the status register in both the alert handler and the show function, but it didn't worked :/
Quoted 7 lines
So I started to suspect the status register got actually clear after reading up to the next conversion, and to test this, I simply read it twice in the alert handler. The first read always read 1, as expected; the second time I always got zero, even if the temperature was still over-limit.. And I soon got another event, that behaved in the same manner.
Quoted 3 lines
Question is if the event bit is set again after the next conversion; this would be relatively easy to handle (cache the status register for one conversion time).
Yes, I meant the HW does exactly this. But it seems to me that caching could end up in subtle races here..
Quoted 10 lines
For example suppose that user is reading the lower limit alarm file, or therm, and this will trigger read of the status reg; but a conversion is just ended and set the high limit alarm just before reading the status reg. Then the event handler is invoked but it will find out the status reg to be (incorrectly) zero, (or rely on a value cached when reading the lower limit, that was zero); thus a thread blocked on poll on the high alarm would not be woken up. (ok, this probably will happen in next conversions, but this doesn't sound quite sane to me..).
Quoted 4 lines
Good catch and thinking, but there is a workaround. If the cache time has not expired, just logically or the cached status value with the current value in the alert function, and update the time stamp. This way nothing gets lost.
Hmm, it seems OK; thanks for suggesting this :) I'll try to go this way..
Quoted 11 lines
BTW I would say that the cache time for the alert should be coservatively a bit longer than the conversion time (to avoid early expiration when the status register has been not restored yet). On the other hand for the temperature reading I'm using a cache time that is shorter (I think it is 1/4) than the conversion interval, so that the user gets a reasonably fresh value (becasue the chip sampling freq is asyncrhronous wrt the reads we do). Do you think it's worth to keep doing this (so we have two different cache time for alarms and temp value), or do you think that it's pointless, and better to unify it (at the longer one, I would say)?
I would suggest to use a single cache time, in line with the real cache time, using the maximum as per the datasheet and possibly rounded up. If the user wants faster readings, that can be made configurable using the 'update-interval' attribute. If the chip is configured to only read the temperature once per, say, 16 seconds, this is what the user wanted, and real-time accuracy is not guaranteed anyway.
Quoted 45 lines
Quoted 45 lines
Quoted 33 lines
Quoted 11 lines
Quoted 11 lines
(also I'm thinking about a way to mask events once we know that we are in a out-of-limit condition, to avoid having a lot of them, but simply disabling events and re-enabling them after userspace reads the alarm and find it to be zero, like lm90 driver does, IMHO wouldn't be good here, because we have a single bit to disable both high and low limits..)
Quoted 2 lines
The THRM bit doesn't seem to self-clean either, at least not according to the datasheet.
Quoted 6 lines
Quoted 5 lines
I need to test this.. But this doesn't generate any event, it seems only inteded to drive an external circuit, say a fan, and not to notify SW.. And currently I even have no show function to read it's status (now that I'm on it, I think I should add it...)..
Yes. Note that this pin might be wired to an interrupt.
IMHO it isn't straightforward to handle this..
AFAIK what is done by the smbus code to allow handling smbus events lines with regular interrupt lines is to briefly disabling the IRQ line until the event can be acknowledged with the ARA (in non atomic context).
But here is even worse because there is no ARA; it seems that the therm line can't be "cleared" at all.
Maybe it's possible to do hacks similar to what the smbus code does, but keeping the IRQ line disabled (given that it must not be shared).. Maybe if the IRQ controller could be set in edge-triggered mode it would be easier..
Quoted 6 lines
But does it really worth struggling supporting this scenario? There's the event pin explicitly designed for events, and there is code that make it possible to use this even with regular IRQs.. Using also the therm pin as interrupt seems honestly a bit pointless and inappropriate..
Correct, one would use an edge triggered interrupt in this case. But you are right, this should only be implemented if that use case is actually encountered.
Thanks,
Guenter
Quoted 118 lines · Guenter Roeck
Quoted 105 lines · Andrea Merello
Quoted 97 lines · Guenter Roeck
Quoted 86 lines · Andrea Merello
Quoted 78 lines · Guenter Roeck
Quoted 70 lines · Andrea Merello
Quoted 61 lines · Guenter Roeck
Quoted 45 lines · Andrea Merello
Quoted 37 lines · Guenter Roeck
Quoted 35 lines · Andrea Merello
Quoted 19 lines
Quoted 19 lines
Quoted 16 lines
+ if (ret < 0)
+ return ret;
+
+ priv->max_alert = priv->max_alert || !!(ret &
STTS751_STATUS_TRIPH);
+ priv->min_alert = priv->min_alert || !!(ret &
STTS751_STATUS_TRIPL);
+I am not really happy about the caching of alert values. This isn't
really
common. Usually we report current alert values when reading. I don't
really
see why this driver should be different to all the others.Quoted 1 line
Quoted 1 line
OK
I've tried to avoid caching, but as soon as I read the status register in the alert handler, the device clears that reg, and it will remain cleared up to the next conversion.
So for example if the reader is blocked on poll over the alert attribute, it gets unblocked on alert, and it will very likely read zero..
The only way I can see for avoiding caching, and for changing the API about clearing events as you asked below, is performing a temperature read from the temperature registers and doing software compare with the limits in show_[max/min]_alert.
Quoted 6 lines
Quoted 2 lines
Does this sounds OK for you ?
Not really. That could be done from user space by just reading the temperature
and comparing it against limits.Quoted 6 lines
Sorry, I don't see the point here.. Why the fact that something could be done in userspace should suggest we don't want to do it in the driver? We do, for example, a simple calculation to give an absolute value for the hysteresis; we could do this also in userspace, but we do this in the driver in order to support the proper API..
Datasheet says:
T HIGH : [6] Bit = 1 indicates temperature high limit has been exceeded (T A > high limit). T HIGH is cleared when the status register is read, provided the condition no longer exists.
T LOW : [5] Bit = 1 indicates the is at or below the low limit (T A ≤ low limit). T LOW is cleared when the status register is read, provided the condition no longer exists.Quoted 3 lines
This isn't exactly what you are claiming, since it says "cleared ... provided that the condition no longer exists", which would be more in line with other chips.
Yes, I know.. I've also read that on the datasheet.. And I tried reading the status register in both the alert handler and the show function, but it didn't worked :/
Quoted 7 lines
So I started to suspect the status register got actually clear after reading up to the next conversion, and to test this, I simply read it twice in the alert handler. The first read always read 1, as expected; the second time I always got zero, even if the temperature was still over-limit.. And I soon got another event, that behaved in the same manner.
Quoted 3 lines
Question is if the event bit is set again after the next conversion; this would be relatively easy to handle (cache the status register for one conversion time).
Yes, I meant the HW does exactly this. But it seems to me that caching could end up in subtle races here..
Quoted 10 lines
For example suppose that user is reading the lower limit alarm file, or therm, and this will trigger read of the status reg; but a conversion is just ended and set the high limit alarm just before reading the status reg. Then the event handler is invoked but it will find out the status reg to be (incorrectly) zero, (or rely on a value cached when reading the lower limit, that was zero); thus a thread blocked on poll on the high alarm would not be woken up. (ok, this probably will happen in next conversions, but this doesn't sound quite sane to me..).
Quoted 4 lines
Good catch and thinking, but there is a workaround. If the cache time has not expired, just logically or the cached status value with the current value in the alert function, and update the time stamp. This way nothing gets lost.
Hmm, it seems OK; thanks for suggesting this :) I'll try to go this way..
Quoted 11 lines
BTW I would say that the cache time for the alert should be coservatively a bit longer than the conversion time (to avoid early expiration when the status register has been not restored yet). On the other hand for the temperature reading I'm using a cache time that is shorter (I think it is 1/4) than the conversion interval, so that the user gets a reasonably fresh value (becasue the chip sampling freq is asyncrhronous wrt the reads we do). Do you think it's worth to keep doing this (so we have two different cache time for alarms and temp value), or do you think that it's pointless, and better to unify it (at the longer one, I would say)?
Quoted 6 lines
I would suggest to use a single cache time, in line with the real cache time, using the maximum as per the datasheet and possibly rounded up. If the user wants faster readings, that can be made configurable using the 'update-interval' attribute. If the chip is configured to only read the temperature once per, say, 16 seconds, this is what the user wanted, and real-time accuracy is not guaranteed anyway.
Ok, I'll come back with a new version of the patch changed as discussed.
Thanks
Andrea
Quoted 46 lines
Quoted 46 lines
Quoted 46 lines
Quoted 34 lines
Quoted 12 lines
Quoted 12 lines
(also I'm thinking about a way to mask events once we know that we are in a out-of-limit condition, to avoid having a lot of them, but simply disabling events and re-enabling them after userspace reads the alarm and find it to be zero, like lm90 driver does, IMHO wouldn't be good here, because we have a single bit to disable both high and low limits..)
Quoted 2 lines
The THRM bit doesn't seem to self-clean either, at least not according to the datasheet.
Quoted 6 lines
Quoted 5 lines
I need to test this.. But this doesn't generate any event, it seems only inteded to drive an external circuit, say a fan, and not to notify SW.. And currently I even have no show function to read it's status (now that I'm on it, I think I should add it...)..
Yes. Note that this pin might be wired to an interrupt.
IMHO it isn't straightforward to handle this..
AFAIK what is done by the smbus code to allow handling smbus events lines with regular interrupt lines is to briefly disabling the IRQ line until the event can be acknowledged with the ARA (in non atomic context).
But here is even worse because there is no ARA; it seems that the therm line can't be "cleared" at all.
Maybe it's possible to do hacks similar to what the smbus code does, but keeping the IRQ line disabled (given that it must not be shared).. Maybe if the IRQ controller could be set in edge-triggered mode it would be easier..
Quoted 6 lines
But does it really worth struggling supporting this scenario? There's the event pin explicitly designed for events, and there is code that make it possible to use this even with regular IRQs.. Using also the therm pin as interrupt seems honestly a bit pointless and inappropriate..
Correct, one would use an edge triggered interrupt in this case. But you are right, this should only be implemented if that use case is actually encountered.
Thanks,
Guenter