On Friday, March 28, 2014 at 09:25:40 AM, Hans de Goede wrote:
Hi,
On 03/28/2014 09:20 AM, Ian Campbell wrote:
On Thu, 2014-03-27 at 23:36 +0100, Marek Vasut wrote:
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.
Sorry, but I think inlining the readl (and along with it the interesting/subtle) inverting functionality is a terrible idea, it should be wrapped in some sort of accessor precisely because it is not a raw readl.
I'm going to make it a function as I suggested.
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.
What people? What plumbing? I've no idea what you are concerned about here.
I think what Marek is concerned about is people making some global u-boot change which for some reason requires fixing up a bunch of platform specific files, and they end up touching our timer.c without ever test-compiling it.
We don't do such changes without test-compiling those (most of us don't and those who do ... well ... tsk tsk tsk ! ;-) ).
These kind of things happen in qemu / the kernel too. In this case they could move a READ_TIMER() somewhere where the timer_base is not defined.
Yeah.
Your suggestion of making it a proper function will fix that though, and I think is the best solution.
I think wrapping a plain readl() into a function is not necessary, but I will wait for V3 to see it in action and then I will decide .