[U-Boot] [PATCH v2 4/7] rtc: add support for 4543 RTC (manufactured by e.g. EPSON)

[Detlev Zundel]

Hi Anatolij,

> some more small style issues, see comments below.
>
> Detlev Zundel wrote:
>
>> diff --git a/drivers/rtc/rtc4543.c b/drivers/rtc/rtc4543.c
>> new file mode 100644
>> index 0000000..242d9bc
>> --- /dev/null
>> +++ b/drivers/rtc/rtc4543.c
>> @@ -0,0 +1,118 @@
> ...
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> ----------------------------------------------------------^
> please, remove space here.

Wow, you want me to change the default GPL boiler plate?  You've got
guts ;)

> <snip>
>> +/*
>> + * Note: The acrobatics below is due to the hideously ingenius idea of
>> + * the chip designers.  As the chip does not allow register
> -------------------------^
> please, remove space here.
>
>> + * addressing, all values need to be read and written in one go.  Sure
> -------------------------------------------------------------------^
> please, remove space here.

As said by Wolfgang, you may infer that I use Emacs as my preferred text
editor, which knows about the double space.  Citing from the info file:

     The sentence commands assume that you follow the American typist's
  convention of putting two spaces at the end of a sentence; they consider
  a sentence to end wherever there is a `.', `?' or `!' followed by the
  end of a line or two spaces, with any number of `)', `]', `'', or `"'
  characters allowed in between.  A sentence also begins or ends wherever
  a paragraph begins or ends.  It is useful to follow this convention,
  because it makes a distinction between periods that end a sentence and
  periods that indicate abbreviations; that enables the Emacs sentence
  commands to distinguish, too.  These commands do not stop for periods
  that indicate abbreviations.

So I really ask you to reconsider your critique.
  
>> +int rtc_get(struct rtc_time *tm)
>> +{
>> +	int rel = 0;
>> +	uchar buffer[7];
>> +
>> +	memset(buffer, 0, 7);
>> +
>> +	/* Read 52 bits into our buffer */
>> +	tws_read(buffer, 52);
>> +
>> +	tm->tm_sec  = BCD2BIN( buffer[0] & 0x7F);
>> +	tm->tm_min  = BCD2BIN( buffer[1] & 0x7F);
>> +	tm->tm_hour = BCD2BIN( buffer[2] & 0x3F);
>> +	tm->tm_wday = BCD2BIN( buffer[3] & 0x07);
> ------------------------------^
> please, remove space here.

Are you sure?  You may notice that the spaces are intentional and
*actually improve* the readability.

>> +	tm->tm_mday = BCD2BIN((buffer[3] & 0xF0) >> 4 | (buffer[4] & 0x0F) << 4);
>> +	tm->tm_mon  = BCD2BIN((buffer[4] & 0x30) >> 4 | (buffer[5] & 0x0F) << 4);
>> +	tm->tm_year = BCD2BIN((buffer[5] & 0xF0) >> 4 | (buffer[6] & 0x0F) << 4) + 2000;
>
> these tree lines above are too long.

Oh well.  I really checked CodingStyle in the Linux kernel and it has
this to say:

  The only exception to this is where exceeding 80 columns significantly
  increases readability and does not hide information.

So please reconsider your critique with this sentence in mind.  What do
you think?  

> ...
>> diff --git a/include/rtc.h b/include/rtc.h
>> index 785fbe3..019c2eb 100644
>> --- a/include/rtc.h
>> +++ b/include/rtc.h
>> @@ -61,4 +61,8 @@ void to_tm (int, struct rtc_time *);
>>  unsigned long mktime (unsigned int, unsigned int, unsigned int,
>>  		      unsigned int, unsigned int, unsigned int);
>>  
>> +uchar rtc_read(uchar reg) __attribute__((weak));
>> +void  rtc_write(uchar reg, uchar val) __attribute__((weak));
>> +
>> +
>
> remove one blank line here, please.

Ok, I don't have a problem with that.  I will not fix however, because I
actually realize that this is another leftover of the previous
implementation which did not use the tws protocol driver, so I remove
the lines entirely.

Cheers
  Detlev

-- 
Those who would give up essential liberty to purchase a little
temporary safety, deserve neither liberty nor safety.
                                     -- Benjamin Franklin
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de