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

Bo Shen
2013-08-22T03:15:20Z

Hi Andreas,

On 8/21/2013 23:08, Andreas Bie?mann wrote:
> 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@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

OK. Thanks.

>> + 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

OK, I will change style as is.

>> + 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()?

I will 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()

Yes, this check should be in 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.

For the erroneous return value, actually we never check it (consider it
always successfully). So, I think now we just keep it as is, and
consider a proper method after this patch set. Would it be OK?

> Best regards
>
> Andreas Bie?mann
>

Best Regards,
Bo Shen