| Message ID | 20211129153330.37719-13-nbd@nbd.name |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
Hi Felix! Thanks for your patch! On Mon, Nov 29, 2021 at 4:54 PM Felix Fietkau <nbd@nbd.name> wrote: > From: John Crispin <john@phrozen.org> > > Airoha's GPIO controller on their ARM EN7523 SoCs consists of two banks of 32 > GPIOs. Each instance in DT is for an single bank. > > Signed-off-by: John Crispin <john@phrozen.org> > Signed-off-by: Felix Fietkau <nbd@nbd.name> (...) > +config GPIO_EN7523 > + tristate "Airoha GPIO support" > + depends on ARCH_AIROHA > + default ARCH_AIROHA > + select GPIO_GENERIC Yes that looks applicable, but why isn't it used? The few 32-bit registers look like an ideal candidate for using the generic GPIO. Check similar drivers such as drivers/gpio/gpio-ftgpio010.c and how it uses bgpio_init() and the nice doc for bgpio_init() in drivers/gpio/gpio-mmio.c. If it's not working already with generic GPIO I do not think it would be far fetched to fix it. Yours, Linus Walleij
On 2021-12-02 02:47, Linus Walleij wrote: > Hi Felix! > > Thanks for your patch! > > On Mon, Nov 29, 2021 at 4:54 PM Felix Fietkau <nbd@nbd.name> wrote: > >> From: John Crispin <john@phrozen.org> >> >> Airoha's GPIO controller on their ARM EN7523 SoCs consists of two banks of 32 >> GPIOs. Each instance in DT is for an single bank. >> >> Signed-off-by: John Crispin <john@phrozen.org> >> Signed-off-by: Felix Fietkau <nbd@nbd.name> > > (...) >> +config GPIO_EN7523 >> + tristate "Airoha GPIO support" >> + depends on ARCH_AIROHA >> + default ARCH_AIROHA >> + select GPIO_GENERIC > > Yes that looks applicable, but why isn't it used? > > The few 32-bit registers look like an ideal candidate for > using the generic GPIO. Check similar drivers such as > drivers/gpio/gpio-ftgpio010.c and how it uses > bgpio_init() and the nice doc for bgpio_init() in > drivers/gpio/gpio-mmio.c. I just looked at the datasheet and the driver code again, and I think EN7523 is too strange for proper generic GPIO support. For each bank there are two control registers (not consecutive), which have 2-bit fields for every GPIO line to control direction. No idea why 2 bits per line, because only values 0 and 1 are valid, the rest are reserved. For lines configured as output, an extra output-enable bit also needs to be set in a separate register before output values can be written. The code does use bgpio to read/write values, but that's about it. I don't think it would do the generic GPIO code any good to support this weirdness. - Felix
On 02.12.21 18:59, Felix Fietkau wrote: > > On 2021-12-02 02:47, Linus Walleij wrote: >> Hi Felix! >> >> Thanks for your patch! >> >> On Mon, Nov 29, 2021 at 4:54 PM Felix Fietkau <nbd@nbd.name> wrote: >> >>> From: John Crispin <john@phrozen.org> >>> >>> Airoha's GPIO controller on their ARM EN7523 SoCs consists of two >>> banks of 32 >>> GPIOs. Each instance in DT is for an single bank. >>> >>> Signed-off-by: John Crispin <john@phrozen.org> >>> Signed-off-by: Felix Fietkau <nbd@nbd.name> >> >> (...) >>> +config GPIO_EN7523 >>> + tristate "Airoha GPIO support" >>> + depends on ARCH_AIROHA >>> + default ARCH_AIROHA >>> + select GPIO_GENERIC >> >> Yes that looks applicable, but why isn't it used? >> >> The few 32-bit registers look like an ideal candidate for >> using the generic GPIO. Check similar drivers such as >> drivers/gpio/gpio-ftgpio010.c and how it uses >> bgpio_init() and the nice doc for bgpio_init() in >> drivers/gpio/gpio-mmio.c. > I just looked at the datasheet and the driver code again, and I think > EN7523 is too strange for proper generic GPIO support. > > For each bank there are two control registers (not consecutive), which > have 2-bit fields for every GPIO line to control direction. No idea why > 2 bits per line, because only values 0 and 1 are valid, the rest are > reserved. > For lines configured as output, an extra output-enable bit also needs to > be set in a separate register before output values can be written. > > The code does use bgpio to read/write values, but that's about it. > I don't think it would do the generic GPIO code any good to support this > weirdness. > > - Felix Hi Linus, I sent an email to you 16.06.21 explaining all of this and you replied, telling me that this approach is the most reasonable one to take. John
On Thu, Dec 2, 2021 at 8:02 PM John Crispin <john@phrozen.org> wrote: > Hi Linus, > I sent an email to you 16.06.21 explaining all of this and you replied, > telling me that this approach is the most reasonable one to take. > John Sadly I don't remember that, too long ago! Anyways let's just go ahead with this. Yours, Linus Walleij
On Thu, Dec 2, 2021 at 6:59 PM Felix Fietkau <nbd@nbd.name> wrote: > I just looked at the datasheet and the driver code again, and I think > EN7523 is too strange for proper generic GPIO support. Yup I see that John explained it to me in the past, no problem. Let's do it like this then, but just drop select GPIO_GENERIC from KConfig since it's not used. With that oneliner change: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On 2021-12-05 00:57, Linus Walleij wrote: > On Thu, Dec 2, 2021 at 6:59 PM Felix Fietkau <nbd@nbd.name> wrote: > >> I just looked at the datasheet and the driver code again, and I think >> EN7523 is too strange for proper generic GPIO support. > > Yup I see that John explained it to me in the past, no problem. > Let's do it like this then, but just drop select GPIO_GENERIC > from KConfig since it's not used. > > With that oneliner change: > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Yours, > Linus Walleij The generic GPIO API is still used for the data register. The driver calls bgpio_init(), so you suggested oneliner change won't work. - Felix
On Tue, Nov 30, 2021 at 1:08 AM Felix Fietkau <nbd@nbd.name> wrote: > > From: John Crispin <john@phrozen.org> > > Airoha's GPIO controller on their ARM EN7523 SoCs consists of two banks of 32 > GPIOs. Each instance in DT is for an single bank. a single ... > +/** > + * airoha_gpio_ctrl - Airoha GPIO driver data > + * Unnecessary blank line. > + * @gc: Associated gpio_chip instance. > + * @data: The data register. > + * @dir0: The direction register for the lower 16 pins. > + * @dir1: The direction register for the higher 16 pins. > + * @output: The output enable register. > + */ ... > +static int airoha_dir_set(struct gpio_chip *gc, unsigned int gpio, > + int val, int out) > +{ > + struct airoha_gpio_ctrl *ctrl = gc_to_ctrl(gc); > + u32 dir = ioread32(ctrl->dir[gpio / 16]); > + u32 output = ioread32(ctrl->output); > + u32 mask = BIT((gpio % 16) * 2); > + > + if (out) { > + dir |= mask; > + output |= BIT(gpio); > + } else { > + dir &= ~mask; > + output &= ~BIT(gpio); > + } > + > + iowrite32(dir, ctrl->dir[gpio / 16]); > + iowrite32(output, ctrl->output); > + if (out) > + gc->set(gc, gpio, val); Needs a fix or a comment to explain why it's fine that there is a glitch possible. > + return 0; > +} ... > + err = bgpio_init(&ctrl->gc, dev, 4, ctrl->data, NULL, > + NULL, NULL, NULL, 0); > + if (err) { > + dev_err(dev, "unable to init generic GPIO"); > + return err; return dev_err_probe(...); > + }
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 072ed610f9c6..e4a34272504f 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -247,6 +247,15 @@ config GPIO_EM help Say yes here to support GPIO on Renesas Emma Mobile SoCs. +config GPIO_EN7523 + tristate "Airoha GPIO support" + depends on ARCH_AIROHA + default ARCH_AIROHA + select GPIO_GENERIC + select GPIOLIB_IRQCHIP + help + Say yes here to support the GPIO controller on Airoha EN7523. + config GPIO_EP93XX def_bool y depends on ARCH_EP93XX diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 71ee9fc2ff83..d2269ee0948e 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -56,6 +56,7 @@ obj-$(CONFIG_GPIO_DLN2) += gpio-dln2.o obj-$(CONFIG_GPIO_DWAPB) += gpio-dwapb.o obj-$(CONFIG_GPIO_EIC_SPRD) += gpio-eic-sprd.o obj-$(CONFIG_GPIO_EM) += gpio-em.o +obj-$(CONFIG_GPIO_EN7523) += gpio-en7523.o obj-$(CONFIG_GPIO_EP93XX) += gpio-ep93xx.o obj-$(CONFIG_GPIO_EXAR) += gpio-exar.o obj-$(CONFIG_GPIO_F7188X) += gpio-f7188x.o diff --git a/drivers/gpio/gpio-en7523.c b/drivers/gpio/gpio-en7523.c new file mode 100644 index 000000000000..0032f99e366d --- /dev/null +++ b/drivers/gpio/gpio-en7523.c @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: GPL-2.0-only + +#include <linux/gpio/driver.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/property.h> + +#define AIROHA_GPIO_MAX 32 + +/** + * airoha_gpio_ctrl - Airoha GPIO driver data + * + * @gc: Associated gpio_chip instance. + * @data: The data register. + * @dir0: The direction register for the lower 16 pins. + * @dir1: The direction register for the higher 16 pins. + * @output: The output enable register. + */ +struct airoha_gpio_ctrl { + struct gpio_chip gc; + void __iomem *data; + void __iomem *dir[2]; + void __iomem *output; +}; + +static struct airoha_gpio_ctrl *gc_to_ctrl(struct gpio_chip *gc) +{ + return container_of(gc, struct airoha_gpio_ctrl, gc); +} + +static int airoha_dir_set(struct gpio_chip *gc, unsigned int gpio, + int val, int out) +{ + struct airoha_gpio_ctrl *ctrl = gc_to_ctrl(gc); + u32 dir = ioread32(ctrl->dir[gpio / 16]); + u32 output = ioread32(ctrl->output); + u32 mask = BIT((gpio % 16) * 2); + + if (out) { + dir |= mask; + output |= BIT(gpio); + } else { + dir &= ~mask; + output &= ~BIT(gpio); + } + + iowrite32(dir, ctrl->dir[gpio / 16]); + iowrite32(output, ctrl->output); + + if (out) + gc->set(gc, gpio, val); + + return 0; +} + +static int airoha_dir_out(struct gpio_chip *gc, unsigned int gpio, + int val) +{ + return airoha_dir_set(gc, gpio, val, 1); +} + +static int airoha_dir_in(struct gpio_chip *gc, unsigned int gpio) +{ + return airoha_dir_set(gc, gpio, 0, 0); +} + +static int airoha_get_dir(struct gpio_chip *gc, unsigned int gpio) +{ + struct airoha_gpio_ctrl *ctrl = gc_to_ctrl(gc); + u32 dir = ioread32(ctrl->dir[gpio / 16]); + u32 mask = BIT((gpio % 16) * 2); + + return dir & mask; +} + +static const struct of_device_id airoha_gpio_of_match[] = { + { .compatible = "airoha,en7523-gpio" }, + { } +}; +MODULE_DEVICE_TABLE(of, airoha_gpio_of_match); + +static int airoha_gpio_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct airoha_gpio_ctrl *ctrl; + int err; + + ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL); + if (!ctrl) + return -ENOMEM; + + ctrl->data = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(ctrl->data)) + return PTR_ERR(ctrl->data); + + ctrl->dir[0] = devm_platform_ioremap_resource(pdev, 1); + if (IS_ERR(ctrl->dir[0])) + return PTR_ERR(ctrl->dir[0]); + + ctrl->dir[1] = devm_platform_ioremap_resource(pdev, 2); + if (IS_ERR(ctrl->dir[1])) + return PTR_ERR(ctrl->dir[1]); + + ctrl->output = devm_platform_ioremap_resource(pdev, 3); + if (IS_ERR(ctrl->output)) + return PTR_ERR(ctrl->output); + + err = bgpio_init(&ctrl->gc, dev, 4, ctrl->data, NULL, + NULL, NULL, NULL, 0); + if (err) { + dev_err(dev, "unable to init generic GPIO"); + return err; + } + + ctrl->gc.ngpio = AIROHA_GPIO_MAX; + ctrl->gc.owner = THIS_MODULE; + ctrl->gc.direction_output = airoha_dir_out; + ctrl->gc.direction_input = airoha_dir_in; + ctrl->gc.get_direction = airoha_get_dir; + + return devm_gpiochip_add_data(dev, &ctrl->gc, ctrl); +} + +static struct platform_driver airoha_gpio_driver = { + .driver = { + .name = "airoha-gpio", + .of_match_table = airoha_gpio_of_match, + }, + .probe = airoha_gpio_probe, +}; +module_platform_driver(airoha_gpio_driver); + +MODULE_DESCRIPTION("Airoha GPIO support"); +MODULE_AUTHOR("John Crispin <john@phrozen.org>"); +MODULE_LICENSE("GPL v2");