* [PATCH v2 3/5] hwmon: (tmp102) Improve handling of initial read delay
2016-06-26 2:40 [PATCH v2 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function Guenter Roeck
2016-06-26 2:40 ` [PATCH v2 2/5] hwmon: (tmp102) Drop FSF address Guenter Roeck
@ 2016-06-26 2:40 ` Guenter Roeck
2016-06-26 13:31 ` Nishanth Menon
2016-06-26 2:40 ` [PATCH v2 4/5] hwmon: (tmp102) Rework chip configuration Guenter Roeck
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2016-06-26 2:40 UTC (permalink / raw)
To: Jean Delvare; +Cc: Nishanth Menon, linux-hwmon, linux-kernel, Guenter Roeck
If the chip was in shutdown mode when the driver was loaded, the first
conversion is ready no more than 35 milli-seconds after the chip was
taken out of shutdown. The driver delay was so far set to 333 ms (HZ / 3),
which is much higher than the maximum time needed by the chip.
Reduce the time to 35 milli-seconds. Instead of returning -EAGAIN if data
is not yet available, go to sleep for the remaining time.
Introduce a 'valid' flag to ensure that sensor data is actually read
even if requested less than 333 ms after the driver was loaded.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: No change
drivers/hwmon/tmp102.c | 44 ++++++++++++++++++++++++++++----------------
1 file changed, 28 insertions(+), 16 deletions(-)
diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c
index 5bdf262e6a0e..615f9c176aaf 100644
--- a/drivers/hwmon/tmp102.c
+++ b/drivers/hwmon/tmp102.c
@@ -13,6 +13,7 @@
* GNU General Public License for more details.
*/
+#include <linux/delay.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/slab.h>
@@ -46,13 +47,16 @@
#define TMP102_TLOW_REG 0x02
#define TMP102_THIGH_REG 0x03
+#define CONVERSION_TIME_MS 35 /* in milli-seconds */
+
struct tmp102 {
struct i2c_client *client;
struct mutex lock;
u16 config_orig;
unsigned long last_update;
+ unsigned long ready_time;
+ bool valid;
int temp[3];
- bool first_time;
};
/* convert left adjusted 13-bit TMP102 register value to milliCelsius */
@@ -78,8 +82,16 @@ static struct tmp102 *tmp102_update_device(struct device *dev)
struct tmp102 *tmp102 = dev_get_drvdata(dev);
struct i2c_client *client = tmp102->client;
+ /* Is it too early to return a conversion ? */
+ if (time_before(jiffies, tmp102->ready_time)) {
+ unsigned long sleeptime = tmp102->ready_time - jiffies;
+
+ msleep(jiffies_to_msecs(sleeptime));
+ }
+
mutex_lock(&tmp102->lock);
- if (time_after(jiffies, tmp102->last_update + HZ / 3)) {
+ if (!tmp102->valid ||
+ time_after(jiffies, tmp102->last_update + HZ / 3)) {
int i;
for (i = 0; i < ARRAY_SIZE(tmp102->temp); ++i) {
int status = i2c_smbus_read_word_swapped(client,
@@ -88,7 +100,7 @@ static struct tmp102 *tmp102_update_device(struct device *dev)
tmp102->temp[i] = tmp102_reg_to_mC(status);
}
tmp102->last_update = jiffies;
- tmp102->first_time = false;
+ tmp102->valid = true;
}
mutex_unlock(&tmp102->lock);
return tmp102;
@@ -98,12 +110,6 @@ static int tmp102_read_temp(void *dev, int *temp)
{
struct tmp102 *tmp102 = tmp102_update_device(dev);
- /* Is it too early even to return a conversion? */
- if (tmp102->first_time) {
- dev_dbg(dev, "%s: Conversion not ready yet..\n", __func__);
- return -EAGAIN;
- }
-
*temp = tmp102->temp[0];
return 0;
@@ -116,10 +122,6 @@ static ssize_t tmp102_show_temp(struct device *dev,
struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
struct tmp102 *tmp102 = tmp102_update_device(dev);
- /* Is it too early even to return a read? */
- if (tmp102->first_time)
- return -EAGAIN;
-
return sprintf(buf, "%d\n", tmp102->temp[sda->index]);
}
@@ -224,11 +226,18 @@ static int tmp102_probe(struct i2c_client *client,
dev_err(dev, "config settings did not stick\n");
return -ENODEV;
}
- tmp102->last_update = jiffies;
- /* Mark that we are not ready with data until conversion is complete */
- tmp102->first_time = true;
+
mutex_init(&tmp102->lock);
+ tmp102->ready_time = jiffies;
+ if (tmp102->config_orig & TMP102_CONF_SD) {
+ /*
+ * Mark that we are not ready with data until the first
+ * conversion is complete
+ */
+ tmp102->ready_time += msecs_to_jiffies(CONVERSION_TIME_MS);
+ }
+
hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
tmp102,
tmp102_groups);
@@ -261,12 +270,15 @@ static int tmp102_suspend(struct device *dev)
static int tmp102_resume(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
+ struct tmp102 *tmp102 = i2c_get_clientdata(client);
int config;
config = i2c_smbus_read_word_swapped(client, TMP102_CONF_REG);
if (config < 0)
return config;
+ tmp102->ready_time = jiffies + msecs_to_jiffies(CONVERSION_TIME_MS);
+
config &= ~TMP102_CONF_SD;
return i2c_smbus_write_word_swapped(client, TMP102_CONF_REG, config);
}
--
2.5.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 4/5] hwmon: (tmp102) Rework chip configuration
2016-06-26 2:40 [PATCH v2 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function Guenter Roeck
2016-06-26 2:40 ` [PATCH v2 2/5] hwmon: (tmp102) Drop FSF address Guenter Roeck
2016-06-26 2:40 ` [PATCH v2 3/5] hwmon: (tmp102) Improve handling of initial read delay Guenter Roeck
@ 2016-06-26 2:40 ` Guenter Roeck
2016-06-26 2:40 ` [PATCH v2 5/5] hwmon: (tmp102) Convert to use regmap, and drop local cache Guenter Roeck
2016-06-26 13:32 ` [PATCH v2 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function Nishanth Menon
4 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2016-06-26 2:40 UTC (permalink / raw)
To: Jean Delvare; +Cc: Nishanth Menon, linux-hwmon, linux-kernel, Guenter Roeck
So far the chip was forced into polarity 0, even if it was preconfigured
differently. Do not touch the polarity when configuring the chip.
Also, the configuration register was read beack to check if the
configuration 'sticks'. Ultimately, that is similar to checking if the
chip is a tmp102 in the first place. Checking if a write into the
configuration register was successful is really not the way to do it,
and quite risky if the chip is not a tmp102, so drop that check.
Instead, verify if the configuration register has unexpected bits set
before writing into it.
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: No change
drivers/hwmon/tmp102.c | 37 ++++++++++++++++++++++---------------
1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c
index 615f9c176aaf..487b4ef5992c 100644
--- a/drivers/hwmon/tmp102.c
+++ b/drivers/hwmon/tmp102.c
@@ -47,6 +47,17 @@
#define TMP102_TLOW_REG 0x02
#define TMP102_THIGH_REG 0x03
+#define TMP102_CONFREG_MASK (TMP102_CONF_SD | TMP102_CONF_TM | \
+ TMP102_CONF_POL | TMP102_CONF_F0 | \
+ TMP102_CONF_F1 | TMP102_CONF_OS | \
+ TMP102_CONF_EM | TMP102_CONF_AL | \
+ TMP102_CONF_CR0 | TMP102_CONF_CR1)
+
+#define TMP102_CONFIG_CLEAR (TMP102_CONF_SD | TMP102_CONF_OS | \
+ TMP102_CONF_CR0)
+#define TMP102_CONFIG_SET (TMP102_CONF_TM | TMP102_CONF_EM | \
+ TMP102_CONF_CR1)
+
#define CONVERSION_TIME_MS 35 /* in milli-seconds */
struct tmp102 {
@@ -163,9 +174,6 @@ static struct attribute *tmp102_attrs[] = {
};
ATTRIBUTE_GROUPS(tmp102);
-#define TMP102_CONFIG (TMP102_CONF_TM | TMP102_CONF_EM | TMP102_CONF_CR1)
-#define TMP102_CONFIG_RD_ONLY (TMP102_CONF_R0 | TMP102_CONF_R1 | TMP102_CONF_AL)
-
static const struct thermal_zone_of_device_ops tmp102_of_thermal_ops = {
.get_temp = tmp102_read_temp,
};
@@ -206,26 +214,25 @@ static int tmp102_probe(struct i2c_client *client,
dev_err(dev, "error reading config register\n");
return status;
}
+
+ if ((status & ~TMP102_CONFREG_MASK) !=
+ (TMP102_CONF_R0 | TMP102_CONF_R1)) {
+ dev_err(dev, "unexpected config register value\n");
+ return -ENODEV;
+ }
+
tmp102->config_orig = status;
devm_add_action(dev, tmp102_restore_config, tmp102);
- status = i2c_smbus_write_word_swapped(client, TMP102_CONF_REG,
- TMP102_CONFIG);
+ status &= ~TMP102_CONFIG_CLEAR;
+ status |= TMP102_CONFIG_SET;
+
+ status = i2c_smbus_write_word_swapped(client, TMP102_CONF_REG, status);
if (status < 0) {
dev_err(dev, "error writing config register\n");
return status;
}
- status = i2c_smbus_read_word_swapped(client, TMP102_CONF_REG);
- if (status < 0) {
- dev_err(dev, "error reading config register\n");
- return status;
- }
- status &= ~TMP102_CONFIG_RD_ONLY;
- if (status != TMP102_CONFIG) {
- dev_err(dev, "config settings did not stick\n");
- return -ENODEV;
- }
mutex_init(&tmp102->lock);
--
2.5.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 5/5] hwmon: (tmp102) Convert to use regmap, and drop local cache
2016-06-26 2:40 [PATCH v2 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function Guenter Roeck
` (2 preceding siblings ...)
2016-06-26 2:40 ` [PATCH v2 4/5] hwmon: (tmp102) Rework chip configuration Guenter Roeck
@ 2016-06-26 2:40 ` Guenter Roeck
2016-06-26 13:27 ` Nishanth Menon
2016-06-26 13:32 ` [PATCH v2 1/5] hwmon: (tmp102) Use devm_add_action to register cleanup function Nishanth Menon
4 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2016-06-26 2:40 UTC (permalink / raw)
To: Jean Delvare
Cc: Nishanth Menon, linux-hwmon, linux-kernel, Guenter Roeck, Mark Brown
By concerting the driver to regmap, we can use regmap to cache non-volatile
registers. Stop caching the temperature register; while potentially reading
it more often can result in reading it more often than necessary, this is
offset by the gain due to not re-reading the limit registers.
Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Set use_single_rw to indicate that the chip can not perform
read operations crossing register boundaries.
Use REGCACHE_RBTREE instead REGCACHE_FLAT. REGCACHE_FLAT does not
initialize the cache from the chip unless num_reg_defaults_raw is
provided. If it is provided without actual cache values, it complains
with 'No cache defaults, reading back from HW'. REGCACHE_RBTREE
automatically initializes register values from the chip if no
defaults are provided, and does not complain about it.
Note: I'll drop the Cc: to Mark later.
drivers/hwmon/tmp102.c | 156 +++++++++++++++++++++++++------------------------
1 file changed, 79 insertions(+), 77 deletions(-)
diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c
index 487b4ef5992c..e4a2314a2c49 100644
--- a/drivers/hwmon/tmp102.c
+++ b/drivers/hwmon/tmp102.c
@@ -24,6 +24,7 @@
#include <linux/mutex.h>
#include <linux/device.h>
#include <linux/jiffies.h>
+#include <linux/regmap.h>
#include <linux/thermal.h>
#include <linux/of.h>
@@ -61,13 +62,9 @@
#define CONVERSION_TIME_MS 35 /* in milli-seconds */
struct tmp102 {
- struct i2c_client *client;
- struct mutex lock;
+ struct regmap *regmap;
u16 config_orig;
- unsigned long last_update;
unsigned long ready_time;
- bool valid;
- int temp[3];
};
/* convert left adjusted 13-bit TMP102 register value to milliCelsius */
@@ -82,16 +79,11 @@ static inline u16 tmp102_mC_to_reg(int val)
return (val * 128) / 1000;
}
-static const u8 tmp102_reg[] = {
- TMP102_TEMP_REG,
- TMP102_TLOW_REG,
- TMP102_THIGH_REG,
-};
-
-static struct tmp102 *tmp102_update_device(struct device *dev)
+static int tmp102_read_temp(void *dev, int *temp)
{
struct tmp102 *tmp102 = dev_get_drvdata(dev);
- struct i2c_client *client = tmp102->client;
+ unsigned int reg;
+ int ret;
/* Is it too early to return a conversion ? */
if (time_before(jiffies, tmp102->ready_time)) {
@@ -100,28 +92,11 @@ static struct tmp102 *tmp102_update_device(struct device *dev)
msleep(jiffies_to_msecs(sleeptime));
}
- mutex_lock(&tmp102->lock);
- if (!tmp102->valid ||
- time_after(jiffies, tmp102->last_update + HZ / 3)) {
- int i;
- for (i = 0; i < ARRAY_SIZE(tmp102->temp); ++i) {
- int status = i2c_smbus_read_word_swapped(client,
- tmp102_reg[i]);
- if (status > -1)
- tmp102->temp[i] = tmp102_reg_to_mC(status);
- }
- tmp102->last_update = jiffies;
- tmp102->valid = true;
- }
- mutex_unlock(&tmp102->lock);
- return tmp102;
-}
-
-static int tmp102_read_temp(void *dev, int *temp)
-{
- struct tmp102 *tmp102 = tmp102_update_device(dev);
+ ret = regmap_read(tmp102->regmap, TMP102_TEMP_REG, ®);
+ if (ret < 0)
+ return ret;
- *temp = tmp102->temp[0];
+ *temp = tmp102_reg_to_mC(reg);
return 0;
}
@@ -131,9 +106,24 @@ static ssize_t tmp102_show_temp(struct device *dev,
char *buf)
{
struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
- struct tmp102 *tmp102 = tmp102_update_device(dev);
+ struct tmp102 *tmp102 = dev_get_drvdata(dev);
+ int regaddr = sda->index;
+ unsigned int reg;
+ int err;
+
+ /* Is it too early to return a conversion ? */
+ if (regaddr == TMP102_TEMP_REG &&
+ time_before(jiffies, tmp102->ready_time)) {
+ unsigned long sleeptime = tmp102->ready_time - jiffies;
+
+ msleep(jiffies_to_msecs(sleeptime));
+ }
+
+ err = regmap_read(tmp102->regmap, regaddr, ®);
+ if (err < 0)
+ return err;
- return sprintf(buf, "%d\n", tmp102->temp[sda->index]);
+ return sprintf(buf, "%d\n", tmp102_reg_to_mC(reg));
}
static ssize_t tmp102_set_temp(struct device *dev,
@@ -142,29 +132,26 @@ static ssize_t tmp102_set_temp(struct device *dev,
{
struct sensor_device_attribute *sda = to_sensor_dev_attr(attr);
struct tmp102 *tmp102 = dev_get_drvdata(dev);
- struct i2c_client *client = tmp102->client;
+ int reg = sda->index;
long val;
- int status;
+ int err;
if (kstrtol(buf, 10, &val) < 0)
return -EINVAL;
val = clamp_val(val, -256000, 255000);
- mutex_lock(&tmp102->lock);
- tmp102->temp[sda->index] = val;
- status = i2c_smbus_write_word_swapped(client, tmp102_reg[sda->index],
- tmp102_mC_to_reg(val));
- mutex_unlock(&tmp102->lock);
- return status ? : count;
+ err = regmap_write(tmp102->regmap, reg, tmp102_mC_to_reg(val));
+ return err ? : count;
}
-static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, tmp102_show_temp, NULL , 0);
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, tmp102_show_temp, NULL,
+ TMP102_TEMP_REG);
static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO, tmp102_show_temp,
- tmp102_set_temp, 1);
+ tmp102_set_temp, TMP102_TLOW_REG);
static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, tmp102_show_temp,
- tmp102_set_temp, 2);
+ tmp102_set_temp, TMP102_THIGH_REG);
static struct attribute *tmp102_attrs[] = {
&sensor_dev_attr_temp1_input.dev_attr.attr,
@@ -181,19 +168,39 @@ static const struct thermal_zone_of_device_ops tmp102_of_thermal_ops = {
static void tmp102_restore_config(void *data)
{
struct tmp102 *tmp102 = data;
- struct i2c_client *client = tmp102->client;
- i2c_smbus_write_word_swapped(client, TMP102_CONF_REG,
- tmp102->config_orig);
+ regmap_write(tmp102->regmap, TMP102_CONF_REG, tmp102->config_orig);
+}
+
+static bool tmp102_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+ return reg != TMP102_TEMP_REG;
+}
+
+static bool tmp102_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+ return reg == TMP102_TEMP_REG;
}
+static const struct regmap_config tmp102_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 16,
+ .max_register = TMP102_THIGH_REG,
+ .writeable_reg = tmp102_is_writeable_reg,
+ .volatile_reg = tmp102_is_volatile_reg,
+ .val_format_endian = REGMAP_ENDIAN_BIG,
+ .cache_type = REGCACHE_RBTREE,
+ .use_single_rw = true,
+};
+
static int tmp102_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
struct device *dev = &client->dev;
struct device *hwmon_dev;
struct tmp102 *tmp102;
- int status;
+ unsigned int regval;
+ int err;
if (!i2c_check_functionality(client->adapter,
I2C_FUNC_SMBUS_WORD_DATA)) {
@@ -207,35 +214,36 @@ static int tmp102_probe(struct i2c_client *client,
return -ENOMEM;
i2c_set_clientdata(client, tmp102);
- tmp102->client = client;
- status = i2c_smbus_read_word_swapped(client, TMP102_CONF_REG);
- if (status < 0) {
+ tmp102->regmap = devm_regmap_init_i2c(client, &tmp102_regmap_config);
+ if (IS_ERR(tmp102->regmap))
+ return PTR_ERR(tmp102->regmap);
+
+ err = regmap_read(tmp102->regmap, TMP102_CONF_REG, ®val);
+ if (err < 0) {
dev_err(dev, "error reading config register\n");
- return status;
+ return err;
}
- if ((status & ~TMP102_CONFREG_MASK) !=
+ if ((regval & ~TMP102_CONFREG_MASK) !=
(TMP102_CONF_R0 | TMP102_CONF_R1)) {
dev_err(dev, "unexpected config register value\n");
return -ENODEV;
}
- tmp102->config_orig = status;
+ tmp102->config_orig = regval;
devm_add_action(dev, tmp102_restore_config, tmp102);
- status &= ~TMP102_CONFIG_CLEAR;
- status |= TMP102_CONFIG_SET;
+ regval &= ~TMP102_CONFIG_CLEAR;
+ regval |= TMP102_CONFIG_SET;
- status = i2c_smbus_write_word_swapped(client, TMP102_CONF_REG, status);
- if (status < 0) {
+ err = regmap_write(tmp102->regmap, TMP102_CONF_REG, regval);
+ if (err < 0) {
dev_err(dev, "error writing config register\n");
- return status;
+ return err;
}
- mutex_init(&tmp102->lock);
-
tmp102->ready_time = jiffies;
if (tmp102->config_orig & TMP102_CONF_SD) {
/*
@@ -264,30 +272,24 @@ static int tmp102_probe(struct i2c_client *client,
static int tmp102_suspend(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
- int config;
-
- config = i2c_smbus_read_word_swapped(client, TMP102_CONF_REG);
- if (config < 0)
- return config;
+ struct tmp102 *tmp102 = i2c_get_clientdata(client);
- config |= TMP102_CONF_SD;
- return i2c_smbus_write_word_swapped(client, TMP102_CONF_REG, config);
+ return regmap_update_bits(tmp102->regmap, TMP102_CONF_REG,
+ TMP102_CONF_SD, TMP102_CONF_SD);
}
static int tmp102_resume(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
struct tmp102 *tmp102 = i2c_get_clientdata(client);
- int config;
+ int err;
- config = i2c_smbus_read_word_swapped(client, TMP102_CONF_REG);
- if (config < 0)
- return config;
+ err = regmap_update_bits(tmp102->regmap, TMP102_CONF_REG,
+ TMP102_CONF_SD, 0);
tmp102->ready_time = jiffies + msecs_to_jiffies(CONVERSION_TIME_MS);
- config &= ~TMP102_CONF_SD;
- return i2c_smbus_write_word_swapped(client, TMP102_CONF_REG, config);
+ return err;
}
#endif /* CONFIG_PM */
--
2.5.0
^ permalink raw reply related [flat|nested] 11+ messages in thread