* [PATCHv2 1/2] ads7846: OMAP3: Removal of warnings backtrace in bootup @ 2011-02-03 9:48 Sourav Poddar 2011-02-03 10:10 ` Igor Grinberg 0 siblings, 1 reply; 10+ messages in thread From: Sourav Poddar @ 2011-02-03 9:48 UTC (permalink / raw) To: dmitry.torokhov Cc: linux-omap, linux-arm-kernel, charu, gadiyar, linux-input, balbi, Sourav Poddar, Kishon Vijay Abraham I gpio_pendown in ads7846_probe is not getting initalized (defaulted to 0) resulting in gpio_free to be called without a gpio_request. This results in the following backtrace in bootup. ------------[ 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCHv2 1/2] ads7846: OMAP3: Removal of warnings backtrace in bootup 2011-02-03 9:48 [PATCHv2 1/2] ads7846: OMAP3: Removal of warnings backtrace in bootup Sourav Poddar @ 2011-02-03 10:10 ` Igor Grinberg 2011-02-03 10:17 ` Igor Grinberg 0 siblings, 1 reply; 10+ messages in thread From: Igor Grinberg @ 2011-02-03 10:10 UTC (permalink / raw) To: Sourav Poddar Cc: dmitry.torokhov, balbi, Kishon Vijay Abraham I, charu, linux-input, linux-omap, linux-arm-kernel, gadiyar Hi, On 02/03/11 11:48, Sourav Poddar wrote: > gpio_pendown in ads7846_probe is not getting initalized (defaulted to 0) > resulting in gpio_free to be called without a gpio_request. This > results in the following backtrace in bootup. > > ------------[ 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; Wouldn't it be better putting this into ads7846_setup_pendown() function? This will keep the whole gpio_pendown initialization code close together. Something like: if (pdata->get_pendown_state) { <http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/input/touchscreen/ads7846.c;h=14ea54b78e4648b0ac8a3403440f6f5d120dfdd9;hb=HEAD#l954> ts->get_pendown_state = pdata->get_pendown_state; ts->gpio_pendown = -1; <http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/input/touchscreen/ads7846.c;h=14ea54b78e4648b0ac8a3403440f6f5d120dfdd9;hb=HEAD#l955> return 0; <http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/input/touchscreen/ads7846.c;h=14ea54b78e4648b0ac8a3403440f6f5d120dfdd9;hb=HEAD#l956>} Also, why don't we use -EINVAL for the invalid gpio number instead of -1 constant? > > mutex_init(&ts->lock); > init_waitqueue_head(&ts->wait); -- Regards, Igor. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2 1/2] ads7846: OMAP3: Removal of warnings backtrace in bootup 2011-02-03 10:10 ` Igor Grinberg @ 2011-02-03 10:17 ` Igor Grinberg 2011-02-03 11:00 ` Poddar, Sourav 0 siblings, 1 reply; 10+ messages in thread From: Igor Grinberg @ 2011-02-03 10:17 UTC (permalink / raw) To: Sourav Poddar Cc: dmitry.torokhov, balbi, Kishon Vijay Abraham I, charu, linux-input, linux-omap, linux-arm-kernel, gadiyar Hi again, On 02/03/11 12:10, Igor Grinberg wrote: > Hi, > > On 02/03/11 11:48, Sourav Poddar wrote: > >> gpio_pendown in ads7846_probe is not getting initalized (defaulted to 0) >> resulting in gpio_free to be called without a gpio_request. This >> results in the following backtrace in bootup. >> >> ------------[ 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; > Wouldn't it be better putting this into ads7846_setup_pendown() function? > This will keep the whole gpio_pendown initialization code close together. > Something like: > > if (pdata->get_pendown_state) { > <http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/input/touchscreen/ads7846.c;h=14ea54b78e4648b0ac8a3403440f6f5d120dfdd9;hb=HEAD#l954> ts->get_pendown_state = pdata->get_pendown_state; > ts->gpio_pendown = -1; > <http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/input/touchscreen/ads7846.c;h=14ea54b78e4648b0ac8a3403440f6f5d120dfdd9;hb=HEAD#l955> return 0; > <http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/input/touchscreen/ads7846.c;h=14ea54b78e4648b0ac8a3403440f6f5d120dfdd9;hb=HEAD#l956>} Sorry for the above links (this is embarrassing, but I've got played by my email client), I meant of course: if (pdata->get_pendown_state) { ts->get_pendown_state = pdata->get_pendown_state; ts->gpio_pendown = -1; return 0; } > > Also, why don't we use -EINVAL for the invalid gpio number instead of -1 constant? > >> >> mutex_init(&ts->lock); >> init_waitqueue_head(&ts->wait); -- Regards, Igor. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2 1/2] ads7846: OMAP3: Removal of warnings backtrace in bootup 2011-02-03 10:17 ` Igor Grinberg @ 2011-02-03 11:00 ` Poddar, Sourav 2011-02-03 11:09 ` Felipe Balbi 2011-02-03 11:12 ` Igor Grinberg 0 siblings, 2 replies; 10+ messages in thread From: Poddar, Sourav @ 2011-02-03 11:00 UTC (permalink / raw) To: Igor Grinberg Cc: dmitry.torokhov, balbi, Kishon Vijay Abraham I, charu, linux-input, linux-omap, linux-arm-kernel, gadiyar On Thu, Feb 3, 2011 at 3:47 PM, Igor Grinberg <grinberg@compulab.co.il> wrote: > Hi again, > > On 02/03/11 12:10, Igor Grinberg wrote: > >> Hi, >> >> On 02/03/11 11:48, Sourav Poddar wrote: >> >>> gpio_pendown in ads7846_probe is not getting initalized (defaulted to 0) >>> resulting in gpio_free to be called without a gpio_request. This >>> results in the following backtrace in bootup. >>> >>> ------------[ 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; >> Wouldn't it be better putting this into ads7846_setup_pendown() function? >> This will keep the whole gpio_pendown initialization code close together. >> Something like: >> >> if (pdata->get_pendown_state) { >> <http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/input/touchscreen/ads7846.c;h=14ea54b78e4648b0ac8a3403440f6f5d120dfdd9;hb=HEAD#l954> ts->get_pendown_state = pdata->get_pendown_state; >> ts->gpio_pendown = -1; >> <http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/input/touchscreen/ads7846.c;h=14ea54b78e4648b0ac8a3403440f6f5d120dfdd9;hb=HEAD#l955> return 0; >> <http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/input/touchscreen/ads7846.c;h=14ea54b78e4648b0ac8a3403440f6f5d120dfdd9;hb=HEAD#l956>} > > Sorry for the above links (this is embarrassing, but I've got played by my email client), > I meant of course: > > if (pdata->get_pendown_state) { > ts->get_pendown_state = pdata->get_pendown_state; > ts->gpio_pendown = -1; > return 0; > } > Yes we can do so .I initialise it at a place where other variables where initialised. >> >> Also, why don't we use -EINVAL for the invalid gpio number instead of -1 constant? >> I used -1 because conditional check done in probe ads7846_probe function used this value. err_free_gpio: if (ts->gpio_pendown != -1) gpio_free(ts->gpio_pendown); -- 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] 10+ messages in thread
* Re: [PATCHv2 1/2] ads7846: OMAP3: Removal of warnings backtrace in bootup 2011-02-03 11:00 ` Poddar, Sourav @ 2011-02-03 11:09 ` Felipe Balbi 2011-02-03 11:12 ` Igor Grinberg 1 sibling, 0 replies; 10+ messages in thread From: Felipe Balbi @ 2011-02-03 11:09 UTC (permalink / raw) To: Poddar, Sourav Cc: Igor Grinberg, dmitry.torokhov, balbi, Kishon Vijay Abraham I, charu, linux-input, linux-omap, linux-arm-kernel, gadiyar Hi, On Thu, Feb 03, 2011 at 04:30:29PM +0530, Poddar, Sourav wrote: > I used -1 because conditional check done in probe ads7846_probe function > used this value. > > err_free_gpio: > if (ts->gpio_pendown != -1) > gpio_free(ts->gpio_pendown); you can actually change that (maybe on a later patch) to gpio_is_valid() -- balbi ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2 1/2] ads7846: OMAP3: Removal of warnings backtrace in bootup 2011-02-03 11:00 ` Poddar, Sourav 2011-02-03 11:09 ` Felipe Balbi @ 2011-02-03 11:12 ` Igor Grinberg 2011-02-03 11:28 ` ABRAHAM, KISHON VIJAY 2011-02-03 11:32 ` Lothar Waßmann 1 sibling, 2 replies; 10+ messages in thread From: Igor Grinberg @ 2011-02-03 11:12 UTC (permalink / raw) To: Poddar, Sourav Cc: dmitry.torokhov, balbi, Kishon Vijay Abraham I, charu, linux-input, linux-omap, linux-arm-kernel, gadiyar On 02/03/11 13:00, Poddar, Sourav wrote: > On Thu, Feb 3, 2011 at 3:47 PM, Igor Grinberg <grinberg@compulab.co.il> wrote: >> if (pdata->get_pendown_state) { >> ts->get_pendown_state = pdata->get_pendown_state; >> ts->gpio_pendown = -1; >> return 0; >> } > Yes we can do so .I initialise it at a place where other variables > where initialised. > >>> Also, why don't we use -EINVAL for the invalid gpio number instead of -1 constant? >>> > I used -1 because conditional check done in probe ads7846_probe function > used this value. > > err_free_gpio: > if (ts->gpio_pendown != -1) > gpio_free(ts->gpio_pendown); > Well I understand that and that's why in my proposal I used -1 also, but I thought we can make it even better if we switch to -EINVAL (though wanted to check if there are any reasonable objections) and while you are at this, may be you are willing also to submit a patch for this? -- Regards, Igor. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2 1/2] ads7846: OMAP3: Removal of warnings backtrace in bootup 2011-02-03 11:12 ` Igor Grinberg @ 2011-02-03 11:28 ` ABRAHAM, KISHON VIJAY 2011-02-03 13:00 ` Igor Grinberg 2011-02-03 11:32 ` Lothar Waßmann 1 sibling, 1 reply; 10+ messages in thread From: ABRAHAM, KISHON VIJAY @ 2011-02-03 11:28 UTC (permalink / raw) To: Igor Grinberg Cc: Poddar, Sourav, dmitry.torokhov, balbi, charu, linux-input, linux-omap, linux-arm-kernel, gadiyar On Thu, Feb 3, 2011 at 4:42 PM, Igor Grinberg <grinberg@compulab.co.il> wrote: > > > On 02/03/11 13:00, Poddar, Sourav wrote: > >> On Thu, Feb 3, 2011 at 3:47 PM, Igor Grinberg <grinberg@compulab.co.il> wrote: >>> if (pdata->get_pendown_state) { >>> ts->get_pendown_state = pdata->get_pendown_state; >>> ts->gpio_pendown = -1; >>> return 0; >>> } >> Yes we can do so .I initialise it at a place where other variables >> where initialised. >> >>>> Also, why don't we use -EINVAL for the invalid gpio number instead of -1 constant? >>>> >> I used -1 because conditional check done in probe ads7846_probe function >> used this value. >> >> err_free_gpio: >> if (ts->gpio_pendown != -1) >> gpio_free(ts->gpio_pendown); >> > > Well I understand that and that's why in my proposal I used -1 also, but > I thought we can make it even better if we switch to -EINVAL > (though wanted to check if there are any reasonable objections) > and while you are at this, may be you are willing also to submit a patch for this? I guess instead of -EINVAL, -EIO should be initialized to ts->gpio_pendown since that would be more appropriate for gpio and as Balbi suggested it would be better to use gpio_is_valid() for checking this error condition. -Kishon > > -- > Regards, > Igor. > > -- 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] 10+ messages in thread
* Re: [PATCHv2 1/2] ads7846: OMAP3: Removal of warnings backtrace in bootup 2011-02-03 11:28 ` ABRAHAM, KISHON VIJAY @ 2011-02-03 13:00 ` Igor Grinberg 0 siblings, 0 replies; 10+ messages in thread From: Igor Grinberg @ 2011-02-03 13:00 UTC (permalink / raw) To: ABRAHAM, KISHON VIJAY Cc: gadiyar, dmitry.torokhov, balbi, linux-input, Poddar, Sourav, linux-omap, linux-arm-kernel, charu, LW Hi, On 02/03/11 13:28, ABRAHAM, KISHON VIJAY wrote: > On Thu, Feb 3, 2011 at 4:42 PM, Igor Grinberg <grinberg@compulab.co.il> wrote: >> >> On 02/03/11 13:00, Poddar, Sourav wrote: >> >>> On Thu, Feb 3, 2011 at 3:47 PM, Igor Grinberg <grinberg@compulab.co.il> wrote: >>>> if (pdata->get_pendown_state) { >>>> ts->get_pendown_state = pdata->get_pendown_state; >>>> ts->gpio_pendown = -1; >>>> return 0; >>>> } >>> Yes we can do so .I initialise it at a place where other variables >>> where initialised. >>> >>>>> Also, why don't we use -EINVAL for the invalid gpio number instead of -1 constant? >>>>> >>> I used -1 because conditional check done in probe ads7846_probe function >>> used this value. >>> >>> err_free_gpio: >>> if (ts->gpio_pendown != -1) >>> gpio_free(ts->gpio_pendown); >>> >> Well I understand that and that's why in my proposal I used -1 also, but >> I thought we can make it even better if we switch to -EINVAL >> (though wanted to check if there are any reasonable objections) >> and while you are at this, may be you are willing also to submit a patch for this? > I guess instead of -EINVAL, -EIO should be initialized to > ts->gpio_pendown since that > would be more appropriate for gpio Well, the common practice is to use -EINVAL for gpio _numbers_, I have not seen anyone using -EIO for this. > and as Balbi suggested it would > be better to use > gpio_is_valid() for checking this error condition. Of course gpio_is_valid() should be used for testing the gpio. The -EINVAL in turn should be used for _setting_ the invalid value for that gpio. My suggestion for -EINVAL is related to the initialization: if (pdata->get_pendown_state) { ts->get_pendown_state = pdata->get_pendown_state; ts->gpio_pendown = -EINVAL; return 0; } -- Regards, Igor. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2 1/2] ads7846: OMAP3: Removal of warnings backtrace in bootup 2011-02-03 11:12 ` Igor Grinberg 2011-02-03 11:28 ` ABRAHAM, KISHON VIJAY @ 2011-02-03 11:32 ` Lothar Waßmann 2011-02-03 11:40 ` Poddar, Sourav 1 sibling, 1 reply; 10+ messages in thread From: Lothar Waßmann @ 2011-02-03 11:32 UTC (permalink / raw) To: Igor Grinberg Cc: dmitry.torokhov, balbi, Kishon Vijay Abraham I, charu, linux-input, Poddar, Sourav, linux-omap, linux-arm-kernel, gadiyar Hi, Igor Grinberg writes: > > > On 02/03/11 13:00, Poddar, Sourav wrote: > > > On Thu, Feb 3, 2011 at 3:47 PM, Igor Grinberg <grinberg@compulab.co.il> wrote: > >> if (pdata->get_pendown_state) { > >> ts->get_pendown_state = pdata->get_pendown_state; > >> ts->gpio_pendown = -1; > >> return 0; > >> } > > Yes we can do so .I initialise it at a place where other variables > > where initialised. > > > >>> Also, why don't we use -EINVAL for the invalid gpio number instead of -1 constant? > >>> > > I used -1 because conditional check done in probe ads7846_probe function > > used this value. > > > > err_free_gpio: > > if (ts->gpio_pendown != -1) > > gpio_free(ts->gpio_pendown); > > > > Well I understand that and that's why in my proposal I used -1 also, but > I thought we can make it even better if we switch to -EINVAL > (though wanted to check if there are any reasonable objections) > and while you are at this, may be you are willing also to submit a patch for this? > Since ts->gpio_pendown is used as a GPIO number, the check with gpio_is_valid(), as suggested by Felipe Balbi, would be the most sensible thing to do here. Lothar Waßmann -- ___________________________________________________________ Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10 Geschäftsführer: Matthias Kaussen Handelsregistereintrag: Amtsgericht Aachen, HRB 4996 www.karo-electronics.de | info@karo-electronics.de ___________________________________________________________ _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2 1/2] ads7846: OMAP3: Removal of warnings backtrace in bootup 2011-02-03 11:32 ` Lothar Waßmann @ 2011-02-03 11:40 ` Poddar, Sourav 0 siblings, 0 replies; 10+ messages in thread From: Poddar, Sourav @ 2011-02-03 11:40 UTC (permalink / raw) To: Lothar Waßmann Cc: Igor Grinberg, gadiyar, dmitry.torokhov, balbi, Kishon Vijay Abraham I, linux-input, linux-omap, linux-arm-kernel, charu On Thu, Feb 3, 2011 at 5:02 PM, Lothar Waßmann <LW@karo-electronics.de> wrote: > Hi, > > Igor Grinberg writes: >> >> >> On 02/03/11 13:00, Poddar, Sourav wrote: >> >> > On Thu, Feb 3, 2011 at 3:47 PM, Igor Grinberg <grinberg@compulab.co.il> wrote: >> >> if (pdata->get_pendown_state) { >> >> ts->get_pendown_state = pdata->get_pendown_state; >> >> ts->gpio_pendown = -1; >> >> return 0; >> >> } >> > Yes we can do so .I initialise it at a place where other variables >> > where initialised. >> > >> >>> Also, why don't we use -EINVAL for the invalid gpio number instead of -1 constant? >> >>> >> > I used -1 because conditional check done in probe ads7846_probe function >> > used this value. >> > >> > err_free_gpio: >> > if (ts->gpio_pendown != -1) >> > gpio_free(ts->gpio_pendown); >> > >> >> Well I understand that and that's why in my proposal I used -1 also, but >> I thought we can make it even better if we switch to -EINVAL >> (though wanted to check if there are any reasonable objections) >> and while you are at this, may be you are willing also to submit a patch for this? >> > Since ts->gpio_pendown is used as a GPIO number, the check with > gpio_is_valid(), as suggested by Felipe Balbi, would be the most > sensible thing to do here. > Yes,it seems gpio_is_valid() would be more sensible to use. Will post a patch using gpio_is_valid() asap. -- 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] 10+ messages in thread
end of thread, other threads:[~2011-02-03 13:00 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-02-03 9:48 [PATCHv2 1/2] ads7846: OMAP3: Removal of warnings backtrace in bootup Sourav Poddar 2011-02-03 10:10 ` Igor Grinberg 2011-02-03 10:17 ` Igor Grinberg 2011-02-03 11:00 ` Poddar, Sourav 2011-02-03 11:09 ` Felipe Balbi 2011-02-03 11:12 ` Igor Grinberg 2011-02-03 11:28 ` ABRAHAM, KISHON VIJAY 2011-02-03 13:00 ` Igor Grinberg 2011-02-03 11:32 ` Lothar Waßmann 2011-02-03 11:40 ` Poddar, Sourav
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).