Re: [PATCH] : Add support for NXP PNX833x (STB222/5) into linux kernel‏ (UPDATE)

[Ralf Baechle <ralf@linux-mips.org>]

On Thu, Jun 12, 2008 at 02:41:00PM +0200, Florian Fainelli wrote:

> Le Thursday 12 June 2008 14:29:47 Daniel Laird, vous avez écrit :
> > linux-2.6.26-rc4.orig/include/asm-mips/mach-pnx833x/gpio.h
> > linux-2.6.26-rc4/include/asm-mips/mach-pnx833x/gpio.h
> > +
> > +/* Initialize GPIO to a known state */
> > +static inline void pnx833x_gpio_init(void)
> > +{
> > +	PNX833X_PIO_DIR = 0;
> > +	PNX833X_PIO_DIR2 = 0;
> > +	PNX833X_PIO_SEL = 0;
> > +	PNX833X_PIO_SEL2 = 0;
> > +	PNX833X_PIO_INT_EDGE = 0;
> > +	PNX833X_PIO_INT_HI = 0;
> > +	PNX833X_PIO_INT_LO = 0;
> 
> It would be better if you for instance map a structure to your PIO registers, 
> like this :
> 
> struct pnx833x_pio_reg {
> 	u32	in;
> 	u32	out;
> 	[..]
> };
> 
> Then the gpio code would ioremap this registers like this in 
> pnx833x_gpio_init :
> 
> 	struct pnx833x_pio_reg *gpio_reg = ioremap_nocache(0xF00, sizeof(struct 
> pnx833x_gpio_reg));
> 	[..]
> 
> So that you could use writel/readl like this :
> 
> 	writel(0, &gpio_reg->in);
> 
> which looks nicer.

Actually for portable drivers the practice of using structs is discouraged.
The alignment rules are not the same on every architecture, so:

struct example {
	u16	var1;
	u32	var2;
};

Would have var2 at offset 2 on m68k but on offset 4 on MIPS.  So hardcoding
the offsets by some other means is preferable for portable code.  As for
this code which is meant to be MIPS only I'd not be religous but it's good
practice to try to always write portable code.

  Ralf