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
