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;
Date:   Sat, 16 Mar 2019 11:39:48 +0000
From:   Marc Zyngier <marc.zyngier@arm.com>
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     Lina Iyer <ilina@codeaurora.org>, 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: <20190316113948.1d180259@why.wild-wind.fr.eu.org>
In-Reply-To: <155266731117.20095.4543997300651173812@swboyd.mtv.corp.google.com>
References: <20190313211844.29416-1-ilina@codeaurora.org>
        <20190313211844.29416-8-ilina@codeaurora.org>
        <155266731117.20095.4543997300651173812@swboyd.mtv.corp.google.com>
Organization: ARM Ltd
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 15 Mar 2019 09:28:31 -0700
Stephen Boyd <swboyd@chromium.org> wrote:

> Quoting Lina Iyer (2019-03-13 14:18:41)
> > ---
> > Changes in v4:
> >         - Remove irq_set_wake() on summary IRQ interrupt
> > Changes in v3:
> >         - Use of_irq_domain_map() and pass PDC pin to parent irqdomain
> > Changes in v2:
> >         - Call parent mask when masking GPIO interrupt
> > Changes in v1:
> >         - Fix bug when unmasking PDC interrupt  
> [...]
> > +}
> > +
> > +/*
> > + * TODO: Get rid of this and push it into gpiochip_to_irq()
> > + */  
> 
> Any chance this TODO can be resolved?
> 
> > @@ -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).

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.