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

  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(-)
  

Hi Quentin,

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.

Regards,
Samuel

>  		touchscreen-inverted-x;
>  		touchscreen-inverted-y;
>  	};
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5205-5inmfd.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5205-5inmfd.dts
> index 577f9e1d08a14..990f042f5a5b1 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5205-5inmfd.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5205-5inmfd.dts
> @@ -45,7 +45,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 11 GPIO_ACTIVE_HIGH>;	/* CTP-RST: PH11 */
> +		reset-gpios = <&pio 7 11 GPIO_ACTIVE_LOW>;	/* CTP-RST: PH11 */
>  		touchscreen-inverted-x;
>  		touchscreen-inverted-y;
>  	};
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> index 87847116ab6d9..97359cc7f13e2 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> @@ -167,7 +167,7 @@ touchscreen@5d {
>  		interrupt-parent = <&pio>;
>  		interrupts = <7 4 IRQ_TYPE_LEVEL_HIGH>; /* PH4 */
>  		irq-gpios = <&pio 7 4 GPIO_ACTIVE_HIGH>; /* PH4 */
> -		reset-gpios = <&pio 7 11 GPIO_ACTIVE_HIGH>; /* PH11 */
> +		reset-gpios = <&pio 7 11 GPIO_ACTIVE_LOW>; /* PH11 */
>  		AVDD28-supply = <&reg_ldo_io0>;
>  		VDDIO-supply = <&reg_ldo_io0>;
>  		touchscreen-size-x = <720>;
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts
> index 0a5607f73049e..c0eccc753e3f5 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts
> @@ -189,7 +189,7 @@ touchscreen@5d {
>  		interrupt-parent = <&pio>;
>  		interrupts = <7 4 IRQ_TYPE_LEVEL_HIGH>; /* PH4 */
>  		irq-gpios = <&pio 7 4 GPIO_ACTIVE_HIGH>; /* PH4 */
> -		reset-gpios = <&pio 7 8 GPIO_ACTIVE_HIGH>; /* PH8 */
> +		reset-gpios = <&pio 7 8 GPIO_ACTIVE_LOW>; /* PH8 */
>  		AVDD28-supply = <&reg_ldo_io1>;
>  	};
>  };
>
  

Hi Samuel,

On 12/6/22 01:26, Samuel Holland wrote:
> Hi Quentin,
> 
> 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.

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.

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.

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

Cheers,
Quentin
  

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.

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
  

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