On Wed, 2020-11-04 at 16:30 +0100, Linus Walleij wrote:
> This driver uses GPIO descriptors to drive the touchscreen
> RESET line. In the existing device trees this has in
> conflict with intution been flagged as GPIO_ACTIVE_HIGH
> and the driver then applies the reverse action by
> driving the line low (setting to 0) to enter
> reset state and driving the line high (setting to 1) to
> get out of reset state.
>
> The correct way to handle active low GPIO lines is to
> provide the GPIO_ACTIVE_LOW in the device tree (thus
> properly describing the hardware) and letting the GPIO
> framework invert the assertion (driving high) to a low
> level and vice versa.
>
> This is considered a bug since the device trees are
> incorrectly mis-specifying the line as active high.
>
> Fix the driver and all device trees specifying a reset
> line.
>
> Cc: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: linux-tegra@xxxxxxxxxxxxxxx
> Cc: linux-samsung-soc@xxxxxxxxxxxxxxx
> Cc: NXP Linux Team <linux-imx@xxxxxxx>
> Cc: Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
Thanks for fixing this!
Reviewed-by: Philippe Schenker <philippe.schenker@xxxxxxxxxxx>
> ---
> ChangeLog v1->v2:
> - New patch fixing this confusion before adding the
> new YAML bindings.
> - CC some misc maintainers and mailing lists that should
> be aware that we do this change.
> ---
> arch/arm/boot/dts/imx53-ppd.dts | 2 +-
> arch/arm/boot/dts/imx6dl-colibri-eval-v3.dts | 2 +-
> arch/arm/boot/dts/imx6q-apalis-eval.dts | 2 +-
> arch/arm/boot/dts/imx6q-apalis-ixora-v1.1.dts | 2 +-
> arch/arm/boot/dts/imx6q-apalis-ixora.dts | 2 +-
> arch/arm/boot/dts/imx7-colibri-aster.dtsi | 2 +-
> arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi | 2 +-
> arch/arm/boot/dts/motorola-mapphone-common.dtsi | 2 +-
> arch/arm/boot/dts/s5pv210-aries.dtsi | 2 +-
> arch/arm/boot/dts/tegra20-acer-a500-picasso.dts | 2 +-
> drivers/input/touchscreen/atmel_mxt_ts.c | 6 ++++--
> 11 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx53-ppd.dts
> b/arch/arm/boot/dts/imx53-ppd.dts
> index f7dcdf96e5c0..8f4a63ea912e 100644
> --- a/arch/arm/boot/dts/imx53-ppd.dts
> +++ b/arch/arm/boot/dts/imx53-ppd.dts
> @@ -589,7 +589,7 @@ &i2c2 {
>
> touchscreen@4b {
> compatible = "atmel,maxtouch";
> - reset-gpio = <&gpio5 19 GPIO_ACTIVE_HIGH>;
> + reset-gpio = <&gpio5 19 GPIO_ACTIVE_LOW>;
> reg = <0x4b>;
> interrupt-parent = <&gpio5>;
> interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
> diff --git a/arch/arm/boot/dts/imx6dl-colibri-eval-v3.dts
> b/arch/arm/boot/dts/imx6dl-colibri-eval-v3.dts
> index 65359aece950..7da74e6f46d9 100644
> --- a/arch/arm/boot/dts/imx6dl-colibri-eval-v3.dts
> +++ b/arch/arm/boot/dts/imx6dl-colibri-eval-v3.dts
> @@ -143,7 +143,7 @@ touchscreen@4a {
> reg = <0x4a>;
> interrupt-parent = <&gpio1>;
> interrupts = <9 IRQ_TYPE_EDGE_FALLING>; /*
> SODIMM 28 */
> - reset-gpios = <&gpio2 10 GPIO_ACTIVE_HIGH>; /*
> SODIMM 30 */
> + reset-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>; /*
> SODIMM 30 */
> status = "disabled";
> };
>
> diff --git a/arch/arm/boot/dts/imx6q-apalis-eval.dts
> b/arch/arm/boot/dts/imx6q-apalis-eval.dts
> index fab83abb6466..a0683b4aeca1 100644
> --- a/arch/arm/boot/dts/imx6q-apalis-eval.dts
> +++ b/arch/arm/boot/dts/imx6q-apalis-eval.dts
> @@ -140,7 +140,7 @@ touchscreen@4a {
> reg = <0x4a>;
> interrupt-parent = <&gpio6>;
> interrupts = <10 IRQ_TYPE_EDGE_FALLING>;
> - reset-gpios = <&gpio6 9 GPIO_ACTIVE_HIGH>; /* SODIMM 13
> */
> + reset-gpios = <&gpio6 9 GPIO_ACTIVE_LOW>; /* SODIMM 13
> */
> status = "disabled";
> };
>
> diff --git a/arch/arm/boot/dts/imx6q-apalis-ixora-v1.1.dts
> b/arch/arm/boot/dts/imx6q-apalis-ixora-v1.1.dts
> index 1614b1ae501d..86e84781cf5d 100644
> --- a/arch/arm/boot/dts/imx6q-apalis-ixora-v1.1.dts
> +++ b/arch/arm/boot/dts/imx6q-apalis-ixora-v1.1.dts
> @@ -145,7 +145,7 @@ touchscreen@4a {
> reg = <0x4a>;
> interrupt-parent = <&gpio6>;
> interrupts = <10 IRQ_TYPE_EDGE_FALLING>;
> - reset-gpios = <&gpio6 9 GPIO_ACTIVE_HIGH>; /* SODIMM 13
> */
> + reset-gpios = <&gpio6 9 GPIO_ACTIVE_LOW>; /* SODIMM 13
> */
> status = "disabled";
> };
>
> diff --git a/arch/arm/boot/dts/imx6q-apalis-ixora.dts
> b/arch/arm/boot/dts/imx6q-apalis-ixora.dts
> index fa9f98dd15ac..62e72773e53b 100644
> --- a/arch/arm/boot/dts/imx6q-apalis-ixora.dts
> +++ b/arch/arm/boot/dts/imx6q-apalis-ixora.dts
> @@ -144,7 +144,7 @@ touchscreen@4a {
> reg = <0x4a>;
> interrupt-parent = <&gpio6>;
> interrupts = <10 IRQ_TYPE_EDGE_FALLING>;
> - reset-gpios = <&gpio6 9 GPIO_ACTIVE_HIGH>; /* SODIMM 13
> */
> + reset-gpios = <&gpio6 9 GPIO_ACTIVE_LOW>; /* SODIMM 13
> */
> status = "disabled";
> };
>
> diff --git a/arch/arm/boot/dts/imx7-colibri-aster.dtsi
> b/arch/arm/boot/dts/imx7-colibri-aster.dtsi
> index 9fa701bec2ec..139188eb9f40 100644
> --- a/arch/arm/boot/dts/imx7-colibri-aster.dtsi
> +++ b/arch/arm/boot/dts/imx7-colibri-aster.dtsi
> @@ -99,7 +99,7 @@ touchscreen@4a {
> reg = <0x4a>;
> interrupt-parent = <&gpio2>;
> interrupts = <15 IRQ_TYPE_EDGE_FALLING>; /* SODIMM 107
> */
> - reset-gpios = <&gpio2 28 GPIO_ACTIVE_HIGH>; /*
> SODIMM 106 */
> + reset-gpios = <&gpio2 28 GPIO_ACTIVE_LOW>; /*
> SODIMM 106 */
> };
>
> /* M41T0M6 real time clock on carrier board */
> diff --git a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> index 97601375f264..3caf450735d7 100644
> --- a/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> +++ b/arch/arm/boot/dts/imx7-colibri-eval-v3.dtsi
> @@ -124,7 +124,7 @@ touchscreen@4a {
> reg = <0x4a>;
> interrupt-parent = <&gpio1>;
> interrupts = <9 IRQ_TYPE_EDGE_FALLING>; /*
> SODIMM 28 */
> - reset-gpios = <&gpio1 10 GPIO_ACTIVE_HIGH>; /*
> SODIMM 30 */
> + reset-gpios = <&gpio1 10 GPIO_ACTIVE_LOW>; /*
> SODIMM 30 */
> status = "disabled";
> };
>
> diff --git a/arch/arm/boot/dts/motorola-mapphone-common.dtsi
> b/arch/arm/boot/dts/motorola-mapphone-common.dtsi
> index d5ded4f794df..5f8f77cfbe59 100644
> --- a/arch/arm/boot/dts/motorola-mapphone-common.dtsi
> +++ b/arch/arm/boot/dts/motorola-mapphone-common.dtsi
> @@ -430,7 +430,7 @@ touchscreen@4a {
> pinctrl-names = "default";
> pinctrl-0 = <&touchscreen_pins>;
>
> - reset-gpios = <&gpio6 13 GPIO_ACTIVE_HIGH>; /* gpio173
> */
> + reset-gpios = <&gpio6 13 GPIO_ACTIVE_LOW>; /* gpio173 */
>
> /* gpio_183 with sys_nirq2 pad as wakeup */
> interrupts-extended = <&gpio6 23 IRQ_TYPE_LEVEL_LOW>,
> diff --git a/arch/arm/boot/dts/s5pv210-aries.dtsi
> b/arch/arm/boot/dts/s5pv210-aries.dtsi
> index bd4450dbdcb6..4da33d0f2748 100644
> --- a/arch/arm/boot/dts/s5pv210-aries.dtsi
> +++ b/arch/arm/boot/dts/s5pv210-aries.dtsi
> @@ -632,7 +632,7 @@ touchscreen@4a {
> interrupts = <5 IRQ_TYPE_EDGE_FALLING>;
> pinctrl-names = "default";
> pinctrl-0 = <&ts_irq>;
> - reset-gpios = <&gpj1 3 GPIO_ACTIVE_HIGH>;
> + reset-gpios = <&gpj1 3 GPIO_ACTIVE_LOW>;
> };
> };
>
> diff --git a/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
> b/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
> index a0b829738e8f..10794a870776 100644
> --- a/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
> +++ b/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
> @@ -446,7 +446,7 @@ touchscreen@4c {
> interrupt-parent = <&gpio>;
> interrupts = <TEGRA_GPIO(V, 6)
> IRQ_TYPE_LEVEL_LOW>;
>
> - reset-gpios = <&gpio TEGRA_GPIO(Q, 7)
> GPIO_ACTIVE_HIGH>;
> + reset-gpios = <&gpio TEGRA_GPIO(Q, 7)
> GPIO_ACTIVE_LOW>;
>
> avdd-supply = <&vdd_3v3_sys>;
> vdd-supply = <&vdd_3v3_sys>;
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c
> b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 98f17fa3a892..ef7915400c9f 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -3134,8 +3134,9 @@ static int mxt_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> if (error)
> return error;
>
> + /* Request the RESET line as asserted so we go into reset */
> data->reset_gpio = devm_gpiod_get_optional(&client->dev,
> - "reset",
> GPIOD_OUT_LOW);
> + "reset",
> GPIOD_OUT_HIGH);
> if (IS_ERR(data->reset_gpio)) {
> error = PTR_ERR(data->reset_gpio);
> dev_err(&client->dev, "Failed to get reset gpio: %d\n",
> error);
> @@ -3153,8 +3154,9 @@ static int mxt_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> disable_irq(client->irq);
>
> if (data->reset_gpio) {
> + /* Wait a while and then de-assert the RESET GPIO line
> */
> msleep(MXT_RESET_GPIO_TIME);
> - gpiod_set_value(data->reset_gpio, 1);
> + gpiod_set_value(data->reset_gpio, 0);
> msleep(MXT_RESET_INVALID_CHG);
> }
>
