On Thu, 2014-03-27 at 23:00 +0100, Marek Vasut wrote:
On Thursday, March 27, 2014 at 10:29:56 PM, Ian Campbell wrote:
On Mon, 2014-03-24 at 21:52 +0100, Marek Vasut wrote:
+static struct sunxi_timer *timer_base =
&((struct sunxi_timer_reg *)SUNXI_TIMER_BASE)->timer[TIMER_NUM];+/* macro to read the 32 bit timer: since it decrements, we invert read value */ +#define READ_TIMER() (~readl(&timer_base->val))
This macro has to go, just use ~readl() in place. But still, why do you use that negation in "~readl()" anyway ?
The comment right above it explains why: the timer counts backwards and inverting it accounts for that.
This is subtle enough that I don't think using ~readl() in place in the 3 callers would be an improvement.
Please do it, we don't want any implementers down the line using this "READ_TIMER()" call and getting hit by "timer_base undefined" . That macro hides the dependency on this symbol, while if you expanded it in-place, the dependency would be explicit. I really do want to see that macro gone, sorry.
How about a static inline instead of the macro? I'm thinking with a body: { struct sunxi_timer *timers = (struct sunxi_timer_reg *)SUNXI_TIMER_BASE; return timers[TIMER_NUM]->val; } With something similar in timer_init then both the macro and the static global timer_base can be dropped.
BTW this macro is in arch/arm/cpu/armv7/sunxi/timer.c not a header, so I'm not sure which implementers down the line you were worried about using it in some other context where it breaks.
Ian.