On Thu, Nov 29, 2012 at 11:41:50AM -0700, Stephen Warren wrote:
On 11/29/2012 01:44 AM, Thierry Reding wrote:
On Mon, Nov 26, 2012 at 03:19:08PM +0200, Terje Bergstrom wrote:
diff --git a/drivers/video/tegra/host/host1x/host1x_intr.c b/drivers/video/tegra/host/host1x/host1x_intr.c
[...]
+/* Spacing between sync registers */ +#define REGISTER_STRIDE 4
Erm... no. The usual way you should be doing this is either make the register definitions account for the stride or use accessors that apply the stride. You should be doing the latter anyway to make accesses. For example:
static inline void host1x_syncpt_writel(struct host1x *host1x, unsigned long value, unsigned long offset) { writel(value, host1x->regs + SYNCPT_BASE + offset); }
static inline unsigned long host1x_syncpt_readl(struct host1x *host1x, unsigned long offset) { return readl(host1x->regs + SYNCPT_BASE + offset); }
Alternatively, if you want to pass the register index instead of the offset, you can use just multiply the offset in that function:
writel(value, host1x->regs + SYNCPT_BASE + (offset << 2));
The same can also be done with the non-syncpt registers.
It seems like reasonable documentation to replace "<< 2" with "* REGISTER_STRIDE" here.
Given that it is a very common pattern, << 2 seems enough documentation to me, but sure, if you prefer to be extra explicit that's fine with me.
Thierry