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

[Robin Murphy <robin.murphy@arm.com>]

On 2022-12-12 06:32, Samuel Holland wrote:
> 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@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/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@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.

No, the whole point of a binding is to define a contract between 
producers and consumers. It is a specification, not a consensus.

(I see up-thread there was some use of "binding" to refer to DTS 
producers, so maybe there's a bit of confusion in play here)

The goodix.yaml binding does not state that any polarity flag in the 
"reset-gpios" specifier should be ignored, therefore consumers are 
objectively wrong to ignore it, and producers are objectively wrong to 
specify a polarity that does not represent the underlying hardware.

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

As discussed previously, there are already established DTBs in use that 
*correctly* specify both active-low and active-high (hardware-inverted) 
polarities here, so that would mean Zephyr is already broken in general. 
However, since it looks like they've chosen to maintain their own 
project-specific bindings and DTS files, if they do also have the 
equivalent bug then it would seem to be entirely in their own hands.

Thanks,
Robin.

> 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
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip