Re: [PATCH v4 2/4] gpio: visconti: Add Toshiba Visconti GPIO support

[Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@xxxxxxxxxxxxx>]

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