[U-Boot] [PATCH] gpio: dwapb: Add support for port B

[Simon Glass]

Hi Phil,

On 3 November 2016 at 08:02, Phil Edworthy <phil.edworthy at renesas.com> wrote:
>
> Hi Simon,
>
> >On 2 November 2016 at 13:38, Marek Vasut <marex at 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 at 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