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

[Anatolij Gustschin <agust@denx.de>]

Detlev Zundel wrote:

>>> + * 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 ;)

I checked some other files before suggesting to fix, and accidentally
caught the files with one space character here. Now running

[ag at wker u-boot]$ fgrep -r "PURPOSE. See" * | wc -l
151
[ag at wker u-boot]$ fgrep -r "PURPOSE.  See" * | wc -l
3530

shows, that these are outnumbered. So, forget it. Sorry for the noise.

>> <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.

Ok, your and Wolfgang's arguments show me, that being consistent with
something is often brain-damaged. Sorry for the noise.
   
>>> +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.

OK.

> 
>>> +	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?

OK. Something like

	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;

isn't more readable than your version. Sorry for the noise, again.

Best regards,
Anatolij