[U-Boot] [PATCH 4/7] S3C24XX: Add RTC driver

["José Miguel Gonçalves" <jose.goncalves@inov.pt>]

Hi Marek,

On 09/12/2012 10:03 PM, Marek Vasut wrote:
> Dear Jos? Miguel Gon?alves,
>
>> +static inline void rtc_access_disable(void)
>> +{
>> +	s3c24xx_rtc *const rtc = s3c24xx_get_base_rtc();
>> +	uchar rtccon;
>> +
>> +	rtccon = readb(&rtc->rtccon);
>> +	rtccon &= ~0x01;
> Magic numbers, fix globally in the patchset

OK.

>> +	writeb(rtccon, &rtc->rtccon);
>> +}
>> +
>> +/*
>> -------------------------------------------------------------------------
>> */ +
>> +int rtc_get(struct rtc_time *tmp)
>> +{
>> +	s3c24xx_rtc *const rtc = s3c24xx_get_base_rtc();
>> +	uchar sec, min, hour, mday, wday, mon, year;
>> +	int have_retried = 0;
>> +
>> +	rtc_access_enable();
>> +
>> +	/* Read RTC registers */
>> +retry_get_time:
>> +	min = readb(&rtc->bcdmin);
>> +	hour = readb(&rtc->bcdhour);
>> +	mday = readb(&rtc->bcddate);
>> +	wday = readb(&rtc->bcdday);
>> +	mon = readb(&rtc->bcdmon);
>> +	year = readb(&rtc->bcdyear);
>> +	sec = readb(&rtc->bcdsec);
>> +
>> +	/* The only way to work out whether the RTC was mid-update
>> +	 * when we read it is to check the seconds counter.
>> +	 * If it's zero, then we re-try the entire read.
>> +	 */
> Wrong multiline comment style ... use ./tools/checkpatch.pl before resending

OK.

>
>> +	if (rtc->bcdsec == 0 && !have_retried) {
> I'm sure you can avoid the goto here ...besides, this rtc->bcdsec doesn't make
> much sense

I could do it with a do..while loop instead... this part was copied from 
the linux driver.
And there is a bug... instead of 'rtc->bcdsec' it should be 'sec'!


Regards,
Jos? Gon?alves