[U-Boot] [PATCH] use "boot selecr" jumper on NGW100 to select USART

[Wolfgang Denk <wd@denx.de>]

Dear Thomas Sprinkmeier,

In message <e355f1140911041955q1adf2c1fp75375e4b98cfdc00@mail.gmail.com> you wrote:
> Without the "boot select" jumper U-Boot will use the USART selected
> using the CONFIG_USART1, CONFIG_USART2, directive.

Please fix the typo in the Subject:

> If  CONFIG_ALT_USART1 (or ..2, ..3, ..0) is defined then,
> with the jumper in place, that USART is used instead.
> 
> Signed-off-by: Thomas Sprinkmeier <thomas.sprinkmeier@gmail.com>
> ---
>  board/atmel/atngw100/atngw100.c |   40 ++++++++++++++++++++++++++++
>  drivers/serial/atmel_usart.c    |   54 ++++++++++++++++++++++++++++++++++++++-
>  drivers/serial/atmel_usart.h    |    9 +++---
>  include/configs/atngw100.h      |    1 +
>  4 files changed, 98 insertions(+), 6 deletions(-)
> 
> diff --git a/board/atmel/atngw100/atngw100.c b/board/atmel/atngw100/atngw100.c
> index 004d8da..a2ffdb4 100644
> --- a/board/atmel/atngw100/atngw100.c
> +++ b/board/atmel/atngw100/atngw100.c
> @@ -53,7 +53,47 @@ int board_early_init_f(void)
>         hmatrix_slave_write(EBI, SFR, HMATRIX_BIT(EBI_SDRAM_ENABLE));
> 
>         portmux_enable_ebi(16, 23, 0, PORTMUX_DRIVE_HIGH);
> +
> +#if defined(CONFIG_ALT_USART0) || defined(CONFIG_ALT_USART1) || \
> +    defined(CONFIG_ALT_USART2) || defined(CONFIG_ALT_USART3)
> +#define USART_USE_ALT  (!gpio_get_value(GPIO_PIN_PB(30)))

Your patch is white space corrupted. All "original" indentation was by
TABs, but in your patch it's all spaces.

> +       /* PB30, the 'boot' jumper with external pull-up resistor */
> +       /* PORTMUX_DIR_INPUT == 0 as high-impedance input is the default state.
> +          In other words, I don't think this actually does anything... */

Incorrect multiline comment style. Please fix globally.

> +       if (USART_USE_ALT) {
> +#if   defined(CONFIG_ALT_USART0)
> +         portmux_enable_usart0(PORTMUX_DRIVE_MIN);
> +#elif defined(CONFIG_ALT_USART1)
> +         portmux_enable_usart1(PORTMUX_DRIVE_MIN);
> +#elif defined(CONFIG_ALT_USART2)
> +         portmux_enable_usart2(PORTMUX_DRIVE_MIN);
> +#elif defined(CONFIG_ALT_USART3)
> +         portmux_enable_usart3(PORTMUX_DRIVE_MIN);
> +#endif

Incorrect indentation. Please fix globally.


> +       } else {
> +#endif
> +#if   defined(CONFIG_USART0)
> +       portmux_enable_usart0(PORTMUX_DRIVE_MIN);
> +#elif defined(CONFIG_USART1)
>         portmux_enable_usart1(PORTMUX_DRIVE_MIN);
> +#elif defined(CONFIG_USART2)
> +       portmux_enable_usart2(PORTMUX_DRIVE_MIN);
> +#elif defined(CONFIG_USART3)
> +       portmux_enable_usart3(PORTMUX_DRIVE_MIN);
> +#else

If only one CONFIG_USARTx is allowed, you should think opf a way to
get rid of these long, repeated #if...#elif...#elif constructs.
> 
> -/* Register access macros */
> -#define usart3_readl(reg)                              \
> -       readl((void *)USART_BASE + USART3_##reg)
> -#define usart3_writel(reg,value)                       \
> -       writel((value), (void *)USART_BASE + USART3_##reg)
> +int atmel_serial_read(int reg);
> +void atmel_serial_write(int reg, int value);
> +#define usart3_readl(reg) atmel_serial_read(USART3_##reg)
> +#define usart3_writel(reg, value) atmel_serial_write(USART3_##reg, value)

Instead of making code more AT91 specific, we should try the opposite:
make it _less_ so.


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
I really hate this damned machine     It never does quite what I want
I wish that they would sell it.              But only what I tell it.