Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1431302 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 3/5] hwmon: (tmp102) Improve handling of initial read delay Guenter Roeck <linux@roeck-us.net> - 2016-06-26 04:50 +0200
    Re: [PATCH v2 3/5] hwmon: (tmp102) Improve handling of initial read  delay Nishanth Menon <nm@ti.com> - 2016-06-26 15:40 +0200
      Re: [PATCH v2 3/5] hwmon: (tmp102) Improve handling of initial read  delay Guenter Roeck <linux@roeck-us.net> - 2016-06-26 16:10 +0200

#1431302 — [PATCH v2 3/5] hwmon: (tmp102) Improve handling of initial read delay

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

[toc] | [next] | [standalone]

#1431501 — Re: [PATCH v2 3/5] hwmon: (tmp102) Improve handling of initial read delay

On 06/25/2016 09:40 PM, Guenter Roeck wrote:
[...]
>  /* 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));
> +	}
> +

While msleep can indeed work and simplify, in case of usage for
example with thermal framework, if the data is not ready and we return
-EAGAIN, it lets the thermal framework go and read other sensors
instead of being blocked on the tmp102 conversion of data.

Eduardo, Rui: what is your view on this approach?
Patch: https://patchwork.kernel.org/patch/9198961/

-- 
Regards,
Nishanth Menon

[toc] | [prev] | [next] | [standalone]

#1431513 — Re: [PATCH v2 3/5] hwmon: (tmp102) Improve handling of initial read delay

On 06/26/2016 06:31 AM, Nishanth Menon wrote:
> On 06/25/2016 09:40 PM, Guenter Roeck wrote:
> [...]
>>   /* 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));
>> +	}
>> +
>
> While msleep can indeed work and simplify, in case of usage for
> example with thermal framework, if the data is not ready and we return
> -EAGAIN, it lets the thermal framework go and read other sensors
> instead of being blocked on the tmp102 conversion of data.
>

My thought was that returning -EAGAIN was more appropriate for the
1/3s wait time we had earlier. With 35ms, it is much more questionable
if we hit this condition in the first place than it was before,
and the impact if waiting is much less severe. Furthermore,
it is no longer unconditional but only occurs if the sensor
was in fact disabled.

But, yes, you have a point. I do prefer to wait, but if Rui and you
both agree that we should return -EAGAIN, I'll be happy to change
the code back to the old behavior.

Thanks,
Guenter

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web