Skip to contentSkip to conversation

gpio: atmel: fix code to use pointer for pio port

PATCH Superseded Bo Shen 3 participants 21 messages Aug 13, 2013 – Aug 22, 2013

Subsystems (3)
Arm (32-bit) GPIO Arm/Microchip (AT91) SoC support
lore
Share & tools
lore

Bo Shen

author
#0 lore raw

cover gpio: atmel: fix code to use pointer for pio port

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(-)
Signature · 1 line
1.7.9.5
author
#1 lore raw

PATCH 1/4 gpio: atmel: fix code to use pointer for pio port

fix code to use pointer for pio port as the warning message suggested remove the warning message

Signed-off-by Bo Shen <[email protected]>
---
 drivers/gpio/at91_gpio.c |  232 ++++++++++++++++++++++++++--------------------
 1 file changed, 134 insertions(+), 98 deletions(-)
drivers/gpio/at91_gpio.c +134 -98
index 2322914..15f396f 100644
@@ -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;
 }
Signature · 1 line
1.7.9.5
#2 lore raw

PATCH 1/4 gpio: atmel: fix code to use pointer for pio port

Hi Bo,

Quoted 38 lines · Bo Shen

fix code to use pointer for pio port as the warning message suggested remove the warning message

Signed-off-by Bo Shen <[email protected]>
---
 drivers/gpio/at91_gpio.c |  232 ++++++++++++++++++++++++++--------------------
 1 file changed, 134 insertions(+), 98 deletions(-)
drivers/gpio/at91_gpio.c +5 -10
index 2322914..15f396f 100644
@@ -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) {

drivers/gpio/at91_gpio.c line 32

Quoted 10 lines
drivers/gpio/at91_gpio.c +10 -0
+	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

drivers/gpio/at91_gpio.c line 37

Quoted 5 lines
drivers/gpio/at91_gpio.c +5 -0
+	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

Quoted 7 lines
drivers/gpio/at91_gpio.c +7 -0
+	case AT91_PIO_PORTE:
+		at91_port = ATMEL_BASE_PIOE;
+		break;
+	#endif
+	default:
+		at91_port = 0;
+		break;

drivers/gpio/at91_gpio.c line 52

Quoted 10 lines
drivers/gpio/at91_gpio.c +6 -2
+	}
+
+	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()?

drivers/gpio/at91_gpio.c line 55

Quoted 3 lines
drivers/gpio/at91_gpio.c +1 -0
+	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()

drivers/gpio/at91_gpio.c line 63

Quoted 11 lines
drivers/gpio/at91_gpio.c +4 -3
 		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.

drivers/gpio/at91_gpio.c line 65

Quoted 2 lines
drivers/gpio/at91_gpio.c +0 -0
 	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

author
#3 lore raw

PATCH 1/4 gpio: atmel: fix code to use pointer for pio port

Hi Andreas,

Quoted 45 lines · Andreas Bie?mann

Hi Bo,

Quoted 42 lines · Bo Shen

fix code to use pointer for pio port as the warning message suggested remove the warning message

Signed-off-by Bo Shen <[email protected]>
---
  drivers/gpio/at91_gpio.c |  232 ++++++++++++++++++++++++++--------------------
  1 file changed, 134 insertions(+), 98 deletions(-)
drivers/gpio/at91_gpio.c +9 -10
index 2322914..15f396f 100644
@@ -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:
Quoted 8 lines

drivers/gpio/at91_gpio.c line 32

Quoted 6 lines
drivers/gpio/at91_gpio.c +6 -0
+		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.

Quoted 8 lines

drivers/gpio/at91_gpio.c line 37

Quoted 5 lines
drivers/gpio/at91_gpio.c +5 -0
+	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'

Quoted 4 lines

if >3
if >4
endif
endif

OK, I will change style as is.

Quoted 12 lines
Quoted 12 lines
drivers/gpio/at91_gpio.c +12 -0
+	case AT91_PIO_PORTE:
+		at91_port = ATMEL_BASE_PIOE;
+		break;
+	#endif
+	default:
+		at91_port = 0;
+		break;
+	}
+
+	return at91_port;
+}
+
Quoted 8 lines

drivers/gpio/at91_gpio.c line 52

Quoted 5 lines
drivers/gpio/at91_gpio.c +1 -2
  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().

Quoted 7 lines

drivers/gpio/at91_gpio.c line 55

Quoted 3 lines
drivers/gpio/at91_gpio.c +1 -0
+	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();

Quoted 15 lines

drivers/gpio/at91_gpio.c line 63

Quoted 11 lines
drivers/gpio/at91_gpio.c +4 -3
  		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.

Quoted 7 lines

drivers/gpio/at91_gpio.c line 65

Quoted 2 lines
drivers/gpio/at91_gpio.c +0 -0
  	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?

Quoted 4 lines

Best regards

Andreas Bie?mann

Best Regards,
Bo Shen

#4 lore raw

PATCH 1/4 gpio: atmel: fix code to use pointer for pio port

Hi Bo,

Quoted 6 lines · Bo Shen

Hi Andreas,

Quoted 3 lines · Andreas Bie?mann

Hi Bo,

On 08/13/2013 08:38 AM, Bo Shen wrote:

<snip>

Quoted 6 lines
Quoted 2 lines

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

author
#5 lore raw

PATCH 1/4 gpio: atmel: fix code to use pointer for pio port

Hi Andreas,

Quoted 9 lines · Andreas Bie?mann
Quoted 6 lines
Quoted 6 lines

Please adopt all places in this file with mentioned changes and tell me

Quoted 1 line
Quoted 1 line

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.

Quoted 3 lines

Best regards

Andreas Bie?mann

Best Regards,
Bo Shen

author
#6 lore raw

PATCH v2 1/1 gpio: atmel: fix code to use pointer for pio port

In reply to Bo Shen

fix code to use pointer for pio port as the warning message suggested remove the warning message

Signed-off-by Bo Shen <[email protected]>

---
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(-)
drivers/gpio/at91_gpio.c +138 -112
index 2322914..6d227d3 100644
@@ -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;
 }
Signature · 1 line
1.7.9.5
#7 lore raw

PATCH v2 1/1 gpio: atmel: fix code to use pointer for pio port

Dear Bo Shen,

Quoted 5 lines · Bo Shen

fix code to use pointer for pio port as the warning message suggested remove the warning message

Signed-off-by Bo Shen <[email protected]>
Quoted 7 lines

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

author
#8 lore raw

PATCH 2/4 gpio: atmel: remove the at91_pio definition

In reply to Bo Shen

the at91_pio definition is no longer needed, so remove it

Signed-off-by Bo Shen <[email protected]>
---
 arch/arm/include/asm/arch-at91/at91_pio.h |   15 ---------------
 1 file changed, 15 deletions(-)
arch/arm/include/asm/arch-at91/at91_pio.h +0 -15
index 676f024..ba61542 100644
@@ -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);
Signature · 1 line
1.7.9.5
NAK
#9 lore raw

PATCH 2/4 gpio: atmel: remove the at91_pio definition

Quoted 20 lines · Bo Shen

the at91_pio definition is no longer needed, so remove it

Signed-off-by Bo Shen <[email protected]>
---
 arch/arm/include/asm/arch-at91/at91_pio.h |   15 ---------------
 1 file changed, 15 deletions(-)
arch/arm/include/asm/arch-at91/at91_pio.h +0 -5
index 676f024..ba61542 100644
@@ -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;

arch/arm/include/asm/arch-at91/at91_pio.h typedef struct at91_port { · line 126

Quoted 10 lines
arch/arm/include/asm/arch-at91/at91_pio.h +0 -10
-	#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

author
#10 lore raw

PATCH 3/4 gpio: atmel: add gpio common API support

In reply to Bo Shen

add gpio common API support for gpio command

Signed-off-by Bo Shen <[email protected]>
---
 drivers/gpio/at91_gpio.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
drivers/gpio/at91_gpio.c +43 -0
index 15f396f..3de0844 100644
@@ -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;
+}
Signature · 1 line
1.7.9.5
#11 lore raw

PATCH 3/4 gpio: atmel: add gpio common API support

Quoted 39 lines · Bo Shen

add gpio common API support for gpio command

Signed-off-by Bo Shen <[email protected]>
---
 drivers/gpio/at91_gpio.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
drivers/gpio/at91_gpio.c +24 -0
index 15f396f..3de0844 100644
@@ -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)

drivers/gpio/at91_gpio.c int at91_get_pio_value(unsigned port, unsigned pin) · line 399

Quoted 10 lines
drivers/gpio/at91_gpio.c +10 -0
+{
+	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?

drivers/gpio/at91_gpio.c int at91_get_pio_value(unsigned port, unsigned pin) · line 408

Quoted 10 lines
drivers/gpio/at91_gpio.c +9 -0
+}
+
+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

author
#12 lore raw

PATCH 3/4 gpio: atmel: add gpio common API support

Hi Andreas,

Quoted 44 lines · Andreas Bie?mann
Quoted 43 lines · Bo Shen

add gpio common API support for gpio command

Signed-off-by Bo Shen <[email protected]>
---
  drivers/gpio/at91_gpio.c |   43 +++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 43 insertions(+)
drivers/gpio/at91_gpio.c +28 -0
index 15f396f..3de0844 100644
@@ -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;
Quoted 8 lines

drivers/gpio/at91_gpio.c int at91_get_pio_value(unsigned port, unsigned pin) · line 399

Quoted 6 lines
drivers/gpio/at91_gpio.c +6 -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.

Quoted 4 lines
Quoted 4 lines
drivers/gpio/at91_gpio.c +4 -0
+}
+
+int gpio_set_value(unsigned gpio, int value)
+{
Quoted 8 lines

drivers/gpio/at91_gpio.c int at91_get_pio_value(unsigned port, unsigned pin) · line 408

Quoted 6 lines
drivers/gpio/at91_gpio.c +5 -0
+	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 (?)

Quoted 1 line

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

You mean, not in this file or anything else?

Quoted 4 lines

Best regards

Andreas Bie?mann

Best Regards,
Bo Shen

#13 lore raw

PATCH 3/4 gpio: atmel: add gpio common API support

Hi Bo,

Quoted 57 lines · Bo Shen

Hi Andreas,

Quoted 45 lines · Andreas Bie?mann
Quoted 44 lines · Bo Shen

add gpio common API support for gpio command

Signed-off-by Bo Shen <[email protected]>
---
  drivers/gpio/at91_gpio.c |   43
+++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 43 insertions(+)
drivers/gpio/at91_gpio.c +28 -0
index 15f396f..3de0844 100644
@@ -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;
Quoted 8 lines

drivers/gpio/at91_gpio.c int at91_get_pio_value(unsigned port, unsigned pin) · line 399

Quoted 6 lines
drivers/gpio/at91_gpio.c +6 -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?

Quoted 2 lines

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.

Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
drivers/gpio/at91_gpio.c +4 -0
+}
+
+int gpio_set_value(unsigned gpio, int value)
+{
Quoted 8 lines

drivers/gpio/at91_gpio.c int at91_get_pio_value(unsigned port, unsigned pin) · line 408

Quoted 6 lines
drivers/gpio/at91_gpio.c +5 -0
+    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?

Quoted 2 lines

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

author
#14 lore raw

PATCH 3/4 gpio: atmel: add gpio common API support

Hi Andreas,

Quoted 2 lines · Andreas Bie?mann

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.

Quoted 3 lines

Best regards

Andreas Bie?mann

Best Regards,
Bo Shen

author
#16 lore raw

PATCH 4/4 gpio: atmel: add copyright and remove error header info

In reply to Bo Shen
Signed-off-by Bo Shen <[email protected]>
---
 drivers/gpio/at91_gpio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/at91_gpio.c +1 -1
index 3de0844..72161dc 100644
@@ -1,5 +1,5 @@
 /*
- * Memory Setup stuff - taken from blob memsetup.S
+ * Copyright (C) 2013 Bo Shen <[email protected]>
  *
  * Copyright (C) 2009 Jens Scharsig (js_at_ng at scharsoft.de)
  *
Signature · 1 line
1.7.9.5
#17 lore raw

PATCH 4/4 gpio: atmel: add copyright and remove error header info

Hi Bo, Jens,

drivers/gpio/at91_gpio.c line 2

Quoted 13 lines · Bo Shen
Signed-off-by Bo Shen <[email protected]>
---
 drivers/gpio/at91_gpio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/at91_gpio.c +0 -1
index 3de0844..72161dc 100644
@@ -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?

drivers/gpio/at91_gpio.c line 5

Quoted 5 lines
drivers/gpio/at91_gpio.c +1 -0
+ * Copyright (C) 2013 Bo Shen <[email protected]>
  *
  * Copyright (C) 2009 Jens Scharsig (js_at_ng at scharsoft.de)
  *

Best regards

Andreas Bie?mann

#18 lore raw

PATCH 4/4 gpio: atmel: add copyright and remove error header info

Am 2013-08-21 16:17, schrieb Andreas Bie?mann:

Quoted 9 lines

Hi Bo, Jens,

Quoted 6 lines · Bo Shen
Signed-off-by Bo Shen <[email protected]>
---
 drivers/gpio/at91_gpio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 10 lines

drivers/gpio/at91_gpio.c line 2

Quoted 7 lines
drivers/gpio/at91_gpio.c +0 -1
index 3de0844..72161dc 100644
@@ -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

Quoted 10 lines

drivers/gpio/at91_gpio.c line 5

Quoted 5 lines
drivers/gpio/at91_gpio.c +1 -0
+ * Copyright (C) 2013 Bo Shen <[email protected]>
  *
  * Copyright (C) 2009 Jens Scharsig (js_at_ng at scharsoft.de)
  *

Best regards

Andreas Bie?mann

Regards Jens

#19 lore raw

PATCH 4/4 gpio: atmel: add copyright and remove error header info

In reply to Bo Shen

Am 2013-08-13 07:38, schrieb Bo Shen:

Quoted 12 lines
Signed-off-by Bo Shen <[email protected]>
---
 drivers/gpio/at91_gpio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/at91_gpio.c +0 -0
index 3de0844..72161dc 100644

drivers/gpio/at91_gpio.c line 5

Quoted 6 lines
drivers/gpio/at91_gpio.c +1 -1
@@ -1,5 +1,5 @@
 /*
- * Memory Setup stuff - taken from blob memsetup.S
+ * Copyright (C) 2013 Bo Shen <[email protected]>
  *
  * Copyright (C) 2009 Jens Scharsig (js_at_ng at scharsoft.de)
  *
Acked-by Jens Scharsig <[email protected]>