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

[Bo Shen]

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

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