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