From: Igor Grinberg <grinberg@compulab.co.il> To: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: Sourav Poddar <sourav.poddar@ti.com>, LW@karo-electronics.de, linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-input@vger.kernel.org, gadiyar@ti.com, charu@ti.com, balbi@ti.com Subject: Re: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio Date: Fri, 04 Feb 2011 17:13:01 +0200 [thread overview] Message-ID: <4D4C177D.1040605@compulab.co.il> (raw) In-Reply-To: <20110203171953.GA13997@core.coreip.homeip.net> 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.
WARNING: multiple messages have this Message-ID (diff)
From: grinberg@compulab.co.il (Igor Grinberg) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio Date: Fri, 04 Feb 2011 17:13:01 +0200 [thread overview] Message-ID: <4D4C177D.1040605@compulab.co.il> (raw) In-Reply-To: <20110203171953.GA13997@core.coreip.homeip.net> 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.
next prev parent reply other threads:[~2011-02-04 15:13 UTC|newest] Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-02-03 15:21 [PATCH v3 2/2] Input: ads7846: use gpio_request_one to configure pendown_gpio Sourav Poddar 2011-02-03 15:21 ` Sourav Poddar 2011-02-03 16:54 ` Dmitry Torokhov 2011-02-03 16:54 ` Dmitry Torokhov 2011-02-03 17:19 ` Dmitry Torokhov 2011-02-03 17:19 ` Dmitry Torokhov 2011-02-03 22:12 ` Wolfram Sang 2011-02-03 22:12 ` Wolfram Sang 2011-02-04 8:05 ` Varadarajan, Charulatha 2011-02-04 8:05 ` Varadarajan, Charulatha 2011-02-04 12:59 ` Poddar, Sourav 2011-02-04 12:59 ` Poddar, Sourav 2011-02-04 13:32 ` G, Manjunath Kondaiah 2011-02-04 13:32 ` G, Manjunath Kondaiah 2011-02-04 13:37 ` Kishore Kadiyala 2011-02-04 13:37 ` Kishore Kadiyala 2011-02-04 13:41 ` G, Manjunath Kondaiah 2011-02-04 13:41 ` G, Manjunath Kondaiah 2011-02-04 14:08 ` Wolfram Sang 2011-02-04 14:08 ` Wolfram Sang 2011-02-04 14:16 ` G, Manjunath Kondaiah 2011-02-04 14:16 ` G, Manjunath Kondaiah 2011-02-04 14:47 ` Igor Grinberg 2011-02-04 14:47 ` Igor Grinberg 2011-02-04 15:11 ` Poddar, Sourav 2011-02-04 15:11 ` Poddar, Sourav 2011-02-04 15:30 ` Igor Grinberg 2011-02-04 15:30 ` Igor Grinberg 2011-02-05 6:59 ` Poddar, Sourav 2011-02-05 6:59 ` Poddar, Sourav 2011-02-06 7:31 ` Igor Grinberg 2011-02-06 7:31 ` Igor Grinberg 2011-02-04 15:15 ` G, Manjunath Kondaiah 2011-02-04 15:15 ` G, Manjunath Kondaiah 2011-02-04 15:37 ` Igor Grinberg 2011-02-04 15:37 ` Igor Grinberg 2011-02-04 16:09 ` Dmitry Torokhov 2011-02-04 16:09 ` Dmitry Torokhov 2011-02-04 14:54 ` Wolfram Sang 2011-02-04 14:54 ` Wolfram Sang 2011-02-04 15:13 ` Igor Grinberg [this message] 2011-02-04 15:13 ` Igor Grinberg 2011-02-03 17:05 ` Wolfram Sang 2011-02-03 17:05 ` Wolfram Sang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4D4C177D.1040605@compulab.co.il \ --to=grinberg@compulab.co.il \ --cc=LW@karo-electronics.de \ --cc=balbi@ti.com \ --cc=charu@ti.com \ --cc=dmitry.torokhov@gmail.com \ --cc=gadiyar@ti.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-input@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=sourav.poddar@ti.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.