[1/2] gpio: mediatek: add driver for MT7621

Message ID 1527924610-13135-2-git-send-email-sergio.paracuellos@gmail.com
State New
Headers show
Series
  • gpio: mediatek: driver for gpio chip in MT7621 SoC
Related show

Commit Message

Sergio Paracuellos June 2, 2018, 7:30 a.m.
Add driver support for gpio of MT7621 SoC.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Reviewed-by: NeilBrown <neil@brown.name>
---
 drivers/gpio/Kconfig       |   7 +
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-mt7621.c | 370 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 378 insertions(+)
 create mode 100644 drivers/gpio/gpio-mt7621.c

Comments

Linus Walleij June 8, 2018, 11:59 a.m. | #1
Hi Sergio!

Thanks for your patch!

Given that we have combined pin control and GPIO drivers for
almost all Mediatek chips in drivers/pinctrl/mediatek/*
I would ideally like to have some input from the Mediatek
maintainers (especially Sean Wang) on this, especially:

- Is MT7621 a non-pincontrol GPIO controller, or can it
  eventually use pin control as a back-end? Will a separate
  pin control driver appear later for this SoC?

- Would it make sense to have a combined driver just like
  for the other Mediatek SoCs in drivers/pinctrl/mediatek?
  If this GPIO controller does not do pin control I understand
  why it is submitted as a GPIO driver only.

drivers/pinctrl/mediatek/pinctrl-mt7622.c is suspiciously
similarly named. Is this a relative or just as different as
night and day?

Also you can see that this driver has a built-in GPIO driver,
using an external interrupt.

On Sat, Jun 2, 2018 at 9:30 AM, Sergio Paracuellos
<sergio.paracuellos@gmail.com> wrote:

> Add driver support for gpio of MT7621 SoC.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> Reviewed-by: NeilBrown <neil@brown.name>

> +config GPIO_MT7621
> +       bool "Mediatek GPIO Support"

Specify in the option that it is for MT7621 as there are so
many of these now.

> +       depends on SOC_MT7620 || SOC_MT7621

Can we enable COMPILE_TEST?

> +       select GPIOLIB_IRQCHIP

You are not using this so I guess remove that line.

> +       help
> +         Say yes here to support the Mediatek SoC GPIO device

Elaborate on SoC type please.

> +#include <linux/err.h>
> +#include <linux/gpio.h>

This should not be included in new code, just remove it
(should be fine).

> +#include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#define MTK_BANK_CNT           3
> +#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,
> +};

So these are all registers? I usually prefer some #defines
for each offset.

In this case I definately think you should define them all
relative the memory base:

#define GPIO_REG_CTRL0 0x00
#define GPIO_REG_CTRL1 0x04
(...)

as that makes it easier to use GPIO_GENERIC as described
below.

> +struct mtk_gc {
> +       struct gpio_chip chip;
> +       spinlock_t lock;
> +       int bank;
> +       u32 rising;
> +       u32 falling;
> +};

> +struct mtk_data {
> +       void __iomem *gpio_membase;
> +       int gpio_irq;
> +       struct irq_domain *gpio_irq_domain;
> +       struct mtk_gc gc_map[MTK_BANK_CNT];
> +};

These two state containers make it a bit confusing, maybe it
can be alleviated with a bit of comments explaining what is
going on?

> +static inline struct mtk_gc *
> +to_mediatek_gpio(struct gpio_chip *chip)
> +{
> +       return container_of(chip, struct mtk_gc, chip);
> +}

This is a bit confusing as the other state container comes
out of the gpio_chip, but that is a member of the mtk_gc.
But maybe this is the only way to do it.

> +static inline void
> +mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
> +{
> +       struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> +       u32 offset = (reg * 0x10) + (rg->bank * 0x4);

So the register stride is 0x10 and the bank is 0x4 wide?

Following this gets terse so I guess that is why I prefer to
just #define the registers and have them relate directly
to membase and no reader/writer functions like this.
But it's not a strong opinion, maybe you have
good reasons for having it like this.

> +       iowrite32(val, gpio_data->gpio_membase + offset);
> +}
> +
> +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);
> +}

These two could possibly be wrapped into a custom regmap
as well, that makes for nice abstraction. Just an idea.

> +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)
> +{
> +       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);
> +
> +       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;
> +}

How do these calls end up?

If this complexity is just masking the fact that offset is always
0..n and writes to bits 0..n of some memory address, this whole
thing can just me converted to use GPIO_GENERIC and replace all
the code from mtk_gpio_w32() to here.

Given that MTK_BANK_WIDTH is 32, I think it's very likely that
you can just use GPIO_GENERIC and send in the memory
offsets for all these registers to bgpio_init().

As bonus you get a proven implementation supporting
.get/set_multiple() at the same time.

> +static int
> +mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
> +{
> +       struct mtk_data *gpio_data = gpiochip_get_data(chip);
> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
> +
> +       return irq_create_mapping(gpio_data->gpio_irq_domain,
> +                                 pin + (rg->bank * MTK_BANK_WIDTH));
> +}

So this is the result of a custom IRQdomain because you
can't use the generic GPIO IRQ lib.  Oh well, we have to live
with it I guess.

> +static int
> +mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
> +{
> +       struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
> +       const __be32 *id = of_get_property(bank, "reg", NULL);
> +       struct mtk_gc *rg;
> +       int ret;
> +
> +       if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
> +               return -EINVAL;
> +
> +       rg = &gpio_data->gc_map[be32_to_cpu(*id)];
> +       memset(rg, 0, sizeof(*rg));
> +
> +       spin_lock_init(&rg->lock);
> +
> +       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);

Nope, do not ever assign base in a new driver. Set this to -1.

New systems should use dynamic IRQ base assignment, and
userspace should use the character device to access GPIOs
if need be.

> +       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);

As stated I think this can be replaced with bgpio_init() and
selecting GPIOCHIP_GENERIC.

> +static void
> +mediatek_gpio_irq_handler(struct irq_desc *desc)
> +{
> +       struct mtk_data *gpio_data = irq_desc_get_handler_data(desc);
> +       int i;
> +
> +       for (i = 0; i < MTK_BANK_CNT; i++) {
> +               struct mtk_gc *rg = &gpio_data->gc_map[i];
> +               unsigned long pending;
> +               int bit;
> +
> +               if (!rg)
> +                       continue;
> +
> +               pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
> +
> +               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));
> +               }
> +       }
> +}
> +
> +static void
> +mediatek_gpio_irq_unmask(struct irq_data *d)
> +{
> +       struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
> +       int pin = d->hwirq;
> +       int bank = pin / MTK_BANK_WIDTH;
> +       struct mtk_gc *rg = &gpio_data->gc_map[bank];
> +       unsigned long flags;
> +       u32 rise, fall;
> +
> +       if (!rg)
> +               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));
> +       spin_unlock_irqrestore(&rg->lock, flags);
> +}
> +
> +static void
> +mediatek_gpio_irq_mask(struct irq_data *d)
> +{
> +       struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
> +       int pin = d->hwirq;
> +       int bank = pin / MTK_BANK_WIDTH;
> +       struct mtk_gc *rg = &gpio_data->gc_map[bank];
> +       unsigned long flags;
> +       u32 rise, fall;
> +
> +       if (!rg)
> +               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));
> +       spin_unlock_irqrestore(&rg->lock, flags);
> +}


Looks OK.

> +static int
> +mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
> +{
> +       struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
> +       int pin = d->hwirq;
> +       int bank = pin / MTK_BANK_WIDTH;
> +       struct mtk_gc *rg = &gpio_data->gc_map[bank];
> +       u32 mask = PIN_MASK(pin);
> +
> +       if (!rg)
> +               return -1;
> +
> +       if (type == IRQ_TYPE_PROBE) {
> +               if ((rg->rising | rg->falling) & mask)
> +                       return 0;
> +
> +               type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING;
> +       }
> +
> +       if (type & IRQ_TYPE_EDGE_RISING)
> +               rg->rising |= mask;
> +       else
> +               rg->rising &= ~mask;
> +
> +       if (type & IRQ_TYPE_EDGE_FALLING)
> +               rg->falling |= mask;
> +       else
> +               rg->falling &= ~mask;

I don't understand this, the register map contains:
GPIO_REG_HLVL, GPIO_REG_LLVL, yet high/low level
interrupts are not implemented? Why not? Can't be that hard
now that you fixed everything else!

> +static struct irq_chip mediatek_gpio_irq_chip = {
> +       .name           = "GPIO",
> +       .irq_unmask     = mediatek_gpio_irq_unmask,
> +       .irq_mask       = mediatek_gpio_irq_mask,
> +       .irq_mask_ack   = mediatek_gpio_irq_mask,
> +       .irq_set_type   = mediatek_gpio_irq_type,
> +};

When implementing custom irqchips it is important to also
implement .irq_request_resources() and
.irq_release_resources() and make sure these call
gpiochip_[un]lock_as_irq().

See for example
drivers/gpio/gpio-dwapb.c

for an example.

> +static const struct of_device_id mediatek_gpio_match[] = {
> +       { .compatible = "mediatek,mt7621-gpio" },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, mediatek_gpio_match);
> +
> +static struct platform_driver mediatek_gpio_driver = {
> +       .probe = mediatek_gpio_probe,
> +       .driver = {
> +               .name = "mt7621_gpio",
> +               .of_match_table = mediatek_gpio_match,
> +       },
> +};
> +
> +module_platform_driver(mediatek_gpio_driver);

If you're not implementing .remove() I don't think this will
really work fine as a module. Also the Kconfig is a bool.

I guess you want to use
builtin_platform_driver()?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij June 8, 2018, 12:17 p.m. | #2
A second thought:

On Fri, Jun 8, 2018 at 1:59 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> +       select GPIOLIB_IRQCHIP
>
> You are not using this so I guess remove that line.
(...)
>> +static int
>> +mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
>> +{
>> +       struct mtk_data *gpio_data = gpiochip_get_data(chip);
>> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
>> +
>> +       return irq_create_mapping(gpio_data->gpio_irq_domain,
>> +                                 pin + (rg->bank * MTK_BANK_WIDTH));
>> +}
>
> So this is the result of a custom IRQdomain because you
> can't use the generic GPIO IRQ lib.  Oh well, we have to live
> with it I guess.

I think maybe you can actually use the generic GPIO IRQCHIP.

It is OK to call gpiochip_set_chained_irqchip() several times.
If you just mark the line with IRQF_SHARED then the handler
will just loop over all three banks until you find a hit, provided
you code it up properly.

There were some problems with removing an irqchip like
that but your driver is anyway a bool so I think it might work
just fine.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergio Paracuellos June 9, 2018, 5:54 a.m. | #3
On Fri, Jun 8, 2018 at 1:59 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> Hi Sergio!
>
> Thanks for your patch!

Hi Linus,

First of all, thanks for your feedback and effort reviewing this.

>
> On Sat, Jun 2, 2018 at 9:30 AM, Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
>
>> Add driver support for gpio of MT7621 SoC.
>>
>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
>> Reviewed-by: NeilBrown <neil@brown.name>
>
>> +config GPIO_MT7621
>> +       bool "Mediatek GPIO Support"
>
> Specify in the option that it is for MT7621 as there are so
> many of these now.

I see. I will.

>
>> +       depends on SOC_MT7620 || SOC_MT7621
>
> Can we enable COMPILE_TEST?

Sure.

>
>> +       select GPIOLIB_IRQCHIP
>
> You are not using this so I guess remove that line.
>
>> +       help
>> +         Say yes here to support the Mediatek SoC GPIO device
>
> Elaborate on SoC type please.
>
>> +#include <linux/err.h>
>> +#include <linux/gpio.h>
>
> This should not be included in new code, just remove it
> (should be fine).

If I am correct this header contains GPIOF_DIR_OUT and GPIOF_DIR_IN
definitions and mediatek_gpio_set function was using them. That's the
reason why this was included.

>
>> +#include <linux/gpio/driver.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/module.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/spinlock.h>
>> +
>> +#define MTK_BANK_CNT           3
>> +#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,
>> +};
>
> So these are all registers? I usually prefer some #defines
> for each offset.
>
> In this case I definately think you should define them all
> relative the memory base:
>
> #define GPIO_REG_CTRL0 0x00
> #define GPIO_REG_CTRL1 0x04
> (...)
>
> as that makes it easier to use GPIO_GENERIC as described
> below.

I see. Agreed. Will change them.

>
>> +struct mtk_gc {
>> +       struct gpio_chip chip;
>> +       spinlock_t lock;
>> +       int bank;
>> +       u32 rising;
>> +       u32 falling;
>> +};
>
>> +struct mtk_data {
>> +       void __iomem *gpio_membase;
>> +       int gpio_irq;
>> +       struct irq_domain *gpio_irq_domain;
>> +       struct mtk_gc gc_map[MTK_BANK_CNT];
>> +};
>
> These two state containers make it a bit confusing, maybe it
> can be alleviated with a bit of comments explaining what is
> going on?

Sure.

>
>> +static inline struct mtk_gc *
>> +to_mediatek_gpio(struct gpio_chip *chip)
>> +{
>> +       return container_of(chip, struct mtk_gc, chip);
>> +}
>
> This is a bit confusing as the other state container comes
> out of the gpio_chip, but that is a member of the mtk_gc.
> But maybe this is the only way to do it.

With the comments in the state containers should be more clear.

>
>> +static inline void
>> +mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
>> +{
>> +       struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
>> +       u32 offset = (reg * 0x10) + (rg->bank * 0x4);
>
> So the register stride is 0x10 and the bank is 0x4 wide?
>
> Following this gets terse so I guess that is why I prefer to
> just #define the registers and have them relate directly
> to membase and no reader/writer functions like this.
> But it's not a strong opinion, maybe you have
> good reasons for having it like this.

Not really. The original code was as it is here and with my
cleanups I didn't make that kind of changes. And it make sense
to change them because readability gets improve in the way you are
pointing out here.

>
>> +       iowrite32(val, gpio_data->gpio_membase + offset);
>> +}
>> +
>> +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);
>> +}
>
> These two could possibly be wrapped into a custom regmap
> as well, that makes for nice abstraction. Just an idea.
>
>> +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)
>> +{
>> +       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);
>> +
>> +       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;
>> +}
>
> How do these calls end up?
>
> If this complexity is just masking the fact that offset is always
> 0..n and writes to bits 0..n of some memory address, this whole
> thing can just me converted to use GPIO_GENERIC and replace all
> the code from mtk_gpio_w32() to here.
>
> Given that MTK_BANK_WIDTH is 32, I think it's very likely that
> you can just use GPIO_GENERIC and send in the memory
> offsets for all these registers to bgpio_init().

I'll give it a try using this.

>
> As bonus you get a proven implementation supporting
> .get/set_multiple() at the same time.
>
>> +static int
>> +mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
>> +{
>> +       struct mtk_data *gpio_data = gpiochip_get_data(chip);
>> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
>> +
>> +       return irq_create_mapping(gpio_data->gpio_irq_domain,
>> +                                 pin + (rg->bank * MTK_BANK_WIDTH));
>> +}
>
> So this is the result of a custom IRQdomain because you
> can't use the generic GPIO IRQ lib.  Oh well, we have to live
> with it I guess.
>
>> +static int
>> +mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>> +{
>> +       struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
>> +       const __be32 *id = of_get_property(bank, "reg", NULL);
>> +       struct mtk_gc *rg;
>> +       int ret;
>> +
>> +       if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
>> +               return -EINVAL;
>> +
>> +       rg = &gpio_data->gc_map[be32_to_cpu(*id)];
>> +       memset(rg, 0, sizeof(*rg));
>> +
>> +       spin_lock_init(&rg->lock);
>> +
>> +       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);
>
> Nope, do not ever assign base in a new driver. Set this to -1.
>
> New systems should use dynamic IRQ base assignment, and
> userspace should use the character device to access GPIOs
> if need be.
>

Thanks for your explanation in this.

>> +       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);
>
> As stated I think this can be replaced with bgpio_init() and
> selecting GPIOCHIP_GENERIC.
>
If it works much more clean with that, yes. Thanks.

>> +static void
>> +mediatek_gpio_irq_handler(struct irq_desc *desc)
>> +{
>> +       struct mtk_data *gpio_data = irq_desc_get_handler_data(desc);
>> +       int i;
>> +
>> +       for (i = 0; i < MTK_BANK_CNT; i++) {
>> +               struct mtk_gc *rg = &gpio_data->gc_map[i];
>> +               unsigned long pending;
>> +               int bit;
>> +
>> +               if (!rg)
>> +                       continue;
>> +
>> +               pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
>> +
>> +               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));
>> +               }
>> +       }
>> +}
>> +
>> +static void
>> +mediatek_gpio_irq_unmask(struct irq_data *d)
>> +{
>> +       struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
>> +       int pin = d->hwirq;
>> +       int bank = pin / MTK_BANK_WIDTH;
>> +       struct mtk_gc *rg = &gpio_data->gc_map[bank];
>> +       unsigned long flags;
>> +       u32 rise, fall;
>> +
>> +       if (!rg)
>> +               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));
>> +       spin_unlock_irqrestore(&rg->lock, flags);
>> +}
>> +
>> +static void
>> +mediatek_gpio_irq_mask(struct irq_data *d)
>> +{
>> +       struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
>> +       int pin = d->hwirq;
>> +       int bank = pin / MTK_BANK_WIDTH;
>> +       struct mtk_gc *rg = &gpio_data->gc_map[bank];
>> +       unsigned long flags;
>> +       u32 rise, fall;
>> +
>> +       if (!rg)
>> +               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));
>> +       spin_unlock_irqrestore(&rg->lock, flags);
>> +}
>
>
> Looks OK.
>
>> +static int
>> +mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
>> +{
>> +       struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
>> +       int pin = d->hwirq;
>> +       int bank = pin / MTK_BANK_WIDTH;
>> +       struct mtk_gc *rg = &gpio_data->gc_map[bank];
>> +       u32 mask = PIN_MASK(pin);
>> +
>> +       if (!rg)
>> +               return -1;
>> +
>> +       if (type == IRQ_TYPE_PROBE) {
>> +               if ((rg->rising | rg->falling) & mask)
>> +                       return 0;
>> +
>> +               type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING;
>> +       }
>> +
>> +       if (type & IRQ_TYPE_EDGE_RISING)
>> +               rg->rising |= mask;
>> +       else
>> +               rg->rising &= ~mask;
>> +
>> +       if (type & IRQ_TYPE_EDGE_FALLING)
>> +               rg->falling |= mask;
>> +       else
>> +               rg->falling &= ~mask;
>
> I don't understand this, the register map contains:
> GPIO_REG_HLVL, GPIO_REG_LLVL, yet high/low level
> interrupts are not implemented? Why not? Can't be that hard
> now that you fixed everything else!

The original code didn't has support for them so I
didn't add them also. But if they are supported in the chip,
I think those two should be added also.

>
>> +static struct irq_chip mediatek_gpio_irq_chip = {
>> +       .name           = "GPIO",
>> +       .irq_unmask     = mediatek_gpio_irq_unmask,
>> +       .irq_mask       = mediatek_gpio_irq_mask,
>> +       .irq_mask_ack   = mediatek_gpio_irq_mask,
>> +       .irq_set_type   = mediatek_gpio_irq_type,
>> +};
>
> When implementing custom irqchips it is important to also
> implement .irq_request_resources() and
> .irq_release_resources() and make sure these call
> gpiochip_[un]lock_as_irq().
>
> See for example
> drivers/gpio/gpio-dwapb.c
>
> for an example.

I will, thanks.

>
>> +static const struct of_device_id mediatek_gpio_match[] = {
>> +       { .compatible = "mediatek,mt7621-gpio" },
>> +       {},
>> +};
>> +MODULE_DEVICE_TABLE(of, mediatek_gpio_match);
>> +
>> +static struct platform_driver mediatek_gpio_driver = {
>> +       .probe = mediatek_gpio_probe,
>> +       .driver = {
>> +               .name = "mt7621_gpio",
>> +               .of_match_table = mediatek_gpio_match,
>> +       },
>> +};
>> +
>> +module_platform_driver(mediatek_gpio_driver);
>
> If you're not implementing .remove() I don't think this will
> really work fine as a module. Also the Kconfig is a bool.
>
> I guess you want to use
> builtin_platform_driver()?

Yes, That should be definitely the one to use :-).

>
> Yours,
> Linus Walleij

I think the best way to achieve this is make all of this changes in
the actual staging driver sending a new cleanups patch series
in order to let NeilBrown to can test them and when we think this is
clean enough again send you v2 with all of the changes pointed out here.
So... it is up to you, Neil the way to go here. Just let me know.

Thanks in advance.

Best regards,
    Sergio Paracuellos
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergio Paracuellos June 9, 2018, 6:17 a.m. | #4
On Fri, Jun 8, 2018 at 2:17 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> A second thought:

Second thought is always a good thing :-)

>
> On Fri, Jun 8, 2018 at 1:59 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>>> +       select GPIOLIB_IRQCHIP
>>
>> You are not using this so I guess remove that line.
> (...)
>>> +static int
>>> +mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
>>> +{
>>> +       struct mtk_data *gpio_data = gpiochip_get_data(chip);
>>> +       struct mtk_gc *rg = to_mediatek_gpio(chip);
>>> +
>>> +       return irq_create_mapping(gpio_data->gpio_irq_domain,
>>> +                                 pin + (rg->bank * MTK_BANK_WIDTH));
>>> +}
>>
>> So this is the result of a custom IRQdomain because you
>> can't use the generic GPIO IRQ lib.  Oh well, we have to live
>> with it I guess.
>
> I think maybe you can actually use the generic GPIO IRQCHIP.
>
> It is OK to call gpiochip_set_chained_irqchip() several times.
> If you just mark the line with IRQF_SHARED then the handler
> will just loop over all three banks until you find a hit, provided
> you code it up properly.

Ok, I'll give it a try to use this also.

>
> There were some problems with removing an irqchip like
> that but your driver is anyway a bool so I think it might work
> just fine.
>
> Yours,
> Linus Walleij

Thanks again.

Best regards,
    Sergio Paracuellos
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sean Wang June 11, 2018, 3:29 a.m. | #5
Hi,

On Fri, 2018-06-08 at 13:59 +0200, Linus Walleij wrote:
> Hi Sergio!
> 
> Thanks for your patch!
> 
> Given that we have combined pin control and GPIO drivers for
> almost all Mediatek chips in drivers/pinctrl/mediatek/*
> I would ideally like to have some input from the Mediatek
> maintainers (especially Sean Wang) on this, especially:
> 
> - Is MT7621 a non-pincontrol GPIO controller, or can it
>   eventually use pin control as a back-end? Will a separate
>   pin control driver appear later for this SoC?
> 

MT7621 also have the circuit for pad setup tweaking Tx driving and mux
setup switching to either gpio mode or specific hardware mode, but the
circuit for all of them is being accessed in a different register range
from gpio controller being implemented here.

the part of pad or mux control for MT7621 I thought that is really
simple, so it seems worth joining pinmux and pinconf together into a
single driver to become a full function about pin setup. 

> - Would it make sense to have a combined driver just like
>   for the other Mediatek SoCs in drivers/pinctrl/mediatek?
>   If this GPIO controller does not do pin control I understand
>   why it is submitted as a GPIO driver only.
> 
> drivers/pinctrl/mediatek/pinctrl-mt7622.c is suspiciously
> similarly named. Is this a relative or just as different as
> night and day?
> 

The MT7621 is just as completely different as night and day from MT7622.

MT7622 pinctrl originate from MediaTek IPs but MT7621 pinctrl originate
from Ralink IPs and even MT7621 should be the last one machine using the
Ralink pinctrl IPs. And for these machine MT762x appearing later MT7622,
they all will be developed based on pinctrl-mt7622 architecture .

> Also you can see that this driver has a built-in GPIO driver,
> using an external interrupt.
> 
> On Sat, Jun 2, 2018 at 9:30 AM, Sergio Paracuellos

[ ... ]

> I guess you want to use
> builtin_platform_driver()?
> 
> Yours,
> Linus Walleij


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 71c0ab4..2646949 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -359,6 +359,13 @@  config GPIO_MPC8XXX
 	  Say Y here if you're going to use hardware that connects to the
 	  MPC512x/831x/834x/837x/8572/8610/QorIQ GPIOs.
 
+config GPIO_MT7621
+	bool "Mediatek GPIO Support"
+	depends on SOC_MT7620 || SOC_MT7621
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to support the Mediatek SoC GPIO device
+
 config GPIO_MVEBU
 	def_bool y
 	depends on PLAT_ORION || ARCH_MVEBU
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 1324c8f..03647ae 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -88,6 +88,7 @@  obj-$(CONFIG_GPIO_MOCKUP)      += gpio-mockup.o
 obj-$(CONFIG_GPIO_MPC5200)	+= gpio-mpc5200.o
 obj-$(CONFIG_GPIO_MPC8XXX)	+= gpio-mpc8xxx.o
 obj-$(CONFIG_GPIO_MSIC)		+= gpio-msic.o
+obj-$(CONFIG_GPIO_MT7621)	+= gpio-mt7621.o
 obj-$(CONFIG_GPIO_MVEBU)        += gpio-mvebu.o
 obj-$(CONFIG_GPIO_MXC)		+= gpio-mxc.o
 obj-$(CONFIG_GPIO_MXS)		+= gpio-mxs.o
diff --git a/drivers/gpio/gpio-mt7621.c b/drivers/gpio/gpio-mt7621.c
new file mode 100644
index 0000000..0c4fb4a
--- /dev/null
+++ b/drivers/gpio/gpio-mt7621.c
@@ -0,0 +1,370 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2009-2011 Gabor Juhos <juhosg@openwrt.org>
+ * Copyright (C) 2013 John Crispin <blogic@openwrt.org>
+ */
+
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define MTK_BANK_CNT		3
+#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,
+};
+
+struct mtk_gc {
+	struct gpio_chip chip;
+	spinlock_t lock;
+	int bank;
+	u32 rising;
+	u32 falling;
+};
+
+struct mtk_data {
+	void __iomem *gpio_membase;
+	int gpio_irq;
+	struct irq_domain *gpio_irq_domain;
+	struct mtk_gc gc_map[MTK_BANK_CNT];
+};
+
+static inline struct mtk_gc *
+to_mediatek_gpio(struct gpio_chip *chip)
+{
+	return container_of(chip, struct mtk_gc, chip);
+}
+
+static inline void
+mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
+{
+	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
+	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
+
+	iowrite32(val, gpio_data->gpio_membase + offset);
+}
+
+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)
+{
+	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);
+
+	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;
+}
+
+static int
+mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
+{
+	struct mtk_data *gpio_data = gpiochip_get_data(chip);
+	struct mtk_gc *rg = to_mediatek_gpio(chip);
+
+	return irq_create_mapping(gpio_data->gpio_irq_domain,
+				  pin + (rg->bank * MTK_BANK_WIDTH));
+}
+
+static int
+mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
+{
+	struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
+	const __be32 *id = of_get_property(bank, "reg", NULL);
+	struct mtk_gc *rg;
+	int ret;
+
+	if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
+		return -EINVAL;
+
+	rg = &gpio_data->gc_map[be32_to_cpu(*id)];
+	memset(rg, 0, sizeof(*rg));
+
+	spin_lock_init(&rg->lock);
+
+	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);
+	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) {
+		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
+			rg->chip.ngpio, ret);
+		return ret;
+	}
+
+	/* set polarity to low for all gpios */
+	mtk_gpio_w32(rg, GPIO_REG_POL, 0);
+
+	dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
+
+	return 0;
+}
+
+static void
+mediatek_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct mtk_data *gpio_data = irq_desc_get_handler_data(desc);
+	int i;
+
+	for (i = 0; i < MTK_BANK_CNT; i++) {
+		struct mtk_gc *rg = &gpio_data->gc_map[i];
+		unsigned long pending;
+		int bit;
+
+		if (!rg)
+			continue;
+
+		pending = mtk_gpio_r32(rg, GPIO_REG_STAT);
+
+		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));
+		}
+	}
+}
+
+static void
+mediatek_gpio_irq_unmask(struct irq_data *d)
+{
+	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
+	int pin = d->hwirq;
+	int bank = pin / MTK_BANK_WIDTH;
+	struct mtk_gc *rg = &gpio_data->gc_map[bank];
+	unsigned long flags;
+	u32 rise, fall;
+
+	if (!rg)
+		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));
+	spin_unlock_irqrestore(&rg->lock, flags);
+}
+
+static void
+mediatek_gpio_irq_mask(struct irq_data *d)
+{
+	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
+	int pin = d->hwirq;
+	int bank = pin / MTK_BANK_WIDTH;
+	struct mtk_gc *rg = &gpio_data->gc_map[bank];
+	unsigned long flags;
+	u32 rise, fall;
+
+	if (!rg)
+		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));
+	spin_unlock_irqrestore(&rg->lock, flags);
+}
+
+static int
+mediatek_gpio_irq_type(struct irq_data *d, unsigned int type)
+{
+	struct mtk_data *gpio_data = irq_data_get_irq_chip_data(d);
+	int pin = d->hwirq;
+	int bank = pin / MTK_BANK_WIDTH;
+	struct mtk_gc *rg = &gpio_data->gc_map[bank];
+	u32 mask = PIN_MASK(pin);
+
+	if (!rg)
+		return -1;
+
+	if (type == IRQ_TYPE_PROBE) {
+		if ((rg->rising | rg->falling) & mask)
+			return 0;
+
+		type = IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING;
+	}
+
+	if (type & IRQ_TYPE_EDGE_RISING)
+		rg->rising |= mask;
+	else
+		rg->rising &= ~mask;
+
+	if (type & IRQ_TYPE_EDGE_FALLING)
+		rg->falling |= mask;
+	else
+		rg->falling &= ~mask;
+
+	return 0;
+}
+
+static struct irq_chip mediatek_gpio_irq_chip = {
+	.name		= "GPIO",
+	.irq_unmask	= mediatek_gpio_irq_unmask,
+	.irq_mask	= mediatek_gpio_irq_mask,
+	.irq_mask_ack	= mediatek_gpio_irq_mask,
+	.irq_set_type	= mediatek_gpio_irq_type,
+};
+
+static int
+mediatek_gpio_gpio_map(struct irq_domain *d, unsigned int irq,
+		       irq_hw_number_t hw)
+{
+	int ret;
+
+	ret = irq_set_chip_data(irq, d->host_data);
+	if (ret < 0)
+		return ret;
+	irq_set_chip_and_handler(irq, &mediatek_gpio_irq_chip,
+				 handle_level_irq);
+	irq_set_handler_data(irq, d);
+
+	return 0;
+}
+
+static const struct irq_domain_ops irq_domain_ops = {
+	.xlate = irq_domain_xlate_twocell,
+	.map = mediatek_gpio_gpio_map,
+};
+
+static int
+mediatek_gpio_probe(struct platform_device *pdev)
+{
+	struct device_node *bank, *np = pdev->dev.of_node;
+	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	struct mtk_data *gpio_data;
+
+	gpio_data = devm_kzalloc(&pdev->dev, sizeof(*gpio_data), GFP_KERNEL);
+	if (!gpio_data)
+		return -ENOMEM;
+
+	gpio_data->gpio_membase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(gpio_data->gpio_membase))
+		return PTR_ERR(gpio_data->gpio_membase);
+
+	gpio_data->gpio_irq = irq_of_parse_and_map(np, 0);
+	if (gpio_data->gpio_irq) {
+		gpio_data->gpio_irq_domain = irq_domain_add_linear(np,
+			MTK_BANK_CNT * MTK_BANK_WIDTH,
+			&irq_domain_ops, gpio_data);
+		if (!gpio_data->gpio_irq_domain)
+			dev_err(&pdev->dev, "irq_domain_add_linear failed\n");
+	}
+
+	platform_set_drvdata(pdev, gpio_data);
+
+	for_each_child_of_node(np, bank)
+		if (of_device_is_compatible(bank, "mediatek,mt7621-gpio-bank"))
+			mediatek_gpio_bank_probe(pdev, bank);
+
+	if (gpio_data->gpio_irq_domain)
+		irq_set_chained_handler_and_data(gpio_data->gpio_irq,
+						 mediatek_gpio_irq_handler,
+						 gpio_data);
+
+	return 0;
+}
+
+static const struct of_device_id mediatek_gpio_match[] = {
+	{ .compatible = "mediatek,mt7621-gpio" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mediatek_gpio_match);
+
+static struct platform_driver mediatek_gpio_driver = {
+	.probe = mediatek_gpio_probe,
+	.driver = {
+		.name = "mt7621_gpio",
+		.of_match_table = mediatek_gpio_match,
+	},
+};
+
+module_platform_driver(mediatek_gpio_driver);