Messages in this thread | | | Date | Mon, 12 Dec 2022 13:21:38 +0000 | Subject | Re: [PATCH v3 6/9] arm64: dts: allwinner: fix touchscreen reset GPIO polarity | From | Robin Murphy <> |
| |
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
| |