Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 1/4] gpio: gpiolib: Allow GPIO IRQs to lazy disable
To:     Linus Walleij <linus.walleij@linaro.org>,
        Maulik Shah <mkshah@codeaurora.org>
Cc:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        Marc Zyngier <maz@kernel.org>,
        Stephen Boyd <swboyd@chromium.org>,
        Evan Green <evgreen@chromium.org>,
        Matthias Kaehlcke <mka@chromium.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        MSM <linux-arm-msm@vger.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Andy Gross <agross@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Jason Cooper <jason@lakedaemon.net>,
        Doug Anderson <dianders@chromium.org>,
        Rajendra Nayak <rnayak@codeaurora.org>,
        Lina Iyer <ilina@codeaurora.org>, lsrao@codeaurora.org
References: <1590253873-11556-1-git-send-email-mkshah@codeaurora.org>
 <1590253873-11556-2-git-send-email-mkshah@codeaurora.org>
 <CACRpkdba9j4EdCkD5OeL=3A4Zeb57vO78FAXA9fo0SOgBE57ag@mail.gmail.com>
From:   Hans Verkuil <hverkuil@xs4all.nl>
Message-ID: <3efa1f69-1e1d-f919-d47e-b4c5c73532b7@xs4all.nl>
Date:   Wed, 27 May 2020 12:38:04 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <CACRpkdba9j4EdCkD5OeL=3A4Zeb57vO78FAXA9fo0SOgBE57ag@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 25/05/2020 13:55, Linus Walleij wrote:
> On Sat, May 23, 2020 at 7:11 PM Maulik Shah <mkshah@codeaurora.org> wrote:
> 
>> With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib
>> overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable
>> callback is implemented then genirq takes unlazy path to disable irq.
>>
>> Underlying irqchip may not want to implement irq_disable callback to lazy
>> disable irq when client drivers invokes disable_irq(). By overriding
>> irq_disable callback, gpiolib ends up always unlazy disabling IRQ.
>>
>> Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only
>> if irqchip implemented irq_disable. In cases where irq_disable is not
>> implemented irq_mask is overridden. Similarly override irq_enable callback
>> only if irqchip implemented irq_enable otherwise irq_unmask is overridden.
>>
>> Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable)
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> 
> I definitely want Hans Verkuils test and review on this, since it
> is a usecase that he is really dependent on.

For this patch:

Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

However, I discovered that patch 256efaea1fdc ("gpiolib: fix up emulated
open drain outputs") broke the cec-gpio driver on the Raspberry Pi starting
with kernel v5.5.

The CEC pin is an open drain pin that is used in both input and output
directions and has an interrupt (which is of course disabled while in
output mode).

With this patch the interrupt can no longer be requested:

[    4.157806] gpio gpiochip0: (pinctrl-bcm2835): gpiochip_lock_as_irq: tried to flag a GPIO set as output for IRQ

[    4.168086] gpio gpiochip0: (pinctrl-bcm2835): unable to lock HW IRQ 7 for IRQ
[    4.175425] genirq: Failed to request resources for cec-gpio@7 (irq 79) on irqchip pinctrl-bcm2835
[    4.184597] cec-gpio: probe of cec-gpio@7 failed with error -5

You can easily test this with a RPi by enabling cec-gpio:

------------------------------------------------------
diff --git a/arch/arm/boot/dts/bcm2837-rpi-3-b.dts b/arch/arm/boot/dts/bcm2837-rpi-3-b.dts
index 054ecaa355c9..87d6ed711ce2 100644
--- a/arch/arm/boot/dts/bcm2837-rpi-3-b.dts
+++ b/arch/arm/boot/dts/bcm2837-rpi-3-b.dts
@@ -29,6 +29,13 @@ wifi_pwrseq: wifi-pwrseq {
 		compatible = "mmc-pwrseq-simple";
 		reset-gpios = <&expgpio 1 GPIO_ACTIVE_LOW>;
 	};
+
+	cec-gpio@7 {
+		compatible = "cec-gpio";
+		cec-gpios = <&gpio 7 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
+		hpd-gpios = <&gpio 17 GPIO_ACTIVE_HIGH>;
+		v5-gpios = <&gpio 22 GPIO_ACTIVE_HIGH>;
+	};
 };

 &firmware {
------------------------------------------------------

Reverting that patch makes it work again.

I'm happy to test a fix for this.

Regards,

	Hans

> 
> Also the irqchip people preferredly.
> 
> But it does seem to mop up my mistakes and fix this up properly!
> 
> So with some testing I'll be happy to merge it, even this one
> patch separately if Hans can verify that it works.
> 
> Yours,
> Linus Walleij
>