[lm-sensors] DS75

[Jean Delvare]

Hi Alan,

On Wed, 07 Feb 2007 10:14:36 +0000, Alan Clucas wrote:
> Attached is the kernel patch and a patch for libsensors as well. The 
> libsensors patch does not include your sensors-detect patch.

Could you please also include the required changes to the sensors
program and possibly sensord as well? These should be trivial.

> I have changed the detected name to ds75, hence the libsensors patch. I 
> haven't managed to find any lm75 devices here, so I haven't tested this 
> on one of those, which would be good before it goes too far. The patch 
> is simple enough, but you never can tell.

I have an LM75, I just tested and it works OK.

Comments on your kernel patch:

> diff -ur linux-2.6.19.2/drivers/hwmon/lm75.c linux/drivers/hwmon/lm75.c
> --- linux-2.6.19.2/drivers/hwmon/lm75.c	2007-02-06 15:53:57.000000000 +0000
> +++ linux/drivers/hwmon/lm75.c	2007-02-06 17:45:19.000000000 +0000
> @@ -34,7 +34,7 @@
>  					0x4d, 0x4e, 0x4f, I2C_CLIENT_END };
>  
>  /* Insmod parameters */
> -I2C_CLIENT_INSMOD_1(lm75);
> +I2C_CLIENT_INSMOD_2(lm75, ds75);
>  
>  /* Many LM75 constants specified below */
>  
> @@ -153,50 +153,63 @@
>  	new_client->flags = 0;
>  
>  	/* Now, we do the remaining detection. There is no identification-
> -	   dedicated register so we have to rely on several tricks:
> -	   unused bits, registers cycling over 8-address boundaries,
> -	   addresses 0x04-0x07 returning the last read value.
> +	   dedicated register so we have to rely on several tricks.
> +	   The LM75 and DS75 share unused bits. The LM75 has registers
> +	   cycling over 8-address boundaries, and addresses 0x04-0x07 
> +	   returning the last read value.
> +	   The DS75 has addresses 0x04-0x0f returning the last read value,
> +	   but does not register cycle.
>  	   The cycling+unused addresses combination is not tested,
>  	   since it would significantly slow the detection down and would
>  	   hardly add any value. */
>  	if (kind < 0) {
> -		int cur, conf, hyst, os;
> +		int cur, conf, hyst, os, addr;
>  
>  		/* Unused addresses */
>  		cur = i2c_smbus_read_word_data(new_client, 0);
>  		conf = i2c_smbus_read_byte_data(new_client, 1);
>  		hyst = i2c_smbus_read_word_data(new_client, 2);
> -		if (i2c_smbus_read_word_data(new_client, 4) != hyst
> -		 || i2c_smbus_read_word_data(new_client, 5) != hyst
> -		 || i2c_smbus_read_word_data(new_client, 6) != hyst
> -		 || i2c_smbus_read_word_data(new_client, 7) != hyst)
> -		 	goto exit_free;
> +
> +		for (addr = 0x04; addr <= 0x0f; addr++)
> +			if (i2c_smbus_read_word_data(new_client, addr) != hyst)
> +				break;
> +
> +		if (addr < 0x08)
> +			goto exit_free;
> +		if (addr == 0x10)
> +			kind = ds75;
> +		else
> +			kind = lm75;
> +
>  		os = i2c_smbus_read_word_data(new_client, 3);
> -		if (i2c_smbus_read_word_data(new_client, 4) != os
> -		 || i2c_smbus_read_word_data(new_client, 5) != os
> -		 || i2c_smbus_read_word_data(new_client, 6) != os
> -		 || i2c_smbus_read_word_data(new_client, 7) != os)
> -		 	goto exit_free;
> +		for (addr = 0x04; addr <= 0x0f; addr++)
> +			if (i2c_smbus_read_word_data(new_client, addr) != os)
> +				break;
> +
> +		if (addr < (kind == ds75 ? 0x10 : 0x08))
> +			goto exit_free;

It's smart :) I admit I had no clear idea how to implement the
detection efficiently. I like your approach.

>  
>  		/* Unused bits */
>  		if (conf & 0xe0)
>  		 	goto exit_free;

Note that this requires the DS75 to be in 9-bit resolution mode. The
dump you provided suggests your DS75 is in 10-bit resolution mode, so I
expect it not to be detected properly. Maybe this is OK as a first
approximation, but you may want to submit a second patch to properly
support the high resolution modes of the DS75.

>  
> -		/* Addresses cycling */
> -		for (i = 8; i < 0xff; i += 8)
> -			if (i2c_smbus_read_byte_data(new_client, i + 1) != conf
> -			 || i2c_smbus_read_word_data(new_client, i + 2) != hyst
> -			 || i2c_smbus_read_word_data(new_client, i + 3) != os)
> -				goto exit_free;
> -	}
> +		/* Addresses cycling - LM75 only*/

Missing space before end of comment.

> +		if (kind == lm75)
> +			for (i = 8; i < 0xff; i += 8)
> +				if (i2c_smbus_read_byte_data(new_client, i + 1) != conf
> +				    || i2c_smbus_read_word_data(new_client, i + 2) != hyst
> +				    || i2c_smbus_read_word_data(new_client, i + 3) != os)
> +					goto exit_free;

I strongly suggest adding a pair of curly braces for the outer-most if
statement. Three nested statements without any curly braces is asking
for trouble on any further change.

>  
> -	/* Determine the chip type - only one kind supported! */
> -	if (kind <= 0)
> -		kind = lm75;
> +	}

This change breaks the case kind == 0, you'll end up with an empty
name. You need to force the type to one of the supported ones if the
user uses force without a kind.

>  
> +	/* Determine the chip type */
>  	if (kind == lm75) {
>  		name = "lm75";
>  	}
> +	else if (kind == ds75) {
> +		name = "ds75";
> +	}
>  
>  	/* Fill in the remaining client fields and put it into the global list */
>  	strlcpy(new_client->name, name, I2C_NAME_SIZE);
> 

Can you also please update Documentation/hwmon/lm75 to mention the
separate prefix?

Thanks,
-- 
Jean Delvare