[PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller

[PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller

[Thomas Bogendoerfer]

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

[PATCH v3 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 GPIO controller

[Thomas Bogendoerfer]

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

Re: [PATCH v3 2/2] dt-bindings: gpio: Add devicetree binding for IDT 79RC32434 GPIO controller

[Rob Herring]

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
> 

Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller

[Andy Shevchenko]

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

Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller

[Thomas Bogendoerfer]

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 ]

Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller

[Andy Shevchenko]

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

Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller

[Andy Shevchenko]

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

Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller

[Thomas Bogendoerfer]

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 ]