[PATCH 1/2] ads7846: fix gpio free without requesting

[PATCH 1/2] ads7846: fix gpio free without requesting

[Sourav Poddar]

gpio_pendown in ads7846_probe is not getting initalized (defaulted to 0)
resulting in gpio_free being called without a gpio_request. This
results in the following backtrace in bootup (at least on an OMAP3430 SDP).

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

Initializing gpio_pendown in ads7846_probe to -1 before
ads7846_setup_pendown function removes the above backtrace
warning.

Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/input/touchscreen/ads7846.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 14ea54b..036f245 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -1221,6 +1221,7 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 	ts->input = input_dev;
 	ts->vref_mv = pdata->vref_mv;
 	ts->swap_xy = pdata->swap_xy;
+	ts->gpio_pendown = -1;
 
 	mutex_init(&ts->lock);
 	init_waitqueue_head(&ts->wait);
-- 
1.7.0.4

Re: [PATCH 1/2] ads7846: fix gpio free without requesting

[Varadarajan, Charulatha]

Sourav,

On Wed, Feb 2, 2011 at 21:00, Sourav Poddar <sourav.poddar@ti.com> wrote:
> gpio_pendown in ads7846_probe is not getting initalized (defaulted to 0)
> resulting in gpio_free being called without a gpio_request. This
> results in the following backtrace in bootup (at least on an OMAP3430 SDP).

NACK to this patch. see my comments below.

>
> ------------[ 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 ]---
>
> Initializing gpio_pendown in ads7846_probe to -1 before
> ads7846_setup_pendown function removes the above backtrace
> warning.
>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/input/touchscreen/ads7846.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index 14ea54b..036f245 100644
> --- a/drivers/input/touchscreen/ads7846.c
> +++ b/drivers/input/touchscreen/ads7846.c
> @@ -1221,6 +1221,7 @@ static int __devinit ads7846_probe(struct spi_device *spi)
>        ts->input = input_dev;
>        ts->vref_mv = pdata->vref_mv;
>        ts->swap_xy = pdata->swap_xy;
> +       ts->gpio_pendown = -1;

Going through the code, I see that ts->gpio_pendown is being filled
in ads7846_setup_pendown() which in turn is called by the
ads7846_probe().

Actually in ads7846_dev_init() [board-3430sdp.c], the gpio_pendown
pin has been requested already. The problem is with the following
line in ads7846_probe():
         if (pdata->get_pendown_state) {
                 ts->get_pendown_state = pdata->get_pendown_state;
                 return 0;
         }

Instead it should have been
         if (pdata->get_pendown_state( )) {
                 ts->get_pendown_state = pdata->get_pendown_state;
                 return 0;
         }

Also, filling ts->gpio_pendown = -1 should not be done in the driver file.
Instead pdata should either send a valid GPIO pin number or an -1
if it is not applicable to the board and this has to be done in the board file.

Also in board-3430sdp.c file, I observed that pdata->gpio_pendown
is not filled at all. But the ads7846_setup_pendown() in the driver relies
on pdata->gpio_pendown. This also needs to be fixed.

-V Charulatha
>
>        mutex_init(&ts->lock);
>        init_waitqueue_head(&ts->wait);
> --
> 1.7.0.4
--
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

RE: [PATCH 1/2] ads7846: fix gpio free without requesting

[Anand Gadiyar]

Varadarajan, Charulatha wrote:
> Sourav,
>
> On Wed, Feb 2, 2011 at 21:00, Sourav Poddar <sourav.poddar@ti.com>
wrote:
> > gpio_pendown in ads7846_probe is not getting initalized (defaulted to
0)
> > resulting in gpio_free being called without a gpio_request. This
> > results in the following backtrace in bootup (at least on an OMAP3430
SDP).
>
> NACK to this patch. see my comments below.
>
...

> >
> > Initializing gpio_pendown in ads7846_probe to -1 before
> > ads7846_setup_pendown function removes the above backtrace
> > warning.
> >
> > Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

This series should go to linux-input as well.

> > ---
> >  drivers/input/touchscreen/ads7846.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/ads7846.c
b/drivers/input/touchscreen/ads7846.c
> > index 14ea54b..036f245 100644
> > --- a/drivers/input/touchscreen/ads7846.c
> > +++ b/drivers/input/touchscreen/ads7846.c
> > @@ -1221,6 +1221,7 @@ static int __devinit ads7846_probe(struct
spi_device *spi)
> >        ts->input = input_dev;
> >        ts->vref_mv = pdata->vref_mv;
> >        ts->swap_xy = pdata->swap_xy;
> > +       ts->gpio_pendown = -1;
>
> Going through the code, I see that ts->gpio_pendown is being filled
> in ads7846_setup_pendown() which in turn is called by the
> ads7846_probe().
>
> Actually in ads7846_dev_init() [board-3430sdp.c], the gpio_pendown
> pin has been requested already. The problem is with the following
> line in ads7846_probe():
>          if (pdata->get_pendown_state) {
>                  ts->get_pendown_state = pdata->get_pendown_state;
>                  return 0;
>          }
>
> Instead it should have been
>          if (pdata->get_pendown_state( )) {
>                  ts->get_pendown_state = pdata->get_pendown_state;
>                  return 0;
>          }
>

This is not correct. This section of the code is simply assigning
the function pointer that platform data passed down to the
local structure. So all you need to check is if the platform data
pointer is valid or not.

So that section of the code is correct - you should not call
pdata->get_pendown_state(). It is unnecessary during probe.


> Also, filling ts->gpio_pendown = -1 should not be done in the
> driver file.
> Instead pdata should either send a valid GPIO pin number or an -1
> if it is not applicable to the board and this has to be done
> in the board file.

I'm not very familiar with this driver, but it looks to me like
there is no hard and fast rule that a gpio be passed down from
platform files.

The platform file could just as well pass down a way to get
the pendown state directly (which is what board-3430sdp.c does).

> Also in board-3430sdp.c file, I observed that pdata->gpio_pendown
> is not filled at all. But the ads7846_setup_pendown() in the
> driver relies on pdata->gpio_pendown. This also needs to be fixed.

ads7846_setup_pendown calls the get_pendown_state function pointer
if the platform provided one, or requests the gpio to use if
the platform provided one. The platform could provide either one,
and doesn't necessarily have to provide both (I'm not sure).

ads7846_setup_pendown doesn't populate the ts->gpio_pendown at all,
if there was a get_pendown_state pointer passed. So fixing the
platform files will not be enough. This is the bug that Sourav's
patch appears to fix.

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

Re: [PATCH 1/2] ads7846: fix gpio free without requesting

[Varadarajan, Charulatha]

<<snip>>

>> > diff --git a/drivers/input/touchscreen/ads7846.c
> b/drivers/input/touchscreen/ads7846.c
>> > index 14ea54b..036f245 100644
>> > --- a/drivers/input/touchscreen/ads7846.c
>> > +++ b/drivers/input/touchscreen/ads7846.c
>> > @@ -1221,6 +1221,7 @@ static int __devinit ads7846_probe(struct
> spi_device *spi)
>> >        ts->input = input_dev;
>> >        ts->vref_mv = pdata->vref_mv;
>> >        ts->swap_xy = pdata->swap_xy;
>> > +       ts->gpio_pendown = -1;
>>
>> Going through the code, I see that ts->gpio_pendown is being filled
>> in ads7846_setup_pendown() which in turn is called by the
>> ads7846_probe().
>>
>> Actually in ads7846_dev_init() [board-3430sdp.c], the gpio_pendown
>> pin has been requested already. The problem is with the following
>> line in ads7846_probe():
>>          if (pdata->get_pendown_state) {
>>                  ts->get_pendown_state = pdata->get_pendown_state;
>>                  return 0;
>>          }
>>
>> Instead it should have been
>>          if (pdata->get_pendown_state( )) {
>>                  ts->get_pendown_state = pdata->get_pendown_state;
>>                  return 0;
>>          }
>>
>
> This is not correct. This section of the code is simply assigning
> the function pointer that platform data passed down to the
> local structure. So all you need to check is if the platform data
> pointer is valid or not.
>
> So that section of the code is correct - you should not call
> pdata->get_pendown_state(). It is unnecessary during probe.

Okay.

>
>
>> Also, filling ts->gpio_pendown = -1 should not be done in the
>> driver file.
>> Instead pdata should either send a valid GPIO pin number or an -1
>> if it is not applicable to the board and this has to be done
>> in the board file.
>
> I'm not very familiar with this driver, but it looks to me like
> there is no hard and fast rule that a gpio be passed down from
> platform files.
>
> The platform file could just as well pass down a way to get
> the pendown state directly (which is what board-3430sdp.c does).
>
>> Also in board-3430sdp.c file, I observed that pdata->gpio_pendown
>> is not filled at all. But the ads7846_setup_pendown() in the
>> driver relies on pdata->gpio_pendown. This also needs to be fixed.
>
> ads7846_setup_pendown calls the get_pendown_state function pointer
> if the platform provided one, or requests the gpio to use if
> the platform provided one. The platform could provide either one,
> and doesn't necessarily have to provide both (I'm not sure).
>
> ads7846_setup_pendown doesn't populate the ts->gpio_pendown at all,
> if there was a get_pendown_state pointer passed. So fixing the
> platform files will not be enough. This is the bug that Sourav's
> patch appears to fix.

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