[PATCH] Input: zforce - make the interrupt GPIO optional

[PATCH] Input: zforce - make the interrupt GPIO optional

[Dirk Behme]

Add support for hardware which uses an I2C Serializer / Deserializer
(SerDes) to communicate with the zFroce touch driver. In this case the
SerDes will be configured as an interrupt controller and the zForce driver
will have no access to poll the GPIO line.

To support this, we add two dedicated new GPIOs in the device tree:
rst-gpio and int-gpio. With the int-gpio being optional, then.

To not break the existing device trees, the index based 'gpios' entries
are still supported, but marked as depreciated.

With this, if the interrupt GPIO is available, either via the old or new
device tree style, the while loop will read and handle the packets as long
as the GPIO indicates that the interrupt is asserted (existing, unchanged
driver behavior).

If the interrupt GPIO isn't available, i.e. not configured via the new
device tree style, we are falling back to one read per ISR invocation
(new behavior to support the SerDes).

Note that the gpiod functions help to handle the optional GPIO:
devm_gpiod_get_index_optional() will return NULL in case the interrupt
GPIO isn't available. And gpiod_get_value_cansleep() does cover this, too,
by returning 0 in this case.

Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
 .../bindings/input/touchscreen/zforce_ts.txt       |  8 ++--
 drivers/input/touchscreen/zforce_ts.c              | 49 +++++++++++++++++++++-
 2 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt b/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
index 80c37df..c6be925 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
@@ -4,12 +4,12 @@ Required properties:
 - compatible: must be "neonode,zforce"
 - reg: I2C address of the chip
 - interrupts: interrupt to which the chip is connected
-- gpios: gpios the chip is connected to
-  first one is the interrupt gpio and second one the reset gpio
+- rst-gpio: reset gpio the chip is connected to
 - x-size: horizontal resolution of touchscreen
 - y-size: vertical resolution of touchscreen
 
 Optional properties:
+- int-gpio : interrupt gpio the chip is connected to
 - vdd-supply: Regulator controlling the controller supply
 
 Example:
@@ -23,8 +23,8 @@ Example:
 			interrupts = <2 0>;
 			vdd-supply = <&reg_zforce_vdd>;
 
-			gpios = <&gpio5 6 0>, /* INT */
-				<&gpio5 9 0>; /* RST */
+			rst-gpio = <&gpio5 9 0>; /* RST */
+			int-gpio = <&gpio5 6 0>; /* INT, optional */
 
 			x-size = <800>;
 			y-size = <600>;
diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
index edf01c3..bf1e944 100644
--- a/drivers/input/touchscreen/zforce_ts.c
+++ b/drivers/input/touchscreen/zforce_ts.c
@@ -494,6 +494,7 @@ static irqreturn_t zforce_irq_thread(int irq, void *dev_id)
 	int ret;
 	u8 payload_buffer[FRAME_MAXSIZE];
 	u8 *payload;
+	int run = 1;
 
 	/*
 	 * When still suspended, return.
@@ -510,7 +511,18 @@ static irqreturn_t zforce_irq_thread(int irq, void *dev_id)
 	if (!ts->suspending && device_may_wakeup(&client->dev))
 		pm_stay_awake(&client->dev);
 
-	while (gpiod_get_value_cansleep(ts->gpio_int)) {
+	while (run) {
+		/*
+		 * Exit the loop if either
+		 * - the optional interrupt GPIO isn't specified
+		 *   (there is only one packet read per ISR invocation, then)
+		 * or
+		 * - the GPIO isn't active any more
+		 *   (packet read until the level GPIO indicates that there is
+		 *    no IRQ any more)
+		 */
+		run = gpiod_get_value_cansleep(ts->gpio_int);
+
 		ret = zforce_read_packet(ts, payload_buffer);
 		if (ret < 0) {
 			dev_err(&client->dev,
@@ -754,6 +766,40 @@ static int zforce_probe(struct i2c_client *client,
 	if (!ts)
 		return -ENOMEM;
 
+	/*
+	 * The reset GPIO isn't optional, but we might get it
+	 * via the old style DT entries below, too. So it's
+	 * not an error if we don't get it here. Therefore use
+	 * devm_gpiod_get_optional() here.
+	 */
+	ts->gpio_rst = devm_gpiod_get_optional(&client->dev, "rst",
+					       GPIOD_OUT_HIGH);
+	if (IS_ERR(ts->gpio_rst)) {
+		ret = PTR_ERR(ts->gpio_rst);
+		dev_err(&client->dev,
+			"failed to request reset GPIO: %d\n", ret);
+		return ret;
+	}
+
+	ts->gpio_int = devm_gpiod_get_optional(&client->dev, "int", GPIOD_IN);
+	if (IS_ERR(ts->gpio_int)) {
+		ret = PTR_ERR(ts->gpio_int);
+		dev_err(&client->dev,
+			"failed to request interrupt GPIO: %d\n", ret);
+		return ret;
+	}
+
+	/* Skip the old style GPIO if we have the new one */
+	if (ts->gpio_rst)
+		goto skip;
+
+	/*
+	 * Depreciated GPIO device tree handling for compatibility
+	 * with existing device trees. For new device trees, don't
+	 * use it any more. Instead use rst-gpio and the optional
+	 * int-gpio above.
+	 */
+
 	/* INT GPIO */
 	ts->gpio_int = devm_gpiod_get_index(&client->dev, NULL, 0, GPIOD_IN);
 	if (IS_ERR(ts->gpio_int)) {
@@ -773,6 +819,7 @@ static int zforce_probe(struct i2c_client *client,
 		return ret;
 	}
 
+skip:
 	ts->reg_vdd = devm_regulator_get_optional(&client->dev, "vdd");
 	if (IS_ERR(ts->reg_vdd)) {
 		ret = PTR_ERR(ts->reg_vdd);
-- 
2.3.4

Re: [PATCH] Input: zforce - make the interrupt GPIO optional

[Dmitry Torokhov]

On Tue, Jul 28, 2015 at 10:26:48AM +0200, Dirk Behme wrote:
> Add support for hardware which uses an I2C Serializer / Deserializer
> (SerDes) to communicate with the zFroce touch driver. In this case the
> SerDes will be configured as an interrupt controller and the zForce driver
> will have no access to poll the GPIO line.
> 
> To support this, we add two dedicated new GPIOs in the device tree:
> rst-gpio and int-gpio. With the int-gpio being optional, then.
> 
> To not break the existing device trees, the index based 'gpios' entries
> are still supported, but marked as depreciated.
> 
> With this, if the interrupt GPIO is available, either via the old or new
> device tree style, the while loop will read and handle the packets as long
> as the GPIO indicates that the interrupt is asserted (existing, unchanged
> driver behavior).
> 
> If the interrupt GPIO isn't available, i.e. not configured via the new
> device tree style, we are falling back to one read per ISR invocation
> (new behavior to support the SerDes).
> 
> Note that the gpiod functions help to handle the optional GPIO:
> devm_gpiod_get_index_optional() will return NULL in case the interrupt
> GPIO isn't available. And gpiod_get_value_cansleep() does cover this, too,
> by returning 0 in this case.

Let's let Heiko take a look as well.

> 
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
>  .../bindings/input/touchscreen/zforce_ts.txt       |  8 ++--
>  drivers/input/touchscreen/zforce_ts.c              | 49 +++++++++++++++++++++-
>  2 files changed, 52 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt b/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> index 80c37df..c6be925 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> @@ -4,12 +4,12 @@ Required properties:
>  - compatible: must be "neonode,zforce"
>  - reg: I2C address of the chip
>  - interrupts: interrupt to which the chip is connected
> -- gpios: gpios the chip is connected to
> -  first one is the interrupt gpio and second one the reset gpio
> +- rst-gpio: reset gpio the chip is connected to
>  - x-size: horizontal resolution of touchscreen
>  - y-size: vertical resolution of touchscreen
>  
>  Optional properties:
> +- int-gpio : interrupt gpio the chip is connected to
>  - vdd-supply: Regulator controlling the controller supply
>  
>  Example:
> @@ -23,8 +23,8 @@ Example:
>  			interrupts = <2 0>;
>  			vdd-supply = <&reg_zforce_vdd>;
>  
> -			gpios = <&gpio5 6 0>, /* INT */
> -				<&gpio5 9 0>; /* RST */
> +			rst-gpio = <&gpio5 9 0>; /* RST */
> +			int-gpio = <&gpio5 6 0>; /* INT, optional */
>  
>  			x-size = <800>;
>  			y-size = <600>;
> diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
> index edf01c3..bf1e944 100644
> --- a/drivers/input/touchscreen/zforce_ts.c
> +++ b/drivers/input/touchscreen/zforce_ts.c
> @@ -494,6 +494,7 @@ static irqreturn_t zforce_irq_thread(int irq, void *dev_id)
>  	int ret;
>  	u8 payload_buffer[FRAME_MAXSIZE];
>  	u8 *payload;
> +	int run = 1;
>  
>  	/*
>  	 * When still suspended, return.
> @@ -510,7 +511,18 @@ static irqreturn_t zforce_irq_thread(int irq, void *dev_id)
>  	if (!ts->suspending && device_may_wakeup(&client->dev))
>  		pm_stay_awake(&client->dev);
>  
> -	while (gpiod_get_value_cansleep(ts->gpio_int)) {
> +	while (run) {
> +		/*
> +		 * Exit the loop if either
> +		 * - the optional interrupt GPIO isn't specified
> +		 *   (there is only one packet read per ISR invocation, then)
> +		 * or
> +		 * - the GPIO isn't active any more
> +		 *   (packet read until the level GPIO indicates that there is
> +		 *    no IRQ any more)
> +		 */
> +		run = gpiod_get_value_cansleep(ts->gpio_int);
> +
>  		ret = zforce_read_packet(ts, payload_buffer);
>  		if (ret < 0) {
>  			dev_err(&client->dev,
> @@ -754,6 +766,40 @@ static int zforce_probe(struct i2c_client *client,
>  	if (!ts)
>  		return -ENOMEM;
>  
> +	/*
> +	 * The reset GPIO isn't optional, but we might get it
> +	 * via the old style DT entries below, too. So it's
> +	 * not an error if we don't get it here. Therefore use
> +	 * devm_gpiod_get_optional() here.
> +	 */
> +	ts->gpio_rst = devm_gpiod_get_optional(&client->dev, "rst",
> +					       GPIOD_OUT_HIGH);
> +	if (IS_ERR(ts->gpio_rst)) {
> +		ret = PTR_ERR(ts->gpio_rst);
> +		dev_err(&client->dev,
> +			"failed to request reset GPIO: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ts->gpio_int = devm_gpiod_get_optional(&client->dev, "int", GPIOD_IN);
> +	if (IS_ERR(ts->gpio_int)) {
> +		ret = PTR_ERR(ts->gpio_int);
> +		dev_err(&client->dev,
> +			"failed to request interrupt GPIO: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Skip the old style GPIO if we have the new one */
> +	if (ts->gpio_rst)
> +		goto skip;
> +
> +	/*
> +	 * Depreciated GPIO device tree handling for compatibility
> +	 * with existing device trees. For new device trees, don't
> +	 * use it any more. Instead use rst-gpio and the optional
> +	 * int-gpio above.
> +	 */
> +
>  	/* INT GPIO */
>  	ts->gpio_int = devm_gpiod_get_index(&client->dev, NULL, 0, GPIOD_IN);
>  	if (IS_ERR(ts->gpio_int)) {
> @@ -773,6 +819,7 @@ static int zforce_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +skip:
>  	ts->reg_vdd = devm_regulator_get_optional(&client->dev, "vdd");
>  	if (IS_ERR(ts->reg_vdd)) {
>  		ret = PTR_ERR(ts->reg_vdd);
> -- 
> 2.3.4
> 

-- 
Dmitry

Re: [PATCH] Input: zforce - make the interrupt GPIO optional

[Heiko Stübner]

Hi,

it's nice to see that my driver seems to be used somewhere :-)


Am Dienstag, 28. Juli 2015, 14:06:21 schrieb Dmitry Torokhov:
> On Tue, Jul 28, 2015 at 10:26:48AM +0200, Dirk Behme wrote:
> > Add support for hardware which uses an I2C Serializer / Deserializer
> > (SerDes) to communicate with the zFroce touch driver. In this case the
> > SerDes will be configured as an interrupt controller and the zForce driver
> > will have no access to poll the GPIO line.
> > To support this, we add two dedicated new GPIOs in the device tree:
> > rst-gpio and int-gpio. With the int-gpio being optional, then.
> > 
> > To not break the existing device trees, the index based 'gpios' entries
> > are still supported, but marked as depreciated.
								^ typo deprecated

> > 
> > With this, if the interrupt GPIO is available, either via the old or new
> > device tree style, the while loop will read and handle the packets as long
> > as the GPIO indicates that the interrupt is asserted (existing, unchanged
> > driver behavior).
> > 
> > If the interrupt GPIO isn't available, i.e. not configured via the new
> > device tree style, we are falling back to one read per ISR invocation
> > (new behavior to support the SerDes).
> > 
> > Note that the gpiod functions help to handle the optional GPIO:
> > devm_gpiod_get_index_optional() will return NULL in case the interrupt
> > GPIO isn't available. And gpiod_get_value_cansleep() does cover this, too,
> > by returning 0 in this case.
> 
> Let's let Heiko take a look as well.
> 
> > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> > ---
> > 
> >  .../bindings/input/touchscreen/zforce_ts.txt       |  8 ++--
> >  drivers/input/touchscreen/zforce_ts.c              | 49
> >  +++++++++++++++++++++- 2 files changed, 52 insertions(+), 5 deletions(-)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> > b/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt index
> > 80c37df..c6be925 100644
> > --- a/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> > 
> > @@ -4,12 +4,12 @@ Required properties:
> >  - compatible: must be "neonode,zforce"
> >  - reg: I2C address of the chip
> >  - interrupts: interrupt to which the chip is connected
> > 
> > -- gpios: gpios the chip is connected to
> > -  first one is the interrupt gpio and second one the reset gpio
> > +- rst-gpio: reset gpio the chip is connected to
> > 
> >  - x-size: horizontal resolution of touchscreen
> >  - y-size: vertical resolution of touchscreen
> > 
> >  Optional properties:
> > +- int-gpio : interrupt gpio the chip is connected to
> > 
> >  - vdd-supply: Regulator controlling the controller supply
> > 
> >  Example:
> > @@ -23,8 +23,8 @@ Example:
> >  			interrupts = <2 0>;
> >  			vdd-supply = <&reg_zforce_vdd>;
> > 
> > -			gpios = <&gpio5 6 0>, /* INT */
> > -				<&gpio5 9 0>; /* RST */
> > +			rst-gpio = <&gpio5 9 0>; /* RST */
> > +			int-gpio = <&gpio5 6 0>; /* INT, optional */

just today in #armlinux I read that it's always supposed to be "x-gpios" even 
if it's only one gpio being defined. The gpio core handles both but only the 
variant with "s" at the end is actually specified, see
Documentation/devicetree/bindings/gpio/gpio.txt

Also I think it's common to not use abbreviation in the devicetree. so it 
probably should be
				reset-gpios =
				irq-gpios =
				

> > 
> >  			x-size = <800>;
> >  			y-size = <600>;
> > 
> > diff --git a/drivers/input/touchscreen/zforce_ts.c
> > b/drivers/input/touchscreen/zforce_ts.c index edf01c3..bf1e944 100644
> > --- a/drivers/input/touchscreen/zforce_ts.c
> > +++ b/drivers/input/touchscreen/zforce_ts.c
> > @@ -494,6 +494,7 @@ static irqreturn_t zforce_irq_thread(int irq, void
> > *dev_id)> 
> >  	int ret;
> >  	u8 payload_buffer[FRAME_MAXSIZE];
> >  	u8 *payload;
> > 
> > +	int run = 1;
> > 
> >  	/*
> >  	
> >  	 * When still suspended, return.
> > 
> > @@ -510,7 +511,18 @@ static irqreturn_t zforce_irq_thread(int irq, void
> > *dev_id)> 
> >  	if (!ts->suspending && device_may_wakeup(&client->dev))
> >  	
> >  		pm_stay_awake(&client->dev);
> > 
> > -	while (gpiod_get_value_cansleep(ts->gpio_int)) {
> > +	while (run) {
> > +		/*
> > +		 * Exit the loop if either
> > +		 * - the optional interrupt GPIO isn't specified
> > +		 *   (there is only one packet read per ISR invocation, then)
> > +		 * or
> > +		 * - the GPIO isn't active any more
> > +		 *   (packet read until the level GPIO indicates that there is
> > +		 *    no IRQ any more)
> > +		 */
> > +		run = gpiod_get_value_cansleep(ts->gpio_int);
> > +

alteratively you could simply convert to a
	/* Run at least once, or as long as the interrupt gpio is active. */
	do {
		...
	} while(gpiod_get_value_cansleep(ts->gpio_int));

saving the additional variable run and a lot of lines, while still running at 
least once.


> > 
> >  		ret = zforce_read_packet(ts, payload_buffer);
> >  		if (ret < 0) {
> >  		
> >  			dev_err(&client->dev,
> > 
> > @@ -754,6 +766,40 @@ static int zforce_probe(struct i2c_client *client,
> > 
> >  	if (!ts)
> >  	
> >  		return -ENOMEM;
> > 
> > +	/*
> > +	 * The reset GPIO isn't optional, but we might get it
> > +	 * via the old style DT entries below, too. So it's
> > +	 * not an error if we don't get it here. Therefore use
> > +	 * devm_gpiod_get_optional() here.
> > +	 */

hmm, you shouldn't mix styles. I.e. the new binding is using reset- and irq-
gpios exclusively. And old devicetrees will use the old binding exclusively.
So if you don't find the reset-gpio here, you can jump to the legacy binding 
directly.


> > +	ts->gpio_rst = devm_gpiod_get_optional(&client->dev, "rst",
> > +					       GPIOD_OUT_HIGH);
> > +	if (IS_ERR(ts->gpio_rst)) {
> > +		ret = PTR_ERR(ts->gpio_rst);
> > +		dev_err(&client->dev,
> > +			"failed to request reset GPIO: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ts->gpio_int = devm_gpiod_get_optional(&client->dev, "int", 
GPIOD_IN);
> > +	if (IS_ERR(ts->gpio_int)) {
> > +		ret = PTR_ERR(ts->gpio_int);
> > +		dev_err(&client->dev,
> > +			"failed to request interrupt GPIO: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Skip the old style GPIO if we have the new one */
> > +	if (ts->gpio_rst)
> > +		goto skip;

personally I would prefer to not introduce non-error-handling gotos. After you 
tried to get the reset gpio you can already decide which paradigm to use, so
could do something like:


	ts->gpio_rst = devm_gpiod_get_optional(&client->dev, "rst",
					       GPIOD_OUT_HIGH);
	if (IS_ERR(ts->gpio_rst)) {
		ret = PTR_ERR(ts->gpio_rst);
		dev_err(&client->dev,
			"failed to request reset GPIO: %d\n", ret);
		return ret;
	}

	if (ts->gpio_rst) {
		ts->gpio_int = devm_gpiod_get_optional(&client->dev, "int",
										GPIOD_IN);
		if (IS_ERR(ts->gpio_int)) {
			ret = PTR_ERR(ts->gpio_int);
			dev_err(&client->dev,
				"failed to request interrupt GPIO: %d\n", ret);
			return ret;
		}
	} else {
		/* Deprecated gpio handling for legacy binding */

		... old code ...
	}


But I guess this is a matter of style and up to what Dmitry prefers.


> > +
> > +	/*
> > +	 * Depreciated GPIO device tree handling for compatibility
			^ Deprecated


> > +	 * with existing device trees. For new device trees, don't
> > +	 * use it any more. Instead use rst-gpio and the optional
> > +	 * int-gpio above.
> > +	 */
> > +

you can skip the second sentence in the comment. People are supposed to read 
the binding docs and will thus only find the new binding there ;-)


> > 
> >  	/* INT GPIO */
> >  	ts->gpio_int = devm_gpiod_get_index(&client->dev, NULL, 0, 
GPIOD_IN);
> >  	if (IS_ERR(ts->gpio_int)) {
> > 
> > @@ -773,6 +819,7 @@ static int zforce_probe(struct i2c_client *client,
> > 
> >  		return ret;
> >  	
> >  	}
> > 
> > +skip:
> >  	ts->reg_vdd = devm_regulator_get_optional(&client->dev, "vdd");
> >  	if (IS_ERR(ts->reg_vdd)) {
> >  	
> >  		ret = PTR_ERR(ts->reg_vdd);


Heiko

Re: [PATCH] Input: zforce - make the interrupt GPIO optional

[Dmitry Torokhov]

On Tue, Jul 28, 2015 at 11:43:20PM +0200, Heiko Stübner wrote:
> Hi,
> 
> it's nice to see that my driver seems to be used somewhere :-)
> 
> 
> Am Dienstag, 28. Juli 2015, 14:06:21 schrieb Dmitry Torokhov:
> > On Tue, Jul 28, 2015 at 10:26:48AM +0200, Dirk Behme wrote:
> > > Add support for hardware which uses an I2C Serializer / Deserializer
> > > (SerDes) to communicate with the zFroce touch driver. In this case the
> > > SerDes will be configured as an interrupt controller and the zForce driver
> > > will have no access to poll the GPIO line.
> > > To support this, we add two dedicated new GPIOs in the device tree:
> > > rst-gpio and int-gpio. With the int-gpio being optional, then.
> > > 
> > > To not break the existing device trees, the index based 'gpios' entries
> > > are still supported, but marked as depreciated.
> 								^ typo deprecated
> 
> > > 
> > > With this, if the interrupt GPIO is available, either via the old or new
> > > device tree style, the while loop will read and handle the packets as long
> > > as the GPIO indicates that the interrupt is asserted (existing, unchanged
> > > driver behavior).
> > > 
> > > If the interrupt GPIO isn't available, i.e. not configured via the new
> > > device tree style, we are falling back to one read per ISR invocation
> > > (new behavior to support the SerDes).
> > > 
> > > Note that the gpiod functions help to handle the optional GPIO:
> > > devm_gpiod_get_index_optional() will return NULL in case the interrupt
> > > GPIO isn't available. And gpiod_get_value_cansleep() does cover this, too,
> > > by returning 0 in this case.
> > 
> > Let's let Heiko take a look as well.
> > 
> > > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> > > ---
> > > 
> > >  .../bindings/input/touchscreen/zforce_ts.txt       |  8 ++--
> > >  drivers/input/touchscreen/zforce_ts.c              | 49
> > >  +++++++++++++++++++++- 2 files changed, 52 insertions(+), 5 deletions(-)
> > > 
> > > diff --git
> > > a/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> > > b/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt index
> > > 80c37df..c6be925 100644
> > > --- a/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> > > +++ b/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> > > 
> > > @@ -4,12 +4,12 @@ Required properties:
> > >  - compatible: must be "neonode,zforce"
> > >  - reg: I2C address of the chip
> > >  - interrupts: interrupt to which the chip is connected
> > > 
> > > -- gpios: gpios the chip is connected to
> > > -  first one is the interrupt gpio and second one the reset gpio
> > > +- rst-gpio: reset gpio the chip is connected to
> > > 
> > >  - x-size: horizontal resolution of touchscreen
> > >  - y-size: vertical resolution of touchscreen
> > > 
> > >  Optional properties:
> > > +- int-gpio : interrupt gpio the chip is connected to
> > > 
> > >  - vdd-supply: Regulator controlling the controller supply
> > > 
> > >  Example:
> > > @@ -23,8 +23,8 @@ Example:
> > >  			interrupts = <2 0>;
> > >  			vdd-supply = <&reg_zforce_vdd>;
> > > 
> > > -			gpios = <&gpio5 6 0>, /* INT */
> > > -				<&gpio5 9 0>; /* RST */
> > > +			rst-gpio = <&gpio5 9 0>; /* RST */
> > > +			int-gpio = <&gpio5 6 0>; /* INT, optional */
> 
> just today in #armlinux I read that it's always supposed to be "x-gpios" even 
> if it's only one gpio being defined. The gpio core handles both but only the 
> variant with "s" at the end is actually specified, see
> Documentation/devicetree/bindings/gpio/gpio.txt
> 
> Also I think it's common to not use abbreviation in the devicetree. so it 
> probably should be
> 				reset-gpios =
> 				irq-gpios =

Agree.

> 				
> 
> > > 
> > >  			x-size = <800>;
> > >  			y-size = <600>;
> > > 
> > > diff --git a/drivers/input/touchscreen/zforce_ts.c
> > > b/drivers/input/touchscreen/zforce_ts.c index edf01c3..bf1e944 100644
> > > --- a/drivers/input/touchscreen/zforce_ts.c
> > > +++ b/drivers/input/touchscreen/zforce_ts.c
> > > @@ -494,6 +494,7 @@ static irqreturn_t zforce_irq_thread(int irq, void
> > > *dev_id)> 
> > >  	int ret;
> > >  	u8 payload_buffer[FRAME_MAXSIZE];
> > >  	u8 *payload;
> > > 
> > > +	int run = 1;
> > > 
> > >  	/*
> > >  	
> > >  	 * When still suspended, return.
> > > 
> > > @@ -510,7 +511,18 @@ static irqreturn_t zforce_irq_thread(int irq, void
> > > *dev_id)> 
> > >  	if (!ts->suspending && device_may_wakeup(&client->dev))
> > >  	
> > >  		pm_stay_awake(&client->dev);
> > > 
> > > -	while (gpiod_get_value_cansleep(ts->gpio_int)) {
> > > +	while (run) {
> > > +		/*
> > > +		 * Exit the loop if either
> > > +		 * - the optional interrupt GPIO isn't specified
> > > +		 *   (there is only one packet read per ISR invocation, then)
> > > +		 * or
> > > +		 * - the GPIO isn't active any more
> > > +		 *   (packet read until the level GPIO indicates that there is
> > > +		 *    no IRQ any more)
> > > +		 */
> > > +		run = gpiod_get_value_cansleep(ts->gpio_int);
> > > +
> 
> alteratively you could simply convert to a
> 	/* Run at least once, or as long as the interrupt gpio is active. */
> 	do {
> 		...
> 	} while(gpiod_get_value_cansleep(ts->gpio_int));
> 
> saving the additional variable run and a lot of lines, while still running at 
> least once.

But I think that means that we'll try to read even if gpio is not active
anymore on the first iteration. So maybe we need to factor out the body
and do:

	if (!ts->gpio_int)
		zforce_report_data(...)
	else
		while (gpiod_get_value_cansleep(ts->gpio_int))
			zforce_report_data(...);

Or we could install 2 different interrupt handlers, depending on the
presence of interrupt gpio.

> 
> 
> > > 
> > >  		ret = zforce_read_packet(ts, payload_buffer);
> > >  		if (ret < 0) {
> > >  		
> > >  			dev_err(&client->dev,
> > > 
> > > @@ -754,6 +766,40 @@ static int zforce_probe(struct i2c_client *client,
> > > 
> > >  	if (!ts)
> > >  	
> > >  		return -ENOMEM;
> > > 
> > > +	/*
> > > +	 * The reset GPIO isn't optional, but we might get it
> > > +	 * via the old style DT entries below, too. So it's
> > > +	 * not an error if we don't get it here. Therefore use
> > > +	 * devm_gpiod_get_optional() here.
> > > +	 */
> 
> hmm, you shouldn't mix styles. I.e. the new binding is using reset- and irq-
> gpios exclusively. And old devicetrees will use the old binding exclusively.
> So if you don't find the reset-gpio here, you can jump to the legacy binding 
> directly.
> 
> 
> > > +	ts->gpio_rst = devm_gpiod_get_optional(&client->dev, "rst",
> > > +					       GPIOD_OUT_HIGH);
> > > +	if (IS_ERR(ts->gpio_rst)) {
> > > +		ret = PTR_ERR(ts->gpio_rst);
> > > +		dev_err(&client->dev,
> > > +			"failed to request reset GPIO: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	ts->gpio_int = devm_gpiod_get_optional(&client->dev, "int", 
> GPIOD_IN);
> > > +	if (IS_ERR(ts->gpio_int)) {
> > > +		ret = PTR_ERR(ts->gpio_int);
> > > +		dev_err(&client->dev,
> > > +			"failed to request interrupt GPIO: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	/* Skip the old style GPIO if we have the new one */
> > > +	if (ts->gpio_rst)
> > > +		goto skip;
> 
> personally I would prefer to not introduce non-error-handling gotos. After you 
> tried to get the reset gpio you can already decide which paradigm to use, so
> could do something like:
> 
> 
> 	ts->gpio_rst = devm_gpiod_get_optional(&client->dev, "rst",
> 					       GPIOD_OUT_HIGH);
> 	if (IS_ERR(ts->gpio_rst)) {
> 		ret = PTR_ERR(ts->gpio_rst);
> 		dev_err(&client->dev,
> 			"failed to request reset GPIO: %d\n", ret);
> 		return ret;
> 	}
> 
> 	if (ts->gpio_rst) {
> 		ts->gpio_int = devm_gpiod_get_optional(&client->dev, "int",
> 										GPIOD_IN);
> 		if (IS_ERR(ts->gpio_int)) {
> 			ret = PTR_ERR(ts->gpio_int);
> 			dev_err(&client->dev,
> 				"failed to request interrupt GPIO: %d\n", ret);
> 			return ret;
> 		}
> 	} else {
> 		/* Deprecated gpio handling for legacy binding */
> 
> 		... old code ...
> 	}
> 
> 
> But I guess this is a matter of style and up to what Dmitry prefers.

Yes, I'd prefer this too, although I can see why Dirk used label as it
reduces the size of the patch considerably.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: zforce - make the interrupt GPIO optional

[Heiko Stübner]

Hi Dmitry,

Am Mittwoch, 29. Juli 2015, 10:53:04 schrieb Dmitry Torokhov:
> On Tue, Jul 28, 2015 at 11:43:20PM +0200, Heiko Stübner wrote:

[snip]

> > > > @@ -510,7 +511,18 @@ static irqreturn_t zforce_irq_thread(int irq,
> > > > void
> > > > *dev_id)>
> > > > 
> > > >  	if (!ts->suspending && device_may_wakeup(&client->dev))
> > > >  	
> > > >  		pm_stay_awake(&client->dev);
> > > > 
> > > > -	while (gpiod_get_value_cansleep(ts->gpio_int)) {
> > > > +	while (run) {
> > > > +		/*
> > > > +		 * Exit the loop if either
> > > > +		 * - the optional interrupt GPIO isn't specified
> > > > +		 *   (there is only one packet read per ISR invocation, 
then)
> > > > +		 * or
> > > > +		 * - the GPIO isn't active any more
> > > > +		 *   (packet read until the level GPIO indicates that there 
is
> > > > +		 *    no IRQ any more)
> > > > +		 */
> > > > +		run = gpiod_get_value_cansleep(ts->gpio_int);
> > > > +
> > 
> > alteratively you could simply convert to a
> > 
> > 	/* Run at least once, or as long as the interrupt gpio is active. */
> > 	do {
> > 	
> > 		...
> > 	
> > 	} while(gpiod_get_value_cansleep(ts->gpio_int));
> > 
> > saving the additional variable run and a lot of lines, while still running
> > at least once.
> 
> But I think that means that we'll try to read even if gpio is not active
> anymore on the first iteration.

I don't think this is a possible scenario :-)

The interrupt is only used for transmission of touch-data and is pulled low as 
long as the host hasn't fetched all touch-packages. So if nothing else except 
the zforce-driver talks to the chip, the gpio will stay on.

And when we're entering the interrupt handler, there is at least one touch-
data package waiting.


> So maybe we need to factor out the body
> and do:
> 
> 	if (!ts->gpio_int)
> 		zforce_report_data(...)
> 	else
> 		while (gpiod_get_value_cansleep(ts->gpio_int))
> 			zforce_report_data(...);
> 
> Or we could install 2 different interrupt handlers, depending on the
> presence of interrupt gpio.


Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: zforce - make the interrupt GPIO optional

[Dmitry Torokhov]

On Wed, Jul 29, 2015 at 08:17:10PM +0200, Heiko Stübner wrote:
> Hi Dmitry,
> 
> Am Mittwoch, 29. Juli 2015, 10:53:04 schrieb Dmitry Torokhov:
> > On Tue, Jul 28, 2015 at 11:43:20PM +0200, Heiko Stübner wrote:
> 
> [snip]
> 
> > > > > @@ -510,7 +511,18 @@ static irqreturn_t zforce_irq_thread(int irq,
> > > > > void
> > > > > *dev_id)>
> > > > > 
> > > > >  	if (!ts->suspending && device_may_wakeup(&client->dev))
> > > > >  	
> > > > >  		pm_stay_awake(&client->dev);
> > > > > 
> > > > > -	while (gpiod_get_value_cansleep(ts->gpio_int)) {
> > > > > +	while (run) {
> > > > > +		/*
> > > > > +		 * Exit the loop if either
> > > > > +		 * - the optional interrupt GPIO isn't specified
> > > > > +		 *   (there is only one packet read per ISR invocation, 
> then)
> > > > > +		 * or
> > > > > +		 * - the GPIO isn't active any more
> > > > > +		 *   (packet read until the level GPIO indicates that there 
> is
> > > > > +		 *    no IRQ any more)
> > > > > +		 */
> > > > > +		run = gpiod_get_value_cansleep(ts->gpio_int);
> > > > > +
> > > 
> > > alteratively you could simply convert to a
> > > 
> > > 	/* Run at least once, or as long as the interrupt gpio is active. */
> > > 	do {
> > > 	
> > > 		...
> > > 	
> > > 	} while(gpiod_get_value_cansleep(ts->gpio_int));
> > > 
> > > saving the additional variable run and a lot of lines, while still running
> > > at least once.
> > 
> > But I think that means that we'll try to read even if gpio is not active
> > anymore on the first iteration.
> 
> I don't think this is a possible scenario :-)
> 
> The interrupt is only used for transmission of touch-data and is pulled low as 
> long as the host hasn't fetched all touch-packages. So if nothing else except 
> the zforce-driver talks to the chip, the gpio will stay on.
> 
> And when we're entering the interrupt handler, there is at least one touch-
> data package waiting.

I see. Then please ignore me ;)

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html