* Re: [lm-sensors] [PATCH] sensors-detect: Add TMP42x chip detection
2009-07-12 19:34 [lm-sensors] [PATCH] sensors-detect: Add TMP42x chip detection Andre Prendel
@ 2009-07-13 8:53 ` Jean Delvare
2009-07-13 11:44 ` Andre Prendel
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2009-07-13 8:53 UTC (permalink / raw)
To: lm-sensors
Hi Andre,
On Sun, 12 Jul 2009 21:34:04 +0200, Andre Prendel wrote:
> This patch adds detection for TI's TMP421, TMP422 and TMP423 chips to
> sensors-detect. Only the manufactory ID and the device ID is used for
> detection. I hope this is reliable enough.
> ---
>
> sensors-detect | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> Index: sensors/prog/detect/sensors-detect
> =================================> --- sensors.orig/prog/detect/sensors-detect 2009-07-10 22:15:41.000000000 +0200
> +++ sensors/prog/detect/sensors-detect 2009-07-12 19:26:14.000000000 +0200
> @@ -908,6 +908,21 @@
> i2c_addrs => [0x4c..0x4e],
> i2c_detect => sub { lm90_detect(@_, 10); },
> }, {
> + name => "Texas Instruments TMP421",
> + driver => "tmp421",
> + i2c_addrs => [0x2a, 0x4c..0x4f],
Maybe you can add a comment about the other addresses supported by the
chip but not probed?
> + i2c_detect => sub { tmp42x_detect(@_, 0); },
> + }, {
> + name => "Texas Instruments TMP422",
> + driver => "tmp421",
> + i2c_addrs => [0x2a, 0x4c..0x4f],
> + i2c_detect => sub { tmp42x_detect(@_, 1); },
> + }, {
> + name => "Texas Instruments TMP423",
> + driver => "tmp421",
> + i2c_addrs => [0x2a, 0x4c..0x4f],
> + i2c_detect => sub { tmp42x_detect(@_, 2); },
> + }, {
> name => "National Semiconductor LM95231",
> driver => "to-be-written",
> i2c_addrs => [0x2b, 0x19, 0x2a],
> @@ -3907,6 +3922,25 @@
> }
>
> # Registers used:
> +# 0xfe: Manufactorer ID
> +# 0xff: Device ID
> +sub tmp42x_detect()
> +{
> + my ($file, $addr, $chip) = @_;
> +
> + my $mid = i2c_smbus_read_byte_data($file, 0xfe);
> + my $cid = i2c_smbus_read_byte_data($file, 0xff);
> +
> + return if ($mid != 0x55);
> +
> + return 6 if ($chip = 0 && $cid = 0x21); # TMP421
> + return 6 if ($chip = 1 && $cid = 0x22); # TMP422
> + return 6 if ($chip = 2 && $cid = 0x23); # TMP423
> +
> + return;
> +}
> +
> +# Registers used:
> # 0x03: Configuration (no low nibble, returns the previous low nibble)
> # 0x04: Conversion rate
> # 0xfe: Manufacturer ID
Other than that it looks OK, please apply.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [lm-sensors] [PATCH] sensors-detect: Add TMP42x chip detection
2009-07-12 19:34 [lm-sensors] [PATCH] sensors-detect: Add TMP42x chip detection Andre Prendel
2009-07-13 8:53 ` Jean Delvare
@ 2009-07-13 11:44 ` Andre Prendel
2009-07-13 12:10 ` Jean Delvare
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Andre Prendel @ 2009-07-13 11:44 UTC (permalink / raw)
To: lm-sensors
On Mon, Jul 13, 2009 at 10:53:54AM +0200, Jean Delvare wrote:
> Hi Andre,
>
> On Sun, 12 Jul 2009 21:34:04 +0200, Andre Prendel wrote:
> > This patch adds detection for TI's TMP421, TMP422 and TMP423 chips to
> > sensors-detect. Only the manufactory ID and the device ID is used for
> > detection. I hope this is reliable enough.
> > ---
> >
> > sensors-detect | 34 ++++++++++++++++++++++++++++++++++
> > 1 file changed, 34 insertions(+)
> >
> > Index: sensors/prog/detect/sensors-detect
> > =================================> > --- sensors.orig/prog/detect/sensors-detect 2009-07-10 22:15:41.000000000 +0200
> > +++ sensors/prog/detect/sensors-detect 2009-07-12 19:26:14.000000000 +0200
> > @@ -908,6 +908,21 @@
> > i2c_addrs => [0x4c..0x4e],
> > i2c_detect => sub { lm90_detect(@_, 10); },
> > }, {
> > + name => "Texas Instruments TMP421",
> > + driver => "tmp421",
> > + i2c_addrs => [0x2a, 0x4c..0x4f],
>
> Maybe you can add a comment about the other addresses supported by the
> chip but not probed?
Ok, will do that. What's actually the reason why we dont probe all addresses?
Why is it unsafe and what can happen probing an unsafe address?
> > + i2c_detect => sub { tmp42x_detect(@_, 0); },
> > + }, {
> > + name => "Texas Instruments TMP422",
> > + driver => "tmp421",
> > + i2c_addrs => [0x2a, 0x4c..0x4f],
> > + i2c_detect => sub { tmp42x_detect(@_, 1); },
> > + }, {
> > + name => "Texas Instruments TMP423",
> > + driver => "tmp421",
> > + i2c_addrs => [0x2a, 0x4c..0x4f],
> > + i2c_detect => sub { tmp42x_detect(@_, 2); },
> > + }, {
> > name => "National Semiconductor LM95231",
> > driver => "to-be-written",
> > i2c_addrs => [0x2b, 0x19, 0x2a],
> > @@ -3907,6 +3922,25 @@
> > }
> >
> > # Registers used:
> > +# 0xfe: Manufactorer ID
> > +# 0xff: Device ID
> > +sub tmp42x_detect()
> > +{
> > + my ($file, $addr, $chip) = @_;
> > +
> > + my $mid = i2c_smbus_read_byte_data($file, 0xfe);
> > + my $cid = i2c_smbus_read_byte_data($file, 0xff);
> > +
> > + return if ($mid != 0x55);
> > +
> > + return 6 if ($chip = 0 && $cid = 0x21); # TMP421
> > + return 6 if ($chip = 1 && $cid = 0x22); # TMP422
> > + return 6 if ($chip = 2 && $cid = 0x23); # TMP423
> > +
> > + return;
> > +}
> > +
> > +# Registers used:
> > # 0x03: Configuration (no low nibble, returns the previous low nibble)
> > # 0x04: Conversion rate
> > # 0xfe: Manufacturer ID
>
> Other than that it looks OK, please apply.
Thanks,
Andre
> --
> Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [lm-sensors] [PATCH] sensors-detect: Add TMP42x chip detection
2009-07-12 19:34 [lm-sensors] [PATCH] sensors-detect: Add TMP42x chip detection Andre Prendel
2009-07-13 8:53 ` Jean Delvare
2009-07-13 11:44 ` Andre Prendel
@ 2009-07-13 12:10 ` Jean Delvare
2009-07-14 17:11 ` Andre Prendel
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2009-07-13 12:10 UTC (permalink / raw)
To: lm-sensors
On Mon, 13 Jul 2009 13:44:35 +0200, Andre Prendel wrote:
> On Mon, Jul 13, 2009 at 10:53:54AM +0200, Jean Delvare wrote:
> > Hi Andre,
> >
> > On Sun, 12 Jul 2009 21:34:04 +0200, Andre Prendel wrote:
> > > This patch adds detection for TI's TMP421, TMP422 and TMP423 chips to
> > > sensors-detect. Only the manufactory ID and the device ID is used for
> > > detection. I hope this is reliable enough.
> > > ---
> > >
> > > sensors-detect | 34 ++++++++++++++++++++++++++++++++++
> > > 1 file changed, 34 insertions(+)
> > >
> > > Index: sensors/prog/detect/sensors-detect
> > > =================================> > > --- sensors.orig/prog/detect/sensors-detect 2009-07-10 22:15:41.000000000 +0200
> > > +++ sensors/prog/detect/sensors-detect 2009-07-12 19:26:14.000000000 +0200
> > > @@ -908,6 +908,21 @@
> > > i2c_addrs => [0x4c..0x4e],
> > > i2c_detect => sub { lm90_detect(@_, 10); },
> > > }, {
> > > + name => "Texas Instruments TMP421",
> > > + driver => "tmp421",
> > > + i2c_addrs => [0x2a, 0x4c..0x4f],
> >
> > Maybe you can add a comment about the other addresses supported by the
> > chip but not probed?
>
> Ok, will do that. What's actually the reason why we dont probe all addresses?
> Why is it unsafe and what can happen probing an unsafe address?
We avoid probing addresses which are not very popular for hardware
monitoring chips. For addresses which are already probed for many
chips, adding one doesn't change much.
The reason is that I2C transactions do not have semantics attached. A
given transaction can do different things depending on the device which
receives it. Even transactions which SMBus says are "read"
transactions, can write to chips. Or they read but have side effects.
So we want to be extra cautious and not probe more than needed.
Bad things can happen when probing I2C devices in a way they did not
expect. The lm-sensors project has an history of (thankfully rare)
reports of sensors-detect confusing or even breaking pieces of hardware.
Sometimes the reason is design flaws in chips [1], sometimes it's just
that the chip did not like the probe [2].
[1] http://www.lm-sensors.org/browser/lm-sensors/branches/lm-sensors-2.10/README.thinkpad
[2] http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023020.html
Over time we have improved both sensors-detect [3] and some kernel
drivers [4] to prevent all known issues. But more issues are bound to
arise in the future again as long as we do SMBus probing. This is the
main reason why I recently changed the order of probes in
sensors-detect [5] and sensors-detect now skips SMBus probing if
Super-I/O sensors were found [6] (with some exceptions.) This is better
than the previous situation, but still not bullet-proof.
[3] http://www.lm-sensors.org/changeset/5245
[4] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h\b851d6eb4eeb0894f4d095dfdf8ab61c435ad57
[5] http://www.lm-sensors.org/changeset/5498
[6] http://www.lm-sensors.org/changeset/5511
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [lm-sensors] [PATCH] sensors-detect: Add TMP42x chip detection
2009-07-12 19:34 [lm-sensors] [PATCH] sensors-detect: Add TMP42x chip detection Andre Prendel
` (2 preceding siblings ...)
2009-07-13 12:10 ` Jean Delvare
@ 2009-07-14 17:11 ` Andre Prendel
2009-07-14 17:47 ` Jean Delvare
2009-07-14 20:19 ` Andre Prendel
5 siblings, 0 replies; 7+ messages in thread
From: Andre Prendel @ 2009-07-14 17:11 UTC (permalink / raw)
To: lm-sensors
On Mon, Jul 13, 2009 at 02:10:41PM +0200, Jean Delvare wrote:
<snip>
> > Ok, will do that. What's actually the reason why we dont probe all addresses?
> > Why is it unsafe and what can happen probing an unsafe address?
>
> We avoid probing addresses which are not very popular for hardware
> monitoring chips. For addresses which are already probed for many
> chips, adding one doesn't change much.
>
> The reason is that I2C transactions do not have semantics attached. A
> given transaction can do different things depending on the device which
> receives it. Even transactions which SMBus says are "read"
> transactions, can write to chips. Or they read but have side effects.
> So we want to be extra cautious and not probe more than needed.
>
> Bad things can happen when probing I2C devices in a way they did not
> expect. The lm-sensors project has an history of (thankfully rare)
> reports of sensors-detect confusing or even breaking pieces of hardware.
> Sometimes the reason is design flaws in chips [1], sometimes it's just
> that the chip did not like the probe [2].
>
> [1] http://www.lm-sensors.org/browser/lm-sensors/branches/lm-sensors-2.10/README.thinkpad
> [2] http://lists.lm-sensors.org/pipermail/lm-sensors/2008-May/023020.html
>
> Over time we have improved both sensors-detect [3] and some kernel
> drivers [4] to prevent all known issues. But more issues are bound to
> arise in the future again as long as we do SMBus probing. This is the
> main reason why I recently changed the order of probes in
> sensors-detect [5] and sensors-detect now skips SMBus probing if
> Super-I/O sensors were found [6] (with some exceptions.) This is better
> than the previous situation, but still not bullet-proof.
>
> [3] http://www.lm-sensors.org/changeset/5245
> [4] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h\b851d6eb4eeb0894f4d095dfdf8ab61c435ad57
> [5] http://www.lm-sensors.org/changeset/5498
> [6] http://www.lm-sensors.org/changeset/5511
>
Thank for the lesson. Very informativ.
Below you can find an updated patch with the comment for addresses not
probed. Further I've adjusted the address range for TMP422 and
TMP423. TMP422 only supports 0x4c-0x4f and TMP423 0x4c, 0x4d.
---
Index: sensors/prog/detect/sensors-detect
=================================--- sensors.orig/prog/detect/sensors-detect 2009-07-10 22:15:41.000000000 +0200
+++ sensors/prog/detect/sensors-detect 2009-07-14 18:34:05.000000000 +0200
@@ -908,6 +908,21 @@
i2c_addrs => [0x4c..0x4e],
i2c_detect => sub { lm90_detect(@_, 10); },
}, {
+ name => "Texas Instruments TMP421",
+ driver => "tmp421",
+ i2c_addrs => [0x2a, 0x4c..0x4f], # 0x1c-0x1f not probed.
+ i2c_detect => sub { tmp42x_detect(@_, 0); },
+ }, {
+ name => "Texas Instruments TMP422",
+ driver => "tmp421",
+ i2c_addrs => [0x4c..0x4f],
+ i2c_detect => sub { tmp42x_detect(@_, 1); },
+ }, {
+ name => "Texas Instruments TMP423",
+ driver => "tmp421",
+ i2c_addrs => [0x4c, 0x4d],
+ i2c_detect => sub { tmp42x_detect(@_, 2); },
+ }, {
name => "National Semiconductor LM95231",
driver => "to-be-written",
i2c_addrs => [0x2b, 0x19, 0x2a],
@@ -3907,6 +3922,25 @@
}
# Registers used:
+# 0xfe: Manufactorer ID
+# 0xff: Device ID
+sub tmp42x_detect()
+{
+ my ($file, $addr, $chip) = @_;
+
+ my $mid = i2c_smbus_read_byte_data($file, 0xfe);
+ my $cid = i2c_smbus_read_byte_data($file, 0xff);
+
+ return if ($mid != 0x55);
+
+ return 6 if ($chip = 0 && $cid = 0x21); # TMP421
+ return 6 if ($chip = 1 && $cid = 0x22); # TMP422
+ return 6 if ($chip = 2 && $cid = 0x23); # TMP423
+
+ return;
+}
+
+# Registers used:
# 0x03: Configuration (no low nibble, returns the previous low nibble)
# 0x04: Conversion rate
# 0xfe: Manufacturer ID
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [lm-sensors] [PATCH] sensors-detect: Add TMP42x chip detection
2009-07-12 19:34 [lm-sensors] [PATCH] sensors-detect: Add TMP42x chip detection Andre Prendel
` (3 preceding siblings ...)
2009-07-14 17:11 ` Andre Prendel
@ 2009-07-14 17:47 ` Jean Delvare
2009-07-14 20:19 ` Andre Prendel
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2009-07-14 17:47 UTC (permalink / raw)
To: lm-sensors
On Tue, 14 Jul 2009 19:11:32 +0200, Andre Prendel wrote:
> Below you can find an updated patch with the comment for addresses not
> probed. Further I've adjusted the address range for TMP422 and
> TMP423. TMP422 only supports 0x4c-0x4f and TMP423 0x4c, 0x4d.
> ---
Ah, yeah, good catch. BTW this could be used to improve detection
reliability in the tmp421 driver.
>
> Index: sensors/prog/detect/sensors-detect
> =================================> --- sensors.orig/prog/detect/sensors-detect 2009-07-10 22:15:41.000000000 +0200
> +++ sensors/prog/detect/sensors-detect 2009-07-14 18:34:05.000000000 +0200
> @@ -908,6 +908,21 @@
> i2c_addrs => [0x4c..0x4e],
> i2c_detect => sub { lm90_detect(@_, 10); },
> }, {
> + name => "Texas Instruments TMP421",
> + driver => "tmp421",
> + i2c_addrs => [0x2a, 0x4c..0x4f], # 0x1c-0x1f not probed.
> + i2c_detect => sub { tmp42x_detect(@_, 0); },
> + }, {
> + name => "Texas Instruments TMP422",
> + driver => "tmp421",
> + i2c_addrs => [0x4c..0x4f],
> + i2c_detect => sub { tmp42x_detect(@_, 1); },
> + }, {
> + name => "Texas Instruments TMP423",
> + driver => "tmp421",
> + i2c_addrs => [0x4c, 0x4d],
> + i2c_detect => sub { tmp42x_detect(@_, 2); },
> + }, {
> name => "National Semiconductor LM95231",
> driver => "to-be-written",
> i2c_addrs => [0x2b, 0x19, 0x2a],
> @@ -3907,6 +3922,25 @@
> }
>
> # Registers used:
Please add a reminder about the values of $chip, as other detect
functions do.
> +# 0xfe: Manufactorer ID
> +# 0xff: Device ID
> +sub tmp42x_detect()
> +{
> + my ($file, $addr, $chip) = @_;
> +
> + my $mid = i2c_smbus_read_byte_data($file, 0xfe);
> + my $cid = i2c_smbus_read_byte_data($file, 0xff);
> +
> + return if ($mid != 0x55);
> +
> + return 6 if ($chip = 0 && $cid = 0x21); # TMP421
> + return 6 if ($chip = 1 && $cid = 0x22); # TMP422
> + return 6 if ($chip = 2 && $cid = 0x23); # TMP423
> +
> + return;
> +}
> +
> +# Registers used:
> # 0x03: Configuration (no low nibble, returns the previous low nibble)
> # 0x04: Conversion rate
> # 0xfe: Manufacturer ID
Other than that, looks good, please apply.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [lm-sensors] [PATCH] sensors-detect: Add TMP42x chip detection
2009-07-12 19:34 [lm-sensors] [PATCH] sensors-detect: Add TMP42x chip detection Andre Prendel
` (4 preceding siblings ...)
2009-07-14 17:47 ` Jean Delvare
@ 2009-07-14 20:19 ` Andre Prendel
5 siblings, 0 replies; 7+ messages in thread
From: Andre Prendel @ 2009-07-14 20:19 UTC (permalink / raw)
To: lm-sensors
On Tue, Jul 14, 2009 at 07:47:00PM +0200, Jean Delvare wrote:
> On Tue, 14 Jul 2009 19:11:32 +0200, Andre Prendel wrote:
> > Below you can find an updated patch with the comment for addresses not
> > probed. Further I've adjusted the address range for TMP422 and
> > TMP423. TMP422 only supports 0x4c-0x4f and TMP423 0x4c, 0x4d.
> > ---
>
> Ah, yeah, good catch. BTW this could be used to improve detection
> reliability in the tmp421 driver.
>
> >
> > Index: sensors/prog/detect/sensors-detect
> > =================================> > --- sensors.orig/prog/detect/sensors-detect 2009-07-10 22:15:41.000000000 +0200
> > +++ sensors/prog/detect/sensors-detect 2009-07-14 18:34:05.000000000 +0200
> > @@ -908,6 +908,21 @@
> > i2c_addrs => [0x4c..0x4e],
> > i2c_detect => sub { lm90_detect(@_, 10); },
> > }, {
> > + name => "Texas Instruments TMP421",
> > + driver => "tmp421",
> > + i2c_addrs => [0x2a, 0x4c..0x4f], # 0x1c-0x1f not probed.
> > + i2c_detect => sub { tmp42x_detect(@_, 0); },
> > + }, {
> > + name => "Texas Instruments TMP422",
> > + driver => "tmp421",
> > + i2c_addrs => [0x4c..0x4f],
> > + i2c_detect => sub { tmp42x_detect(@_, 1); },
> > + }, {
> > + name => "Texas Instruments TMP423",
> > + driver => "tmp421",
> > + i2c_addrs => [0x4c, 0x4d],
> > + i2c_detect => sub { tmp42x_detect(@_, 2); },
> > + }, {
> > name => "National Semiconductor LM95231",
> > driver => "to-be-written",
> > i2c_addrs => [0x2b, 0x19, 0x2a],
> > @@ -3907,6 +3922,25 @@
> > }
> >
> > # Registers used:
>
> Please add a reminder about the values of $chip, as other detect
> functions do.
>
> > +# 0xfe: Manufactorer ID
> > +# 0xff: Device ID
> > +sub tmp42x_detect()
> > +{
> > + my ($file, $addr, $chip) = @_;
> > +
> > + my $mid = i2c_smbus_read_byte_data($file, 0xfe);
> > + my $cid = i2c_smbus_read_byte_data($file, 0xff);
> > +
> > + return if ($mid != 0x55);
> > +
> > + return 6 if ($chip = 0 && $cid = 0x21); # TMP421
> > + return 6 if ($chip = 1 && $cid = 0x22); # TMP422
> > + return 6 if ($chip = 2 && $cid = 0x23); # TMP423
> > +
> > + return;
> > +}
> > +
> > +# Registers used:
> > # 0x03: Configuration (no low nibble, returns the previous low nibble)
> > # 0x04: Conversion rate
> > # 0xfe: Manufacturer ID
>
> Other than that, looks good, please apply.
Done.
Thanks,
Andre
> --
> Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread