lkml.org 
[lkml]   [2019]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy
    Date
    Quoting Marc Zyngier (2019-03-16 04:39:48)
    > On Fri, 15 Mar 2019 09:28:31 -0700
    > Stephen Boyd <swboyd@chromium.org> wrote:
    >
    > > Quoting Lina Iyer (2019-03-13 14:18:41)
    > > > @@ -994,6 +1092,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
    > > > pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
    > > > pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
    > > >
    > > > + chip->irq.chip = &pctrl->irq_chip;
    > > > + chip->irq.domain_ops = &msm_gpio_domain_ops;
    > > > + chip->irq.handler = handle_edge_irq;
    > > > + chip->irq.default_type = IRQ_TYPE_EDGE_RISING;
    > >
    > > This also changed from v3. It used to be IRQ_TYPE_NONE. Specifying this
    > > here seems to cause gpiolib to print a WARN.
    > >
    > >
    > > /*
    > > * Specifying a default trigger is a terrible idea if DT or ACPI is
    > > * used to configure the interrupts, as you may end up with
    > > * conflicting triggers. Tell the user, and reset to NONE.
    > > */
    > > if (WARN(np && type != IRQ_TYPE_NONE,
    > > "%s: Ignoring %u default trigger\n", np->full_name, type))
    > > type = IRQ_TYPE_NONE;
    > >
    > >
    > > So I guess this change should be dropped. Or at the least, it should be
    > > split out to it's own patch and the motivations can be discussed in the
    > > commit text.
    >
    > It is something I requested (although I expected this to be a
    > different patch, and even a clarification would have been OK).
    >
    > One way or another, the default trigger must match the flow handler. If
    > we set it up with IRQ_TYPE_NONE, what does it mean? The fact that
    > IRQ_TYPE_NONE acts as a wildcard doesn't mean the handle_edge_irq flow
    > handler is a good match for all interrupt types (it is rarely OK for
    > level interrupts).

    I think this is a question for Thierry or Linus. I'm not sure why this
    check was put in place in the code. I tried to dig into it really quick
    but I didn't find anything obvious and then I gave up.

    Maybe with hierarchical irqdomains we can drop this check? I don't think
    the gpiolib core ever uses this 'default_type' or 'handler' for anything
    once we replace the irqdomain that's used for a particular gpiochip with
    a custom irqdomain. The only user I see, gpiochip_irq_map(), won't ever
    be called so it really ends up being a thing that the driver specific
    irqdomains should check for and reject when parsing the DT and it sees
    IRQ_TYPE_NONE come out.

    ------8<-------
    diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
    index 144af0733581..fe2f7888c473 100644
    --- a/drivers/gpio/gpiolib.c
    +++ b/drivers/gpio/gpiolib.c
    @@ -1922,7 +1922,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
    * used to configure the interrupts, as you may end up with
    * conflicting triggers. Tell the user, and reset to NONE.
    */
    - if (WARN(np && type != IRQ_TYPE_NONE,
    + if (WARN(!gpiochip->irq.domain_ops && np && type != IRQ_TYPE_NONE,
    "%s: Ignoring %u default trigger\n", np->full_name, type))
    type = IRQ_TYPE_NONE;

    \
     
     \ /
      Last update: 2019-03-21 22:55    [W:2.084 / U:0.344 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site