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>

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