ARM: OMAP2+: ads7846 init: fix fault caused by freeing pen-down GPIO

commit 97ee9f01d6 (ARM: OMAP: fix the ads7846 init code) mistakenly frees the pen-down GPIO even though it will be used by the ads7846 driver.

Freeing a GPIO means that the GPIO bank containing that GPIO can be runtime suspended if its the last/only GPIO being used in that bank. If the GPIO bank is runtime suspended, any accesses to that bank will cause faults.

Because the current code frees the GPIO, the ads7846 driver probe will fault when it requests its IRQ line.

Because the IRQ is a GPIO line,
the request IRQ will trickle down into the OMAP GPIO layer:
gpio_irq_type() --> _set_gpio_triggering() which can fault if the bank has been runtime suspended.

This is exctly what happens on Overo platforms (3530 Water, 3730 Overo FireSTORM) since this is the only GPIO used in the bank.

To fix, don't free the GPIO at all since it is always in use.

Cc Igor Grinberg <[email protected]>

Signed-off-by Kevin Hilman <[email protected]>

---
Tony, this applies on top of your current fixes-non-critical branch and should probably go in to v3.5-rc since the patch which introduced the problem did as well.

 arch/arm/mach-omap2/common-board-devices.c |    3 ---
 1 file changed, 3 deletions(-)

arch/arm/mach-omap2/common-board-devices.c +0 -3

index c187586..1ae6fd6 100644
@@ -84,9 +84,6 @@ void __init omap_ads7846_init(int bus_num, int gpio_pendown, int gpio_debounce,
 		ads7846_config.gpio_pendown = gpio_pendown;
 	}
 
-	if (!board_pdata || (board_pdata && !board_pdata->get_pendown_state))
-		gpio_free(gpio_pendown);
-
 	spi_register_board_info(&ads7846_spi_board_info, 1);
 }
 #else

Quoted 3 lines · Kevin Hilman

commit 97ee9f01d6 (ARM: OMAP: fix the ads7846 init code) mistakenly frees the pen-down GPIO even though it will be used by the ads7846 driver.

But the driver requests the GPIO in ads7846_setup_pendown() method...

Quoted 5 lines

Freeing a GPIO means that the GPIO bank containing that GPIO can be runtime suspended if its the last/only GPIO being used in that bank. If the GPIO bank is runtime suspended, any accesses to that bank will cause faults.

Unless, it is requested again, or am I missing something?

Quoted 3 lines

Because the current code frees the GPIO, the ads7846 driver probe will fault when it requests its IRQ line.

But it requests that GPIO... in ads7846_setup_pendown() prior to requesting the IRQ...

Quoted 12 lines

Because the IRQ is a GPIO line,
the request IRQ will trickle down into the OMAP GPIO layer:
gpio_irq_type() --> _set_gpio_triggering() which can fault if the bank has been runtime suspended.

This is exctly what happens on Overo platforms (3530 Water, 3730 Overo FireSTORM) since this is the only GPIO used in the bank.

To fix, don't free the GPIO at all since it is always in use.

Cc Igor Grinberg <[email protected]>

Signed-off-by Kevin Hilman <[email protected]>

Quoted 4 lines

---
Tony, this applies on top of your current fixes-non-critical branch and should probably go in to v3.5-rc since the patch which introduced the problem did as well.

Quoted 18 lines

 arch/arm/mach-omap2/common-board-devices.c |    3 ---
 1 file changed, 3 deletions(-)

arch/arm/mach-omap2/common-board-devices.c +0 -3

index c187586..1ae6fd6 100644
@@ -84,9 +84,6 @@ void __init omap_ads7846_init(int bus_num, int gpio_pendown, int gpio_debounce,
 		ads7846_config.gpio_pendown = gpio_pendown;
 	}
 
-	if (!board_pdata || (board_pdata && !board_pdata->get_pendown_state))
-		gpio_free(gpio_pendown);
-
 	spi_register_board_info(&ads7846_spi_board_info, 1);
 }
 #else

Hi Kevin,

I've just noticed that the patch has been modified by Arnd in a way that of course will trigger GPIO use without being requested. I'm sorry, I was not available by that time Arnd changed the patch.

From 85516c6a3354967caf4cff434d28c3001cd411eb Mon Sep 17 00:00:00 2001

+ Zumeng Chen

Quoted 5 lines · Igor Grinberg

Hi Kevin,

I've just noticed that the patch has been modified by Arnd in a way that of course will trigger GPIO use without being requested. I'm sorry, I was not available by that time Arnd changed the patch.

Your right, your original patch isn't the problem. I found the root cause.

The real problem is actually introduced by the merge of your patch from the arm-soc/cleanup branch, and this one from Zumeng Chen: commit 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846) from the arm-soc/boards branch.

However, looking closer at the one from Zumeng, that one is clearly not right. It unconditionally adds a *board-specific* ->get_pendown_state function to the pdata that is common to *all* boards. That's just wrong and has the side-effect of making ->get_pendown_state() wrong on every board except the OMAP3EVM. Oops.

So, IMO, in addition to $SUBJECT patch, in order to get the touchscreen GPIO working on non OMAP3EVM boards, we also need something like this as well.

Igor, Zumeng, could you try this out on your boards anc confirm if it's working? I currently don't have a board setup with a touchscreen in my board farm.

Kevin

From 85516c6a3354967caf4cff434d28c3001cd411eb Mon Sep 17 00:00:00 2001
From: Kevin Hilman<[email protected]>
Date: Thu, 26 Jul 2012 12:15:38 -0700
Subject: [PATCH 2/2] ARM: OMAP2+: ads7846: fix ->get_pendown_state() to work
on all boards

commit 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846) introduced a common ->get_pendown_state() function into the generic code, but that function was board-specific for the OMAP3EVM.

Fix this up to work for all boards that pass in a pendown GPIO.

Cc Zumeng Chen<[email protected]>

Cc Igor Grinberg<[email protected]>

Signed-off-by Kevin Hilman<[email protected]>

---
  arch/arm/mach-omap2/board-omap3evm.c       |    1 +
  arch/arm/mach-omap2/common-board-devices.c |    4 +++-
  arch/arm/mach-omap2/common-board-devices.h |    1 -
  3 files changed, 4 insertions(+), 2 deletions(-)

arch/arm/mach-omap2/board-omap3evm.c +1 -0

index ef230a0..0d362e9 100644
@@ -58,6 +58,7 @@
  #include "hsmmc.h"
  #include "common-board-devices.h"
 
+#define OMAP3_EVM_TS_GPIO	175
  #define OMAP3_EVM_EHCI_VBUS	22
  #define OMAP3_EVM_EHCI_SELECT	61
 

arch/arm/mach-omap2/common-board-devices.c +3 -1

index 0fee95f..06f176f 100644
@@ -40,9 +40,10 @@ static struct omap2_mcspi_device_config ads7846_mcspi_config = {
   * of pdata->get_pendown_state, but we have done this. So set
   * get_pendown_state to avoid twice gpio requesting.
   */
+static int omap3_gpio_pendown;
  static int omap3_get_pendown_state(void)
  {
-	return !gpio_get_value(OMAP3_EVM_TS_GPIO);
+	return !gpio_get_value(omap3_gpio_pendown);
  }
 
  static struct ads7846_platform_data ads7846_config = {

@@ -74,6 +75,7 @@ void __init omap_ads7846_init(int bus_num, int gpio_pendown, int gpio_debounce,
  	struct spi_board_info *spi_bi =&ads7846_spi_board_info;
  	int err;
 
+	omap3_gpio_pendown = gpio_pendown;
  	err = gpio_request_one(gpio_pendown, GPIOF_IN, "TSPenDown");
  	if (err) {
  		pr_err("Couldn't obtain gpio for TSPenDown: %d\n", err);

arch/arm/mach-omap2/common-board-devices.h +0 -1

index 4c4ef6a..a0b4a428 100644
@@ -4,7 +4,6 @@
  #include "twl-common.h"
 
  #define NAND_BLOCK_SIZE	SZ_128K
-#define OMAP3_EVM_TS_GPIO	175
 
  struct mtd_partition;
  struct ads7846_platform_data;

Quoted 23 lines · Kevin Hilman

+ Zumeng Chen

Quoted 5 lines · Igor Grinberg

Hi Kevin,

I've just noticed that the patch has been modified by Arnd in a way that of course will trigger GPIO use without being requested. I'm sorry, I was not available by that time Arnd changed the patch.

Your right, your original patch isn't the problem. I found the root cause.

The real problem is actually introduced by the merge of your patch from the arm-soc/cleanup branch, and this one from Zumeng Chen: commit 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846) from the arm-soc/boards branch.

However, looking closer at the one from Zumeng, that one is clearly not right. It unconditionally adds a *board-specific* ->get_pendown_state function to the pdata that is common to *all* boards. That's just wrong and has the side-effect of making ->get_pendown_state() wrong on every board except the OMAP3EVM. Oops.

Quoted 3 lines

So, IMO, in addition to $SUBJECT patch, in order to get the touchscreen GPIO working on non OMAP3EVM boards, we also need something like this as well.

Definitely, thanks Kevin.

Quoted 3 lines

Igor, Zumeng, could you try this out on your boards anc confirm if it's working? I currently don't have a board setup with a touchscreen in my board farm.

Acked

Quoted 27 lines · zumeng.chen

Quoted 26 lines · Kevin Hilman

+ Zumeng Chen

Quoted 5 lines · Igor Grinberg

Hi Kevin,

I've just noticed that the patch has been modified by Arnd in a way that of course will trigger GPIO use without being requested. I'm sorry, I was not available by that time Arnd changed the patch.

Your right, your original patch isn't the problem. I found the root cause.

The real problem is actually introduced by the merge of your patch from the arm-soc/cleanup branch, and this one from Zumeng Chen: commit 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846) from the arm-soc/boards branch.

However, looking closer at the one from Zumeng, that one is clearly not right. It unconditionally adds a *board-specific* ->get_pendown_state function to the pdata that is common to *all* boards. That's just wrong and has the side-effect of making ->get_pendown_state() wrong on every board except the OMAP3EVM. Oops.

So, IMO, in addition to $SUBJECT patch, in order to get the touchscreen GPIO working on non OMAP3EVM boards, we also need something like this as well.

Quoted 5 lines

Definitely, thanks Kevin.

Quoted 3 lines

Igor, Zumeng, could you try this out on your boards anc confirm if it's working? I currently don't have a board setup with a touchscreen in my board farm.

Acked

Did you test this on your board?

If so, could you respond with a
Tested-by tag? Thanks

Quoted 27 lines · zumeng.chen

Quoted 26 lines · Kevin Hilman

+ Zumeng Chen

Quoted 5 lines · Igor Grinberg

Hi Kevin,

I've just noticed that the patch has been modified by Arnd in a way that of course will trigger GPIO use without being requested. I'm sorry, I was not available by that time Arnd changed the patch.

Your right, your original patch isn't the problem. I found the root cause.

The real problem is actually introduced by the merge of your patch from the arm-soc/cleanup branch, and this one from Zumeng Chen: commit 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846) from the arm-soc/boards branch.

However, looking closer at the one from Zumeng, that one is clearly not right. It unconditionally adds a *board-specific* ->get_pendown_state function to the pdata that is common to *all* boards. That's just wrong and has the side-effect of making ->get_pendown_state() wrong on every board except the OMAP3EVM. Oops.

So, IMO, in addition to $SUBJECT patch, in order to get the touchscreen GPIO working on non OMAP3EVM boards, we also need something like this as well.

Quoted 5 lines

Definitely, thanks Kevin.

Quoted 3 lines

Igor, Zumeng, could you try this out on your boards anc confirm if it's working? I currently don't have a board setup with a touchscreen in my board farm.

Acked

Did you test this on your board? If so, could you respond with a Tested-by tag? Thanks.

Kevin

+ Zumeng Chen

Quoted 5 lines · Igor Grinberg

Hi Kevin,

I've just noticed that the patch has been modified by Arnd in a way that of course will trigger GPIO use without being requested. I'm sorry, I was not available by that time Arnd changed the patch.

Your right, your original patch isn't the problem. I found the root cause.

The real problem is actually introduced by the merge of your patch from the arm-soc/cleanup branch, and this one from Zumeng Chen: commit 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846) from the arm-soc/boards branch.

However, looking closer at the one from Zumeng, that one is clearly not right. It unconditionally adds a *board-specific* ->get_pendown_state function to the pdata that is common to *all* boards. That's just wrong and has the side-effect of making ->get_pendown_state() wrong on every board except the OMAP3EVM. Oops.

So, IMO, in addition to $SUBJECT patch, in order to get the touchscreen GPIO working on non OMAP3EVM boards, we also need something like this as well.

Igor, Zumeng, could you try this out on your boards anc confirm if it's working? I currently don't have a board setup with a touchscreen in my board farm.

Kevin

Quoted 1 line

From 85516c6a3354967caf4cff434d28c3001cd411eb Mon Sep 17 00:00:00 2001

From: Kevin Hilman <[email protected]>
Date: Thu, 26 Jul 2012 12:15:38 -0700
Subject: [PATCH 2/2] ARM: OMAP2+: ads7846: fix ->get_pendown_state() to work
on all boards

commit 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846) introduced a common ->get_pendown_state() function into the generic code, but that function was board-specific for the OMAP3EVM.

Fix this up to work for all boards that pass in a pendown GPIO.

Cc Zumeng Chen <[email protected]>

Cc Igor Grinberg <[email protected]>

Signed-off-by Kevin Hilman <[email protected]>

---
 arch/arm/mach-omap2/board-omap3evm.c       |    1 +
 arch/arm/mach-omap2/common-board-devices.c |    4 +++-
 arch/arm/mach-omap2/common-board-devices.h |    1 -
 3 files changed, 4 insertions(+), 2 deletions(-)

arch/arm/mach-omap2/board-omap3evm.c +1 -0

index ef230a0..0d362e9 100644
@@ -58,6 +58,7 @@
 #include "hsmmc.h"
 #include "common-board-devices.h"
 
+#define OMAP3_EVM_TS_GPIO	175
 #define OMAP3_EVM_EHCI_VBUS	22
 #define OMAP3_EVM_EHCI_SELECT	61
 

arch/arm/mach-omap2/common-board-devices.c +3 -1

index 0fee95f..06f176f 100644
@@ -40,9 +40,10 @@ static struct omap2_mcspi_device_config ads7846_mcspi_config = {
  * of pdata->get_pendown_state, but we have done this. So set
  * get_pendown_state to avoid twice gpio requesting.
  */
+static int omap3_gpio_pendown;
 static int omap3_get_pendown_state(void)
 {
-	return !gpio_get_value(OMAP3_EVM_TS_GPIO);
+	return !gpio_get_value(omap3_gpio_pendown);
 }
 
 static struct ads7846_platform_data ads7846_config = {

@@ -74,6 +75,7 @@ void __init omap_ads7846_init(int bus_num, int gpio_pendown, int gpio_debounce,
 	struct spi_board_info *spi_bi = &ads7846_spi_board_info;
 	int err;
 
+	omap3_gpio_pendown = gpio_pendown;
 	err = gpio_request_one(gpio_pendown, GPIOF_IN, "TSPenDown");
 	if (err) {
 		pr_err("Couldn't obtain gpio for TSPenDown: %d\n", err);

arch/arm/mach-omap2/common-board-devices.h +0 -1

index 4c4ef6a..a0b4a428 100644
@@ -4,7 +4,6 @@
 #include "twl-common.h"
 
 #define NAND_BLOCK_SIZE	SZ_128K
-#define OMAP3_EVM_TS_GPIO	175
 
 struct mtd_partition;
 struct ads7846_platform_data;

Quoted 18 lines · Kevin Hilman

+ Zumeng Chen

Quoted 5 lines · Igor Grinberg

Hi Kevin,

I've just noticed that the patch has been modified by Arnd in a way that of course will trigger GPIO use without being requested. I'm sorry, I was not available by that time Arnd changed the patch.

Your right, your original patch isn't the problem. I found the root cause.

The real problem is actually introduced by the merge of your patch from the arm-soc/cleanup branch, and this one from Zumeng Chen: commit 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846) from the arm-soc/boards branch.

Quoted 5 lines

However, looking closer at the one from Zumeng, that one is clearly not right. It unconditionally adds a *board-specific* ->get_pendown_state function to the pdata that is common to *all* boards. That's just wrong and has the side-effect of making ->get_pendown_state() wrong on every board except the OMAP3EVM. Oops.

Argh... that should not be applied in first place...

I double sorry, I was not available when that conflict was resolved... The right resolution would be just to revert the 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846), but who cares now...

Quoted 5 lines

So, IMO, in addition to $SUBJECT patch, in order to get the touchscreen GPIO working on non OMAP3EVM boards, we also need something like this as well.

Quoted 3 lines

Igor, Zumeng, could you try this out on your boards anc confirm if it's working? I currently don't have a board setup with a touchscreen in my board farm.

Since you have already dig into this, which branch would be good for testing?

Quoted 19 lines · Igor Grinberg

Quoted 18 lines · Kevin Hilman

+ Zumeng Chen

Quoted 5 lines · Igor Grinberg

Hi Kevin,

I've just noticed that the patch has been modified by Arnd in a way that of course will trigger GPIO use without being requested. I'm sorry, I was not available by that time Arnd changed the patch.

Your right, your original patch isn't the problem. I found the root cause.

The real problem is actually introduced by the merge of your patch from the arm-soc/cleanup branch, and this one from Zumeng Chen: commit 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846) from the arm-soc/boards branch.

Quoted 7 lines

Quoted 5 lines

However, looking closer at the one from Zumeng, that one is clearly not right. It unconditionally adds a *board-specific* ->get_pendown_state function to the pdata that is common to *all* boards. That's just wrong and has the side-effect of making ->get_pendown_state() wrong on every board except the OMAP3EVM. Oops.

Argh... that should not be applied in first place...

Agreed, but it's already in mainline.

Quoted 16 lines

I double sorry, I was not available when that conflict was resolved... The right resolution would be just to revert the 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846), but who cares now...

Quoted 8 lines

So, IMO, in addition to $SUBJECT patch, in order to get the touchscreen GPIO working on non OMAP3EVM boards, we also need something like this as well.

Igor, Zumeng, could you try this out on your boards anc confirm if it's working? I currently don't have a board setup with a touchscreen in my board farm.

Quoted 1 line

Since you have already dig into this, which branch would be good for testing?

git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.6/fixes/ads7846

I just pushed this out, so it might take a bit to propagate to all the mirrors.

Quoted 50 lines · Kevin Hilman

Quoted 19 lines · Igor Grinberg

Quoted 18 lines · Kevin Hilman

+ Zumeng Chen

Quoted 5 lines · Igor Grinberg

Hi Kevin,

I've just noticed that the patch has been modified by Arnd in a way that of course will trigger GPIO use without being requested. I'm sorry, I was not available by that time Arnd changed the patch.

Your right, your original patch isn't the problem. I found the root cause.

The real problem is actually introduced by the merge of your patch from the arm-soc/cleanup branch, and this one from Zumeng Chen: commit 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846) from the arm-soc/boards branch.

Quoted 7 lines

Quoted 5 lines

However, looking closer at the one from Zumeng, that one is clearly not right. It unconditionally adds a *board-specific* ->get_pendown_state function to the pdata that is common to *all* boards. That's just wrong and has the side-effect of making ->get_pendown_state() wrong on every board except the OMAP3EVM. Oops.

Argh... that should not be applied in first place...

Agreed, but it's already in mainline.

Quoted 17 lines

I double sorry, I was not available when that conflict was resolved... The right resolution would be just to revert the 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846), but who cares now...

Quoted 8 lines

So, IMO, in addition to $SUBJECT patch, in order to get the touchscreen GPIO working on non OMAP3EVM boards, we also need something like this as well.

Igor, Zumeng, could you try this out on your boards anc confirm if it's working? I currently don't have a board setup with a touchscreen in my board farm.

Since you have already dig into this, which branch would be good for testing?

Quoted 4 lines

git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.6/fixes/ads7846

I just pushed this out, so it might take a bit to propagate to all the mirrors.

Thanks, Kevin, I've just sent the tested-by.
I would also like to see only one patch instead of current two patches, can you please, amend them into one, write a correct message and resend?

1) The above commit introduced a common ->get_pendown_state() function into the generic code, but that function was board-specific for the OMAP3EVM and thus broke most other boards using this code.

2) The above commit was mis-merged introducing another bug which prevents the ads7846 driver probe function to succeed.
The omap_ads7846_init() function frees the pendown GPIO in case there is no ->get_pendown_state() function set by the caller (board specific code), so it can be requested later by the ads7846 driver. The above commit add a common ->get_pendown_state() function without removing the gpio_free() call and thus once the ads7846 driver tries to use the pendown GPIO, it crashes as the pendown GPIO has not been requested.

3) The above commit introduces NO new functionality as get_pendown_state() function is already implemented in a suitable way by the ads7846 driver and the debounce time handling has already been fixed by commit 97ee9f01 (ARM: OMAP: fix the ads7846 init code).

This reverts commit 16aced80f6739beb2a6ff7b6f96c83ba80d331e8.

Conflicts:
arch/arm/mach-omap2/common-board-devices.c

Solved by taking the working version prior to the above commit.

Cc Zumeng Chen <[email protected]>

Cc Arnd Bergmann <[email protected]>

Signed-off-by Igor Grinberg <[email protected]>

---
Kevin, sorry for the late reply.
How about the above commit message and the below patch?

The patch applies cleanly to Tony's master branch (6 Aug 2012) or Kevin's kevin-omap-pm/for_3.6/fixes/ads7846
after resetting two top most commits.

This patch has been tested on both above branches on cm-t3730. Any other board tested will be greately appreciated.

Also, if after all said in the commit message, you still don't want to revert the original patch, feel free to post your thoughts.

 arch/arm/mach-omap2/board-omap3evm.c       |    1 +
 arch/arm/mach-omap2/common-board-devices.c |   11 -----------
 arch/arm/mach-omap2/common-board-devices.h |    1 -
 3 files changed, 1 insertions(+), 12 deletions(-)

arch/arm/mach-omap2/board-omap3evm.c +1 -0

index ef230a0..0d362e9 100644
@@ -58,6 +58,7 @@
 #include "hsmmc.h"
 #include "common-board-devices.h"
 
+#define OMAP3_EVM_TS_GPIO	175
 #define OMAP3_EVM_EHCI_VBUS	22
 #define OMAP3_EVM_EHCI_SELECT	61
 

arch/arm/mach-omap2/common-board-devices.c +0 -11

index 1473474..c187586 100644
@@ -35,16 +35,6 @@ static struct omap2_mcspi_device_config ads7846_mcspi_config = {
 	.turbo_mode	= 0,
 };
 
-/*
- * ADS7846 driver maybe request a gpio according to the value
- * of pdata->get_pendown_state, but we have done this. So set
- * get_pendown_state to avoid twice gpio requesting.
- */
-static int omap3_get_pendown_state(void)
-{
-	return !gpio_get_value(OMAP3_EVM_TS_GPIO);
-}
-
 static struct ads7846_platform_data ads7846_config = {
 	.x_max			= 0x0fff,
 	.y_max			= 0x0fff,

@@ -55,7 +45,6 @@ static struct ads7846_platform_data ads7846_config = {
 	.debounce_rep		= 1,
 	.gpio_pendown		= -EINVAL,
 	.keep_vref_on		= 1,
-	.get_pendown_state	= &omap3_get_pendown_state,
 };
 
 static struct spi_board_info ads7846_spi_board_info __initdata = {

arch/arm/mach-omap2/common-board-devices.h +0 -1

index 4c4ef6a..a0b4a428 100644
@@ -4,7 +4,6 @@
 #include "twl-common.h"
 
 #define NAND_BLOCK_SIZE	SZ_128K
-#define OMAP3_EVM_TS_GPIO	175
 
 struct mtd_partition;
 struct ads7846_platform_data;

Quoted 29 lines · Igor Grinberg

1) The above commit introduced a common ->get_pendown_state() function into the generic code, but that function was board-specific for the OMAP3EVM and thus broke most other boards using this code.

2) The above commit was mis-merged introducing another bug which prevents the ads7846 driver probe function to succeed.
The omap_ads7846_init() function frees the pendown GPIO in case there is no ->get_pendown_state() function set by the caller (board specific code), so it can be requested later by the ads7846 driver. The above commit add a common ->get_pendown_state() function without removing the gpio_free() call and thus once the ads7846 driver tries to use the pendown GPIO, it crashes as the pendown GPIO has not been requested.

3) The above commit introduces NO new functionality as get_pendown_state() function is already implemented in a suitable way by the ads7846 driver and the debounce time handling has already been fixed by commit 97ee9f01 (ARM: OMAP: fix the ads7846 init code).

This reverts commit 16aced80f6739beb2a6ff7b6f96c83ba80d331e8.

Conflicts:
arch/arm/mach-omap2/common-board-devices.c

Solved by taking the working version prior to the above commit.

Cc Zumeng Chen<[email protected]>

Cc Arnd Bergmann<[email protected]>

Signed-off-by Igor Grinberg<[email protected]>

Quoted 7 lines

---
Kevin, sorry for the late reply.
How about the above commit message and the below patch?

The patch applies cleanly to Tony's master branch (6 Aug 2012) or Kevin's kevin-omap-pm/for_3.6/fixes/ads7846
after resetting two top most commits.

Now that v3.6-rc1 is out, it should probalby be applied on top of -rc1. I've taken care of that and tested as well.

Quoted 3 lines

This patch has been tested on both above branches on cm-t3730. Any other board tested will be greately appreciated.

Quoted 2 lines

Also, if after all said in the commit message, you still don't want to revert the original patch, feel free to post your thoughts.

After all the digging I did, I agree that the revert is the best solution.

While it's a slightly different problem/bug, I think the gpio_free() in common-board-devices.c should still be unconditonal since the gpio_request() is now unconditional. That can be a separate patch though.

Acked-by Kevin Hilman<[email protected]>

Tested-by Kevin Hilman<[email protected]>

Tested on 3430/n900, 3530/Overo, 3730/Overo STORM, 3730/BB-xM.

The Overo boards were the ones that were crashing due to this bug since the pendown GPIO was the only one used in the bank.

Tony, can you queue this up for v3.6-rc? I have a version in my for_3.6/fixes/ads7846-2 branch with the tags above applied, based on v3.6-rc1.

Thanks

Kevin

Quoted 58 lines

  arch/arm/mach-omap2/board-omap3evm.c       |    1 +
  arch/arm/mach-omap2/common-board-devices.c |   11 -----------
  arch/arm/mach-omap2/common-board-devices.h |    1 -
  3 files changed, 1 insertions(+), 12 deletions(-)

arch/arm/mach-omap2/board-omap3evm.c +1 -0

index ef230a0..0d362e9 100644
@@ -58,6 +58,7 @@
  #include "hsmmc.h"
  #include "common-board-devices.h"
 
+#define OMAP3_EVM_TS_GPIO	175
  #define OMAP3_EVM_EHCI_VBUS	22
  #define OMAP3_EVM_EHCI_SELECT	61
 

arch/arm/mach-omap2/common-board-devices.c +0 -11

index 1473474..c187586 100644
@@ -35,16 +35,6 @@ static struct omap2_mcspi_device_config ads7846_mcspi_config = {
  	.turbo_mode	= 0,
  };
 
-/*
- * ADS7846 driver maybe request a gpio according to the value
- * of pdata->get_pendown_state, but we have done this. So set
- * get_pendown_state to avoid twice gpio requesting.
- */
-static int omap3_get_pendown_state(void)
-{
-	return !gpio_get_value(OMAP3_EVM_TS_GPIO);
-}
-
  static struct ads7846_platform_data ads7846_config = {
  	.x_max			= 0x0fff,
  	.y_max			= 0x0fff,

@@ -55,7 +45,6 @@ static struct ads7846_platform_data ads7846_config = {
  	.debounce_rep		= 1,
  	.gpio_pendown		= -EINVAL,
  	.keep_vref_on		= 1,
-	.get_pendown_state	=&omap3_get_pendown_state,
  };
 
  static struct spi_board_info ads7846_spi_board_info __initdata = {

arch/arm/mach-omap2/common-board-devices.h +0 -1

index 4c4ef6a..a0b4a428 100644
@@ -4,7 +4,6 @@
  #include "twl-common.h"
 
  #define NAND_BLOCK_SIZE	SZ_128K
-#define OMAP3_EVM_TS_GPIO	175
 
  struct mtd_partition;
  struct ads7846_platform_data;

Quoted 29 lines · Igor Grinberg

1) The above commit introduced a common ->get_pendown_state() function into the generic code, but that function was board-specific for the OMAP3EVM and thus broke most other boards using this code.

2) The above commit was mis-merged introducing another bug which prevents the ads7846 driver probe function to succeed.
The omap_ads7846_init() function frees the pendown GPIO in case there is no ->get_pendown_state() function set by the caller (board specific code), so it can be requested later by the ads7846 driver. The above commit add a common ->get_pendown_state() function without removing the gpio_free() call and thus once the ads7846 driver tries to use the pendown GPIO, it crashes as the pendown GPIO has not been requested.

3) The above commit introduces NO new functionality as get_pendown_state() function is already implemented in a suitable way by the ads7846 driver and the debounce time handling has already been fixed by commit 97ee9f01 (ARM: OMAP: fix the ads7846 init code).

This reverts commit 16aced80f6739beb2a6ff7b6f96c83ba80d331e8.

Conflicts:
arch/arm/mach-omap2/common-board-devices.c

Solved by taking the working version prior to the above commit.

Cc Zumeng Chen <[email protected]>

Cc Arnd Bergmann <[email protected]>

Signed-off-by Igor Grinberg <[email protected]>

Quoted 7 lines

---
Kevin, sorry for the late reply.
How about the above commit message and the below patch?

The patch applies cleanly to Tony's master branch (6 Aug 2012) or Kevin's kevin-omap-pm/for_3.6/fixes/ads7846
after resetting two top most commits.

Now that v3.6-rc1 is out, it should probalby be applied on top of -rc1. I've taken care of that and tested as well.

Quoted 5 lines

This patch has been tested on both above branches on cm-t3730. Any other board tested will be greately appreciated.

Also, if after all said in the commit message, you still don't want to revert the original patch, feel free to post your thoughts.

After all the digging I did, I agree that the revert is the best solution.

While it's a slightly different problem/bug, I think the gpio_free() in common-board-devices.c should still be unconditonal since the gpio_request() is now unconditional. That can be a separate patch though.

Acked-by Kevin Hilman <[email protected]>

Tested-by Kevin Hilman <[email protected]>

Tested on 3430/n900, 3530/Overo, 3730/Overo STORM, 3730/BB-xM.

The Overo boards were the ones that were crashing due to this bug since the pendown GPIO was the only one used in the bank.

Tony, can you queue this up for v3.6-rc? I have a version in my for_3.6/fixes/ads7846-2 branch with the tags above applied, based on v3.6-rc1.

1) The above commit introduced a common ->get_pendown_state() function into the generic code, but that function was board-specific for the OMAP3EVM and thus broke most other boards using this code.

2) The above commit was mis-merged introducing another bug which prevents the ads7846 driver probe function to succeed.
The omap_ads7846_init() function frees the pendown GPIO in case there is no ->get_pendown_state() function set by the caller (board specific code), so it can be requested later by the ads7846 driver. The above commit add a common ->get_pendown_state() function without removing the gpio_free() call and thus once the ads7846 driver tries to use the pendown GPIO, it crashes as the pendown GPIO has not been requested.

3) The above commit introduces NO new functionality as get_pendown_state() function is already implemented in a suitable way by the ads7846 driver and the debounce time handling has already been fixed by commit 97ee9f01 (ARM: OMAP: fix the ads7846 init code).

This reverts commit 16aced80f6739beb2a6ff7b6f96c83ba80d331e8.

Conflicts:
arch/arm/mach-omap2/common-board-devices.c

Solved by taking the working version prior to the above commit.

Cc Zumeng Chen <[email protected]>

Cc Arnd Bergmann <[email protected]>

Signed-off-by Igor Grinberg <[email protected]>

---
Kevin, sorry for the late reply.
How about the above commit message and the below patch?

The patch applies cleanly to Tony's master branch (6 Aug 2012) or Kevin's kevin-omap-pm/for_3.6/fixes/ads7846
after resetting two top most commits.

This patch has been tested on both above branches on cm-t3730. Any other board tested will be greately appreciated.

Also, if after all said in the commit message, you still don't want to revert the original patch, feel free to post your thoughts.

 arch/arm/mach-omap2/board-omap3evm.c       |    1 +
 arch/arm/mach-omap2/common-board-devices.c |   11 -----------
 arch/arm/mach-omap2/common-board-devices.h |    1 -
 3 files changed, 1 insertions(+), 12 deletions(-)

arch/arm/mach-omap2/board-omap3evm.c +1 -0

index ef230a0..0d362e9 100644
@@ -58,6 +58,7 @@
 #include "hsmmc.h"
 #include "common-board-devices.h"
 
+#define OMAP3_EVM_TS_GPIO	175
 #define OMAP3_EVM_EHCI_VBUS	22
 #define OMAP3_EVM_EHCI_SELECT	61
 

arch/arm/mach-omap2/common-board-devices.c +0 -10

index 1473474..c187586 100644
@@ -35,16 +35,6 @@ static struct omap2_mcspi_device_config ads7846_mcspi_config = {
 	.turbo_mode	= 0,
 };
 
-/*
- * ADS7846 driver maybe request a gpio according to the value
- * of pdata->get_pendown_state, but we have done this. So set
- * get_pendown_state to avoid twice gpio requesting.
- */
-static int omap3_get_pendown_state(void)
-{
-	return !gpio_get_value(OMAP3_EVM_TS_GPIO);
-}
-
 static struct ads7846_platform_data ads7846_config = {
 	.x_max			= 0x0fff,
 	.y_max			= 0x0fff,

arch/arm/mach-omap2/common-board-devices.c +0 -0

 };
 
 static struct spi_board_info ads7846_spi_board_info __initdata = {

arch/arm/mach-omap2/common-board-devices.h +0 -1

index 4c4ef6a..a0b4a428 100644
@@ -4,7 +4,6 @@
 #include "twl-common.h"
 
 #define NAND_BLOCK_SIZE	SZ_128K
-#define OMAP3_EVM_TS_GPIO	175
 
 struct mtd_partition;
 struct ads7846_platform_data;

? 2012?08?07? 04:22, Igor Grinberg ??:

Quoted 14 lines

1) The above commit introduced a common ->get_pendown_state() function into the generic code, but that function was board-specific for the OMAP3EVM and thus broke most other boards using this code.

2) The above commit was mis-merged introducing another bug which prevents the ads7846 driver probe function to succeed.
The omap_ads7846_init() function frees the pendown GPIO in case there is no ->get_pendown_state() function set by the caller (board specific code), so it can be requested later by the ads7846 driver. The above commit add a common ->get_pendown_state() function without removing the gpio_free() call and thus once the ads7846 driver tries to use the pendown GPIO, it crashes as the pendown GPIO has not been requested.

Quoted 4 lines

3) The above commit introduces NO new functionality as get_pendown_state() function is already implemented in a suitable way by the ads7846 driver and the debounce time handling has already been fixed by commit 97ee9f01 (ARM: OMAP: fix the ads7846 init code).

Igor,

I suspect these two patches(this and 97ee9f01) works well, and there is something new introduced in ads7846.c, I'll try to look at that new stuff in this weekend.

Please refer to in-line comments:

Regards,
Zumeng

Quoted 64 lines

This reverts commit 16aced80f6739beb2a6ff7b6f96c83ba80d331e8.

Conflicts:
arch/arm/mach-omap2/common-board-devices.c

Solved by taking the working version prior to the above commit.

Cc Zumeng Chen <[email protected]>

Cc Arnd Bergmann <[email protected]>

Signed-off-by Igor Grinberg <[email protected]>

---
Kevin, sorry for the late reply.
How about the above commit message and the below patch?

The patch applies cleanly to Tony's master branch (6 Aug 2012) or Kevin's kevin-omap-pm/for_3.6/fixes/ads7846
after resetting two top most commits.

This patch has been tested on both above branches on cm-t3730. Any other board tested will be greately appreciated.

Also, if after all said in the commit message, you still don't want to revert the original patch, feel free to post your thoughts.

 arch/arm/mach-omap2/board-omap3evm.c       |    1 +
 arch/arm/mach-omap2/common-board-devices.c |   11 -----------
 arch/arm/mach-omap2/common-board-devices.h |    1 -
 3 files changed, 1 insertions(+), 12 deletions(-)

arch/arm/mach-omap2/board-omap3evm.c +1 -0

index ef230a0..0d362e9 100644
@@ -58,6 +58,7 @@
 #include "hsmmc.h"
 #include "common-board-devices.h"
 
+#define OMAP3_EVM_TS_GPIO	175
 #define OMAP3_EVM_EHCI_VBUS	22
 #define OMAP3_EVM_EHCI_SELECT	61
 

arch/arm/mach-omap2/common-board-devices.c +0 -10

index 1473474..c187586 100644
@@ -35,16 +35,6 @@ static struct omap2_mcspi_device_config ads7846_mcspi_config = {
 	.turbo_mode	= 0,
 };
 
-/*
- * ADS7846 driver maybe request a gpio according to the value
- * of pdata->get_pendown_state, but we have done this. So set
- * get_pendown_state to avoid twice gpio requesting.
- */
-static int omap3_get_pendown_state(void)
-{
-	return !gpio_get_value(OMAP3_EVM_TS_GPIO);
-}
-
 static struct ads7846_platform_data ads7846_config = {
 	.x_max			= 0x0fff,
 	.y_max			= 0x0fff,

arch/arm/mach-omap2/common-board-devices.c static struct ads7846_platform_data ads7846_config = { · line 58

Quoted 4 lines

arch/arm/mach-omap2/common-board-devices.c +0 -1

@@ -55,7 +45,6 @@ static struct ads7846_platform_data ads7846_config = {
 	.debounce_rep		= 1,
 	.gpio_pendown		= -EINVAL,
 	.keep_vref_on		= 1,
-	.get_pendown_state	= &omap3_get_pendown_state,

You remove this, then please look at the following codes:

drivers/input/touchscreen/ads7846.c

969 if (pdata->get_pendown_state) {
970 ts->get_pendown_state = pdata->get_pendown_state; 971 } else if (gpio_is_valid(pdata->gpio_pendown)) { 972
973 err = gpio_request_one(pdata->gpio_pendown, GPIOF_IN, ^^^, the driver will want to request again, then it should be error if i'm right.

974 "ads7846_pendown");
975 if (err) {
976 dev_err(&spi->dev,
977 "failed to request/setup pendown GPIO%d: %d\n", 978 pdata->gpio_pendown, err);
979 return err;
980 }
981
982 ts->gpio_pendown = pdata->gpio_pendown; 983
984 } else {

+ Zumeng Chen

Quoted 5 lines · Igor Grinberg

Hi Kevin,

I've just noticed that the patch has been modified by Arnd in a way that of course will trigger GPIO use without being requested. I'm sorry, I was not available by that time Arnd changed the patch.

Your right, your original patch isn't the problem. I found the root cause.

The real problem is actually introduced by the merge of your patch from the arm-soc/cleanup branch, and this one from Zumeng Chen: commit 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846) from the arm-soc/boards branch.

However, looking closer at the one from Zumeng, that one is clearly not right. It unconditionally adds a *board-specific* ->get_pendown_state function to the pdata that is common to *all* boards. That's just wrong and has the side-effect of making ->get_pendown_state() wrong on every board except the OMAP3EVM. Oops.

So, IMO, in addition to $SUBJECT patch, in order to get the touchscreen GPIO working on non OMAP3EVM boards, we also need something like this as well.

Igor, Zumeng, could you try this out on your boards anc confirm if it's working? I currently don't have a board setup with a touchscreen in my board farm.

Kevin

Quoted 1 line

From 85516c6a3354967caf4cff434d28c3001cd411eb Mon Sep 17 00:00:00 2001

From: Kevin Hilman <[email protected]>
Date: Thu, 26 Jul 2012 12:15:38 -0700
Subject: [PATCH 2/2] ARM: OMAP2+: ads7846: fix ->get_pendown_state() to work
on all boards

commit 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846) introduced a common ->get_pendown_state() function into the generic code, but that function was board-specific for the OMAP3EVM.

Fix this up to work for all boards that pass in a pendown GPIO.

Cc Zumeng Chen <[email protected]>

Cc Igor Grinberg <[email protected]>

Signed-off-by Kevin Hilman <[email protected]>

---
 arch/arm/mach-omap2/board-omap3evm.c       |    1 +
 arch/arm/mach-omap2/common-board-devices.c |    4 +++-
 arch/arm/mach-omap2/common-board-devices.h |    1 -
 3 files changed, 4 insertions(+), 2 deletions(-)

arch/arm/mach-omap2/board-omap3evm.c +1 -0

index ef230a0..0d362e9 100644
@@ -58,6 +58,7 @@
 #include "hsmmc.h"
 #include "common-board-devices.h"
 
+#define OMAP3_EVM_TS_GPIO	175
 #define OMAP3_EVM_EHCI_VBUS	22
 #define OMAP3_EVM_EHCI_SELECT	61
 

arch/arm/mach-omap2/common-board-devices.c +3 -1

index 0fee95f..06f176f 100644
@@ -40,9 +40,10 @@ static struct omap2_mcspi_device_config ads7846_mcspi_config = {
  * of pdata->get_pendown_state, but we have done this. So set
  * get_pendown_state to avoid twice gpio requesting.
  */
+static int omap3_gpio_pendown;
 static int omap3_get_pendown_state(void)
 {
-	return !gpio_get_value(OMAP3_EVM_TS_GPIO);
+	return !gpio_get_value(omap3_gpio_pendown);
 }
 
 static struct ads7846_platform_data ads7846_config = {

@@ -74,6 +75,7 @@ void __init omap_ads7846_init(int bus_num, int gpio_pendown, int gpio_debounce,
 	struct spi_board_info *spi_bi = &ads7846_spi_board_info;
 	int err;
 
+	omap3_gpio_pendown = gpio_pendown;
 	err = gpio_request_one(gpio_pendown, GPIOF_IN, "TSPenDown");
 	if (err) {
 		pr_err("Couldn't obtain gpio for TSPenDown: %d\n", err);

arch/arm/mach-omap2/common-board-devices.h +0 -1

index 4c4ef6a..a0b4a428 100644
@@ -4,7 +4,6 @@
 #include "twl-common.h"
 
 #define NAND_BLOCK_SIZE	SZ_128K
-#define OMAP3_EVM_TS_GPIO	175
 
 struct mtd_partition;
 struct ads7846_platform_data;

Quoted 45 lines · Kevin Hilman

+ Zumeng Chen

Quoted 5 lines · Igor Grinberg

Hi Kevin,

I've just noticed that the patch has been modified by Arnd in a way that of course will trigger GPIO use without being requested. I'm sorry, I was not available by that time Arnd changed the patch.

Your right, your original patch isn't the problem. I found the root cause.

The real problem is actually introduced by the merge of your patch from the arm-soc/cleanup branch, and this one from Zumeng Chen: commit 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846) from the arm-soc/boards branch.

However, looking closer at the one from Zumeng, that one is clearly not right. It unconditionally adds a *board-specific* ->get_pendown_state function to the pdata that is common to *all* boards. That's just wrong and has the side-effect of making ->get_pendown_state() wrong on every board except the OMAP3EVM. Oops.

So, IMO, in addition to $SUBJECT patch, in order to get the touchscreen GPIO working on non OMAP3EVM boards, we also need something like this as well.

Igor, Zumeng, could you try this out on your boards anc confirm if it's working? I currently don't have a board setup with a touchscreen in my board farm.

Kevin

Quoted 1 line

From 85516c6a3354967caf4cff434d28c3001cd411eb Mon Sep 17 00:00:00 2001

From: Kevin Hilman <[email protected]>
Date: Thu, 26 Jul 2012 12:15:38 -0700
Subject: [PATCH 2/2] ARM: OMAP2+: ads7846: fix ->get_pendown_state() to work
on all boards

commit 16aced80f6 (ARM: OMAP3530evm: set pendown_state and debounce time for ads7846) introduced a common ->get_pendown_state() function into the generic code, but that function was board-specific for the OMAP3EVM.

Quoted 5 lines

Fix this up to work for all boards that pass in a pendown GPIO.

Cc Zumeng Chen <[email protected]>

Cc Igor Grinberg <[email protected]>

Signed-off-by Kevin Hilman <[email protected]>

Tested-by Igor Grinberg <[email protected]>