* [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller @ 2021-04-26 9:54 Thomas Bogendoerfer 2021-04-26 9:54 ` [PATCH v4 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 " Thomas Bogendoerfer 2021-04-26 10:29 ` [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x " Andy Shevchenko 0 siblings, 2 replies; 17+ messages in thread From: Thomas Bogendoerfer @ 2021-04-26 9:54 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, linux-kernel, linux-gpio IDT 79RC3243x SoCs integrated a gpio controller, which handles up to 32 gpios. All gpios could be used as an interrupt source. Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de> --- Changes in v4: - added spinlock to serialize access to irq registers - reworked checking of irq sense bits - start with handle_bad_irq and set handle_level_irq in idt_gpio_irq_set_type - cleaned up #includes - use platform_get_irq Changes in v3: - changed compatible string to idt,32434-gpio - registers now start with gpio direction register and leaves out alternate function register for pinmux/pinctrl driver Changes in v2: - made driver buildable as module - use for_each_set_bit() in irq dispatch handler - use gpiochip_get_data instead of own container_of helper - use module_platform_driver() instead of arch_initcall - don't default y for Mikrotik RB532 drivers/gpio/Kconfig | 12 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-idt3243x.c | 209 +++++++++++++++++++++++++++++++++++ 3 files changed, 222 insertions(+) create mode 100644 drivers/gpio/gpio-idt3243x.c diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index e3607ec4c2e8..90543a95dbb8 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -770,6 +770,18 @@ config GPIO_MSC313 Say Y here to support the main GPIO block on MStar/SigmaStar ARMv7 based SoCs. +config GPIO_IDT3243X + tristate "IDT 79RC3243X GPIO support" + depends on MIKROTIK_RB532 || COMPILE_TEST + select GPIO_GENERIC + select GPIOLIB_IRQCHIP + help + Select this option to enable GPIO driver for + IDT 79RC3243X based devices like Mikrotik RB532. + + To compile this driver as a module, choose M here: the module will + be called gpio-idt3243x. + endmenu menu "Port-mapped I/O GPIO drivers" diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index c58a90a3c3b1..75dd9c5665c5 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -67,6 +67,7 @@ obj-$(CONFIG_GPIO_HISI) += gpio-hisi.o obj-$(CONFIG_GPIO_HLWD) += gpio-hlwd.o obj-$(CONFIG_HTC_EGPIO) += gpio-htc-egpio.o obj-$(CONFIG_GPIO_ICH) += gpio-ich.o +obj-$(CONFIG_GPIO_IDT3243X) += gpio-idt3243x.o obj-$(CONFIG_GPIO_IOP) += gpio-iop.o obj-$(CONFIG_GPIO_IT87) += gpio-it87.o obj-$(CONFIG_GPIO_IXP4XX) += gpio-ixp4xx.o diff --git a/drivers/gpio/gpio-idt3243x.c b/drivers/gpio/gpio-idt3243x.c new file mode 100644 index 000000000000..62e5643a0228 --- /dev/null +++ b/drivers/gpio/gpio-idt3243x.c @@ -0,0 +1,209 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Driver for IDT/Renesas 79RC3243x Interrupt Controller */ + +#include <linux/gpio/driver.h> +#include <linux/irq.h> +#include <linux/module.h> +#include <linux/mod_devicetable.h> +#include <linux/platform_device.h> + +#define IDT_PIC_IRQ_PEND 0x00 +#define IDT_PIC_IRQ_MASK 0x08 + +#define IDT_GPIO_DIR 0x00 +#define IDT_GPIO_DATA 0x04 +#define IDT_GPIO_ILEVEL 0x08 +#define IDT_GPIO_ISTAT 0x0C + +struct idt_gpio_ctrl { + struct gpio_chip gc; + void __iomem *pic; + void __iomem *gpio; + u32 mask_cache; + spinlock_t irq_lock; /* serialize access to irq registers */ +}; + +static void idt_gpio_dispatch(struct irq_desc *desc) +{ + struct gpio_chip *gc = irq_desc_get_handler_data(desc); + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc); + struct irq_chip *host_chip = irq_desc_get_chip(desc); + unsigned int bit, virq; + unsigned long pending; + + chained_irq_enter(host_chip, desc); + + pending = readl(ctrl->pic + IDT_PIC_IRQ_PEND); + pending &= ~ctrl->mask_cache; + for_each_set_bit(bit, &pending, gc->ngpio) { + virq = irq_linear_revmap(gc->irq.domain, bit); + if (virq) + generic_handle_irq(virq); + } + + chained_irq_exit(host_chip, desc); +} + +static int idt_gpio_irq_set_type(struct irq_data *d, unsigned int flow_type) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc); + unsigned int sense = flow_type & IRQ_TYPE_SENSE_MASK; + unsigned long flags; + u32 ilevel; + + /* hardware only supports level triggered */ + if (sense & IRQ_TYPE_EDGE_BOTH) + return -EINVAL; + + if (sense == 0 || sense == IRQ_TYPE_LEVEL_MASK) + return -EINVAL; + + spin_lock_irqsave(&ctrl->irq_lock, flags); + + ilevel = readl(ctrl->gpio + IDT_GPIO_ILEVEL); + if (sense & IRQ_TYPE_LEVEL_HIGH) + ilevel |= BIT(d->hwirq); + else if (sense & IRQ_TYPE_LEVEL_LOW) + ilevel &= ~BIT(d->hwirq); + + writel(ilevel, ctrl->gpio + IDT_GPIO_ILEVEL); + irq_set_handler_locked(d, handle_level_irq); + + spin_unlock_irqrestore(&ctrl->irq_lock, flags); + return 0; +} + +static void idt_gpio_ack(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc); + + writel(~BIT(d->hwirq), ctrl->gpio + IDT_GPIO_ISTAT); +} + +static void idt_gpio_mask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc); + unsigned long flags; + + spin_lock_irqsave(&ctrl->irq_lock, flags); + + ctrl->mask_cache |= BIT(d->hwirq); + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK); + + spin_unlock_irqrestore(&ctrl->irq_lock, flags); +} + +static void idt_gpio_unmask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc); + unsigned long flags; + + spin_lock_irqsave(&ctrl->irq_lock, flags); + + ctrl->mask_cache &= ~BIT(d->hwirq); + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK); + + spin_unlock_irqrestore(&ctrl->irq_lock, flags); +} + +static int idt_gpio_irq_init_hw(struct gpio_chip *gc) +{ + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc); + + /* Mask interrupts. */ + ctrl->mask_cache = 0xffffffff; + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK); + + return 0; +} + +static struct irq_chip idt_gpio_irqchip = { + .name = "IDTGPIO", + .irq_mask = idt_gpio_mask, + .irq_ack = idt_gpio_ack, + .irq_unmask = idt_gpio_unmask, + .irq_set_type = idt_gpio_irq_set_type +}; + +static int idt_gpio_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct gpio_irq_chip *girq; + struct idt_gpio_ctrl *ctrl; + unsigned int parent_irq; + int ngpios; + int ret; + + ret = device_property_read_u32(dev, "ngpios", &ngpios); + if (ret) { + dev_err(dev, "ngpios property is not valid\n"); + return ret; + } + + ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL); + if (!ctrl) + return -ENOMEM; + + ctrl->gpio = devm_platform_ioremap_resource_byname(pdev, "gpio"); + if (!ctrl->gpio) + return -ENOMEM; + + ctrl->gc.parent = dev; + + ret = bgpio_init(&ctrl->gc, &pdev->dev, 4, ctrl->gpio + IDT_GPIO_DATA, + NULL, NULL, ctrl->gpio + IDT_GPIO_DIR, NULL, 0); + if (ret) { + dev_err(dev, "bgpio_init failed\n"); + return ret; + } + ctrl->gc.ngpio = ngpios; + + ctrl->pic = devm_platform_ioremap_resource_byname(pdev, "pic"); + if (!ctrl->pic) + return -ENOMEM; + + parent_irq = platform_get_irq(pdev, 0); + if (!parent_irq) + return -EINVAL; + + girq = &ctrl->gc.irq; + girq->chip = &idt_gpio_irqchip; + girq->init_hw = idt_gpio_irq_init_hw; + girq->parent_handler = idt_gpio_dispatch; + girq->num_parents = 1; + girq->parents = devm_kcalloc(dev, girq->num_parents, + sizeof(*girq->parents), GFP_KERNEL); + if (!girq->parents) + return -ENOMEM; + + girq->parents[0] = parent_irq; + girq->default_type = IRQ_TYPE_NONE; + girq->handler = handle_bad_irq; + + spin_lock_init(&ctrl->irq_lock); + + return devm_gpiochip_add_data(&pdev->dev, &ctrl->gc, ctrl); +} + +static const struct of_device_id idt_gpio_of_match[] = { + { .compatible = "idt,32434-gpio" }, + { } +}; +MODULE_DEVICE_TABLE(of, idt_gpio_of_match); + +static struct platform_driver idt_gpio_driver = { + .probe = idt_gpio_probe, + .driver = { + .name = "idt3243x-gpio", + .of_match_table = idt_gpio_of_match, + }, +}; +module_platform_driver(idt_gpio_driver); + +MODULE_DESCRIPTION("IDT 79RC3243x GPIO/PIC Driver"); +MODULE_AUTHOR("Thomas Bogendoerfer <tsbogend@alpha.franken.de>"); +MODULE_LICENSE("GPL"); -- 2.29.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v4 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 GPIO controller 2021-04-26 9:54 [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller Thomas Bogendoerfer @ 2021-04-26 9:54 ` Thomas Bogendoerfer 2021-04-30 20:19 ` Rob Herring 2021-05-01 12:13 ` Linus Walleij 2021-04-26 10:29 ` [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x " Andy Shevchenko 1 sibling, 2 replies; 17+ messages in thread From: Thomas Bogendoerfer @ 2021-04-26 9:54 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, Rob Herring, linux-gpio, devicetree, linux-kernel Add YAML devicetree binding for IDT 79RC32434 GPIO controller Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de> --- Changes in v4: - renamed to idt,32434-gpio this time for real Changes in v3: - renamed to idt,32434-gpio - drop ngpio description - use gpio0: gpio@50004 in example .../bindings/gpio/idt,32434-gpio.yaml | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/idt,32434-gpio.yaml diff --git a/Documentation/devicetree/bindings/gpio/idt,32434-gpio.yaml b/Documentation/devicetree/bindings/gpio/idt,32434-gpio.yaml new file mode 100644 index 000000000000..517d14b6c2e2 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/idt,32434-gpio.yaml @@ -0,0 +1,71 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/gpio/idt,32434-gpio.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: IDT 79RC32434 GPIO controller + +maintainers: + - Thomas Bogendoerfer <tsbogend@alpha.franken.de> + +properties: + compatible: + const: idt,32434-gpio + + reg: + maxItems: 2 + + reg-names: + items: + - const: gpio + - const: pic + + gpio-controller: true + + "#gpio-cells": + const: 2 + + ngpios: + minimum: 1 + maximum: 32 + + interrupt-controller: true + + "#interrupt-cells": + const: 2 + + interrupts: + maxItems: 1 + +required: + - compatible + - reg + - reg-names + - gpio-controller + - "#gpio-cells" + - ngpios + - interrupt-controller + - "#interrupt-cells" + - interrupts + +additionalProperties: false + +examples: + - | + gpio0: gpio@50004 { + compatible = "idt,32434-gpio"; + reg = <0x50004 0x10>, <0x38030 0x0c>; + reg-names = "gpio", "pic"; + + interrupt-controller; + #interrupt-cells = <2>; + + interrupt-parent = <&cpuintc>; + interrupts = <6>; + + gpio-controller; + #gpio-cells = <2>; + + ngpios = <14>; + }; -- 2.29.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 GPIO controller 2021-04-26 9:54 ` [PATCH v4 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 " Thomas Bogendoerfer @ 2021-04-30 20:19 ` Rob Herring 2021-05-01 12:13 ` Linus Walleij 1 sibling, 0 replies; 17+ messages in thread From: Rob Herring @ 2021-04-30 20:19 UTC (permalink / raw) To: Thomas Bogendoerfer Cc: Linus Walleij, linux-gpio, linux-kernel, devicetree, Bartosz Golaszewski, Rob Herring On Mon, 26 Apr 2021 11:54:26 +0200, Thomas Bogendoerfer wrote: > Add YAML devicetree binding for IDT 79RC32434 GPIO controller > > Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > --- > Changes in v4: > - renamed to idt,32434-gpio this time for real > > Changes in v3: > - renamed to idt,32434-gpio > - drop ngpio description > - use gpio0: gpio@50004 in example > > .../bindings/gpio/idt,32434-gpio.yaml | 71 +++++++++++++++++++ > 1 file changed, 71 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/idt,32434-gpio.yaml > Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 GPIO controller 2021-04-26 9:54 ` [PATCH v4 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 " Thomas Bogendoerfer 2021-04-30 20:19 ` Rob Herring @ 2021-05-01 12:13 ` Linus Walleij 2021-05-04 13:44 ` Rob Herring 2021-05-11 21:13 ` Thomas Bogendoerfer 1 sibling, 2 replies; 17+ messages in thread From: Linus Walleij @ 2021-05-01 12:13 UTC (permalink / raw) To: Thomas Bogendoerfer Cc: Bartosz Golaszewski, Rob Herring, open list:GPIO SUBSYSTEM, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel On Mon, Apr 26, 2021 at 11:54 AM Thomas Bogendoerfer <tsbogend@alpha.franken.de> wrote: > Add YAML devicetree binding for IDT 79RC32434 GPIO controller > > Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > --- > Changes in v4: > - renamed to idt,32434-gpio this time for real Overall looks good to me. > +required: (...) > + - ngpios Is there a *technical* reason why this is required? Can't the driver just default to 32 gpios when not specified? > + - interrupt-controller > + - "#interrupt-cells" > + - interrupts Why can't interrupt support be made optional? It is fine if the driver errors out if not provided, but for the bindings this feels optional. Or does the thing break unless you handle the IRQs? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 GPIO controller 2021-05-01 12:13 ` Linus Walleij @ 2021-05-04 13:44 ` Rob Herring 2021-05-06 11:11 ` Linus Walleij 2021-05-11 21:13 ` Thomas Bogendoerfer 1 sibling, 1 reply; 17+ messages in thread From: Rob Herring @ 2021-05-04 13:44 UTC (permalink / raw) To: Linus Walleij Cc: Thomas Bogendoerfer, Bartosz Golaszewski, open list:GPIO SUBSYSTEM, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel On Sat, May 1, 2021 at 7:13 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Mon, Apr 26, 2021 at 11:54 AM Thomas Bogendoerfer > <tsbogend@alpha.franken.de> wrote: > > > Add YAML devicetree binding for IDT 79RC32434 GPIO controller > > > > Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > --- > > Changes in v4: > > - renamed to idt,32434-gpio this time for real > > Overall looks good to me. > > > +required: > (...) > > + - ngpios > > Is there a *technical* reason why this is required? > > Can't the driver just default to 32 gpios when not specified? > > > + - interrupt-controller > > + - "#interrupt-cells" > > + - interrupts > > Why can't interrupt support be made optional? > > It is fine if the driver errors out if not provided, but > for the bindings this feels optional. > > Or does the thing break unless you handle the IRQs? If the hardware has interrupts, then we should describe that. It's the OS driver that may or may not support interrupts. Rob ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 GPIO controller 2021-05-04 13:44 ` Rob Herring @ 2021-05-06 11:11 ` Linus Walleij 0 siblings, 0 replies; 17+ messages in thread From: Linus Walleij @ 2021-05-06 11:11 UTC (permalink / raw) To: Rob Herring Cc: Thomas Bogendoerfer, Bartosz Golaszewski, open list:GPIO SUBSYSTEM, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel On Tue, May 4, 2021 at 3:44 PM Rob Herring <robh+dt@kernel.org> wrote: > On Sat, May 1, 2021 at 7:13 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > Why can't interrupt support be made optional? (...) > > If the hardware has interrupts, then we should describe that. It's the > OS driver that may or may not support interrupts. You're right of course. What was I thinking. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 GPIO controller 2021-05-01 12:13 ` Linus Walleij 2021-05-04 13:44 ` Rob Herring @ 2021-05-11 21:13 ` Thomas Bogendoerfer 1 sibling, 0 replies; 17+ messages in thread From: Thomas Bogendoerfer @ 2021-05-11 21:13 UTC (permalink / raw) To: Linus Walleij Cc: Bartosz Golaszewski, Rob Herring, open list:GPIO SUBSYSTEM, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel On Sat, May 01, 2021 at 02:13:35PM +0200, Linus Walleij wrote: > On Mon, Apr 26, 2021 at 11:54 AM Thomas Bogendoerfer > <tsbogend@alpha.franken.de> wrote: > > > Add YAML devicetree binding for IDT 79RC32434 GPIO controller > > > > Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > --- > > Changes in v4: > > - renamed to idt,32434-gpio this time for real > > Overall looks good to me. > > > +required: > (...) > > + - ngpios > > Is there a *technical* reason why this is required? > > Can't the driver just default to 32 gpios when not specified? sure, I make it optional. > > + - interrupt-controller > > + - "#interrupt-cells" > > + - interrupts > > Why can't interrupt support be made optional? > > It is fine if the driver errors out if not provided, but > for the bindings this feels optional. I'll make them optional. > Or does the thing break unless you handle the IRQs? no, they could be used just as GPIOs. Thomas. -- Crap can work. Given enough thrust pigs will fly, but it's not necessarily a good idea. [ RFC1925, 2.3 ] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller 2021-04-26 9:54 [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller Thomas Bogendoerfer 2021-04-26 9:54 ` [PATCH v4 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 " Thomas Bogendoerfer @ 2021-04-26 10:29 ` Andy Shevchenko 2021-04-27 22:51 ` Michael Walle 1 sibling, 1 reply; 17+ messages in thread From: Andy Shevchenko @ 2021-04-26 10:29 UTC (permalink / raw) To: Thomas Bogendoerfer Cc: Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List, open list:GPIO SUBSYSTEM On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer <tsbogend@alpha.franken.de> wrote: > > IDT 79RC3243x SoCs integrated a gpio controller, which handles up > to 32 gpios. All gpios could be used as an interrupt source. Thank you! Honestly speaking, I was about to give a tag but realized 1) we missed v5.13 anyway, and 2) there is gpio-regmap generic code, that may be worth considering. Otherwise this is in a pretty good shape. My comments below. > Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > --- > Changes in v4: > - added spinlock to serialize access to irq registers I'm not sure it's enough to have separated locks for these registers versus direction / value ones. Can't you reuse bgpio_lock? Looking into bgpio code, I think it has issues with locking in some cases (it does two or more operations each of them serialized, but not together, it means there is a window where another I/O may happen and potentially screw up the GPIO state. Dunno if gpio-regmap has this solved (I suggest to look into it as well, at least regmap API provides locking by default). > - reworked checking of irq sense bits > - start with handle_bad_irq and set handle_level_irq in idt_gpio_irq_set_type > - cleaned up #includes > - use platform_get_irq > > Changes in v3: > - changed compatible string to idt,32434-gpio > - registers now start with gpio direction register and leaves > out alternate function register for pinmux/pinctrl driver > > Changes in v2: > - made driver buildable as module > - use for_each_set_bit() in irq dispatch handler > - use gpiochip_get_data instead of own container_of helper > - use module_platform_driver() instead of arch_initcall > - don't default y for Mikrotik RB532 > > drivers/gpio/Kconfig | 12 ++ > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-idt3243x.c | 209 +++++++++++++++++++++++++++++++++++ > 3 files changed, 222 insertions(+) > create mode 100644 drivers/gpio/gpio-idt3243x.c > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index e3607ec4c2e8..90543a95dbb8 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -770,6 +770,18 @@ config GPIO_MSC313 > Say Y here to support the main GPIO block on MStar/SigmaStar > ARMv7 based SoCs. > > +config GPIO_IDT3243X > + tristate "IDT 79RC3243X GPIO support" > + depends on MIKROTIK_RB532 || COMPILE_TEST > + select GPIO_GENERIC > + select GPIOLIB_IRQCHIP > + help > + Select this option to enable GPIO driver for > + IDT 79RC3243X based devices like Mikrotik RB532. > + > + To compile this driver as a module, choose M here: the module will > + be called gpio-idt3243x. > + > endmenu > > menu "Port-mapped I/O GPIO drivers" > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > index c58a90a3c3b1..75dd9c5665c5 100644 > --- a/drivers/gpio/Makefile > +++ b/drivers/gpio/Makefile > @@ -67,6 +67,7 @@ obj-$(CONFIG_GPIO_HISI) += gpio-hisi.o > obj-$(CONFIG_GPIO_HLWD) += gpio-hlwd.o > obj-$(CONFIG_HTC_EGPIO) += gpio-htc-egpio.o > obj-$(CONFIG_GPIO_ICH) += gpio-ich.o > +obj-$(CONFIG_GPIO_IDT3243X) += gpio-idt3243x.o > obj-$(CONFIG_GPIO_IOP) += gpio-iop.o > obj-$(CONFIG_GPIO_IT87) += gpio-it87.o > obj-$(CONFIG_GPIO_IXP4XX) += gpio-ixp4xx.o > diff --git a/drivers/gpio/gpio-idt3243x.c b/drivers/gpio/gpio-idt3243x.c > new file mode 100644 > index 000000000000..62e5643a0228 > --- /dev/null > +++ b/drivers/gpio/gpio-idt3243x.c > @@ -0,0 +1,209 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Driver for IDT/Renesas 79RC3243x Interrupt Controller */ > + + bitops.h > +#include <linux/gpio/driver.h> > +#include <linux/irq.h> > +#include <linux/module.h> > +#include <linux/mod_devicetable.h> > +#include <linux/platform_device.h> + spinlock.h (but see above) > + > +#define IDT_PIC_IRQ_PEND 0x00 > +#define IDT_PIC_IRQ_MASK 0x08 > + > +#define IDT_GPIO_DIR 0x00 > +#define IDT_GPIO_DATA 0x04 > +#define IDT_GPIO_ILEVEL 0x08 > +#define IDT_GPIO_ISTAT 0x0C > + > +struct idt_gpio_ctrl { > + struct gpio_chip gc; > + void __iomem *pic; > + void __iomem *gpio; > + u32 mask_cache; > + spinlock_t irq_lock; /* serialize access to irq registers */ > +}; > + > +static void idt_gpio_dispatch(struct irq_desc *desc) > +{ > + struct gpio_chip *gc = irq_desc_get_handler_data(desc); > + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc); > + struct irq_chip *host_chip = irq_desc_get_chip(desc); > + unsigned int bit, virq; > + unsigned long pending; > + > + chained_irq_enter(host_chip, desc); > + > + pending = readl(ctrl->pic + IDT_PIC_IRQ_PEND); > + pending &= ~ctrl->mask_cache; > + for_each_set_bit(bit, &pending, gc->ngpio) { > + virq = irq_linear_revmap(gc->irq.domain, bit); > + if (virq) > + generic_handle_irq(virq); > + } > + > + chained_irq_exit(host_chip, desc); > +} > + > +static int idt_gpio_irq_set_type(struct irq_data *d, unsigned int flow_type) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc); > + unsigned int sense = flow_type & IRQ_TYPE_SENSE_MASK; > + unsigned long flags; > + u32 ilevel; > + > + /* hardware only supports level triggered */ > + if (sense & IRQ_TYPE_EDGE_BOTH) > + return -EINVAL; > + > + if (sense == 0 || sense == IRQ_TYPE_LEVEL_MASK) 0 => IRQ_TYPE_NONE Now I have got the below exit. You need to check here for EDGE So, if (sense == IRQ_TYPE_NONE || (sense & IRQ_TYPE_EDGE_BOTH) And setting LEVEL_HIGH + LEVEL_LOW shouldn't stop you here. It's fine, just declared HIGH as a winner. > + return -EINVAL; > + > + spin_lock_irqsave(&ctrl->irq_lock, flags); > + > + ilevel = readl(ctrl->gpio + IDT_GPIO_ILEVEL); > + if (sense & IRQ_TYPE_LEVEL_HIGH) > + ilevel |= BIT(d->hwirq); > + else if (sense & IRQ_TYPE_LEVEL_LOW) > + ilevel &= ~BIT(d->hwirq); > + > + writel(ilevel, ctrl->gpio + IDT_GPIO_ILEVEL); > + irq_set_handler_locked(d, handle_level_irq); > + > + spin_unlock_irqrestore(&ctrl->irq_lock, flags); > + return 0; > +} > + > +static void idt_gpio_ack(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc); > + > + writel(~BIT(d->hwirq), ctrl->gpio + IDT_GPIO_ISTAT); > +} > + > +static void idt_gpio_mask(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc); > + unsigned long flags; > + > + spin_lock_irqsave(&ctrl->irq_lock, flags); > + > + ctrl->mask_cache |= BIT(d->hwirq); > + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK); > + > + spin_unlock_irqrestore(&ctrl->irq_lock, flags); > +} > + > +static void idt_gpio_unmask(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc); > + unsigned long flags; > + > + spin_lock_irqsave(&ctrl->irq_lock, flags); > + > + ctrl->mask_cache &= ~BIT(d->hwirq); > + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK); > + > + spin_unlock_irqrestore(&ctrl->irq_lock, flags); > +} > + > +static int idt_gpio_irq_init_hw(struct gpio_chip *gc) > +{ > + struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc); > + > + /* Mask interrupts. */ > + ctrl->mask_cache = 0xffffffff; > + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK); > + > + return 0; > +} > + > +static struct irq_chip idt_gpio_irqchip = { > + .name = "IDTGPIO", > + .irq_mask = idt_gpio_mask, > + .irq_ack = idt_gpio_ack, > + .irq_unmask = idt_gpio_unmask, > + .irq_set_type = idt_gpio_irq_set_type > +}; > + > +static int idt_gpio_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct gpio_irq_chip *girq; > + struct idt_gpio_ctrl *ctrl; > + unsigned int parent_irq; > + int ngpios; > + int ret; > + > + ret = device_property_read_u32(dev, "ngpios", &ngpios); > + if (ret) { > + dev_err(dev, "ngpios property is not valid\n"); > + return ret; > + } > + > + ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL); > + if (!ctrl) > + return -ENOMEM; > + > + ctrl->gpio = devm_platform_ioremap_resource_byname(pdev, "gpio"); > + if (!ctrl->gpio) > + return -ENOMEM; > + > + ctrl->gc.parent = dev; > + > + ret = bgpio_init(&ctrl->gc, &pdev->dev, 4, ctrl->gpio + IDT_GPIO_DATA, > + NULL, NULL, ctrl->gpio + IDT_GPIO_DIR, NULL, 0); > + if (ret) { > + dev_err(dev, "bgpio_init failed\n"); > + return ret; > + } > + ctrl->gc.ngpio = ngpios; > + > + ctrl->pic = devm_platform_ioremap_resource_byname(pdev, "pic"); > + if (!ctrl->pic) > + return -ENOMEM; > + > + parent_irq = platform_get_irq(pdev, 0); > + if (!parent_irq) > + return -EINVAL; > + > + girq = &ctrl->gc.irq; > + girq->chip = &idt_gpio_irqchip; > + girq->init_hw = idt_gpio_irq_init_hw; > + girq->parent_handler = idt_gpio_dispatch; > + girq->num_parents = 1; > + girq->parents = devm_kcalloc(dev, girq->num_parents, > + sizeof(*girq->parents), GFP_KERNEL); > + if (!girq->parents) > + return -ENOMEM; > + > + girq->parents[0] = parent_irq; > + girq->default_type = IRQ_TYPE_NONE; > + girq->handler = handle_bad_irq; > + > + spin_lock_init(&ctrl->irq_lock); > + > + return devm_gpiochip_add_data(&pdev->dev, &ctrl->gc, ctrl); > +} > + > +static const struct of_device_id idt_gpio_of_match[] = { > + { .compatible = "idt,32434-gpio" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, idt_gpio_of_match); > + > +static struct platform_driver idt_gpio_driver = { > + .probe = idt_gpio_probe, > + .driver = { > + .name = "idt3243x-gpio", > + .of_match_table = idt_gpio_of_match, > + }, > +}; > +module_platform_driver(idt_gpio_driver); > + > +MODULE_DESCRIPTION("IDT 79RC3243x GPIO/PIC Driver"); > +MODULE_AUTHOR("Thomas Bogendoerfer <tsbogend@alpha.franken.de>"); > +MODULE_LICENSE("GPL"); > -- > 2.29.2 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller 2021-04-26 10:29 ` [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x " Andy Shevchenko @ 2021-04-27 22:51 ` Michael Walle 2021-04-28 11:07 ` Andy Shevchenko 0 siblings, 1 reply; 17+ messages in thread From: Michael Walle @ 2021-04-27 22:51 UTC (permalink / raw) To: Andy Shevchenko Cc: Thomas Bogendoerfer, Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List, open list:GPIO SUBSYSTEM Hi, Am 2021-04-26 12:29, schrieb Andy Shevchenko: > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer > <tsbogend@alpha.franken.de> wrote: > > 2) there is gpio-regmap generic code, that may be worth > considering. This driver uses memory mapped registers. While that is also possible with gpio-regmap, there is one drawback: it assumes gpiochip->can_sleep = true for now, see [1]. Unfortunately, there is no easy way to ask the regmap if its mmio/fastio. -michael [1] https://elixir.bootlin.com/linux/v5.12/source/drivers/gpio/gpio-regmap.c#L257 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller 2021-04-27 22:51 ` Michael Walle @ 2021-04-28 11:07 ` Andy Shevchenko 2021-04-28 11:57 ` Michael Walle 0 siblings, 1 reply; 17+ messages in thread From: Andy Shevchenko @ 2021-04-28 11:07 UTC (permalink / raw) To: Michael Walle Cc: Thomas Bogendoerfer, Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List, open list:GPIO SUBSYSTEM On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <michael@walle.cc> wrote: > Am 2021-04-26 12:29, schrieb Andy Shevchenko: > > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer > > <tsbogend@alpha.franken.de> wrote: > > > > 2) there is gpio-regmap generic code, that may be worth > > considering. > > This driver uses memory mapped registers. While that is > also possible with gpio-regmap, there is one drawback: > it assumes gpiochip->can_sleep = true for now, see [1]. > Unfortunately, there is no easy way to ask the regmap > if its mmio/fastio. I don't see how it is an impediment. Prerequisite patch? > [1] > https://elixir.bootlin.com/linux/v5.12/source/drivers/gpio/gpio-regmap.c#L257 -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller 2021-04-28 11:07 ` Andy Shevchenko @ 2021-04-28 11:57 ` Michael Walle 2021-04-28 13:44 ` Andy Shevchenko 0 siblings, 1 reply; 17+ messages in thread From: Michael Walle @ 2021-04-28 11:57 UTC (permalink / raw) To: Andy Shevchenko Cc: Thomas Bogendoerfer, Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List, open list:GPIO SUBSYSTEM Am 2021-04-28 13:07, schrieb Andy Shevchenko: > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <michael@walle.cc> wrote: >> Am 2021-04-26 12:29, schrieb Andy Shevchenko: >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer >> > <tsbogend@alpha.franken.de> wrote: >> > >> > 2) there is gpio-regmap generic code, that may be worth >> > considering. >> >> This driver uses memory mapped registers. While that is >> also possible with gpio-regmap, there is one drawback: >> it assumes gpiochip->can_sleep = true for now, see [1]. >> Unfortunately, there is no easy way to ask the regmap >> if its mmio/fastio. > > I don't see how it is an impediment. You'd have to use the *_cansleep() variants with the gpios, which cannot be used everywhere, no? -michael ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller 2021-04-28 11:57 ` Michael Walle @ 2021-04-28 13:44 ` Andy Shevchenko 2021-04-28 14:04 ` Michael Walle 0 siblings, 1 reply; 17+ messages in thread From: Andy Shevchenko @ 2021-04-28 13:44 UTC (permalink / raw) To: Michael Walle Cc: Thomas Bogendoerfer, Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List, open list:GPIO SUBSYSTEM On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <michael@walle.cc> wrote: > > Am 2021-04-28 13:07, schrieb Andy Shevchenko: > > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <michael@walle.cc> wrote: > >> Am 2021-04-26 12:29, schrieb Andy Shevchenko: > >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer > >> > <tsbogend@alpha.franken.de> wrote: > >> > > >> > 2) there is gpio-regmap generic code, that may be worth > >> > considering. > >> > >> This driver uses memory mapped registers. While that is > >> also possible with gpio-regmap, there is one drawback: > >> it assumes gpiochip->can_sleep = true for now, see [1]. > >> Unfortunately, there is no easy way to ask the regmap > >> if its mmio/fastio. > > > > I don't see how it is an impediment. > > You'd have to use the *_cansleep() variants with the gpios, > which cannot be used everywhere, no? *can* sleep means that it requires a sleeping context to run, if your controller is fine with that, there are no worries. OTOH if you want to run this in an atomic context, then consumers can't do with that kind of controller. What I meant above (and you stripped it here) is to add a patch that will fix that and set it based on gpio_regmap_config. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller 2021-04-28 13:44 ` Andy Shevchenko @ 2021-04-28 14:04 ` Michael Walle 2021-04-28 14:32 ` Andy Shevchenko 0 siblings, 1 reply; 17+ messages in thread From: Michael Walle @ 2021-04-28 14:04 UTC (permalink / raw) To: Andy Shevchenko Cc: Thomas Bogendoerfer, Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List, open list:GPIO SUBSYSTEM Am 2021-04-28 15:44, schrieb Andy Shevchenko: > On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <michael@walle.cc> wrote: >> >> Am 2021-04-28 13:07, schrieb Andy Shevchenko: >> > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <michael@walle.cc> wrote: >> >> Am 2021-04-26 12:29, schrieb Andy Shevchenko: >> >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer >> >> > <tsbogend@alpha.franken.de> wrote: >> >> > >> >> > 2) there is gpio-regmap generic code, that may be worth >> >> > considering. >> >> >> >> This driver uses memory mapped registers. While that is >> >> also possible with gpio-regmap, there is one drawback: >> >> it assumes gpiochip->can_sleep = true for now, see [1]. >> >> Unfortunately, there is no easy way to ask the regmap >> >> if its mmio/fastio. >> > >> > I don't see how it is an impediment. >> >> You'd have to use the *_cansleep() variants with the gpios, >> which cannot be used everywhere, no? > > *can* sleep means that it requires a sleeping context to run, if your > controller is fine with that, there are no worries. OTOH if you want > to run this in an atomic context, then consumers can't do with that > kind of controller. Ok, then we are on the same track. > What I meant above (and you stripped it here) is > to add a patch that will fix that and set it based on > gpio_regmap_config. Yes, but ideally, it would ask the regmap. Otherwise that information is redundant and might mismatch, i.e. gpio_regmap_config tell can_sleep=false but the regmap is an I2C type for example. Also if a driver wants to support both regmap types, we are no step further. -michael ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller 2021-04-28 14:04 ` Michael Walle @ 2021-04-28 14:32 ` Andy Shevchenko 2021-04-28 14:48 ` Michael Walle 0 siblings, 1 reply; 17+ messages in thread From: Andy Shevchenko @ 2021-04-28 14:32 UTC (permalink / raw) To: Michael Walle Cc: Thomas Bogendoerfer, Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List, open list:GPIO SUBSYSTEM On Wed, Apr 28, 2021 at 5:04 PM Michael Walle <michael@walle.cc> wrote: > Am 2021-04-28 15:44, schrieb Andy Shevchenko: > > On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <michael@walle.cc> wrote: > >> > >> Am 2021-04-28 13:07, schrieb Andy Shevchenko: > >> > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <michael@walle.cc> wrote: > >> >> Am 2021-04-26 12:29, schrieb Andy Shevchenko: > >> >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer > >> >> > <tsbogend@alpha.franken.de> wrote: > >> >> > > >> >> > 2) there is gpio-regmap generic code, that may be worth > >> >> > considering. > >> >> > >> >> This driver uses memory mapped registers. While that is > >> >> also possible with gpio-regmap, there is one drawback: > >> >> it assumes gpiochip->can_sleep = true for now, see [1]. > >> >> Unfortunately, there is no easy way to ask the regmap > >> >> if its mmio/fastio. > >> > > >> > I don't see how it is an impediment. > >> > >> You'd have to use the *_cansleep() variants with the gpios, > >> which cannot be used everywhere, no? > > > > *can* sleep means that it requires a sleeping context to run, if your > > controller is fine with that, there are no worries. OTOH if you want > > to run this in an atomic context, then consumers can't do with that > > kind of controller. > > Ok, then we are on the same track. > > > What I meant above (and you stripped it here) is > > to add a patch that will fix that and set it based on > > gpio_regmap_config. > > Yes, but ideally, it would ask the regmap. Otherwise that > information is redundant and might mismatch, i.e. gpio_regmap_config > tell can_sleep=false but the regmap is an I2C type for example. Also > if a driver wants to support both regmap types, we are no step > further. Yeah, I agree that is a band aid, but you are free to fix it actually on regmap level. I don't think it will require an enormous amount of work there. We have time :-) -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller 2021-04-28 14:32 ` Andy Shevchenko @ 2021-04-28 14:48 ` Michael Walle 2021-04-28 15:02 ` Andy Shevchenko 0 siblings, 1 reply; 17+ messages in thread From: Michael Walle @ 2021-04-28 14:48 UTC (permalink / raw) To: Andy Shevchenko Cc: Thomas Bogendoerfer, Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, Mark Brown [Adding Mark here, too] Am 2021-04-28 16:32, schrieb Andy Shevchenko: > On Wed, Apr 28, 2021 at 5:04 PM Michael Walle <michael@walle.cc> wrote: >> Am 2021-04-28 15:44, schrieb Andy Shevchenko: >> > On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <michael@walle.cc> wrote: >> >> >> >> Am 2021-04-28 13:07, schrieb Andy Shevchenko: >> >> > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <michael@walle.cc> wrote: >> >> >> Am 2021-04-26 12:29, schrieb Andy Shevchenko: >> >> >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer >> >> >> > <tsbogend@alpha.franken.de> wrote: >> >> >> > >> >> >> > 2) there is gpio-regmap generic code, that may be worth >> >> >> > considering. >> >> >> >> >> >> This driver uses memory mapped registers. While that is >> >> >> also possible with gpio-regmap, there is one drawback: >> >> >> it assumes gpiochip->can_sleep = true for now, see [1]. >> >> >> Unfortunately, there is no easy way to ask the regmap >> >> >> if its mmio/fastio. >> >> > >> >> > I don't see how it is an impediment. >> >> >> >> You'd have to use the *_cansleep() variants with the gpios, >> >> which cannot be used everywhere, no? >> > >> > *can* sleep means that it requires a sleeping context to run, if your >> > controller is fine with that, there are no worries. OTOH if you want >> > to run this in an atomic context, then consumers can't do with that >> > kind of controller. >> >> Ok, then we are on the same track. >> >> > What I meant above (and you stripped it here) is >> > to add a patch that will fix that and set it based on >> > gpio_regmap_config. >> >> Yes, but ideally, it would ask the regmap. Otherwise that >> information is redundant and might mismatch, i.e. gpio_regmap_config >> tell can_sleep=false but the regmap is an I2C type for example. Also >> if a driver wants to support both regmap types, we are no step >> further. > > Yeah, I agree that is a band aid, but you are free to fix it actually > on regmap level. > I don't think it will require an enormous amount of work there. I'd love to fix that, but Mark was against exposing that property outside of regmap. So it it what it is for now ;) Maybe he'll change his mind or someone has another idea. -michael ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller 2021-04-28 14:48 ` Michael Walle @ 2021-04-28 15:02 ` Andy Shevchenko 2021-04-28 15:07 ` Michael Walle 0 siblings, 1 reply; 17+ messages in thread From: Andy Shevchenko @ 2021-04-28 15:02 UTC (permalink / raw) To: Michael Walle Cc: Thomas Bogendoerfer, Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, Mark Brown On Wed, Apr 28, 2021 at 5:48 PM Michael Walle <michael@walle.cc> wrote: > > [Adding Mark here, too] > > Am 2021-04-28 16:32, schrieb Andy Shevchenko: > > On Wed, Apr 28, 2021 at 5:04 PM Michael Walle <michael@walle.cc> wrote: > >> Am 2021-04-28 15:44, schrieb Andy Shevchenko: > >> > On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <michael@walle.cc> wrote: > >> >> > >> >> Am 2021-04-28 13:07, schrieb Andy Shevchenko: > >> >> > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <michael@walle.cc> wrote: > >> >> >> Am 2021-04-26 12:29, schrieb Andy Shevchenko: > >> >> >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer > >> >> >> > <tsbogend@alpha.franken.de> wrote: > >> >> >> > > >> >> >> > 2) there is gpio-regmap generic code, that may be worth > >> >> >> > considering. > >> >> >> > >> >> >> This driver uses memory mapped registers. While that is > >> >> >> also possible with gpio-regmap, there is one drawback: > >> >> >> it assumes gpiochip->can_sleep = true for now, see [1]. > >> >> >> Unfortunately, there is no easy way to ask the regmap > >> >> >> if its mmio/fastio. > >> >> > > >> >> > I don't see how it is an impediment. > >> >> > >> >> You'd have to use the *_cansleep() variants with the gpios, > >> >> which cannot be used everywhere, no? > >> > > >> > *can* sleep means that it requires a sleeping context to run, if your > >> > controller is fine with that, there are no worries. OTOH if you want > >> > to run this in an atomic context, then consumers can't do with that > >> > kind of controller. > >> > >> Ok, then we are on the same track. > >> > >> > What I meant above (and you stripped it here) is > >> > to add a patch that will fix that and set it based on > >> > gpio_regmap_config. > >> > >> Yes, but ideally, it would ask the regmap. Otherwise that > >> information is redundant and might mismatch, i.e. gpio_regmap_config > >> tell can_sleep=false but the regmap is an I2C type for example. Also > >> if a driver wants to support both regmap types, we are no step > >> further. > > > > Yeah, I agree that is a band aid, but you are free to fix it actually > > on regmap level. > > I don't think it will require an enormous amount of work there. > > I'd love to fix that, but Mark was against exposing that property > outside of regmap. So it it what it is for now ;) Maybe he'll change > his mind or someone has another idea. Then let's go to ugly variant with duplicating it in gpio-regmap config. with a FIXME note or so. I don't think we should allow new drivers be based on bgpio_init(). -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller 2021-04-28 15:02 ` Andy Shevchenko @ 2021-04-28 15:07 ` Michael Walle 0 siblings, 0 replies; 17+ messages in thread From: Michael Walle @ 2021-04-28 15:07 UTC (permalink / raw) To: Andy Shevchenko Cc: Thomas Bogendoerfer, Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, Mark Brown Am 2021-04-28 17:02, schrieb Andy Shevchenko: > On Wed, Apr 28, 2021 at 5:48 PM Michael Walle <michael@walle.cc> wrote: >> >> [Adding Mark here, too] >> >> Am 2021-04-28 16:32, schrieb Andy Shevchenko: >> > On Wed, Apr 28, 2021 at 5:04 PM Michael Walle <michael@walle.cc> wrote: >> >> Am 2021-04-28 15:44, schrieb Andy Shevchenko: >> >> > On Wed, Apr 28, 2021 at 2:57 PM Michael Walle <michael@walle.cc> wrote: >> >> >> >> >> >> Am 2021-04-28 13:07, schrieb Andy Shevchenko: >> >> >> > On Wed, Apr 28, 2021 at 1:51 AM Michael Walle <michael@walle.cc> wrote: >> >> >> >> Am 2021-04-26 12:29, schrieb Andy Shevchenko: >> >> >> >> > On Mon, Apr 26, 2021 at 12:55 PM Thomas Bogendoerfer >> >> >> >> > <tsbogend@alpha.franken.de> wrote: >> >> >> >> > >> >> >> >> > 2) there is gpio-regmap generic code, that may be worth >> >> >> >> > considering. >> >> >> >> >> >> >> >> This driver uses memory mapped registers. While that is >> >> >> >> also possible with gpio-regmap, there is one drawback: >> >> >> >> it assumes gpiochip->can_sleep = true for now, see [1]. >> >> >> >> Unfortunately, there is no easy way to ask the regmap >> >> >> >> if its mmio/fastio. >> >> >> > >> >> >> > I don't see how it is an impediment. >> >> >> >> >> >> You'd have to use the *_cansleep() variants with the gpios, >> >> >> which cannot be used everywhere, no? >> >> > >> >> > *can* sleep means that it requires a sleeping context to run, if your >> >> > controller is fine with that, there are no worries. OTOH if you want >> >> > to run this in an atomic context, then consumers can't do with that >> >> > kind of controller. >> >> >> >> Ok, then we are on the same track. >> >> >> >> > What I meant above (and you stripped it here) is >> >> > to add a patch that will fix that and set it based on >> >> > gpio_regmap_config. >> >> >> >> Yes, but ideally, it would ask the regmap. Otherwise that >> >> information is redundant and might mismatch, i.e. gpio_regmap_config >> >> tell can_sleep=false but the regmap is an I2C type for example. Also >> >> if a driver wants to support both regmap types, we are no step >> >> further. >> > >> > Yeah, I agree that is a band aid, but you are free to fix it actually >> > on regmap level. >> > I don't think it will require an enormous amount of work there. >> >> I'd love to fix that, but Mark was against exposing that property >> outside of regmap. So it it what it is for now ;) Maybe he'll change >> his mind or someone has another idea. > > Then let's go to ugly variant with duplicating it in gpio-regmap > config. with a FIXME note or so. I don't think we should allow new > drivers be based on bgpio_init(). Agreed, given that a possible fix should be easy enough later. -michael ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-05-11 21:31 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-26 9:54 [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x GPIO controller Thomas Bogendoerfer 2021-04-26 9:54 ` [PATCH v4 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 " Thomas Bogendoerfer 2021-04-30 20:19 ` Rob Herring 2021-05-01 12:13 ` Linus Walleij 2021-05-04 13:44 ` Rob Herring 2021-05-06 11:11 ` Linus Walleij 2021-05-11 21:13 ` Thomas Bogendoerfer 2021-04-26 10:29 ` [PATCH v4 1/2] gpio: Add support for IDT 79RC3243x " Andy Shevchenko 2021-04-27 22:51 ` Michael Walle 2021-04-28 11:07 ` Andy Shevchenko 2021-04-28 11:57 ` Michael Walle 2021-04-28 13:44 ` Andy Shevchenko 2021-04-28 14:04 ` Michael Walle 2021-04-28 14:32 ` Andy Shevchenko 2021-04-28 14:48 ` Michael Walle 2021-04-28 15:02 ` Andy Shevchenko 2021-04-28 15:07 ` Michael Walle
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.