[U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite

[Wolfgang Denk]

Dear Graeme Russ,

In message <1309261269-4363-1-git-send-email-graeme.russ at gmail.com> you wrote:
> The following series is a work-in-progress revamp of the timer API. The aim
> is to create a new userland API consisting of the following functions
> (along with a few arch level support functions):

I have to apologize for not commenting on all of this for such a long
time.  There are a number of reeasons for this silence - lack of
available time being one of them (and probably the most pressing one),
but I also needed some time to lean back and think through all of
this.

One thing I always wanted to do in the previous discussion was to check
what other projects (like Linux, barebox, etc.) are doing in this area.
I think it is worth reading the Linux Documentation/timers/highres.txt
document, especially the sections about John Stultz's Generic Time Of
Day (GTOD) framework, please the documents linked there (i. e. the OLS
slides [the link in Documentation/timers/highres.txt is stale; use
http://www.kernel.org/pub/linux/kernel/people/tglx/hrtimers/ols2006-hrtimers.pdf
instead] and the paper "We Are Not Getting Any Younger: A New Approach
to Time and Timers" by J. Stultz et al. in
http://www.linuxsymposium.org/2005/linuxsymposium_procv1.pdf p. 219ff).

Having this still in mind, I took a look across the fence to what
barebox is doing.  Guess what?  From "common/clock.c":

	 * clock.c - generic clocksource implementation
	 *
	 * This file contains the clocksource implementation from the Linux
	 * kernel originally by John Stultz

OK.  Message received.

What I'm asking myself (and now you) is: Should we really re-invent
the wheel again?


Regarding the function names:

> u32 time_now_ms(void);
> u32 time_since_ms(u32 from, u32 to);
> u32 time_max_since_ms(u32 from, u32 to);

I don't like these.  They appear wrong to me, and they are not in sync
with the wiki page either.

First, I would very much like to get rid of this "_ms" thing.  We
should rather make very clear in the documentation which unit the time
services are based on, and use this consequently.  Only functions
using a different unit should make this clean in their names.


Second, I don't feel well with "time_now()" - what is this, what does
it do?  Does it set or get a time?  The "get_time()" suggestion we
discussed earlier feels still much better to me (as it clearly says
which operation the function performs).  Or we could even follow the
example of the Unix system call time(2) and just use "time()".


The signature of time_since(from, to) is broken.  For "time since" it
seems logically to expect a single argument only: time_since(when).
The wiki page still lists this function as time_delta() which seems
way more logical to me [although I have to admit that I don;t
understand what the "delta_type" argument might be.


Third: all this time_*_max(), ..._min() and ...raw() stuff.  Aren't we
over-engineering here?  We have been successfully writing U-Boot code
for 11 years now, and never needed these before.  Neither does Linux
need any of those, nor every other project I'm aware of.  Come on,
let's keep the code small and efficient and do without these bells and
whistles.  Quote Antoine de Saint-Exupery: "Perfection is reached,
not when there is no longer anything to add, but when there is no
longer anything to take away."


> This current patch series migrates the users of the existing timer API
> consisting of get_timer() and reset_timer() to the new API while still
> retaining the arch specific framework in the background.

I feel bad that you already have spent so much work, and only now I
come and call everything into question again.  Sorry.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
How many NASA managers does it take to screw in a lightbulb?  "That's
a known problem... don't worry about it."