| Message ID | 1478100259-6574-1-git-send-email-phil.edworthy@renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
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); > + 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; > } >
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); > > + 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; > > } > > > > > -- > 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.
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; }
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(-)