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

  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.

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 I cannot test ACPI
support. 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?

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>
---
Changes in v2:
- implemented ACPI support as suggested by Hans,
- removed Qcom SC7180 Trogdor-based devices changes as they are not using this Goodix driver,
- added comment on how to read gpiod_request_output and the GPIO DT polarity,
- Link to v1: https://lore.kernel.org/r/20221103-upstream-goodix-reset-v1-0-87b49ae589f1@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 +-
 .../dts/allwinner/sun50i-a64-amarula-relic.dts     |  2 +-
 .../allwinner/sun50i-a64-oceanic-5205-5inmfd.dts   |  2 +-
 .../boot/dts/allwinner/sun50i-a64-pinephone.dtsi   |  2 +-
 .../boot/dts/allwinner/sun50i-a64-pinetab.dts      |  2 +-
 arch/arm64/boot/dts/freescale/imx8mm-prt8mm.dts    |  2 +-
 .../boot/dts/freescale/imx8mq-librem5-devkit.dts   |  2 +-
 arch/arm64/boot/dts/qcom/msm8998-fxtec-pro1.dts    |  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                 | 45 +++++++++++++++++-----
 14 files changed, 48 insertions(+), 23 deletions(-)
---
base-commit: 84368d882b9688bfac77ce48d33b1e20a4e4a787
change-id: 20221103-upstream-goodix-reset-aa1c65994f57

Best regards,
  

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>


The reset line is asserted for selecting the I2C target address and then

deasserted.



This inverted logic works because the boards using this touchscreen

controller also invert the polarity of their reset GPIO.



Instead of depending on this double-inversion of meaning, let's *assert*

the line.



Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>

---

Changes in v2:

- implemented ACPI support as suggested by Hans,

- added comment on how to read gpiod_request_output and the GPIO DT polarity,



 drivers/input/touchscreen/goodix.c | 45 +++++++++++++++++++++++++++++---------

 1 file changed, 35 insertions(+), 10 deletions(-)



diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c

index a33cc7950cf5b..da10cbb6f8264 100644

--- a/drivers/input/touchscreen/goodix.c

+++ b/drivers/input/touchscreen/goodix.c

@@ -742,8 +742,25 @@ int goodix_reset_no_int_sync(struct goodix_ts_data *ts)

 {

 	int error;

 

-	/* begin select I2C slave addr */

-	error = gpiod_direction_output(ts->gpiod_rst, 0);

+	/*

+	 * begin select I2C slave addr by activating/asserting RESET.

+	 *

+	 * The value passed to gpiod_direction_output is decorrelated from the

+	 * actual physical state of the line. The 1 value here is just to

+	 * specify the *assertion* of the line, its meaning being dependent on

+	 * the HW design of the system.

+	 *

+	 * DT-based systems need to specify the GPIO level in which the reset is

+	 * active. Since the touchscreen controller is in reset when its RESET

+	 * line is low, it is the level of the GPIO that results in the RESET

+	 * pin on the touchscreen controller side being low. In a HW design in

+	 * which the GPIO is directly connected to the touchscreen controller

+	 * RESET pin, this would be GPIO_ACTIVE_LOW.

+	 *

+	 * ACPI systems do not have the ability to specify the level of the GPIO

+	 * and they are therefore all assumed active low.

+	 */

+	error = gpiod_direction_output(ts->gpiod_rst, 1);

 	if (error)

 		goto error;

 

@@ -756,7 +773,8 @@ int goodix_reset_no_int_sync(struct goodix_ts_data *ts)

 

 	usleep_range(100, 2000);		/* T3: > 100us */

 

-	error = gpiod_direction_output(ts->gpiod_rst, 1);

+	/* Disable/de-assert RESET */

+	error = gpiod_direction_output(ts->gpiod_rst, 0);

 	if (error)

 		goto error;

 

@@ -797,23 +815,30 @@ 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 };

+

+/*

+ * The controller is in reset when the RESET GPIO is output low, so

+ * set acpi_gpio_params.active_low appropriately.

+ */

+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 },

 	{ },

 };
  

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>

---

Changes in v2:

- removed Qcom SC7180 Trogdor-based devices changes as they are not using this Goodix driver,



 arch/arm64/boot/dts/qcom/msm8998-fxtec-pro1.dts | 2 +-

 1 file changed, 1 insertion(+), 1 deletion(-)



diff --git a/arch/arm64/boot/dts/qcom/msm8998-fxtec-pro1.dts b/arch/arm64/boot/dts/qcom/msm8998-fxtec-pro1.dts

index 429ba57e20f71..8f738cade2652 100644

--- a/arch/arm64/boot/dts/qcom/msm8998-fxtec-pro1.dts

+++ b/arch/arm64/boot/dts/qcom/msm8998-fxtec-pro1.dts

@@ -249,7 +249,7 @@ touchscreen@14 {

 		reg = <0x14>;

 		interrupt-parent = <&tlmm>;

 		interrupts = <125 IRQ_TYPE_LEVEL_LOW>;

-		reset-gpios = <&tlmm 89 GPIO_ACTIVE_HIGH>;

+		reset-gpios = <&tlmm 89 GPIO_ACTIVE_LOW>;

 		AVDD28-supply = <&vreg_l28_3p0>;

 		VDDIO-supply = <&ts_vio_vreg>;

 		pinctrl-names = "active";