Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <84af9a9e-5d82-7420-e72c-4d984135f2d6@theobroma-systems.com>
Date:   Tue, 22 Nov 2022 17:22:45 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH RFC v2 5/7] arm64: dts: imx: fix touchscreen reset GPIO
 polarity
Content-Language: en-US
To:     Robin Murphy <robin.murphy@arm.com>,
        David Jander <david@protonic.nl>,
        Fabio Estevam <festevam@gmail.com>
Cc:     Quentin Schulz <foss+kernel@0leil.net>,
        "Angus Ainslie (Purism)" <angus@akkea.ca>,
        Shawn Guo <shawnguo@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Heiko Stuebner <heiko@sntech.de>,
        Samuel Holland <samuel@sholland.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Bastien Nocera <hadess@hadess.net>,
        Chen-Yu Tsai <wens@csie.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Hans de Goede <hdegoede@redhat.com>,
        Andy Gross <agross@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        NXP Linux Team <linux-imx@nxp.com>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org,
        linux-arm-msm@vger.kernel.org, linux-input@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev,
        devicetree@vger.kernel.org
References: <20221103-upstream-goodix-reset-v2-0-2c38fb03a300@theobroma-systems.com>
 <20221103-upstream-goodix-reset-v2-5-2c38fb03a300@theobroma-systems.com>
 <CAOMZO5BzWsHAy7KjZe+KEiXVq-Mfpggqjk0vswuzx7nkups3gA@mail.gmail.com>
 <20221122081851.6cb762d8@erd992>
 <907b9321-14dc-dc99-80fc-e1a20ee33a1e@theobroma-systems.com>
 <916a6953-d9b4-c257-c08b-f5277ead71af@arm.com>
From:   Quentin Schulz <quentin.schulz@theobroma-systems.com>
In-Reply-To: <916a6953-d9b4-c257-c08b-f5277ead71af@arm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TkQzOHpwdTdKSU5qUG1PdmtRS2hDNjhBS1RLbGZkZnhZYnVsVVd6M3pmUUFQ?=
 =?utf-8?B?a3FaL0xqbFJ3VHFOdTQwdThPNlJmK2l6VW5yaVNzSTRXWXE0Z1NweHo2clov?=
 =?utf-8?B?d3ZHQUw3N0owSDNpSi9aMTMzV2RXUWV6Qzdqcmp5cnRoeHZHS0JDZnlJZFFQ?=
 =?utf-8?B?cU42WnVud0FhcVZWZDVtcmVKRm5mckR3TUpVRlRGV2hJYlVqaDhyVEtsNEFS?=
 =?utf-8?B?Q2YxeXpMbVprN0NLMkZ6RnNVQk1CUFFCVjIzZ3pDMWU3TjV1SWh2MHlwWEFr?=
 =?utf-8?B?MzhmTnc3UmRpdi9UOTFYbDBRd0UxeE44eTUxZEw3d1BYay9nUnM3VEVVVWNE?=
 =?utf-8?B?YnU5V2xybTNhQmpaV1VJckRpSmZ3bW1OWGZ1ZWFGbWlrMDFOVnhpZ3I4aGli?=
 =?utf-8?B?U1lLSkV1RERneXYvVDQ4eTd5MFJsazk0aDRPZkY4RnkzTW1vdE5QNkNkNDVu?=
 =?utf-8?B?aG5hVTlWdW1GcW5vZVFLakRYYUZGdG40aTlEV0I4UnFiVzdhMjdVQTFDR1o1?=
 =?utf-8?B?TWsrTlhFdkZnS3EyeW12SE4xYksyZEp2OTlSckljVGxEbDA4YUhPNmpQSit1?=
 =?utf-8?B?UVN6V21PRUp5Z0dHNXAzS0RoNWpwVWZXWlNQeVZNbjJiZGVCNlpUK2ZhRnpU?=
 =?utf-8?B?eWpzcTdzUnJtQmlQR0QyRmNvaU05VG9SRFkxRHc3cnFtR2FxM0xwTktqdjhP?=
 =?utf-8?B?dXJoWUNkWnhibFB4bHRpMnNSQTRJYjQ2UUhxU2MrNmJmamlHR0pTZm5lSFJw?=
 =?utf-8?B?VU5Rclh6d2VNNGUrUGs0UVQyRlJ6bjlMUUlFVXBoVTJhTWpkMFdHa0c3Qmtv?=
 =?utf-8?B?MGRscXlpSDlxUG5sSG9JeStIckhTWGwwT2tCdHVNZ2JEaHR0RzMxbGgrSFhD?=
 =?utf-8?B?aDI0VXA0WGlBV2g4cjYzVmR4cEtIQytpcUNVTkxoVHpkdVM0TkNlT3NtWm0r?=
 =?utf-8?B?d2RoRGJDN3ZTYlErUDZQTXB0RCtjKzlWTU5KNGxaNlZkRjdueTh0R1c5UGU5?=
 =?utf-8?B?Sk1ScE0zT2ViWHo4NGRoV1RqSDdGbFc3OG9RT3QvbWp1cVJZSUhFcittdUtB?=
 =?utf-8?B?SWJaVWdzQXUzUmNsdUszSXVmbTE3K0d0Ty9ocUM4N1JIckdUSlkwL0lzcVVq?=
 =?utf-8?B?U2hxbVhmcEp1Uk1zZjZLN093MVlGMEhOa1UvZjR0R3dpUDg0QjQ2VWRVOEJz?=
 =?utf-8?B?K2RqN0pFbFA4Z1V1NlhmZnl1eE1MMDkrc2tUeWRObndsK3BRSVRDT0tqL0FO?=
 =?utf-8?B?UndaODhSMG1tUmZDWGlUcGNXaUtsQVI4NnZhbThmYk5VYVdzT2ZiUjlNMDRs?=
 =?utf-8?B?dVRSakJIMDROTUkzaHFqbG1mRjRHYWFZdm9YN1ZVbk9OQWVPRFAxbnE2aElo?=
 =?utf-8?B?cW5kc1U0ajdtYlR1UmxWTDNETmpvTDdpVTZXVVRLK2NVQjZaSUhlZ1VDdklR?=
 =?utf-8?B?bCtwNWJyYmV6RzVwYTZnNVBORzNORWovdzB6VS9McDBRMlhOWC9ocnZTd1ZU?=
 =?utf-8?B?RUl4WGJKQ2Jlc29TN0xaTHUwM3ZubU9lODBhQVdpb0N6cU5IQnN6REg3Q3Bv?=
 =?utf-8?B?cmVsSm13R2JyaFJ5UTdRRGhNMk9qUUh5M09TK05lT1hZM2hDVkMrcFN5ZnJm?=
 =?utf-8?B?aHpKMXFrSHRBWTVjOTlpV295UEZhdWJXem5NTDNCWGJyWElZVS9oT1BxNThj?=
 =?utf-8?B?U0lGbGlsYkQwUGdBQ0QyQlk4K0lYcTl2VkZuMjc5SER1ejZSWHpqM1VUUTAz?=
 =?utf-8?B?dENoZ2hhVWYrbG04UU0rM2s4d0NPQTdXazJudmc4aWVGeVRITGswaFJBZWcw?=
 =?utf-8?B?dTMrSXFWb2IxTzNwOGdaSVFWM0dWT0dCNUNmL0xFN1NRaTJJa2EzeEdIa1Vp?=
 =?utf-8?B?RXlXdXNwaG50cEQ3NWJ3Q3MvRHJQWk9lSXZyL05mUGdZTVl3b0ZZQi9xb0s1?=
 =?utf-8?B?S2U4WUZwZEhycUMzRXREM3dsdVJDbzBaejhGdGRiUitWTERHb3NDMitpT3cz?=
 =?utf-8?B?VU9uL0I2Zk5RZ2xBaE9KbzE0L2FFV3praFU0ZUZia0dsUXJXSDF5cE00Vmxu?=
 =?utf-8?B?Y3VIaDNhQ0EzNHVNZUN6dFJrOFVMVitpelpyME5QRHZybGxpOWtVM3ZvRWk3?=
 =?utf-8?B?R1N2Q3kxNUdMc3dZYk96ekRKczB3OGZrTHVWY3lZS21ZQmFyQUQxd1dqWXBL?=
 =?utf-8?Q?69ZDs0ZeqQEG3K0/xapM/8M=3D?=
X-OriginatorOrg: theobroma-systems.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2eb923b4-2cb6-4f45-03aa-08dacca5cb31
X-MS-Exchange-CrossTenant-AuthSource: DU2PR04MB8536.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Nov 2022 16:22:49.8095
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 5e0e1b52-21b5-4e7b-83bb-514ec460677e
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: I24sxmFUzR5uPUe5aNFCLJC3zBRdhBjQWX6Mwdx2wXh1QLbnQbM35w3FeGIkmUeSMSfjp31+KkXrlSJUYPmq95LLpVlsGqKfq/XR4q8LfB6Dxbjn8eCM2qJtfm7wWhq8
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB7182
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Robin,

On 11/22/22 13:46, Robin Murphy wrote:
> On 2022-11-22 09:58, Quentin Schulz wrote:
>> Hi David,
>>
>> Thanks Fabio for the Cc.
>>
>> On 11/22/22 08:18, David Jander wrote:
>>> On Mon, 21 Nov 2022 15:18:32 -0300
>>> Fabio Estevam <festevam@gmail.com> wrote:
>>>
>>>> [Adding Angus and David]
>>>
>>> Thanks. This was apparently necessary ;-)
>>>
>>>> On Mon, Nov 21, 2022 at 3:12 PM Quentin Schulz 
>>>> <foss+kernel@0leil.net> wrote:
>>>>>
>>>>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>>>>
>>>>> The reset line is active low for the Goodix touchscreen controller so
>>>>> let's fix the polarity in the Device Tree node.
>>>>>
>>>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>>>> ---
>>>>>   arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts         | 2 +-
>>>>>   arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts | 2 +-
>>>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts 
>>>>> b/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts
>>>>> index 9fbbbb556c0b3..df7e5ae9698e1 100644
>>>>> --- a/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts
>>>>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts
>>>>> @@ -107,7 +107,7 @@ touchscreeen@5d {
>>>>>                  interrupt-parent = <&gpio1>;
>>>>>                  interrupts = <8 IRQ_TYPE_NONE>;
>>>>>                  irq-gpios = <&gpio1 8 GPIO_ACTIVE_HIGH>;
>>>>> -               reset-gpios = <&gpio1 9 GPIO_ACTIVE_HIGH>;
>>>>> +               reset-gpios = <&gpio1 9 GPIO_ACTIVE_LOW>;
>>>
>>> NACK!
>>>
>>> The PRT8MM has an inverter in the reset line. The reason for that is 
>>> that the
>>> reset line needs to be inactive when the driving side is unpowered.
>>> The DT was correct, this change will break it.
>>>
>>
>> The DT was correct. The implementation in the driver is changed (the 
>> polarity is swapped) in this patch series, therefore the DT isn't 
>> correct anymore, hence this patch.
> 
> I'm not sure it's quite that simple... FWIW I'm using an add-on LCD 
> module with a GT9271[1] (and I won't be the only one - Raspberry Pi and 
> other SBC users using DT overlays or custom-built DTBs are a whole other 
> can of worms here), where GPIO_ACTIVE_LOW is correctly specified per the 
> schematics, thus "wrong" for the current driver behaviour, yet it *is* 
> working OK as-is. I guess that's because /RSTB ends up driven low for 
> long enough between the current "deassertion" by 
> gpiod_direction_output(1) and gpiod_direction_input() allowing the 
> external pull-up to take it high again.
> 

Correct, that seems like it is the case for "current-wrong" GPIO level, 
provided you have a pull-up on the RESET line (as recommended in the 
datasheet).

c.f. 
https://github.com/hadess/gt9xx/blob/master/specifications/GT9271%20Datasheet.pdf

with the current driver, what I assume we should get is:
             ___________________
INT _______|                   |___________

     ____________           __________________
RST             |_________|

    ^
    L__ pull-up on RST so high by default
         ^
         L___ gpiod_direction_output(0) (deassert GPIO active-low, so high)
            ^
            L____ goodix_irq_direction_output
                 ^
                 L___ gpiod_direction_output(1) (assert GPIO active-low, 
so low)
                           ^
                           L____ gpiod_direction_input() (floating, 
pull-up on RST so high)

This works because of the pull-up on RST and that what matters is that 
the INT lane is configured 100µs before a rising edge on RST line (for 
at least 5ms). However, the init sequence is not properly followed and 
might get broken in the future since it is not something that we 
explicitly support.

With the proposed patch:
             ___________________
INT _______|                   |___________

     ____         __________________
RST     |_______|

    ^
    L__ pull-up on RST so high by default
         ^
         L___ gpiod_direction_output(1) (assert GPIO active-low, so low)
            ^
            L____ goodix_irq_direction_output
                 ^
                 L___ gpiod_direction_output(1) (deassert GPIO 
active-low, so high)
                           ^
                           L____ gpiod_direction_input() (floating, 
pull-up on RST so high)

This should work too and does not rely on some side effects/timings and 
should be future-proof.

As for OOT DTB/DTBO, nothing I can do unfortunately, that is correct.

The fact that we have both "interpretations" of the RESET GPIO level in 
Linux is worrying, albeit with OOT DTBs. A driver or subsystem rework 
might break boards in which case people should update their DTB as 
suggested here or we'll decide to support both interpretations, which 
ain't great either.

Cheers,
Quentin