Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 1FAF260746
Date:   Tue, 16 Apr 2019 15:26:44 -0600
From:   Lina Iyer <ilina@codeaurora.org>
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     Marc Zyngier <marc.zyngier@arm.com>, evgreen@chromium.org,
        linux-kernel@vger.kernel.org, rplsssn@codeaurora.org,
        linux-arm-msm@vger.kernel.org, thierry.reding@gmail.com,
        bjorn.andersson@linaro.org, dianders@chromium.org,
        linus.walleij@linaro.org
Subject: Re: [PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip
 hierarchy
Message-ID: <20190416212644.GD16124@codeaurora.org>
References: <20190313211844.29416-1-ilina@codeaurora.org>
 <20190313211844.29416-8-ilina@codeaurora.org>
 <155266731117.20095.4543997300651173812@swboyd.mtv.corp.google.com>
 <20190316113948.1d180259@why.wild-wind.fr.eu.org>
 <155320527587.20095.3351235428610314272@swboyd.mtv.corp.google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <155320527587.20095.3351235428610314272@swboyd.mtv.corp.google.com>
User-Agent: Mutt/1.11.3 (2019-02-01)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Mar 21 2019 at 15:54 -0600, Stephen Boyd wrote:
>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;
>
Linus,

Any thoughts on this?

-- Lina