* [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
@ 2021-04-22 15:20 Thomas Bogendoerfer
2021-04-22 15:20 ` [PATCH v3 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 " Thomas Bogendoerfer
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Thomas Bogendoerfer @ 2021-04-22 15:20 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 interrupt source.
Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
---
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 | 198 +++++++++++++++++++++++++++++++++++
3 files changed, 211 insertions(+)
create mode 100644 drivers/gpio/gpio-idt3243x.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e3607ec4c2e8..2948eb4ab8a5 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 SoC devices.
+
+ 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..1e609bc4d839
--- /dev/null
+++ b/drivers/gpio/gpio-idt3243x.c
@@ -0,0 +1,198 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for IDT/Renesas 79RC3243x Interrupt Controller.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.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;
+};
+
+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;
+ u32 ilevel;
+
+ if (sense & ~(IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
+ return -EINVAL;
+
+ 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);
+ else
+ return -EINVAL;
+
+ writel(ilevel, ctrl->gpio + IDT_GPIO_ILEVEL);
+ 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);
+
+ ctrl->mask_cache |= BIT(d->hwirq);
+ writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
+}
+
+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);
+
+ ctrl->mask_cache &= ~BIT(d->hwirq);
+ writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
+}
+
+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 = irq_of_parse_and_map(pdev->dev.of_node, 0);
+ if (!parent_irq) {
+ dev_err(&pdev->dev, "Failed to map parent IRQ!\n");
+ return -EINVAL;
+ }
+
+ /* Mask interrupts. */
+ ctrl->mask_cache = 0xffffffff;
+ writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
+
+ girq = &ctrl->gc.irq;
+ girq->chip = &idt_gpio_irqchip;
+ girq->parent_handler = idt_gpio_dispatch;
+ girq->num_parents = 1;
+ girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
+ GFP_KERNEL);
+ if (!girq->parents) {
+ ret = -ENOMEM;
+ goto out_unmap_irq;
+ }
+ girq->parents[0] = parent_irq;
+ girq->default_type = IRQ_TYPE_NONE;
+ girq->handler = handle_level_irq;
+
+ ret = devm_gpiochip_add_data(&pdev->dev, &ctrl->gc, ctrl);
+ if (ret)
+ goto out_unmap_irq;
+
+ return 0;
+
+out_unmap_irq:
+ irq_dispose_mapping(parent_irq);
+ return ret;
+}
+
+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] 8+ messages in thread
* [PATCH v3 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 GPIO controller
2021-04-22 15:20 [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller Thomas Bogendoerfer
@ 2021-04-22 15:20 ` Thomas Bogendoerfer
2021-04-23 17:53 ` Rob Herring
2021-04-23 15:37 ` [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x " Andy Shevchenko
2021-04-23 15:56 ` Andy Shevchenko
2 siblings, 1 reply; 8+ messages in thread
From: Thomas Bogendoerfer @ 2021-04-22 15:20 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 v3:
- renamed to idt,32434-gpio
- drop ngpio description
- use gpio0: gpio@50004 in example
.../devicetree/bindings/gpio/idt,32434.yaml | 71 +++++++++++++++++++
1 file changed, 71 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/idt,32434.yaml
diff --git a/Documentation/devicetree/bindings/gpio/idt,32434.yaml b/Documentation/devicetree/bindings/gpio/idt,32434.yaml
new file mode 100644
index 000000000000..bdbbe01855e0
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/idt,32434.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.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] 8+ messages in thread
* Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
2021-04-22 15:20 [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller Thomas Bogendoerfer
2021-04-22 15:20 ` [PATCH v3 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 " Thomas Bogendoerfer
@ 2021-04-23 15:37 ` Andy Shevchenko
2021-04-24 10:35 ` Thomas Bogendoerfer
2021-04-23 15:56 ` Andy Shevchenko
2 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2021-04-23 15:37 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM
On Thu, Apr 22, 2021 at 6:21 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 interrupt source.
as an interrupt
Thanks for an update, my comments below (minus that you already figured out).
...
> +config GPIO_IDT3243X
> + tristate "IDT 79RC3243X GPIO support"
> + depends on MIKROTIK_RB532 || COMPILE_TEST
Right.
But if MikroTik is dependent on this you may return default in a form of
default MIKROTIK_RB532
Up to you. (What I meant previously is the unnecessary ' y if' part).
> + select GPIO_GENERIC
> + select GPIOLIB_IRQCHIP
> + help
> + Select this option to enable GPIO driver for
> + IDT 79RC3243X SoC devices.
Seems like you may elaborate a bit more here, what kind of the
devices, list one or couple of examples, etc.
> + To compile this driver as a module, choose M here: the module will
> + be called gpio-idt3243x.
...
> +/*
> + * Driver for IDT/Renesas 79RC3243x Interrupt Controller.
> + */
One line?
...
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Why is this?
...
> +#include <linux/gpio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
Aren't those guaranteed to be included by irq.h?
> +#include <linux/irqdomain.h>
Missed mod_devicetable.h
module.h
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
Do you use them anyhow? (See below as well)
Missed types.h
...
> +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);
Is it guaranteed to be linear always?
> + if (virq)
> + generic_handle_irq(virq);
> + }
> +
> + chained_irq_exit(host_chip, desc);
> +}
...
> + if (sense & ~(IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
There is a _BOTH variant.
> + return -EINVAL;
> + 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);
> + else
> + return -EINVAL;
Is it a double check of the above?
...
> + ctrl->gc.parent = dev;
Wondering if it's already done by GPIO library.
...
> + ctrl->gc.ngpio = ngpios;
Shouldn't you do this before calling for bgpio_init()?
...
> + parent_irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
platform_get_irq() ?..
> + if (!parent_irq) {
> + dev_err(&pdev->dev, "Failed to map parent IRQ!\n");
...and drop this, since it will be taken care of.
> + return -EINVAL;
> + }
...
> + /* Mask interrupts. */
> + ctrl->mask_cache = 0xffffffff;
> + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
What about using ->init_hw() call back?
...
> + girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
1 -> girq->num_parents
> + GFP_KERNEL);
> + if (!girq->parents) {
> + ret = -ENOMEM;
> + goto out_unmap_irq;
> + }
...
> + girq->handler = handle_level_irq;
handle_bad_irq()
...
> + ret = devm_gpiochip_add_data(&pdev->dev, &ctrl->gc, ctrl);
> + if (ret)
> + goto out_unmap_irq;
> +
> + return 0;
return devm_...;
...
> +out_unmap_irq:
> + irq_dispose_mapping(parent_irq);
> + return ret;
> +}
No need.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
2021-04-22 15:20 [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller Thomas Bogendoerfer
2021-04-22 15:20 ` [PATCH v3 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 " Thomas Bogendoerfer
2021-04-23 15:37 ` [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x " Andy Shevchenko
@ 2021-04-23 15:56 ` Andy Shevchenko
2021-04-26 9:33 ` Thomas Bogendoerfer
2 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2021-04-23 15:56 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM
On Thu, Apr 22, 2021 at 6:21 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 interrupt source.
One missed question: No I/O serialization in the entire driver? Is it
taken care of by bgpio wrapper or ...?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 GPIO controller
2021-04-22 15:20 ` [PATCH v3 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 " Thomas Bogendoerfer
@ 2021-04-23 17:53 ` Rob Herring
0 siblings, 0 replies; 8+ messages in thread
From: Rob Herring @ 2021-04-23 17:53 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, devicetree, linux-kernel
On Thu, Apr 22, 2021 at 05:20:54PM +0200, Thomas Bogendoerfer wrote:
> Add YAML devicetree binding for IDT 79RC32434 GPIO controller
>
> Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> ---
> Changes in v3:
> - renamed to idt,32434-gpio
> - drop ngpio description
> - use gpio0: gpio@50004 in example
>
> .../devicetree/bindings/gpio/idt,32434.yaml | 71 +++++++++++++++++++
Not quite: idt,32434-gpio.yaml
> 1 file changed, 71 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpio/idt,32434.yaml
>
> diff --git a/Documentation/devicetree/bindings/gpio/idt,32434.yaml b/Documentation/devicetree/bindings/gpio/idt,32434.yaml
> new file mode 100644
> index 000000000000..bdbbe01855e0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/idt,32434.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.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 [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
2021-04-23 15:37 ` [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x " Andy Shevchenko
@ 2021-04-24 10:35 ` Thomas Bogendoerfer
2021-04-24 11:35 ` Andy Shevchenko
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Bogendoerfer @ 2021-04-24 10:35 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM
On Fri, Apr 23, 2021 at 06:37:41PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 22, 2021 at 6:21 PM Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> wrote:
> > +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);
>
> Is it guaranteed to be linear always?
yes
> > + if (virq)
> > + generic_handle_irq(virq);
> > + }
> > +
> > + chained_irq_exit(host_chip, desc);
> > +}
>
> ...
>
> > + if (sense & ~(IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
>
> There is a _BOTH variant.
that's IRQ_TYPE_EDGE_BOTH. LEVEL_BOTH would be an interesing concept.
> > + 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);
>
> > + else
> > + return -EINVAL;
>
> Is it a double check of the above?
no, the above test is for anything not LEVEL and this now takes care
to be at least LEVEL_LOW or LEVEL_HIGH. This doesn't check for LOW|HIGH,
which I assumed nobody tries to set...
> > + ctrl->gc.parent = dev;
>
> Wondering if it's already done by GPIO library.
no it uses it:
if (gc->parent) {
gdev->dev.parent = gc->parent;
gdev->dev.of_node = gc->parent->of_node;
}
> ...
>
> > + ctrl->gc.ngpio = ngpios;
>
> Shouldn't you do this before calling for bgpio_init()?
no, bgpio_init() initializes ngpios to size of register width, which is
32 for this hardware. And this statement restricts it to the real available
number of gpios.
> ...
>
> > + parent_irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
>
> platform_get_irq() ?..
yes, looks better :-)
> > + /* Mask interrupts. */
> > + ctrl->mask_cache = 0xffffffff;
> > + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
>
> What about using ->init_hw() call back?
sure, doesn't look like it's worth the effort, but I changed it.
> > + girq->handler = handle_level_irq;
>
> handle_bad_irq()
the hardware only supports level interrupts. That's also why there is
no handler change in idt_gpio_irq_set_type.
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] 8+ messages in thread
* Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
2021-04-24 10:35 ` Thomas Bogendoerfer
@ 2021-04-24 11:35 ` Andy Shevchenko
0 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2021-04-24 11:35 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM
On Sat, Apr 24, 2021 at 1:47 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
> On Fri, Apr 23, 2021 at 06:37:41PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 22, 2021 at 6:21 PM Thomas Bogendoerfer
> > <tsbogend@alpha.franken.de> wrote:
...
> > > + virq = irq_linear_revmap(gc->irq.domain, bit);
> >
> > Is it guaranteed to be linear always?
>
> yes
OK.
...
> > > + if (sense & ~(IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
> >
> > There is a _BOTH variant.
>
> that's IRQ_TYPE_EDGE_BOTH. LEVEL_BOTH would be an interesing concept.
Sorry, I meant _MASK in case of level. No need to open code the
existing definition.
> > > + 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);
> >
> > > + else
> > > + return -EINVAL;
> >
> > Is it a double check of the above?
>
> no, the above test is for anything not LEVEL and this now takes care
> to be at least LEVEL_LOW or LEVEL_HIGH. This doesn't check for LOW|HIGH,
> which I assumed nobody tries to set...
And? Seems you have it as a dead code.
In your case HIGH is the winner anyway.
...
> > > + /* Mask interrupts. */
> > > + ctrl->mask_cache = 0xffffffff;
> > > + writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
> >
> > What about using ->init_hw() call back?
>
> sure, doesn't look like it's worth the effort, but I changed it.
The problem here (which you may not notice from day 1) is the
ordering. We carefully put the ->init_hw() call in the proper place
and time.
...
> > > + girq->handler = handle_level_irq;
> >
> > handle_bad_irq()
>
> the hardware only supports level interrupts. That's also why there is
> no handler change in idt_gpio_irq_set_type.
After I fixed a nasty issue in the pca953x related to Intel Galileo
platform, I may tell that setting the handler here is equal to putting
a mine for the future blown. When you set it in BAD here it will
reveal issues, if any, sooner than later.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller
2021-04-23 15:56 ` Andy Shevchenko
@ 2021-04-26 9:33 ` Thomas Bogendoerfer
0 siblings, 0 replies; 8+ messages in thread
From: Thomas Bogendoerfer @ 2021-04-26 9:33 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Bartosz Golaszewski, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM
On Fri, Apr 23, 2021 at 06:56:23PM +0300, Andy Shevchenko wrote:
> On Thu, Apr 22, 2021 at 6:21 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 interrupt source.
>
> One missed question: No I/O serialization in the entire driver? Is it
> taken care of by bgpio wrapper or ...?
for gpios bggpio takes care of it, for irqs I've added them in coming v4.
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] 8+ messages in thread
end of thread, other threads:[~2021-04-26 9:57 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-22 15:20 [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller Thomas Bogendoerfer
2021-04-22 15:20 ` [PATCH v3 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 " Thomas Bogendoerfer
2021-04-23 17:53 ` Rob Herring
2021-04-23 15:37 ` [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x " Andy Shevchenko
2021-04-24 10:35 ` Thomas Bogendoerfer
2021-04-24 11:35 ` Andy Shevchenko
2021-04-23 15:56 ` Andy Shevchenko
2021-04-26 9:33 ` Thomas Bogendoerfer
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.