On Thursday, March 27, 2014 at 11:12:38 PM, Ian Campbell wrote:
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.
That's just wrapping a readl() into another function, which seems unnecessary really.
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.
People plumbing in the timer.c file who are not aware the macro has a dependency which is not passed as it's parameter.
Best regards, Marek Vasut