[U-Boot] [PATCH] gpio: dwapb: Add support for port B
The IP supports two ports, A and B, each providing up to 32 gpios. The driver already creates a 2nd gpio bank by reading the 2nd node from DT, so this is quite a simple change to support the 2nd bank.
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com --- drivers/gpio/dwapb_gpio.c | 40 +++++++++++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c index 471e18a..dda0b42 100644 --- a/drivers/gpio/dwapb_gpio.c +++ b/drivers/gpio/dwapb_gpio.c @@ -21,6 +21,8 @@ DECLARE_GLOBAL_DATA_PTR;
#define GPIO_SWPORTA_DR 0x00 #define GPIO_SWPORTA_DDR 0x04 +#define GPIO_SWPORTB_DR 0x0C +#define GPIO_SWPORTB_DDR 0x10 #define GPIO_INTEN 0x30 #define GPIO_INTMASK 0x34 #define GPIO_INTTYPE_LEVEL 0x38 @@ -29,6 +31,7 @@ DECLARE_GLOBAL_DATA_PTR; #define GPIO_PORTA_DEBOUNCE 0x48 #define GPIO_PORTA_EOI 0x4c #define GPIO_EXT_PORTA 0x50 +#define GPIO_EXT_PORTB 0x54
struct gpio_dwapb_platdata { const char *name; @@ -41,7 +44,11 @@ static int dwapb_gpio_direction_input(struct udevice *dev, unsigned pin) { struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
- clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin); + if (plat->bank == 0) + clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin); + else + clrbits_le32(plat->base + GPIO_SWPORTB_DDR, 1 << pin); + return 0; }
@@ -50,12 +57,21 @@ static int dwapb_gpio_direction_output(struct udevice *dev, unsigned pin, { struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
- setbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin); + if (plat->bank == 0) + setbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin); + else + setbits_le32(plat->base + GPIO_SWPORTB_DDR, 1 << pin);
if (val) - setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin); + if (plat->bank == 0) + setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin); + else + setbits_le32(plat->base + GPIO_SWPORTB_DR, 1 << pin); else - clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin); + if (plat->bank == 0) + clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin); + else + clrbits_le32(plat->base + GPIO_SWPORTB_DR, 1 << pin);
return 0; } @@ -63,7 +79,11 @@ static int dwapb_gpio_direction_output(struct udevice *dev, unsigned pin, static int dwapb_gpio_get_value(struct udevice *dev, unsigned pin) { struct gpio_dwapb_platdata *plat = dev_get_platdata(dev); - return !!(readl(plat->base + GPIO_EXT_PORTA) & (1 << pin)); + + if (plat->bank == 0) + return !!(readl(plat->base + GPIO_EXT_PORTA) & (1 << pin)); + else + return !!(readl(plat->base + GPIO_EXT_PORTB) & (1 << pin)); }
@@ -72,9 +92,15 @@ static int dwapb_gpio_set_value(struct udevice *dev, unsigned pin, int val) struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
if (val) - setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin); + if (plat->bank == 0) + setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin); + else + setbits_le32(plat->base + GPIO_SWPORTB_DR, 1 << pin); else - clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin); + if (plat->bank == 0) + clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin); + else + clrbits_le32(plat->base + GPIO_SWPORTB_DR, 1 << pin);
return 0; }
On 11/02/2016 04:24 PM, Phil Edworthy wrote:
The IP supports two ports, A and B, each providing up to 32 gpios. The driver already creates a 2nd gpio bank by reading the 2nd node from DT, so this is quite a simple change to support the 2nd bank.
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com
drivers/gpio/dwapb_gpio.c | 40 +++++++++++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c index 471e18a..dda0b42 100644 --- a/drivers/gpio/dwapb_gpio.c +++ b/drivers/gpio/dwapb_gpio.c @@ -21,6 +21,8 @@ DECLARE_GLOBAL_DATA_PTR;
#define GPIO_SWPORTA_DR 0x00 #define GPIO_SWPORTA_DDR 0x04 +#define GPIO_SWPORTB_DR 0x0C +#define GPIO_SWPORTB_DDR 0x10 #define GPIO_INTEN 0x30 #define GPIO_INTMASK 0x34 #define GPIO_INTTYPE_LEVEL 0x38 @@ -29,6 +31,7 @@ DECLARE_GLOBAL_DATA_PTR; #define GPIO_PORTA_DEBOUNCE 0x48 #define GPIO_PORTA_EOI 0x4c #define GPIO_EXT_PORTA 0x50 +#define GPIO_EXT_PORTB 0x54
struct gpio_dwapb_platdata { const char *name; @@ -41,7 +44,11 @@ static int dwapb_gpio_direction_input(struct udevice *dev, unsigned pin) { struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
- clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
- if (plat->bank == 0)
clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);- else
clrbits_le32(plat->base + GPIO_SWPORTB_DDR, 1 << pin);
What about doing plat->base + plat->offset + GPIO_SWPORT_DDR ? Then you don't need this if-else stuff.
return 0; }
@@ -50,12 +57,21 @@ static int dwapb_gpio_direction_output(struct udevice *dev, unsigned pin, { struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
- setbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
if (plat->bank == 0)
setbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);else
setbits_le32(plat->base + GPIO_SWPORTB_DDR, 1 << pin);if (val)
setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
if (plat->bank == 0)setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);elsesetbits_le32(plat->base + GPIO_SWPORTB_DR, 1 << pin);
clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
if (plat->bank == 0)clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);elseclrbits_le32(plat->base + GPIO_SWPORTB_DR, 1 << pin);return 0;
} @@ -63,7 +79,11 @@ static int dwapb_gpio_direction_output(struct udevice *dev, unsigned pin, static int dwapb_gpio_get_value(struct udevice *dev, unsigned pin) { struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
- return !!(readl(plat->base + GPIO_EXT_PORTA) & (1 << pin));
- if (plat->bank == 0)
return !!(readl(plat->base + GPIO_EXT_PORTA) & (1 << pin));- else
return !!(readl(plat->base + GPIO_EXT_PORTB) & (1 << pin));}
@@ -72,9 +92,15 @@ static int dwapb_gpio_set_value(struct udevice *dev, unsigned pin, int val) struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
if (val)
setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
if (plat->bank == 0)setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);elsesetbits_le32(plat->base + GPIO_SWPORTB_DR, 1 << pin);
clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
if (plat->bank == 0)clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);elseclrbits_le32(plat->base + GPIO_SWPORTB_DR, 1 << pin);return 0;
}
Hi Marek,
On 02 November 2016 19:38, Marek Vasut wrote:
On 11/02/2016 04:24 PM, Phil Edworthy wrote:
The IP supports two ports, A and B, each providing up to 32 gpios. The driver already creates a 2nd gpio bank by reading the 2nd node from DT, so this is quite a simple change to support the 2nd bank.
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com
drivers/gpio/dwapb_gpio.c | 40 +++++++++++++++++++++++++++++++++----
1 file changed, 33 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c index 471e18a..dda0b42 100644 --- a/drivers/gpio/dwapb_gpio.c +++ b/drivers/gpio/dwapb_gpio.c @@ -21,6 +21,8 @@ DECLARE_GLOBAL_DATA_PTR;
#define GPIO_SWPORTA_DR 0x00 #define GPIO_SWPORTA_DDR 0x04 +#define GPIO_SWPORTB_DR 0x0C +#define GPIO_SWPORTB_DDR 0x10 #define GPIO_INTEN 0x30 #define GPIO_INTMASK 0x34 #define GPIO_INTTYPE_LEVEL 0x38 @@ -29,6 +31,7 @@ DECLARE_GLOBAL_DATA_PTR; #define GPIO_PORTA_DEBOUNCE 0x48 #define GPIO_PORTA_EOI 0x4c #define GPIO_EXT_PORTA 0x50 +#define GPIO_EXT_PORTB 0x54
struct gpio_dwapb_platdata { const char *name; @@ -41,7 +44,11 @@ static int dwapb_gpio_direction_input(struct udevice
*dev, unsigned pin)
{ struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
- clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
- if (plat->bank == 0)
clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);- else
clrbits_le32(plat->base + GPIO_SWPORTB_DDR, 1 << pin);What about doing plat->base + plat->offset + GPIO_SWPORT_DDR ? Then you don't need this if-else stuff.
Makes sense, I'll change it to use an offset.
return 0; }
@@ -50,12 +57,21 @@ static int dwapb_gpio_direction_output(struct udevice
*dev, unsigned pin,
{ struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
- setbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
if (plat->bank == 0)
setbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);else
setbits_le32(plat->base + GPIO_SWPORTB_DDR, 1 << pin);if (val)
setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
if (plat->bank == 0)setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);elsesetbits_le32(plat->base + GPIO_SWPORTB_DR, 1 << pin);
clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
if (plat->bank == 0)clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);elseclrbits_le32(plat->base + GPIO_SWPORTB_DR, 1 << pin);return 0;
} @@ -63,7 +79,11 @@ static int dwapb_gpio_direction_output(struct udevice
*dev, unsigned pin,
static int dwapb_gpio_get_value(struct udevice *dev, unsigned pin) { struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
- return !!(readl(plat->base + GPIO_EXT_PORTA) & (1 << pin));
- if (plat->bank == 0)
return !!(readl(plat->base + GPIO_EXT_PORTA) & (1 << pin));- else
return !!(readl(plat->base + GPIO_EXT_PORTB) & (1 << pin));}
@@ -72,9 +92,15 @@ static int dwapb_gpio_set_value(struct udevice *dev,
unsigned pin, int val)
struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
if (val)
setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
if (plat->bank == 0)setbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);elsesetbits_le32(plat->base + GPIO_SWPORTB_DR, 1 << pin);
clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);
if (plat->bank == 0)clrbits_le32(plat->base + GPIO_SWPORTA_DR, 1 << pin);elseclrbits_le32(plat->base + GPIO_SWPORTB_DR, 1 << pin);return 0;
}
-- Best regards, Marek Vasut
Thanks Phil
On 11/03/2016 08:39 AM, Phil Edworthy wrote:
Hi Marek,
On 02 November 2016 19:38, Marek Vasut wrote:
On 11/02/2016 04:24 PM, Phil Edworthy wrote:
The IP supports two ports, A and B, each providing up to 32 gpios. The driver already creates a 2nd gpio bank by reading the 2nd node from DT, so this is quite a simple change to support the 2nd bank.
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com
drivers/gpio/dwapb_gpio.c | 40 +++++++++++++++++++++++++++++++++----
1 file changed, 33 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c index 471e18a..dda0b42 100644 --- a/drivers/gpio/dwapb_gpio.c +++ b/drivers/gpio/dwapb_gpio.c @@ -21,6 +21,8 @@ DECLARE_GLOBAL_DATA_PTR;
#define GPIO_SWPORTA_DR 0x00 #define GPIO_SWPORTA_DDR 0x04 +#define GPIO_SWPORTB_DR 0x0C +#define GPIO_SWPORTB_DDR 0x10 #define GPIO_INTEN 0x30 #define GPIO_INTMASK 0x34 #define GPIO_INTTYPE_LEVEL 0x38 @@ -29,6 +31,7 @@ DECLARE_GLOBAL_DATA_PTR; #define GPIO_PORTA_DEBOUNCE 0x48 #define GPIO_PORTA_EOI 0x4c #define GPIO_EXT_PORTA 0x50 +#define GPIO_EXT_PORTB 0x54
struct gpio_dwapb_platdata { const char *name; @@ -41,7 +44,11 @@ static int dwapb_gpio_direction_input(struct udevice
*dev, unsigned pin)
{ struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
- clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
- if (plat->bank == 0)
clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);- else
clrbits_le32(plat->base + GPIO_SWPORTB_DDR, 1 << pin);What about doing plat->base + plat->offset + GPIO_SWPORT_DDR ? Then you don't need this if-else stuff.
Makes sense, I'll change it to use an offset.
Thanks!
Hi,
On 2 November 2016 at 13:38, Marek Vasut marex@denx.de wrote:
On 11/02/2016 04:24 PM, Phil Edworthy wrote:
The IP supports two ports, A and B, each providing up to 32 gpios. The driver already creates a 2nd gpio bank by reading the 2nd node from DT, so this is quite a simple change to support the 2nd bank.
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com
drivers/gpio/dwapb_gpio.c | 40 +++++++++++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c index 471e18a..dda0b42 100644 --- a/drivers/gpio/dwapb_gpio.c +++ b/drivers/gpio/dwapb_gpio.c @@ -21,6 +21,8 @@ DECLARE_GLOBAL_DATA_PTR;
#define GPIO_SWPORTA_DR 0x00 #define GPIO_SWPORTA_DDR 0x04 +#define GPIO_SWPORTB_DR 0x0C +#define GPIO_SWPORTB_DDR 0x10 #define GPIO_INTEN 0x30 #define GPIO_INTMASK 0x34 #define GPIO_INTTYPE_LEVEL 0x38 @@ -29,6 +31,7 @@ DECLARE_GLOBAL_DATA_PTR; #define GPIO_PORTA_DEBOUNCE 0x48 #define GPIO_PORTA_EOI 0x4c #define GPIO_EXT_PORTA 0x50 +#define GPIO_EXT_PORTB 0x54
struct gpio_dwapb_platdata { const char *name; @@ -41,7 +44,11 @@ static int dwapb_gpio_direction_input(struct udevice
*dev, unsigned pin)
{ struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
- clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
- if (plat->bank == 0)
- clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
- else
- clrbits_le32(plat->base + GPIO_SWPORTB_DDR, 1 << pin);
What about doing plat->base + plat->offset + GPIO_SWPORT_DDR ? Then you don't need this if-else stuff.
Yes. In fact this seems to be crying out to use a C structure instead.
struct gpio_port { uint32_t dr; uint32_t ddr; };
struct gpio_regs { struct gpio_port port[2]; uint32_t reserved[8]; uint32_t inten; ... }
Regards, Simon
Hi Simon,
On 2 November 2016 at 13:38, Marek Vasut marex@denx.de wrote:
On 11/02/2016 04:24 PM, Phil Edworthy wrote:
The IP supports two ports, A and B, each providing up to 32 gpios. The driver already creates a 2nd gpio bank by reading the 2nd node from DT, so this is quite a simple change to support the 2nd bank.
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com
drivers/gpio/dwapb_gpio.c | 40 +++++++++++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c index 471e18a..dda0b42 100644 --- a/drivers/gpio/dwapb_gpio.c +++ b/drivers/gpio/dwapb_gpio.c @@ -21,6 +21,8 @@ DECLARE_GLOBAL_DATA_PTR;
#define GPIO_SWPORTA_DR 0x00 #define GPIO_SWPORTA_DDR 0x04 +#define GPIO_SWPORTB_DR 0x0C +#define GPIO_SWPORTB_DDR 0x10 #define GPIO_INTEN 0x30 #define GPIO_INTMASK 0x34 #define GPIO_INTTYPE_LEVEL 0x38 @@ -29,6 +31,7 @@ DECLARE_GLOBAL_DATA_PTR; #define GPIO_PORTA_DEBOUNCE 0x48 #define GPIO_PORTA_EOI 0x4c #define GPIO_EXT_PORTA 0x50 +#define GPIO_EXT_PORTB 0x54
struct gpio_dwapb_platdata { const char *name; @@ -41,7 +44,11 @@ static int dwapb_gpio_direction_input(struct udevice *dev, unsigned pin) { struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
- clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
- if (plat->bank == 0)
- clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
- else
- clrbits_le32(plat->base + GPIO_SWPORTB_DDR, 1 << pin);
What about doing plat->base + plat->offset + GPIO_SWPORT_DDR ? Then you don't need this if-else stuff.
Yes. In fact this seems to be crying out to use a C structure instead.
struct gpio_port { uint32_t dr; uint32_t ddr; };
struct gpio_regs { struct gpio_port port[2]; uint32_t reserved[8]; uint32_t inten; ... }
Unfortunately not because the registers for each port are spread over the place, for example see the GPIO_EXT_PORTA/B registers.
After Marek's feedback, I think we now have a reasonably clean solution.
Regards, Simon
Thanks Phil
Hi Phil,
On 3 November 2016 at 08:02, Phil Edworthy phil.edworthy@renesas.com wrote:
Hi Simon,
On 2 November 2016 at 13:38, Marek Vasut marex@denx.de wrote:
On 11/02/2016 04:24 PM, Phil Edworthy wrote:
The IP supports two ports, A and B, each providing up to 32 gpios. The driver already creates a 2nd gpio bank by reading the 2nd node from DT, so this is quite a simple change to support the 2nd bank.
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com
drivers/gpio/dwapb_gpio.c | 40 +++++++++++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c index 471e18a..dda0b42 100644 --- a/drivers/gpio/dwapb_gpio.c +++ b/drivers/gpio/dwapb_gpio.c @@ -21,6 +21,8 @@ DECLARE_GLOBAL_DATA_PTR;
#define GPIO_SWPORTA_DR 0x00 #define GPIO_SWPORTA_DDR 0x04 +#define GPIO_SWPORTB_DR 0x0C +#define GPIO_SWPORTB_DDR 0x10 #define GPIO_INTEN 0x30 #define GPIO_INTMASK 0x34 #define GPIO_INTTYPE_LEVEL 0x38 @@ -29,6 +31,7 @@ DECLARE_GLOBAL_DATA_PTR; #define GPIO_PORTA_DEBOUNCE 0x48 #define GPIO_PORTA_EOI 0x4c #define GPIO_EXT_PORTA 0x50 +#define GPIO_EXT_PORTB 0x54
struct gpio_dwapb_platdata { const char *name; @@ -41,7 +44,11 @@ static int dwapb_gpio_direction_input(struct udevice *dev, unsigned pin) { struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
- clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
- if (plat->bank == 0)
- clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
- else
- clrbits_le32(plat->base + GPIO_SWPORTB_DDR, 1 << pin);
What about doing plat->base + plat->offset + GPIO_SWPORT_DDR ? Then you don't need this if-else stuff.
Yes. In fact this seems to be crying out to use a C structure instead.
struct gpio_port { uint32_t dr; uint32_t ddr; };
struct gpio_regs { struct gpio_port port[2]; uint32_t reserved[8]; uint32_t inten; ... }
Unfortunately not because the registers for each port are spread over the place, for example see the GPIO_EXT_PORTA/B registers.
What do you mean by 'spread all over the place'?
After Marek's feedback, I think we now have a reasonably clean solution.
Well I don't like it much, but it's up to you.
Regards, Simon
Hi Simon,
On 03 November 2016 15:49, Simon Glass wrote:
On 3 November 2016 at 08:02, Phil Edworthy phil.edworthy@renesas.com wrote:
On 2 November 2016 at 13:38, Marek Vasut marex@denx.de wrote:
On 11/02/2016 04:24 PM, Phil Edworthy wrote:
The IP supports two ports, A and B, each providing up to 32 gpios. The driver already creates a 2nd gpio bank by reading the 2nd node from DT, so this is quite a simple change to support the 2nd bank.
Signed-off-by: Phil Edworthy phil.edworthy@renesas.com
drivers/gpio/dwapb_gpio.c | 40
+++++++++++++++++++++++++++++++++-------
1 file changed, 33 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c index 471e18a..dda0b42 100644 --- a/drivers/gpio/dwapb_gpio.c +++ b/drivers/gpio/dwapb_gpio.c @@ -21,6 +21,8 @@ DECLARE_GLOBAL_DATA_PTR;
#define GPIO_SWPORTA_DR 0x00 #define GPIO_SWPORTA_DDR 0x04 +#define GPIO_SWPORTB_DR 0x0C +#define GPIO_SWPORTB_DDR 0x10 #define GPIO_INTEN 0x30 #define GPIO_INTMASK 0x34 #define GPIO_INTTYPE_LEVEL 0x38 @@ -29,6 +31,7 @@ DECLARE_GLOBAL_DATA_PTR; #define GPIO_PORTA_DEBOUNCE 0x48 #define GPIO_PORTA_EOI 0x4c #define GPIO_EXT_PORTA 0x50 +#define GPIO_EXT_PORTB 0x54
struct gpio_dwapb_platdata { const char *name; @@ -41,7 +44,11 @@ static int dwapb_gpio_direction_input(struct udevice
*dev, unsigned pin)
{ struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
- clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
- if (plat->bank == 0)
- clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
- else
- clrbits_le32(plat->base + GPIO_SWPORTB_DDR, 1 << pin);
What about doing plat->base + plat->offset + GPIO_SWPORT_DDR ? Then
you
don't need this if-else stuff.
Yes. In fact this seems to be crying out to use a C structure instead.
struct gpio_port { uint32_t dr; uint32_t ddr; };
struct gpio_regs { struct gpio_port port[2]; uint32_t reserved[8]; uint32_t inten; ... }
Unfortunately not because the registers for each port are spread over the
place,
for example see the GPIO_EXT_PORTA/B registers.
What do you mean by 'spread all over the place'?
The registers for port A are 0x0, 0x4, 0x50, whereas port B are 0xc, 0x10, 0x54, so you can't use a 'struct gpio_port' simply in the way you have suggested. Of course it can be done, just not so cleanly.
After Marek's feedback, I think we now have a reasonably clean solution.
Well I don't like it much, but it's up to you.
Ok, I'll change it.
Regards, Simon
Thanks Phil
On 11/03/2016 02:56 PM, Simon Glass wrote:
Hi,
On 2 November 2016 at 13:38, Marek Vasut <marex@denx.de mailto:marex@denx.de> wrote:
On 11/02/2016 04:24 PM, Phil Edworthy wrote:
The IP supports two ports, A and B, each providing up to 32 gpios. The driver already creates a 2nd gpio bank by reading the 2nd node from DT, so this is quite a simple change to support the 2nd bank.
Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com
mailto:phil.edworthy@renesas.com>
drivers/gpio/dwapb_gpio.c | 40 +++++++++++++++++++++++++++++++++------- 1 file changed, 33 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/dwapb_gpio.c b/drivers/gpio/dwapb_gpio.c index 471e18a..dda0b42 100644 --- a/drivers/gpio/dwapb_gpio.c +++ b/drivers/gpio/dwapb_gpio.c @@ -21,6 +21,8 @@ DECLARE_GLOBAL_DATA_PTR;
#define GPIO_SWPORTA_DR 0x00 #define GPIO_SWPORTA_DDR 0x04 +#define GPIO_SWPORTB_DR 0x0C +#define GPIO_SWPORTB_DDR 0x10 #define GPIO_INTEN 0x30 #define GPIO_INTMASK 0x34 #define GPIO_INTTYPE_LEVEL 0x38 @@ -29,6 +31,7 @@ DECLARE_GLOBAL_DATA_PTR; #define GPIO_PORTA_DEBOUNCE 0x48 #define GPIO_PORTA_EOI 0x4c #define GPIO_EXT_PORTA 0x50 +#define GPIO_EXT_PORTB 0x54
struct gpio_dwapb_platdata { const char *name; @@ -41,7 +44,11 @@ static int dwapb_gpio_direction_input(struct
udevice *dev, unsigned pin)
{ struct gpio_dwapb_platdata *plat = dev_get_platdata(dev);
- clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
- if (plat->bank == 0)
- clrbits_le32(plat->base + GPIO_SWPORTA_DDR, 1 << pin);
- else
- clrbits_le32(plat->base + GPIO_SWPORTB_DDR, 1 << pin);
What about doing plat->base + plat->offset + GPIO_SWPORT_DDR ? Then you don't need this if-else stuff.
Yes. In fact this seems to be crying out to use a C structure instead.
struct gpio_port { uint32_t dr; uint32_t ddr; };
struct gpio_regs { struct gpio_port port[2]; uint32_t reserved[8]; uint32_t inten; ... }
Please no c-structure register definitions, this does not scale :(
Hi Marek,
On 03 November 2016 17:49, Marek Vasut wrote:
On 11/03/2016 02:56 PM, Simon Glass wrote:
On 2 November 2016 at 13:38, Marek Vasut <marex@denx.de mailto:marex@denx.de> wrote:
On 11/02/2016 04:24 PM, Phil Edworthy wrote:
<snip>
Please no c-structure register definitions, this does not scale :(
I think this is a long standing issue... For some IP blocks using a C-structure for register definitions is painful. On the other hand, for some others it may make the code more readable.
In this case, I would tend to agree with Simon. However, I'll happily use whatever you two agree to.
Best regards Phil
On 11/04/2016 09:09 AM, Phil Edworthy wrote:
Hi Marek,
Hi,
On 03 November 2016 17:49, Marek Vasut wrote:
On 11/03/2016 02:56 PM, Simon Glass wrote:
On 2 November 2016 at 13:38, Marek Vasut <marex@denx.de mailto:marex@denx.de> wrote:
On 11/02/2016 04:24 PM, Phil Edworthy wrote:
<snip> > Please no c-structure register definitions, this does not scale :( I think this is a long standing issue... For some IP blocks using a C-structure for register definitions is painful. On the other hand, for some others it may make the code more readable.
In this case, I would tend to agree with Simon. However, I'll happily use whatever you two agree to.
Since the code uses macros, let's not make it inconsistent.
participants (3)
-
Marek Vasut -
Phil Edworthy -
Simon Glass