Re: [PATCH] ARM: OMAP2+: ads7846 init: fix fault caused by freeing pen-down GPIO

[Igor Grinberg <grinberg@compulab.co.il>]

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.

So that is not my original patch that is triggering the issue,
but it does not meter already...
And currently, I think that the proper solution would be
to remove the gpio_free() call as your patch does, but.......

Tony, please pay attention to which branch it gets applied....
It should only apply to the branches that have the patch modified
by Arnd and not to those with unmodified patch
(e.g. fixes-non-critical should not receive the patch).

Kevin, thanks for the patch.

On 07/23/12 15:53, Igor Grinberg wrote:
> On 07/12/12 02:18, Kevin Hilman wrote:
>> 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...
> 
>>
>> 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?
> 
>>
>> 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...
> 
>> 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>

Acked-by: Igor Grinberg <grinberg@compulab.co.il>

>> ---
>> 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.

No, this patch is not good for fixes-non-critical,
as fixes-non-critical has unmodified version and
will result in double requesting.
It should be applied to master or derivatives that
have the modified patch version.

>>
>>  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.