Re: [PATCH 1/8] staging: mt7621-gpio: make use 'bgpio_init' from GPIO_GENERIC
On Sat, Jun 09 2018, Sergio Paracuellos wrote:
> Gpio complexity is just masking the fact that offset is always
> 0..n and writes to bits 0..n of some memory address. Because
> of this whole thing can just me converted to use GPIO_GENERIC
> and avoid duplications of a lot of driver custom functions.
> So use bgpio_init instead of custom code adding GPIO_GENERIC
> dependency to the Kconfig file. Also to make easier using
> bgpio_init function offset for each gpio bank, enumeration
> where register were defined has been replaced in favour of
> some macros which handle each gpio offset taking into account
> the bank where are located. Because of this change write and
> read functions which are being used for remaining irq handling
> stuff have been updated also as well as its dependencies along
> the code.
Thanks for this. It is a big change, and unsurprisingly there are a few
bugs.
I haven't actually got the driver working yet so there is still
something not quite right after the things listed here get fixed.
Firstly, I don't think it is useful to create the macros that you
describe above. Every time we access a register, we have an 'rg' which
has a bank number in it so I think it is much better to do the
multiplication in the read/write functions rather than in the macro.
But that isn't a bug exactly read on..
>
> Signed-off-by: Sergio Paracuellos
> ---
> drivers/staging/mt7621-gpio/Kconfig | 1 +
> drivers/staging/mt7621-gpio/gpio-mt7621.c | 148
> ++
> 2 files changed, 47 insertions(+), 102 deletions(-)
>
> diff --git a/drivers/staging/mt7621-gpio/Kconfig
> b/drivers/staging/mt7621-gpio/Kconfig
> index c741ec3..f4453e8 100644
> --- a/drivers/staging/mt7621-gpio/Kconfig
> +++ b/drivers/staging/mt7621-gpio/Kconfig
> @@ -1,6 +1,7 @@
> config GPIO_MT7621
> bool "Mediatek GPIO Support"
> depends on SOC_MT7620 || SOC_MT7621
> + select GPIO_GENERIC
> select ARCH_REQUIRE_GPIOLIB
> help
> Say yes here to support the Mediatek SoC GPIO device
> diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> index 0c4fb4a..cc08505 100644
> --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> @@ -19,19 +19,18 @@
> #define MTK_BANK_WIDTH 32
> #define PIN_MASK(nr) (1UL << ((nr % MTK_BANK_WIDTH)))
>
> -enum mediatek_gpio_reg {
> - GPIO_REG_CTRL = 0,
> - GPIO_REG_POL,
> - GPIO_REG_DATA,
> - GPIO_REG_DSET,
> - GPIO_REG_DCLR,
> - GPIO_REG_REDGE,
> - GPIO_REG_FEDGE,
> - GPIO_REG_HLVL,
> - GPIO_REG_LLVL,
> - GPIO_REG_STAT,
> - GPIO_REG_EDGE,
> -};
> +#define GPIO_BANK_WIDE 0x04
> +#define GPIO_REG_CTRL(bank) (((bank) * GPIO_BANK_WIDE) + 0x00)
> +#define GPIO_REG_POL(bank) (((bank) * GPIO_BANK_WIDE) + 0x10)
> +#define GPIO_REG_DATA(bank) (((bank) * GPIO_BANK_WIDE) + 0x20)
> +#define GPIO_REG_DSET(bank) (((bank) * GPIO_BANK_WIDE) + 0x30)
> +#define GPIO_REG_DCLR(bank) (((bank) * GPIO_BANK_WIDE) + 0x40)
> +#define GPIO_REG_REDGE(bank) (((bank) * GPIO_BANK_WIDE) + 0x50)
> +#define GPIO_REG_FEDGE(bank) (((bank) * GPIO_BANK_WIDE) + 0x60)
> +#define GPIO_REG_HLVL(bank) (((bank) * GPIO_BANK_WIDE) + 0x70)
> +#define GPIO_REG_LLVL(bank) (((bank) * GPIO_BANK_WIDE) + 0x80)
> +#define GPIO_REG_STAT(bank) (((bank) * GPIO_BANK_WIDE) + 0x90)
> +#define GPIO_REG_EDGE(bank) (((bank) * GPIO_BANK_WIDE) + 0xA0)
>
> struct mtk_gc {
> struct gpio_chip chip;
> @@ -55,80 +54,21 @@ to_mediatek_gpio(struct gpio_chip *chip)
> }
>
> static inline void
> -mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
> +mtk_gpio_w32(struct mtk_gc *rg, u32 offset, u32 val)
> {
> - struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> - u32 offset = (reg * 0x10) + (rg->bank * 0x4);
> + struct gpio_chip *gc = &rg->chip;
> + struct mtk_data *gpio_data = gpiochip_get_data(gc);
>
> - iowrite32(val, gpio_data->gpio_membase + offset);
> + gc->write_reg(gpio_data->gpio_membase + offset, val);
> }
>
> static inline u32
> -mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
> -{
> - struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> - u32 offset = (reg * 0x10) + (rg->bank * 0x4);
> -
> - return ioread32(gpio_data->gpio_membase + offset);
> -}
> -
> -static void
> -mediatek_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
> -{
> - struct mtk_gc *rg = to_mediatek_gpio(chip);
> -
> - mtk_gpio_w32(rg, (value) ? GPIO_REG_DSET : GPIO_REG_DCLR, BIT(offset));
> -}
> -
> -static int
> -mediatek_gpio_get(struct gpio_chip *chip, unsigned int offset)
> -{
> - struct mtk_gc *rg = to_mediatek_gpio(chip);
> -
> - return !!(mtk_gpio_r32(rg, GPIO_REG_DATA) & BIT(offset));
> -}
> -
> -static int
> -mediatek_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> -{
> - struct mtk_gc *rg
Re: [PATCH 1/8] staging: mt7621-gpio: make use 'bgpio_init' from GPIO_GENERIC
On Sun, Jun 10, 2018 at 06:53:04PM +1000, NeilBrown wrote:
> On Sat, Jun 09 2018, Sergio Paracuellos wrote:
>
> > Gpio complexity is just masking the fact that offset is always
> > 0..n and writes to bits 0..n of some memory address. Because
> > of this whole thing can just me converted to use GPIO_GENERIC
> > and avoid duplications of a lot of driver custom functions.
> > So use bgpio_init instead of custom code adding GPIO_GENERIC
> > dependency to the Kconfig file. Also to make easier using
> > bgpio_init function offset for each gpio bank, enumeration
> > where register were defined has been replaced in favour of
> > some macros which handle each gpio offset taking into account
> > the bank where are located. Because of this change write and
> > read functions which are being used for remaining irq handling
> > stuff have been updated also as well as its dependencies along
> > the code.
>
> Thanks for this. It is a big change, and unsurprisingly there are a few
> bugs.
> I haven't actually got the driver working yet so there is still
> something not quite right after the things listed here get fixed.
Thanks for testing and review. See my comments below.
>
> Firstly, I don't think it is useful to create the macros that you
> describe above. Every time we access a register, we have an 'rg' which
> has a bank number in it so I think it is much better to do the
> multiplication in the read/write functions rather than in the macro.
> But that isn't a bug exactly read on..
Linus prefers to pass offsets instead bank number to that kind
of read/write functions and create the macros make easier to
handle both: offsets and bgpio_init parameters. That is why this have
been introduced. There is some similar code in 'gpio-brcmstb.c'
driver.
>
>
> >
> > Signed-off-by: Sergio Paracuellos
> > ---
> > drivers/staging/mt7621-gpio/Kconfig | 1 +
> > drivers/staging/mt7621-gpio/gpio-mt7621.c | 148
> > ++
> > 2 files changed, 47 insertions(+), 102 deletions(-)
> >
> > diff --git a/drivers/staging/mt7621-gpio/Kconfig
> > b/drivers/staging/mt7621-gpio/Kconfig
> > index c741ec3..f4453e8 100644
> > --- a/drivers/staging/mt7621-gpio/Kconfig
> > +++ b/drivers/staging/mt7621-gpio/Kconfig
> > @@ -1,6 +1,7 @@
> > config GPIO_MT7621
> > bool "Mediatek GPIO Support"
> > depends on SOC_MT7620 || SOC_MT7621
> > + select GPIO_GENERIC
> > select ARCH_REQUIRE_GPIOLIB
> > help
> > Say yes here to support the Mediatek SoC GPIO device
> > diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > index 0c4fb4a..cc08505 100644
> > --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > @@ -19,19 +19,18 @@
> > #define MTK_BANK_WIDTH 32
> > #define PIN_MASK(nr) (1UL << ((nr % MTK_BANK_WIDTH)))
> >
> > -enum mediatek_gpio_reg {
> > - GPIO_REG_CTRL = 0,
> > - GPIO_REG_POL,
> > - GPIO_REG_DATA,
> > - GPIO_REG_DSET,
> > - GPIO_REG_DCLR,
> > - GPIO_REG_REDGE,
> > - GPIO_REG_FEDGE,
> > - GPIO_REG_HLVL,
> > - GPIO_REG_LLVL,
> > - GPIO_REG_STAT,
> > - GPIO_REG_EDGE,
> > -};
> > +#define GPIO_BANK_WIDE 0x04
> > +#define GPIO_REG_CTRL(bank) (((bank) * GPIO_BANK_WIDE) + 0x00)
> > +#define GPIO_REG_POL(bank) (((bank) * GPIO_BANK_WIDE) + 0x10)
> > +#define GPIO_REG_DATA(bank) (((bank) * GPIO_BANK_WIDE) + 0x20)
> > +#define GPIO_REG_DSET(bank) (((bank) * GPIO_BANK_WIDE) + 0x30)
> > +#define GPIO_REG_DCLR(bank) (((bank) * GPIO_BANK_WIDE) + 0x40)
> > +#define GPIO_REG_REDGE(bank) (((bank) * GPIO_BANK_WIDE) + 0x50)
> > +#define GPIO_REG_FEDGE(bank) (((bank) * GPIO_BANK_WIDE) + 0x60)
> > +#define GPIO_REG_HLVL(bank) (((bank) * GPIO_BANK_WIDE) + 0x70)
> > +#define GPIO_REG_LLVL(bank) (((bank) * GPIO_BANK_WIDE) + 0x80)
> > +#define GPIO_REG_STAT(bank) (((bank) * GPIO_BANK_WIDE) + 0x90)
> > +#define GPIO_REG_EDGE(bank) (((bank) * GPIO_BANK_WIDE) + 0xA0)
> >
> > struct mtk_gc {
> > struct gpio_chip chip;
> > @@ -55,80 +54,21 @@ to_mediatek_gpio(struct gpio_chip *chip)
> > }
> >
> > static inline void
> > -mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
> > +mtk_gpio_w32(struct mtk_gc *rg, u32 offset, u32 val)
> > {
> > - struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> > - u32 offset = (reg * 0x10) + (rg->bank * 0x4);
> > + struct gpio_chip *gc = &rg->chip;
> > + struct mtk_data *gpio_data = gpiochip_get_data(gc);
> >
> > - iowrite32(val, gpio_data->gpio_membase + offset);
> > + gc->write_reg(gpio_data->gpio_membase + offset, val);
> > }
> >
> > static inline u32
> > -mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
> > -{
> > - struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> > - u32 offset = (reg * 0x10) + (rg->bank * 0x4);
> > -
> > - return ioread32(gpio_data->gpio_membase + offset);
> > -}
> > -
> > -static void
> > -med
