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