lkml.org 
[lkml]   [2018]   [Mar]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH] gpio: dwapb: Add support for 32 interrupts
    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

    \
     
     \ /
      Last update: 2018-03-30 23:27    [W:4.949 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site