[PATCH v3 1/3] hwmon: (tmp102) Improve handling of initial read delay

[PATCH v3 1/3] hwmon: (tmp102) Improve handling of initial read delay

[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.

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

[PATCH v3 2/3] hwmon: (tmp102) Rework chip configuration

[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>
---
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

[PATCH v3 3/3] hwmon: (tmp102) Convert to use regmap, and drop local cache

[Guenter Roeck]

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, &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, &reg);
+	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, &regval);
+	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

Re: [PATCH v3 1/3] hwmon: (tmp102) Improve handling of initial read delay

[Nishanth Menon]

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

Re: [PATCH v3 1/3] hwmon: (tmp102) Improve handling of initial read delay

[Guenter Roeck]

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