| Message ID | 20221103-upstream-goodix-reset-v2-0-2c38fb03a300@theobroma-systems.com |
|---|---|
| Headers | show |
| Series | fix reset line polarity for Goodix touchscreen controllers | expand |
[Adding Angus and David] On Mon, Nov 21, 2022 at 3:12 PM Quentin Schulz <foss+kernel@0leil.net> 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/freescale/imx8mm-prt8mm.dts | 2 +- > arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts b/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts > index 9fbbbb556c0b3..df7e5ae9698e1 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts > +++ b/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts > @@ -107,7 +107,7 @@ touchscreeen@5d { > interrupt-parent = <&gpio1>; > interrupts = <8 IRQ_TYPE_NONE>; > irq-gpios = <&gpio1 8 GPIO_ACTIVE_HIGH>; > - reset-gpios = <&gpio1 9 GPIO_ACTIVE_HIGH>; > + reset-gpios = <&gpio1 9 GPIO_ACTIVE_LOW>; > }; > > temp-sense@70 { > diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > index 6445c6b90b5bb..b038300812b1e 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > +++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > @@ -542,7 +542,7 @@ touchscreen@5d { > pinctrl-0 = <&pinctrl_ts>; > interrupt-parent = <&gpio3>; > interrupts = <0 IRQ_TYPE_LEVEL_LOW>; > - reset-gpios = <&gpio1 5 GPIO_ACTIVE_HIGH>; > + reset-gpios = <&gpio1 5 GPIO_ACTIVE_LOW>; > irq-gpios = <&gpio3 0 GPIO_ACTIVE_HIGH>; > touchscreen-size-x = <720>; > touchscreen-size-y = <1440>; > > -- > b4 0.10.1
On Mon, 21 Nov 2022 15:18:32 -0300 Fabio Estevam <festevam@gmail.com> wrote: > [Adding Angus and David] Thanks. This was apparently necessary ;-) > On Mon, Nov 21, 2022 at 3:12 PM Quentin Schulz <foss+kernel@0leil.net> 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/freescale/imx8mm-prt8mm.dts | 2 +- > > arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts b/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts > > index 9fbbbb556c0b3..df7e5ae9698e1 100644 > > --- a/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts > > +++ b/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts > > @@ -107,7 +107,7 @@ touchscreeen@5d { > > interrupt-parent = <&gpio1>; > > interrupts = <8 IRQ_TYPE_NONE>; > > irq-gpios = <&gpio1 8 GPIO_ACTIVE_HIGH>; > > - reset-gpios = <&gpio1 9 GPIO_ACTIVE_HIGH>; > > + reset-gpios = <&gpio1 9 GPIO_ACTIVE_LOW>; NACK! The PRT8MM has an inverter in the reset line. The reason for that is that the reset line needs to be inactive when the driving side is unpowered. The DT was correct, this change will break it. > > }; > > > > temp-sense@70 { > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > > index 6445c6b90b5bb..b038300812b1e 100644 > > --- a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > > +++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > > @@ -542,7 +542,7 @@ touchscreen@5d { > > pinctrl-0 = <&pinctrl_ts>; > > interrupt-parent = <&gpio3>; > > interrupts = <0 IRQ_TYPE_LEVEL_LOW>; > > - reset-gpios = <&gpio1 5 GPIO_ACTIVE_HIGH>; > > + reset-gpios = <&gpio1 5 GPIO_ACTIVE_LOW>; > > irq-gpios = <&gpio3 0 GPIO_ACTIVE_HIGH>; > > touchscreen-size-x = <720>; > > touchscreen-size-y = <1440>; > > > > -- > > b4 0.10.1 Best regards,
Hi David, Thanks Fabio for the Cc. On 11/22/22 08:18, David Jander wrote: > On Mon, 21 Nov 2022 15:18:32 -0300 > Fabio Estevam <festevam@gmail.com> wrote: > >> [Adding Angus and David] > > Thanks. This was apparently necessary ;-) > >> On Mon, Nov 21, 2022 at 3:12 PM Quentin Schulz <foss+kernel@0leil.net> 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/freescale/imx8mm-prt8mm.dts | 2 +- >>> arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts b/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts >>> index 9fbbbb556c0b3..df7e5ae9698e1 100644 >>> --- a/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts >>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts >>> @@ -107,7 +107,7 @@ touchscreeen@5d { >>> interrupt-parent = <&gpio1>; >>> interrupts = <8 IRQ_TYPE_NONE>; >>> irq-gpios = <&gpio1 8 GPIO_ACTIVE_HIGH>; >>> - reset-gpios = <&gpio1 9 GPIO_ACTIVE_HIGH>; >>> + reset-gpios = <&gpio1 9 GPIO_ACTIVE_LOW>; > > NACK! > > The PRT8MM has an inverter in the reset line. The reason for that is that the > reset line needs to be inactive when the driving side is unpowered. > The DT was correct, this change will break it. > The DT was correct. The implementation in the driver is changed (the polarity is swapped) in this patch series, therefore the DT isn't correct anymore, hence this patch. See https://lore.kernel.org/linux-input/20221103-upstream-goodix-reset-v2-0-2c38fb03a300@theobroma-systems.com/ for the whole patch series. This DT patch alone is obviously incorrect, but the context around it matters. I could/should have made it all into one big patch, the question is then how this big tree-crossing patch would be merged into Linux (if there's consensus). We're not there yet. For some additional background on the discussion that was had in the v1: https://lore.kernel.org/all/267de96a-0129-a97d-9bf6-e1001b422a1a@theobroma-systems.com/ I messed up the Cc list in the v1, apologies for the missing context in the archived mails, I think one should be able to understand the important bits by reading the answers in-mail. There, Dmitry, Hans and I discussed the meaning of the active level of GPIOs/reset lines and I expressed the reasons for such a change (which are also listed in the cover letter of this patch series). As stated in v1 cover letter, no implementation will satisfy every one. We either make the DT binding implementation specific (which is what it shouldn't be), or we swap the polarity in the Linux implementation and thus the DT but then break DT backward compatibility. Cheers, Quentin
On 2022-11-22 09:58, Quentin Schulz wrote: > Hi David, > > Thanks Fabio for the Cc. > > On 11/22/22 08:18, David Jander wrote: >> On Mon, 21 Nov 2022 15:18:32 -0300 >> Fabio Estevam <festevam@gmail.com> wrote: >> >>> [Adding Angus and David] >> >> Thanks. This was apparently necessary ;-) >> >>> On Mon, Nov 21, 2022 at 3:12 PM Quentin Schulz >>> <foss+kernel@0leil.net> 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/freescale/imx8mm-prt8mm.dts | 2 +- >>>> arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts | 2 +- >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts >>>> b/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts >>>> index 9fbbbb556c0b3..df7e5ae9698e1 100644 >>>> --- a/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts >>>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts >>>> @@ -107,7 +107,7 @@ touchscreeen@5d { >>>> interrupt-parent = <&gpio1>; >>>> interrupts = <8 IRQ_TYPE_NONE>; >>>> irq-gpios = <&gpio1 8 GPIO_ACTIVE_HIGH>; >>>> - reset-gpios = <&gpio1 9 GPIO_ACTIVE_HIGH>; >>>> + reset-gpios = <&gpio1 9 GPIO_ACTIVE_LOW>; >> >> NACK! >> >> The PRT8MM has an inverter in the reset line. The reason for that is >> that the >> reset line needs to be inactive when the driving side is unpowered. >> The DT was correct, this change will break it. >> > > The DT was correct. The implementation in the driver is changed (the > polarity is swapped) in this patch series, therefore the DT isn't > correct anymore, hence this patch. I'm not sure it's quite that simple... FWIW I'm using an add-on LCD module with a GT9271[1] (and I won't be the only one - Raspberry Pi and other SBC users using DT overlays or custom-built DTBs are a whole other can of worms here), where GPIO_ACTIVE_LOW is correctly specified per the schematics, thus "wrong" for the current driver behaviour, yet it *is* working OK as-is. I guess that's because /RSTB ends up driven low for long enough between the current "deassertion" by gpiod_direction_output(1) and gpiod_direction_input() allowing the external pull-up to take it high again. Robin. [1] https://www.friendlyelec.com/index.php?route=product/product&path=81&product_id=230 > > See > https://lore.kernel.org/linux-input/20221103-upstream-goodix-reset-v2-0-2c38fb03a300@theobroma-systems.com/ for the whole patch series. > > This DT patch alone is obviously incorrect, but the context around it > matters. I could/should have made it all into one big patch, the > question is then how this big tree-crossing patch would be merged into > Linux (if there's consensus). We're not there yet. > > For some additional background on the discussion that was had in the v1: > https://lore.kernel.org/all/267de96a-0129-a97d-9bf6-e1001b422a1a@theobroma-systems.com/ > I messed up the Cc list in the v1, apologies for the missing context in > the archived mails, I think one should be able to understand the > important bits by reading the answers in-mail. There, Dmitry, Hans and I > discussed the meaning of the active level of GPIOs/reset lines and I > expressed the reasons for such a change (which are also listed in the > cover letter of this patch series). > > As stated in v1 cover letter, no implementation will satisfy every one. > We either make the DT binding implementation specific (which is what it > shouldn't be), or we swap the polarity in the Linux implementation and > thus the DT but then break DT backward compatibility. > > Cheers, > Quentin > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
Hi Robin, On 11/22/22 13:46, Robin Murphy wrote: > On 2022-11-22 09:58, Quentin Schulz wrote: >> Hi David, >> >> Thanks Fabio for the Cc. >> >> On 11/22/22 08:18, David Jander wrote: >>> On Mon, 21 Nov 2022 15:18:32 -0300 >>> Fabio Estevam <festevam@gmail.com> wrote: >>> >>>> [Adding Angus and David] >>> >>> Thanks. This was apparently necessary ;-) >>> >>>> On Mon, Nov 21, 2022 at 3:12 PM Quentin Schulz >>>> <foss+kernel@0leil.net> 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/freescale/imx8mm-prt8mm.dts | 2 +- >>>>> arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts | 2 +- >>>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts >>>>> b/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts >>>>> index 9fbbbb556c0b3..df7e5ae9698e1 100644 >>>>> --- a/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts >>>>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts >>>>> @@ -107,7 +107,7 @@ touchscreeen@5d { >>>>> interrupt-parent = <&gpio1>; >>>>> interrupts = <8 IRQ_TYPE_NONE>; >>>>> irq-gpios = <&gpio1 8 GPIO_ACTIVE_HIGH>; >>>>> - reset-gpios = <&gpio1 9 GPIO_ACTIVE_HIGH>; >>>>> + reset-gpios = <&gpio1 9 GPIO_ACTIVE_LOW>; >>> >>> NACK! >>> >>> The PRT8MM has an inverter in the reset line. The reason for that is >>> that the >>> reset line needs to be inactive when the driving side is unpowered. >>> The DT was correct, this change will break it. >>> >> >> The DT was correct. The implementation in the driver is changed (the >> polarity is swapped) in this patch series, therefore the DT isn't >> correct anymore, hence this patch. > > I'm not sure it's quite that simple... FWIW I'm using an add-on LCD > module with a GT9271[1] (and I won't be the only one - Raspberry Pi and > other SBC users using DT overlays or custom-built DTBs are a whole other > can of worms here), where GPIO_ACTIVE_LOW is correctly specified per the > schematics, thus "wrong" for the current driver behaviour, yet it *is* > working OK as-is. I guess that's because /RSTB ends up driven low for > long enough between the current "deassertion" by > gpiod_direction_output(1) and gpiod_direction_input() allowing the > external pull-up to take it high again. > Correct, that seems like it is the case for "current-wrong" GPIO level, provided you have a pull-up on the RESET line (as recommended in the datasheet). c.f. https://github.com/hadess/gt9xx/blob/master/specifications/GT9271%20Datasheet.pdf with the current driver, what I assume we should get 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. As for OOT DTB/DTBO, nothing I can do unfortunately, that is correct. The fact that we have both "interpretations" of the RESET GPIO level in Linux is worrying, albeit with OOT DTBs. A driver or subsystem rework might break boards in which case people should update their DTB as suggested here or we'll decide to support both interpretations, which ain't great either. Cheers, Quentin
On Tue, 22 Nov 2022 17:22:45 +0100 Quentin Schulz <quentin.schulz@theobroma-systems.com> wrote: > Hi Robin, > > On 11/22/22 13:46, Robin Murphy wrote: > > On 2022-11-22 09:58, Quentin Schulz wrote: > >> Hi David, > >> > >> Thanks Fabio for the Cc. > >> > >> On 11/22/22 08:18, David Jander wrote: > >>> On Mon, 21 Nov 2022 15:18:32 -0300 > >>> Fabio Estevam <festevam@gmail.com> wrote: > >>> > >>>> [Adding Angus and David] > >>> > >>> Thanks. This was apparently necessary ;-) > >>> > >>>> On Mon, Nov 21, 2022 at 3:12 PM Quentin Schulz > >>>> <foss+kernel@0leil.net> 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/freescale/imx8mm-prt8mm.dts | 2 +- > >>>>> arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts | 2 +- > >>>>> 2 files changed, 2 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts > >>>>> b/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts > >>>>> index 9fbbbb556c0b3..df7e5ae9698e1 100644 > >>>>> --- a/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts > >>>>> +++ b/arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts > >>>>> @@ -107,7 +107,7 @@ touchscreeen@5d { > >>>>> interrupt-parent = <&gpio1>; > >>>>> interrupts = <8 IRQ_TYPE_NONE>; > >>>>> irq-gpios = <&gpio1 8 GPIO_ACTIVE_HIGH>; > >>>>> - reset-gpios = <&gpio1 9 GPIO_ACTIVE_HIGH>; > >>>>> + reset-gpios = <&gpio1 9 GPIO_ACTIVE_LOW>; > >>> > >>> NACK! > >>> > >>> The PRT8MM has an inverter in the reset line. The reason for that is > >>> that the > >>> reset line needs to be inactive when the driving side is unpowered. > >>> The DT was correct, this change will break it. > >>> > >> > >> The DT was correct. The implementation in the driver is changed (the > >> polarity is swapped) in this patch series, therefore the DT isn't > >> correct anymore, hence this patch. > > > > I'm not sure it's quite that simple... FWIW I'm using an add-on LCD > > module with a GT9271[1] (and I won't be the only one - Raspberry Pi and > > other SBC users using DT overlays or custom-built DTBs are a whole other > > can of worms here), where GPIO_ACTIVE_LOW is correctly specified per the > > schematics, thus "wrong" for the current driver behaviour, yet it *is* > > working OK as-is. I guess that's because /RSTB ends up driven low for > > long enough between the current "deassertion" by > > gpiod_direction_output(1) and gpiod_direction_input() allowing the > > external pull-up to take it high again. > > > > Correct, that seems like it is the case for "current-wrong" GPIO level, > provided you have a pull-up on the RESET line (as recommended in the > datasheet). > > c.f. > https://github.com/hadess/gt9xx/blob/master/specifications/GT9271%20Datasheet.pdf > > with the current driver, what I assume we should get 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. > > As for OOT DTB/DTBO, nothing I can do unfortunately, that is correct. > > The fact that we have both "interpretations" of the RESET GPIO level in > Linux is worrying, albeit with OOT DTBs. A driver or subsystem rework > might break boards in which case people should update their DTB as > suggested here or we'll decide to support both interpretations, which > ain't great either. Thanks for the detailed drawings. Yes, it seems this is an unfortunate case of both polarities kind of working. Our DT was correct, but the driver was wrong, even if it seemed to work correctly, so this NACK still stands. Our board has this GPIO pin wired to the gate of a MOSFET which acts as an inverter and also isolates the signal from the GPIO port power rail. So from the GPIO point of view, the signal is ACTIVE_HIGH. Please leave the DT like this. Thanks for fixing the driver. Best regards,