Varadarajan, Charulatha wrote:
> Sourav,
>
> On Wed, Feb 2, 2011 at 21:00, Sourav Poddar <sourav.pod...@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.pod...@ti.com>
> > Signed-off-by: Kishon Vijay Abraham I <kis...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
