[PATCH] ARM: OMAP: fix the ads7846 init code

[PATCH] ARM: OMAP: fix the ads7846 init code

[Igor Grinberg]

In case a board provides the gpio_pendown and not board_pdata,
the GPIO debounce is not taken care of.
Fix this by taking care of GPIO debounce in any case.

Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
---
 arch/arm/mach-omap2/common-board-devices.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-omap2/common-board-devices.c b/arch/arm/mach-omap2/common-board-devices.c
index 1706ebc..c187586 100644
--- a/arch/arm/mach-omap2/common-board-devices.c
+++ b/arch/arm/mach-omap2/common-board-devices.c
@@ -63,28 +63,30 @@ 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;
 
-	if (board_pdata && board_pdata->get_pendown_state) {
-		err = gpio_request_one(gpio_pendown, GPIOF_IN, "TSPenDown");
-		if (err) {
-			pr_err("Couldn't obtain gpio for TSPenDown: %d\n", err);
-			return;
-		}
-		gpio_export(gpio_pendown, 0);
-
-		if (gpio_debounce)
-			gpio_set_debounce(gpio_pendown, gpio_debounce);
+	err = gpio_request_one(gpio_pendown, GPIOF_IN, "TSPenDown");
+	if (err) {
+		pr_err("Couldn't obtain gpio for TSPenDown: %d\n", err);
+		return;
 	}
 
+	if (gpio_debounce)
+		gpio_set_debounce(gpio_pendown, gpio_debounce);
+
 	spi_bi->bus_num	= bus_num;
 	spi_bi->irq	= gpio_to_irq(gpio_pendown);
 
 	if (board_pdata) {
 		board_pdata->gpio_pendown = gpio_pendown;
 		spi_bi->platform_data = board_pdata;
+		if (board_pdata->get_pendown_state)
+			gpio_export(gpio_pendown, 0);
 	} else {
 		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
-- 
1.7.3.4

Re: [PATCH] ARM: OMAP: fix the ads7846 init code

[Kevin Hilman]

Hi Igor,

Igor Grinberg <grinberg@compulab.co.il> writes:

> In case a board provides the gpio_pendown and not board_pdata,
> the GPIO debounce is not taken care of.
> Fix this by taking care of GPIO debounce in any case.
>
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>

I just notice this this patch causing some faults in current l-o master
branch.  

> ---
>  arch/arm/mach-omap2/common-board-devices.c |   22 ++++++++++++----------
>  1 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/common-board-devices.c b/arch/arm/mach-omap2/common-board-devices.c
> index 1706ebc..c187586 100644
> --- a/arch/arm/mach-omap2/common-board-devices.c
> +++ b/arch/arm/mach-omap2/common-board-devices.c
> @@ -63,28 +63,30 @@ 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;
>  
> -	if (board_pdata && board_pdata->get_pendown_state) {
> -		err = gpio_request_one(gpio_pendown, GPIOF_IN, "TSPenDown");
> -		if (err) {
> -			pr_err("Couldn't obtain gpio for TSPenDown: %d\n", err);
> -			return;
> -		}
> -		gpio_export(gpio_pendown, 0);
> -
> -		if (gpio_debounce)
> -			gpio_set_debounce(gpio_pendown, gpio_debounce);
> +	err = gpio_request_one(gpio_pendown, GPIOF_IN, "TSPenDown");
> +	if (err) {
> +		pr_err("Couldn't obtain gpio for TSPenDown: %d\n", err);
> +		return;
>  	}
>  
> +	if (gpio_debounce)
> +		gpio_set_debounce(gpio_pendown, gpio_debounce);
> +
>  	spi_bi->bus_num	= bus_num;
>  	spi_bi->irq	= gpio_to_irq(gpio_pendown);
>  
>  	if (board_pdata) {
>  		board_pdata->gpio_pendown = gpio_pendown;
>  		spi_bi->platform_data = board_pdata;
> +		if (board_pdata->get_pendown_state)
> +			gpio_export(gpio_pendown, 0);
>  	} else {
>  		ads7846_config.gpio_pendown = gpio_pendown;
>  	}
>  
> +	if (!board_pdata || (board_pdata && !board_pdata->get_pendown_state))
> +		gpio_free(gpio_pendown);

The logic here for freeing the GPIO doesn't make any sense to me.
IIUC, the gpio_pendown is always used, so should not be freed.

Moreover, if this GPIO is freed, that allows that GPIO bank to runtime
suspend if there are no other GPIOs in use in that bank.  So the first
attempt to use the GPIO will fault.

For example, on my Overo board(s), I noticed this failing as soon as teh
ads7846 driver probes, requests the IRQ and the GPIO triggering is set.

Getting rid of this free fixes the problem.

The changelog doesn't describe why the GPIO is freed here so I'm not
sure I follow, but it seems to me that once this GPIO is requesed, it
should not be freed as long as it's used, otherwise PM faults can occur.

Kevin

>From bb87c3b5586950e480d0699504997a9ad587fd85 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@ti.com>
Date: Wed, 11 Jul 2012 15:47:29 -0700
Subject: [PATCH] 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 <grinberg@compulab.co.il>
Signed-off-by: Kevin Hilman <khilman@ti.com>
---
 arch/arm/mach-omap2/common-board-devices.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm/mach-omap2/common-board-devices.c b/arch/arm/mach-omap2/common-board-devices.c
index c187586..1ae6fd6 100644
--- a/arch/arm/mach-omap2/common-board-devices.c
+++ b/arch/arm/mach-omap2/common-board-devices.c
@@ -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
-- 
1.7.9.2

Re: [PATCH] ARM: OMAP: fix the ads7846 init code

[Igor Grinberg]

Hi Kevin,

On 07/12/12 02:00, Kevin Hilman wrote:
> Hi Igor,
> 
> Igor Grinberg <grinberg@compulab.co.il> writes:
> 
>> In case a board provides the gpio_pendown and not board_pdata,
>> the GPIO debounce is not taken care of.
>> Fix this by taking care of GPIO debounce in any case.
>>
>> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> 
> I just notice this this patch causing some faults in current l-o master
> branch.  

This was not the case by the time I did the patch...

> 
>> ---
>>  arch/arm/mach-omap2/common-board-devices.c |   22 ++++++++++++----------
>>  1 files changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/common-board-devices.c b/arch/arm/mach-omap2/common-board-devices.c
>> index 1706ebc..c187586 100644
>> --- a/arch/arm/mach-omap2/common-board-devices.c
>> +++ b/arch/arm/mach-omap2/common-board-devices.c
>> @@ -63,28 +63,30 @@ 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;
>>  
>> -	if (board_pdata && board_pdata->get_pendown_state) {
>> -		err = gpio_request_one(gpio_pendown, GPIOF_IN, "TSPenDown");
>> -		if (err) {
>> -			pr_err("Couldn't obtain gpio for TSPenDown: %d\n", err);
>> -			return;
>> -		}
>> -		gpio_export(gpio_pendown, 0);
>> -
>> -		if (gpio_debounce)
>> -			gpio_set_debounce(gpio_pendown, gpio_debounce);
>> +	err = gpio_request_one(gpio_pendown, GPIOF_IN, "TSPenDown");
>> +	if (err) {
>> +		pr_err("Couldn't obtain gpio for TSPenDown: %d\n", err);
>> +		return;
>>  	}
>>  
>> +	if (gpio_debounce)
>> +		gpio_set_debounce(gpio_pendown, gpio_debounce);
>> +
>>  	spi_bi->bus_num	= bus_num;
>>  	spi_bi->irq	= gpio_to_irq(gpio_pendown);
>>  
>>  	if (board_pdata) {
>>  		board_pdata->gpio_pendown = gpio_pendown;
>>  		spi_bi->platform_data = board_pdata;
>> +		if (board_pdata->get_pendown_state)
>> +			gpio_export(gpio_pendown, 0);
>>  	} else {
>>  		ads7846_config.gpio_pendown = gpio_pendown;
>>  	}
>>  
>> +	if (!board_pdata || (board_pdata && !board_pdata->get_pendown_state))
>> +		gpio_free(gpio_pendown);
> 
> The logic here for freeing the GPIO doesn't make any sense to me.
> IIUC, the gpio_pendown is always used, so should not be freed.

The gpio_pendown is requested by the ads7846 driver and
of course you can't request the GPIO second time, so the driver failed.

> 
> Moreover, if this GPIO is freed, that allows that GPIO bank to runtime
> suspend if there are no other GPIOs in use in that bank.  So the first
> attempt to use the GPIO will fault.

The ads7846 driver requests the GPIO prior to using it.

> 
> For example, on my Overo board(s), I noticed this failing as soon as teh
> ads7846 driver probes, requests the IRQ and the GPIO triggering is set.

Prior to requesting the IRQ, the ads7846_setup_pendown(spi, ts) method
is called, which requests the GPIO.

> 
> Getting rid of this free fixes the problem.

But then don't you have double requesting?

> 
> The changelog doesn't describe why the GPIO is freed here so I'm not
> sure I follow, but it seems to me that once this GPIO is requesed, it
> should not be freed as long as it's used, otherwise PM faults can occur.

It is not used until the ads7846 driver probes.

> 
> Kevin
> 
>>From bb87c3b5586950e480d0699504997a9ad587fd85 Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <khilman@ti.com>
> Date: Wed, 11 Jul 2012 15:47:29 -0700
> Subject: [PATCH] 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 <grinberg@compulab.co.il>
> Signed-off-by: Kevin Hilman <khilman@ti.com>
> ---
>  arch/arm/mach-omap2/common-board-devices.c |    3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/common-board-devices.c b/arch/arm/mach-omap2/common-board-devices.c
> index c187586..1ae6fd6 100644
> --- a/arch/arm/mach-omap2/common-board-devices.c
> +++ b/arch/arm/mach-omap2/common-board-devices.c
> @@ -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

-- 
Regards,
Igor.