Re: [PATCH 1/3 v2] Input: atmel_mxt_ts - Fix up inverted RESET handler

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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);
>  	}
>  




[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux for Synopsys ARC Processors]    
  • [Linux on Unisoc (RDA Micro) SoCs]     [Linux Actions SoC]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  •   Powered by Linux