* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-03 17:19 ` Dmitry Torokhov
0 siblings, 0 replies; 44+ messages in thread
From: Dmitry Torokhov @ 2011-02-03 17:19 UTC (permalink / raw)
To: linux-arm-kernel
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);
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-03 17:19 ` Dmitry Torokhov
@ 2011-02-03 22:12 ` Wolfram Sang
-1 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2011-02-03 22:12 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Sourav Poddar, balbi, charu, grinberg, linux-input, linux-omap,
LW, linux-arm-kernel, gadiyar
[-- 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 --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-03 22:12 ` Wolfram Sang
0 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2011-02-03 22:12 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-03 17:19 ` Dmitry Torokhov
@ 2011-02-04 8:05 ` Varadarajan, Charulatha
-1 siblings, 0 replies; 44+ messages in thread
From: Varadarajan, Charulatha @ 2011-02-04 8:05 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Sourav Poddar, LW, linux-omap, linux-arm-kernel, linux-input,
gadiyar, grinberg, balbi
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
^ permalink raw reply [flat|nested] 44+ messages in thread* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-04 8:05 ` Varadarajan, Charulatha
0 siblings, 0 replies; 44+ messages in thread
From: Varadarajan, Charulatha @ 2011-02-04 8:05 UTC (permalink / raw)
To: linux-arm-kernel
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);
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-03 17:19 ` Dmitry Torokhov
@ 2011-02-04 12:59 ` Poddar, Sourav
-1 siblings, 0 replies; 44+ messages in thread
From: Poddar, Sourav @ 2011-02-04 12:59 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: LW, linux-omap, linux-arm-kernel, linux-input, gadiyar, charu,
grinberg, balbi
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>
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-04 12:59 ` Poddar, Sourav
0 siblings, 0 replies; 44+ messages in thread
From: Poddar, Sourav @ 2011-02-04 12:59 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-03 17:19 ` Dmitry Torokhov
@ 2011-02-04 13:32 ` G, Manjunath Kondaiah
-1 siblings, 0 replies; 44+ messages in thread
From: G, Manjunath Kondaiah @ 2011-02-04 13:32 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Sourav Poddar, LW, linux-omap, linux-arm-kernel, linux-input,
gadiyar, charu, grinberg, balbi
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
^ permalink raw reply related [flat|nested] 44+ messages in thread* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-04 13:32 ` G, Manjunath Kondaiah
0 siblings, 0 replies; 44+ messages in thread
From: G, Manjunath Kondaiah @ 2011-02-04 13:32 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 44+ messages in thread* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-04 13:32 ` G, Manjunath Kondaiah
@ 2011-02-04 13:37 ` Kishore Kadiyala
-1 siblings, 0 replies; 44+ messages in thread
From: Kishore Kadiyala @ 2011-02-04 13:37 UTC (permalink / raw)
To: G, Manjunath Kondaiah
Cc: Dmitry Torokhov, Sourav Poddar, LW, linux-omap, linux-arm-kernel,
linux-input, gadiyar, charu, grinberg, balbi
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
^ permalink raw reply [flat|nested] 44+ messages in thread* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-04 13:37 ` Kishore Kadiyala
0 siblings, 0 replies; 44+ messages in thread
From: Kishore Kadiyala @ 2011-02-04 13:37 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-04 13:37 ` Kishore Kadiyala
@ 2011-02-04 13:41 ` G, Manjunath Kondaiah
-1 siblings, 0 replies; 44+ messages in thread
From: G, Manjunath Kondaiah @ 2011-02-04 13:41 UTC (permalink / raw)
To: Kishore Kadiyala
Cc: G, Manjunath Kondaiah, Dmitry Torokhov, Sourav Poddar, LW,
linux-omap, linux-arm-kernel, linux-input, gadiyar, charu,
grinberg, balbi
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-04 13:32 ` G, Manjunath Kondaiah
@ 2011-02-04 14:08 ` Wolfram Sang
-1 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2011-02-04 14:08 UTC (permalink / raw)
To: G, Manjunath Kondaiah
Cc: Dmitry Torokhov, balbi, charu, grinberg, linux-input,
Sourav Poddar, linux-omap, LW, linux-arm-kernel, gadiyar
[-- 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 --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-04 14:08 ` Wolfram Sang
0 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2011-02-04 14:08 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-04 14:08 ` Wolfram Sang
@ 2011-02-04 14:16 ` G, Manjunath Kondaiah
-1 siblings, 0 replies; 44+ messages in thread
From: G, Manjunath Kondaiah @ 2011-02-04 14:16 UTC (permalink / raw)
To: Wolfram Sang
Cc: G, Manjunath Kondaiah, Dmitry Torokhov, balbi, charu, grinberg,
linux-input, Sourav Poddar, linux-omap, LW, linux-arm-kernel,
gadiyar
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-04 14:16 ` G, Manjunath Kondaiah
0 siblings, 0 replies; 44+ messages in thread
From: G, Manjunath Kondaiah @ 2011-02-04 14:16 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-04 14:16 ` G, Manjunath Kondaiah
@ 2011-02-04 14:47 ` Igor Grinberg
-1 siblings, 0 replies; 44+ messages in thread
From: Igor Grinberg @ 2011-02-04 14:47 UTC (permalink / raw)
To: G, Manjunath Kondaiah
Cc: Wolfram Sang, Dmitry Torokhov, balbi, charu, linux-input,
Sourav Poddar, linux-omap, LW, linux-arm-kernel, gadiyar
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-04 14:47 ` Igor Grinberg
0 siblings, 0 replies; 44+ messages in thread
From: Igor Grinberg @ 2011-02-04 14:47 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-04 14:47 ` Igor Grinberg
@ 2011-02-04 15:11 ` Poddar, Sourav
-1 siblings, 0 replies; 44+ messages in thread
From: Poddar, Sourav @ 2011-02-04 15:11 UTC (permalink / raw)
To: Igor Grinberg
Cc: G, Manjunath Kondaiah, gadiyar, Dmitry Torokhov, Wolfram Sang,
balbi, linux-input, linux-omap, LW, linux-arm-kernel, charu
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.
>
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-04 15:11 ` Poddar, Sourav
0 siblings, 0 replies; 44+ messages in thread
From: Poddar, Sourav @ 2011-02-04 15:11 UTC (permalink / raw)
To: linux-arm-kernel
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.
>
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-04 15:11 ` Poddar, Sourav
@ 2011-02-04 15:30 ` Igor Grinberg
-1 siblings, 0 replies; 44+ messages in thread
From: Igor Grinberg @ 2011-02-04 15:30 UTC (permalink / raw)
To: Poddar, Sourav
Cc: G, Manjunath Kondaiah, Wolfram Sang, Dmitry Torokhov, balbi,
charu, linux-input, linux-omap, LW, linux-arm-kernel, gadiyar
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-04 15:30 ` Igor Grinberg
0 siblings, 0 replies; 44+ messages in thread
From: Igor Grinberg @ 2011-02-04 15:30 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-04 15:30 ` Igor Grinberg
@ 2011-02-05 6:59 ` Poddar, Sourav
-1 siblings, 0 replies; 44+ messages in thread
From: Poddar, Sourav @ 2011-02-05 6:59 UTC (permalink / raw)
To: Igor Grinberg
Cc: G, Manjunath Kondaiah, Wolfram Sang, Dmitry Torokhov, balbi,
charu, linux-input, linux-omap, LW, linux-arm-kernel, gadiyar
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-05 6:59 ` Poddar, Sourav
0 siblings, 0 replies; 44+ messages in thread
From: Poddar, Sourav @ 2011-02-05 6:59 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-05 6:59 ` Poddar, Sourav
@ 2011-02-06 7:31 ` Igor Grinberg
-1 siblings, 0 replies; 44+ messages in thread
From: Igor Grinberg @ 2011-02-06 7:31 UTC (permalink / raw)
To: Poddar, Sourav
Cc: G, Manjunath Kondaiah, Wolfram Sang, Dmitry Torokhov, balbi,
charu, linux-input, linux-omap, LW, linux-arm-kernel, gadiyar
> 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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-06 7:31 ` Igor Grinberg
0 siblings, 0 replies; 44+ messages in thread
From: Igor Grinberg @ 2011-02-06 7:31 UTC (permalink / raw)
To: linux-arm-kernel
> 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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-04 14:47 ` Igor Grinberg
@ 2011-02-04 15:15 ` G, Manjunath Kondaiah
-1 siblings, 0 replies; 44+ messages in thread
From: G, Manjunath Kondaiah @ 2011-02-04 15:15 UTC (permalink / raw)
To: Igor Grinberg
Cc: G, Manjunath Kondaiah, Wolfram Sang, Dmitry Torokhov, balbi,
charu, linux-input, Sourav Poddar, linux-omap, LW,
linux-arm-kernel, gadiyar
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-04 15:15 ` G, Manjunath Kondaiah
0 siblings, 0 replies; 44+ messages in thread
From: G, Manjunath Kondaiah @ 2011-02-04 15:15 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-04 15:15 ` G, Manjunath Kondaiah
@ 2011-02-04 15:37 ` Igor Grinberg
-1 siblings, 0 replies; 44+ messages in thread
From: Igor Grinberg @ 2011-02-04 15:37 UTC (permalink / raw)
To: G, Manjunath Kondaiah
Cc: Wolfram Sang, Dmitry Torokhov, balbi, charu, linux-input,
Sourav Poddar, linux-omap, LW, linux-arm-kernel, gadiyar
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-04 15:37 ` Igor Grinberg
0 siblings, 0 replies; 44+ messages in thread
From: Igor Grinberg @ 2011-02-04 15:37 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-04 15:37 ` Igor Grinberg
@ 2011-02-04 16:09 ` Dmitry Torokhov
-1 siblings, 0 replies; 44+ messages in thread
From: Dmitry Torokhov @ 2011-02-04 16:09 UTC (permalink / raw)
To: Igor Grinberg
Cc: G, Manjunath Kondaiah, Wolfram Sang, balbi, charu, linux-input,
Sourav Poddar, linux-omap, LW, linux-arm-kernel, gadiyar
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-04 16:09 ` Dmitry Torokhov
0 siblings, 0 replies; 44+ messages in thread
From: Dmitry Torokhov @ 2011-02-04 16:09 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-04 14:16 ` G, Manjunath Kondaiah
@ 2011-02-04 14:54 ` Wolfram Sang
-1 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2011-02-04 14:54 UTC (permalink / raw)
To: G, Manjunath Kondaiah
Cc: Dmitry Torokhov, balbi, charu, grinberg, linux-input,
Sourav Poddar, linux-omap, LW, linux-arm-kernel, gadiyar
[-- 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 --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-04 14:54 ` Wolfram Sang
0 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2011-02-04 14:54 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
2011-02-03 17:19 ` Dmitry Torokhov
@ 2011-02-04 15:13 ` Igor Grinberg
-1 siblings, 0 replies; 44+ messages in thread
From: Igor Grinberg @ 2011-02-04 15:13 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Sourav Poddar, LW, linux-omap, linux-arm-kernel, linux-input,
gadiyar, charu, balbi
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.
^ permalink raw reply [flat|nested] 44+ messages in thread* [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio
@ 2011-02-04 15:13 ` Igor Grinberg
0 siblings, 0 replies; 44+ messages in thread
From: Igor Grinberg @ 2011-02-04 15:13 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 44+ messages in thread