[PATCH v3 6/9] arm64: dts: allwinner: fix touchscreen reset GPIO polarity
Robin Murphy
robin.murphy at arm.com
Mon Dec 12 05:21:38 PST 2022
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 at 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 at 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 at 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
More information about the Linux-rockchip
mailing list