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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxxxxxxxxx>
---
  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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux