On Thu, Nov 29, 2012 at 11:41:50AM -0700, Stephen Warren wrote:

On 11/29/2012 01:44 AM, Thierry Reding wrote:

quoted

On Mon, Nov 26, 2012 at 03:19:08PM +0200, Terje Bergstrom wrote:

quoted

quoted

diff --git a/drivers/video/tegra/host/host1x/host1x_intr.c

b/drivers/video/tegra/host/host1x/host1x_intr.c

[...]

quoted

+/* 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