[PATCH v3 6/9] arm64: dts: allwinner: fix touchscreen reset GPIO polarity

[Samuel Holland]

Hi Quentin,

On 12/6/22 05:11, Quentin Schulz wrote:
> On 12/6/22 01:26, Samuel Holland wrote:
>> On 12/5/22 07:40, Quentin Schulz wrote:
>>> From: Quentin Schulz <quentin.schulz at 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 at theobroma-systems.com>
>>> ---
>>>   arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts       |
>>> 2 +-
>>>   arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5205-5inmfd.dts |
>>> 2 +-
>>>   arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi          |
>>> 2 +-
>>>   arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts             |
>>> 2 +-
>>>   4 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git
>>> a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
>>> b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
>>> index 8233582f62881..5fd581037d987 100644
>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
>>> @@ -122,7 +122,7 @@ touchscreen at 5d {
>>>           interrupt-parent = <&pio>;
>>>           interrupts = <7 4 IRQ_TYPE_EDGE_FALLING>;
>>>           irq-gpios = <&pio 7 4 GPIO_ACTIVE_HIGH>;    /* CTP-INT: PH4 */
>>> -        reset-gpios = <&pio 7 8 GPIO_ACTIVE_HIGH>;    /* CTP-RST:
>>> PH8 */
>>> +        reset-gpios = <&pio 7 8 GPIO_ACTIVE_LOW>;    /* CTP-RST: PH8 */
>>
>> You are changing the DT binding here, in a way that breaks backward
>> compatibility with existing devicetrees. NACK.
>>
> 
> Yes.
> 
> Some boards will get their DT binding broken, there's no way around it
> sadly.
> 
> We know already that the PRT8MM DT binding was written with a different
> understanding than for other boards. There are some board schematics I
> don't have access to so maybe the same applies to those.
> 
> A reminder that even if you got your polarity wrong, it could still work
> in some cases (timings right today but nothing guaranteed it'll stay
> this way forever).
> 
> with the current driver, what I assume we should get for an "incorrect"
> polarity (with GPIO_ACTIVE_LOW) 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.

We as platform DT/binding maintainers explicitly support compatibility
with existing devicetrees, whether those devicetrees are "correct" or
not. If a new version of Linux does not work with an old DT, that is a
regression in Linux.

> 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.

Thanks for the explanation. So the reset sequence happens to work with
either GPIO polarity because the pin is set to high impedance before and
afterward. I tested this patch (no driver changes) on a PinePhone, and
indeed Linux's touchscreen driver still loads and works fine.

> The other option would be to only fix known "broken" boards (e.g.
> PRT8MM, maybe others) and specify in the DT binding documentation that
> the reset-gpios polarity is "inverted" (that is, the reset is asserted
> when the reset-gpios as specified in the DT is deasserted). This makes
> the DT binding documentation **implementation specific** which is
> everything the DT binding is trying to avoid.

Not really, the binding just encodes existing practice. New boards must
invert the polarity relative to the datasheet because existing boards
did the same thing previously. The board devicetrees drive the binding;
Linux is only a consumer of it. So the binding is still not Linux-specific.

In fact, here you rely on the "implementation specific" behavior of the
Linux driver to claim that this a non-breaking change. If some other DT
consumer has a driver which leaves the reset line as an output, this
patch would be a breaking change for them. And it turns out that such a
driver exists:

https://github.com/zephyrproject-rtos/zephyr/commit/17089a2e14acb0428502
https://github.com/zephyrproject-rtos/zephyr/pull/48927

> Something needs to be done, and no solution will make everyone happy.

I am happy as long as the change does not create any DT compatibility
issues, either between OSes or between OS versions. You demonstrated
that Linux was fine, and the BSDs don't have appear to have a driver for
this hardware. So from an Allwinner platform perspective, I am fine with
this patch.

But you should ensure the Zephyr folks are okay with making the same
change to their driver and devicetrees, since it is a breaking change
for them.

Regards,
Samuel