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