Re: [PATCH] OMAP3EVM: Update pad configuration for wakeup enabled pads

[Kevin Hilman <khilman@deeprootsystems.com>]

Ranjith Lohithakshan <ranjithl@ti.com> writes:

> On Wed, 28-Apr-10 11:53 PM +0530, Kevin Hilman wrote:
>> Ranjith Lohithakshan <ranjithl@ti.com> writes:
>> 
>>> Kevin,
>>>
>>> On Tue, 27-Apr-10 8:46 PM +0530, Kevin Hilman wrote:
>>>> Ranjith Lohithakshan <ranjithl@ti.com> writes:
>>>>
>>>>> OMAP3530 TRM section 7.4.4.4.2 requires OFFOUTENABLE to be set (active low)
>>>>> if wakeup capabilities are enabled on a pad. During OFF mode testing
>>>>> on OMAP3530 EVM, it was observed that the device was not residing in
>>>>> the OFF state. The device enters into the OFF state and immediately exits
>>>>> from that state as if an IO wakeup event has occured. The issue was traced
>>>>> down to the pad configuration of wkaeup enabled pad's.
>>>> Nice.
>>>>
>>>>> Also, the pad configuration is included only if the respective drivers are
>>>>> enabled in the defconfig.
>>>> Hmm, do you really want this?  If you don't have the driver enabled,
>>>> you have to rely on the bootloader settings of these pads which may
>>>> also be wrong and trigger an IO wakeup as well.
>>> The thought process was that, for example, if keypad is not enabled
>>> in a system configuration you probably don't want to see a wakeup
>>> occurring if someone presses a key stroke. I understand the concern
>>> that you have raised regarding bootloader mis-configurations. My
>>> impression was that the bootloaders typically set the mux modes and
>>> pull up's/downs and dont really program or enable the wakeup
>>> capability.  But we cannot depend on that thumb rule.
>> 
>> Unfortunately, Bootloaders don't "typically" do anything.  They are
>> routinely hacked/patched and cannot be trusted at all.
>> 
>>> What is your recommendation?
>> 
>> First, I suggest you fix the OFFOUTENABLE bug in a single patch
>> without introducing the #ifdefs.  Then, address the enable/disable of
>> the wakeups in a separate patch.
>
> I will do that.
>
>> Next, ideally wakeups should not be configured a this level of board
>> code.  There are APIs for that: enable_irq_wake()/disable_irq_wake()
>> 
>> For GPIOs (like the touchscreen), you really need to enable wakeups
>> using existing APIs, either in the driver or in board init code and be
>> sure there is an interrupt handler.  Please see the 'Known Problems'
>> section of the OMAP PM wiki[2] where it talks about GPIO wakeups.
>> Below[2] is an test patch I've used.
>
> I have pushed a patch on ads7846 to linux-input some time ago adding
> wakeup support.
>
> fdba2bb : Input: ads7846 - add wakeup support
>
> There is now a wakeup flag added to the ads7846 platform data which can
> enabled at the board level. Once this is set , the driver will do an
> enable_irq_wake. The patch is now accepted and in mainline. I will
> remove the wakeup mux configuration from the board file and instead will
> just set the wakeup flag in the ads7846 platform data.

Brilliant!  you're several steps ahead of me.  Would you mind
submitting a patch to l-o to enable that for SDP and omap3evm so I
can drop my suggested patch?

> The keypad uses a pin in the non-gpio mode. Is enable_irq_wake supported
> for non-gpio mode?

Not currently, as enable_irq_wake() ends up calling into the
irq_chip's set_wake() method.  This then calls into the OMAP GPIO
layer as plat-omap/gpio.c:gpio_wake_enable()

For now, the keypad could just use the mux API at runtime like SDP does
to enable wakeups when needed.

Kevin