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. Ian. -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.