From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?ISO-8859-1?Q?Jos=E9_Miguel_Gon=E7alves?= <jose.goncalves@inov.pt>
Date: Thu, 13 Sep 2012 00:28:32 +0100
Subject: [U-Boot] [PATCH 4/7] S3C24XX: Add RTC driver
In-Reply-To: <201209122303.35665.marex@denx.de>
References: <1347448523-19565-1-git-send-email-jose.goncalves@inov.pt>
	<1347448523-19565-5-git-send-email-jose.goncalves@inov.pt>
	<201209122303.35665.marex@denx.de>
Message-ID: <50511AA0.3030104@inov.pt>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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