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

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

[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 <sourav.poddar@ti.com>
Cc: Dmitry Torokhov <dtor@mail.ru>

---
 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

[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 <sourav.poddar@ti.com>
Cc: Dmitry Torokhov <dtor@mail.ru>

---
 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

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

[Dmitry Torokhov]

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 <sourav.poddar@ti.com>
> Cc: Dmitry Torokhov <dtor@mail.ru>

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

[Dmitry Torokhov]

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 <sourav.poddar@ti.com>
> Cc: Dmitry Torokhov <dtor@mail.ru>

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

Thanks.

-- 
Dmitry

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

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 <sourav.poddar@ti.com>
> Cc: Dmitry Torokhov <dtor@mail.ru>

Since the two are won't be merged

Acked-by: Wolfram Sang <w.sang@pengutronix.de>

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Wolfram Sang]

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 <sourav.poddar@ti.com>
> Cc: Dmitry Torokhov <dtor@mail.ru>

Since the two are won't be merged

Acked-by: Wolfram Sang <w.sang@pengutronix.de>

-- 
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>

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

[Dmitry Torokhov]

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:
> > 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 <sourav.poddar@ti.com>
> > Cc: Dmitry Torokhov <dtor@mail.ru>
> 
> 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 <sourav.poddar@ti.com>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 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

[Dmitry Torokhov]

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:
> > 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 <sourav.poddar@ti.com>
> > Cc: Dmitry Torokhov <dtor@mail.ru>
> 
> 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 <sourav.poddar@ti.com>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 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);

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

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:
> > 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 <sourav.poddar@ti.com>
> > > Cc: Dmitry Torokhov <dtor@mail.ru>
> > 
> > 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 <w.sang@pengutronix.de>

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Wolfram Sang]

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:
> > 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 <sourav.poddar@ti.com>
> > > Cc: Dmitry Torokhov <dtor@mail.ru>
> > 
> > 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 <w.sang@pengutronix.de>

-- 
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>

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

[Varadarajan, Charulatha]

On Thu, Feb 3, 2011 at 22:49, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 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:
>> > 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 <sourav.poddar@ti.com>
>> > Cc: Dmitry Torokhov <dtor@mail.ru>
>>
>> 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 <sourav.poddar@ti.com>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>

Reviewed-by: Charulatha V <charu@ti.com>

> ---
>
>  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-input" in
the body of a message to majordomo@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

[Varadarajan, Charulatha]

On Thu, Feb 3, 2011 at 22:49, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 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:
>> > 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 <sourav.poddar@ti.com>
>> > Cc: Dmitry Torokhov <dtor@mail.ru>
>>
>> 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 <sourav.poddar@ti.com>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>

Reviewed-by: Charulatha V <charu@ti.com>

> ---
>
> ?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);
>

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

[Poddar, Sourav]

On Thu, Feb 3, 2011 at 10:49 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> 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:
>> > 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 <sourav.poddar@ti.com>
>> > Cc: Dmitry Torokhov <dtor@mail.ru>
>>
>> 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 <sourav.poddar@ti.com>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---

Acked-by: Sourav Poddar <sourav.poddar@ti.com>

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

[Poddar, Sourav]

On Thu, Feb 3, 2011 at 10:49 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> 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:
>> > 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 <sourav.poddar@ti.com>
>> > Cc: Dmitry Torokhov <dtor@mail.ru>
>>
>> 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 <sourav.poddar@ti.com>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---

Acked-by: Sourav Poddar <sourav.poddar@ti.com>

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

[G, Manjunath Kondaiah]

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:
> > 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 <sourav.poddar@ti.com>
> > > Cc: Dmitry Torokhov <dtor@mail.ru>
> > 
> > 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);


> 
> -- 
> 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 <sourav.poddar@ti.com>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
> 
>  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@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

[G, Manjunath Kondaiah]

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:
> > 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 <sourav.poddar@ti.com>
> > > Cc: Dmitry Torokhov <dtor@mail.ru>
> > 
> > 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);


> 
> -- 
> 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 <sourav.poddar@ti.com>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
> 
>  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

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

[Kishore Kadiyala]

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 <manjugk@ti.com> wrote:
> 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:
>> > 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 <sourav.poddar@ti.com>
>> > > Cc: Dmitry Torokhov <dtor@mail.ru>
>> >
>> > 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);
>
>
>>
>> --
>> 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 <sourav.poddar@ti.com>
>> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
>> ---
>>
>>  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@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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Kishore Kadiyala]

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 <manjugk@ti.com> wrote:
> 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:
>> > 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 <sourav.poddar@ti.com>
>> > > Cc: Dmitry Torokhov <dtor@mail.ru>
>> >
>> > 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);
>
>
>>
>> --
>> 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 <sourav.poddar@ti.com>
>> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
>> ---
>>
>> ?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
>

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

[G, Manjunath Kondaiah]

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

[G, Manjunath Kondaiah]

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1117 bytes --]

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:
> > 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:
> > > > 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 <sourav.poddar@ti.com>
> > > > Cc: Dmitry Torokhov <dtor@mail.ru>
> > > 
> > > 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/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Wolfram Sang]

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:
> > 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:
> > > > 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 <sourav.poddar@ti.com>
> > > > Cc: Dmitry Torokhov <dtor@mail.ru>
> > > 
> > > 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>

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

[G, Manjunath Kondaiah]

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:
> > 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:
> > > > 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 <sourav.poddar@ti.com>
> > > > > Cc: Dmitry Torokhov <dtor@mail.ru>
> > > > 
> > > > 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

[G, Manjunath Kondaiah]

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:
> > 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:
> > > > 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 <sourav.poddar@ti.com>
> > > > > Cc: Dmitry Torokhov <dtor@mail.ru>
> > > > 
> > > > 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

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

[Igor Grinberg]

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

> 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:
>>> 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:
>>>>> 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 <sourav.poddar@ti.com>
>>>>>> Cc: Dmitry Torokhov <dtor@mail.ru>
>>>>> 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

[Igor Grinberg]

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

> 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:
>>> 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:
>>>>> 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 <sourav.poddar@ti.com>
>>>>>> Cc: Dmitry Torokhov <dtor@mail.ru>
>>>>> 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.

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1583 bytes --]

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:
> > 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:
> > > > 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:
> > > > > > 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 <sourav.poddar@ti.com>
> > > > > > Cc: Dmitry Torokhov <dtor@mail.ru>
> > > > > 
> > > > > 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/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Wolfram Sang]

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:
> > 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:
> > > > 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:
> > > > > > 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 <sourav.poddar@ti.com>
> > > > > > Cc: Dmitry Torokhov <dtor@mail.ru>
> > > > > 
> > > > > 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>

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

[Poddar, Sourav]

On Fri, Feb 4, 2011 at 8:17 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>
>
> On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
>
>> 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:
>>>> 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:
>>>>>> 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 <sourav.poddar@ti.com>
>>>>>>> Cc: Dmitry Torokhov <dtor@mail.ru>
>>>>>> 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

[Poddar, Sourav]

On Fri, Feb 4, 2011 at 8:17 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>
>
> On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
>
>> 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:
>>>> 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:
>>>>>> 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 <sourav.poddar@ti.com>
>>>>>>> Cc: Dmitry Torokhov <dtor@mail.ru>
>>>>>> 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.
>
>

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

[Igor Grinberg]

Hi Dmitry,

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:
>>> 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 <sourav.poddar@ti.com>
>>> Cc: Dmitry Torokhov <dtor@mail.ru>
>> 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 :)

>
> 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 <sourav.poddar@ti.com>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
>
>  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

[Igor Grinberg]

Hi Dmitry,

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:
>>> 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 <sourav.poddar@ti.com>
>>> Cc: Dmitry Torokhov <dtor@mail.ru>
>> 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 :)

>
> 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 <sourav.poddar@ti.com>
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
>
>  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.

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

[G, Manjunath Kondaiah]

On Fri, Feb 04, 2011 at 04:47:09PM +0200, Igor Grinberg wrote:
> 
> 
> On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
> 
> > 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:
> >>> 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:
> >>>>> 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 <sourav.poddar@ti.com>
> >>>>>> Cc: Dmitry Torokhov <dtor@mail.ru>
> >>>>> 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

[G, Manjunath Kondaiah]

On Fri, Feb 04, 2011 at 04:47:09PM +0200, Igor Grinberg wrote:
> 
> 
> On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
> 
> > 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:
> >>> 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:
> >>>>> 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 <sourav.poddar@ti.com>
> >>>>>> Cc: Dmitry Torokhov <dtor@mail.ru>
> >>>>> 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

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

[Igor Grinberg]

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

> On Fri, Feb 4, 2011 at 8:17 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>
>> On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
>>
>>> 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:
>>>>> 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:
>>>>>>> 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 <sourav.poddar@ti.com>
>>>>>>>> Cc: Dmitry Torokhov <dtor@mail.ru>
>>>>>>> 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

[Igor Grinberg]

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

> On Fri, Feb 4, 2011 at 8:17 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>
>> On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
>>
>>> 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:
>>>>> 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:
>>>>>>> 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 <sourav.poddar@ti.com>
>>>>>>>> Cc: Dmitry Torokhov <dtor@mail.ru>
>>>>>>> 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.

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

[Igor Grinberg]

Hi,

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

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

[Igor Grinberg]

Hi,

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

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

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

[Dmitry Torokhov]

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

> >> 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

[Dmitry Torokhov]

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

> >> 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

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

[Poddar, Sourav]

On Fri, Feb 4, 2011 at 9:00 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>
>
> On 02/04/11 17:11, Poddar, Sourav wrote:
>
>> On Fri, Feb 4, 2011 at 8:17 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>
>>> On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
>>>
>>>> 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:
>>>>>> 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:
>>>>>>>> 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 <sourav.poddar@ti.com>
>>>>>>>>> Cc: Dmitry Torokhov <dtor@mail.ru>
>>>>>>>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@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

[Poddar, Sourav]

On Fri, Feb 4, 2011 at 9:00 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>
>
> On 02/04/11 17:11, Poddar, Sourav wrote:
>
>> On Fri, Feb 4, 2011 at 8:17 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>
>>> On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
>>>
>>>> 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:
>>>>>> 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:
>>>>>>>> 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 <sourav.poddar@ti.com>
>>>>>>>>> Cc: Dmitry Torokhov <dtor@mail.ru>
>>>>>>>> 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.

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

[Igor Grinberg]

> On Fri, Feb 4, 2011 at 9:00 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>
>> On 02/04/11 17:11, Poddar, Sourav wrote:
>>
>>> On Fri, Feb 4, 2011 at 8:17 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>> On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
>>>>
>>>>> 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:
>>>>>>> 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:
>>>>>>>>> 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

[Igor Grinberg]

> On Fri, Feb 4, 2011 at 9:00 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>
>> On 02/04/11 17:11, Poddar, Sourav wrote:
>>
>>> On Fri, Feb 4, 2011 at 8:17 PM, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>> On 02/04/11 16:16, G, Manjunath Kondaiah wrote:
>>>>
>>>>> 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:
>>>>>>> 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:
>>>>>>>>> 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.