From patchwork Tue Dec 1 14:21:53 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nishanth Menon X-Patchwork-Id: 57493 Delivered-To: patch@linaro.org Received: by 10.112.155.196 with SMTP id vy4csp2205622lbb; Tue, 1 Dec 2015 06:23:27 -0800 (PST) X-Received: by 10.98.72.134 with SMTP id q6mr80478095pfi.56.1448979806948; Tue, 01 Dec 2015 06:23:26 -0800 (PST) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id es4si19836175pac.153.2015.12.01.06.23.25; Tue, 01 Dec 2015 06:23:26 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756022AbbLAOXY (ORCPT + 28 others); Tue, 1 Dec 2015 09:23:24 -0500 Received: from arroyo.ext.ti.com ([192.94.94.40]:44477 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755220AbbLAOXW (ORCPT ); Tue, 1 Dec 2015 09:23:22 -0500 Received: from dflxv15.itg.ti.com ([128.247.5.124]) by arroyo.ext.ti.com (8.13.7/8.13.7) with ESMTP id tB1ELshU022107; Tue, 1 Dec 2015 08:21:54 -0600 Received: from DLEE71.ent.ti.com (dlee71.ent.ti.com [157.170.170.114]) by dflxv15.itg.ti.com (8.14.3/8.13.8) with ESMTP id tB1ELrPl027872; Tue, 1 Dec 2015 08:21:53 -0600 Received: from dlep32.itg.ti.com (157.170.170.100) by DLEE71.ent.ti.com (157.170.170.114) with Microsoft SMTP Server id 14.3.224.2; Tue, 1 Dec 2015 08:21:53 -0600 Received: from localhost (ileax41-snat.itg.ti.com [10.172.224.153]) by dlep32.itg.ti.com (8.14.3/8.13.8) with ESMTP id tB1ELrRv018403; Tue, 1 Dec 2015 08:21:53 -0600 Date: Tue, 1 Dec 2015 08:21:53 -0600 From: Nishanth Menon To: Guenter Roeck , Jean Delvare CC: , , , , Eduardo Valentin Subject: Re: [PATCH] hwmon: (tmp102) Force wait for conversion time for the first valid data Message-ID: <20151201142152.GA10891@ogun.home> References: <1448943955-2385-1-git-send-email-nm@ti.com> <565D3513.7050905@roeck-us.net> <565DA504.4060600@ti.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <565DA504.4060600@ti.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07:47-20151201, Nishanth Menon wrote: > Hi Guenter, > > Thanks for the detailed review.. > > On 11/30/2015 11:50 PM, Guenter Roeck wrote: > > On 11/30/2015 08:25 PM, Nishanth Menon wrote: > [...] > > >> > >> A simpler alternative approach could be to sleep in the probe for the > >> duration required, but that will result in latency that is undesirable > >> that can delay boot sequence un-necessarily. > >> > > A really simpler solution would be to mark when the device is ready > > to be accessed in the probe function, and go to sleep for the remaining > > time > > in the update function if necessary. This would not affect the probe > > function, > > avoid the somewhat awkward -EAGAIN, avoid overloading the value cache, > > and only > > sleep if necessary and as long as needed. > > We already have that logic in a different form: > We use last_update to know when to go read the temperature value. Until > the conversion time has elapsed, we keep providing previously cached > value. Trouble is the first time read before conversion time is complete: > > On sleep during update: > unfortunately, forcing the delay in update for the first time: > a) Will also cause the latency in the thermal_zone_device_check which > triggers right after tmp102_probe->thermal_zone_of_sensor_register > b) -EAGAIN is used by other hwmon drivers such as > drivers/hwmon/adt7470.c, drivers/hwmon/ltc4245.c, drivers/hwmon/sht15.c, > drivers/hwmon/tc74.c, drivers/hwmon/via-cputemp.c in similar ways when > data cannot be provided back. > > Overriding the temp value to indicate first time read: > I can setup a bool in struct tmp102 instead -> but that serves the same > purpose as what we did with override, except increase 1 char footprint - > though I agree, it might be a little more readable. > > > > >> [1] http://www.ti.com/lit/ds/symlink/tmp102.pdf > >> > >> Cc: Eduardo Valentin > >> Reported-by: Aparna Balasubramanian > >> Reported-by: Elvita Lobo > >> Reported-by: Yan Liu > >> Signed-off-by: Nishanth Menon > >> --- > >> > >> Example case (from Beagleboard-x15 using an older kernel revision): > >> http://pastebin.ubuntu.com/13591711/ > >> Notice the thermal shutdown trigger: > >> thermal thermal_zone3: critical temperature reached(108 > >> C),shutting down > >> > >> drivers/hwmon/tmp102.c | 19 ++++++++++++++++++- > >> 1 file changed, 18 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c > >> index 65482624ea2c..145f69108f23 100644 > >> --- a/drivers/hwmon/tmp102.c > >> +++ b/drivers/hwmon/tmp102.c > >> @@ -50,6 +50,9 @@ > >> #define TMP102_TLOW_REG 0x02 > >> #define TMP102_THIGH_REG 0x03 > >> > >> +/* TMP102 range is -55 to 150C -> we use -128 as a default invalid > >> value */ > >> +#define TMP102_NOTREADY -128 > >> + > > > > This is a bit misleading, and also not correct, since the temperature is > > stored in > > milli-degrees C, so a value of -128 reflects -0.128 degreees C. While > > that value > > will not be seen in practice, it is still not a good idea to use it for > > this purpose. > > > > Even though the chip temperature range is -55 .. 150 C, that doesn't mean > > it never returns a value outside that range, for example if nothing is > > connected > > to an external sensor or if something is broken. > > > > You should use a value outside the value range, ie outside > > [-128,000 .. 127,999 ] to detect the "not ready" condition. > > > That is true.. I will just drop this and introduce a bool in tmp102 instead. > > >> struct tmp102 { > >> struct i2c_client *client; > >> struct device *hwmon_dev; > >> @@ -102,6 +105,12 @@ 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->temp[0] == TMP102_NOTREADY) { > >> + dev_dbg(dev, "%s: Conversion not ready yet..\n", __func__); > >> + return -EAGAIN; > > > > Does this cause a hard loop in the calling code, or will the thermal code > > delay before it reads again ? > > > > If it causes a hard loop, it may be better to go to sleep if needed > > when reading the data, as suggested above. > > Thermal framework is capable of handling -EAGAIN without a hard loop > around this (it just seems to reschedule around the polling interval and > comes back to check if data is ready). > > If you are ok with the above, then I will send a v2 introducing a bool > to setup a flag for first_time read, but will leave the -EAGAIN alone. Hint about how the patch will look like: -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c index 65482624ea2c..5289aa0980a8 100644 --- a/drivers/hwmon/tmp102.c +++ b/drivers/hwmon/tmp102.c @@ -58,6 +58,7 @@ struct tmp102 { u16 config_orig; unsigned long last_update; int temp[3]; + bool first_time; }; /* convert left adjusted 13-bit TMP102 register value to milliCelsius */ @@ -93,6 +94,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; } mutex_unlock(&tmp102->lock); return tmp102; @@ -102,6 +104,12 @@ 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; @@ -114,6 +122,10 @@ 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]); } @@ -207,7 +219,9 @@ static int tmp102_probe(struct i2c_client *client, status = -ENODEV; goto fail_restore_config; } - tmp102->last_update = jiffies - HZ; + tmp102->last_update = jiffies; + /* Mark that we are not ready with data until conversion is complete */ + tmp102->first_time = true; mutex_init(&tmp102->lock); hwmon_dev = hwmon_device_register_with_groups(dev, client->name,