[U-Boot] [PATCH V3 09/11] ARM/PPC: add a common way to access registers

[Wolfgang Denk]

Dear Stefano Babic,

In message <1264008133-20906-1-git-send-email-sbabic at denx.de> you wrote:
> Some Freescale's processors of different architecture
> have the same peripheral (eSDHC controller in PowerPC
> and i.MX51). This patch adds neutral functions to access
> to the internal registers of the SOCs that can be used
> by both architectures.
> 
> Signed-off-by: Stefano Babic <sbabic at denx.de>
> ---
>  include/asm-arm/io.h |   39 +++++++++++++++++++++++++++++++++++++++
>  include/asm-ppc/io.h |   21 +++++++++++++++++++++
>  2 files changed, 60 insertions(+), 0 deletions(-)

Please document these new macros.  Please be explicit about the
behaviour of these macros on systems with different endianess.

> +/* Clear and set bits in one shot. These macros can be used to clear and
> + * set multiple bits in a register using a single call. These macros can
> + * also be used to set a multiple-bit bit pattern using a mask, by
> + * specifying the mask in the 'clear' parameter and the new bit pattern
> + * in the 'set' parameter.
> + */

Incorrect multi-line comment style.

> +#define clrbits(type, addr, clear) \
> +	write##type(__raw_read##type(addr) & ~(clear), (addr))
> +
> +#define setbits(type, addr, set) \
> +	write##type(__raw_read##type(addr) | (set), (addr))
> +
> +#define clrsetbits(type, addr, clear, set) \
> +	write##type((__raw_read##type(addr) & ~(clear)) | (set), (addr))
> +
> +#define write_reg(type,a,v)	write##type(v,a)
> +#define read_reg(type,a)	__raw_read##type(a)
> +
> +#define write_reg32(a,v)	write_reg(l,a,v)
> +#define write_reg16(a,v)	write_reg(w,a,v)
> +#define write_reg8(a,v)		write_reg(b,a,v)
> +
> +#define read_reg32(a)		read_reg(l,a)
> +#define read_reg16(a)		read_reg(w,a)
> +#define read_reg8(a)		read_reg(b,a)

What exactly is the definition of a "register" here? Is this memory
mapped I/O?

and - should we really invent our own, private way of doing this?
Should we not rather reuse definitions already present in Linux?

How about using ioread*()/iowrite*() instead?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The Gates in my computer are AND, OR and NOT; they are not Bill.