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