Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756837Ab0HaHGQ (ORCPT ); Tue, 31 Aug 2010 03:06:16 -0400 Received: from mail-wy0-f174.google.com ([74.125.82.174]:44232 "EHLO mail-wy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756326Ab0HaHGP convert rfc822-to-8bit (ORCPT ); Tue, 31 Aug 2010 03:06:15 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type:content-transfer-encoding; b=cTzehbIsXqtYMrQFD4jE+iR/wdmuipezSDMS4J3afsROYVIZiPZN59LY7gtZfS4FCm ajUyjidhmMvgtM0RonGwdKBYHALBy9SlY8zQ2NRLroWoX/yfHZXOWgAVuPEKu/htcqv7 NkB+cwnxkBjAvenuweceCqVO/lXsEL3g022oI= MIME-Version: 1.0 In-Reply-To: References: <1282814339-10934-1-git-send-email-mark.brown314@gmail.com> <1282814339-10934-5-git-send-email-mark.brown314@gmail.com> From: Eric Miao Date: Tue, 31 Aug 2010 15:05:53 +0800 Message-ID: Subject: Re: [PATCH 4/5] ARM: pxa168: added special case handler for keypad interrupts To: "Mark F. Brown" Cc: Haojian Zhuang , Haojian Zhuang , linux-arm-kernel , linux-kernel , linux-input Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5568 Lines: 155 On Tue, Aug 31, 2010 at 2:28 PM, Mark F. Brown wrote: > On Tue, Aug 31, 2010 at 12:53 AM, Haojian Zhuang > wrote: >> On Thu, Aug 26, 2010 at 5:18 PM, Mark F. Brown wrote: >>> Signed-off-by: Mark F. Brown >>> --- >>>  arch/arm/mach-mmp/irq-pxa168.c |   23 ++++++++++++++++++++++- >>>  1 files changed, 22 insertions(+), 1 deletions(-) >>> >>> diff --git a/arch/arm/mach-mmp/irq-pxa168.c b/arch/arm/mach-mmp/irq-pxa168.c >>> index 52ff2f0..bfb0984 100644 >>> --- a/arch/arm/mach-mmp/irq-pxa168.c >>> +++ b/arch/arm/mach-mmp/irq-pxa168.c >>> @@ -17,6 +17,8 @@ >>>  #include >>> >>>  #include >>> +#include >>> +#include >>> >>>  #include "common.h" >>> >>> @@ -42,6 +44,19 @@ static struct irq_chip icu_irq_chip = { >>>        .unmask = icu_unmask_irq, >>>  }; >>> >>> +void handle_pxa168_keypad_irq(unsigned int irq, struct irq_desc *desc) >>> +{ >>> +       uint32_t val; >>> +       uint32_t mask = APMU_PXA168_KP_WAKE_CLR; >>> + >>> +       /* clear keypad wake event */ >>> +       val = __raw_readl(APMU_WAKE_CLR); >>> +       __raw_writel(val |  mask, APMU_WAKE_CLR); >>> + >>> +       /* handle irq */ >>> +       handle_level_irq(irq, desc); >>> +} >>> + >> >> Why should you clear wakeup event status in IRQ handler? If system is >> resumed by keypad event, you should clear this wakeup source in resume >> code. I think that IRQ code should not be binded with wakeup event at >> here. >> >>>  void __init icu_init_irq(void) >>>  { >>>        int irq; >>> @@ -49,7 +64,13 @@ void __init icu_init_irq(void) >>>        for (irq = 0; irq < 64; irq++) { >>>                icu_mask_irq(irq); >>>                set_irq_chip(irq, &icu_irq_chip); >>> -               set_irq_handler(irq, handle_level_irq); >>> + >>> +               /* special handler for keypad */ >>> +               if (cpu_is_pxa168() && irq == IRQ_PXA168_KEYPAD) >>> +                       set_irq_handler(irq, handle_pxa168_keypad_irq); >>> +               else >>> +                       set_irq_handler(irq, handle_level_irq); >>> + >>>                set_irq_flags(irq, IRQF_VALID); >>>        } >>>  } >>> -- >>> 1.7.0.4 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at  http://vger.kernel.org/majordomo-info.html >>> Please read the FAQ at  http://www.tux.org/lkml/ >>> >> > > Haojian, this is an usual case, but without that clear wake event you > will not be able to clear the keypad interrupt. For my push to open > source I re-wrote the workaround code and added it as a special case > IRQ handler. In the Marvell pxa168 code base it is actually located in > the keypad interrupt handler as the function clear_wakeup(). I felt it > was better to isolate this code in the mmp common code base instead. I'd rather the irq handler not being changed and use platform_data instead. How about something below: diff --git a/arch/arm/mach-mmp/include/mach/pxa168.h b/arch/arm/mach-mmp/include/mach/pxa168.h index 27e1bc7..d2b7946 100644 --- a/arch/arm/mach-mmp/include/mach/pxa168.h +++ b/arch/arm/mach-mmp/include/mach/pxa168.h @@ -5,6 +5,7 @@ struct sys_timer; extern struct sys_timer pxa168_timer; extern void __init pxa168_init_irq(void); +extern void pxa168_clear_keypad_wakeup(void); #include #include @@ -97,4 +98,10 @@ static inline int pxa168_add_nand(struct pxa3xx_nand_platform_data *info) { return pxa_register_device(&pxa168_device_nand, info, sizeof(*info)); } + +static inline int pxa168_add_keypad(struct pxa27x_keypad_platform_data *info) +{ + info->clear_wakeup = pxa168_clear_keypad_wakeup; + return pxa_register_device(&pxa168_device_keypad, info, sizeof(*info)); +} #endif /* __ASM_MACH_PXA168_H */ diff --git a/arch/arm/mach-pxa/include/mach/pxa27x_keypad.h b/arch/arm/mach-pxa/include/mach/pxa27x_keypad.h index 7b4eadc..3a6bfe7 100644 --- a/arch/arm/mach-pxa/include/mach/pxa27x_keypad.h +++ b/arch/arm/mach-pxa/include/mach/pxa27x_keypad.h @@ -52,6 +52,7 @@ struct pxa27x_keypad_platform_data { /* key debounce interval */ unsigned int debounce_interval; + void (*clear_wakeup); }; extern void pxa_set_keypad_info(struct pxa27x_keypad_platform_data *info); diff --git a/drivers/input/keyboard/pxa27x_keypad.c b/drivers/input/keyboard/pxa27x_keypad.c index f32404f..3fb94fe 100644 --- a/drivers/input/keyboard/pxa27x_keypad.c +++ b/drivers/input/keyboard/pxa27x_keypad.c @@ -330,10 +330,20 @@ static void pxa27x_keypad_scan_direct(struct pxa27x_keypad *keypad) keypad->direct_key_state = new_state; } +static void clear_wakeup(struct pxa27x_keypad *keypad) +{ + struct pxa27x_keypad_platform_data *pdata = keypad->pdata; + + if (pdata->clear_wakeup) + (pdata->clear_wakeup)(); +} + static irqreturn_t pxa27x_keypad_irq_handler(int irq, void *dev_id) { struct pxa27x_keypad *keypad = dev_id; - unsigned long kpc = keypad_readl(KPC); + unsigned long kpc; + + kpc = keypad_readl(KPC); if (kpc & KPC_DI) pxa27x_keypad_scan_direct(keypad); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/