[lm-sensors] [PATCH 3/3] hwmon: (i5500_temp) Don't bind to disabled sensors

[lm-sensors] [PATCH 3/3] hwmon: (i5500_temp) Don't bind to disabled sensors

[Jean Delvare]

On many motherboards, for an unknown reason, the thermal sensor seems
to be disabled and will return a constant temperature value of 36.5
degrees Celsius. Don't bind to the device in that case, so that we
don't report this bogus value to userspace.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Romain Dolbeau <romain@dolbeau.org>
Cc: Guenter Roeck <linux@roeck-us.net>
---
As suggested by Guenter.

 drivers/hwmon/i5500_temp.c |    7 +++++++
 1 file changed, 7 insertions(+)

--- linux-3.17.orig/drivers/hwmon/i5500_temp.c	2014-10-22 10:22:18.526856485 +0200
+++ linux-3.17/drivers/hwmon/i5500_temp.c	2014-10-22 10:55:22.700413828 +0200
@@ -121,6 +121,7 @@ static int i5500_temp_probe(struct pci_d
 {
 	int err;
 	struct device *hwmon_dev;
+	s8 tsfsc;
 
 	err = pci_enable_device(pdev);
 	if (err) {
@@ -128,6 +129,12 @@ static int i5500_temp_probe(struct pci_d
 		return err;
 	}
 
+	pci_read_config_byte(pdev, REG_TSFSC, &tsfsc);
+	if (tsfsc = 0x7F) {
+		dev_warn(&pdev->dev, "Sensor seems to be disabled\n");
+		return -ENODEV;
+	}
+
 	hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
 							   "intel5500", NULL,
 							   i5500_temp_groups);


-- 
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 3/3] hwmon: (i5500_temp) Don't bind to disabled sensors

[Romain Dolbeau]

MjAxNC0xMC0yMiAxMToxMyBHTVQrMDI6MDAgSmVhbiBEZWx2YXJlIDxqZGVsdmFyZUBzdXNlLmRl
PjoKPiBPbiBtYW55IG1vdGhlcmJvYXJkcywgZm9yIGFuIHVua25vd24gcmVhc29uLCB0aGUgdGhl
cm1hbCBzZW5zb3Igc2VlbXMKPiB0byBiZSBkaXNhYmxlZCBhbmQgd2lsbCByZXR1cm4gYSBjb25z
dGFudCB0ZW1wZXJhdHVyZSB2YWx1ZSBvZiAzNi41Cj4gZGVncmVlcyBDZWxzaXVzLiBEb24ndCBi
aW5kIHRvIHRoZSBkZXZpY2UgaW4gdGhhdCBjYXNlLCBzbyB0aGF0IHdlCj4gZG9uJ3QgcmVwb3J0
IHRoaXMgYm9ndXMgdmFsdWUgdG8gdXNlcnNwYWNlLgoKMzYuNcKwQyBtaWdodCBiZSBhIGxlZ2l0
aW1hdGUgdmFsdWUgLSBmb3IgaW5zdGFuY2UsIGlmIHRoZSBkcml2ZXIgaXMgbG9hZGVkCmFmdGVy
IGEgY29sZCBib290IGluIGEgdGVtcGVyYXR1cmUtY29udHJvbGxlZCBtYWNoaW5lIHJvb20sIHRo
ZSBwcm9wZXJseQpjb29sZWQgY2hpcHNldCBtaWdodCBub3QgaGF2ZSByZWFjaGVkIDM3wrBDIHll
dCwgc28gdGhlIGNoaXAgd2lsbCBsZWdpdGltYXRlbHkKcmVwb3J0IDB4N0YgaW4gVFNGU0MuLi4K
CkkgX3RoaW5rXyBhIHNhZmVyIHdheSB3b3VsZCBiZSB0byBjaGVjayB2YXJpYWJpbGl0eSAtIGlm
IGl0J3Mgbm90IDB4N0YKdGhlbiBmaW5lLCBpdCdzIGxpa2VseSB3b3JraW5nLiBJZiBpcyBpcywg
dGhlbiBkcm9wIFRTVEhSVEhJIGJ5IH4zMMKwQyA7CmlmIFRTRlNDIHN0aWxsIHJlcG9ydHMgMHg3
RiB0aGVuIGl0J3MgZGVmaW5pdGVseSB3cm9uZyBzaW5jZSB0aGF0IHdvdWxkCmJlIDYuNcKwQyAo
bWFjaGluZSByb29tIGluIHRoZSBbYW50XWFyY3RpYyB3aGVyZSB0aGF0IHdvdWxkIHN0aWxsIGJl
IGxlZ2l0CnNob3VsZG4ndCB3b3JyeSBhYm91dCBjaGlwc2V0IHRlbXBlcmF0dXJlIGFueXdheSA6
LSkKCk9UT0gsIGp1c3QgbG9hZGluZyB0aGUgZHJpdmVyIGEgY291cGxlIG9mIG1pbnV0ZXMgYWZ0
ZXIgYm9vdCB3b3VsZCB3b3JrCndpdGggeW91ciBjdXJyZW50IHRlc3QuCgpDb3JkaWFsbHksCgot
LSAKUm9tYWluIERvbGJlYXUKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f
X19fX19fX19fCmxtLXNlbnNvcnMgbWFpbGluZyBsaXN0CmxtLXNlbnNvcnNAbG0tc2Vuc29ycy5v
cmcKaHR0cDovL2xpc3RzLmxtLXNlbnNvcnMub3JnL21haWxtYW4vbGlzdGluZm8vbG0tc2Vuc29y
cw=

Re: [lm-sensors] [PATCH 3/3] hwmon: (i5500_temp) Don't bind to disabled sensors

[Guenter Roeck]

On Wed, Oct 22, 2014 at 11:13:21AM +0200, Jean Delvare wrote:
> On many motherboards, for an unknown reason, the thermal sensor seems
> to be disabled and will return a constant temperature value of 36.5
> degrees Celsius. Don't bind to the device in that case, so that we
> don't report this bogus value to userspace.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Romain Dolbeau <romain@dolbeau.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> ---
> As suggested by Guenter.
> 
>  drivers/hwmon/i5500_temp.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> --- linux-3.17.orig/drivers/hwmon/i5500_temp.c	2014-10-22 10:22:18.526856485 +0200
> +++ linux-3.17/drivers/hwmon/i5500_temp.c	2014-10-22 10:55:22.700413828 +0200
> @@ -121,6 +121,7 @@ static int i5500_temp_probe(struct pci_d
>  {
>  	int err;
>  	struct device *hwmon_dev;
> +	s8 tsfsc;
>  
>  	err = pci_enable_device(pdev);
>  	if (err) {
> @@ -128,6 +129,12 @@ static int i5500_temp_probe(struct pci_d
>  		return err;
>  	}
>  
> +	pci_read_config_byte(pdev, REG_TSFSC, &tsfsc);
> +	if (tsfsc = 0x7F) {
> +		dev_warn(&pdev->dev, "Sensor seems to be disabled\n");
> +		return -ENODEV;
> +	}
> +
Is there really no other register you can use to detect if the sensor is enabled ?
How about the TSTIMER register ?

Thanks,
Guenter

>  	hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
>  							   "intel5500", NULL,
>  							   i5500_temp_groups);
> 
> 
> -- 
> Jean Delvare
> SUSE L3 Support

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 3/3] hwmon: (i5500_temp) Don't bind to disabled sensors

[Romain Dolbeau]

2014-10-22 18:26 GMT+02:00 Guenter Roeck <linux@roeck-us.net>:
> Is there really no other register you can use to detect if the sensor is enabled ?
> How about the TSTIMER register ?

As far as I can tell (but I'm not an expert), all the documented
registers display "sane" values, so it's hard to tell apart working
and non-working systems. I tried copying the whole register
space from the working to the non-working system, and it didn't
help one bit. Seems there's some undocumented initialization
missing (or the sensor is just not there).

So among the possibilities I see

1) Jean's solution, which I think is the best one. Simply reject
if TSFSC=0x7F and document why.  In the odd case where a
cold system won't load, the admin can always load it on a
warm (literally) system (@reboot in crontab).

2) the variability solution. perhaps a tiny bit more reliable, but
much more complicated (and how much time does it take
of TSFSC to react to change in TSTHRHI anyway ?)

3) depends on some undocumented behavior, such as the
byte @ 0xE4 seemingly containing (TSTHRTHI-TSFSC).

4) the TSTHRCATA register is write-once, and (apparently)
already written on the "working" system but not on the
"non-working" system. But that's on a sample of 1 system
of either kind, so again not very safe.

I think 1) is still the best solution, because it's simple
and has no side-effect other than not loading in an hypothetical
scenario unlikely to happen. 5500-based system are 3 to 4
generations back, it's unlikely chipset temperature is going
to be a critical factor where the driver just _must_ load :-)

Cordially,

-- 
Romain Dolbeau

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 3/3] hwmon: (i5500_temp) Don't bind to disabled sensors

[Guenter Roeck]

Hi Romain,

On Wed, Oct 22, 2014 at 07:54:49PM +0200, Romain Dolbeau wrote:
> 2014-10-22 18:26 GMT+02:00 Guenter Roeck <linux@roeck-us.net>:
> > Is there really no other register you can use to detect if the sensor is enabled ?
> > How about the TSTIMER register ?
> 
> As far as I can tell (but I'm not an expert), all the documented
> registers display "sane" values, so it's hard to tell apart working

Too bad. I thought that maybe the TSTIMER would have its poweron
value of 0, though the datasheet is a bit confusing in that regard.
It seems to state that the default value is 0 and 200,000 at the
same time.

Maybe it is as simple as VCCTS or TSIREF not connected on the
'failing' board.

Did you check for 'thermal error enable' in the GERRCTL register ?

> and non-working systems. I tried copying the whole register
> space from the working to the non-working system, and it didn't
> help one bit. Seems there's some undocumented initialization
> missing (or the sensor is just not there).
> 
> So among the possibilities I see
> 
> 1) Jean's solution, which I think is the best one. Simply reject
> if TSFSC=0x7F and document why.  In the odd case where a
> cold system won't load, the admin can always load it on a
> warm (literally) system (@reboot in crontab).
> 
> 2) the variability solution. perhaps a tiny bit more reliable, but
> much more complicated (and how much time does it take
> of TSFSC to react to change in TSTHRHI anyway ?)
> 
> 3) depends on some undocumented behavior, such as the
> byte @ 0xE4 seemingly containing (TSTHRTHI-TSFSC).
> 
> 4) the TSTHRCATA register is write-once, and (apparently)
> already written on the "working" system but not on the
> "non-working" system. But that's on a sample of 1 system
> of either kind, so again not very safe.
> 
> I think 1) is still the best solution, because it's simple
> and has no side-effect other than not loading in an hypothetical
> scenario unlikely to happen. 5500-based system are 3 to 4
> generations back, it's unlikely chipset temperature is going
> to be a critical factor where the driver just _must_ load :-)
> 
I thought you were suggesting solution 2) earlier ?

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 3/3] hwmon: (i5500_temp) Don't bind to disabled sensors

[Guenter Roeck]

On 10/22/2014 02:13 AM, Jean Delvare wrote:
> On many motherboards, for an unknown reason, the thermal sensor seems
> to be disabled and will return a constant temperature value of 36.5
> degrees Celsius. Don't bind to the device in that case, so that we
> don't report this bogus value to userspace.
>
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Romain Dolbeau <romain@dolbeau.org>
> Cc: Guenter Roeck <linux@roeck-us.net>

Assuming there is no simple better way:

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> ---
> As suggested by Guenter.
>
>   drivers/hwmon/i5500_temp.c |    7 +++++++
>   1 file changed, 7 insertions(+)
>
> --- linux-3.17.orig/drivers/hwmon/i5500_temp.c	2014-10-22 10:22:18.526856485 +0200
> +++ linux-3.17/drivers/hwmon/i5500_temp.c	2014-10-22 10:55:22.700413828 +0200
> @@ -121,6 +121,7 @@ static int i5500_temp_probe(struct pci_d
>   {
>   	int err;
>   	struct device *hwmon_dev;
> +	s8 tsfsc;
>
>   	err = pci_enable_device(pdev);
>   	if (err) {
> @@ -128,6 +129,12 @@ static int i5500_temp_probe(struct pci_d
>   		return err;
>   	}
>
> +	pci_read_config_byte(pdev, REG_TSFSC, &tsfsc);
> +	if (tsfsc = 0x7F) {
> +		dev_warn(&pdev->dev, "Sensor seems to be disabled\n");
> +		return -ENODEV;
> +	}
> +
>   	hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
>   							   "intel5500", NULL,
>   							   i5500_temp_groups);
>
>


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 3/3] hwmon: (i5500_temp) Don't bind to disabled sensors

[Romain Dolbeau]

2014-10-23 0:53 GMT+02:00 Guenter Roeck <linux@roeck-us.net>:
> I thought you were suggesting solution 2) earlier ?

Merely mentioning a possible alternative solution to a very
minor potential flaw. And if someone encounters a
load-at-bootime issue, s/he will be able to find why by
a google search :-)

Cordially,

-- 
Romain Dolbeau

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 3/3] hwmon: (i5500_temp) Don't bind to disabled sensors

[Jean Delvare]

SGkgUm9tYWluLAoKT24gV2VkLCAyMiBPY3QgMjAxNCAxMToyNTowMyArMDIwMCwgUm9tYWluIERv
bGJlYXUgd3JvdGU6Cj4gMjAxNC0xMC0yMiAxMToxMyBHTVQrMDI6MDAgSmVhbiBEZWx2YXJlIDxq
ZGVsdmFyZUBzdXNlLmRlPjoKPiA+IE9uIG1hbnkgbW90aGVyYm9hcmRzLCBmb3IgYW4gdW5rbm93
biByZWFzb24sIHRoZSB0aGVybWFsIHNlbnNvciBzZWVtcwo+ID4gdG8gYmUgZGlzYWJsZWQgYW5k
IHdpbGwgcmV0dXJuIGEgY29uc3RhbnQgdGVtcGVyYXR1cmUgdmFsdWUgb2YgMzYuNQo+ID4gZGVn
cmVlcyBDZWxzaXVzLiBEb24ndCBiaW5kIHRvIHRoZSBkZXZpY2UgaW4gdGhhdCBjYXNlLCBzbyB0
aGF0IHdlCj4gPiBkb24ndCByZXBvcnQgdGhpcyBib2d1cyB2YWx1ZSB0byB1c2Vyc3BhY2UuCj4g
Cj4gMzYuNcKwQyBtaWdodCBiZSBhIGxlZ2l0aW1hdGUgdmFsdWUgLSBmb3IgaW5zdGFuY2UsIGlm
IHRoZSBkcml2ZXIgaXMgbG9hZGVkCj4gYWZ0ZXIgYSBjb2xkIGJvb3QgaW4gYSB0ZW1wZXJhdHVy
ZS1jb250cm9sbGVkIG1hY2hpbmUgcm9vbSwgdGhlIHByb3Blcmx5Cj4gY29vbGVkIGNoaXBzZXQg
bWlnaHQgbm90IGhhdmUgcmVhY2hlZCAzN8KwQyB5ZXQsIHNvIHRoZSBjaGlwIHdpbGwgbGVnaXRp
bWF0ZWx5Cj4gcmVwb3J0IDB4N0YgaW4gVFNGU0MuLi4KCkkgd29uZGVyIGlmIHRoaXMgY2FuIHJl
YWxseSBoYXBwZW4uIEluIG15IGV4cGVyaWVuY2UgdGhlc2UgY2hpcHNldHMKZ2VuZXJhdGUgYSBs
b3Qgb2YgaGVhdC4gT24gbXkgYm9hcmQsIHRoZSBvcmlnaW5hbCBoZWF0c2luayB3YXMgbm90CnN1
aXRlZCBmb3IgdGhlIGNhc2UgZm9ybSBmYWN0b3IgYW5kIGl0IHdvdWxkIGlkbGUgYXQgODDCsEMu
IFVuZGVyIGxvYWQKaXQgY291bGQgcmVhY2ggMTAwwrBDIFsxXS4KClsxXSBodHRwOi8vamRlbHZh
cmUubmVyaW0ubmV0L2FydGljbGVzL2k1NTAwLWhlYXRzaW5rLXR1bmluZy8KCkkgY291bGQgaW5z
dHJ1bWVudCB0aGUgZHJpdmVyIHRvIGxvZyB0aGUgdGVtcGVyYXR1cmUgdmFsdWUgd2hlbiB0aGUK
ZHJpdmVyIGlzIGxvYWRlZC4KCj4gSSBfdGhpbmtfIGEgc2FmZXIgd2F5IHdvdWxkIGJlIHRvIGNo
ZWNrIHZhcmlhYmlsaXR5IC0gaWYgaXQncyBub3QgMHg3Rgo+IHRoZW4gZmluZSwgaXQncyBsaWtl
bHkgd29ya2luZy4gSWYgaXMgaXMsIHRoZW4gZHJvcCBUU1RIUlRISSBieSB+MzDCsEMgOwo+IGlm
IFRTRlNDIHN0aWxsIHJlcG9ydHMgMHg3RiB0aGVuIGl0J3MgZGVmaW5pdGVseSB3cm9uZyBzaW5j
ZSB0aGF0IHdvdWxkCj4gYmUgNi41wrBDIChtYWNoaW5lIHJvb20gaW4gdGhlIFthbnRdYXJjdGlj
IHdoZXJlIHRoYXQgd291bGQgc3RpbGwgYmUgbGVnaXQKPiBzaG91bGRuJ3Qgd29ycnkgYWJvdXQg
Y2hpcHNldCB0ZW1wZXJhdHVyZSBhbnl3YXkgOi0pCgpJIGFtIHJlbHVjdGFudCB0byBjaGFuZ2Ug
dGhlIHNldHRpbmdzIGF0IGRyaXZlciBsb2FkIHRpbWUsIGV2ZW4gaWYgd2UKYXR0ZW1wdCB0byBy
ZXZlcnQgc2hvcnRseSBhZnRlciB0aGF0LiBJdCBjb3VsZCBlYXNpbHkgY2F1c2UgYSBnbGl0Y2gu
CkZvciBleGFtcGxlIHdlIGhhdmUgbm8gaWRlYSBob3cgdGhlIGNoaXAgcmVhY3RzIGlmIHRoZSBo
aWdoIHRlbXBlcmF0dXJlCmxpbWl0IGlzIGxvd2VyIHRoYW4gdGhlICJsb3ciIHRlbXBlcmF0dXJl
IGxpbWl0LgoKLS0gCkplYW4gRGVsdmFyZQpTVVNFIEwzIFN1cHBvcnQKCl9fX19fX19fX19fX19f
X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmxtLXNlbnNvcnMgbWFpbGluZyBsaXN0
CmxtLXNlbnNvcnNAbG0tc2Vuc29ycy5vcmcKaHR0cDovL2xpc3RzLmxtLXNlbnNvcnMub3JnL21h
aWxtYW4vbGlzdGluZm8vbG0tc2Vuc29ycw=

Re: [lm-sensors] [PATCH 3/3] hwmon: (i5500_temp) Don't bind to disabled sensors

[Romain Dolbeau]

MjAxNC0xMC0yMyAxMDowMyBHTVQrMDI6MDAgSmVhbiBEZWx2YXJlIDxqZGVsdmFyZUBzdXNlLmRl
PjoKPiBJIHdvbmRlciBpZiB0aGlzIGNhbiByZWFsbHkgaGFwcGVuLiBJbiBteSBleHBlcmllbmNl
IHRoZXNlIGNoaXBzZXRzCj4gZ2VuZXJhdGUgYSBsb3Qgb2YgaGVhdC4gT24gbXkgYm9hcmQsIHRo
ZSBvcmlnaW5hbCBoZWF0c2luayB3YXMgbm90Cj4gc3VpdGVkIGZvciB0aGUgY2FzZSBmb3JtIGZh
Y3RvciBhbmQgaXQgd291bGQgaWRsZSBhdCA4MMKwQy4gVW5kZXIgbG9hZAo+IGl0IGNvdWxkIHJl
YWNoIDEwMMKwQyBbMV0uCgpNeSBib2FyZCBzZWVtcyB0byBoYXZlIGEgbXVjaCBiZXR0ZXIgaGVh
dHNpbmsgKGJ1dCBpcyBwcm9iYWJseQpvdmVyY29vbGVkIGF0IHRoZSBtb21lbnQ6IHRoZSBwdXNo
LXB1bGwgZmFucyBhcmUgdmVyeSBub2lzeSBhbmQKd2VyZSBkZXNpZ25lZCB0byBtb3ZlIGFpciBh
cm91bmQgYSBwYWlyIG9mIDMwMFcgUENJZSBjYXJkcykuCkF0IHJlc3QgaXQgaXMgYXQgYWJvdXQg
NTAtNTXCsEMsIGFuZCBhZnRlciBzZXZlcmFsIGhvdXIgYXQgZnVsbCBDUFUKbG9hZCBpdCBkaWRu
J3QgZ28gb3ZlciA3MsKwQy4gTW92aW5nIGEgbG90IG9mIGRhdGEgYXJvdW5kIHdpdGggQ1VEQQp0
byBhIEdUWDQ2MCBkaWRuJ3Qgc2VlbSB0byBwdXNoIHRoZSB0ZW1wZXJhdHVyZSBoaWdoZXIgdGhh
bgp0aGF0LCBlaXRoZXIuCgpBbnl3YXkgYXMgbWVudGlvbmVkIGluIGFub3RoZXIgbWVzc2FnZSAt
IEkgdGhpbmsgeW91ciBjdXJyZW50CnNvbHV0aW9uIGlzIGZpbmUuIEkganVzdCB3YW50ZWQgdGhl
IHBvdGVudGlhbCBjYXZlYXQgdG8gYmUgb3V0IHRoZXJlCnNvIGl0IGNhbiBiZSBnb29nbGVkIGJ5
IGludGVyZXN0ZWQgdGhpcmQtcGFydGllcy4KCkNvcmRpYWxseSwKCi0tIApSb21haW4gRG9sYmVh
dQoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KbG0tc2Vu
c29ycyBtYWlsaW5nIGxpc3QKbG0tc2Vuc29yc0BsbS1zZW5zb3JzLm9yZwpodHRwOi8vbGlzdHMu
bG0tc2Vuc29ycy5vcmcvbWFpbG1hbi9saXN0aW5mby9sbS1zZW5zb3Jz

Re: [lm-sensors] [PATCH 3/3] hwmon: (i5500_temp) Don't bind to disabled sensors

[Jean Delvare]

Hi Guenter,

On Wed, 22 Oct 2014 15:53:35 -0700, Guenter Roeck wrote:
> On Wed, Oct 22, 2014 at 07:54:49PM +0200, Romain Dolbeau wrote:
> > 2014-10-22 18:26 GMT+02:00 Guenter Roeck <linux@roeck-us.net>:
> > > Is there really no other register you can use to detect if the sensor is enabled ?
> > > How about the TSTIMER register ?
> > 
> > As far as I can tell (but I'm not an expert), all the documented
> > registers display "sane" values, so it's hard to tell apart working
> 
> Too bad. I thought that maybe the TSTIMER would have its poweron
> value of 0, though the datasheet is a bit confusing in that regard.
> It seems to state that the default value is 0 and 200,000 at the
> same time.

Value of this register is 0x07d30d40 on three boards where the driver
doesn't work. This means PRESCALER = 0x30d40 = 200000 and FILTER 0x07d = 125, as indicated in the datasheet. On the two boards where the
driver does work, the FILTER field has the same value, however the
PRESCALER has a different value (257600 on one, 257201 on the other.)

So we could at least improve the heuristic and only fail the driver
binding if TSFSC = 0x7F _and_ TSTIMER = 0x07D30D40. If someone changed
the prescaler or filter values, there are good chances that the driver
should work.

> Maybe it is as simple as VCCTS or TSIREF not connected on the
> 'failing' board.

I indeed suspect that this could be a wiring issue. Out of curiosity,
how did you find the relevant pin names?

> Did you check for 'thermal error enable' in the GERRCTL register ?

Interesting suggestion. But the bit is set on the only working board I
have access to (Thermal error reporting disabled) while it is cleared
on the non-working boards (Thermal error reporting enabled.) So I don't
think we can rely on it.

Another interesting register in device 00:14.2 is THRERRCTL. It's set
to 0x5 on the one working board I have access to, and to 0x0 on
non-working ones.

-- 
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 3/3] hwmon: (i5500_temp) Don't bind to disabled sensors

[Guenter Roeck]

On 10/23/2014 01:44 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Wed, 22 Oct 2014 15:53:35 -0700, Guenter Roeck wrote:
>> On Wed, Oct 22, 2014 at 07:54:49PM +0200, Romain Dolbeau wrote:
>>> 2014-10-22 18:26 GMT+02:00 Guenter Roeck <linux@roeck-us.net>:
>>>> Is there really no other register you can use to detect if the sensor is enabled ?
>>>> How about the TSTIMER register ?
>>>
>>> As far as I can tell (but I'm not an expert), all the documented
>>> registers display "sane" values, so it's hard to tell apart working
>>
>> Too bad. I thought that maybe the TSTIMER would have its poweron
>> value of 0, though the datasheet is a bit confusing in that regard.
>> It seems to state that the default value is 0 and 200,000 at the
>> same time.
>
> Value of this register is 0x07d30d40 on three boards where the driver
> doesn't work. This means PRESCALER = 0x30d40 = 200000 and FILTER > 0x07d = 125, as indicated in the datasheet. On the two boards where the
> driver does work, the FILTER field has the same value, however the
> PRESCALER has a different value (257600 on one, 257201 on the other.)
>
> So we could at least improve the heuristic and only fail the driver
> binding if TSFSC = 0x7F _and_ TSTIMER = 0x07D30D40. If someone changed
> the prescaler or filter values, there are good chances that the driver
> should work.
>
>> Maybe it is as simple as VCCTS or TSIREF not connected on the
>> 'failing' board.
>
> I indeed suspect that this could be a wiring issue. Out of curiosity,
> how did you find the relevant pin names?
>
Hi Jean,

The datasheet is online. I just browsed through it, looking for the
term "thermal".

>> Did you check for 'thermal error enable' in the GERRCTL register ?
>
> Interesting suggestion. But the bit is set on the only working board I
> have access to (Thermal error reporting disabled) while it is cleared
> on the non-working boards (Thermal error reporting enabled.) So I don't
> think we can rely on it.
>
Interesting.

> Another interesting register in device 00:14.2 is THRERRCTL. It's set
> to 0x5 on the one working board I have access to, and to 0x0 on
> non-working ones.
>
Can you set it on the non-working boards ?

Thanks,
Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 3/3] hwmon: (i5500_temp) Don't bind to disabled sensors

[Jean Delvare]

On Thu, 23 Oct 2014 06:15:50 -0700, Guenter Roeck wrote:
> On 10/23/2014 01:44 AM, Jean Delvare wrote:
> > Another interesting register in device 00:14.2 is THRERRCTL. It's set
> > to 0x5 on the one working board I have access to, and to 0x0 on
> > non-working ones.
>
> Can you set it on the non-working boards ?

Mind you, I tried that already, but it didn't help :( So either it is a
hardware issue (pins not connected as you suggested before) or it takes
a combination of changes to get it to work and we did not find all the
steps, or didn't apply them all at the same time, or not in the right
order... Maybe Romain will have more luck than me.

-- 
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 3/3] hwmon: (i5500_temp) Don't bind to disabled sensors

[Romain Dolbeau]

2014-10-23 15:26 GMT+02:00 Jean Delvare <jdelvare@suse.de>:
> Mind you, I tried that already, but it didn't help :(
[...]
> Maybe Romain will have more luck than me.

Nope, those registers are pretty much identical on
both systems for me, AFAICT with lspci -xxxx.

Cordially,

-- 
Romain Dolbeau

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 3/3] hwmon: (i5500_temp) Don't bind to disabled sensors

[Guenter Roeck]

On 10/23/2014 06:26 AM, Jean Delvare wrote:
> On Thu, 23 Oct 2014 06:15:50 -0700, Guenter Roeck wrote:
>> On 10/23/2014 01:44 AM, Jean Delvare wrote:
>>> Another interesting register in device 00:14.2 is THRERRCTL. It's set
>>> to 0x5 on the one working board I have access to, and to 0x0 on
>>> non-working ones.
>>
>> Can you set it on the non-working boards ?
>
> Mind you, I tried that already, but it didn't help :( So either it is a
> hardware issue (pins not connected as you suggested before) or it takes
> a combination of changes to get it to work and we did not find all the
> steps, or didn't apply them all at the same time, or not in the right
> order... Maybe Romain will have more luck than me.
>
Should have guessed. Well, you can only do so much.

Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors