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
> <nobuhiro1.iwamatsu@xxxxxxxxxxxxx> 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?
>
> > Add the GPIO driver for Toshiba Visconti ARM SoCs.
> >
> > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>
> > Reviewed-by: Punit Agrawal <punit1.agrawal@xxxxxxxxxxxxx>
> (...)
>
> > +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.
>
> > +struct visconti_gpio {
> > + void __iomem *base;
> > + int *irq;
>
> Don't keep these irqs around.
>
OK, I will remove this
.
> > + 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:
>
> > + 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.
> > + priv->gpio_chip.irq.init_valid_mask = visconti_init_irq_valid_mask;
>
> This will be set up by gpiolib when using hierarchical irqs.
>
OK.
> > + /* 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.
>
> > + girq->default_type = IRQ_TYPE_NONE;
> > + girq->handler = handle_level_irq;
>
> But this stays.
>
OK, I will update to these.
> > + 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
