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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdZ-umK4troerSA6S2rvyU5XV+KsFdxuWE5Sy0PCgdfT6A@mail.gmail.com>
Date:   Sat, 12 Dec 2020 00:20:47 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...hiba.co.jp>,
        Marc Zyngier <maz@...nel.org>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Punit Agrawal <punit1.agrawal@...hiba.co.jp>,
        yuji2.ishikawa@...hiba.co.jp,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/4] gpio: visconti: Add Toshiba Visconti GPIO support

On Fri, Dec 11, 2020 at 1:43 AM Nobuhiro Iwamatsu
<nobuhiro1.iwamatsu@...hiba.co.jp> 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.

> Add the GPIO driver for Toshiba Visconti ARM SoCs.
>
> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...hiba.co.jp>
> Reviewed-by: Punit Agrawal <punit1.agrawal@...hiba.co.jp>
(...)

> +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

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

Don't keep these irqs around.

> +       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;

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;
}

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

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

> +       /* 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.

> +       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.

The rest looks solid!

Yours,
Linus Walleij