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 <sergio.paracuellos@xxxxxxxxx>
> > ---
> > 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 = to_mediatek_gpio(chip);
> > - unsigned long flags;
> > - u32 t;
> > -
> > - spin_lock_irqsave(&rg->lock, flags);
> > - t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
> > - t &= ~BIT(offset);
> > - mtk_gpio_w32(rg, GPIO_REG_CTRL, t);
> > - spin_unlock_irqrestore(&rg->lock, flags);
> > -
> > - return 0;
> > -}
> > -
> > -static int
> > -mediatek_gpio_direction_output(struct gpio_chip *chip,
> > - unsigned int offset, int value)
> > +mtk_gpio_r32(struct mtk_gc *rg, u32 offset)
> > {
> > - struct mtk_gc *rg = to_mediatek_gpio(chip);
> > - unsigned long flags;
> > - u32 t;
> > -
> > - spin_lock_irqsave(&rg->lock, flags);
> > - t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
> > - t |= BIT(offset);
> > - mtk_gpio_w32(rg, GPIO_REG_CTRL, t);
> > - mediatek_gpio_set(chip, offset, value);
> > - spin_unlock_irqrestore(&rg->lock, flags);
> > + struct gpio_chip *gc = &rg->chip;
> > + struct mtk_data *gpio_data = gpiochip_get_data(gc);
> >
> > - return 0;
> > -}
> > -
> > -static int
> > -mediatek_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> > -{
> > - struct mtk_gc *rg = to_mediatek_gpio(chip);
> > - u32 t = mtk_gpio_r32(rg, GPIO_REG_CTRL);
> > -
> > - return (t & BIT(offset)) ? GPIOF_DIR_OUT : GPIOF_DIR_IN;
> > + return gc->read_reg(gpio_data->gpio_membase + offset);
> > }
> >
> > static int
> > @@ -156,20 +96,22 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
> > memset(rg, 0, sizeof(*rg));
> >
> > spin_lock_init(&rg->lock);
> > + rg->bank = be32_to_cpu(*id);
> > +
> > + ret = bgpio_init(&rg->chip, &pdev->dev, 4,
> > + gpio_data->gpio_membase + GPIO_REG_DATA(rg->bank),
> > + gpio_data->gpio_membase + GPIO_REG_DSET(rg->bank),
> > + gpio_data->gpio_membase + GPIO_REG_DCLR(rg->bank),
> > + gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> > + gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> > + 0);
>
> This is the first problem I git. You've provided both 'dirout' and
> 'dirin' and that is not permitted. You've given exactly the same value
> for both, which isn't really meaningful.
> dirout is correct, dirin should be NULL.
True, I misunderstood the code I was looking at. Thanks for pointing this out.
>
> > + if (ret) {
> > + dev_err(&pdev->dev, "bgpio_init() failed\n");
> > + return ret;
> > + }
> >
> > - rg->chip.parent = &pdev->dev;
> > - rg->chip.label = dev_name(&pdev->dev);
>
> > - rg->chip.of_node = bank;
> > - rg->chip.base = MTK_BANK_WIDTH * be32_to_cpu(*id);
>
> These last two are still needed - bgpio_init doesn't set them up.
Mmmm chip.base is set by bgpio_init to -1 which is the correct
value to set it as Linus said.
>
> I think that is all in this patch.
>
> Thanks,
> NeilBrown
Best regards,
Sergio Paracuellos
>
> > - rg->chip.ngpio = MTK_BANK_WIDTH;
> > - rg->chip.direction_input = mediatek_gpio_direction_input;
> > - rg->chip.direction_output = mediatek_gpio_direction_output;
> > - rg->chip.get_direction = mediatek_gpio_get_direction;
> > - rg->chip.get = mediatek_gpio_get;
> > - rg->chip.set = mediatek_gpio_set;
> > if (gpio_data->gpio_irq_domain)
> > rg->chip.to_irq = mediatek_gpio_to_irq;
> > - rg->bank = be32_to_cpu(*id);
> >
> > ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
> > if (ret < 0) {
> > @@ -179,7 +121,7 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
> > }
> >
> > /* set polarity to low for all gpios */
> > - mtk_gpio_w32(rg, GPIO_REG_POL, 0);
> > + mtk_gpio_w32(rg, GPIO_REG_POL(rg->bank), 0);
> >
> > dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
> >
> > @@ -200,14 +142,14 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
> > if (!rg)
> > continue;
> >
> > - pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
> > + pending = mtk_gpio_r32(rg, GPIO_REG_STAT(rg->bank));
> >
> > for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
> > u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
> > (MTK_BANK_WIDTH * i) + bit);
> >
> > generic_handle_irq(map);
> > - mtk_gpio_w32(rg, GPIO_REG_STAT, BIT(bit));
> > + mtk_gpio_w32(rg, GPIO_REG_STAT(i), BIT(bit));
> > }
> > }
> > }
> > @@ -226,10 +168,12 @@ mediatek_gpio_irq_unmask(struct irq_data *d)
> > return;
> >
> > spin_lock_irqsave(&rg->lock, flags);
> > - rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
> > - fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
> > - mtk_gpio_w32(rg, GPIO_REG_REDGE, rise | (PIN_MASK(pin) & rg->rising));
> > - mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall | (PIN_MASK(pin) & rg->falling));
> > + rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
> > + fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
> > + mtk_gpio_w32(rg, GPIO_REG_REDGE(bank),
> > + rise | (PIN_MASK(pin) & rg->rising));
> > + mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank),
> > + fall | (PIN_MASK(pin) & rg->falling));
> > spin_unlock_irqrestore(&rg->lock, flags);
> > }
> >
> > @@ -247,10 +191,10 @@ mediatek_gpio_irq_mask(struct irq_data *d)
> > return;
> >
> > spin_lock_irqsave(&rg->lock, flags);
> > - rise = mtk_gpio_r32(rg, GPIO_REG_REDGE);
> > - fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE);
> > - mtk_gpio_w32(rg, GPIO_REG_FEDGE, fall & ~PIN_MASK(pin));
> > - mtk_gpio_w32(rg, GPIO_REG_REDGE, rise & ~PIN_MASK(pin));
> > + rise = mtk_gpio_r32(rg, GPIO_REG_REDGE(bank));
> > + fall = mtk_gpio_r32(rg, GPIO_REG_FEDGE(bank));
> > + mtk_gpio_w32(rg, GPIO_REG_FEDGE(bank), fall & ~PIN_MASK(pin));
> > + mtk_gpio_w32(rg, GPIO_REG_REDGE(bank), rise & ~PIN_MASK(pin));
> > spin_unlock_irqrestore(&rg->lock, flags);
> > }
> >
> > --
> > 2.7.4
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
