[U-Boot] [PATCH 2/3] A320: driver for FTRTC010 real time clock

[Detlev Zundel]

Hi Po-Yu Chuang,

> Dear Jean-Christophe PLAGNIOL-VILLARD,
>
> 2009/6/24 Jean-Christophe PLAGNIOL-VILLARD <plagnioj at jcrosoft.com>:
>>> +
>>> +static volatile struct ftrtc010 *rtc = (struct ftrtc010 *)CONFIG_SYS_RTC_BASE;
>>> +
>>> +static void ftrtc_enable (void)
>> you use it at one please only the reset
> Sorry, I don't understand what do you mean

I guess he means that he doesn't like functions which are only called
from one place - he'd rather like to see the code directly at the
callers site.

However I do not agree here - functions have also a documentation
effect, so I vote to keep the function.  If neccessary, we could declare
the function inline, but this is really overkill here.

>>> +{
>>> +     rtc->cr = cpu_to_le32 (FTRTC010_CR_ENABLE);
>> so please move this code there
>>> +}
>>> +
>>> +/*
>>> + * return current time in seconds
>>> + */
>>> +static unsigned long ftrtc_time (void)
>>> +{
>>> +     unsigned long day;
>>> +     unsigned long hour;
>>> +     unsigned long minute;
>>> +     unsigned long second;
>>> +     unsigned long second2;
>>> +
>>> +     do {
>>> +             second  = le32_to_cpu (rtc->sec);
>> please use proper accessor
>> readl/writel
> Should I use
> second = readl(&rtc->sec);

Yep.

Cheers
  Detlev

-- 
BUGS:
  It is not yet possible to change operating system by writing to
  /proc/sys/kernel/ostype.
                                     -- Linux sysctl(2) man page
--
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