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.s...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
