Broken DS1621 detection / ds1621 module / A7V8X

Broken DS1621 detection / ds1621 module / A7V8X

[Prof. Dr. Peter A. Henning]

Hi there,

I have some DS1621 temperature sensors attached to an VIA 8235 sitting on my 
Asus A7V8X.

111111111111111111111111111111111111111111111111111111111
Detection of the DS1621 in sensors-detect is broken. According to the data 
sheet of the DS1621, one bit of the configuration register is always 1 and 
another always 0. This is simply not true, if the chip is not properly 
initialized. I suggest the following replacement for sub ds1621_detect in 
sensors-detect

--------------- SNIP ---------------------------------------------------
# $_[0]: A reference to the file descriptor to access this chip.
#        We may assume an i2c_set_slave_addr was already done.
# $_[1]: Address
# Returns: undef if not detected, (3) if detected,
#   (5) or (7) if even more bits match. 
# Registers used:
#   0xAA: Temperature
#   0xA1: High limit
#   0xA2: Low limit
#   0xAC: Configuration
#
# subroutine modified by Peter A. Henning
# Old version checks, if Bit 3 is set and Bit 2 is clear. 
# => THIS IS NOT ALWAYS TRUE
# The DS1621 will however aways have a config like 0x????1??? 
# Also, the calculation of temperatures needs to swap bytes. The lower byte 
# (before swapping) contains the temperature as signed 8-bit integer, the
# higher byte's (before swapping) highest bit is worth another 0.5 degree.
# Hence, logical AND with 0x7F00 should give zero. 

sub ds1621_detect
{
  my $i;
  my ($file,$addr) = @_;
  my $temp = i2c_smbus_read_word_data($file,0xAA);
  return if ($temp & 0x7F00);
  $temp = i2c_smbus_read_word_data($file,0xA1);
  return if ($temp & 0x7F00);
  $temp = i2c_smbus_read_word_data($file,0xA2);
  return if ($temp & 0x7F00);
  my $conf = i2c_smbus_read_byte_data($file,0xAC);
  return (7) if ($conf & 0x10) = 0x10;
}
------------- SNAP --------------------------------------------------------

22222222222222222222222222222222222222222222222222222222222
The DS1621 kernel driver module only loads correctly, if each of the DS1621 
chips is initialized properly. For this I use a simple Perl script writing 
into the configuration register a value of 0x09

i2c_smbus_write_byte_data(\*FILE,0xAC,0x09);

Could one integrate this into the kernel module ?


3333333333333333333333333333333333333333333333333333333333
A note on the Asus Mainboard A7V8X: It carries an ASB100 chip, which occupies 
addresses 0x48 and 0x49. These collide with the two lower addresses possible 
for the DS1621 (ranging from 0x48 - 0x4f). Address bits for the DS1621 must 
be set accordingly. Also, I find that for this Asus Mainboard one needs a 
different temperature algorithm in sensors.conf:

Contribution to sensors.conf:

------------- SNIP -------------------------------------------
# Dallas 1621 Chips connected to internal SMBus
# Last number = 48 + 3-Bit Adress set at Sensor Chip
# Careful: Adresses 0x48 and 0x49 need to be kept free for asb100
#
chip "ds1621-i2c-*-4f"
    label temp  "Power Temp"
    set temp_hyst 35
    set temp_over 40
chip "ds1621-i2c-*-4e"
    label temp  "GPU Temp"
    set temp_hyst 45
    set temp_over 55
chip "ds1621-i2c-*-4d"
    label temp  "Harddisk Temp"
    set temp_hyst 35
    set temp_over 40
#
# Configuration for Asus A7V8X
#
# asb100 Chip on Board
#
chip "asb100-*"

    (Here is the usual stuff for voltages & fans)

    # Used for Athlon Socket A
    # obtained by careful comparison against Asus Probe under Windows
    label temp2 "CPU Socket Temp"
    set temp2_over 60
    set temp2_hyst 50
    compute temp2 (@*90/100)+7, (@-7)*100/90

    # Used for Athlon diode
    # obtained by careful comparison against Asus Probe under Windows
    label temp4 "CPU Diode Temp"
    set temp4_over 60
    set temp4_hyst 50
    compute temp4 (@*90/100)+7, (@-7)*100/90
---------------- SNAP ----------------------------------------------------             

Regards

-- 
Peter Henning

----------------------------------------------PGP 0x5CDC14A1 -------
| Prof.Dr.Peter A.Henning
| 
| European E-Learning Award EureleA
| http://www.eurelea.org
|
| Director MediaLab
| Computer Science, Karlsruhe University of Applied Sciences
| http://medialab.fh-karlsruhe.de/
|-------------------------------------------------------------------

Broken DS1621 detection / ds1621 module / A7V8X

[Aurelien Jarno]

On Tue, Jan 04, 2005 at 02:08:12AM +0100, Prof. Dr. Peter A. Henning wrote:
> Hi there,
Hi!

> I have some DS1621 temperature sensors attached to an VIA 8235 sitting on my 
> Asus A7V8X.
> 
> 111111111111111111111111111111111111111111111111111111111
> Detection of the DS1621 in sensors-detect is broken. According to the data 
> sheet of the DS1621, one bit of the configuration register is always 1 and 
> another always 0. This is simply not true, if the chip is not properly 
> initialized. I suggest the following replacement for sub ds1621_detect in 
> sensors-detect
That sounds me strange. I am using a ds1621 plugged onto my parallel
port I2C interface and the module is always loaded at boot. I am using
this for more than 4 years (the chip is dated 0022, ie year 2000 week 
22).

> --------------- SNIP ---------------------------------------------------
> # $_[0]: A reference to the file descriptor to access this chip.
> #        We may assume an i2c_set_slave_addr was already done.
> # $_[1]: Address
> # Returns: undef if not detected, (3) if detected,
> #   (5) or (7) if even more bits match. 
> # Registers used:
> #   0xAA: Temperature
> #   0xA1: High limit
> #   0xA2: Low limit
> #   0xAC: Configuration
> #
> # subroutine modified by Peter A. Henning
> # Old version checks, if Bit 3 is set and Bit 2 is clear. 
> # => THIS IS NOT ALWAYS TRUE
> # The DS1621 will however aways have a config like 0x????1??? 
> # Also, the calculation of temperatures needs to swap bytes. The lower byte 
> # (before swapping) contains the temperature as signed 8-bit integer, the
> # higher byte's (before swapping) highest bit is worth another 0.5 degree.
> # Hence, logical AND with 0x7F00 should give zero. 
If the chip is not correctly initialised, there is also some chances 
that this is not always true. Moreover it will be weakening the
detection which is already not very strong. That could make some chips
to be falsely detected as ds1621.

The better solution would be to understand why your chip does not
conform to the datasheet.

Are you using a ds1621 chip or a compatible one (ds1625, ds1629, ds1721
or something like that)?
Are you sure they is no interference between your asb100 chip and your
ds1621?
AFAIK, the VT8235 only has support for smbus and not i2c. The minor
differences in the protocol (mainly timing protocol), may cause bus
lockup. Do you think it may correspond to what you are observing?

Or maybe the new Dalls chips are buggy...


-- 
  .''`.  Aurelien Jarno	              GPG: 1024D/F1BCDB73
 : :' :  Debian GNU/Linux developer | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Broken DS1621 detection / ds1621 module / A7V8X

[Prof. Dr. Peter A. Henning]

Hi there,

Am Freitag, 7. Januar 2005 21:14 schrieb Aurelien Jarno:
> > 111111111111111111111111111111111111111111111111111111111
> > Detection of the DS1621 in sensors-detect is broken. According to the
> > data sheet of the DS1621, one bit of the configuration register is always
> > 1 and another always 0. This is simply not true, if the chip is not
> > properly initialized. I suggest the following replacement for sub
> > ds1621_detect in sensors-detect
>
> That sounds me strange. I am using a ds1621 plugged onto my parallel
> port I2C interface and the module is always loaded at boot. I am using
> this for more than 4 years (the chip is dated 0022, ie year 2000 week
> 22).

Well, may be strange - but that's fact. The data sheet does not have a single 
line indicating that the two "fixed" bits may be overwritten - and yet, 
something does this.

>> # subroutine modified by Peter A. Henning
>> # Old version checks, if Bit 3 is set and Bit 2 is clear. 
>> # => THIS IS NOT ALWAYS TRUE
> ># The DS1621 will however aways have a config like 0x????1???
> If the chip is not correctly initialised, there is also some chances 
> that this is not always true. 

Could be, but with three different DS162 chips I found not a single case where 
the 0x????1??? was not true.

> Moreover it will be weakening the 
> detection which is already not very strong. That could make some chips
> to be falsely detected as ds1621.

Agreed. But worse is the case where a positively identified DS1621 i snot 
recognized as such - and the detection gets stronger than in the old code 
when the temperature is properly identified.
>
> The better solution would be to understand why your chip does not
> conform to the datasheet.

Agreed. But I have three chips on the same bus now, and they all behave the 
same
>
> Are you using a ds1621 chip or a compatible one (ds1625, ds1629, ds1721
> or something like that)?, 
DS1621, stamped 0445B4 265AE
Three chips, bought December 2004

> Are you sure they is no interference between your asb100 chip and your
> ds162b1?

Of course not. But I deem it unlikely to have the asb interfering with the 
three different chips (sitting at different addresses of course) in the same 
way
> AFAIK, the VT8235 only has support for smbus and not i2c. The minor
> differences in the protocol (mainly timing protocol), may cause bus
> lockup. Do you think it may correspond to what you are observing?
>
Asus advertizes the board to have an SMBus interface. And yet, Dallas does not 
say anything of SMBus and s quoted as being I2C compatible.


> Or maybe the new Dalls chips are buggy...
Do you want to test one of mine ?

Best regards
-- 
Peter Henning

----------------------------------------------PGP 0x5CDC14A1 -------
| Prof.Dr.Peter A.Henning
| 
| European E-Learning Award EureleA
| http://www.eurelea.org
|
| Director MediaLab
| Computer Science, Karlsruhe University of Applied Sciences
| http://medialab.fh-karlsruhe.de/
|-------------------------------------------------------------------

Broken DS1621 detection / ds1621 module / A7V8X

[Aurélien Jarno]

Prof. Dr. Peter A. Henning wrote:
> Hi there,
> 
> Am Freitag, 7. Januar 2005 21:14 schrieb Aurelien Jarno:
> 
>>>111111111111111111111111111111111111111111111111111111111
>>>Detection of the DS1621 in sensors-detect is broken. According to the
>>>data sheet of the DS1621, one bit of the configuration register is always
>>>1 and another always 0. This is simply not true, if the chip is not
>>>properly initialized. I suggest the following replacement for sub
>>>ds1621_detect in sensors-detect
>>
>>That sounds me strange. I am using a ds1621 plugged onto my parallel
>>port I2C interface and the module is always loaded at boot. I am using
>>this for more than 4 years (the chip is dated 0022, ie year 2000 week
>>22).
> 
> 
> Well, may be strange - but that's fact. The data sheet does not have a single 
> line indicating that the two "fixed" bits may be overwritten - and yet, 
> something does this.

Ok, I have done some tests on my ds1621 chip, and I can't write to bits 
3 and 2, there are always respectively 1 and 0.

However, I have seen that Dallas has released a new datasheet for the 
DS1621 which now presents the two bits as 'X', and marks them as 
'reserved'. I suspect they have done some changes in there design, 
that's why you are having some problems.

I have looked to the datasheet to find a way to detect the DS1621 chip. 
Here is my proposal:

1) Verify that the 7 lowest bits of the TL register are 0. This is 
basically what you proposed. It is specified in the datasheet that's 
these bit are always 0, and I doubt it will change in the future 
(because in that case a lot of existing design won't work anymore).

2) Verify that bit 4 of the config register is 0. It means that the chip 
is not currently writing to its EEPROM, or that no EEPROM write has been 
requested in the last 10ms. I think we could safely say that it is 
always the case with the kernel driver.

3) Issue a "stop conversion T" (= sending 0x22 to the chip), and verify 
that the bit 7 of the config register is set to 1 (no conversion in 
progress).

Any comment on using such a detection? If everybody agrees, I'll send a 
patch for both the 2.4 and 2.6 modules.

Bye,
Aurelien

-- 
   .''`.  Aurelien Jarno               GPG: 1024D/F1BCDB73
  : :' :  Debian GNU/Linux developer | Electrical Engineer
  `. `'   aurel32@debian.org         | aurelien@aurel32.net
    `-    people.debian.org/~aurel32 | www.aurel32.net

Broken DS1621 detection / ds1621 module / A7V8X

[Aurélien Jarno]

On Wed, Jan 19, 2005 at 05:27:53PM +0100, Aur?lien Jarno wrote:
 
> I have looked to the datasheet to find a way to detect the DS1621 chip. 
> Here is my proposal:
> 
> 1) Verify that the 7 lowest bits of the TL register are 0. This is 
> basically what you proposed. It is specified in the datasheet that's 
> these bit are always 0, and I doubt it will change in the future 
> (because in that case a lot of existing design won't work anymore).
> 
> 2) Verify that bit 4 of the config register is 0. It means that the chip 
> is not currently writing to its EEPROM, or that no EEPROM write has been 
> requested in the last 10ms. I think we could safely say that it is 
> always the case with the kernel driver.
> 
> 3) Issue a "stop conversion T" (= sending 0x22 to the chip), and verify 
> that the bit 7 of the config register is set to 1 (no conversion in 
> progress).
The 3) doesn't work, because the bit is actually "conversion done", so
on power-up this bit is set to 0. Anyway, starting a conversion toggle
this bit to 0 during 500ms to 1s so if we read it shortly after sending
such a command, it should be 0.

I have attached a patch for the 2.6 kernel as well for lm_sensors CVS. I
have tested the 2.6 module, and it works as expected. For the 2.4
module I only built it as I don't have a 2.4 kernel. Note that I also
improved the detection routine by backporting the 1) from the 2.6
kernel.

If everybody agrees, I'll send the 2.6 patch to Greg.

Bye,
Aurelien

-- 
  .''`.  Aurelien Jarno	              GPG: 1024D/F1BCDB73
 : :' :  Debian GNU/Linux developer | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net
-------------- next part --------------
Index: kernel/chips/ds1621.c
=================================RCS file: /home/cvs/lm_sensors2/kernel/chips/ds1621.c,v
retrieving revision 1.16
diff -u -1 -b -p -r1.16 ds1621.c
--- kernel/chips/ds1621.c	4 Dec 2004 21:18:34 -0000	1.16
+++ kernel/chips/ds1621.c	19 Jan 2005 17:58:36 -0000
@@ -45,5 +45,4 @@ SENSORS_INSMOD_1(ds1621);
 /*  7    6    5    4    3    2    1    0      */
-/* |Done|THF |TLF |NVB | 1  | 0  |POL |1SHOT| */
-#define DS1621_REG_CONFIG_MASK 0x0C
-#define DS1621_REG_CONFIG_VAL 0x08
+/* |Done|THF |TLF |NVB | X  | X  |POL |1SHOT| */
+#define DS1621_REG_CONFIG_NVB 0x10
 #define DS1621_REG_CONFIG_POLARITY 0x02
@@ -167,3 +166,3 @@ int ds1621_detect(struct i2c_adapter *ad
 {
-	int i, conf;
+	int i, conf, temp;
 	struct i2c_client *new_client;
@@ -205,7 +204,33 @@ int ds1621_detect(struct i2c_adapter *ad
 	if (kind < 0) {
+		/* The NVB bit should be low if no EEPROM write has been
+		   requested during the latest 10ms, which is highly
+		   improbable in our case. */
 		conf = i2c_smbus_read_byte_data(new_client,
 						DS1621_REG_CONF);
-		if ((conf & DS1621_REG_CONFIG_MASK)
-		    != DS1621_REG_CONFIG_VAL)
+		if (conf & DS1621_REG_CONFIG_NVB)
 			goto ERROR1;
+		/* The 7 lowest bits of a temperature should always be 0. */
+		temp = ds1621_read_value(new_client, 
+					 DS1621_REG_TEMP);
+		if (temp & 0x007f)
+			goto ERROR1;
+		temp = ds1621_read_value(new_client, 
+					 DS1621_REG_TEMP_OVER);
+		if (temp & 0x007f)
+			goto ERROR1;
+			temp = ds1621_read_value(new_client, 
+					 DS1621_REG_TEMP_HYST);
+		if (temp & 0x007f)
+			goto ERROR1;
+		/* Start conversion. DONE should go to low. */
+		i2c_smbus_write_byte(new_client, 
+				     DS1621_COM_START);
+		conf = i2c_smbus_read_byte_data(new_client,
+						DS1621_REG_CONF);
+		if (conf & DS1621_REG_CONFIG_DONE)
+			goto ERROR1;
+		/* Stop conversion. */
+		i2c_smbus_write_byte(new_client, 
+				     DS1621_COM_STOP);
+	
 	}
-------------- next part --------------
diff -urN linux-2.6.11-rc1.orig/drivers/i2c/chips/ds1621.c linux-2.6.11-rc1/drivers/i2c/chips/ds1621.c
--- linux-2.6.11-rc1.orig/drivers/i2c/chips/ds1621.c	2004-12-24 22:35:40.000000000 +0100
+++ linux-2.6.11-rc1/drivers/i2c/chips/ds1621.c	2005-01-19 18:39:23.000000000 +0100
@@ -42,9 +42,8 @@
 /* Many DS1621 constants specified below */
 /* Config register used for detection         */
 /*  7    6    5    4    3    2    1    0      */
-/* |Done|THF |TLF |NVB | 1  | 0  |POL |1SHOT| */
-#define DS1621_REG_CONFIG_MASK		0x0C
-#define DS1621_REG_CONFIG_VAL		0x08
+/* |Done|THF |TLF |NVB | X  | X  |POL |1SHOT| */
+#define DS1621_REG_CONFIG_NVB		0x10
 #define DS1621_REG_CONFIG_POLARITY	0x02
 #define DS1621_REG_CONFIG_1SHOT		0x01
 #define DS1621_REG_CONFIG_DONE		0x80
@@ -55,6 +54,7 @@
 #define DS1621_REG_TEMP_MAX		0xA2 /* word, RW */
 #define DS1621_REG_CONF			0xAC /* byte, RW */
 #define DS1621_COM_START		0xEE /* no data */
+#define DS1621_COM_STOP			0x22 /* no data */
 
 /* The DS1621 configuration register */
 #define DS1621_ALARM_TEMP_HIGH		0x40
@@ -212,9 +212,13 @@
 
 	/* Now, we do the remaining detection. It is lousy. */
 	if (kind < 0) {
+		/* The NVB bit should be low if no EEPROM write has been 
+		   requested during the latest 10ms, which is highly 
+		   improbable in our case. */
 		conf = ds1621_read_value(new_client, DS1621_REG_CONF);
-		if ((conf & DS1621_REG_CONFIG_MASK) != DS1621_REG_CONFIG_VAL)
+		if (conf & DS1621_REG_CONFIG_NVB)
 			goto exit_free;
+		/* The 7 lowest bits of a temperature should always be 0. */
 		temp = ds1621_read_value(new_client, DS1621_REG_TEMP);
 		if (temp & 0x007f)
 			goto exit_free;
@@ -224,6 +228,13 @@
 		temp = ds1621_read_value(new_client, DS1621_REG_TEMP_MAX);
 		if (temp & 0x007f)
 			goto exit_free;
+		/* Start conversion. DONE should go to low. */
+		i2c_smbus_write_byte(new_client, DS1621_COM_START);
+		conf = ds1621_read_value(new_client, DS1621_REG_CONF);
+		if (conf & DS1621_REG_CONFIG_DONE)
+			goto exit_free;
+		/* Stop conversion. */
+		i2c_smbus_write_byte(new_client, DS1621_COM_STOP);
 	}
 
 	/* Determine the chip type - only one kind supported! */

Broken DS1621 detection / ds1621 module / A7V8X

[Aurélien Jarno]

Hi all,

After some discussion on IRC with Jean Delvare, here is the final patch
for the 2.4 driver. The 2.6 patch has already been sent to Greg KH in 
an other mail.

Bye,
Aurelien

-- 
  .''`.  Aurelien Jarno	              GPG: 1024D/F1BCDB73
 : :' :  Debian GNU/Linux developer | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net
-------------- next part --------------
Index: kernel/chips/ds1621.c
=================================RCS file: /home/cvs/lm_sensors2/kernel/chips/ds1621.c,v
retrieving revision 1.16
diff -u -1 -b -p -r1.16 ds1621.c
--- kernel/chips/ds1621.c	4 Dec 2004 21:18:34 -0000	1.16
+++ kernel/chips/ds1621.c	19 Jan 2005 20:22:41 -0000
@@ -45,5 +45,4 @@ SENSORS_INSMOD_1(ds1621);
 /*  7    6    5    4    3    2    1    0      */
-/* |Done|THF |TLF |NVB | 1  | 0  |POL |1SHOT| */
-#define DS1621_REG_CONFIG_MASK 0x0C
-#define DS1621_REG_CONFIG_VAL 0x08
+/* |Done|THF |TLF |NVB | X  | X  |POL |1SHOT| */
+#define DS1621_REG_CONFIG_NVB 0x10
 #define DS1621_REG_CONFIG_POLARITY 0x02
@@ -167,3 +166,3 @@ int ds1621_detect(struct i2c_adapter *ad
 {
-	int i, conf;
+	int i, conf, temp;
 	struct i2c_client *new_client;
@@ -205,7 +204,22 @@ int ds1621_detect(struct i2c_adapter *ad
 	if (kind < 0) {
+		/* The NVB bit should be low if no EEPROM write has been
+		   requested during the latest 10ms, which is highly
+		   improbable in our case. */
 		conf = i2c_smbus_read_byte_data(new_client,
 						DS1621_REG_CONF);
-		if ((conf & DS1621_REG_CONFIG_MASK)
-		    != DS1621_REG_CONFIG_VAL)
+		if (conf & DS1621_REG_CONFIG_NVB)
 			goto ERROR1;
+		/* The 7 lowest bits of a temperature should always be 0. */
+		temp = ds1621_read_value(new_client, 
+					 DS1621_REG_TEMP);
+		if (temp & 0x007f)
+			goto exit_free;
+		temp = ds1621_read_value(new_client, 
+					 DS1621_REG_TEMP_MIN);
+		if (temp & 0x007f)
+			goto exit_free;
+			temp = ds1621_read_value(new_client, 
+					 DS1621_REG_TEMP_MAX);
+		if (temp & 0x007f)
+			goto exit_free;
 	}