Re: [PATCH 1/2]: Add support for Dallas/Maxim DS1685/1687 RTC

[Kumba <kumba@gentoo.org>]

On 02/17/2011 06:43, Lars-Peter Clausen wrote:
> That is what I meant. Pass the return value of rtc_valid_tm on, instead of
> setting the time to 0 and pretend everything went fine.
> You can still keep the dev_err though, no problem with that.

Well, that particular return is in the while loop to check for an access 
lockout.  I'll be honest, I got that little bit out of a driver originally 
written for SGI Octane systems running Linux (an unofficial patch that has since 
suffered severe bitrot).

I know what bit to check to avoid an access lockout, but the DS1685 manual 
offers several options, and not being a hardware person, I'm not real sure how 
to represent them in code.

Here's what it says:

> There are three methods that can handle access of the RTC that avoid any
> possibility of accessing inconsistent time and calendar data. The first method
> uses the update-ended interrupt. If enabled, an interrupt occurs after every
> update cycle that indicates that over 999ms is available to read valid time and
> date information. If this interrupt is used, the IRQF bit in Register C should
> be cleared before leaving the interrupt routine.
>
> A second method uses the UIP bit in Register A to determine if the update cycle
> is in progress. The UIP bit pulses once per second. After the UIP bit goes
> high, the update transfer occurs 244μs later. If a low is read on the UIP bit,
> the user has at least 244μs before the time/calendar data is changed.
> Therefore, the user should avoid interrupt service routines that would cause
> the time needed to read valid time/calendar data to exceed 244μs.
>
> The third method uses a periodic interrupt to determine if an update cycle is
> in progress. The UIP bit in Register A is set high between the setting of the
> PF bit in Register C (Figure 4). Periodic interrupts that occur at a rate of
> greater than tBUC allow valid time and date information to be reached at each
> occurrence of the periodic interrupt. The reads should be complete within
> (tPI / 2 + tBUC) to ensure that data is not read during the update cycle.

I believe the driver is currently using #2.  Other drivers (rtc-sh.c) appear to 
use a do/while loop and check a bit that I assume is functionally equivalent to 
UIP here, but I'm not sure if that is accurate or not (and I have not dug up the 
manual for whatever chip rtc-sh.c touches).

What's the best approach to use here to avoid an access lockout? Should I run 
into an access lockout, what's the best way to handle that?


> What do you mean by 'quickly turning around and writing the values back'?
> The rtc_time struct passed to the set_time callback is supposed to contain only
> valid values.

The way I wrote ds1685_rtc_set_time copies the values from rtc_time out to local 
variables, runs the checks you say I need to remove, then if those pass, writes 
those values to the RTC registers.  I guess what your saying is that the checks 
are unnecessary because rtc_time has already been checked, so I'm just 
duplicating work and can write the values straight to the RTC registers?


> Well, if it is going to be shared it should probably remain somewhere in
> include/, but everything thats not shared should be moved to rtc-ds1685.c like
> for example ds1685_priv.

I think for now, moving everything into drivers/rtc works best.  I'll work from 
the linux-mips end on how IP32 needs to access this header file when I get that far.


> I don't know what you are trying to do, but the current code is extremely
> unreadable.
> You have all those variables declared in your functions which are on first
> sight not used, because they are only referenced from the macros. Furthermore
> the invocation of the macro has not the syntax of a function call, although
> semantically that is what it is.
> And especially ds1685_rtc_begin_data_access is dangerous, because of the
> 'return 1', there is no indication when you read the code that a function could
> magically exit upon invoking that macro.

Well, I thought I was being fancy and cute, but that's what code review is all 
about.  I'm working on breaking these up into inlinable functions and leave that 
bit of work up to the compiler.


> Why? The compiled code will probably be exactly the same as now.

Per above, if I can re-work the RTC access loop, I can eliminate this one bit 
and then this concern becomes moot.


Thanks!,

-- 
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org

"The past tempts us, the present confuses us, the future frightens us.  And our 
lives slip away, moment by moment, lost in that vast, terrible in-between."

--Emperor Turhan, Centauri Republic