linux-kernel - Re: [PATCH] gpio: dwapb: Add support for 32 interrupts

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: <CAHp75VeOTjZ30ok1JOW0cefZfxqUM-f0-zpTnC5TonyeqQVUKg@mail.gmail.com>
Date:   Sat, 31 Mar 2018 00:26:23 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Phil Edworthy <phil.edworthy@...esas.com>
Cc:     Hoan Tran <hotran@....com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Michel Pollet <michel.pollet@...renesas.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] gpio: dwapb: Add support for 32 interrupts

On Wed, Mar 28, 2018 at 5:22 PM, Phil Edworthy
<phil.edworthy@...esas.com> wrote:
> The DesignWare GPIO IP can be configured for either 1 or 32 interrupts,

1 to 32, or just a choice between two?

> but the driver currently only supports 1 interrupt. See the DesignWare
> DW_apb_gpio Databook description of the 'GPIO_INTR_IO' parameter.

Will see after holiday and perhaps make more comments. Here is just a
brief review.

> +- interrupts : The interrupts to the parent controller raised when GPIOs
> +  generate the interrupts. If the controller provides one combined interrupt
> +  for all GPIOs, specify a single interrupt. If the controller provides one
> +  interrupt for each GPIO, provide a list of interrupts that correspond to each
> +  of the GPIO pins. When specifying multiple interrupts, if any of the GPIOs are
> +  not connected to an interrupt, use the interrupt-mask property.
> +- interrupt-mask : a 32-bit bit mask that specifies which interrupts in the list
> +  of interrupts is valid, bit is 1 for a valid irq.

So, but why one will need that in practice? GPIO driver usually
provides a pin based IRQ chip which maps each pin to the corresponding
offset inside specific IRQ domain.

> +                       struct device_node *np = to_of_node(fwnode);
> +                       u32 irq_mask = 0xFFFFFFFF;

Why? Shouldn't it be dependent to the amount of actual pins / ports?
Intel Quark has only 8 AFAIR.

> +                       int j;
> +
> +                       /* Optional irq mask */
> +                       fwnode_property_read_u32(fwnode, "interrupt-mask", &irq_mask);
> +
> +                       /*
> +                        * The IP has configuration options to allow a single
> +                        * combined interrupt or one per gpio. If one per gpio,
> +                        * some might not be used.
> +                        */

> +                       for (j = 0; j < pp->ngpio; j++) {
> +                               if (irq_mask & BIT(j)) {

for_each_set_bit() is in kernel for ages!

> +                                       pp->irq[j] = irq_of_parse_and_map(np, j);
> +                                       if (pp->irq[j])
> +                                               pp->has_irq = true;
> +                               }
> +                       }


So, on the first glance the patch looks either superfluous or taking
wrong approach. Please, elaborate more why it's done in this way and
what the case for all this in practice.

-- 
With Best Regards,
Andy Shevchenko