Re: [PATCH v2] Input: tsc2007 - use GPIO descriptor

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Fri, Aug 23, 2019 at 09:10:21AM +0200, Linus Walleij wrote:
> This switches the TSC2007 to use a GPIO descriptor to read
> the pendown GPIO line.
> 
> As this will make the gpiolib start to respect polarity
> inversion flags on the GPIO lines, drop the inversion when
> reading the line with gpio_get_value(), fix two offenders
> in the i.MX device trees, and also emphasize the importance
> of marking the polarity right in the device tree bindings.
> 
> Cc: Denis Carikli <denis@eukrea.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: H. Nikolaus Schaller <hns@goldelico.com>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Drop inversion on the GPIO descriptor value, rely on the
>   gpiolib to handle polarity inversion.
> - Comb through device trees, identify two offenders, fix
>   them as part of the patch for a clean cut.
> - Also fix the device tree bindings.
> ---
>  .../bindings/input/touchscreen/tsc2007.txt         |  5 +++--
>  arch/arm/boot/dts/imx35-eukrea-cpuimx35.dtsi       |  3 ++-
>  arch/arm/boot/dts/imx51-eukrea-cpuimx51.dtsi       |  3 ++-

Shawn, Sascha, any objections to these DTS changes?

>  drivers/input/touchscreen/tsc2007.h                |  4 +++-
>  drivers/input/touchscreen/tsc2007_core.c           | 14 +++++++-------
>  5 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> index ed00f61b8c08..b08b54d49699 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> @@ -7,7 +7,8 @@ Required properties:
>  
>  Optional properties:
>  - gpios: the interrupt gpio the chip is connected to (trough the penirq pin).
> -  The penirq pin goes to low when the panel is touched.
> +  The penirq pin goes to low when the panel is touched, so make sure to tag
> +  the GPIO line with GPIO_ACTIVE_LOW.
>    (see GPIO binding[1] for more details).
>  - interrupts: (gpio) interrupt to which the chip is connected
>    (see interrupt binding[0]).
> @@ -31,7 +32,7 @@ Example:
>  			reg = <0x49>;
>  			interrupt-parent = <&gpio4>;
>  			interrupts = <0x0 0x8>;
> -			gpios = <&gpio4 0 0>;
> +			gpios = <&gpio4 0 GPIO_ACTIVE_LOW>;
>  			ti,x-plate-ohms = <180>;
>  		};
>  
> diff --git a/arch/arm/boot/dts/imx35-eukrea-cpuimx35.dtsi b/arch/arm/boot/dts/imx35-eukrea-cpuimx35.dtsi
> index 17bd2a97609a..b8ac60622936 100644
> --- a/arch/arm/boot/dts/imx35-eukrea-cpuimx35.dtsi
> +++ b/arch/arm/boot/dts/imx35-eukrea-cpuimx35.dtsi
> @@ -3,6 +3,7 @@
>   * Copyright 2013 Eukr�a Electromatique <denis@eukrea.com>
>   */
>  
> +#include <dt-bindings/gpio/gpio.h>
>  #include "imx35.dtsi"
>  
>  / {
> @@ -33,7 +34,7 @@
>  
>  	tsc2007: tsc2007@48 {
>  		compatible = "ti,tsc2007";
> -		gpios = <&gpio3 2 0>;
> +		gpios = <&gpio3 2 GPIO_ACTIVE_LOW>;
>  		interrupt-parent = <&gpio3>;
>  		interrupts = <0x2 0x8>;
>  		pinctrl-names = "default";
> diff --git a/arch/arm/boot/dts/imx51-eukrea-cpuimx51.dtsi b/arch/arm/boot/dts/imx51-eukrea-cpuimx51.dtsi
> index c2a929ba8ceb..016d0bc64bdb 100644
> --- a/arch/arm/boot/dts/imx51-eukrea-cpuimx51.dtsi
> +++ b/arch/arm/boot/dts/imx51-eukrea-cpuimx51.dtsi
> @@ -3,6 +3,7 @@
>   * Copyright 2013 Eukr�a Electromatique <denis@eukrea.com>
>   */
>  
> +#include <dt-bindings/gpio/gpio.h>
>  #include "imx51.dtsi"
>  
>  / {
> @@ -33,7 +34,7 @@
>  
>  	tsc2007: tsc2007@49 {
>  		compatible = "ti,tsc2007";
> -		gpios = <&gpio4 0 1>;
> +		gpios = <&gpio4 0 GPIO_ACTIVE_LOW>;
>  		interrupt-parent = <&gpio4>;
>  		interrupts = <0x0 0x8>;
>  		pinctrl-names = "default";
> diff --git a/drivers/input/touchscreen/tsc2007.h b/drivers/input/touchscreen/tsc2007.h
> index 91c60bf6dcaf..0306c38b56c7 100644
> --- a/drivers/input/touchscreen/tsc2007.h
> +++ b/drivers/input/touchscreen/tsc2007.h
> @@ -49,6 +49,8 @@
>  #define READ_X		(ADC_ON_12BIT | TSC2007_MEASURE_X)
>  #define PWRDOWN		(TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
>  
> +struct gpio_desc;
> +
>  struct ts_event {
>  	u16	x;
>  	u16	y;
> @@ -69,7 +71,7 @@ struct tsc2007 {
>  	int			fuzzy;
>  	int			fuzzz;
>  
> -	unsigned int		gpio;
> +	struct gpio_desc	*gpiod;
>  	int			irq;
>  
>  	wait_queue_head_t	wait;
> diff --git a/drivers/input/touchscreen/tsc2007_core.c b/drivers/input/touchscreen/tsc2007_core.c
> index 3b80abfc1eca..0eadd6d86fa0 100644
> --- a/drivers/input/touchscreen/tsc2007_core.c
> +++ b/drivers/input/touchscreen/tsc2007_core.c
> @@ -23,7 +23,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/i2c.h>
>  #include <linux/of_device.h>
> -#include <linux/of_gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/platform_data/tsc2007.h>
>  #include "tsc2007.h"
>  
> @@ -226,7 +226,7 @@ static int tsc2007_get_pendown_state_gpio(struct device *dev)
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct tsc2007 *ts = i2c_get_clientdata(client);
>  
> -	return !gpio_get_value(ts->gpio);
> +	return gpiod_get_value(ts->gpiod);
>  }
>  
>  static int tsc2007_probe_dt(struct i2c_client *client, struct tsc2007 *ts)
> @@ -266,13 +266,13 @@ static int tsc2007_probe_dt(struct i2c_client *client, struct tsc2007 *ts)
>  		return -EINVAL;
>  	}
>  
> -	ts->gpio = of_get_gpio(np, 0);
> -	if (gpio_is_valid(ts->gpio))
> +	ts->gpiod = devm_gpiod_get_optional(&client->dev, NULL, GPIOD_IN);
> +	if (IS_ERR(ts->gpiod))
> +		return PTR_ERR(ts->gpiod);
> +	if (ts->gpiod)
>  		ts->get_pendown_state = tsc2007_get_pendown_state_gpio;
>  	else
> -		dev_warn(&client->dev,
> -			 "GPIO not specified in DT (of_get_gpio returned %d)\n",
> -			 ts->gpio);
> +		dev_warn(&client->dev, "GPIO not specified in DT\n");
>  
>  	return 0;
>  }
> -- 
> 2.21.0
> 

-- 
Dmitry