[U-Boot-Users] [PATCH 1/3] Apollon BSP support

[Wolfgang Denk <wd@denx.de>]

In message <20070914005131.GA14660@party> you wrote:
> Apollon BSP support
> 
> The Apollon based on OMAP2420 is designed for OneNAND development.
> It's similar with OMAP2420 H4 except some peripherals.
...
...
> +	MuxConfigReg = (volatile uint8 *)CONTROL_PADCONF_USB0_PUEN;
> +	*MuxConfigReg &= (uint8) (~0x1F);
> +
> +	MuxConfigReg = (volatile uint8 *)CONTROL_PADCONF_USB0_VP;
> +	*MuxConfigReg &= (uint8) (~0x1F);
> +
> +	MuxConfigReg = (volatile uint8 *)CONTROL_PADCONF_USB0_VM;
> +	*MuxConfigReg &= (uint8) (~0x1F);
> +
> +	MuxConfigReg = (volatile uint8 *)CONTROL_PADCONF_USB0_RCV;
> +	*MuxConfigReg &= (uint8) (~0x1F);
> +
> +	MuxConfigReg = (volatile uint8 *)CONTROL_PADCONF_USB0_TXEN;
> +	*MuxConfigReg &= (uint8) (~0x1F);
> +
> +	MuxConfigReg = (volatile uint8 *)CONTROL_PADCONF_USB0_SE0;
> +	*MuxConfigReg &= (uint8) (~0x1F);
> +
> +	MuxConfigReg = (volatile uint8 *)CONTROL_PADCONF_USB0_DAT;
> +	*MuxConfigReg &= (uint8) (~0x1F);
...

You use this sequence extensively. In the current form, it's extremly
lengthy, difficult to read and difficult to maintain.

Please define an (inline?) fintion which performs the actions and
allows you to change (for example)

> +	/* V19 */
> +	MuxConfigReg = (volatile uint8 *)CONTROL_PADCONF_USB1_RCV;
> +	*MuxConfigReg = 1;
> +	/* W20 */
> +	MuxConfigReg = (volatile uint8 *)CONTROL_PADCONF_USB1_TXEN;
> +	*MuxConfigReg = 1;
> +	/* N14 */
> +	MuxConfigReg = (volatile uint8 *)CONTROL_PADCONF_GPIO69;
> +	*MuxConfigReg = 3;
> +	/* P15 */
> +	MuxConfigReg = (volatile uint8 *)CONTROL_PADCONF_GPIO70;
> +	*MuxConfigReg = 3;

into

	write_config_reg (CONTROL_PADCONF_USB1_RCV, 1);		/* V19 */
	write_config_reg (CONTROL_PADCONF_USB1_TXEN, 1);	/* W20 */
	write_config_reg (CONTROL_PADCONF_GPIO69, 3);		/* N14 */
	write_config_reg (CONTROL_PADCONF_GPIO70, 3);		/* P15 */

This is IMO *much* easier to read and understand.

Please  also  note  that  identifiers  like  "MuxConfigReg"  are   in
violation with the Coding Style:

    C is a Spartan language, and so should your naming be.  Unlike Modula-2
    and Pascal programmers, C programmers do not use cute names like
    ThisVariableIsATemporaryCounter.  A C programmer would call that
    variable "tmp", which is much easier to write, and not the least more
    difficult to understand.


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
"Virtual" means never knowing where your next byte is coming from.