[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

Subsystems: input (keyboard, mouse, joystick, touchscreen) drivers, the rest

STALE5656d

22 messages, 7 authors, 2011-02-06 · open the first message on its own page

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: Sourav Poddar <hidden>
Date: 2011-02-03 15:21:46 

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 <redacted>
Cc: Dmitry Torokhov <redacted>

---
 Links related to the previous discussions:
 http://permalink.gmane.org/gmane.linux.ports.arm.omap/51300

 drivers/input/touchscreen/ads7846.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index ce5baee..7480460 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -956,7 +956,8 @@ static int __devinit ads7846_setup_pendown(struct spi_device *spi, struct ads784
 		return 0;
 	}
 
-	err = gpio_request(pdata->gpio_pendown, "ads7846_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);
-- 
1.7.0.4

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: dmitry.torokhov@gmail.com (Dmitry Torokhov)
Date: 2011-02-03 16:54:05 

On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
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 <redacted>
Cc: Dmitry Torokhov <redacted>
Will apply this one, the other one is a bit messy IMO, will have to
think about it.

Thanks.

-- 
Dmitry

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: dmitry.torokhov@gmail.com (Dmitry Torokhov)
Date: 2011-02-03 17:19:53 

On Thu, Feb 03, 2011 at 08:54:05AM -0800, Dmitry Torokhov wrote:
On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
quoted
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 <redacted>
Cc: Dmitry Torokhov <redacted>
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 <dmitry.torokhov@gmail.com>

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 <redacted>
Signed-off-by: Dmitry Torokhov <redacted>
---

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


diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 0e9492d..b1217e1 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -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);

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: Wolfram Sang <hidden>
Date: 2011-02-03 22:12:07 

On Thu, Feb 03, 2011 at 09:19:53AM -0800, Dmitry Torokhov wrote:
On Thu, Feb 03, 2011 at 08:54:05AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
quoted
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 <redacted>
Cc: Dmitry Torokhov <redacted>
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.
Yeah, that was what I was suggesting, too

Acked-by: Wolfram Sang <redacted>

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110203/a68d36a8/attachment.sig>

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: Varadarajan, Charulatha <hidden>
Date: 2011-02-04 08:05:10 

On Thu, Feb 3, 2011 at 22:49, Dmitry Torokhov [off-list ref] wrote:
On Thu, Feb 03, 2011 at 08:54:05AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
quoted
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 <redacted>
Cc: Dmitry Torokhov <redacted>
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 <dmitry.torokhov@gmail.com>

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 <redacted>
Signed-off-by: Dmitry Torokhov <redacted>
Reviewed-by: Charulatha V <redacted>
quoted hunk
---

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


diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 0e9492d..b1217e1 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -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);

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: Poddar, Sourav <hidden>
Date: 2011-02-04 12:59:23 

On Thu, Feb 3, 2011 at 10:49 PM, Dmitry Torokhov
[off-list ref] wrote:
On Thu, Feb 03, 2011 at 08:54:05AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
quoted
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 <redacted>
Cc: Dmitry Torokhov <redacted>
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 <dmitry.torokhov@gmail.com>

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 <redacted>
Signed-off-by: Dmitry Torokhov <redacted>
---
Acked-by: Sourav Poddar <redacted>

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: G, Manjunath Kondaiah <hidden>
Date: 2011-02-04 13:32:50 

On Thu, Feb 03, 2011 at 09:19:53AM -0800, Dmitry Torokhov wrote:
On Thu, Feb 03, 2011 at 08:54:05AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
quoted
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 <redacted>
Cc: Dmitry Torokhov <redacted>
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:

diff --git a/drivers/input/touchscreen/ads7846.c
b/drivers/input/touchscreen/ads7846.c
index 14ea54b..940967b 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -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;
 }
 
@@ -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)
@@ -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 hunk
-- 
Dmitry


Input: ads7846 - check proper condition when freeing gpio

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

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 <redacted>
Signed-off-by: Dmitry Torokhov <redacted>
---

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


diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 0e9492d..b1217e1 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -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

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: Kishore Kadiyala <hidden>
Date: 2011-02-04 13:37:37 

Manju,

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

Regards,
Kishore

On Fri, Feb 4, 2011 at 7:02 PM, G, Manjunath Kondaiah [off-list ref] wrote:
quoted hunk
On Thu, Feb 03, 2011 at 09:19:53AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:54:05AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
quoted
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 <redacted>
Cc: Dmitry Torokhov <redacted>
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:

diff --git a/drivers/input/touchscreen/ads7846.c
b/drivers/input/touchscreen/ads7846.c
index 14ea54b..940967b 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -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;
?}

@@ -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)
@@ -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
--
Dmitry


Input: ads7846 - check proper condition when freeing gpio

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

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 <redacted>
Signed-off-by: Dmitry Torokhov <redacted>
---

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


diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 0e9492d..b1217e1 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -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
--
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

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: G, Manjunath Kondaiah <hidden>
Date: 2011-02-04 13:41:34 

Kishore,
On Fri, Feb 04, 2011 at 07:07:37PM +0530, Kishore Kadiyala wrote:
Manju,

Line wrapped, looks like your .muttrc needs to be configured for 80
characters limit.
Thanks for reporting. Configured mutt on new machine recently. I will
check and fix it.

-Manjunath

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: Wolfram Sang <hidden>
Date: 2011-02-04 14:08:47 

On Fri, Feb 04, 2011 at 07:02:50PM +0530, G, Manjunath Kondaiah wrote:
On Thu, Feb 03, 2011 at 09:19:53AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:54:05AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
quoted
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 <redacted>
Cc: Dmitry Torokhov <redacted>
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?
http://www.spinics.net/lists/linux-omap/msg45167.html

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110204/26110287/attachment.sig>

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: G, Manjunath Kondaiah <hidden>
Date: 2011-02-04 14:16:18 

On Fri, Feb 04, 2011 at 03:08:47PM +0100, Wolfram Sang wrote:
On Fri, Feb 04, 2011 at 07:02:50PM +0530, G, Manjunath Kondaiah wrote:
quoted
On Thu, Feb 03, 2011 at 09:19:53AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:54:05AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
quoted
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 <redacted>
Cc: Dmitry Torokhov <redacted>
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?
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.

-Manjunath

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: Igor Grinberg <hidden>
Date: 2011-02-04 14:47:09 


On 02/04/11 16:16, G, Manjunath Kondaiah wrote:

On Fri, Feb 04, 2011 at 03:08:47PM +0100, Wolfram Sang wrote:
quoted
On Fri, Feb 04, 2011 at 07:02:50PM +0530, G, Manjunath Kondaiah wrote:
quoted
On Thu, Feb 03, 2011 at 09:19:53AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:54:05AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
quoted
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 <redacted>
Cc: Dmitry Torokhov <redacted>
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?
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.
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.

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: Poddar, Sourav <hidden>
Date: 2011-02-04 15:11:22 

On Fri, Feb 4, 2011 at 8:17 PM, Igor Grinberg [off-list ref] wrote:

On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
quoted
On Fri, Feb 04, 2011 at 03:08:47PM +0100, Wolfram Sang wrote:
quoted
On Fri, Feb 04, 2011 at 07:02:50PM +0530, G, Manjunath Kondaiah wrote:
quoted
On Thu, Feb 03, 2011 at 09:19:53AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:54:05AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
quoted
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 <redacted>
Cc: Dmitry Torokhov <redacted>
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?
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.
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).

--
Regards,
Igor.

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: Igor Grinberg <hidden>
Date: 2011-02-04 15:30:11 


On 02/04/11 17:11, Poddar, Sourav wrote:

On Fri, Feb 4, 2011 at 8:17 PM, Igor Grinberg [off-list ref] wrote:
quoted
On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
quoted
On Fri, Feb 04, 2011 at 03:08:47PM +0100, Wolfram Sang wrote:
quoted
On Fri, Feb 04, 2011 at 07:02:50PM +0530, G, Manjunath Kondaiah wrote:
quoted
On Thu, Feb 03, 2011 at 09:19:53AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:54:05AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
quoted
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 <redacted>
Cc: Dmitry Torokhov <redacted>
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?
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.
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.

-- 
Regards,
Igor.

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: Poddar, Sourav <hidden>
Date: 2011-02-05 06:59:58 

On Fri, Feb 4, 2011 at 9:00 PM, Igor Grinberg [off-list ref] wrote:

On 02/04/11 17:11, Poddar, Sourav wrote:
quoted
On Fri, Feb 4, 2011 at 8:17 PM, Igor Grinberg [off-list ref] wrote:
quoted
On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
quoted
On Fri, Feb 04, 2011 at 03:08:47PM +0100, Wolfram Sang wrote:
quoted
On Fri, Feb 04, 2011 at 07:02:50PM +0530, G, Manjunath Kondaiah wrote:
quoted
On Thu, Feb 03, 2011 at 09:19:53AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:54:05AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
quoted
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 <redacted>
Cc: Dmitry Torokhov <redacted>
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?
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.
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.
gpio_request_one is a different API which calls  gpio_direction_input
for configuring the direction.

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: Igor Grinberg <hidden>
Date: 2011-02-06 07:31:28 

On Fri, Feb 4, 2011 at 9:00 PM, Igor Grinberg [off-list ref] wrote:
quoted
On 02/04/11 17:11, Poddar, Sourav wrote:
quoted
On Fri, Feb 4, 2011 at 8:17 PM, Igor Grinberg [off-list ref] wrote:
quoted
On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
quoted
On Fri, Feb 04, 2011 at 03:08:47PM +0100, Wolfram Sang wrote:
quoted
On Fri, Feb 04, 2011 at 07:02:50PM +0530, G, Manjunath Kondaiah wrote:
quoted
On Thu, Feb 03, 2011 at 09:19:53AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:54:05AM -0800, Dmitry Torokhov wrote:
quoted
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.
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.
gpio_request_one is a different API which calls  gpio_direction_input
for configuring the direction.
That is exactly the point :)
Your patch has added the gpio_request_one() and thus changed the gpio direction.
This patch only moves it around.

-- 
Regards,
Igor.

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: G, Manjunath Kondaiah <hidden>
Date: 2011-02-04 15:15:32 

On Fri, Feb 04, 2011 at 04:47:09PM +0200, Igor Grinberg wrote:

On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
quoted
On Fri, Feb 04, 2011 at 03:08:47PM +0100, Wolfram Sang wrote:
quoted
On Fri, Feb 04, 2011 at 07:02:50PM +0530, G, Manjunath Kondaiah wrote:
quoted
On Thu, Feb 03, 2011 at 09:19:53AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:54:05AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
quoted
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 <redacted>
Cc: Dmitry Torokhov <redacted>
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?
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.
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.

-Manjunath

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: Igor Grinberg <hidden>
Date: 2011-02-04 15:37:29 

Hi,

On 02/04/11 17:15, G, Manjunath Kondaiah wrote:

On Fri, Feb 04, 2011 at 04:47:09PM +0200, Igor Grinberg wrote:
quoted
On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
quoted
On Fri, Feb 04, 2011 at 03:08:47PM +0100, Wolfram Sang wrote:
quoted
On Fri, Feb 04, 2011 at 07:02:50PM +0530, G, Manjunath Kondaiah wrote:
quoted
On Thu, Feb 03, 2011 at 09:19:53AM -0800, Dmitry Torokhov wrote:
quoted
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.
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.


-- 
Regards,
Igor.

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: dmitry.torokhov@gmail.com (Dmitry Torokhov)
Date: 2011-02-04 16:09:12 

Hi,

On Fri, Feb 04, 2011 at 05:37:29PM +0200, Igor Grinberg wrote:
Hi,

On 02/04/11 17:15, G, Manjunath Kondaiah wrote:
quoted
On Fri, Feb 04, 2011 at 04:47:09PM +0200, Igor Grinberg wrote:
quoted
On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
quoted
On Fri, Feb 04, 2011 at 03:08:47PM +0100, Wolfram Sang wrote:
quoted
On Fri, Feb 04, 2011 at 07:02:50PM +0530, G, Manjunath Kondaiah wrote:
quoted
On Thu, Feb 03, 2011 at 09:19:53AM -0800, Dmitry Torokhov wrote:
quoted
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.
I kept 2 patches because they solve 2 different problems.
quoted
quoted
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.
I am pretty sure I expanded on the scope of the change in the body of the
changelog.

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.
The both patches are already in my public branch so patch description is
set.

Thanks.

-- 
Dmitry

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: Wolfram Sang <hidden>
Date: 2011-02-04 14:54:38 

On Fri, Feb 04, 2011 at 07:46:18PM +0530, G, Manjunath Kondaiah wrote:
On Fri, Feb 04, 2011 at 03:08:47PM +0100, Wolfram Sang wrote:
quoted
On Fri, Feb 04, 2011 at 07:02:50PM +0530, G, Manjunath Kondaiah wrote:
quoted
On Thu, Feb 03, 2011 at 09:19:53AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:54:05AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
quoted
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 <redacted>
Cc: Dmitry Torokhov <redacted>
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?
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
I understood. Exactly this is discussed in the first paragraph of that
link.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110204/3be7e4aa/attachment.sig>

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: Igor Grinberg <hidden>
Date: 2011-02-04 15:13:01 

Hi Dmitry,

On Thu, Feb 03, 2011 at 08:54:05AM -0800, Dmitry Torokhov wrote:
quoted
On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
quoted
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 <redacted>
Cc: Dmitry Torokhov <redacted>
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.
Personally, I don't like the "if{}else if{}else{}" stuff and prefer spartan
programming techniques instead, but the "if-else" below still looks ok,
having a single success exit point is important and everybody is fine
with that approach - then I'm fine with it too.

Thanks for hopping in :)
quoted hunk
Dmitry


Input: ads7846 - check proper condition when freeing gpio

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

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 <redacted>
Signed-off-by: Dmitry Torokhov <redacted>
---

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


diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 0e9492d..b1217e1 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -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);


-- 
Regards,
Igor.

[PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio

From: Wolfram Sang <hidden>
Date: 2011-02-03 17:05:07 

On Thu, Feb 03, 2011 at 08:51:46PM +0530, Sourav Poddar wrote:
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 <redacted>
Cc: Dmitry Torokhov <redacted>
Since the two are won't be merged

Acked-by: Wolfram Sang <redacted>

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110203/5006f063/attachment.sig>
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help