[U-Boot] [PATCH v2 1/9] sunxi: initial sun7i clocks and timer support.

[Marek Vasut]

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