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. 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> --- v3: Don't go to sleep if sensor readings are not available; instead, return -EAGAIN (matching previous behavior) tmp102_update_device() is now a void function. v2: No change Note: This series depends on the following two patches in -next. hwmon: (tmp102) Drop FSF address hwmon: (tmp102) Use devm_add_action to register cleanup function drivers/hwmon/tmp102.c | 44 ++++++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c index 5bdf262e6a0e..fd4a4515692a 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 */ @@ -73,13 +77,14 @@ static const u8 tmp102_reg[] = { TMP102_THIGH_REG, }; -static struct tmp102 *tmp102_update_device(struct device *dev) +static void tmp102_update_device(struct device *dev) { struct tmp102 *tmp102 = dev_get_drvdata(dev); struct i2c_client *client = tmp102->client; 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,22 +93,22 @@ 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; } static int tmp102_read_temp(void *dev, int *temp) { - struct tmp102 *tmp102 = tmp102_update_device(dev); + struct tmp102 *tmp102 = dev_get_drvdata(dev); - /* Is it too early even to return a conversion? */ - if (tmp102->first_time) { + if (time_before(jiffies, tmp102->ready_time)) { dev_dbg(dev, "%s: Conversion not ready yet..\n", __func__); return -EAGAIN; } + tmp102_update_device(dev); + *temp = tmp102->temp[0]; return 0; @@ -114,12 +119,13 @@ 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); - /* Is it too early even to return a read? */ - if (tmp102->first_time) + if (time_before(jiffies, tmp102->ready_time)) return -EAGAIN; + tmp102_update_device(dev); + return sprintf(buf, "%d\n", tmp102->temp[sda->index]); } @@ -224,11 +230,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 +274,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
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> --- v3: No change 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 fd4a4515692a..82a8a29af2e4 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 { @@ -167,9 +178,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, }; @@ -210,26 +218,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
By converting 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. A positive side effect of this change is that limit registers can now be read and updated before the first temperature conversion is complete. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- v3: Add missing 'select REGMAP_I2C' Fix typo in description Adjust to changes in patch 1/3 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. drivers/hwmon/Kconfig | 1 + drivers/hwmon/tmp102.c | 148 +++++++++++++++++++++++-------------------------- 2 files changed, 71 insertions(+), 78 deletions(-) diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index ff940075bb90..731378853eea 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1538,6 +1538,7 @@ config SENSORS_TMP102 tristate "Texas Instruments TMP102" depends on I2C depends on THERMAL || !THERMAL_OF + select REGMAP_I2C help If you say yes here you get support for Texas Instruments TMP102 sensor chips. diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c index 82a8a29af2e4..a942a2574a4d 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,45 +79,22 @@ 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 void tmp102_update_device(struct device *dev) -{ - struct tmp102 *tmp102 = dev_get_drvdata(dev); - struct i2c_client *client = tmp102->client; - - 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); -} - static int tmp102_read_temp(void *dev, int *temp) { struct tmp102 *tmp102 = dev_get_drvdata(dev); + unsigned int reg; + int ret; if (time_before(jiffies, tmp102->ready_time)) { dev_dbg(dev, "%s: Conversion not ready yet..\n", __func__); return -EAGAIN; } - 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,13 +105,19 @@ static ssize_t tmp102_show_temp(struct device *dev, { struct sensor_device_attribute *sda = to_sensor_dev_attr(attr); struct tmp102 *tmp102 = dev_get_drvdata(dev); + int regaddr = sda->index; + unsigned int reg; + int err; - if (time_before(jiffies, tmp102->ready_time)) + if (regaddr == TMP102_TEMP_REG && + time_before(jiffies, tmp102->ready_time)) return -EAGAIN; - tmp102_update_device(dev); + 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, @@ -146,29 +126,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, @@ -185,19 +162,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)) { @@ -211,35 +208,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) { /* @@ -268,30 +266,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
On 06/29/2016 10:41 PM, Guenter Roeck wrote:
> 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.
>
> 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>
> ---
> v3: Don't go to sleep if sensor readings are not available;
> instead, return -EAGAIN (matching previous behavior)
> tmp102_update_device() is now a void function.
> v2: No change
>
> Note: This series depends on the following two patches in -next.
a little too late perhaps.. Apologies, was on vacation.. tested and
for the series:
Acked-by: Nishanth Menon <nm@ti.com>
--
Regards,
Nishanth Menon
On Tue, Jul 12, 2016 at 03:53:17PM -0500, Nishanth Menon wrote:
> On 06/29/2016 10:41 PM, Guenter Roeck wrote:
> >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.
> >
> >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>
> >---
> >v3: Don't go to sleep if sensor readings are not available;
> > instead, return -EAGAIN (matching previous behavior)
> > tmp102_update_device() is now a void function.
> >v2: No change
> >
> >Note: This series depends on the following two patches in -next.
>
>
> a little too late perhaps.. Apologies, was on vacation.. tested and for the
> series:
>
> Acked-by: Nishanth Menon <nm@ti.com>
>
Thanks!
Guenter