[i2c] [PATCH] i2c: fix broken ds1337 initialization

Dirk Eibach eibach at gdsys.de
Thu Nov 30 13:31:38 CET 2006


> Hi Dirk,
Hi Jean,
> 
> On Wed, 29 Nov 2006 08:12:59 +0100, Dirk Eibach wrote:
>> On a custom board with ds1337 RTC I found that upgrade from 2.6.15 to
>> 2.6.18 broke RTC support.
> 
> Oops, that's bad :(
> 
>> The main problem are changes to ds1337_init_client().
>> When a ds1337 recognizes a problem (e.g. power or clock failure) bit 7
>> in status register is set. This has to be reset by writing 0 to status
>> register. But since there are only 16 byte written to the chip and the
>> first byte is interpreted as an address, the status register (which is
>> the 16th) is never written.
> 
> Good catch.
> 
>> The other problem is, that initializing all registers to zero is not
>> valid for day, date and month register. Funny enough this is checked by
>> ds1337_detect(), which depends on this values not being zero. So then
>> treated by ds1337_init_client() the ds1337 is not detected anymore,
>> whereas the failure bit in the status register is still set.
> 
> Is it really a problem in practice? I'd expect the chip to bump to the
> next valid value once it is properly started, isn't it the case? Your
> proposed change makes sense either way.

We did some debugging that clearly showed that at least our chip (ds1339)
held the invalid wrong values without being ashamed ;)

>> This patch applies to kernel 2.6.18.
> 
> Actually, no, it doesn't, because your mailer broke it. Please submit
> it again and make sure it can still be applied afterwards. text/plain
> attachement is OK for me if you can't make it inline.

Why do  all graphical mailclients have to be full of shit(TM)? :(
OK. Attached it. Try migrating to mutt in my spare time.

> 
>> Signed-off-by: Dirk Stieler <stieler at gdsys.de>
>> Signed-off-by: Dirk Eibach <eibach at gdsys.de>
>> ---
>> --- linux-2.6.18-denx-git.new/drivers/i2c/chips/ds1337.c	2006-10-03
>> 20:15:48.000000000 +0200
>> +++ linux-2.6.18-denx-git/drivers/i2c/chips/ds1337.c	2006-11-28
>> 12:53:31.210765674 +0100
>> @@ -385,13 +385,19 @@ static void ds1337_init_client(struct i2
>>
>>    	if ((status & 0x80) || (control & 0x80)) {
>>    		/* RTC not running */
>> -		u8 buf[16];
>> +		u8 buf[17]; /* First byte is interpreted as address */
> 
> It might be appropriate to spell it 1+16 to be even clearer.

ACK.
> 
>>    		struct i2c_msg msg[1];
>>
>>    		dev_dbg(&client->dev, "%s: RTC not running!\n", __FUNCTION__);
>>
>>    		/* Initialize all, including STATUS and CONTROL to zero */
>>    		memset(buf, 0, sizeof(buf));
>> +
>> +		/* Write valid values in the date/time registers */
>> +		buf[4] = 1; /* Day */
>> +		buf[5] = 1; /* Date */
>> +		buf[6] = 1; /* Month */
> 
> What about using 1+DS1337_REG_DAY, 1+DS1337_REG_DATE and
> 1+DS1337_REG_MONTH instead of hardcoding 4, 5 and 6 respectively? The
> comments nearby would no longer be needed, and it's always better to
> use symbols instead of raw numbers where possible.

ACK.

> 
>> +
>>    		msg[0].addr = client->addr;
>>    		msg[0].flags = 0;
>>    		msg[0].len = sizeof(buf);
> 
> Thanks,

Cheers
-- 
Dirk Eibach <eibach at gdsys.de>
Guntermann & Drunck Systementwicklung GmbH
F & E
Dortmunder Str. 4a, D-57234 Wilnsdorf
Tel.: +49 (0) 2739 8901 100, Fax.: +49 (0) 2739 8901 120
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: ds1337.patch
Url: http://lists.lm-sensors.org/pipermail/i2c/attachments/20061130/950336a3/attachment.pl 


More information about the i2c mailing list