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

[Phil Edworthy]

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.

After Marek's feedback, I think we now have a reasonably clean solution.

>Regards,
>Simon

Thanks
Phil