Input: ads7846: use gpio_request_one to configure pendown_gpio

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Quoted 6 lines · Sourav Poddar

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Will apply this one, the other one is a bit messy IMO, will have to think about it.

Quoted 5 lines · Dmitry Torokhov

Quoted 4 lines · Sourav Poddar

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Quoted 6 lines

Quoted 2 lines

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Will apply this one, the other one is a bit messy IMO, will have to think about it.

Something like below should do I think.

Quoted 11 lines · Dmitry Torokhov

Quoted 6 lines · Sourav Poddar

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Will apply this one, the other one is a bit messy IMO, will have to think about it.

Something like below should do I think.

--
Dmitry

Input: ads7846 - check proper condition when freeing gpio

From: Dmitry Torokhov <[email protected]>

When driver uses custom pendown detection method gpio_pendown is not set up and so we should not try to free it, otherwise we are presented with:

------------[ cut here ]------------
WARNING: at drivers/gpio/gpiolib.c:1258 gpio_free+0x100/0x12c()
Modules linked in:
[<c0061208>] (unwind_backtrace+0x0/0xe4) from [<c0091f58>](warn_slowpath_common+0x4c/0x64)
[<c0091f58>] (warn_slowpath_common+0x4c/0x64) from [<c0091f88>](warn_slowpath_null+0x18/0x1c)
[<c0091f88>] (warn_slowpath_null+0x18/0x1c) from [<c024e610>](gpio_free+0x100/0x12c)
[<c024e610>] (gpio_free+0x100/0x12c) from [<c03e9fbc>](ads7846_probe+0xa38/0xc5c)
[<c03e9fbc>] (ads7846_probe+0xa38/0xc5c) from [<c02cff14>](spi_drv_probe+0x18/0x1c)
[<c02cff14>] (spi_drv_probe+0x18/0x1c) from [<c028bca4>](driver_probe_device+0xc8/0x184)
[<c028bca4>] (driver_probe_device+0xc8/0x184) from [<c028bdc8>](__driver_attach+0x68/0x8c)
[<c028bdc8>] (__driver_attach+0x68/0x8c) from [<c028b4c8>](bus_for_each_dev+0x48/0x74)
[<c028b4c8>] (bus_for_each_dev+0x48/0x74) from [<c028ae08>](bus_add_driver+0xa0/0x220)
[<c028ae08>] (bus_add_driver+0xa0/0x220) from [<c028c0c0>](driver_register+0xa8/0x134)
[<c028c0c0>] (driver_register+0xa8/0x134) from [<c0050550>](do_one_initcall+0xcc/0x1a4)
[<c0050550>] (do_one_initcall+0xcc/0x1a4) from [<c00084e4>](kernel_init+0x14c/0x214)
[<c00084e4>] (kernel_init+0x14c/0x214) from [<c005b494>](kernel_thread_exit+0x0/0x8)
---[ end trace 4053287f8a5ec18f ]---

Also rearrange ads7846_setup_pendown() to have only one exit point returning success.

Reported-by Sourav Poddar <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

---

?drivers/input/touchscreen/ads7846.c | ? 39 ++++++++++++++++++++--------------- ?1 files changed, 22 insertions(+), 17 deletions(-)

drivers/input/touchscreen/ads7846.c +2 -1

index 0e9492d..b1217e1 100644
@@ -946,30 +946,30 @@ static int __devinit ads7846_setup_pendown(struct spi_device *spi, struct ads784
 ? ? ? ?struct ads7846_platform_data *pdata = spi->dev.platform_data;
 ? ? ? ?int err;
 
- ? ? ? /* REVISIT when the irq can be triggered active-low, or if for some
+ ? ? ? /*
+ ? ? ? ?* REVISIT when the irq can be triggered active-low, or if for some

? ? ? ? * reason the touchscreen isn't hooked up, we don't need to access ? ? ? ? * the pendown state.
? ? ? ? */
- ? ? ? if (!pdata->get_pendown_state && !gpio_is_valid(pdata->gpio_pendown)) {
- ? ? ? ? ? ? ? dev_err(&spi->dev, "no get_pendown_state nor gpio_pendown?\n");
- ? ? ? ? ? ? ? return -EINVAL;
- ? ? ? }

? ? ? ?if (pdata->get_pendown_state) {
? ? ? ? ? ? ? ?ts->get_pendown_state = pdata->get_pendown_state;
- ? ? ? ? ? ? ? return 0;
- ? ? ? }
+ ? ? ? } else if (gpio_is_valid(pdata->gpio_pendown)) {
+
+ ? ? ? ? ? ? ? err = gpio_request_one(pdata->gpio_pendown, GPIOF_DIR_IN,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "ads7846_pendown");
+ ? ? ? ? ? ? ? if (err) {
+ ? ? ? ? ? ? ? ? ? ? ? dev_err(&spi->dev, "failed to request pendown GPIO%d\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->gpio_pendown);
+ ? ? ? ? ? ? ? ? ? ? ? return err;
+ ? ? ? ? ? ? ? }

- ? ? ? err = gpio_request_one(pdata->gpio_pendown, GPIOF_DIR_IN,
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "ads7846_pendown");
- ? ? ? if (err) {
- ? ? ? ? ? ? ? dev_err(&spi->dev, "failed to request pendown GPIO%d\n",
- ? ? ? ? ? ? ? ? ? ? ? pdata->gpio_pendown);
- ? ? ? ? ? ? ? return err;
+ ? ? ? ? ? ? ? ts->gpio_pendown = pdata->gpio_pendown;
+ ? ? ? } else {
+ ? ? ? ? ? ? ? dev_err(&spi->dev, "no get_pendown_state nor gpio_pendown?\n");
+ ? ? ? ? ? ? ? return -EINVAL;
? ? ? ?}

- ? ? ? ts->gpio_pendown = pdata->gpio_pendown;
-
? ? ? ?return 0;
?}

drivers/input/touchscreen/ads7846.c +0 -0

@@ -1359,7 +1359,7 @@ static int __devinit ads7846_probe(struct spi_device *spi)

?err_put_regulator:
? ? ? ?regulator_put(ts->reg);
?err_free_gpio:
- ? ? ? if (ts->gpio_pendown != -1)
+ ? ? ? if (!ts->get_pendown_state)
? ? ? ? ? ? ? ?gpio_free(ts->gpio_pendown);
?err_cleanup_filter:
? ? ? ?if (ts->filter_cleanup)

drivers/input/touchscreen/ads7846.c +6 -1

@@ -1389,8 +1389,13 @@ static int __devexit ads7846_remove(struct spi_device *spi)
 ? ? ? ?regulator_disable(ts->reg);
 ? ? ? ?regulator_put(ts->reg);
 
- ? ? ? if (ts->gpio_pendown != -1)
+ ? ? ? if (!ts->get_pendown_state) {
+ ? ? ? ? ? ? ? /*
+ ? ? ? ? ? ? ? ?* If we are not using specialized pendown method we must
+ ? ? ? ? ? ? ? ?* have been relying on gpio we set up ourselves.
+ ? ? ? ? ? ? ? ?*/
 ? ? ? ? ? ? ? ?gpio_free(ts->gpio_pendown);
+ ? ? ? }
 
 ? ? ? ?if (ts->filter_cleanup)
 ? ? ? ? ? ? ? ?ts->filter_cleanup(ts->filter_data);

Quoted 11 lines · Dmitry Torokhov

Quoted 6 lines · Sourav Poddar

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Will apply this one, the other one is a bit messy IMO, will have to think about it.

Something like below should do I think.

--
Dmitry

Input: ads7846 - check proper condition when freeing gpio

From: Dmitry Torokhov <[email protected]>

When driver uses custom pendown detection method gpio_pendown is not set up and so we should not try to free it, otherwise we are presented with:

------------[ cut here ]------------
WARNING: at drivers/gpio/gpiolib.c:1258 gpio_free+0x100/0x12c()
Modules linked in:
[<c0061208>] (unwind_backtrace+0x0/0xe4) from [<c0091f58>](warn_slowpath_common+0x4c/0x64)
[<c0091f58>] (warn_slowpath_common+0x4c/0x64) from [<c0091f88>](warn_slowpath_null+0x18/0x1c)
[<c0091f88>] (warn_slowpath_null+0x18/0x1c) from [<c024e610>](gpio_free+0x100/0x12c)
[<c024e610>] (gpio_free+0x100/0x12c) from [<c03e9fbc>](ads7846_probe+0xa38/0xc5c)
[<c03e9fbc>] (ads7846_probe+0xa38/0xc5c) from [<c02cff14>](spi_drv_probe+0x18/0x1c)
[<c02cff14>] (spi_drv_probe+0x18/0x1c) from [<c028bca4>](driver_probe_device+0xc8/0x184)
[<c028bca4>] (driver_probe_device+0xc8/0x184) from [<c028bdc8>](__driver_attach+0x68/0x8c)
[<c028bdc8>] (__driver_attach+0x68/0x8c) from [<c028b4c8>](bus_for_each_dev+0x48/0x74)
[<c028b4c8>] (bus_for_each_dev+0x48/0x74) from [<c028ae08>](bus_add_driver+0xa0/0x220)
[<c028ae08>] (bus_add_driver+0xa0/0x220) from [<c028c0c0>](driver_register+0xa8/0x134)
[<c028c0c0>] (driver_register+0xa8/0x134) from [<c0050550>](do_one_initcall+0xcc/0x1a4)
[<c0050550>] (do_one_initcall+0xcc/0x1a4) from [<c00084e4>](kernel_init+0x14c/0x214)
[<c00084e4>] (kernel_init+0x14c/0x214) from [<c005b494>](kernel_thread_exit+0x0/0x8)
---[ end trace 4053287f8a5ec18f ]---

Also rearrange ads7846_setup_pendown() to have only one exit point returning success.

Reported-by Sourav Poddar <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

---

Quoted 5 lines · Dmitry Torokhov

Quoted 4 lines · Sourav Poddar

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Quoted 6 lines

Quoted 2 lines

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Will apply this one, the other one is a bit messy IMO, will have to think about it.

Something like below should do I think.

Signature · 39 lines

-- 
Dmitry


Input: ads7846 - check proper condition when freeing gpio

From: Dmitry Torokhov <[email protected]>

When driver uses custom pendown detection method gpio_pendown is not
set up and so we should not try to free it, otherwise we are presented
with:

------------[ cut here ]------------
WARNING: at drivers/gpio/gpiolib.c:1258 gpio_free+0x100/0x12c()
Modules linked in:
[<c0061208>] (unwind_backtrace+0x0/0xe4) from [<c0091f58>](warn_slowpath_common+0x4c/0x64)
[<c0091f58>] (warn_slowpath_common+0x4c/0x64) from [<c0091f88>](warn_slowpath_null+0x18/0x1c)
[<c0091f88>] (warn_slowpath_null+0x18/0x1c) from [<c024e610>](gpio_free+0x100/0x12c)
[<c024e610>] (gpio_free+0x100/0x12c) from [<c03e9fbc>](ads7846_probe+0xa38/0xc5c)
[<c03e9fbc>] (ads7846_probe+0xa38/0xc5c) from [<c02cff14>](spi_drv_probe+0x18/0x1c)
[<c02cff14>] (spi_drv_probe+0x18/0x1c) from [<c028bca4>](driver_probe_device+0xc8/0x184)
[<c028bca4>] (driver_probe_device+0xc8/0x184) from [<c028bdc8>](__driver_attach+0x68/0x8c)
[<c028bdc8>] (__driver_attach+0x68/0x8c) from [<c028b4c8>](bus_for_each_dev+0x48/0x74)
[<c028b4c8>] (bus_for_each_dev+0x48/0x74) from [<c028ae08>](bus_add_driver+0xa0/0x220)
[<c028ae08>] (bus_add_driver+0xa0/0x220) from [<c028c0c0>](driver_register+0xa8/0x134)
[<c028c0c0>] (driver_register+0xa8/0x134) from [<c0050550>](do_one_initcall+0xcc/0x1a4)
[<c0050550>] (do_one_initcall+0xcc/0x1a4) from [<c00084e4>](kernel_init+0x14c/0x214)
[<c00084e4>] (kernel_init+0x14c/0x214) from [<c005b494>](kernel_thread_exit+0x0/0x8)
---[ end trace 4053287f8a5ec18f ]---

Also rearrange ads7846_setup_pendown() to have only one exit point
returning success.

Reported-by: Sourav Poddar <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---

 drivers/input/touchscreen/ads7846.c |   39 ++++++++++++++++++++---------------
 1 files changed, 22 insertions(+), 17 deletions(-)

drivers/input/touchscreen/ads7846.c +22 -18

index 0e9492d..b1217e1 100644
@@ -946,30 +946,30 @@ static int __devinit ads7846_setup_pendown(struct spi_device *spi, struct ads784
 	struct ads7846_platform_data *pdata = spi->dev.platform_data;
 	int err;
 
-	/* REVISIT when the irq can be triggered active-low, or if for some
+	/*
+	 * REVISIT when the irq can be triggered active-low, or if for some
 	 * reason the touchscreen isn't hooked up, we don't need to access
 	 * the pendown state.
 	 */
-	if (!pdata->get_pendown_state && !gpio_is_valid(pdata->gpio_pendown)) {
-		dev_err(&spi->dev, "no get_pendown_state nor gpio_pendown?\n");
-		return -EINVAL;
-	}
 
 	if (pdata->get_pendown_state) {
 		ts->get_pendown_state = pdata->get_pendown_state;
-		return 0;
-	}
+	} else if (gpio_is_valid(pdata->gpio_pendown)) {
+
+		err = gpio_request_one(pdata->gpio_pendown, GPIOF_DIR_IN,
+							"ads7846_pendown");
+		if (err) {
+			dev_err(&spi->dev, "failed to request pendown GPIO%d\n",
+				pdata->gpio_pendown);
+			return err;
+		}
 
-	err = gpio_request_one(pdata->gpio_pendown, GPIOF_DIR_IN,
-						"ads7846_pendown");
-	if (err) {
-		dev_err(&spi->dev, "failed to request pendown GPIO%d\n",
-			pdata->gpio_pendown);
-		return err;
+		ts->gpio_pendown = pdata->gpio_pendown;
+	} else {
+		dev_err(&spi->dev, "no get_pendown_state nor gpio_pendown?\n");
+		return -EINVAL;
 	}
 
-	ts->gpio_pendown = pdata->gpio_pendown;
-
 	return 0;
 }
 

@@ -1359,7 +1359,7 @@ static int __devinit ads7846_probe(struct spi_device *spi)
  err_put_regulator:
 	regulator_put(ts->reg);
  err_free_gpio:
-	if (ts->gpio_pendown != -1)
+	if (!ts->get_pendown_state)
 		gpio_free(ts->gpio_pendown);
  err_cleanup_filter:
 	if (ts->filter_cleanup)

@@ -1389,8 +1389,13 @@ static int __devexit ads7846_remove(struct spi_device *spi)
 	regulator_disable(ts->reg);
 	regulator_put(ts->reg);
 
-	if (ts->gpio_pendown != -1)
+	if (!ts->get_pendown_state) {
+		/*
+		 * If we are not using specialized pendown method we must
+		 * have been relying on gpio we set up ourselves.
+		 */
 		gpio_free(ts->gpio_pendown);
+	}
 
 	if (ts->filter_cleanup)
 		ts->filter_cleanup(ts->filter_data);
--

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

Quoted 6 lines · Dmitry Torokhov

Quoted 5 lines · Dmitry Torokhov

Quoted 4 lines · Sourav Poddar

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Will apply this one, the other one is a bit messy IMO, will have to think about it.

Something like below should do I think.

Patch looks good but it applies only on top of previous patch:
https://patchwork.kernel.org/patch/529941/

Why to have two patches for this fix?

Both the patches can be merged as:

drivers/input/touchscreen/ads7846.c +0 -0

index 14ea54b..940967b 100644
@@ -941,29 +941,30 @@ static int __devinit ads7846_setup_pendown(struct

spi_device *spi, struct ads784
? ? ? ?struct ads7846_platform_data *pdata = spi->dev.platform_data; ? ? ? ?int err;

- ? ? ? /* REVISIT when the irq can be triggered active-low, or if for ? ? ? ?some
+ ? ? ? /*
+ ? ? ? ?* REVISIT when the irq can be triggered active-low, or if for some
? ? ? ? * reason the touchscreen isn't hooked up, we don't need to ? ? ? ? * access
? ? ? ? * the pendown state.
? ? ? ? */
- ? ? ? if (!pdata->get_pendown_state &&
? ? ? ?!gpio_is_valid(pdata->gpio_pendown)) {
- ? ? ? ? ? ? ? dev_err(&spi->dev, "no get_pendown_state nor ? ? ? ? ? ? ? ?gpio_pendown?\n");
- ? ? ? ? ? ? ? return -EINVAL;
- ? ? ? }

? ? ? ?if (pdata->get_pendown_state) {
? ? ? ? ? ? ? ?ts->get_pendown_state = pdata->get_pendown_state;
- ? ? ? ? ? ? ? return 0;
- ? ? ? }
+ ? ? ? } else if (gpio_is_valid(pdata->gpio_pendown)) {
+
+ ? ? ? ? ? ? ? err = gpio_request_one(pdata->gpio_pendown, GPIOF_DIR_IN,
+
"ads7846_pendown");
+ ? ? ? ? ? ? ? if (err) {
+ ? ? ? ? ? ? ? ? ? ? ? dev_err(&spi->dev, "failed to request pendown GPIO%d\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->gpio_pendown);
+ ? ? ? ? ? ? ? ? ? ? ? return err;
+ ? ? ? ? ? ? ? }

- ? ? ? err = gpio_request(pdata->gpio_pendown, "ads7846_pendown");
- ? ? ? if (err) {
- ? ? ? ? ? ? ? dev_err(&spi->dev, "failed to request pendown GPIO%d\n",
- ? ? ? ? ? ? ? ? ? ? ? pdata->gpio_pendown);
- ? ? ? ? ? ? ? return err;
+ ? ? ? ? ? ? ? ts->gpio_pendown = pdata->gpio_pendown;
+ ? ? ? } else {
+ ? ? ? ? ? ? ? dev_err(&spi->dev, "no get_pendown_state nor gpio_pendown?\n");
+ ? ? ? ? ? ? ? return -EINVAL;
? ? ? ?}

- ? ? ? ts->gpio_pendown = pdata->gpio_pendown;
-
? ? ? ?return 0;
?}

drivers/input/touchscreen/ads7846.c +0 -0

@@ -1353,7 +1354,7 @@ static int __devinit ads7846_probe(struct
 spi_device *spi)

?err_put_regulator:
? ? ? ?regulator_put(ts->reg);
?err_free_gpio:
- ? ? ? if (ts->gpio_pendown != -1)
+ ? ? ? if (!ts->get_pendown_state)
? ? ? ? ? ? ? ?gpio_free(ts->gpio_pendown);
?err_cleanup_filter:
? ? ? ?if (ts->filter_cleanup)

drivers/input/touchscreen/ads7846.c +3 -1

@@ -1383,8 +1384,13 @@ static int __devexit ads7846_remove(struct
 spi_device *spi)
 ? ? ? ?regulator_disable(ts->reg);
 ? ? ? ?regulator_put(ts->reg);
 
- ? ? ? if (ts->gpio_pendown != -1)
+ ? ? ? if (!ts->get_pendown_state) {
+ ? ? ? ? ? ? ? /*
+ ? ? ? ? ? ? ? ?* If we are not using specialized pendown method we

must
+ ? ? ? ? ? ? ? ?* have been relying on gpio we set up ourselves.
+ ? ? ? ? ? ? ? ?*/
? ? ? ? ? ? ? ?gpio_free(ts->gpio_pendown);
+ ? ? ? }

? ? ? ?if (ts->filter_cleanup)
? ? ? ? ? ? ? ?ts->filter_cleanup(ts->filter_data);

Quoted 117 lines

--
Dmitry

Input: ads7846 - check proper condition when freeing gpio

From: Dmitry Torokhov <[email protected]>

When driver uses custom pendown detection method gpio_pendown is not set up and so we should not try to free it, otherwise we are presented with:

------------[ cut here ]------------
WARNING: at drivers/gpio/gpiolib.c:1258 gpio_free+0x100/0x12c()
Modules linked in:
[<c0061208>] (unwind_backtrace+0x0/0xe4) from [<c0091f58>](warn_slowpath_common+0x4c/0x64)
[<c0091f58>] (warn_slowpath_common+0x4c/0x64) from [<c0091f88>](warn_slowpath_null+0x18/0x1c)
[<c0091f88>] (warn_slowpath_null+0x18/0x1c) from [<c024e610>](gpio_free+0x100/0x12c)
[<c024e610>] (gpio_free+0x100/0x12c) from [<c03e9fbc>](ads7846_probe+0xa38/0xc5c)
[<c03e9fbc>] (ads7846_probe+0xa38/0xc5c) from [<c02cff14>](spi_drv_probe+0x18/0x1c)
[<c02cff14>] (spi_drv_probe+0x18/0x1c) from [<c028bca4>](driver_probe_device+0xc8/0x184)
[<c028bca4>] (driver_probe_device+0xc8/0x184) from [<c028bdc8>](__driver_attach+0x68/0x8c)
[<c028bdc8>] (__driver_attach+0x68/0x8c) from [<c028b4c8>](bus_for_each_dev+0x48/0x74)
[<c028b4c8>] (bus_for_each_dev+0x48/0x74) from [<c028ae08>](bus_add_driver+0xa0/0x220)
[<c028ae08>] (bus_add_driver+0xa0/0x220) from [<c028c0c0>](driver_register+0xa8/0x134)
[<c028c0c0>] (driver_register+0xa8/0x134) from [<c0050550>](do_one_initcall+0xcc/0x1a4)
[<c0050550>] (do_one_initcall+0xcc/0x1a4) from [<c00084e4>](kernel_init+0x14c/0x214)
[<c00084e4>] (kernel_init+0x14c/0x214) from [<c005b494>](kernel_thread_exit+0x0/0x8)
---[ end trace 4053287f8a5ec18f ]---

Also rearrange ads7846_setup_pendown() to have only one exit point returning success.

Reported-by Sourav Poddar <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

---

?drivers/input/touchscreen/ads7846.c | ? 39 ++++++++++++++++++++--------------- ?1 files changed, 22 insertions(+), 17 deletions(-)

drivers/input/touchscreen/ads7846.c +2 -1

index 0e9492d..b1217e1 100644
@@ -946,30 +946,30 @@ static int __devinit ads7846_setup_pendown(struct spi_device *spi, struct ads784
 ? ? ? struct ads7846_platform_data *pdata = spi->dev.platform_data;
 ? ? ? int err;
 
- ? ? /* REVISIT when the irq can be triggered active-low, or if for some
+ ? ? /*
+ ? ? ?* REVISIT when the irq can be triggered active-low, or if for some

? ? ? ?* reason the touchscreen isn't hooked up, we don't need to access ? ? ? ?* the pendown state.
? ? ? ?*/
- ? ? if (!pdata->get_pendown_state && !gpio_is_valid(pdata->gpio_pendown)) {
- ? ? ? ? ? ? dev_err(&spi->dev, "no get_pendown_state nor gpio_pendown?\n");
- ? ? ? ? ? ? return -EINVAL;
- ? ? }

? ? ? if (pdata->get_pendown_state) {
? ? ? ? ? ? ? ts->get_pendown_state = pdata->get_pendown_state;
- ? ? ? ? ? ? return 0;
- ? ? }
+ ? ? } else if (gpio_is_valid(pdata->gpio_pendown)) {
+
+ ? ? ? ? ? ? err = gpio_request_one(pdata->gpio_pendown, GPIOF_DIR_IN,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "ads7846_pendown");
+ ? ? ? ? ? ? if (err) {
+ ? ? ? ? ? ? ? ? ? ? dev_err(&spi->dev, "failed to request pendown GPIO%d\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? pdata->gpio_pendown);
+ ? ? ? ? ? ? ? ? ? ? return err;
+ ? ? ? ? ? ? }

- ? ? err = gpio_request_one(pdata->gpio_pendown, GPIOF_DIR_IN,
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "ads7846_pendown");
- ? ? if (err) {
- ? ? ? ? ? ? dev_err(&spi->dev, "failed to request pendown GPIO%d\n",
- ? ? ? ? ? ? ? ? ? ? pdata->gpio_pendown);
- ? ? ? ? ? ? return err;
+ ? ? ? ? ? ? ts->gpio_pendown = pdata->gpio_pendown;
+ ? ? } else {
+ ? ? ? ? ? ? dev_err(&spi->dev, "no get_pendown_state nor gpio_pendown?\n");
+ ? ? ? ? ? ? return -EINVAL;
? ? ? }

- ? ? ts->gpio_pendown = pdata->gpio_pendown;
-
? ? ? return 0;
?}

drivers/input/touchscreen/ads7846.c +0 -0

@@ -1359,7 +1359,7 @@ static int __devinit ads7846_probe(struct spi_device *spi)

? err_put_regulator:
? ? ? regulator_put(ts->reg);
? err_free_gpio:
- ? ? if (ts->gpio_pendown != -1)
+ ? ? if (!ts->get_pendown_state)
? ? ? ? ? ? ? gpio_free(ts->gpio_pendown);
? err_cleanup_filter:
? ? ? if (ts->filter_cleanup)

drivers/input/touchscreen/ads7846.c +6 -2

@@ -1389,8 +1389,13 @@ static int __devexit ads7846_remove(struct spi_device *spi)
 ? ? ? regulator_disable(ts->reg);
 ? ? ? regulator_put(ts->reg);
 
- ? ? if (ts->gpio_pendown != -1)
+ ? ? if (!ts->get_pendown_state) {
+ ? ? ? ? ? ? /*
+ ? ? ? ? ? ? ?* If we are not using specialized pendown method we must
+ ? ? ? ? ? ? ?* have been relying on gpio we set up ourselves.
+ ? ? ? ? ? ? ?*/
 ? ? ? ? ? ? ? gpio_free(ts->gpio_pendown);
+ ? ? }
 
 ? ? ? if (ts->filter_cleanup)
 ? ? ? ? ? ? ? ts->filter_cleanup(ts->filter_data);
--

Quoted 3 lines

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

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

Manju,

Line wrapped, looks like your .muttrc needs to be configured for 80 characters limit.

Quoted 13 lines · Dmitry Torokhov

Quoted 11 lines · Dmitry Torokhov

Quoted 6 lines · Sourav Poddar

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Will apply this one, the other one is a bit messy IMO, will have to think about it.

Quoted 1 line

Something like below should do I think.

Patch looks good but it applies only on top of previous patch:
https://patchwork.kernel.org/patch/529941/

Why to have two patches for this fix?

Quoted 14 lines · Manjunath Kondaiah

Quoted 13 lines · Dmitry Torokhov

Quoted 11 lines · Dmitry Torokhov

Quoted 6 lines · Sourav Poddar

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Will apply this one, the other one is a bit messy IMO, will have to think about it.

Quoted 5 lines

Quoted 1 line

Something like below should do I think.

Patch looks good but it applies only on top of previous patch:
https://patchwork.kernel.org/patch/529941/

Why to have two patches for this fix?

http://www.spinics.net/lists/linux-omap/msg45167.html

Quoted 17 lines · Wolfram Sang

Quoted 16 lines · Manjunath Kondaiah

Quoted 6 lines · Dmitry Torokhov

Quoted 5 lines · Dmitry Torokhov

Quoted 4 lines · Sourav Poddar

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Will apply this one, the other one is a bit messy IMO, will have to think about it.

Something like below should do I think.

Patch looks good but it applies only on top of previous patch:
https://patchwork.kernel.org/patch/529941/

Quoted 2 lines

Quoted 1 line

Why to have two patches for this fix?

http://www.spinics.net/lists/linux-omap/msg45167.html

My point here is:
1. The first patch only replaces gpio_request with gpio_request_one
2. Rest of the things are handled in 2nd patch posted by dmitry

What is harm in merging both the patches? I don't think it affects readability.

Quoted 24 lines · Manjunath Kondaiah

Quoted 13 lines · Wolfram Sang

Quoted 12 lines · Manjunath Kondaiah

Quoted 11 lines · Dmitry Torokhov

Quoted 10 lines · Dmitry Torokhov

Quoted 6 lines · Sourav Poddar

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Will apply this one, the other one is a bit messy IMO, will have to think about it.

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

Something like below should do I think.

Patch looks good but it applies only on top of previous patch:
https://patchwork.kernel.org/patch/529941/

Why to have two patches for this fix?

http://www.spinics.net/lists/linux-omap/msg45167.html

My point here is:
1. The first patch only replaces gpio_request with gpio_request_one
2. Rest of the things are handled in 2nd patch posted by dmitry

Quoted 2 lines

What is harm in merging both the patches? I don't think it affects readability.

Because the changes introduced by the patches are from different nature. As stated in the link above, one is a functional change (gpio setup change) and second is fixing the imbalance in request - free calls.
The impact is not readability, but bad bisect-ability.

--
Regards,
Igor.

Quoted 29 lines · Igor Grinberg

Quoted 26 lines · Manjunath Kondaiah

Quoted 13 lines · Wolfram Sang

Quoted 12 lines · Manjunath Kondaiah

Quoted 11 lines · Dmitry Torokhov

Quoted 10 lines · Dmitry Torokhov

Quoted 6 lines · Sourav Poddar

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Will apply this one, the other one is a bit messy IMO, will have to think about it.

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

Something like below should do I think.

Patch looks good but it applies only on top of previous patch:
https://patchwork.kernel.org/patch/529941/

Why to have two patches for this fix?

http://www.spinics.net/lists/linux-omap/msg45167.html

My point here is:
1. The first patch only replaces gpio_request with gpio_request_one
2. Rest of the things are handled in 2nd patch posted by dmitry

What is harm in merging both the patches? I don't think it affects readability.

Quoted 4 lines

Because the changes introduced by the patches are from different nature. As stated in the link above, one is a functional change (gpio setup change) and second is fixing the imbalance in request - free calls.
The impact is not readability, but bad bisect-ability.

Dmitry's patch fixes both the problems(request/free and direction) in a single patch itself.Now there is no need of merging any patches. Just that Dmitry's patch need to be rebased over the top of HEAD. (Currently, its on top of my patch series).

Quoted 30 lines · Sourav

Quoted 29 lines · Igor Grinberg

Quoted 26 lines · Manjunath Kondaiah

Quoted 13 lines · Wolfram Sang

Quoted 12 lines · Manjunath Kondaiah

Quoted 11 lines · Dmitry Torokhov

Quoted 10 lines · Dmitry Torokhov

Quoted 6 lines · Sourav Poddar

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Will apply this one, the other one is a bit messy IMO, will have to think about it.

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

Something like below should do I think.

Patch looks good but it applies only on top of previous patch:
https://patchwork.kernel.org/patch/529941/

Why to have two patches for this fix?

http://www.spinics.net/lists/linux-omap/msg45167.html

My point here is:
1. The first patch only replaces gpio_request with gpio_request_one
2. Rest of the things are handled in 2nd patch posted by dmitry

What is harm in merging both the patches? I don't think it affects readability.

Quoted 8 lines

Quoted 4 lines

Because the changes introduced by the patches are from different nature. As stated in the link above, one is a functional change (gpio setup change) and second is fixing the imbalance in request - free calls.
The impact is not readability, but bad bisect-ability.

? Dmitry's patch fixes both the problems(request/free and direction) ? in a single patch itself.Now there is no need of merging any patches. ? Just that Dmitry's patch need to be rebased over the top of HEAD. (Currently, ? its on top of my patch series).

Well, here you have missed the point of direction:
...

+ ? ? ? ? ? ? ? err = gpio_request_one(pdata->gpio_pendown, GPIOF_DIR_IN,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "ads7846_pendown");

...

- ? ? ? err = gpio_request_one(pdata->gpio_pendown, GPIOF_DIR_IN,
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? "ads7846_pendown");
...

It does not deal with direction, but only with request - free balance. The gpio direction is fixed by your patch.

Quoted 43 lines · Sourav

Quoted 20 lines · Igor Grinberg

Quoted 18 lines · Manjunath Kondaiah

Quoted 17 lines · Wolfram Sang

Quoted 9 lines · Manjunath Kondaiah

Quoted 7 lines · Dmitry Torokhov

Quoted 2 lines · Dmitry Torokhov

Quoted 1 line

Something like below should do I think.

Patch looks good but it applies only on top of previous patch:
https://patchwork.kernel.org/patch/529941/

Why to have two patches for this fix?

http://www.spinics.net/lists/linux-omap/msg45167.html

My point here is:
1. The first patch only replaces gpio_request with gpio_request_one
2. Rest of the things are handled in 2nd patch posted by dmitry

What is harm in merging both the patches? I don't think it affects readability.

Quoted 8 lines

Quoted 4 lines

Because the changes introduced by the patches are from different nature. As stated in the link above, one is a functional change (gpio setup change) and second is fixing the imbalance in request - free calls.
The impact is not readability, but bad bisect-ability.

Dmitry's patch fixes both the problems(request/free and direction) in a single patch itself.Now there is no need of merging any patches. Just that Dmitry's patch need to be rebased over the top of HEAD. (Currently, its on top of my patch series).

Well, here you have missed the point of direction:
...

(quoted diff) +2 -2

+               err = gpio_request_one(pdata->gpio_pendown, GPIOF_DIR_IN,
+                                                       "ads7846_pendown");
 
...
 
-       err = gpio_request_one(pdata->gpio_pendown, GPIOF_DIR_IN,
-                                               "ads7846_pendown");
...

Quoted 3 lines

It does not deal with direction, but only with request - free balance. The gpio direction is fixed by your patch.

gpio_request_one is a different API which calls gpio_direction_input for configuring the direction.

Quoted 24 lines · Manjunath Kondaiah

Quoted 13 lines · Wolfram Sang

Quoted 12 lines · Manjunath Kondaiah

Quoted 11 lines · Dmitry Torokhov

Quoted 10 lines · Dmitry Torokhov

Quoted 6 lines · Sourav Poddar

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Will apply this one, the other one is a bit messy IMO, will have to think about it.

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

Something like below should do I think.

Patch looks good but it applies only on top of previous patch:
https://patchwork.kernel.org/patch/529941/

Why to have two patches for this fix?

http://www.spinics.net/lists/linux-omap/msg45167.html

My point here is:
1. The first patch only replaces gpio_request with gpio_request_one
2. Rest of the things are handled in 2nd patch posted by dmitry

Quoted 2 lines

What is harm in merging both the patches? I don't think it affects readability.

Because the changes introduced by the patches are from different nature. As stated in the link above, one is a functional change (gpio setup change) and second is fixing the imbalance in request - free calls.
The impact is not readability, but bad bisect-ability.

Quoted 16 lines · Igor Grinberg

Quoted 15 lines · Manjunath Kondaiah

Quoted 8 lines · Wolfram Sang

Quoted 6 lines · Manjunath Kondaiah

Quoted 1 line · Dmitry Torokhov

Something like below should do I think.

Patch looks good but it applies only on top of previous patch:
https://patchwork.kernel.org/patch/529941/

Why to have two patches for this fix?

http://www.spinics.net/lists/linux-omap/msg45167.html

My point here is:
1. The first patch only replaces gpio_request with gpio_request_one
2. Rest of the things are handled in 2nd patch posted by dmitry

What is harm in merging both the patches? I don't think it affects readability.

Quoted 4 lines

Because the changes introduced by the patches are from different nature. As stated in the link above, one is a functional change (gpio setup change) and second is fixing the imbalance in request - free calls.
The impact is not readability, but bad bisect-ability.

ok. But the patch2(dmitry's patch) is doing more than what it is mentioned in patch description. It checks for validity of gpio, comment correction etc which needs to be updated in the patch description.

Hi,

Quoted 17 lines · Manjunath Kondaiah

Quoted 16 lines · Igor Grinberg

Quoted 15 lines · Manjunath Kondaiah

Quoted 8 lines · Wolfram Sang

Quoted 6 lines · Manjunath Kondaiah

Quoted 1 line · Dmitry Torokhov

Something like below should do I think.

Patch looks good but it applies only on top of previous patch:
https://patchwork.kernel.org/patch/529941/

Why to have two patches for this fix?

http://www.spinics.net/lists/linux-omap/msg45167.html

My point here is:
1. The first patch only replaces gpio_request with gpio_request_one
2. Rest of the things are handled in 2nd patch posted by dmitry

What is harm in merging both the patches? I don't think it affects readability.

Quoted 7 lines

Quoted 4 lines

Because the changes introduced by the patches are from different nature. As stated in the link above, one is a functional change (gpio setup change) and second is fixing the imbalance in request - free calls.
The impact is not readability, but bad bisect-ability.

ok. But the patch2(dmitry's patch) is doing more than what it is mentioned in patch description. It checks for validity of gpio, comment correction etc which needs to be updated in the patch description.

gpio validity is a part of request - free balance fix, comment change is just a coding style fix - really minor.

Personally, I think Dmitry's description of the patch is just fine, but if you insist on making it somehow better, then suggest it to Dmitry.

Quoted 19 lines · Wolfram Sang

Quoted 18 lines · Manjunath Kondaiah

Quoted 6 lines · Dmitry Torokhov

Quoted 5 lines · Dmitry Torokhov

Quoted 4 lines · Sourav Poddar

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Will apply this one, the other one is a bit messy IMO, will have to think about it.

Something like below should do I think.

Patch looks good but it applies only on top of previous patch:
https://patchwork.kernel.org/patch/529941/

Quoted 3 lines

Quoted 1 line

Why to have two patches for this fix?

http://www.spinics.net/lists/linux-omap/msg45167.html

My point here is:
1. The first patch only replaces gpio_request with gpio_request_one
2. Rest of the things are handled in 2nd patch posted by dmitry

Quoted 10 lines · Sourav Poddar

Quoted 6 lines

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>

Will apply this one, the other one is a bit messy IMO, will have to think about it.

Something like below should do I think.

Dmitry

Input: ads7846 - check proper condition when freeing gpio

From: Dmitry Torokhov <[email protected]>

When driver uses custom pendown detection method gpio_pendown is not set up and so we should not try to free it, otherwise we are presented with:

------------[ cut here ]------------
WARNING: at drivers/gpio/gpiolib.c:1258 gpio_free+0x100/0x12c()
Modules linked in:
[<c0061208>] (unwind_backtrace+0x0/0xe4) from [<c0091f58>](warn_slowpath_common+0x4c/0x64)
[<c0091f58>] (warn_slowpath_common+0x4c/0x64) from [<c0091f88>](warn_slowpath_null+0x18/0x1c)
[<c0091f88>] (warn_slowpath_null+0x18/0x1c) from [<c024e610>](gpio_free+0x100/0x12c)
[<c024e610>] (gpio_free+0x100/0x12c) from [<c03e9fbc>](ads7846_probe+0xa38/0xc5c)
[<c03e9fbc>] (ads7846_probe+0xa38/0xc5c) from [<c02cff14>](spi_drv_probe+0x18/0x1c)
[<c02cff14>] (spi_drv_probe+0x18/0x1c) from [<c028bca4>](driver_probe_device+0xc8/0x184)
[<c028bca4>] (driver_probe_device+0xc8/0x184) from [<c028bdc8>](__driver_attach+0x68/0x8c)
[<c028bdc8>] (__driver_attach+0x68/0x8c) from [<c028b4c8>](bus_for_each_dev+0x48/0x74)
[<c028b4c8>] (bus_for_each_dev+0x48/0x74) from [<c028ae08>](bus_add_driver+0xa0/0x220)
[<c028ae08>] (bus_add_driver+0xa0/0x220) from [<c028c0c0>](driver_register+0xa8/0x134)
[<c028c0c0>] (driver_register+0xa8/0x134) from [<c0050550>](do_one_initcall+0xcc/0x1a4)
[<c0050550>] (do_one_initcall+0xcc/0x1a4) from [<c00084e4>](kernel_init+0x14c/0x214)
[<c00084e4>] (kernel_init+0x14c/0x214) from [<c005b494>](kernel_thread_exit+0x0/0x8)
---[ end trace 4053287f8a5ec18f ]---

Also rearrange ads7846_setup_pendown() to have only one exit point returning success.

Reported-by Sourav Poddar <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/input/touchscreen/ads7846.c |   39 ++++++++++++++++++++---------------
 1 files changed, 22 insertions(+), 17 deletions(-)

drivers/input/touchscreen/ads7846.c +22 -17

index 0e9492d..b1217e1 100644
@@ -946,30 +946,30 @@ static int __devinit ads7846_setup_pendown(struct spi_device *spi, struct ads784
 	struct ads7846_platform_data *pdata = spi->dev.platform_data;
 	int err;
 
-	/* REVISIT when the irq can be triggered active-low, or if for some
+	/*
+	 * REVISIT when the irq can be triggered active-low, or if for some
 	 * reason the touchscreen isn't hooked up, we don't need to access
 	 * the pendown state.
 	 */
-	if (!pdata->get_pendown_state && !gpio_is_valid(pdata->gpio_pendown)) {
-		dev_err(&spi->dev, "no get_pendown_state nor gpio_pendown?\n");
-		return -EINVAL;
-	}
 
 	if (pdata->get_pendown_state) {
 		ts->get_pendown_state = pdata->get_pendown_state;
-		return 0;
-	}
+	} else if (gpio_is_valid(pdata->gpio_pendown)) {
+
+		err = gpio_request_one(pdata->gpio_pendown, GPIOF_DIR_IN,
+							"ads7846_pendown");
+		if (err) {
+			dev_err(&spi->dev, "failed to request pendown GPIO%d\n",
+				pdata->gpio_pendown);
+			return err;
+		}
 
-	err = gpio_request_one(pdata->gpio_pendown, GPIOF_DIR_IN,
-						"ads7846_pendown");
-	if (err) {
-		dev_err(&spi->dev, "failed to request pendown GPIO%d\n",
-			pdata->gpio_pendown);
-		return err;
+		ts->gpio_pendown = pdata->gpio_pendown;
+	} else {
+		dev_err(&spi->dev, "no get_pendown_state nor gpio_pendown?\n");
+		return -EINVAL;
 	}
 
-	ts->gpio_pendown = pdata->gpio_pendown;
-
 	return 0;
 }
 

@@ -1359,7 +1359,7 @@ static int __devinit ads7846_probe(struct spi_device *spi)
  err_put_regulator:
 	regulator_put(ts->reg);
  err_free_gpio:
-	if (ts->gpio_pendown != -1)
+	if (!ts->get_pendown_state)
 		gpio_free(ts->gpio_pendown);
  err_cleanup_filter:
 	if (ts->filter_cleanup)

@@ -1389,8 +1389,13 @@ static int __devexit ads7846_remove(struct spi_device *spi)
 	regulator_disable(ts->reg);
 	regulator_put(ts->reg);
 
-	if (ts->gpio_pendown != -1)
+	if (!ts->get_pendown_state) {
+		/*
+		 * If we are not using specialized pendown method we must
+		 * have been relying on gpio we set up ourselves.
+		 */
 		gpio_free(ts->gpio_pendown);
+	}
 
 	if (ts->filter_cleanup)
 		ts->filter_cleanup(ts->filter_data);

The ads7846 driver requests a gpio but does not currently configure it explicitly as an input. Use gpio_request_one to request and configure it at one shot.

Signed-off-by Sourav Poddar <[email protected]>

Cc Dmitry Torokhov <[email protected]>