ads7846: OMAP3: Removal of warnings backtrace in bootup

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

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/input/touchscreen/ads7846.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/input/touchscreen/ads7846.c +0 -0

index 14ea54b..036f245 100644

drivers/input/touchscreen/ads7846.c +0 -0

 	mutex_init(&ts->lock);
 	init_waitqueue_head(&ts->wait);

Hi,

Quoted 50 lines · Sourav Poddar

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

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/input/touchscreen/ads7846.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/input/touchscreen/ads7846.c +0 -0

index 14ea54b..036f245 100644

drivers/input/touchscreen/ads7846.c static int __devinit ads7846_probe(struct spi_device *spi) · line 1224

Quoted 4 lines

drivers/input/touchscreen/ads7846.c +1 -0

@@ -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?

Quoted 3 lines

drivers/input/touchscreen/ads7846.c +0 -0

 	mutex_init(&ts->lock);
 	init_waitqueue_head(&ts->wait);

Hi again,

Quoted 62 lines · Igor Grinberg

Hi,

Quoted 50 lines · Sourav Poddar

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

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
?drivers/input/touchscreen/ads7846.c | ? ?1 +
?1 files changed, 1 insertions(+), 0 deletions(-)

drivers/input/touchscreen/ads7846.c +0 -0

index 14ea54b..036f245 100644

drivers/input/touchscreen/ads7846.c static int __devinit ads7846_probe(struct spi_device *spi) · line 1224

Quoted 4 lines

drivers/input/touchscreen/ads7846.c +1 -0

@@ -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:

Quoted 5 lines

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;
}

Quoted 3 lines

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);

Quoted 5 lines · Igor Grinberg

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.

Quoted 2 lines

Quoted 2 lines

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);

Quoted 9 lines · Sourav

Quoted 5 lines · Igor Grinberg

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.

Quoted 9 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 21 lines · Igor Grinberg

Quoted 18 lines · Sourav

Quoted 5 lines · Igor Grinberg

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.

Quoted 2 lines

Quoted 2 lines

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);

Quoted 4 lines

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.

Quoted 9 lines · Sourav

Quoted 5 lines · Igor Grinberg

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.

Quoted 9 lines

Quoted 2 lines

Quoted 2 lines

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?

Hi,

Quoted 23 lines · Igor Grinberg

Quoted 18 lines · Sourav

Quoted 5 lines · Igor Grinberg

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.

Quoted 2 lines

Quoted 2 lines

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);

Quoted 5 lines

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.