[RFC PATCH 0/7] fix reset line polarity for Goodix touchscreen controllers

[RFC PATCH 0/7] fix reset line polarity for Goodix touchscreen controllers

[Quentin Schulz]

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 yesterday, trying to figure out why the touchscreen wouldn't work.

The driver is "asserting" this reset GPIO by setting its output to 0, probably
to reflect the physical state of the line. However, this relies on the fact that
the Device Tree node setting the reset line polarity to active high, which is
incorrect since the reset is active low in hardware.

To fix this inconsistency, the polarity is inverted to not confuse the user
about the reset line polarity.

This is marked as RFC because it breaks DT compatibility and also the Google
CoachZ device is the only one with an active low polarity for the reset GPIO
in DT, so not sure if it is a typo or its state is actually inverted (so GPIO
active high to drive the reset line low). Changing it anyways since the polarity
is changed in the driver so it needs to be changed in DT too.

I'm all ears if there's a better way to handle this. We could document this in
the DT binding but this kinda breaks the promise we make that the DT is not
bound to the driver implementation.

Thanks,
Quentin

To: Bastien Nocera <hadess@hadess.net>
To: Hans de Goede <hdegoede@redhat.com>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Rob Herring <robh+dt@kernel.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
To: Shawn Guo <shawnguo@kernel.org>
To: Sascha Hauer <s.hauer@pengutronix.de>
To: Pengutronix Kernel Team <kernel@pengutronix.de>
To: Fabio Estevam <festevam@gmail.com>
To: NXP Linux Team <linux-imx@nxp.com>
To: Chen-Yu Tsai <wens@csie.org>
To: Jernej Skrabec <jernej.skrabec@gmail.com>
To: Samuel Holland <samuel@sholland.org>
To: Andy Gross <agross@kernel.org>
To: Bjorn Andersson <andersson@kernel.org>
To: Konrad Dybcio <konrad.dybcio@somainline.org>
To: Heiko Stuebner <heiko@sntech.de>
Cc: linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-sunxi@lists.linux.dev
Cc: linux-arm-msm@vger.kernel.org
Cc: linux-rockchip@lists.infradead.org
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>

---
Quentin Schulz (7):
      Input: goodix - fix reset polarity
      ARM: dts: imx: fix touchscreen reset GPIO polarity
      ARM: dts: sunxi: fix touchscreen reset GPIO polarity
      arm64: dts: allwinner: fix touchscreen reset GPIO polarity
      arm64: dts: imx: fix touchscreen reset GPIO polarity
      arm64: dts: qcom: fix touchscreen reset GPIO polarity
      arm64: dts: rockchip: fix touchscreen reset GPIO polarity

 arch/arm/boot/dts/imx6q-kp.dtsi                                  | 2 +-
 arch/arm/boot/dts/imx6ul-kontron-bl-43.dts                       | 2 +-
 arch/arm/boot/dts/sun7i-a20-wexler-tab7200.dts                   | 2 +-
 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 +-
 arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts                  | 2 +-
 arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts          | 2 +-
 arch/arm64/boot/dts/qcom/msm8998-fxtec-pro1.dts                  | 2 +-
 arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi              | 2 +-
 arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi            | 2 +-
 arch/arm64/boot/dts/qcom/sc7180-trogdor-mrbland.dtsi             | 2 +-
 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 +-
 drivers/input/touchscreen/goodix.c                               | 4 ++--
 17 files changed, 18 insertions(+), 18 deletions(-)
---
base-commit: 8e5423e991e8cd0988d0c4a3f4ac4ca1af7d148a
change-id: 20221103-upstream-goodix-reset-aa1c65994f57

Best regards,
-- 
Quentin Schulz <quentin.schulz@theobroma-systems.com>

Re: [RFC PATCH 0/7] fix reset line polarity for Goodix touchscreen controllers

[Dmitry Torokhov]

Hi Quentin,

On Thu, Nov 03, 2022 at 03:43:45PM +0100, Quentin Schulz wrote:
> 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 yesterday, trying to figure out why the touchscreen wouldn't work.
> 
> The driver is "asserting" this reset GPIO by setting its output to 0, probably
> to reflect the physical state of the line. However, this relies on the fact that
> the Device Tree node setting the reset line polarity to active high, which is
> incorrect since the reset is active low in hardware.
> 
> To fix this inconsistency, the polarity is inverted to not confuse the user
> about the reset line polarity.
> 
> This is marked as RFC because it breaks DT compatibility and also the Google
> CoachZ device is the only one with an active low polarity for the reset GPIO
> in DT, so not sure if it is a typo or its state is actually inverted (so GPIO
> active high to drive the reset line low). Changing it anyways since the polarity
> is changed in the driver so it needs to be changed in DT too.

I would like to get gpio handling into a better shape, but the above is
completely incorrect. "goodix,gt7375p" that is used in CoachZ and other
Google designs is using i2c-hid compatible firmware and is not being
driven by drivers/input/touchscreen/goodix.c driver, but rather by
i2c-hid + hid-multitouch combo.

You should not be touching arch/arm64/boot/dts/qcom/sc7180* at all.

> 
> I'm all ears if there's a better way to handle this. We could document this in
> the DT binding but this kinda breaks the promise we make that the DT is not
> bound to the driver implementation.

I think Hans has already voiced concerns about x86 devices using these
devices and having GPIO data encoded in the driver, so we need to
accommodate them. On DT side we can add a quirk to gpiolib-of.c to
[maybe temporary] override polarity of reset GPIO lines, then update DTS
to match the reality.

Thanks.

-- 
Dmitry

Re: [RFC PATCH 1/7] Input: goodix - fix reset polarity

[Quentin Schulz]

Hi all,

On 11/3/22 18:45, Hans de Goede wrote:
> Hi,
> 
> On 11/3/22 18:32, Dmitry Torokhov wrote:
>> Hi Hans,
>>
>> On Thu, Nov 03, 2022 at 03:58:47PM +0100, Hans de Goede wrote:
>>> Hi Quentin,
>>>
>>> On 11/3/22 15:43, Quentin Schulz wrote:
>>>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>>>
>>>> The reset line is asserted for selecting the I2C target address and then
>>>> deasserted.
>>>
>>> It is not asserted/deasserted, asserted/deasserted is reset-controller/
>>> reset-framework (drivers/reset/*) terminology.
>>>
>>> We are driving GPIOs here and those are driven low/high.
>>
>> Not quite. GPIOD API operates on a logival level (think of them as
>> active/inactive) and allows platform/firmware to specify polarity from
>> the AP point of view (as opposed to device). This important if the
>> peripheral is not attached directly, but potentially through an inverter
>> or something similar).
> 
> Right and if a line runs through an inverting buffer then marking
> the pin as active-low in the DT makes a lot of sense here.
> 

It doesn't to me. /me shrugs

> But as I mentioned before the datasheet spells out a very specific
> init-sequence.
> 

As Dmitry pointed out, we're talking about logical vs physical level. 
The driver tries to enforce physical level (on the touchscreen 
controller side) by expecting the logical level (of the gpio controller) 
to match.

> By default marking all the direct-attached RST pin connections as
> active-low (1) to then invert the value again in the driver
> (from the datasheet init sequence specified values pov) IMHO
> just makes the driver code harder to read when putting it side
> to side by the init-sequence specified in the datasheet.
> 

When I want to put a device into reset mode, I activate/assert the line 
so that its logical state is "active". For Goodix, its reset line is 
active low. I do a "positive" action, so I activate something. If it was 
called nreset, that would be a different story. If it was named 
enable-gpios, I would understand. I just don't get the current 
implementation with reset-gpios in DT.

Reading:
reset-gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
means that I need to set the logical output to HIGH to have a HW reset, 
which is not what happens for this driver.

> I don't see how playing this double-invert game is going to help
> us / gives us any added value, in any way.
> 

The current implementation is playing the double-invert game for me.

So clearly, we do not agree on what, at least in the DT, the level of a 
reset gpio should mean.

> And in all the ACPI tables the GPIOs are marked as active-high
> so changing this to have the driver now all of a sudden expect
> the reset-gpio to be marked as active-low at the gpio-subsys
> level will be quite cumbersome since normally the active-low vs
> -high info comes from the firmware-tables and now all of a sudden
> we need to override this.
> 

We have the information from which standard we got the GPIO, so we could 
always invert the flag we get from DT to match whatever is in ACPI.

Blindly ignoring the DT flag is not an option since the HW design could 
actually require an inversion (GPIO connected to a transistor for 
example). I'm not sure what exactly could be done on the gpio-subsys 
level to deal with this. I think we just disagree on what the reset 
"active state" should mean and no amount of code would fix that?

> During all my work on the goodix driver I have always been very
> careful to not introduce any behavior changes for the DT users
> of the drivers. It would be nice if this courtesy could also
> be extended in the other direction.
> 

This RFC is clearly breaking ACPI support. I have zero knowledge about 
ACPI and didn't know that devm_gpiod_get_optional fetches from OF or 
ACPI. It was not my intention to break ACPI, sorry if it came this way.

I frankly didn't expect this to be an easy discussion, since changing 
the DT is usually a no-go, but as is making the DT binding 
implementation-specific (which is the current state of affairs), e.g. 
we'll need U-Boot/BSD/whatever driver to also use the same logic. I want 
to be noted that I like none of the options I offered so far.

As I was surprised by the (for me) inverted logic of the GPIO state, I 
preferred fixing the driver and DT to match what my expectations were.

I'm looking for guidance on how we can deal with this, I do not claim 
what I suggest is what we should absolutely go for.

Maybe my expectations were misguided and I should just tell my brain to 
invert whatever my intuition tells me, it wouldn't be the first time.

Cheers,
Quentin

P.S.: I've been notified only the cover letter made it to the mailing 
lists, so adding the mailing lists in Cc right now. Hopefully enough 
context is left in the mail. Apologies.

Re: [RFC PATCH 1/7] Input: goodix - fix reset polarity

[Hans de Goede]

Hi Quentin,

On 11/3/22 19:41, Quentin Schulz wrote:
> Hi all,
> 
> On 11/3/22 18:45, Hans de Goede wrote:
>> Hi,
>>
>> On 11/3/22 18:32, Dmitry Torokhov wrote:
>>> Hi Hans,
>>>
>>> On Thu, Nov 03, 2022 at 03:58:47PM +0100, Hans de Goede wrote:
>>>> Hi Quentin,
>>>>
>>>> On 11/3/22 15:43, Quentin Schulz wrote:
>>>>> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>>>>
>>>>> The reset line is asserted for selecting the I2C target address and then
>>>>> deasserted.
>>>>
>>>> It is not asserted/deasserted, asserted/deasserted is reset-controller/
>>>> reset-framework (drivers/reset/*) terminology.
>>>>
>>>> We are driving GPIOs here and those are driven low/high.
>>>
>>> Not quite. GPIOD API operates on a logival level (think of them as
>>> active/inactive) and allows platform/firmware to specify polarity from
>>> the AP point of view (as opposed to device). This important if the
>>> peripheral is not attached directly, but potentially through an inverter
>>> or something similar).
>>
>> Right and if a line runs through an inverting buffer then marking
>> the pin as active-low in the DT makes a lot of sense here.
>>
> 
> It doesn't to me. /me shrugs
> 
>> But as I mentioned before the datasheet spells out a very specific
>> init-sequence.
>>
> 
> As Dmitry pointed out, we're talking about logical vs physical level. The driver tries to enforce physical level (on the touchscreen controller side) by expecting the logical level (of the gpio controller) to match.
> 
>> By default marking all the direct-attached RST pin connections as
>> active-low (1) to then invert the value again in the driver
>> (from the datasheet init sequence specified values pov) IMHO
>> just makes the driver code harder to read when putting it side
>> to side by the init-sequence specified in the datasheet.
>>
> 
> When I want to put a device into reset mode, I activate/assert the line so that its logical state is "active". For Goodix, its reset line is active low. I do a "positive" action, so I activate something. If it was called nreset, that would be a different story. If it was named enable-gpios, I would understand. I just don't get the current implementation with reset-gpios in DT.
> 
> Reading:
> reset-gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
> means that I need to set the logical output to HIGH to have a HW reset, which is not what happens for this driver.
> 
>> I don't see how playing this double-invert game is going to help
>> us / gives us any added value, in any way.
>>
> 
> The current implementation is playing the double-invert game for me.
> 
> So clearly, we do not agree on what, at least in the DT, the level of a reset gpio should mean.

It would seem that way. Anyways lets agree to disagree here.

It seems that Dmitry is in favor of the change you suggest, so
lets just focus on making sure these changes don't break AcPI
support.

>> And in all the ACPI tables the GPIOs are marked as active-high
>> so changing this to have the driver now all of a sudden expect
>> the reset-gpio to be marked as active-low at the gpio-subsys
>> level will be quite cumbersome since normally the active-low vs
>> -high info comes from the firmware-tables and now all of a sudden
>> we need to override this.
>>
> 
> We have the information from which standard we got the GPIO, so we could always invert the flag we get from DT to match whatever is in ACPI.
> 
> Blindly ignoring the DT flag is not an option since the HW design could actually require an inversion (GPIO connected to a transistor for example). I'm not sure what exactly could be done on the gpio-subsys level to deal with this. I think we just disagree on what the reset "active state" should mean and no amount of code would fix that?

I would prefer for the gpiod_direction_output(ts->gpiod_rst, x)
calls to have x actually matching the timing diagrams in
the datasheet.

At a minimum when you invert those from the datasheet, please
add a comment that the values are inverted from the timing
diagram because the GPIO is marked as active-low in their
gpio_desc ?

>> During all my work on the goodix driver I have always been very
>> careful to not introduce any behavior changes for the DT users
>> of the drivers. It would be nice if this courtesy could also
>> be extended in the other direction.
>>
> 
> This RFC is clearly breaking ACPI support. I have zero knowledge about ACPI and didn't know that devm_gpiod_get_optional fetches from OF or ACPI. It was not my intention to break ACPI, sorry if it came this way.
> 
> I frankly didn't expect this to be an easy discussion, since changing the DT is usually a no-go, but as is making the DT binding implementation-specific (which is the current state of affairs), e.g. we'll need U-Boot/BSD/whatever driver to also use the same logic. I want to be noted that I like none of the options I offered so far.

Yes breaking the existing DT bindings / existing DTB files is
probably also going to be a problem. I'm going to defer reviewing
that part of these changes to other people.

> As I was surprised by the (for me) inverted logic of the GPIO state, I preferred fixing the driver and DT to match what my expectations were.
> 
> I'm looking for guidance on how we can deal with this, I do not claim what I suggest is what we should absolutely go for.

Ok, so I've been taking a look at how we can invert the 'x' passed
to the gpiod_direction_output(ts->gpiod_rst, x) calls and not break
things with ACPI.

The rst pin is looked up through a acpi_gpio_mapping which
contains acpi_gpio_params as one of the per pin parameters
and that does have an active_low flag.

After (re)reading the gpiolib code to fresh up my memory
of how this all fits together that flag should do what it
says on the tin.

So if we want to revert the value of x for all the:

gpiod_direction_output(ts->gpiod_rst, x);

calls, then something like the following should work to get
gpiolib to invert that again to turn it into a no-op:

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index a33cc7950cf5..5c294c56a20d 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -797,23 +797,26 @@ static int goodix_reset(struct goodix_ts_data *ts)
 }
 
 #ifdef ACPI_GPIO_SUPPORT
-static const struct acpi_gpio_params first_gpio = { 0, 0, false };
-static const struct acpi_gpio_params second_gpio = { 1, 0, false };
+static const struct acpi_gpio_params int_first_gpio = { 0, 0, false };
+static const struct acpi_gpio_params int_second_gpio = { 1, 0, false };
+
+static const struct acpi_gpio_params rst_first_gpio = { 0, 0, true };
+static const struct acpi_gpio_params rst_second_gpio = { 1, 0, true };
 
 static const struct acpi_gpio_mapping acpi_goodix_int_first_gpios[] = {
-	{ GOODIX_GPIO_INT_NAME "-gpios", &first_gpio, 1 },
-	{ GOODIX_GPIO_RST_NAME "-gpios", &second_gpio, 1 },
+	{ GOODIX_GPIO_INT_NAME "-gpios", &int_first_gpio, 1 },
+	{ GOODIX_GPIO_RST_NAME "-gpios", &rst_second_gpio, 1 },
 	{ },
 };
 
 static const struct acpi_gpio_mapping acpi_goodix_int_last_gpios[] = {
-	{ GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1 },
-	{ GOODIX_GPIO_INT_NAME "-gpios", &second_gpio, 1 },
+	{ GOODIX_GPIO_RST_NAME "-gpios", &rst_first_gpio, 1 },
+	{ GOODIX_GPIO_INT_NAME "-gpios", &int_second_gpio, 1 },
 	{ },
 };
 
 static const struct acpi_gpio_mapping acpi_goodix_reset_only_gpios[] = {
-	{ GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1 },
+	{ GOODIX_GPIO_RST_NAME "-gpios", &rst_first_gpio, 1 },
 	{ },
 };
 
Note this is missing the actual inverting of the
gpiod_direction_output(ts->gpiod_rst, x) calls!

Regards,

Hans

Re: [RFC PATCH 1/7] Input: goodix - fix reset polarity

[Quentin Schulz]

Hi Hans,

Sorry for the delay.

On 11/3/22 20:28, Hans de Goede wrote:
[...]
> Ok, so I've been taking a look at how we can invert the 'x' passed
> to the gpiod_direction_output(ts->gpiod_rst, x) calls and not break
> things with ACPI.
> 
> The rst pin is looked up through a acpi_gpio_mapping which
> contains acpi_gpio_params as one of the per pin parameters
> and that does have an active_low flag.
> 

I just read the kernel docs about GPIO and ACPI and I'm not entirely 
sure this is always 100% safe to do:

https://docs.kernel.org/firmware-guide/acpi/gpio-properties.html

Specifically:
"""
The GpioIo() resource unfortunately doesn't explicitly provide an 
initial state of the output pin which driver should use during its 
initialization.

Linux tries to use common sense here and derives the state from the bias 
and polarity settings. The table below shows the expectations:

=========  =============  ==============
Pull Bias     Polarity     Requested...
=========  =============  ==============
Implicit     x            AS IS (assumed firmware configured for us)
Explicit     x (no _DSD)  as Pull Bias (Up == High, Down == Low),
                           assuming non-active (Polarity = !Pull Bias)
Down         Low          as low, assuming active
Down         High         as low, assuming non-active
Up           Low          as high, assuming non-active
Up           High         as high, assuming active
=========  =============  ==============
"""

But since we actually override this during our devm_gpiod_get_optional 
by passing forcing the flag to be either GPIOD_IN or GPIOD_ASIS, we 
should be good for this driver IIUC?

Thanks for the pointers,
Cheers,
Quentin

Re: [RFC PATCH 1/7] Input: goodix - fix reset polarity

[Hans de Goede]

Hi,

On 11/21/22 16:06, Quentin Schulz wrote:
> Hi Hans,
> 
> Sorry for the delay.
> 
> On 11/3/22 20:28, Hans de Goede wrote:
> [...]
>> Ok, so I've been taking a look at how we can invert the 'x' passed
>> to the gpiod_direction_output(ts->gpiod_rst, x) calls and not break
>> things with ACPI.
>>
>> The rst pin is looked up through a acpi_gpio_mapping which
>> contains acpi_gpio_params as one of the per pin parameters
>> and that does have an active_low flag.
>>
> 
> I just read the kernel docs about GPIO and ACPI and I'm not entirely sure this is always 100% safe to do:
> 
> https://docs.kernel.org/firmware-guide/acpi/gpio-properties.html
> 
> Specifically:
> """
> The GpioIo() resource unfortunately doesn't explicitly provide an initial state of the output pin which driver should use during its initialization.
> 
> Linux tries to use common sense here and derives the state from the bias and polarity settings. The table below shows the expectations:
> 
> =========  =============  ==============
> Pull Bias     Polarity     Requested...
> =========  =============  ==============
> Implicit     x            AS IS (assumed firmware configured for us)
> Explicit     x (no _DSD)  as Pull Bias (Up == High, Down == Low),
>                           assuming non-active (Polarity = !Pull Bias)
> Down         Low          as low, assuming active
> Down         High         as low, assuming non-active
> Up           Low          as high, assuming non-active
> Up           High         as high, assuming active
> =========  =============  ==============
> """
> 
> But since we actually override this during our devm_gpiod_get_optional by passing forcing the flag to be either GPIOD_IN or GPIOD_ASIS, we should be good for this driver IIUC?

Not entirely I just checked and for some reason the ACPI GPIO
lookup code will override the gpiod_flags passed to gpiod_get()
if it can determine a set of flags from the ACPI GpioIo entry.

For output pins like the reset pin, this requores a pull bias
to be set, which often is not the case, so then the GPIOD_ASIS
which we pass in is used.

But if a pull bias is specified in the ACPI GpioIo entry for
the reset pin then thats get translated to GPIOD_OUT_LOW or
GPIOD_OUT_HIGH and setting acpi_gpio_params.active_low as
your patch do will flip those.

So this may cause unintended side-effects.

This is something which we can fix though, we can force
the ACPI GPIO code to honor the GPIOD_ASIS we pass in
by changing:

static const struct acpi_gpio_params first_gpio = { 0, 0, false };
static const struct acpi_gpio_params second_gpio = { 1, 0, false };

to:

static const struct acpi_gpio_params first_gpio = { 0, 0, false, ACPI_GPIO_QUIRK_NO_IO_RESTRICTION };
static const struct acpi_gpio_params second_gpio = { 1, 0, false, ACPI_GPIO_QUIRK_NO_IO_RESTRICTION };

Which will make gpiod_get honor the GPIOD_ASIS for the reset pin
and the GPIOD_IN for the IRQ pin.

It would be good to do this as a preparation patch, because
this will be good to have regardless of the rest of your series
because the gpiolib-acpi behavior without the
ACPI_GPIO_QUIRK_NO_IO_RESTRICTION flag may already cause the
reset GPIO to get its value changed at probe time which we
do not want to happen in the Goodix driver.

If you can send me such a preparation patch + a new 1/7
on top, then I can give this a test on a x86/ACPI device
with a goodix touchscreen.

(Maybe send them offlist if you don't want to send out another
version this quickly.

Regards,

Hans