Hi,

Thanks for your review.

On Sat, Dec 12, 2020 at 12:20:47AM +0100, Linus Walleij wrote:

On Fri, Dec 11, 2020 at 1:43 AM Nobuhiro Iwamatsu
[off-list ref] wrote:

This iteration is looking really good, but we are not quite there yet,
because now that the driver looks so much better I can see that it
is a hierarchical interrupt controller.

As you pointed out, this GPIO interrupt is directly linked to the GIC
interrupt.
As a function of the GPIO driver, there is a function (IRQ_DOMAIN_HIERARCHY)
that can handle these as one-to-one, so it is pointed out that it is better
to use this. Is this correct?

quoted

Add the GPIO driver for Toshiba Visconti ARM SoCs.

Signed-off-by: Nobuhiro Iwamatsu <redacted>
Reviewed-by: Punit Agrawal <redacted>

(...)

quoted

+config GPIO_VISCONTI
+       tristate "Toshiba Visconti GPIO support"
+       depends on ARCH_VISCONTI || COMPILE_TEST
+       depends on OF_GPIO
+       select GPIOLIB_IRQCHIP
+       select GPIO_GENERIC
+       help
+         Say yes here to support GPIO on Tohisba Visconti.

Add
select IRQ_DOMAIN_HIERARCHY

OK, I will add this.

quoted

+struct visconti_gpio {
+       void __iomem *base;
+       int *irq;

Don't keep these irqs around.

OK, I will remove this
.

quoted

+       ret = platform_irq_count(pdev);
+       if (ret < 0)
+               return ret;
+
+       num_irq = ret;
+
+       priv->irq = devm_kcalloc(dev, num_irq, sizeof(priv->irq), GFP_KERNEL);
+       if (!priv->irq)
+               return -ENOMEM;
+
+       for (i = 0; i < num_irq; i++) {
+               priv->irq[i] = platform_get_irq(pdev, i);
+               if (priv->irq[i] < 0) {
+                       dev_err(dev, "invalid IRQ[%d]\n", i);
+                       return priv->irq[i];
+               }
+       }

Instead of doing this, look in for example
drivers/gpio/gpio-ixp4xx.c

You need:

quoted

+       girq = &priv->gpio_chip.irq;
+       girq->chip = irq_chip;

girq->fwnode = fwnode;
girq->parent_domain = parent;
girq->child_to_parent_hwirq = visconti_gpio_child_to_parent_hwirq;

I understood that the irq_domain specified by girq->parent_domain will be the
GIC. Is this correct?

The mapping function will be something like this:

static int visconti_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
                                             unsigned int child,
                                             unsigned int child_type,
                                             unsigned int *parent,
                                             unsigned int *parent_type)
{
        /* Interrupts 0..15 mapped to interrupts 24..39 on the GIC */
        if (child < 16) {
            /* All these interrupts are level high in the CPU */
            *parent_type = IRQ_TYPE_LEVEL_HIGH;
            *parent = child + 24;
            return 0;
        }
        return -EINVAL;
}

I see, I will add this function.

quoted

+       priv->gpio_chip.irq.init_valid_mask = visconti_init_irq_valid_mask;

This will be set up by gpiolib when using hierarchical irqs.

OK.

quoted

+       /* This will let us handle the parent IRQ in the driver */
+       girq->parent_handler = NULL;
+       girq->num_parents = 0;
+       girq->parents = NULL;

You don't need this.

quoted

+       girq->default_type = IRQ_TYPE_NONE;
+       girq->handler = handle_level_irq;

But this stays.

OK, I will update to these.

quoted

+       for (i = 0; i < num_irq; i++) {
+               desc = irq_to_desc(priv->irq[i]);
+               desc->status_use_accessors |= IRQ_NOAUTOEN;
+               if (devm_request_irq(dev, priv->irq[i],
+                                    visconti_gpio_irq_handler, 0, name, priv)) {
+                       dev_err(dev, "failed to request IRQ[%d]\n", i);
+                       return -ENOENT;
+               }
+       }

This should not be needed either when using hiearchical IRQs,
also the irqchip maintainers will beat us up for poking around in the
descs like this.

I understand that the processing equivalent to request_irq() is processed
by the irqchip frame work (or GIC driver). Is this correct?

The rest looks solid!

Thank you.
I will apply your point into the driver.

Best regards,
  Nobuhiro

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel