[U-Boot] [PATCH 1/4] gpio: atmel: fix code to use pointer for pio port

[Andreas Bießmann]

Hi Bo,

On 08/13/2013 08:38 AM, Bo Shen wrote:
> fix code to use pointer for pio port as the warning message suggested
> remove the warning message
> 
> Signed-off-by: Bo Shen <voice.shen at atmel.com>
> ---
>  drivers/gpio/at91_gpio.c |  232 ++++++++++++++++++++++++++--------------------
>  1 file changed, 134 insertions(+), 98 deletions(-)
> 
> diff --git a/drivers/gpio/at91_gpio.c b/drivers/gpio/at91_gpio.c
> index 2322914..15f396f 100644
> --- a/drivers/gpio/at91_gpio.c
> +++ b/drivers/gpio/at91_gpio.c
> @@ -8,16 +8,6 @@
>   * SPDX-License-Identifier:	GPL-2.0+
>   */
>  
> -/*
> - * WARNING:
> - *
> - * As the code is right now, it expects all PIO ports A,B,C,...
> - * to be evenly spaced in the memory map:
> - * ATMEL_BASE_PIOA + port * sizeof at91pio_t
> - * This might not necessaryly be true in future Atmel SoCs.
> - * This code should be fixed to use a pointer array to the ports.
> - */
> -
>  #include <config.h>
>  #include <common.h>
>  #include <asm/io.h>
> @@ -25,19 +15,52 @@
>  #include <asm/arch/hardware.h>
>  #include <asm/arch/at91_pio.h>
>  
> +static unsigned at91_pio_get_port(unsigned port)
> +{
> +	unsigned at91_port;
> +
> +	switch (port) {
> +	case AT91_PIO_PORTA:
> +		at91_port = ATMEL_BASE_PIOA;
> +		break;
> +	case AT91_PIO_PORTB:
> +		at91_port = ATMEL_BASE_PIOB;
> +		break;
> +	case AT91_PIO_PORTC:
> +		at91_port = ATMEL_BASE_PIOC;
> +		break;
> +	#if (ATMEL_PIO_PORTS > 3)

fix indention

> +	case AT91_PIO_PORTD:
> +		at91_port = ATMEL_BASE_PIOD;
> +		break;
> +	#endif
> +	#if (ATMEL_PIO_PORTS > 4)

nit ... if ATMEL_PIO_PORTS is > 4 it also matches '>3'

if >3
if >4
endif
endif

> +	case AT91_PIO_PORTE:
> +		at91_port = ATMEL_BASE_PIOE;
> +		break;
> +	#endif
> +	default:
> +		at91_port = 0;
> +		break;
> +	}
> +
> +	return at91_port;
> +}
> +
>  int at91_set_pio_pullup(unsigned port, unsigned pin, int use_pullup)
>  {
> -	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
> -	u32		mask;
> +	at91_port_t *at91_port = (at91_port_t *)at91_pio_get_port(port);

This cast here is annoying, can't we just change the return type of
at91_pio_get_port()?

> +	u32 mask;
>  
>  	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {

if (at91_port && (pin < 32))

The logic for correct range of port is delegated to at91_pio_get_port()

>  		mask = 1 << pin;
>  		if (use_pullup)
> -			writel(1 << pin, &pio->port[port].puer);
> +			writel(1 << pin, &at91_port->puer);
>  		else
> -			writel(1 << pin, &pio->port[port].pudr);
> -		writel(mask, &pio->port[port].per);
> +			writel(1 << pin, &at91_port->pudr);
> +		writel(mask, &at91_port->per);
>  	}
> +

I wonder if we should break the current usage and return another value
(-ENODEV ?) on error.

>  	return 0;
>  }

<snip>

Please adopt all places in this file with mentioned changes and tell me
your opinion about erroneous return value.

Best regards

Andreas Bießmann