[PATCH] Input: egalax_ts - do not release gpio if probe successful

Thus preventing anyone to later modify the interrupt GPIO direction
and/or state without the driver knowing.

Also checking if device is present before allocating the input device.

Signed-off-by: Gary Bisson <redacted>
---
 drivers/input/touchscreen/egalax_ts.c | 56 ++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 31 deletions(-)

diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c
index 1afc08b08155..f6b94bb19bd8 100644
--- a/drivers/input/touchscreen/egalax_ts.c
+++ b/drivers/input/touchscreen/egalax_ts.c

@@ -62,6 +62,7 @@
 struct egalax_ts {
 	struct i2c_client		*client;
 	struct input_dev		*input_dev;
+	struct gpio_desc		*wakeup_gpio;
 };
 
 static irqreturn_t egalax_ts_interrupt(int irq, void *dev_id)

@@ -120,36 +121,21 @@ static irqreturn_t egalax_ts_interrupt(int irq, void *dev_id)
 }
 
 /* wake up controller by an falling edge of interrupt gpio.  */
-static int egalax_wake_up_device(struct i2c_client *client)
+static int egalax_wake_up_device(struct gpio_desc *wakeup_gpio)
 {
-	struct device_node *np = client->dev.of_node;
-	int gpio;
 	int ret;
 
-	if (!np)
-		return -ENODEV;
-
-	gpio = of_get_named_gpio(np, "wakeup-gpios", 0);
-	if (!gpio_is_valid(gpio))
-		return -ENODEV;
-
-	ret = gpio_request(gpio, "egalax_irq");
-	if (ret < 0) {
-		dev_err(&client->dev,
-			"request gpio failed, cannot wake up controller: %d\n",
-			ret);
+	/* wake up controller via an falling edge on IRQ gpio. */
+	ret = gpiod_direction_output(wakeup_gpio, 0);
+	if (ret < 0)
 		return ret;
-	}
 
-	/* wake up controller via an falling edge on IRQ gpio. */
-	gpio_direction_output(gpio, 0);
-	gpio_set_value(gpio, 1);
+	gpiod_set_value(wakeup_gpio, 1);
 
 	/* controller should be waken up, return irq.  */
-	gpio_direction_input(gpio);
-	gpio_free(gpio);
+	ret = gpiod_direction_input(wakeup_gpio);
 
-	return 0;
+	return ret;
 }
 
 static int egalax_firmware_version(struct i2c_client *client)

@@ -177,17 +163,15 @@ static int egalax_ts_probe(struct i2c_client *client,
 		return -ENOMEM;
 	}
 
-	input_dev = devm_input_allocate_device(&client->dev);
-	if (!input_dev) {
-		dev_err(&client->dev, "Failed to allocate memory\n");
-		return -ENOMEM;
+	ts->wakeup_gpio = devm_gpiod_get_index(&client->dev, "wakeup",
+					       0, GPIOD_ASIS);
+	if (IS_ERR(ts->wakeup_gpio)) {
+		dev_err(&client->dev, "Failed to get wakeup gpio");
+		return -ENODEV;
 	}
 
-	ts->client = client;
-	ts->input_dev = input_dev;
-
 	/* controller may be in sleep, wake it up. */
-	error = egalax_wake_up_device(client);
+	error = egalax_wake_up_device(ts->wakeup_gpio);
 	if (error) {
 		dev_err(&client->dev, "Failed to wake up the controller\n");
 		return error;

@@ -199,6 +183,15 @@ static int egalax_ts_probe(struct i2c_client *client,
 		return error;
 	}
 
+	input_dev = devm_input_allocate_device(&client->dev);
+	if (!input_dev) {
+		dev_err(&client->dev, "Failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	ts->client = client;
+	ts->input_dev = input_dev;
+
 	input_dev->name = "EETI eGalax Touch Screen";
 	input_dev->id.bustype = BUS_I2C;
 

@@ -254,8 +247,9 @@ static int __maybe_unused egalax_ts_suspend(struct device *dev)
 static int __maybe_unused egalax_ts_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	struct egalax_ts *ts = i2c_get_clientdata(client);
 
-	return egalax_wake_up_device(client);
+	return egalax_wake_up_device(ts->wakeup_gpio);
 }
 
 static SIMPLE_DEV_PM_OPS(egalax_ts_pm_ops, egalax_ts_suspend, egalax_ts_resume);

-- 
2.11.0

Hi Gary,

On Wed, Jan 11, 2017 at 06:28:41PM +0100, Gary Bisson wrote:

Thus preventing anyone to later modify the interrupt GPIO direction
and/or state without the driver knowing.

I am afraid not releasing gpio after waking up the controller will cause
request_irq to fail if we are using the same pin for interrupt and
wakeup (as majority of current DTSes do: see
arch/arm/boot/dts/imx53-tx53-x13x.dts for example).

You'll need to figure out if irq is backed by the same gpio as wakeup
and act accordingly.

What setup did you test this on? Was it shared pin or dedicated wakeup?

Thanks.

-- 
Dmitry

Hi Dmitri,

On Thu, Jan 12, 2017 at 10:30:19AM -0800, Dmitry Torokhov wrote:

Hi Gary,

On Wed, Jan 11, 2017 at 06:28:41PM +0100, Gary Bisson wrote:

quoted

Thus preventing anyone to later modify the interrupt GPIO direction
and/or state without the driver knowing.

I am afraid not releasing gpio after waking up the controller will cause
request_irq to fail if we are using the same pin for interrupt and
wakeup (as majority of current DTSes do: see
arch/arm/boot/dts/imx53-tx53-x13x.dts for example).

No, keeping the GPIO doesn't prevent from requesting the IRQ.

However it keeps other drivers/users from changing the GPIO as output
later on.

You'll need to figure out if irq is backed by the same gpio as wakeup
and act accordingly.

What setup did you test this on? Was it shared pin or dedicated wakeup?

I've tested on i.MX6Q Nitrogen6x [1] with Hannstar 10" display [2].

Regards,
Gary

[1] https://boundarydevices.com/product/nitrogen6x-board-imx6-arm-cortex-a9-sbc/
[2] https://boundarydevices.com/product/nit6x_10-1hannstar/

On Fri, Jan 13, 2017 at 12:03:43AM +0100, Gary Bisson wrote:

Hi Dmitri,

On Thu, Jan 12, 2017 at 10:30:19AM -0800, Dmitry Torokhov wrote:

quoted

Hi Gary,

On Wed, Jan 11, 2017 at 06:28:41PM +0100, Gary Bisson wrote:

quoted

Thus preventing anyone to later modify the interrupt GPIO direction
and/or state without the driver knowing.

I am afraid not releasing gpio after waking up the controller will cause
request_irq to fail if we are using the same pin for interrupt and
wakeup (as majority of current DTSes do: see
arch/arm/boot/dts/imx53-tx53-x13x.dts for example).

No, keeping the GPIO doesn't prevent from requesting the IRQ.

However it keeps other drivers/users from changing the GPIO as output
later on.

Hmm, I think _gpiod_direction_output_raw() will not let them as it
checks FLAG_USED_AS_IRQ, so as long as it's the same line it's OK. But I
guess if they are separate some other component might try to grab it and
mess with it.

OK, in this case please:

- use devm_gpiod_get
- request with GPIOD_OUT_LOW
- do not override the return value with -ENODEV (especially important
  with probe deferrals)
- lose gpiod_direction_output() call
- move everything into egalax_ts_probe() as egalax_wake_up_device() is
  no longer self-contained. 

Thanks!

-- 
Dmitry

Hi Dmitri,

On Fri, Jan 13, 2017 at 12:40 AM, Dmitry Torokhov
[off-list ref] wrote:

On Fri, Jan 13, 2017 at 12:03:43AM +0100, Gary Bisson wrote:

quoted

Hi Dmitri,

On Thu, Jan 12, 2017 at 10:30:19AM -0800, Dmitry Torokhov wrote:

quoted

Hi Gary,

On Wed, Jan 11, 2017 at 06:28:41PM +0100, Gary Bisson wrote:

quoted

Thus preventing anyone to later modify the interrupt GPIO direction
and/or state without the driver knowing.

I am afraid not releasing gpio after waking up the controller will cause
request_irq to fail if we are using the same pin for interrupt and
wakeup (as majority of current DTSes do: see
arch/arm/boot/dts/imx53-tx53-x13x.dts for example).

No, keeping the GPIO doesn't prevent from requesting the IRQ.

However it keeps other drivers/users from changing the GPIO as output
later on.

Hmm, I think _gpiod_direction_output_raw() will not let them as it
checks FLAG_USED_AS_IRQ, so as long as it's the same line it's OK. But I
guess if they are separate some other component might try to grab it and
mess with it.

OK, in this case please:

- use devm_gpiod_get
- request with GPIOD_OUT_LOW
- do not override the return value with -ENODEV (especially important
  with probe deferrals)
- lose gpiod_direction_output() call
- move everything into egalax_ts_probe() as egalax_wake_up_device() is
  no longer self-contained.

Thanks for reviewing. I'll try to do the v2 this week.

Regards,
Gary