From: Quentin Schulz <[email protected]>
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 obviously requires to fix the DT since most
users had the "incorrect" value in there, it needs to be inverted.
Note that the v2 highlighted that I was not the only one that got confused since
PRT8MM board has the "correct" HW representation for this line in DT (which does
not match what the driver was expecting).
This is marked as RFC because I can neither test ACPI support nor boards I don't
own. Please test on the boards you have that are impacted by this patchset and
give your Tested-By.
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?
Thanks,
Quentin
To: Bastien Nocera <[email protected]>
To: Hans de Goede <[email protected]>
To: Dmitry Torokhov <[email protected]>
To: Rob Herring <[email protected]>
To: Krzysztof Kozlowski <[email protected]>
To: Shawn Guo <[email protected]>
To: Sascha Hauer <[email protected]>
To: Pengutronix Kernel Team <[email protected]>
To: Fabio Estevam <[email protected]>
To: NXP Linux Team <[email protected]>
To: Chen-Yu Tsai <[email protected]>
To: Jernej Skrabec <[email protected]>
To: Samuel Holland <[email protected]>
To: Andy Gross <[email protected]>
To: Bjorn Andersson <[email protected]>
To: Konrad Dybcio <[email protected]>
To: Heiko Stuebner <[email protected]>
To: David Jander <[email protected]>
To: Angus Ainslie <[email protected]>
To: Peter Geis <[email protected]>
To: Michael Riesch <[email protected]>
To: Konrad Dybcio <[email protected]>
To: AngeloGioacchino Del Regno <[email protected]>
To: Guido Günther <[email protected]>
To: Jagan Teki <[email protected]>
To: Ondrej Jirman <[email protected]>
To: Icenowy Zheng <[email protected]>
To: Aleksei Mamlin <[email protected]>
To: Lukasz Majewski <[email protected]>
To: Frieder Schrempf <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Quentin Schulz <[email protected]>
---
Changes in v3:
- Cc'ing people who contributed to DTS of impacted boards,
- removed PRT8MM DTS change since it's been reported the polarity is actually
correct (goes through an inverter), keeping the appropriate folks in Cc though
since it'd be a good idea to check this patch series anyways,
- added ACPI_GPIO_QUIRK_NO_IO_RESTRICTION to acpi_gpio_mapping quirks to make
gpiolib-acpi core respect GPIOD_ASIS flag in gpiod_get,
- checked schematics of:
- pinephone: https://files.pine64.org/doc/PinePhone/PinePhone%20v1.2%20Released%20Schematic.pdf
- pinetab: https://files.pine64.org/doc/PineTab/PineTab%20Schematic%20v1.2-20191125.pdf
- px30 evb: https://opensource.rock-chips.com/images/d/db/Px30_mini_evb_v10_20180528.pdf
- rockpro64: https://files.pine64.org/doc/rockpro64/rockpro64_v21-SCH.pdf
- librem5 devkit: https://source.puri.sm/Librem5/dvk-mx8m-bsb/blob/master/dvk-mx8m-bsb.pdf
All seems to be directly connected to the GPIO on the SoC side, without an
inverter on the line.
- Link to v2: https://lore.kernel.org/r/20221103-upstream-goodix-reset-v2-0-2c38fb03a300@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 (9):
Input: goodix - add macro for gpio mapping
Input: goodix - make gpiod_get honor GPIOD_ASIS
Input: goodix - fix reset polarity
ARM: dts: imx: fix touchscreen reset GPIO polarity
ARM: dts: sunxi: fix touchscreen reset GPIO polarity on Wexler TAB7200 tablet
arm64: dts: allwinner: fix touchscreen reset GPIO polarity
arm64: dts: librem5: fix touchscreen reset GPIO polarity
arm64: dts: qcom: msm8998-fxtec: 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 +-
.../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 | 54 ++++++++++++++++++----
13 files changed, 56 insertions(+), 22 deletions(-)
---
base-commit: 76dcd734eca23168cb008912c0f69ff408905235
change-id: 20221103-upstream-goodix-reset-aa1c65994f57
Best regards,
--
Quentin Schulz <[email protected]>
From: Quentin Schulz <[email protected]>
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 <[email protected]>
---
arch/arm/boot/dts/sun7i-a20-wexler-tab7200.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/sun7i-a20-wexler-tab7200.dts b/arch/arm/boot/dts/sun7i-a20-wexler-tab7200.dts
index fef02fcbbdf82..8c79ecdb40608 100644
--- a/arch/arm/boot/dts/sun7i-a20-wexler-tab7200.dts
+++ b/arch/arm/boot/dts/sun7i-a20-wexler-tab7200.dts
@@ -114,7 +114,7 @@ gt911: touchscreen@5d {
interrupt-parent = <&pio>;
interrupts = <7 21 IRQ_TYPE_EDGE_FALLING>; /* EINT21 (PH21) */
irq-gpios = <&pio 7 21 GPIO_ACTIVE_HIGH>; /* INT (PH21) */
- reset-gpios = <&pio 1 13 GPIO_ACTIVE_HIGH>; /* RST (PB13) */
+ reset-gpios = <&pio 1 13 GPIO_ACTIVE_LOW>; /* RST (PB13) */
touchscreen-swapped-x-y;
};
};
--
b4 0.10.1
From: Quentin Schulz <[email protected]>
For some reason the ACPI GPIO lookup code (acpi_find_gpio followed by
acpi_gpio_update_gpiod_flags) 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 requires 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 that gets translated to GPIOD_OUT_LOW or GPIOD_OUT_HIGH meaning
we cannot guarantee the same behavior on all boards.
So this may cause unintended side-effects, c.f. commit a2fd46cd3dbb
("Input: goodix - try not to touch the reset-pin on x86/ACPI devices")
for some background.
This is something which we can fix though, we can force the ACPI GPIO
code to honor the GPIOD_ASIS we pass in by passing
ACPI_GPIO_QUIRK_NO_IO_RESTRICTION to the ACPI gpio mapping.
Suggested-by: Hans de Goede <[email protected]>
Signed-off-by: Quentin Schulz <[email protected]>
---
drivers/input/touchscreen/goodix.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index d73d4272a8ea5..1639f2f8a31e3 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -803,6 +803,7 @@ static int goodix_reset(struct goodix_ts_data *ts)
.name = _name, \
.data = _params, \
.size = _size, \
+ .quirks = ACPI_GPIO_QUIRK_NO_IO_RESTRICTION, \
}
static const struct acpi_gpio_params first_gpio = { 0, 0, false };
--
b4 0.10.1
From: Quentin Schulz <[email protected]>
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 <[email protected]>
---
arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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
From: Quentin Schulz <[email protected]>
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 <[email protected]>
---
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 */
touchscreen-inverted-x;
touchscreen-inverted-y;
};
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5205-5inmfd.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5205-5inmfd.dts
index 577f9e1d08a14..990f042f5a5b1 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5205-5inmfd.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5205-5inmfd.dts
@@ -45,7 +45,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 11 GPIO_ACTIVE_HIGH>; /* CTP-RST: PH11 */
+ reset-gpios = <&pio 7 11 GPIO_ACTIVE_LOW>; /* CTP-RST: PH11 */
touchscreen-inverted-x;
touchscreen-inverted-y;
};
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
index 87847116ab6d9..97359cc7f13e2 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
@@ -167,7 +167,7 @@ touchscreen@5d {
interrupt-parent = <&pio>;
interrupts = <7 4 IRQ_TYPE_LEVEL_HIGH>; /* PH4 */
irq-gpios = <&pio 7 4 GPIO_ACTIVE_HIGH>; /* PH4 */
- reset-gpios = <&pio 7 11 GPIO_ACTIVE_HIGH>; /* PH11 */
+ reset-gpios = <&pio 7 11 GPIO_ACTIVE_LOW>; /* PH11 */
AVDD28-supply = <®_ldo_io0>;
VDDIO-supply = <®_ldo_io0>;
touchscreen-size-x = <720>;
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts
index 0a5607f73049e..c0eccc753e3f5 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts
@@ -189,7 +189,7 @@ touchscreen@5d {
interrupt-parent = <&pio>;
interrupts = <7 4 IRQ_TYPE_LEVEL_HIGH>; /* PH4 */
irq-gpios = <&pio 7 4 GPIO_ACTIVE_HIGH>; /* PH4 */
- reset-gpios = <&pio 7 8 GPIO_ACTIVE_HIGH>; /* PH8 */
+ reset-gpios = <&pio 7 8 GPIO_ACTIVE_LOW>; /* PH8 */
AVDD28-supply = <®_ldo_io1>;
};
};
--
b4 0.10.1
From: Quentin Schulz <[email protected]>
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 <[email protected]>
---
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 1639f2f8a31e3..5fff9483640c9 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;
@@ -806,23 +824,30 @@ static int goodix_reset(struct goodix_ts_data *ts)
.quirks = ACPI_GPIO_QUIRK_NO_IO_RESTRICTION, \
}
-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_MAPPING(GOODIX_GPIO_INT_NAME "-gpios", &first_gpio, 1),
- GOODIX_GPIO_MAPPING(GOODIX_GPIO_RST_NAME "-gpios", &second_gpio, 1),
+ GOODIX_GPIO_MAPPING(GOODIX_GPIO_INT_NAME "-gpios", &int_first_gpio, 1),
+ GOODIX_GPIO_MAPPING(GOODIX_GPIO_RST_NAME "-gpios", &rst_second_gpio, 1),
{ },
};
static const struct acpi_gpio_mapping acpi_goodix_int_last_gpios[] = {
- GOODIX_GPIO_MAPPING(GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1),
- GOODIX_GPIO_MAPPING(GOODIX_GPIO_INT_NAME "-gpios", &second_gpio, 1),
+ GOODIX_GPIO_MAPPING(GOODIX_GPIO_RST_NAME "-gpios", &rst_first_gpio, 1),
+ GOODIX_GPIO_MAPPING(GOODIX_GPIO_INT_NAME "-gpios", &int_second_gpio, 1),
{ },
};
static const struct acpi_gpio_mapping acpi_goodix_reset_only_gpios[] = {
- GOODIX_GPIO_MAPPING(GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1),
+ GOODIX_GPIO_MAPPING(GOODIX_GPIO_RST_NAME "-gpios", &rst_first_gpio, 1),
{ },
};
--
b4 0.10.1
From: Quentin Schulz <[email protected]>
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 <[email protected]>
---
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";
--
b4 0.10.1
From: Quentin Schulz <[email protected]>
To prepare for the quirks member to be set for all pins, which would
make the line longer than allowed, let's move all mappings into a macro.
Signed-off-by: Quentin Schulz <[email protected]>
---
drivers/input/touchscreen/goodix.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index c281e49826c23..d73d4272a8ea5 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -797,23 +797,31 @@ static int goodix_reset(struct goodix_ts_data *ts)
}
#ifdef ACPI_GPIO_SUPPORT
+
+#define GOODIX_GPIO_MAPPING(_name, _params, _size) \
+ { \
+ .name = _name, \
+ .data = _params, \
+ .size = _size, \
+ }
+
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_mapping acpi_goodix_int_first_gpios[] = {
- { GOODIX_GPIO_INT_NAME "-gpios", &first_gpio, 1 },
- { GOODIX_GPIO_RST_NAME "-gpios", &second_gpio, 1 },
+ GOODIX_GPIO_MAPPING(GOODIX_GPIO_INT_NAME "-gpios", &first_gpio, 1),
+ GOODIX_GPIO_MAPPING(GOODIX_GPIO_RST_NAME "-gpios", &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_MAPPING(GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1),
+ GOODIX_GPIO_MAPPING(GOODIX_GPIO_INT_NAME "-gpios", &second_gpio, 1),
{ },
};
static const struct acpi_gpio_mapping acpi_goodix_reset_only_gpios[] = {
- { GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1 },
+ GOODIX_GPIO_MAPPING(GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1),
{ },
};
--
b4 0.10.1
Hi Quentin,
On 12/5/22 14:40, Quentin Schulz wrote:
> From: Quentin Schulz <[email protected]>
>
> 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 obviously requires to fix the DT since most
> users had the "incorrect" value in there, it needs to be inverted.
> Note that the v2 highlighted that I was not the only one that got confused since
> PRT8MM board has the "correct" HW representation for this line in DT (which does
> not match what the driver was expecting).
>
> This is marked as RFC because I can neither test ACPI support nor boards I don't
> own. Please test on the boards you have that are impacted by this patchset and
> give your Tested-By.
I have tested this on a x86/ACPI device where we actually need to reset
the controller at boot to get it to work and things still work fine there
after this series.
I've also reviewd patches 1-3 and they look good to me to.
So for patches 1-3 you may add my:
Tested-by: Hans de Goede <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Regards,
Hans
> 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?
>
> Thanks,
> Quentin
>
> To: Bastien Nocera <[email protected]>
> To: Hans de Goede <[email protected]>
> To: Dmitry Torokhov <[email protected]>
> To: Rob Herring <[email protected]>
> To: Krzysztof Kozlowski <[email protected]>
> To: Shawn Guo <[email protected]>
> To: Sascha Hauer <[email protected]>
> To: Pengutronix Kernel Team <[email protected]>
> To: Fabio Estevam <[email protected]>
> To: NXP Linux Team <[email protected]>
> To: Chen-Yu Tsai <[email protected]>
> To: Jernej Skrabec <[email protected]>
> To: Samuel Holland <[email protected]>
> To: Andy Gross <[email protected]>
> To: Bjorn Andersson <[email protected]>
> To: Konrad Dybcio <[email protected]>
> To: Heiko Stuebner <[email protected]>
> To: David Jander <[email protected]>
> To: Angus Ainslie <[email protected]>
> To: Peter Geis <[email protected]>
> To: Michael Riesch <[email protected]>
> To: Konrad Dybcio <[email protected]>
> To: AngeloGioacchino Del Regno <[email protected]>
> To: Guido Günther <[email protected]>
> To: Jagan Teki <[email protected]>
> To: Ondrej Jirman <[email protected]>
> To: Icenowy Zheng <[email protected]>
> To: Aleksei Mamlin <[email protected]>
> To: Lukasz Majewski <[email protected]>
> To: Frieder Schrempf <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Quentin Schulz <[email protected]>
> ---
> Changes in v3:
> - Cc'ing people who contributed to DTS of impacted boards,
> - removed PRT8MM DTS change since it's been reported the polarity is actually
> correct (goes through an inverter), keeping the appropriate folks in Cc though
> since it'd be a good idea to check this patch series anyways,
> - added ACPI_GPIO_QUIRK_NO_IO_RESTRICTION to acpi_gpio_mapping quirks to make
> gpiolib-acpi core respect GPIOD_ASIS flag in gpiod_get,
> - checked schematics of:
> - pinephone: https://files.pine64.org/doc/PinePhone/PinePhone%20v1.2%20Released%20Schematic.pdf
> - pinetab: https://files.pine64.org/doc/PineTab/PineTab%20Schematic%20v1.2-20191125.pdf
> - px30 evb: https://opensource.rock-chips.com/images/d/db/Px30_mini_evb_v10_20180528.pdf
> - rockpro64: https://files.pine64.org/doc/rockpro64/rockpro64_v21-SCH.pdf
> - librem5 devkit: https://source.puri.sm/Librem5/dvk-mx8m-bsb/blob/master/dvk-mx8m-bsb.pdf
>
> All seems to be directly connected to the GPIO on the SoC side, without an
> inverter on the line.
> - Link to v2: https://lore.kernel.org/r/20221103-upstream-goodix-reset-v2-0-2c38fb03a300@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 (9):
> Input: goodix - add macro for gpio mapping
> Input: goodix - make gpiod_get honor GPIOD_ASIS
> Input: goodix - fix reset polarity
> ARM: dts: imx: fix touchscreen reset GPIO polarity
> ARM: dts: sunxi: fix touchscreen reset GPIO polarity on Wexler TAB7200 tablet
> arm64: dts: allwinner: fix touchscreen reset GPIO polarity
> arm64: dts: librem5: fix touchscreen reset GPIO polarity
> arm64: dts: qcom: msm8998-fxtec: 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 +-
> .../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 | 54 ++++++++++++++++++----
> 13 files changed, 56 insertions(+), 22 deletions(-)
> ---
> base-commit: 76dcd734eca23168cb008912c0f69ff408905235
> change-id: 20221103-upstream-goodix-reset-aa1c65994f57
>
> Best regards,
Hi Quentin,
On 12/5/22 07:40, Quentin Schulz wrote:
> From: Quentin Schulz <[email protected]>
>
> 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 <[email protected]>
> ---
> 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.
Regards,
Samuel
> touchscreen-inverted-x;
> touchscreen-inverted-y;
> };
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5205-5inmfd.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5205-5inmfd.dts
> index 577f9e1d08a14..990f042f5a5b1 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5205-5inmfd.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-oceanic-5205-5inmfd.dts
> @@ -45,7 +45,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 11 GPIO_ACTIVE_HIGH>; /* CTP-RST: PH11 */
> + reset-gpios = <&pio 7 11 GPIO_ACTIVE_LOW>; /* CTP-RST: PH11 */
> touchscreen-inverted-x;
> touchscreen-inverted-y;
> };
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> index 87847116ab6d9..97359cc7f13e2 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
> @@ -167,7 +167,7 @@ touchscreen@5d {
> interrupt-parent = <&pio>;
> interrupts = <7 4 IRQ_TYPE_LEVEL_HIGH>; /* PH4 */
> irq-gpios = <&pio 7 4 GPIO_ACTIVE_HIGH>; /* PH4 */
> - reset-gpios = <&pio 7 11 GPIO_ACTIVE_HIGH>; /* PH11 */
> + reset-gpios = <&pio 7 11 GPIO_ACTIVE_LOW>; /* PH11 */
> AVDD28-supply = <®_ldo_io0>;
> VDDIO-supply = <®_ldo_io0>;
> touchscreen-size-x = <720>;
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts
> index 0a5607f73049e..c0eccc753e3f5 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinetab.dts
> @@ -189,7 +189,7 @@ touchscreen@5d {
> interrupt-parent = <&pio>;
> interrupts = <7 4 IRQ_TYPE_LEVEL_HIGH>; /* PH4 */
> irq-gpios = <&pio 7 4 GPIO_ACTIVE_HIGH>; /* PH4 */
> - reset-gpios = <&pio 7 8 GPIO_ACTIVE_HIGH>; /* PH8 */
> + reset-gpios = <&pio 7 8 GPIO_ACTIVE_LOW>; /* PH8 */
> AVDD28-supply = <®_ldo_io1>;
> };
> };
>
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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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.
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:
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
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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
On Mon, 5 Dec 2022 14:40:29 +0100, Quentin Schulz wrote:
> From: Quentin Schulz <[email protected]>
>
> 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,
--
Bjorn Andersson <[email protected]>
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 <[email protected]>
>>
>> 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 <[email protected]>
>>>
>>> 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