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

[Quentin Schulz]

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