Re: [lm-sensors] [PATCH] sensors-detect: Add detection of TI ADC128D818

[Guenter Roeck <linux@roeck-us.net>]

On 01/26/2014 05:22 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Sat, 25 Jan 2014 16:13:51 -0800, Guenter Roeck wrote:
>> On 01/25/2014 02:05 PM, Jean Delvare wrote:
>>> On Sat, 25 Jan 2014 09:54:51 -0800, Guenter Roeck wrote:
>>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>>> ---
>>>>    CHANGES                    |    3 +++
>>>>    prog/detect/sensors-detect |   26 +++++++++++++++++++++-----
>>>>    2 files changed, 24 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/CHANGES b/CHANGES
>>>> index e1347a6..970dd67 100644
>>>> --- a/CHANGES
>>>> +++ b/CHANGES
>>>> @@ -1,6 +1,9 @@
>>>>    lm-sensors CHANGES file
>>>>    -----------------------
>>>>
>>>> +SVN HEAD
>>>> +  sensors-detect: Add detection of ADC128D818
>>>> +
>>>>    3.3.5 "Happy Birthday Beddy" (2014-01-22)
>>>>      libsensors: Improve documentation of two functions
>>>>                  Increase MAX_SENSORS_PER_TYPE to 33
>>>> diff --git a/prog/detect/sensors-detect b/prog/detect/sensors-detect
>>>> index a2093f3..2136b76 100755
>>>> --- a/prog/detect/sensors-detect
>>>> +++ b/prog/detect/sensors-detect
>>>> @@ -547,6 +547,11 @@ use vars qw(@i2c_adapter_names);
>>>>    		i2c_addrs => [0x28..0x2f],
>>>>    		i2c_detect => sub { lm80_detect(@_, 1); },
>>>>    	}, {
>>>> +		name => "TI / National Semiconductor ADC128D818",
>>>> +		driver => "adc128d818",
>>>> +		i2c_addrs => [0x1d, 0x1e, 0x1f, 0x2d, 0x2e, 0x2f, 0x35, 0x36, 0x37],
>>>> +		i2c_detect => sub { lm80_detect(@_, 2); },
>>>
>>> How does this relate to the LM80?
>>>
>>> Is this chip really something people will need sensors-detect for (i.e.
>>> it is found in PC-like computers) or something which will be always
>>> declared in DT-like declarations anyway?
>>
>> No idea, really. I was asked by someone from TI to write a driver some time ago,
>> and finally found the time to actually do it. I recall they had a request
>> from a customer, but I did not ask for the use case.
>>
>> The chip is a close relative to LM80 and LM96080. Register addresses are
>> almost the same, but the sensor registers are 16 bit wide instead of 8,
>> and there are no fan registers. I didn't use the lm80 driver because of
>> the 16 bit registers, but decided to write a new one.
>
> Then I suggest using a different detect function in sensors-detect.
> There isn't enough in common with detect_lm80 to reuse it, and having a
> 1:1 mapping between sensors-detect functions and drivers has advantages.
>
Ok.

>>> I'm asking because I am worried about the address list. I'm fine with
>>> 0x1d..0x1f and 0x2d..0x2f (you can declare them that way in perl BTW,
>>> it's more compact) but addresses 0x35..0x37 we don't currently scan.
>>> Adding a new address to the scan list is always a risk, as you may start
>>> probing a whole new class of devices and the effects can be very bad.
>>>
>>> Actually we used to scan address 0x37 until r3233 / 2006-01-16. Commit
>>> message was:
>>>
>>> "Lower the confidence of ITE overclocking chips. Do not scan
>>> address 0x37 for these chips, as it may cause problem with some
>>> eeproms."
>>>
>>> Addresses 0x35 and 0x36 I don't think we ever scanned, but EEPROMs can
>>> reply to these as well so I'd rather not add them.
>>
>> Ok with me to take out those three addresses ... or drop the entire detection
>> if you think it isn't worth it.
>
> I am fine with the detection function. Please just drop the 3 addresses
> in the 0x30-0x37 range.
>
Ok.

>> Question is what I should do in the driver. Any thoughts on that ?
>> For my part I am fine with dropping the detect function entirely from it;
>> I suspect you are right and it will mostly be used from DT type systems,
>> and from what you said it sounds a bit risky to scan the 0x35..0x37
>> address range.
>
> You can keep the detect function in the driver as long as you remove
> the 0x35..0x37 addresses from the scan list. Just document it properly
> and that should be fine.
>
> About the code itself:
>
>> @@ -4436,11 +4441,13 @@ sub lm92_detect
>>   # Chip to detect: 0 = LM80, 1 = LM96080
>>   # Registers used:
>>   #   0x00: Configuration register
>> -#   0x02: Interrupt state register
>> -#   0x07: Converstion rate register (LM96080 only)
>> +#   0x02: Interrupt state register (LM80, LM96080 only)
>> +#   0x07: Converstion rate register (LM96080, ADC128D818 only)
>> +#   0x08: Oneshot register (ADC128D818 only)
>> +#   0x09: Shutdown register (ADC128D818 only)
>
> The datasheet I have states that 0x09 is the One-Shot register and 0x0a
> is the Deep Shutdown register.
>
Funny, in the driver I got that right :-)

>>   #   0x2a-0x3d: Limits registers (LM80 only)
>> -#   0x3e: Manufacturer's ID register (LM96080 only)
>> -#   0x3f: Stepping/die revision ID register (LM96080 only)
>> +#   0x3e: Manufacturer's ID register (LM96080, ADC128D818 only)
>> +#   0x3f: Stepping/die revision ID register (LM96080, ADC128D818 only)
>>   # The LM80 is easily misdetected since it doesn't provide identification
>>   # registers. So we have to use some tricks:
>>   #   - 6-bit addressing, so limits readings modulo 0x40 should be unchanged
>> @@ -4458,9 +4465,9 @@ sub lm80_detect
>>   	my ($i, $reg);
>>
>>   	return if (i2c_smbus_read_byte_data($file, 0x00) & 0x80) != 0;
>
> For the ADC128D818 you can even mask with 0xf4, slightly improving the
> reliability.
>
Ok.

>> -	return if (i2c_smbus_read_byte_data($file, 0x02) & 0xc0) != 0;
>>
>>   	if ($chip = 0) {
>> +		return if (i2c_smbus_read_byte_data($file, 0x02) & 0xc0) != 0;
>>   		for ($i = 0x2a; $i <= 0x3d; $i++) {
>>   			$reg = i2c_smbus_read_byte_data($file, $i);
>>   			return if i2c_smbus_read_byte_data($file, $i+0x40) != $reg;
>> @@ -4493,11 +4500,20 @@ sub lm80_detect
>>   		return unless $confidence > 0;
>>   		return $confidence;
>>   	} elsif ($chip = 1) {
>> +		return if (i2c_smbus_read_byte_data($file, 0x02) & 0xc0) != 0;
>>   		return if (i2c_smbus_read_byte_data($file, 0x07) & 0xfe) != 0;
>>   		return if i2c_smbus_read_byte_data($file, 0x3e) != 0x01;
>>   		return if i2c_smbus_read_byte_data($file, 0x3f) != 0x08;
>>
>>   		return 6;
>> +	} elsif ($chip = 2) {
>> +		return if (i2c_smbus_read_byte_data($file, 0x07) & 0xfe) != 0;
>> +		return if (i2c_smbus_read_byte_data($file, 0x08) & 0xfe) != 0;
>> +		return if (i2c_smbus_read_byte_data($file, 0x09) & 0xfe) != 0;
>
> You could also check register 0x0c, which as 6 unused bits.
>
As well as 0x0b.

>> +		return if i2c_smbus_read_byte_data($file, 0x3e) != 0x01;
>> +		return if i2c_smbus_read_byte_data($file, 0x3f) != 0x09;
>> +
>> +		return 6;

If I do all that, is that worth a higher rating ?

Thanks,
Guenter


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