Hi Simon,
On 03 November 2016 15:49, Simon Glass wrote:
> On 3 November 2016 at 08:02, Phil Edworthy <phil.edwor...@renesas.com>
> wrote:
> > >On 2 November 2016 at 13:38, Marek Vasut <ma...@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.edwor...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
