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

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

[Bo Shen]

This patch set fix code to use pointer for pio port as the warning
message suggested, remove the warning message
remove unused at91_pio structure definition
add common gpio API
add copyright and remove error header info

runtime testing on at91sam9x5ek and sama5d3xek board
compile testing for all atmel ek board


Bo Shen (4):
  gpio: atmel: fix code to use pointer for pio port
  gpio: atmel: remove the at91_pio definition
  gpio: atmel: add gpio common API support
  gpio: atmel: add copyright and remove error header info

 arch/arm/include/asm/arch-at91/at91_pio.h |   15 --
 drivers/gpio/at91_gpio.c                  |  277 ++++++++++++++++++-----------
 2 files changed, 178 insertions(+), 114 deletions(-)

-- 
1.7.9.5

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

[Bo Shen]

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)
+	case AT91_PIO_PORTD:
+		at91_port = ATMEL_BASE_PIOD;
+		break;
+	#endif
+	#if (ATMEL_PIO_PORTS > 4)
+	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);
+	u32 mask;
 
 	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
 		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);
 	}
+
 	return 0;
 }
 
@@ -46,15 +69,16 @@ int at91_set_pio_pullup(unsigned port, unsigned pin, int use_pullup)
  */
 int at91_set_pio_periph(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);
+	u32 mask;
 
 	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
 		mask = 1 << pin;
-		writel(mask, &pio->port[port].idr);
+		writel(mask, &at91_port->idr);
 		at91_set_pio_pullup(port, pin, use_pullup);
-		writel(mask, &pio->port[port].per);
+		writel(mask, &at91_port->per);
 	}
+
 	return 0;
 }
 
@@ -63,23 +87,24 @@ int at91_set_pio_periph(unsigned port, unsigned pin, int use_pullup)
  */
 int at91_set_a_periph(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);
+	u32 mask;
 
 	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
 		mask = 1 << pin;
-		writel(mask, &pio->port[port].idr);
+		writel(mask, &at91_port->idr);
 		at91_set_pio_pullup(port, pin, use_pullup);
 #if defined(CPU_HAS_PIO3)
-		writel(readl(&pio->port[port].abcdsr1) & ~mask,
-			&pio->port[port].abcdsr1);
-		writel(readl(&pio->port[port].abcdsr2) & ~mask,
-			&pio->port[port].abcdsr2);
+		writel(readl(&at91_port->abcdsr1) & ~mask,
+		       &at91_port->abcdsr1);
+		writel(readl(&at91_port->abcdsr2) & ~mask,
+		       &at91_port->abcdsr2);
 #else
-		writel(mask, &pio->port[port].asr);
+		writel(mask, &at91_port->asr);
 #endif
-		writel(mask, &pio->port[port].pdr);
+		writel(mask, &at91_port->pdr);
 	}
+
 	return 0;
 }
 
@@ -88,23 +113,24 @@ int at91_set_a_periph(unsigned port, unsigned pin, int use_pullup)
  */
 int at91_set_b_periph(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);
+	u32 mask;
 
 	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
 		mask = 1 << pin;
-		writel(mask, &pio->port[port].idr);
+		writel(mask, &at91_port->idr);
 		at91_set_pio_pullup(port, pin, use_pullup);
 #if defined(CPU_HAS_PIO3)
-		writel(readl(&pio->port[port].abcdsr1) | mask,
-			&pio->port[port].abcdsr1);
-		writel(readl(&pio->port[port].abcdsr2) & ~mask,
-			&pio->port[port].abcdsr2);
+		writel(readl(&at91_port->abcdsr1) | mask,
+		       &at91_port->abcdsr1);
+		writel(readl(&at91_port->abcdsr2) & ~mask,
+		       &at91_port->abcdsr2);
 #else
-		writel(mask, &pio->port[port].bsr);
+		writel(mask, &at91_port->bsr);
 #endif
-		writel(mask, &pio->port[port].pdr);
+		writel(mask, &at91_port->pdr);
 	}
+
 	return 0;
 }
 
@@ -114,19 +140,20 @@ int at91_set_b_periph(unsigned port, unsigned pin, int use_pullup)
  */
 int at91_set_c_periph(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);
+	u32 mask;
 
 	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
 		mask = 1 << pin;
-		writel(mask, &pio->port[port].idr);
+		writel(mask, &at91_port->idr);
 		at91_set_pio_pullup(port, pin, use_pullup);
-		writel(readl(&pio->port[port].abcdsr1) & ~mask,
-			&pio->port[port].abcdsr1);
-		writel(readl(&pio->port[port].abcdsr2) | mask,
-			&pio->port[port].abcdsr2);
-		writel(mask, &pio->port[port].pdr);
+		writel(readl(&at91_port->abcdsr1) & ~mask,
+		       &at91_port->abcdsr1);
+		writel(readl(&at91_port->abcdsr2) | mask,
+		       &at91_port->abcdsr2);
+		writel(mask, &at91_port->pdr);
 	}
+
 	return 0;
 }
 
@@ -135,19 +162,20 @@ int at91_set_c_periph(unsigned port, unsigned pin, int use_pullup)
  */
 int at91_set_d_periph(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);
+	u32 mask;
 
 	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
 		mask = 1 << pin;
-		writel(mask, &pio->port[port].idr);
+		writel(mask, &at91_port->idr);
 		at91_set_pio_pullup(port, pin, use_pullup);
-		writel(readl(&pio->port[port].abcdsr1) | mask,
-			&pio->port[port].abcdsr1);
-		writel(readl(&pio->port[port].abcdsr2) | mask,
-			&pio->port[port].abcdsr2);
-		writel(mask, &pio->port[port].pdr);
+		writel(readl(&at91_port->abcdsr1) | mask,
+		       &at91_port->abcdsr1);
+		writel(readl(&at91_port->abcdsr2) | mask,
+		       &at91_port->abcdsr2);
+		writel(mask, &at91_port->pdr);
 	}
+
 	return 0;
 }
 #endif
@@ -158,15 +186,15 @@ int at91_set_d_periph(unsigned port, unsigned pin, int use_pullup)
  */
 int at91_set_pio_input(unsigned port, u32 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);
+	u32 mask;
 
 	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
 		mask = 1 << pin;
-		writel(mask, &pio->port[port].idr);
+		writel(mask, &at91_port->idr);
 		at91_set_pio_pullup(port, pin, use_pullup);
-		writel(mask, &pio->port[port].odr);
-		writel(mask, &pio->port[port].per);
+		writel(mask, &at91_port->odr);
+		writel(mask, &at91_port->per);
 	}
 	return 0;
 }
@@ -177,20 +205,21 @@ int at91_set_pio_input(unsigned port, u32 pin, int use_pullup)
  */
 int at91_set_pio_output(unsigned port, u32 pin, int value)
 {
-	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);
+	u32 mask;
 
 	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
 		mask = 1 << pin;
-		writel(mask, &pio->port[port].idr);
-		writel(mask, &pio->port[port].pudr);
+		writel(mask, &at91_port->idr);
+		writel(mask, &at91_port->pudr);
 		if (value)
-			writel(mask, &pio->port[port].sodr);
+			writel(mask, &at91_port->sodr);
 		else
-			writel(mask, &pio->port[port].codr);
-		writel(mask, &pio->port[port].oer);
-		writel(mask, &pio->port[port].per);
+			writel(mask, &at91_port->codr);
+		writel(mask, &at91_port->oer);
+		writel(mask, &at91_port->per);
 	}
+
 	return 0;
 }
 
@@ -199,20 +228,21 @@ int at91_set_pio_output(unsigned port, u32 pin, int value)
  */
 int at91_set_pio_deglitch(unsigned port, unsigned pin, int is_on)
 {
-	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);
+	u32 mask;
 
 	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
 		mask = 1 << pin;
 		if (is_on) {
 #if defined(CPU_HAS_PIO3)
-			writel(mask, &pio->port[port].ifscdr);
+			writel(mask, &at91_port->ifscdr);
 #endif
-			writel(mask, &pio->port[port].ifer);
+			writel(mask, &at91_port->ifer);
 		} else {
-			writel(mask, &pio->port[port].ifdr);
+			writel(mask, &at91_port->ifdr);
 		}
 	}
+
 	return 0;
 }
 
@@ -222,19 +252,20 @@ int at91_set_pio_deglitch(unsigned port, unsigned pin, int is_on)
  */
 int at91_set_pio_debounce(unsigned port, unsigned pin, int is_on, int div)
 {
-	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);
+	u32 mask;
 
 	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
 		mask = 1 << pin;
 		if (is_on) {
-			writel(mask, &pio->port[port].ifscer);
-			writel(div & PIO_SCDR_DIV, &pio->port[port].scdr);
-			writel(mask, &pio->port[port].ifer);
+			writel(mask, &at91_port->ifscer);
+			writel(div & PIO_SCDR_DIV, &at91_port->scdr);
+			writel(mask, &at91_port->ifer);
 		} else {
-			writel(mask, &pio->port[port].ifdr);
+			writel(mask, &at91_port->ifdr);
 		}
 	}
+
 	return 0;
 }
 
@@ -244,17 +275,18 @@ int at91_set_pio_debounce(unsigned port, unsigned pin, int is_on, int div)
  */
 int at91_set_pio_pulldown(unsigned port, unsigned pin, int is_on)
 {
-	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);
+	u32 mask;
 
 	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
 		mask = 1 << pin;
-		writel(mask, &pio->port[port].pudr);
+		writel(mask, &at91_port->pudr);
 		if (is_on)
-			writel(mask, &pio->port[port].ppder);
+			writel(mask, &at91_port->ppder);
 		else
-			writel(mask, &pio->port[port].ppddr);
+			writel(mask, &at91_port->ppddr);
 	}
+
 	return 0;
 }
 
@@ -263,14 +295,15 @@ int at91_set_pio_pulldown(unsigned port, unsigned pin, int is_on)
  */
 int at91_set_pio_disable_schmitt_trig(unsigned port, unsigned pin)
 {
-	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);
+	u32 mask;
 
 	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
 		mask = 1 << pin;
-		writel(readl(&pio->port[port].schmitt) | mask,
-			&pio->port[port].schmitt);
+		writel(readl(&at91_port->schmitt) | mask,
+		       &at91_port->schmitt);
 	}
+
 	return 0;
 }
 #endif
@@ -281,16 +314,17 @@ int at91_set_pio_disable_schmitt_trig(unsigned port, unsigned pin)
  */
 int at91_set_pio_multi_drive(unsigned port, unsigned pin, int is_on)
 {
-	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);
+	u32 mask;
 
 	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
 		mask = 1 << pin;
 		if (is_on)
-			writel(mask, &pio->port[port].mder);
+			writel(mask, &at91_port->mder);
 		else
-			writel(mask, &pio->port[port].mddr);
+			writel(mask, &at91_port->mddr);
 	}
+
 	return 0;
 }
 
@@ -299,16 +333,17 @@ int at91_set_pio_multi_drive(unsigned port, unsigned pin, int is_on)
  */
 int at91_set_pio_value(unsigned port, unsigned pin, int value)
 {
-	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);
+	u32 mask;
 
 	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
 		mask = 1 << pin;
 		if (value)
-			writel(mask, &pio->port[port].sodr);
+			writel(mask, &at91_port->sodr);
 		else
-			writel(mask, &pio->port[port].codr);
+			writel(mask, &at91_port->codr);
 	}
+
 	return 0;
 }
 
@@ -317,13 +352,14 @@ int at91_set_pio_value(unsigned port, unsigned pin, int value)
  */
 int at91_get_pio_value(unsigned port, unsigned pin)
 {
-	u32		pdsr = 0;
-	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
-	u32		mask;
+	u32 pdsr = 0;
+	at91_port_t *at91_port = (at91_port_t *)at91_pio_get_port(port);
+	u32 mask;
 
 	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
 		mask = 1 << pin;
-		pdsr = readl(&pio->port[port].pdsr) & mask;
+		pdsr = readl(&at91_port->pdsr) & mask;
 	}
+
 	return pdsr != 0;
 }
-- 
1.7.9.5

[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@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] [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@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

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

[Andreas Bießmann]

Hi Bo,

On 22.08.13 05:15, Bo Shen wrote:
> Hi Andreas,
> 
> On 8/21/2013 23:08, Andreas Bie?mann wrote:
>> Hi Bo,
>>
>> On 08/13/2013 08:38 AM, Bo Shen wrote:

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

I'm fine with that. If no one else complains I'll try to push this into
-rc2 for this release. The API change would then come for next release,
am I right?

Best regards

Andreas Bie?mann

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

[Bo Shen]

Hi Andreas,

On 8/22/2013 14:26, Andreas Bie?mann wrote:
>>> 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?
> I'm fine with that. If no one else complains I'll try to push this into
> -rc2 for this release. The API change would then come for next release,
> am I right?

I will prepare the v2 for this patch, after test OK and compile testing 
for all Atmel EK board, I will send out the patch.

If this series can go into this release will be better. As if without 
the common GPIO API patch applied, we can not use gpio command, software 
i2c support and etc.

> Best regards
>
> Andreas Bie?mann

Best Regards,
Bo Shen

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

[Bo Shen]

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>

---
Changes in v2:
  - fix indention
  - change the return type of at91_pio_get_port() to avoid many cast

 drivers/gpio/at91_gpio.c |  250 +++++++++++++++++++++++++---------------------
 1 file changed, 138 insertions(+), 112 deletions(-)

diff --git a/drivers/gpio/at91_gpio.c b/drivers/gpio/at91_gpio.c
index 2322914..6d227d3 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,42 @@
 #include <asm/arch/hardware.h>
 #include <asm/arch/at91_pio.h>
 
+static struct at91_port *at91_pio_get_port(unsigned port)
+{
+	switch (port) {
+	case AT91_PIO_PORTA:
+		return (struct at91_port *)ATMEL_BASE_PIOA;
+	case AT91_PIO_PORTB:
+		return (struct at91_port *)ATMEL_BASE_PIOB;
+	case AT91_PIO_PORTC:
+		return (struct at91_port *)ATMEL_BASE_PIOC;
+#if (ATMEL_PIO_PORTS > 3)
+	case AT91_PIO_PORTD:
+		return (struct at91_port *)ATMEL_BASE_PIOD;
+#if (ATMEL_PIO_PORTS > 4)
+	case AT91_PIO_PORTE:
+		return (struct at91_port *)ATMEL_BASE_PIOE;
+#endif
+#endif
+	default:
+		return NULL;
+	}
+}
+
 int at91_set_pio_pullup(unsigned port, unsigned pin, int use_pullup)
 {
-	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
-	u32		mask;
+	struct at91_port *at91_port = at91_pio_get_port(port);
+	u32 mask;
 
-	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
+	if (at91_port && (pin < 32)) {
 		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);
 	}
+
 	return 0;
 }
 
@@ -46,15 +59,16 @@ int at91_set_pio_pullup(unsigned port, unsigned pin, int use_pullup)
  */
 int at91_set_pio_periph(unsigned port, unsigned pin, int use_pullup)
 {
-	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
-	u32		mask;
+	struct at91_port *at91_port = at91_pio_get_port(port);
+	u32 mask;
 
-	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
+	if (at91_port && (pin < 32)) {
 		mask = 1 << pin;
-		writel(mask, &pio->port[port].idr);
+		writel(mask, &at91_port->idr);
 		at91_set_pio_pullup(port, pin, use_pullup);
-		writel(mask, &pio->port[port].per);
+		writel(mask, &at91_port->per);
 	}
+
 	return 0;
 }
 
@@ -63,23 +77,24 @@ int at91_set_pio_periph(unsigned port, unsigned pin, int use_pullup)
  */
 int at91_set_a_periph(unsigned port, unsigned pin, int use_pullup)
 {
-	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
-	u32		mask;
+	struct at91_port *at91_port = at91_pio_get_port(port);
+	u32 mask;
 
-	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
+	if (at91_port && (pin < 32)) {
 		mask = 1 << pin;
-		writel(mask, &pio->port[port].idr);
+		writel(mask, &at91_port->idr);
 		at91_set_pio_pullup(port, pin, use_pullup);
 #if defined(CPU_HAS_PIO3)
-		writel(readl(&pio->port[port].abcdsr1) & ~mask,
-			&pio->port[port].abcdsr1);
-		writel(readl(&pio->port[port].abcdsr2) & ~mask,
-			&pio->port[port].abcdsr2);
+		writel(readl(&at91_port->abcdsr1) & ~mask,
+		       &at91_port->abcdsr1);
+		writel(readl(&at91_port->abcdsr2) & ~mask,
+		       &at91_port->abcdsr2);
 #else
-		writel(mask, &pio->port[port].asr);
+		writel(mask, &at91_port->asr);
 #endif
-		writel(mask, &pio->port[port].pdr);
+		writel(mask, &at91_port->pdr);
 	}
+
 	return 0;
 }
 
@@ -88,23 +103,24 @@ int at91_set_a_periph(unsigned port, unsigned pin, int use_pullup)
  */
 int at91_set_b_periph(unsigned port, unsigned pin, int use_pullup)
 {
-	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
-	u32		mask;
+	struct at91_port *at91_port = at91_pio_get_port(port);
+	u32 mask;
 
-	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
+	if (at91_port && (pin < 32)) {
 		mask = 1 << pin;
-		writel(mask, &pio->port[port].idr);
+		writel(mask, &at91_port->idr);
 		at91_set_pio_pullup(port, pin, use_pullup);
 #if defined(CPU_HAS_PIO3)
-		writel(readl(&pio->port[port].abcdsr1) | mask,
-			&pio->port[port].abcdsr1);
-		writel(readl(&pio->port[port].abcdsr2) & ~mask,
-			&pio->port[port].abcdsr2);
+		writel(readl(&at91_port->abcdsr1) | mask,
+		       &at91_port->abcdsr1);
+		writel(readl(&at91_port->abcdsr2) & ~mask,
+		       &at91_port->abcdsr2);
 #else
-		writel(mask, &pio->port[port].bsr);
+		writel(mask, &at91_port->bsr);
 #endif
-		writel(mask, &pio->port[port].pdr);
+		writel(mask, &at91_port->pdr);
 	}
+
 	return 0;
 }
 
@@ -114,19 +130,20 @@ int at91_set_b_periph(unsigned port, unsigned pin, int use_pullup)
  */
 int at91_set_c_periph(unsigned port, unsigned pin, int use_pullup)
 {
-	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
-	u32		mask;
+	struct at91_port *at91_port = at91_pio_get_port(port);
+	u32 mask;
 
-	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
+	if (at91_port && (pin < 32)) {
 		mask = 1 << pin;
-		writel(mask, &pio->port[port].idr);
+		writel(mask, &at91_port->idr);
 		at91_set_pio_pullup(port, pin, use_pullup);
-		writel(readl(&pio->port[port].abcdsr1) & ~mask,
-			&pio->port[port].abcdsr1);
-		writel(readl(&pio->port[port].abcdsr2) | mask,
-			&pio->port[port].abcdsr2);
-		writel(mask, &pio->port[port].pdr);
+		writel(readl(&at91_port->abcdsr1) & ~mask,
+		       &at91_port->abcdsr1);
+		writel(readl(&at91_port->abcdsr2) | mask,
+		       &at91_port->abcdsr2);
+		writel(mask, &at91_port->pdr);
 	}
+
 	return 0;
 }
 
@@ -135,19 +152,20 @@ int at91_set_c_periph(unsigned port, unsigned pin, int use_pullup)
  */
 int at91_set_d_periph(unsigned port, unsigned pin, int use_pullup)
 {
-	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
-	u32		mask;
+	struct at91_port *at91_port = at91_pio_get_port(port);
+	u32 mask;
 
-	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
+	if (at91_port && (pin < 32)) {
 		mask = 1 << pin;
-		writel(mask, &pio->port[port].idr);
+		writel(mask, &at91_port->idr);
 		at91_set_pio_pullup(port, pin, use_pullup);
-		writel(readl(&pio->port[port].abcdsr1) | mask,
-			&pio->port[port].abcdsr1);
-		writel(readl(&pio->port[port].abcdsr2) | mask,
-			&pio->port[port].abcdsr2);
-		writel(mask, &pio->port[port].pdr);
+		writel(readl(&at91_port->abcdsr1) | mask,
+		       &at91_port->abcdsr1);
+		writel(readl(&at91_port->abcdsr2) | mask,
+		       &at91_port->abcdsr2);
+		writel(mask, &at91_port->pdr);
 	}
+
 	return 0;
 }
 #endif
@@ -158,16 +176,17 @@ int at91_set_d_periph(unsigned port, unsigned pin, int use_pullup)
  */
 int at91_set_pio_input(unsigned port, u32 pin, int use_pullup)
 {
-	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
-	u32		mask;
+	struct at91_port *at91_port = at91_pio_get_port(port);
+	u32 mask;
 
-	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
+	if (at91_port && (pin < 32)) {
 		mask = 1 << pin;
-		writel(mask, &pio->port[port].idr);
+		writel(mask, &at91_port->idr);
 		at91_set_pio_pullup(port, pin, use_pullup);
-		writel(mask, &pio->port[port].odr);
-		writel(mask, &pio->port[port].per);
+		writel(mask, &at91_port->odr);
+		writel(mask, &at91_port->per);
 	}
+
 	return 0;
 }
 
@@ -177,20 +196,21 @@ int at91_set_pio_input(unsigned port, u32 pin, int use_pullup)
  */
 int at91_set_pio_output(unsigned port, u32 pin, int value)
 {
-	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
-	u32		mask;
+	struct at91_port *at91_port = at91_pio_get_port(port);
+	u32 mask;
 
 	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
 		mask = 1 << pin;
-		writel(mask, &pio->port[port].idr);
-		writel(mask, &pio->port[port].pudr);
+		writel(mask, &at91_port->idr);
+		writel(mask, &at91_port->pudr);
 		if (value)
-			writel(mask, &pio->port[port].sodr);
+			writel(mask, &at91_port->sodr);
 		else
-			writel(mask, &pio->port[port].codr);
-		writel(mask, &pio->port[port].oer);
-		writel(mask, &pio->port[port].per);
+			writel(mask, &at91_port->codr);
+		writel(mask, &at91_port->oer);
+		writel(mask, &at91_port->per);
 	}
+
 	return 0;
 }
 
@@ -199,20 +219,21 @@ int at91_set_pio_output(unsigned port, u32 pin, int value)
  */
 int at91_set_pio_deglitch(unsigned port, unsigned pin, int is_on)
 {
-	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
-	u32		mask;
+	struct at91_port *at91_port = at91_pio_get_port(port);
+	u32 mask;
 
-	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
+	if (at91_port && (pin < 32)) {
 		mask = 1 << pin;
 		if (is_on) {
 #if defined(CPU_HAS_PIO3)
-			writel(mask, &pio->port[port].ifscdr);
+			writel(mask, &at91_port->ifscdr);
 #endif
-			writel(mask, &pio->port[port].ifer);
+			writel(mask, &at91_port->ifer);
 		} else {
-			writel(mask, &pio->port[port].ifdr);
+			writel(mask, &at91_port->ifdr);
 		}
 	}
+
 	return 0;
 }
 
@@ -222,19 +243,20 @@ int at91_set_pio_deglitch(unsigned port, unsigned pin, int is_on)
  */
 int at91_set_pio_debounce(unsigned port, unsigned pin, int is_on, int div)
 {
-	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
-	u32		mask;
+	struct at91_port *at91_port = at91_pio_get_port(port);
+	u32 mask;
 
-	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
+	if (at91_port && (pin < 32)) {
 		mask = 1 << pin;
 		if (is_on) {
-			writel(mask, &pio->port[port].ifscer);
-			writel(div & PIO_SCDR_DIV, &pio->port[port].scdr);
-			writel(mask, &pio->port[port].ifer);
+			writel(mask, &at91_port->ifscer);
+			writel(div & PIO_SCDR_DIV, &at91_port->scdr);
+			writel(mask, &at91_port->ifer);
 		} else {
-			writel(mask, &pio->port[port].ifdr);
+			writel(mask, &at91_port->ifdr);
 		}
 	}
+
 	return 0;
 }
 
@@ -244,17 +266,18 @@ int at91_set_pio_debounce(unsigned port, unsigned pin, int is_on, int div)
  */
 int at91_set_pio_pulldown(unsigned port, unsigned pin, int is_on)
 {
-	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
-	u32		mask;
+	struct at91_port *at91_port = at91_pio_get_port(port);
+	u32 mask;
 
-	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
+	if (at91_port && (pin < 32)) {
 		mask = 1 << pin;
-		writel(mask, &pio->port[port].pudr);
+		writel(mask, &at91_port->pudr);
 		if (is_on)
-			writel(mask, &pio->port[port].ppder);
+			writel(mask, &at91_port->ppder);
 		else
-			writel(mask, &pio->port[port].ppddr);
+			writel(mask, &at91_port->ppddr);
 	}
+
 	return 0;
 }
 
@@ -263,14 +286,15 @@ int at91_set_pio_pulldown(unsigned port, unsigned pin, int is_on)
  */
 int at91_set_pio_disable_schmitt_trig(unsigned port, unsigned pin)
 {
-	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
-	u32		mask;
+	struct at91_port *at91_port = at91_pio_get_port(port);
+	u32 mask;
 
-	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
+	if (at91_port && (pin < 32)) {
 		mask = 1 << pin;
-		writel(readl(&pio->port[port].schmitt) | mask,
-			&pio->port[port].schmitt);
+		writel(readl(&at91_port->schmitt) | mask,
+		       &at91_port->schmitt);
 	}
+
 	return 0;
 }
 #endif
@@ -281,16 +305,17 @@ int at91_set_pio_disable_schmitt_trig(unsigned port, unsigned pin)
  */
 int at91_set_pio_multi_drive(unsigned port, unsigned pin, int is_on)
 {
-	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
-	u32		mask;
+	struct at91_port *at91_port = at91_pio_get_port(port);
+	u32 mask;
 
-	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
+	if (at91_port && (pin < 32)) {
 		mask = 1 << pin;
 		if (is_on)
-			writel(mask, &pio->port[port].mder);
+			writel(mask, &at91_port->mder);
 		else
-			writel(mask, &pio->port[port].mddr);
+			writel(mask, &at91_port->mddr);
 	}
+
 	return 0;
 }
 
@@ -299,16 +324,17 @@ int at91_set_pio_multi_drive(unsigned port, unsigned pin, int is_on)
  */
 int at91_set_pio_value(unsigned port, unsigned pin, int value)
 {
-	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
-	u32		mask;
+	struct at91_port *at91_port = at91_pio_get_port(port);
+	u32 mask;
 
-	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
+	if (at91_port && (pin < 32)) {
 		mask = 1 << pin;
 		if (value)
-			writel(mask, &pio->port[port].sodr);
+			writel(mask, &at91_port->sodr);
 		else
-			writel(mask, &pio->port[port].codr);
+			writel(mask, &at91_port->codr);
 	}
+
 	return 0;
 }
 
@@ -317,13 +343,13 @@ int at91_set_pio_value(unsigned port, unsigned pin, int value)
  */
 int at91_get_pio_value(unsigned port, unsigned pin)
 {
-	u32		pdsr = 0;
-	at91_pio_t	*pio = (at91_pio_t *) ATMEL_BASE_PIOA;
-	u32		mask;
+	struct at91_port *at91_port = at91_pio_get_port(port);
+	u32 pdsr = 0, mask;
 
-	if ((port < ATMEL_PIO_PORTS) && (pin < 32)) {
+	if (at91_port && (pin < 32)) {
 		mask = 1 << pin;
-		pdsr = readl(&pio->port[port].pdsr) & mask;
+		pdsr = readl(&at91_port->pdsr) & mask;
 	}
+
 	return pdsr != 0;
 }
-- 
1.7.9.5

[U-Boot] [U-Boot,v2] gpio: atmel: fix code to use pointer for pio port

[Andreas Bießmann]

Dear Bo Shen,

Bo Shen <voice.shen@atmel.com> writes:
>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>
>
>---
>Changes in v2:
>  - fix indention
>  - change the return type of at91_pio_get_port() to avoid many cast
>
> drivers/gpio/at91_gpio.c |  250 +++++++++++++++++++++++++---------------------
> 1 file changed, 138 insertions(+), 112 deletions(-)

applied to u-boot-atmel/master, thanks!

Best regards,
Andreas Bie?mann

[U-Boot] [PATCH 2/4] gpio: atmel: remove the at91_pio definition

[Bo Shen]

the at91_pio definition is no longer needed, so remove it

Signed-off-by: Bo Shen <voice.shen@atmel.com>
---
 arch/arm/include/asm/arch-at91/at91_pio.h |   15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/arch/arm/include/asm/arch-at91/at91_pio.h b/arch/arm/include/asm/arch-at91/at91_pio.h
index 676f024..ba61542 100644
--- a/arch/arm/include/asm/arch-at91/at91_pio.h
+++ b/arch/arm/include/asm/arch-at91/at91_pio.h
@@ -109,21 +109,6 @@ typedef struct at91_port {
 #endif
 } at91_port_t;
 
-typedef union at91_pio {
-	struct {
-		at91_port_t	pioa;
-		at91_port_t	piob;
-		at91_port_t	pioc;
-	#if (ATMEL_PIO_PORTS > 3)
-		at91_port_t	piod;
-	#endif
-	#if (ATMEL_PIO_PORTS > 4)
-		at91_port_t	pioe;
-	#endif
-	} ;
-	at91_port_t port[ATMEL_PIO_PORTS];
-} at91_pio_t;
-
 #ifdef CONFIG_AT91_GPIO
 int at91_set_a_periph(unsigned port, unsigned pin, int use_pullup);
 int at91_set_b_periph(unsigned port, unsigned pin, int use_pullup);
-- 
1.7.9.5

[U-Boot] [PATCH 2/4] gpio: atmel: remove the at91_pio definition

[Andreas Bießmann]

On 08/13/2013 08:38 AM, Bo Shen wrote:
> the at91_pio definition is no longer needed, so remove it
> 
> Signed-off-by: Bo Shen <voice.shen@atmel.com>
> ---
>  arch/arm/include/asm/arch-at91/at91_pio.h |   15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-at91/at91_pio.h b/arch/arm/include/asm/arch-at91/at91_pio.h
> index 676f024..ba61542 100644
> --- a/arch/arm/include/asm/arch-at91/at91_pio.h
> +++ b/arch/arm/include/asm/arch-at91/at91_pio.h
> @@ -109,21 +109,6 @@ typedef struct at91_port {
>  #endif
>  } at91_port_t;
>  
> -typedef union at91_pio {
> -	struct {
> -		at91_port_t	pioa;
> -		at91_port_t	piob;
> -		at91_port_t	pioc;
> -	#if (ATMEL_PIO_PORTS > 3)
> -		at91_port_t	piod;
> -	#endif
> -	#if (ATMEL_PIO_PORTS > 4)
> -		at91_port_t	pioe;
> -	#endif
> -	} ;
> -	at91_port_t port[ATMEL_PIO_PORTS];
> -} at91_pio_t;
> -

NAK, this breaks at least 7 boards:

14: gpio: atmel: remove the at91_pio definition
       arm: +   at91sam9263ek_dataflash_cs0 eb_cpux9k2 at91rm9200ek_ram
cpuat91 vl_ma2sc vl_ma2sc_ram cpuat91_ram

We need to have this typedef while we have the new API and change the
users of this struct to the new API after introducing.

Best regards

Andreas Bie?mann

[U-Boot] [PATCH 3/4] gpio: atmel: add gpio common API support

[Bo Shen]

add gpio common API support for gpio command

Signed-off-by: Bo Shen <voice.shen@atmel.com>
---
 drivers/gpio/at91_gpio.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/gpio/at91_gpio.c b/drivers/gpio/at91_gpio.c
index 15f396f..3de0844 100644
--- a/drivers/gpio/at91_gpio.c
+++ b/drivers/gpio/at91_gpio.c
@@ -363,3 +363,46 @@ int at91_get_pio_value(unsigned port, unsigned pin)
 
 	return pdsr != 0;
 }
+
+/* Common GPIO API */
+
+#define at91_gpio_to_port(gpio)		(gpio / 32)
+#define at91_gpio_to_pin(gpio)		(gpio % 32)
+
+int gpio_request(unsigned gpio, const char *label)
+{
+	return 0;
+}
+
+int gpio_free(unsigned gpio)
+{
+	return 0;
+}
+
+int gpio_direction_input(unsigned gpio)
+{
+	at91_set_pio_input(at91_gpio_to_port(gpio),
+			   at91_gpio_to_pin(gpio), 0);
+	return 0;
+}
+
+int gpio_direction_output(unsigned gpio, int value)
+{
+	at91_set_pio_output(at91_gpio_to_port(gpio),
+			    at91_gpio_to_pin(gpio), value);
+	return 0;
+}
+
+int gpio_get_value(unsigned gpio)
+{
+	return (int) at91_get_pio_value(at91_gpio_to_port(gpio),
+					at91_gpio_to_pin(gpio));
+}
+
+int gpio_set_value(unsigned gpio, int value)
+{
+	at91_set_pio_value(at91_gpio_to_port(gpio),
+			   at91_gpio_to_pin(gpio), value);
+
+	return 0;
+}
-- 
1.7.9.5

[U-Boot] [PATCH 3/4] gpio: atmel: add gpio common API support

[Andreas Bießmann]

On 08/13/2013 08:38 AM, Bo Shen wrote:
> add gpio common API support for gpio command
> 
> Signed-off-by: Bo Shen <voice.shen@atmel.com>
> ---
>  drivers/gpio/at91_gpio.c |   43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/gpio/at91_gpio.c b/drivers/gpio/at91_gpio.c
> index 15f396f..3de0844 100644
> --- a/drivers/gpio/at91_gpio.c
> +++ b/drivers/gpio/at91_gpio.c
> @@ -363,3 +363,46 @@ int at91_get_pio_value(unsigned port, unsigned pin)
>  
>  	return pdsr != 0;
>  }
> +
> +/* Common GPIO API */
> +
> +#define at91_gpio_to_port(gpio)		(gpio / 32)
> +#define at91_gpio_to_pin(gpio)		(gpio % 32)
> +
> +int gpio_request(unsigned gpio, const char *label)
> +{
> +	return 0;
> +}
> +
> +int gpio_free(unsigned gpio)
> +{
> +	return 0;
> +}
> +
> +int gpio_direction_input(unsigned gpio)
> +{
> +	at91_set_pio_input(at91_gpio_to_port(gpio),
> +			   at91_gpio_to_pin(gpio), 0);
> +	return 0;
> +}
> +
> +int gpio_direction_output(unsigned gpio, int value)
> +{
> +	at91_set_pio_output(at91_gpio_to_port(gpio),
> +			    at91_gpio_to_pin(gpio), value);
> +	return 0;
> +}
> +
> +int gpio_get_value(unsigned gpio)
> +{
> +	return (int) at91_get_pio_value(at91_gpio_to_port(gpio),
> +					at91_gpio_to_pin(gpio));

why cast to int here?

> +}
> +
> +int gpio_set_value(unsigned gpio, int value)
> +{
> +	at91_set_pio_value(at91_gpio_to_port(gpio),
> +			   at91_gpio_to_pin(gpio), value);
> +
> +	return 0;
> +}
> 

Great, I love this. But wasn't there some define for generic GPIO?
Shouldn't we encapsulate this API into this other define?

Best regards

Andreas Bie?mann

[U-Boot] [PATCH 3/4] gpio: atmel: add gpio common API support

[Bo Shen]

Hi Andreas,

On 8/21/2013 23:14, Andreas Bie?mann wrote:
> On 08/13/2013 08:38 AM, Bo Shen wrote:
>> add gpio common API support for gpio command
>>
>> Signed-off-by: Bo Shen <voice.shen@atmel.com>
>> ---
>>   drivers/gpio/at91_gpio.c |   43 +++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 43 insertions(+)
>>
>> diff --git a/drivers/gpio/at91_gpio.c b/drivers/gpio/at91_gpio.c
>> index 15f396f..3de0844 100644
>> --- a/drivers/gpio/at91_gpio.c
>> +++ b/drivers/gpio/at91_gpio.c
>> @@ -363,3 +363,46 @@ int at91_get_pio_value(unsigned port, unsigned pin)
>>
>>   	return pdsr != 0;
>>   }
>> +
>> +/* Common GPIO API */
>> +
>> +#define at91_gpio_to_port(gpio)		(gpio / 32)
>> +#define at91_gpio_to_pin(gpio)		(gpio % 32)
>> +
>> +int gpio_request(unsigned gpio, const char *label)
>> +{
>> +	return 0;
>> +}
>> +
>> +int gpio_free(unsigned gpio)
>> +{
>> +	return 0;
>> +}
>> +
>> +int gpio_direction_input(unsigned gpio)
>> +{
>> +	at91_set_pio_input(at91_gpio_to_port(gpio),
>> +			   at91_gpio_to_pin(gpio), 0);
>> +	return 0;
>> +}
>> +
>> +int gpio_direction_output(unsigned gpio, int value)
>> +{
>> +	at91_set_pio_output(at91_gpio_to_port(gpio),
>> +			    at91_gpio_to_pin(gpio), value);
>> +	return 0;
>> +}
>> +
>> +int gpio_get_value(unsigned gpio)
>> +{
>> +	return (int) at91_get_pio_value(at91_gpio_to_port(gpio),
>> +					at91_gpio_to_pin(gpio));
>
> why cast to int here?

Actually no need, as the at91_get_pio_value() return value is int.
I will remove the cast in next version.

>> +}
>> +
>> +int gpio_set_value(unsigned gpio, int value)
>> +{
>> +	at91_set_pio_value(at91_gpio_to_port(gpio),
>> +			   at91_gpio_to_pin(gpio), value);
>> +
>> +	return 0;
>> +}
>>
>
> Great, I love this. But wasn't there some define for generic GPIO?

I am not fully get your meaning, what you mean "define for generic 
GPIO"? define gpio pin number (?)

> Shouldn't we encapsulate this API into this other define?

You mean, not in this file or anything else?

> Best regards
>
> Andreas Bie?mann
>

Best Regards,
Bo Shen

[U-Boot] [PATCH 3/4] gpio: atmel: add gpio common API support

[Andreas Bießmann]

Hi Bo,

On 22.08.13 05:21, Bo Shen wrote:
> Hi Andreas,
> 
> On 8/21/2013 23:14, Andreas Bie?mann wrote:
>> On 08/13/2013 08:38 AM, Bo Shen wrote:
>>> add gpio common API support for gpio command
>>>
>>> Signed-off-by: Bo Shen <voice.shen@atmel.com>
>>> ---
>>>   drivers/gpio/at91_gpio.c |   43
>>> +++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 43 insertions(+)
>>>
>>> diff --git a/drivers/gpio/at91_gpio.c b/drivers/gpio/at91_gpio.c
>>> index 15f396f..3de0844 100644
>>> --- a/drivers/gpio/at91_gpio.c
>>> +++ b/drivers/gpio/at91_gpio.c
>>> @@ -363,3 +363,46 @@ int at91_get_pio_value(unsigned port, unsigned pin)
>>>
>>>       return pdsr != 0;
>>>   }
>>> +
>>> +/* Common GPIO API */
>>> +
>>> +#define at91_gpio_to_port(gpio)        (gpio / 32)
>>> +#define at91_gpio_to_pin(gpio)        (gpio % 32)
>>> +
>>> +int gpio_request(unsigned gpio, const char *label)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>> +int gpio_free(unsigned gpio)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>> +int gpio_direction_input(unsigned gpio)
>>> +{
>>> +    at91_set_pio_input(at91_gpio_to_port(gpio),
>>> +               at91_gpio_to_pin(gpio), 0);
>>> +    return 0;
>>> +}
>>> +
>>> +int gpio_direction_output(unsigned gpio, int value)
>>> +{
>>> +    at91_set_pio_output(at91_gpio_to_port(gpio),
>>> +                at91_gpio_to_pin(gpio), value);
>>> +    return 0;
>>> +}
>>> +
>>> +int gpio_get_value(unsigned gpio)
>>> +{
>>> +    return (int) at91_get_pio_value(at91_gpio_to_port(gpio),
>>> +                    at91_gpio_to_pin(gpio));
>>
>> why cast to int here?
> 
> Actually no need, as the at91_get_pio_value() return value is int.
> I will remove the cast in next version.

I could change this also when applying.

>>> +}
>>> +
>>> +int gpio_set_value(unsigned gpio, int value)
>>> +{
>>> +    at91_set_pio_value(at91_gpio_to_port(gpio),
>>> +               at91_gpio_to_pin(gpio), value);
>>> +
>>> +    return 0;
>>> +}
>>>
>>
>> Great, I love this. But wasn't there some define for generic GPIO?
> 
> I am not fully get your meaning, what you mean "define for generic
> GPIO"? define gpio pin number (?)

My fault, I thought there is some CONFIG_XXX for the 'generic GPIO API'
(gpio_set_value/gpio_get_value/gpio_direction_input/...). It seems there
is no such define, at91 gpio did just miss the time when this API was
introduced.

I'm fine if you just send a v2 of the 1/4 patch. I can remove the cast
in here and will _not_ apply 2/4 cause it breaks boards.

Best regards

Andreas Bie?mann

[U-Boot] [PATCH 3/4] gpio: atmel: add gpio common API support

[Bo Shen]

Hi Andreas,

On 8/22/2013 14:34, Andreas Bie?mann wrote:
> I'm fine if you just send a v2 of the 1/4 patch. I can remove the cast
> in here and will_not_  apply 2/4 cause it breaks boards.

OK, I will send out the v2 of the 1/4 patch.

> Best regards
>
> Andreas Bie?mann

Best Regards,
Bo Shen

[U-Boot] [U-Boot,3/4] gpio: atmel: add gpio common API support

[Andreas Bießmann]

Dear Bo Shen,

Bo Shen <voice.shen@atmel.com> writes:
>add gpio common API support for gpio command
>
>Signed-off-by: Bo Shen <voice.shen@atmel.com>
>[fix unnecessary cast]
>Signed-off-by: Andreas Bie?mann <andreas.devel@googlemail.com>
>---
>drivers/gpio/at91_gpio.c |   43 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 43 insertions(+)

applied to u-boot-atmel/master, thanks!

Best regards,
Andreas Bie?mann

[U-Boot] [PATCH 4/4] gpio: atmel: add copyright and remove error header info

[Bo Shen]

Signed-off-by: Bo Shen <voice.shen@atmel.com>

---
 drivers/gpio/at91_gpio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/at91_gpio.c b/drivers/gpio/at91_gpio.c
index 3de0844..72161dc 100644
--- a/drivers/gpio/at91_gpio.c
+++ b/drivers/gpio/at91_gpio.c
@@ -1,5 +1,5 @@
 /*
- * Memory Setup stuff - taken from blob memsetup.S
+ * Copyright (C) 2013 Bo Shen <voice.shen@atmel.com>
  *
  * Copyright (C) 2009 Jens Scharsig (js_at_ng at scharsoft.de)
  *
-- 
1.7.9.5

[U-Boot] [PATCH 4/4] gpio: atmel: add copyright and remove error header info

[Andreas Bießmann]

Hi Bo, Jens,

On 08/13/2013 08:38 AM, Bo Shen wrote:
> Signed-off-by: Bo Shen <voice.shen@atmel.com>
> 
> ---
>  drivers/gpio/at91_gpio.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/at91_gpio.c b/drivers/gpio/at91_gpio.c
> index 3de0844..72161dc 100644
> --- a/drivers/gpio/at91_gpio.c
> +++ b/drivers/gpio/at91_gpio.c
> @@ -1,5 +1,5 @@
>  /*
> - * Memory Setup stuff - taken from blob memsetup.S

Jens, could you please ACK this and maybe clarify why it was here at all?

> + * Copyright (C) 2013 Bo Shen <voice.shen@atmel.com>
>   *
>   * Copyright (C) 2009 Jens Scharsig (js_at_ng at scharsoft.de)
>   *
> 

Best regards

Andreas Bie?mann

[U-Boot] [PATCH 4/4] gpio: atmel: add copyright and remove error header info

[Jens Scharsig]

Am 2013-08-21 16:17, schrieb Andreas Bie?mann:
> Hi Bo, Jens,
> 
> On 08/13/2013 08:38 AM, Bo Shen wrote:
>> Signed-off-by: Bo Shen <voice.shen@atmel.com>
>>
>> ---
>>  drivers/gpio/at91_gpio.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/at91_gpio.c b/drivers/gpio/at91_gpio.c
>> index 3de0844..72161dc 100644
>> --- a/drivers/gpio/at91_gpio.c
>> +++ b/drivers/gpio/at91_gpio.c
>> @@ -1,5 +1,5 @@
>>  /*
>> - * Memory Setup stuff - taken from blob memsetup.S
> 
> Jens, could you please ACK this and maybe clarify why it was here at all?
> 
This line has the origin in a wrong copy & paste

>> + * Copyright (C) 2013 Bo Shen <voice.shen@atmel.com>
>>   *
>>   * Copyright (C) 2009 Jens Scharsig (js_at_ng at scharsoft.de)
>>   *
>>
> 
> Best regards
> 
> Andreas Bie?mann
> 
 Regards Jens

[U-Boot] [PATCH 4/4] gpio: atmel: add copyright and remove error header info

[Jens Scharsig]

Am 2013-08-13 07:38, schrieb Bo Shen:
> Signed-off-by: Bo Shen <voice.shen@atmel.com>
> 
> ---
>  drivers/gpio/at91_gpio.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/at91_gpio.c b/drivers/gpio/at91_gpio.c
> index 3de0844..72161dc 100644
> --- a/drivers/gpio/at91_gpio.c
> +++ b/drivers/gpio/at91_gpio.c
> @@ -1,5 +1,5 @@
>  /*
> - * Memory Setup stuff - taken from blob memsetup.S
> + * Copyright (C) 2013 Bo Shen <voice.shen@atmel.com>
>   *
>   * Copyright (C) 2009 Jens Scharsig (js_at_ng at scharsoft.de)
>   *
> 
Acked-by: Jens Scharsig <js_at_ng@scharsoft.de>

[U-Boot] [U-Boot,4/4] gpio: atmel: add copyright and remove error header info

[Andreas Bießmann]

Dear Bo Shen,

Bo Shen <voice.shen@atmel.com> writes:
>Signed-off-by: Bo Shen <voice.shen@atmel.com>
>Acked-by: Jens Scharsig <js_at_ng@scharsoft.de>
>
>---
>drivers/gpio/at91_gpio.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

applied to u-boot-atmel/master, thanks!

Best regards,
Andreas Bie?mann