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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Wed, Mar 28, 2018 at 5:22 PM, Phil Edworthy
<phil.edworthy@renesas.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