[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 at 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 at 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 at 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 at compulab.co.il>
> Signed-off-by: Kevin Hilman <khilman at 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.