Dear Graeme Russ,
In message 1309261269-4363-1-git-send-email-graeme.russ@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... 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