| Message ID | 20221103-upstream-goodix-reset-v3-0-0975809eb183@theobroma-systems.com |
|---|---|
| Headers | show |
| Series | fix reset line polarity for Goodix touchscreen controllers | expand |
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@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. 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
On 05.12.22 14: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/arm/boot/dts/imx6q-kp.dtsi | 2 +- > arch/arm/boot/dts/imx6ul-kontron-bl-43.dts | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/imx6q-kp.dtsi b/arch/arm/boot/dts/imx6q-kp.dtsi > index 1ade0bff681d6..dae14aaf803a8 100644 > --- a/arch/arm/boot/dts/imx6q-kp.dtsi > +++ b/arch/arm/boot/dts/imx6q-kp.dtsi > @@ -188,7 +188,7 @@ touchscreen@5d { > interrupt-parent = <&gpio1>; > interrupts = <9 IRQ_TYPE_EDGE_FALLING>; > irq-gpios = <&gpio1 9 GPIO_ACTIVE_HIGH>; > - reset-gpios = <&gpio5 2 GPIO_ACTIVE_HIGH>; > + reset-gpios = <&gpio5 2 GPIO_ACTIVE_LOW>; > }; > > ds1307: rtc@32 { > diff --git a/arch/arm/boot/dts/imx6ul-kontron-bl-43.dts b/arch/arm/boot/dts/imx6ul-kontron-bl-43.dts > index 0c643706a158b..767ef5da76136 100644 > --- a/arch/arm/boot/dts/imx6ul-kontron-bl-43.dts > +++ b/arch/arm/boot/dts/imx6ul-kontron-bl-43.dts > @@ -29,7 +29,7 @@ touchscreen@5d { > pinctrl-0 = <&pinctrl_cap_touch>; > interrupt-parent = <&gpio5>; > interrupts = <6 IRQ_TYPE_LEVEL_LOW>; > - reset-gpios = <&gpio5 8 GPIO_ACTIVE_HIGH>; > + reset-gpios = <&gpio5 8 GPIO_ACTIVE_LOW>; > irq-gpios = <&gpio5 6 GPIO_ACTIVE_HIGH>; > }; > }; The imx6ul-kontron-bl-43.dts misses a working panel node, so I'm pretty sure it is not used and nobody will bother about change breaking compatibility. I don't have the hardware at hand at the moment to perform a test, so for now: Acked-by: Frieder Schrempf <frieder.schrempf@kontron.de> # imx6ul-kontron-bl-43.dts
On Tue, 6 Dec 2022 12:25:29 +0100 Frieder Schrempf <frieder.schrempf@kontron.de> wrote: > On 05.12.22 14: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/arm/boot/dts/imx6q-kp.dtsi | 2 +- > > arch/arm/boot/dts/imx6ul-kontron-bl-43.dts | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm/boot/dts/imx6q-kp.dtsi > > b/arch/arm/boot/dts/imx6q-kp.dtsi index > > 1ade0bff681d6..dae14aaf803a8 100644 --- > > a/arch/arm/boot/dts/imx6q-kp.dtsi +++ > > b/arch/arm/boot/dts/imx6q-kp.dtsi @@ -188,7 +188,7 @@ > > touchscreen@5d { interrupt-parent = <&gpio1>; > > interrupts = <9 IRQ_TYPE_EDGE_FALLING>; > > irq-gpios = <&gpio1 9 GPIO_ACTIVE_HIGH>; > > - reset-gpios = <&gpio5 2 GPIO_ACTIVE_HIGH>; > > + reset-gpios = <&gpio5 2 GPIO_ACTIVE_LOW>; > > }; > > > > ds1307: rtc@32 { > > diff --git a/arch/arm/boot/dts/imx6ul-kontron-bl-43.dts > > b/arch/arm/boot/dts/imx6ul-kontron-bl-43.dts index > > 0c643706a158b..767ef5da76136 100644 --- > > a/arch/arm/boot/dts/imx6ul-kontron-bl-43.dts +++ > > b/arch/arm/boot/dts/imx6ul-kontron-bl-43.dts @@ -29,7 +29,7 @@ > > touchscreen@5d { pinctrl-0 = <&pinctrl_cap_touch>; > > interrupt-parent = <&gpio5>; > > interrupts = <6 IRQ_TYPE_LEVEL_LOW>; > > - reset-gpios = <&gpio5 8 GPIO_ACTIVE_HIGH>; > > + reset-gpios = <&gpio5 8 GPIO_ACTIVE_LOW>; > > irq-gpios = <&gpio5 6 GPIO_ACTIVE_HIGH>; > > }; > > }; > > The imx6ul-kontron-bl-43.dts misses a working panel node, so I'm > pretty sure it is not used and nobody will bother about change > breaking compatibility. I don't have the hardware at hand at the > moment to perform a test, so for now: > > Acked-by: Frieder Schrempf <frieder.schrempf@kontron.de> # > imx6ul-kontron-bl-43.dts Reviewed-by: Lukasz Majewski <lukma@denx.de> For imx6 TPC70 device. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
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
Hi Quentin, On 12/5/22 14: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> Cannot test at the moment, but LGTM! At least as far as the RK3568 EVB1 is concerned: Acked-by: Michael Riesch <michael.riesch@wolfvision.net> Thanks and best regards, Michael > --- > arch/arm64/boot/dts/rockchip/px30-evb.dts | 2 +- > arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi | 2 +- > arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/boot/dts/rockchip/px30-evb.dts b/arch/arm64/boot/dts/rockchip/px30-evb.dts > index c1bbd555f5f5b..2087dc7299446 100644 > --- a/arch/arm64/boot/dts/rockchip/px30-evb.dts > +++ b/arch/arm64/boot/dts/rockchip/px30-evb.dts > @@ -420,7 +420,7 @@ touchscreen@14 { > interrupt-parent = <&gpio0>; > interrupts = <RK_PA5 IRQ_TYPE_LEVEL_LOW>; > irq-gpios = <&gpio0 RK_PA5 GPIO_ACTIVE_LOW>; > - reset-gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>; > + reset-gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_LOW>; > VDDIO-supply = <&vcc3v3_lcd>; > }; > > diff --git a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi > index 78157521e9449..e63491fb443be 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi > @@ -588,7 +588,7 @@ touch: touchscreen@5d { > AVDD28-supply = <&vcc3v0_touch>; > VDDIO-supply = <&vcc3v0_touch>; > irq-gpios = <&gpio4 RK_PD5 GPIO_ACTIVE_HIGH>; > - reset-gpios = <&gpio4 RK_PD6 GPIO_ACTIVE_HIGH>; > + reset-gpios = <&gpio4 RK_PD6 GPIO_ACTIVE_LOW>; > status = "disabled"; > }; > }; > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > index 674792567fa6e..234531aaa430a 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts > @@ -495,7 +495,7 @@ touchscreen0: goodix@14 { > irq-gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>; > pinctrl-names = "default"; > pinctrl-0 = <&touch_int &touch_rst>; > - reset-gpios = <&gpio0 RK_PB6 GPIO_ACTIVE_HIGH>; > + reset-gpios = <&gpio0 RK_PB6 GPIO_ACTIVE_LOW>; > VDDIO-supply = <&vcc3v3_lcd0_n>; > }; > }; >
On Mon, 5 Dec 2022 14:40:29 +0100, Quentin Schulz wrote: > From: Quentin Schulz <quentin.schulz@theobroma-systems.com> > > The Goodix touchscreen controller has a reset line active low. It happens to > also be used to configure its i2c address at runtime. If the reset line is > incorrectly asserted, the address will be wrongly configured. This cost me a few > hours, trying to figure out why the touchscreen wouldn't work. > > [...] Applied, thanks! [8/9] arm64: dts: qcom: msm8998-fxtec: fix touchscreen reset GPIO polarity commit: 8a0721dae68fdb4534e220fc9faae7a0ef2f3785 Best regards,
Hi Bjorn, all, On 1/10/23 17:17, Bjorn Andersson wrote: > On Mon, 5 Dec 2022 14:40:29 +0100, Quentin Schulz wrote: >> From: Quentin Schulz <quentin.schulz@theobroma-systems.com> >> >> The Goodix touchscreen controller has a reset line active low. It happens to >> also be used to configure its i2c address at runtime. If the reset line is >> incorrectly asserted, the address will be wrongly configured. This cost me a few >> hours, trying to figure out why the touchscreen wouldn't work. >> >> [...] > > Applied, thanks! > > [8/9] arm64: dts: qcom: msm8998-fxtec: fix touchscreen reset GPIO polarity > commit: 8a0721dae68fdb4534e220fc9faae7a0ef2f3785 > Thank you for the merge, however I think there could be some issue here. This requires the patches 1, 2 and 3 all modifying the input driver in order to not introduce a regression. I mistakenly removed the RFC tag and seemingly didn't make it clear enough that I had some question on how to properly merge this patch series, c.f. "Do we also make this patch series only one patchset since the DT patches depend on the driver patch and vice-versa? In which tree would this go?" in the cover letter. So please, how do we go on with the rest of the patch series? Should I submit a v4 which would be only one patch with DT and input changes all at once and Bjorn reverts the patch they had just merged? @Dmitry, since you would have to merge at least patches 1 to 3 in your tree (I assume), would you be willing to take the DT patches at the same time through your tree too? Are the appropriate device DT maintainers OK with this? Cheers, Quentin
Hi all, On 1/16/23 13:37, Quentin Schulz wrote: > Hi Bjorn, all, > > On 1/10/23 17:17, Bjorn Andersson wrote: >> On Mon, 5 Dec 2022 14:40:29 +0100, Quentin Schulz wrote: >>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com> >>> >>> The Goodix touchscreen controller has a reset line active low. It >>> happens to >>> also be used to configure its i2c address at runtime. If the reset >>> line is >>> incorrectly asserted, the address will be wrongly configured. This >>> cost me a few >>> hours, trying to figure out why the touchscreen wouldn't work. >>> >>> [...] >> >> Applied, thanks! >> >> [8/9] arm64: dts: qcom: msm8998-fxtec: fix touchscreen reset GPIO >> polarity >> commit: 8a0721dae68fdb4534e220fc9faae7a0ef2f3785 >> > > Thank you for the merge, however I think there could be some issue here. > > This requires the patches 1, 2 and 3 all modifying the input driver in > order to not introduce a regression. > > I mistakenly removed the RFC tag and seemingly didn't make it clear > enough that I had some question on how to properly merge this patch > series, c.f. "Do we also make this patch series only one patchset since > the DT patches depend > on the driver patch and vice-versa? In which tree would this go?" in the > cover letter. > > So please, how do we go on with the rest of the patch series? Should I > submit a v4 which would be only one patch with DT and input changes all > at once and Bjorn reverts the patch they had just merged? > > @Dmitry, since you would have to merge at least patches 1 to 3 in your > tree (I assume), would you be willing to take the DT patches at the same > time through your tree too? Are the appropriate device DT maintainers OK > with this? > Ping. Cheers, Quentin