[PATCH/RFC] hardware irq debouncing support - Kernel

This is a discussion on [PATCH/RFC] hardware irq debouncing support - Kernel ; Hardware IRQ debouncing is common for IRQ controllers which are part of GPIO modules ... they often deal with mechanical switches, buttons, and so forth. This patch: - Provides simple support for that in genirq - Includes sample implementations for ...

+ Reply to Thread
Results 1 to 14 of 14

Thread: [PATCH/RFC] hardware irq debouncing support

  1. [PATCH/RFC] hardware irq debouncing support

    Hardware IRQ debouncing is common for IRQ controllers which are
    part of GPIO modules ... they often deal with mechanical switches,
    buttons, and so forth. This patch:

    - Provides simple support for that in genirq

    - Includes sample implementations for some Linux systems
    which already include non-generic support for this:

    * Atmel SOCs (AT91, AT32 -- the same GPIO module)
    * OMAP2/OMAP3 (not quite as simple)

    Control over how long to debounce is less common, and often applies
    to banks of GPIOs not individual signals ... not addressed here.

    Drivers can request this (where available) with a new IRQF_DEBOUNCED
    flag/hint passed to request_irq():

    IF that flag is set when a handler is registered
    AND the relevant irq_chip supports debouncing
    AND the IRQ isn't already flagged as being debounced
    THEN the irq_chip is asked to enable debouncing for this IRQ
    UNTIL the IRQ's last handler is unregistered.
    ELSE
    nothing is done ... the hint is ignored
    FI

    Comments?

    Having this mechanism in genirq would let boards remove a bunch of
    nonportable code, and would let drivers like gpio_keys, gpio_mouse,
    and various touchscreens work more reliably. It'd also let various
    SOC-specific MMC and CF card drivers switch over to more standard
    (and widely understandable) mechanisms.

    I'd like to submit such updates for the 2.6.28 merge window, in
    part to let mainline avoid needing yet another driver-specific
    programming interface for IRQ debouncing. (For TWL4030/TPS659x0,
    as used in most OMAP3 boards including the Gumstix Overo and the
    BeagleBoard.)

    - Dave

    p.s. Tony and Kevin: note the locking bugfix in the OMAP2/3 bit.

    ---
    arch/arm/mach-at91/gpio.c | 13 +++++++++++++
    arch/arm/plat-omap/gpio.c | 15 ++++++++++++++-
    arch/avr32/mach-at32ap/pio.c | 14 ++++++++++++++
    include/linux/interrupt.h | 2 ++
    include/linux/irq.h | 3 +++
    kernel/irq/manage.c | 27 +++++++++++++++++++++++++++
    6 files changed, 73 insertions(+), 1 deletion(-)

    --- a/arch/arm/mach-at91/gpio.c
    +++ b/arch/arm/mach-at91/gpio.c
    @@ -368,12 +368,25 @@ static int gpio_irq_type(unsigned pin, u
    }
    }

    +static int gpio_irq_debounce(unsigned pin, bool is_on)
    +{
    + void __iomem *pio = pin_to_controller(pin);
    + unsigned mask = pin_to_mask(pin);
    +
    + if (is_on)
    + __raw_writel(mask, pio + PIO_IFER);
    + else
    + __raw_writel(mask, pio + PIO_IFDR);
    + return 0;
    +}
    +
    static struct irq_chip gpio_irqchip = {
    .name = "GPIO",
    .mask = gpio_irq_mask,
    .unmask = gpio_irq_unmask,
    .set_type = gpio_irq_type,
    .set_wake = gpio_irq_set_wake,
    + .debounce = gpio_irq_debounce,
    };

    static void gpio_irq_handler(unsigned irq, struct irq_desc *desc)
    --- a/arch/arm/plat-omap/gpio.c
    +++ b/arch/arm/plat-omap/gpio.c
    @@ -472,6 +472,7 @@ void omap_set_gpio_debounce(int gpio, in
    {
    struct gpio_bank *bank;
    void __iomem *reg;
    + unsigned long flags;
    u32 val, l = 1 << get_gpio_index(gpio);

    if (cpu_class_is_omap1())
    @@ -479,8 +480,9 @@ void omap_set_gpio_debounce(int gpio, in

    bank = get_gpio_bank(gpio);
    reg = bank->base;
    -
    reg += OMAP24XX_GPIO_DEBOUNCE_EN;
    +
    + spin_lock_irqsave(&bank->lock, flags);
    val = __raw_readl(reg);

    if (enable)
    @@ -489,6 +491,7 @@ void omap_set_gpio_debounce(int gpio, in
    val &= ~l;

    __raw_writel(val, reg);
    + spin_unlock_irqrestore(&bank->lock, flags);
    }
    EXPORT_SYMBOL(omap_set_gpio_debounce);

    @@ -1109,6 +1112,12 @@ static void gpio_unmask_irq(unsigned int
    _set_gpio_irqenable(bank, gpio, 1);
    }

    +static int gpio_irq_debounce(unsigned int irq, bool is_on)
    +{
    + omap_set_gpio_debounce(irq - IH_GPIO_BASE, is_on);
    + return 0;
    +}
    +
    static struct irq_chip gpio_irq_chip = {
    .name = "GPIO",
    .shutdown = gpio_irq_shutdown,
    @@ -1437,6 +1446,10 @@ static int __init _omap_gpio_init(void)
    (rev >> 4) & 0x0f, rev & 0x0f);
    }
    #endif
    +
    + if (cpu_is_omap242x() || cpu_is_omap243x() || cpu_is_omap34xx())
    + gpio_irq_chip.debounce = gpio_irq_debounce;
    +
    for (i = 0; i < gpio_bank_count; i++) {
    int j, gpio_count = 16;

    --- a/arch/avr32/mach-at32ap/pio.c
    +++ b/arch/avr32/mach-at32ap/pio.c
    @@ -234,11 +234,25 @@ static int gpio_irq_type(unsigned irq, u
    return 0;
    }

    +static int gpio_irq_debounce(unsigned irq, bool is_on)
    +{
    + unsigned gpio = irq_to_gpio(irq);
    + struct pio_device *pio = &pio_dev[gpio >> 5];
    + u32 mask = 1 << (gpio & 0x1f);
    +
    + if (is_on)
    + pio_writel(pio, IFER, mask);
    + else
    + pio_writel(pio, IFDR, mask);
    + return 0;
    +}
    +
    static struct irq_chip gpio_irqchip = {
    .name = "gpio",
    .mask = gpio_irq_mask,
    .unmask = gpio_irq_unmask,
    .set_type = gpio_irq_type,
    + .debounce = gpio_irq_debounce,
    };

    static void gpio_irq_handler(unsigned irq, struct irq_desc *desc)
    --- a/include/linux/interrupt.h
    +++ b/include/linux/interrupt.h
    @@ -45,6 +45,7 @@
    * IRQF_IRQPOLL - Interrupt is used for polling (only the interrupt that is
    * registered first in an shared interrupt is considered for
    * performance reasons)
    + * IRQF_DEBOUNCED - Interrupt should use hardware debouncing where possible
    */
    #define IRQF_DISABLED 0x00000020
    #define IRQF_SAMPLE_RANDOM 0x00000040
    @@ -54,6 +55,7 @@
    #define IRQF_PERCPU 0x00000400
    #define IRQF_NOBALANCING 0x00000800
    #define IRQF_IRQPOLL 0x00001000
    +#define IRQF_DEBOUNCED 0x00002000

    typedef irqreturn_t (*irq_handler_t)(int, void *);

    --- a/include/linux/irq.h
    +++ b/include/linux/irq.h
    @@ -44,6 +44,7 @@ typedef void (*irq_flow_handler_t)(unsig
    #define IRQ_TYPE_LEVEL_LOW 0x00000008 /* Level low type */
    #define IRQ_TYPE_SENSE_MASK 0x0000000f /* Mask of the above */
    #define IRQ_TYPE_PROBE 0x00000010 /* Probing in progress */
    +#define IRQ_TYPE_DEBOUNCED 0x00000020 /* Hardware debouncing enabled */

    /* Internal flags */
    #define IRQ_INPROGRESS 0x00000100 /* IRQ handler active - do not enter! */
    @@ -92,6 +93,7 @@ struct msi_desc;
    * @retrigger: resend an IRQ to the CPU
    * @set_type: set the flow type (IRQ_TYPE_LEVEL/etc.) of an IRQ
    * @set_wake: enable/disable power-management wake-on of an IRQ
    + * @debounce: enable/disable hardware debouncing of an IRQ
    *
    * @release: release function solely used by UML
    * @typename: obsoleted by name, kept as migration helper
    @@ -114,6 +116,7 @@ struct irq_chip {
    int (*retrigger)(unsigned int irq);
    int (*set_type)(unsigned int irq, unsigned int flow_type);
    int (*set_wake)(unsigned int irq, unsigned int on);
    + int (*debounce)(unsigned int irq, bool is_on);

    /* Currently used only by UML, might disappear one day.*/
    #ifdef CONFIG_IRQ_RELEASE_METHOD
    --- a/kernel/irq/manage.c
    +++ b/kernel/irq/manage.c
    @@ -445,6 +445,18 @@ int setup_irq(unsigned int irq, struct i
    desc->irq_count = 0;
    desc->irqs_unhandled = 0;

    + /* Maybe enable hardware debouncing */
    + if ((new->flags & IRQF_DEBOUNCED)
    + && desc->chip->debounce
    + && !(desc->status & IRQ_TYPE_DEBOUNCED)) {
    + ret = desc->chip->debounce(irq, true);
    + if (ret < 0)
    + pr_debug("IRQ: can't debounce irq %d, err %d\n",
    + irq, ret);
    + else
    + desc->status |= IRQ_TYPE_DEBOUNCED;
    + }
    +
    /*
    * Check whether we disabled the irq via the spurious handler
    * before. Reenable it and give it another chance.
    @@ -524,6 +536,21 @@ void free_irq(unsigned int irq, void *de

    if (!desc->action) {
    desc->status |= IRQ_DISABLED;
    +
    + /* Maybe disable hardware debouncing */
    + if ((desc->status & IRQ_TYPE_DEBOUNCED)
    + && desc->chip->debounce) {
    + int status;
    +
    + status = desc->chip->debounce(irq, false);
    + if (status < 0)
    + pr_debug("IRQ: irq %d still "
    + "being debounced, err %d\n",
    + irq, status);
    + else
    + desc->status &= ~IRQ_TYPE_DEBOUNCED;
    + }
    +
    if (desc->chip->shutdown)
    desc->chip->shutdown(irq);
    else
    --
    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/

  2. Re: [PATCH/RFC] hardware irq debouncing support

    * David Brownell [080924 22:51]:
    > Hardware IRQ debouncing is common for IRQ controllers which are
    > part of GPIO modules ... they often deal with mechanical switches,
    > buttons, and so forth. This patch:
    >
    > - Provides simple support for that in genirq
    >
    > - Includes sample implementations for some Linux systems
    > which already include non-generic support for this:
    >
    > * Atmel SOCs (AT91, AT32 -- the same GPIO module)
    > * OMAP2/OMAP3 (not quite as simple)
    >
    > Control over how long to debounce is less common, and often applies
    > to banks of GPIOs not individual signals ... not addressed here.
    >
    > Drivers can request this (where available) with a new IRQF_DEBOUNCED
    > flag/hint passed to request_irq():
    >
    > IF that flag is set when a handler is registered
    > AND the relevant irq_chip supports debouncing
    > AND the IRQ isn't already flagged as being debounced
    > THEN the irq_chip is asked to enable debouncing for this IRQ
    > UNTIL the IRQ's last handler is unregistered.
    > ELSE
    > nothing is done ... the hint is ignored
    > FI
    >
    > Comments?
    >
    > Having this mechanism in genirq would let boards remove a bunch of
    > nonportable code, and would let drivers like gpio_keys, gpio_mouse,
    > and various touchscreens work more reliably. It'd also let various
    > SOC-specific MMC and CF card drivers switch over to more standard
    > (and widely understandable) mechanisms.


    Yeah this would nuke bunch of omap specific code for dealing with
    battery covers, MMC slot open etc..

    > I'd like to submit such updates for the 2.6.28 merge window, in
    > part to let mainline avoid needing yet another driver-specific
    > programming interface for IRQ debouncing. (For TWL4030/TPS659x0,
    > as used in most OMAP3 boards including the Gumstix Overo and the
    > BeagleBoard.)


    What's the plan for sysfs_notify event for these switches? Are you
    planning to add something like that to gpiolib?

    Tony


    > p.s. Tony and Kevin: note the locking bugfix in the OMAP2/3 bit.


    Here's my ack for that:

    Acked-by: Tony Lindgren

    >
    > ---
    > arch/arm/mach-at91/gpio.c | 13 +++++++++++++
    > arch/arm/plat-omap/gpio.c | 15 ++++++++++++++-
    > arch/avr32/mach-at32ap/pio.c | 14 ++++++++++++++
    > include/linux/interrupt.h | 2 ++
    > include/linux/irq.h | 3 +++
    > kernel/irq/manage.c | 27 +++++++++++++++++++++++++++
    > 6 files changed, 73 insertions(+), 1 deletion(-)
    >
    > --- a/arch/arm/mach-at91/gpio.c
    > +++ b/arch/arm/mach-at91/gpio.c
    > @@ -368,12 +368,25 @@ static int gpio_irq_type(unsigned pin, u
    > }
    > }
    >
    > +static int gpio_irq_debounce(unsigned pin, bool is_on)
    > +{
    > + void __iomem *pio = pin_to_controller(pin);
    > + unsigned mask = pin_to_mask(pin);
    > +
    > + if (is_on)
    > + __raw_writel(mask, pio + PIO_IFER);
    > + else
    > + __raw_writel(mask, pio + PIO_IFDR);
    > + return 0;
    > +}
    > +
    > static struct irq_chip gpio_irqchip = {
    > .name = "GPIO",
    > .mask = gpio_irq_mask,
    > .unmask = gpio_irq_unmask,
    > .set_type = gpio_irq_type,
    > .set_wake = gpio_irq_set_wake,
    > + .debounce = gpio_irq_debounce,
    > };
    >
    > static void gpio_irq_handler(unsigned irq, struct irq_desc *desc)
    > --- a/arch/arm/plat-omap/gpio.c
    > +++ b/arch/arm/plat-omap/gpio.c
    > @@ -472,6 +472,7 @@ void omap_set_gpio_debounce(int gpio, in
    > {
    > struct gpio_bank *bank;
    > void __iomem *reg;
    > + unsigned long flags;
    > u32 val, l = 1 << get_gpio_index(gpio);
    >
    > if (cpu_class_is_omap1())
    > @@ -479,8 +480,9 @@ void omap_set_gpio_debounce(int gpio, in
    >
    > bank = get_gpio_bank(gpio);
    > reg = bank->base;
    > -
    > reg += OMAP24XX_GPIO_DEBOUNCE_EN;
    > +
    > + spin_lock_irqsave(&bank->lock, flags);
    > val = __raw_readl(reg);
    >
    > if (enable)
    > @@ -489,6 +491,7 @@ void omap_set_gpio_debounce(int gpio, in
    > val &= ~l;
    >
    > __raw_writel(val, reg);
    > + spin_unlock_irqrestore(&bank->lock, flags);
    > }
    > EXPORT_SYMBOL(omap_set_gpio_debounce);
    >
    > @@ -1109,6 +1112,12 @@ static void gpio_unmask_irq(unsigned int
    > _set_gpio_irqenable(bank, gpio, 1);
    > }
    >
    > +static int gpio_irq_debounce(unsigned int irq, bool is_on)
    > +{
    > + omap_set_gpio_debounce(irq - IH_GPIO_BASE, is_on);
    > + return 0;
    > +}
    > +
    > static struct irq_chip gpio_irq_chip = {
    > .name = "GPIO",
    > .shutdown = gpio_irq_shutdown,
    > @@ -1437,6 +1446,10 @@ static int __init _omap_gpio_init(void)
    > (rev >> 4) & 0x0f, rev & 0x0f);
    > }
    > #endif
    > +
    > + if (cpu_is_omap242x() || cpu_is_omap243x() || cpu_is_omap34xx())
    > + gpio_irq_chip.debounce = gpio_irq_debounce;
    > +
    > for (i = 0; i < gpio_bank_count; i++) {
    > int j, gpio_count = 16;
    >
    > --- a/arch/avr32/mach-at32ap/pio.c
    > +++ b/arch/avr32/mach-at32ap/pio.c
    > @@ -234,11 +234,25 @@ static int gpio_irq_type(unsigned irq, u
    > return 0;
    > }
    >
    > +static int gpio_irq_debounce(unsigned irq, bool is_on)
    > +{
    > + unsigned gpio = irq_to_gpio(irq);
    > + struct pio_device *pio = &pio_dev[gpio >> 5];
    > + u32 mask = 1 << (gpio & 0x1f);
    > +
    > + if (is_on)
    > + pio_writel(pio, IFER, mask);
    > + else
    > + pio_writel(pio, IFDR, mask);
    > + return 0;
    > +}
    > +
    > static struct irq_chip gpio_irqchip = {
    > .name = "gpio",
    > .mask = gpio_irq_mask,
    > .unmask = gpio_irq_unmask,
    > .set_type = gpio_irq_type,
    > + .debounce = gpio_irq_debounce,
    > };
    >
    > static void gpio_irq_handler(unsigned irq, struct irq_desc *desc)
    > --- a/include/linux/interrupt.h
    > +++ b/include/linux/interrupt.h
    > @@ -45,6 +45,7 @@
    > * IRQF_IRQPOLL - Interrupt is used for polling (only the interrupt that is
    > * registered first in an shared interrupt is considered for
    > * performance reasons)
    > + * IRQF_DEBOUNCED - Interrupt should use hardware debouncing where possible
    > */
    > #define IRQF_DISABLED 0x00000020
    > #define IRQF_SAMPLE_RANDOM 0x00000040
    > @@ -54,6 +55,7 @@
    > #define IRQF_PERCPU 0x00000400
    > #define IRQF_NOBALANCING 0x00000800
    > #define IRQF_IRQPOLL 0x00001000
    > +#define IRQF_DEBOUNCED 0x00002000
    >
    > typedef irqreturn_t (*irq_handler_t)(int, void *);
    >
    > --- a/include/linux/irq.h
    > +++ b/include/linux/irq.h
    > @@ -44,6 +44,7 @@ typedef void (*irq_flow_handler_t)(unsig
    > #define IRQ_TYPE_LEVEL_LOW 0x00000008 /* Level low type */
    > #define IRQ_TYPE_SENSE_MASK 0x0000000f /* Mask of the above */
    > #define IRQ_TYPE_PROBE 0x00000010 /* Probing in progress */
    > +#define IRQ_TYPE_DEBOUNCED 0x00000020 /* Hardware debouncing enabled */
    >
    > /* Internal flags */
    > #define IRQ_INPROGRESS 0x00000100 /* IRQ handler active - do not enter! */
    > @@ -92,6 +93,7 @@ struct msi_desc;
    > * @retrigger: resend an IRQ to the CPU
    > * @set_type: set the flow type (IRQ_TYPE_LEVEL/etc.) of an IRQ
    > * @set_wake: enable/disable power-management wake-on of an IRQ
    > + * @debounce: enable/disable hardware debouncing of an IRQ
    > *
    > * @release: release function solely used by UML
    > * @typename: obsoleted by name, kept as migration helper
    > @@ -114,6 +116,7 @@ struct irq_chip {
    > int (*retrigger)(unsigned int irq);
    > int (*set_type)(unsigned int irq, unsigned int flow_type);
    > int (*set_wake)(unsigned int irq, unsigned int on);
    > + int (*debounce)(unsigned int irq, bool is_on);
    >
    > /* Currently used only by UML, might disappear one day.*/
    > #ifdef CONFIG_IRQ_RELEASE_METHOD
    > --- a/kernel/irq/manage.c
    > +++ b/kernel/irq/manage.c
    > @@ -445,6 +445,18 @@ int setup_irq(unsigned int irq, struct i
    > desc->irq_count = 0;
    > desc->irqs_unhandled = 0;
    >
    > + /* Maybe enable hardware debouncing */
    > + if ((new->flags & IRQF_DEBOUNCED)
    > + && desc->chip->debounce
    > + && !(desc->status & IRQ_TYPE_DEBOUNCED)) {
    > + ret = desc->chip->debounce(irq, true);
    > + if (ret < 0)
    > + pr_debug("IRQ: can't debounce irq %d, err %d\n",
    > + irq, ret);
    > + else
    > + desc->status |= IRQ_TYPE_DEBOUNCED;
    > + }
    > +
    > /*
    > * Check whether we disabled the irq via the spurious handler
    > * before. Reenable it and give it another chance.
    > @@ -524,6 +536,21 @@ void free_irq(unsigned int irq, void *de
    >
    > if (!desc->action) {
    > desc->status |= IRQ_DISABLED;
    > +
    > + /* Maybe disable hardware debouncing */
    > + if ((desc->status & IRQ_TYPE_DEBOUNCED)
    > + && desc->chip->debounce) {
    > + int status;
    > +
    > + status = desc->chip->debounce(irq, false);
    > + if (status < 0)
    > + pr_debug("IRQ: irq %d still "
    > + "being debounced, err %d\n",
    > + irq, status);
    > + else
    > + desc->status &= ~IRQ_TYPE_DEBOUNCED;
    > + }
    > +
    > if (desc->chip->shutdown)
    > desc->chip->shutdown(irq);
    > else

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

  3. Re: [PATCH/RFC] hardware irq debouncing support

    David Brownell wrote:
    > Hardware IRQ debouncing is common for IRQ controllers which are
    > part of GPIO modules ... they often deal with mechanical switches,
    > buttons, and so forth. This patch:
    >
    > - Provides simple support for that in genirq
    >
    > - Includes sample implementations for some Linux systems
    > which already include non-generic support for this:
    >
    > * Atmel SOCs (AT91, AT32 -- the same GPIO module)
    > * OMAP2/OMAP3 (not quite as simple)
    >
    > Control over how long to debounce is less common, and often applies
    > to banks of GPIOs not individual signals ... not addressed here.
    >
    > Drivers can request this (where available) with a new IRQF_DEBOUNCED
    > flag/hint passed to request_irq():
    >
    > IF that flag is set when a handler is registered
    > AND the relevant irq_chip supports debouncing
    > AND the IRQ isn't already flagged as being debounced
    > THEN the irq_chip is asked to enable debouncing for this IRQ
    > UNTIL the IRQ's last handler is unregistered.
    > ELSE
    > nothing is done ... the hint is ignored
    > FI
    >
    > Comments?


    Note that at least for AT91 and AVR32, the terminology used is "glitch
    filtering", not "debouncing". The filtering period is very short (half
    a clock cycle), so it probably won't help much when dealing with
    mechanical switches and buttons.

    What kind of guarantees should IRQF_DEBOUNCE provide? Filtering short
    glitches may be useful, but if drivers start assuming it will do real
    debouncing of badly filtered switches and buttons, I think we're in for
    some trouble...

    > Having this mechanism in genirq would let boards remove a bunch of
    > nonportable code, and would let drivers like gpio_keys, gpio_mouse,
    > and various touchscreens work more reliably. It'd also let various
    > SOC-specific MMC and CF card drivers switch over to more standard
    > (and widely understandable) mechanisms.
    >
    > I'd like to submit such updates for the 2.6.28 merge window, in
    > part to let mainline avoid needing yet another driver-specific
    > programming interface for IRQ debouncing. (For TWL4030/TPS659x0,
    > as used in most OMAP3 boards including the Gumstix Overo and the
    > BeagleBoard.)


    Given that the limitations of this interface are clearly documented, I'm
    all for it.

    What would be perhaps even more useful is generic software debouncing
    support. Something like

    int request_debounced_irq(int irq, unsigned long debounce_us,
    void (*handler)(void *data), void *data);

    which would set up a handler which disables the interrupt and sets up a
    timer which will ack/unmask the interrupt and run the handler.

    This would mean the "interrupt handler" really gets run in softirq
    context, and shared interrupt would probably be impossible to support,
    but I think it would be useful for certain kinds of interrupts.

    What do you think?

    Haavard
    --
    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/

  4. Re: [PATCH/RFC] hardware irq debouncing support

    * David Brownell [081003 11:45]:
    > On Friday 03 October 2008, Tony Lindgren wrote:
    > > * David Brownell [080924 22:51]:
    > > > Hardware IRQ debouncing is common for IRQ controllers which are
    > > > part of GPIO modules ... they often deal with mechanical switches,
    > > > buttons, and so forth. This patch:
    > > >
    > > > - Provides simple support for that in genirq
    > > >
    > > > - Includes sample implementations for some Linux systems
    > > > which already include non-generic support for this:
    > > >
    > > > * Atmel SOCs (AT91, AT32 -- the same GPIO module)
    > > > * OMAP2/OMAP3 (not quite as simple)
    > > >
    > > > ...
    > > > Comments?
    > > >
    > > > Having this mechanism in genirq would let boards remove a bunch of
    > > > nonportable code, and would let drivers like gpio_keys, gpio_mouse,
    > > > and various touchscreens work more reliably. It'd also let various
    > > > SOC-specific MMC and CF card drivers switch over to more standard
    > > > (and widely understandable) mechanisms.

    > >
    > > Yeah this would nuke bunch of omap specific code for dealing with
    > > battery covers, MMC slot open etc..

    >
    > It would be hidden away behind IRQF_DEBOUNCE ... not so much
    > making the code vanish, as making hardware-specific interfaces
    > vanish in favor of what seems to be the most popular idiom.
    >
    > (Regular input GPIOs could be debounced too, but every time
    > I've noticed debouncing in use, it's coupled to an IRQ.)
    >
    > > > I'd like to submit such updates for the 2.6.28 merge window, in
    > > > part to let mainline avoid needing yet another driver-specific
    > > > programming interface for IRQ debouncing. (For TWL4030/TPS659x0,
    > > > as used in most OMAP3 boards including the Gumstix Overo and the
    > > > BeagleBoard.)

    > >
    > > What's the plan for sysfs_notify event for these switches? Are you
    > > planning to add something like that to gpiolib?

    >
    > I'm not sure what you mean -- which particular switches?
    > If the issue is the MMC card detect switches, that seems
    > more like an MMC question...
    >
    > There was discussion of having the gpio_export() code
    > support notification that way, triggered by interrupts
    > on IRQ-capable GPIOs, but nobody seems to have wanted
    > it enough to translate from talk to code.


    Well in linux-omap tree we have the gpio-switch.c that is handy for
    generic state switched like headset connected etc. It shows the
    events also via sysfs_notify() so userspace can pop up messages.

    That code should use gpiolib.. But I was wondering if we'll need it
    eventually at all.

    Tony



    >
    > - Dave
    >
    >
    > > Tony
    > >
    > >
    > > > p.s. Tony and Kevin: note the locking bugfix in the OMAP2/3 bit.

    > >
    > > Here's my ack for that:
    > >
    > > Acked-by: Tony Lindgren

    >
    > For the whole patch, I'll presume, not just that locking fix.


    Sure

    >
    > >
    > > >
    > > > ---
    > > > arch/arm/mach-at91/gpio.c | 13 +++++++++++++
    > > > arch/arm/plat-omap/gpio.c | 15 ++++++++++++++-
    > > > arch/avr32/mach-at32ap/pio.c | 14 ++++++++++++++
    > > > include/linux/interrupt.h | 2 ++
    > > > include/linux/irq.h | 3 +++
    > > > kernel/irq/manage.c | 27 +++++++++++++++++++++++++++
    > > > 6 files changed, 73 insertions(+), 1 deletion(-)
    > > >

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

  5. Re: [PATCH/RFC] hardware irq debouncing support

    On Wed 2008-09-24 12:51:32, David Brownell wrote:
    > Hardware IRQ debouncing is common for IRQ controllers which are
    > part of GPIO modules ... they often deal with mechanical switches,
    > buttons, and so forth. This patch:
    >
    > - Provides simple support for that in genirq
    >
    > - Includes sample implementations for some Linux systems
    > which already include non-generic support for this:
    >
    > * Atmel SOCs (AT91, AT32 -- the same GPIO module)
    > * OMAP2/OMAP3 (not quite as simple)
    >
    > Control over how long to debounce is less common, and often applies
    > to banks of GPIOs not individual signals ... not addressed here.
    >
    > Drivers can request this (where available) with a new IRQF_DEBOUNCED
    > flag/hint passed to request_irq():
    >
    > IF that flag is set when a handler is registered
    > AND the relevant irq_chip supports debouncing
    > AND the IRQ isn't already flagged as being debounced
    > THEN the irq_chip is asked to enable debouncing for this IRQ
    > UNTIL the IRQ's last handler is unregistered.
    > ELSE
    > nothing is done ... the hint is ignored
    > FI
    >
    > Comments?


    How is this going to work with shared interrupt lines? If one handler
    wants debouncing and second handler does not, you'll loose interrupts
    for second handler?

    --
    (english) http://www.livejournal.com/~pavelmachek
    (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pav...rses/blog.html
    --
    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/

  6. Re: [PATCH/RFC] hardware irq debouncing support

    On Monday 06 October 2008, Pavel Machek wrote:
    > How is this going to work with shared interrupt lines? If one handler
    > wants debouncing and second handler does not, you'll loose interrupts
    > for second handler?


    That'd be as hardware dependent as the bad decision to mix such
    signals on one line in the first place! As a rule, nothing gets
    lost -- systems care about such events stable at the millisecond
    granuarity, not nanosecond -- but I can't speak for all possible
    bad hardware designs.

    Recall that the systems with this support generally have no
    shortage of interrupt lines, and thus no reason to share them.
    The boards I'm working with right now have over two hundred
    GPIOs, and all but a handful of them (if you exaggerate "two"
    to be a "handful"!) can be configured as interrupt inputs.

    And there's strong incentive NOT to share them ... if a signal
    is dedicated to e.g. MMC1 card detect, it can gate the regulator
    dedicated to that slot; but not if that signal is shared with
    some other status. If two simple buttons both trigger the same
    interrupt, you can't tell them apart. And so on.

    - Dave
    --
    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/

  7. Re: [PATCH/RFC] hardware irq debouncing support

    On Friday 03 October 2008, Haavard Skinnemoen wrote:
    > David Brownell wrote:
    > > Hardware IRQ debouncing is common for IRQ controllers which are
    > > part of GPIO modules ... they often deal with mechanical switches,
    > > buttons, and so forth. This patch:
    > >
    > > - Provides simple support for that in genirq
    > >
    > > - Includes sample implementations for some Linux systems
    > > which already include non-generic support for this:
    > >
    > > * Atmel SOCs (AT91, AT32 -- the same GPIO module)
    > > * OMAP2/OMAP3 (not quite as simple)
    > >
    > > ...

    >
    > Note that at least for AT91 and AVR32, the terminology used is "glitch
    > filtering", not "debouncing". The filtering period is very short (half
    > a clock cycle), so it probably won't help much when dealing with
    > mechanical switches and buttons.


    Right re mechanical switching -- still needs more debouncing -- but it
    would at least help with some of the contact chatter. Quoting from one
    manual (at91sam9263):

    When the glitch filter is enabled, a glitch with a duration of less
    than 1/2 Master Clock (MCK) cycle is automatically rejected, while a
    pulse with a duration of 1 Master Clock cycle or more is accepted.
    For pulse durations between 1/2 Master Clock cycle and 1 Master Clock
    cycle the pulse may or may not be taken into account, depending on
    the precise timing of its occurrence. Thus for a pulse to be visible
    it must exceed 1 Master Clock cycle, whereas for a glitch to be reliably
    filtered out, its duration must not exceed 1/2 Master Clock cycle.

    The mechanism is fairly typical, except for (a) duration of "one" clock
    cycle, which is sometimes configurable and not infrequently more than
    just one cycle, and (b) "Master" clock, which on those chips is the same
    as the memory clock -- e.g. 100 MHz in normal operation, or maybe half
    that on some boards -- but on other chips is a lot slower.

    Example: OMAP2/OMAP3 has a configurable duration of 1 to 256 in units
    of what I'll guess are really 32 KiHZ clock ticks. The default is one
    tick (31 usec), so it can support up to almost 8 msec.

    And on the chip I'm fighting with just now, the debounce is 30 msec,
    take it or leave it.


    > What kind of guarantees should IRQF_DEBOUNCE provide? Filtering short
    > glitches may be useful, but if drivers start assuming it will do real
    > debouncing of badly filtered switches and buttons, I think we're in for
    > some trouble...


    The quality-of-service question rears its ugly head ...

    If QOS is exposed (e.g. "unsigned debounce_usec" in the irq_desc),
    that sort of begs the question of how to *change* that. I had
    hoped to let someone else address the issue of such interfaces...

    What would you think about advice to debounce by default on the
    order of one millisecond, where hardware allows? Later, ways
    to query and update that QOS could be defined.


    > > Having this mechanism in genirq would let boards remove a bunch of
    > > nonportable code, and would let drivers like gpio_keys, gpio_mouse,
    > > and various touchscreens work more reliably. It'd also let various
    > > SOC-specific MMC and CF card drivers switch over to more standard
    > > (and widely understandable) mechanisms.
    > >
    > > I'd like to submit such updates for the 2.6.28 merge window, in
    > > part to let mainline avoid needing yet another driver-specific
    > > programming interface for IRQ debouncing. (For TWL4030/TPS659x0,
    > > as used in most OMAP3 boards including the Gumstix Overo and the
    > > BeagleBoard.)

    >
    > Given that the limitations of this interface are clearly documented, I'm
    > all for it.


    What changes would you suggest in the $SUBJECT patch then?

    I notice that Documentation/DocBook/genericirq.tmpl doesn't
    do a pretty job of formatting the IRQF_* parameters, but
    that seems fixable.


    > What would be perhaps even more useful is generic software debouncing
    > support. Something like
    >
    > int request_debounced_irq(int irq, unsigned long debounce_us,
    > void (*handler)(void *data), void *data);
    >
    > which would set up a handler which disables the interrupt and sets up a
    > timer which will ack/unmask the interrupt and run the handler.


    Why require "software debouncing" if perhaps the hardware could do
    it all for you?


    > This would mean the "interrupt handler" really gets run in softirq
    > context, and shared interrupt would probably be impossible to support,
    > but I think it would be useful for certain kinds of interrupts.
    >
    > What do you think?


    Seems plausible.

    I won't volunteer to write such a thing myself, but I can easily
    imagine it starting to grow users. At least, in the embedded
    Linux space ... the server/desktop crowd seems unlikely to run
    with that sort of hardware.

    - Dave



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

  8. Re: [PATCH/RFC] hardware irq debouncing support

    David Brownell wrote:
    > On Friday 03 October 2008, Haavard Skinnemoen wrote:
    > > David Brownell wrote:
    > > > Hardware IRQ debouncing is common for IRQ controllers which are
    > > > part of GPIO modules ... they often deal with mechanical switches,
    > > > buttons, and so forth. This patch:
    > > >
    > > > - Provides simple support for that in genirq
    > > >
    > > > - Includes sample implementations for some Linux systems
    > > > which already include non-generic support for this:
    > > >
    > > > * Atmel SOCs (AT91, AT32 -- the same GPIO module)
    > > > * OMAP2/OMAP3 (not quite as simple)
    > > >
    > > > ...

    > >
    > > Note that at least for AT91 and AVR32, the terminology used is "glitch
    > > filtering", not "debouncing". The filtering period is very short (half
    > > a clock cycle), so it probably won't help much when dealing with
    > > mechanical switches and buttons.

    >
    > Right re mechanical switching -- still needs more debouncing -- but it
    > would at least help with some of the contact chatter. Quoting from one
    > manual (at91sam9263):
    >
    > When the glitch filter is enabled, a glitch with a duration of less
    > than 1/2 Master Clock (MCK) cycle is automatically rejected, while a
    > pulse with a duration of 1 Master Clock cycle or more is accepted.
    > For pulse durations between 1/2 Master Clock cycle and 1 Master Clock
    > cycle the pulse may or may not be taken into account, depending on
    > the precise timing of its occurrence. Thus for a pulse to be visible
    > it must exceed 1 Master Clock cycle, whereas for a glitch to be reliably
    > filtered out, its duration must not exceed 1/2 Master Clock cycle.
    >
    > The mechanism is fairly typical, except for (a) duration of "one" clock
    > cycle, which is sometimes configurable and not infrequently more than
    > just one cycle, and (b) "Master" clock, which on those chips is the same
    > as the memory clock -- e.g. 100 MHz in normal operation, or maybe half
    > that on some boards -- but on other chips is a lot slower.


    Good point. I'll suggest switching this mechanism over to use a 32 kHz
    clock in future designs.

    > Example: OMAP2/OMAP3 has a configurable duration of 1 to 256 in units
    > of what I'll guess are really 32 KiHZ clock ticks. The default is one
    > tick (31 usec), so it can support up to almost 8 msec.
    >
    > And on the chip I'm fighting with just now, the debounce is 30 msec,
    > take it or leave it.


    Ok, so the limitations of various chips vary a lot...which means that
    it's difficult to predict what IRQF_DEBOUNCED actually does.

    > > What kind of guarantees should IRQF_DEBOUNCE provide? Filtering short
    > > glitches may be useful, but if drivers start assuming it will do real
    > > debouncing of badly filtered switches and buttons, I think we're in for
    > > some trouble...

    >
    > The quality-of-service question rears its ugly head ...
    >
    > If QOS is exposed (e.g. "unsigned debounce_usec" in the irq_desc),
    > that sort of begs the question of how to *change* that. I had
    > hoped to let someone else address the issue of such interfaces...
    >
    > What would you think about advice to debounce by default on the
    > order of one millisecond, where hardware allows? Later, ways
    > to query and update that QOS could be defined.


    I suppose a good start would be to add a comment saying that.

    On the other hand, I don't see how drivers can even tell if the
    hardware supports IRQF_DEBOUNCED at all...

    > > > Having this mechanism in genirq would let boards remove a bunch of
    > > > nonportable code, and would let drivers like gpio_keys, gpio_mouse,
    > > > and various touchscreens work more reliably. It'd also let various
    > > > SOC-specific MMC and CF card drivers switch over to more standard
    > > > (and widely understandable) mechanisms.
    > > >
    > > > I'd like to submit such updates for the 2.6.28 merge window, in
    > > > part to let mainline avoid needing yet another driver-specific
    > > > programming interface for IRQ debouncing. (For TWL4030/TPS659x0,
    > > > as used in most OMAP3 boards including the Gumstix Overo and the
    > > > BeagleBoard.)

    > >
    > > Given that the limitations of this interface are clearly documented, I'm
    > > all for it.

    >
    > What changes would you suggest in the $SUBJECT patch then?


    Just a comment, really. But I realize that it's difficult to specify
    any guarantees since hardware may give you anything from a few
    nanoseconds to 30 ms...

    > I notice that Documentation/DocBook/genericirq.tmpl doesn't
    > do a pretty job of formatting the IRQF_* parameters, but
    > that seems fixable.


    I'm not all that concerned with cosmetics...just having the information
    somewhere would be nice.

    > > What would be perhaps even more useful is generic software debouncing
    > > support. Something like
    > >
    > > int request_debounced_irq(int irq, unsigned long debounce_us,
    > > void (*handler)(void *data), void *data);
    > >
    > > which would set up a handler which disables the interrupt and sets up a
    > > timer which will ack/unmask the interrupt and run the handler.

    >
    > Why require "software debouncing" if perhaps the hardware could do
    > it all for you?


    Because of the "perhaps" part of your sentence.

    But ok, drivers really shouldn't have to care, so let's call it
    "generic debouncing support".

    > > This would mean the "interrupt handler" really gets run in softirq
    > > context, and shared interrupt would probably be impossible to support,
    > > but I think it would be useful for certain kinds of interrupts.
    > >
    > > What do you think?

    >
    > Seems plausible.
    >
    > I won't volunteer to write such a thing myself, but I can easily
    > imagine it starting to grow users. At least, in the embedded
    > Linux space ... the server/desktop crowd seems unlikely to run
    > with that sort of hardware.


    Maybe we should just add this interface and drop the flag? A flag will
    never be able to convey some important parameters like how long to
    debounce. Then a irq chip implementation can decide to do it in
    hardware if the requested debounce delay matches what the hardware can
    provide.

    Maybe we should let drivers provide a range of acceptable delays so
    that the irq chip driver won't have to guess at how long it is
    acceptable to deviate from the specified delay.

    Haavard
    --
    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/

  9. Re: [PATCH/RFC] hardware irq debouncing support

    On Wednesday 08 October 2008, Haavard Skinnemoen wrote:
    >
    > Ok, so the limitations of various chips vary a lot...which means that
    > it's difficult to predict what IRQF_DEBOUNCED actually does.


    The specific QOS achieved is system-specific; the term for
    that kind of mechanism is "hinting". It's very clearly defined
    what the hint means .. but a given system might not use it.

    The madvise(2) system call is a userspace example of hinting.


    > > > What kind of guarantees should IRQF_DEBOUNCE provide? Filtering short
    > > > glitches may be useful, but if drivers start assuming it will do real
    > > > debouncing of badly filtered switches and buttons, I think we're in for
    > > > some trouble...

    > >
    > > The quality-of-service question rears its ugly head ...
    > >
    > > If QOS is exposed (e.g. "unsigned debounce_usec" in the irq_desc),
    > > that sort of begs the question of how to *change* that. I had
    > > hoped to let someone else address the issue of such interfaces...
    > >
    > > What would you think about advice to debounce by default on the
    > > order of one millisecond, where hardware allows? Later, ways
    > > to query and update that QOS could be defined.

    >
    > I suppose a good start would be to add a comment saying that.
    >
    > On the other hand, I don't see how drivers can even tell if the
    > hardware supports IRQF_DEBOUNCED at all...


    That is, "On the other hand, 'later' is not yet..." ?

    Are you suggesting that debouncing support shouldn't
    be provided without QOS query/update support?


    > > > > Having this mechanism in genirq would let boards remove a bunch of
    > > > > nonportable code, and would let drivers like gpio_keys, gpio_mouse,
    > > > > and various touchscreens work more reliably. It'd also let various
    > > > > SOC-specific MMC and CF card drivers switch over to more standard
    > > > > (and widely understandable) mechanisms.
    > > > >
    > > > > I'd like to submit such updates for the 2.6.28 merge window, in
    > > > > part to let mainline avoid needing yet another driver-specific
    > > > > programming interface for IRQ debouncing. (For TWL4030/TPS659x0,
    > > > > as used in most OMAP3 boards including the Gumstix Overo and the
    > > > > BeagleBoard.)
    > > >
    > > > Given that the limitations of this interface are clearly documented, I'm
    > > > all for it.

    > >
    > > What changes would you suggest in the $SUBJECT patch then?

    >
    > Just a comment, really. But I realize that it's difficult to specify
    > any guarantees since hardware may give you anything from a few
    > nanoseconds to 30 ms...


    Done: "as close to 1 msec as hardware allows". (I think less
    than that is probably too little, and more would likely be OK.)


    > > > What would be perhaps even more useful is generic software debouncing
    > > > support. Something like
    > > >
    > > > int request_debounced_irq(int irq, unsigned long debounce_us,
    > > > void (*handler)(void *data), void *data);


    Note by the way what I think is a problematic assumption there:
    that this *exact* debounce period matters. It seems to be more
    usual that it just fit into a range of reasonable values; a bit
    more or less won't matter, almost by definition.

    (And also, that routine is less functional than request_irq ...)


    > > > which would set up a handler which disables the interrupt and sets up a
    > > > timer which will ack/unmask the interrupt and run the handler.

    > >
    > > Why require "software debouncing" if perhaps the hardware could do
    > > it all for you?

    >
    > Because of the "perhaps" part of your sentence.


    I'm not sure which sentence you refer too, but the first
    "perhaps" above is yours!


    > But ok, drivers really shouldn't have to care, so let's call it
    > "generic debouncing support".


    OK..


    > > > This would mean the "interrupt handler" really gets run in softirq
    > > > context, and shared interrupt would probably be impossible to support,
    > > > but I think it would be useful for certain kinds of interrupts.
    > > >
    > > > What do you think?

    > >
    > > Seems plausible.
    > >
    > > I won't volunteer to write such a thing myself, but I can easily
    > > imagine it starting to grow users. At least, in the embedded
    > > Linux space ... the server/desktop crowd seems unlikely to run
    > > with that sort of hardware.

    >
    > Maybe we should just add this interface and drop the flag?


    What I like about the flag is that it's really simple, a
    "fire and forget" model. Easy for drivers to use. And it
    need not be incompatible with a fancier interface...

    The debounce() method might usefully be changed to take the
    requested delay in microseconds, instead of a boolean. And
    to return the actual delay. That would make it easier to
    support fancier calls later, maybe just exposing the raw
    irq_chip call like

    usecs = set_irq_debounce(irq, range_spec);

    The notion of a request_debounced_irq() needs more cooking
    yet though, IMO.


    > A flag will
    > never be able to convey some important parameters like how long to
    > debounce.


    But how important *is* that detail to most drivers? In practice.
    I susct pethey pick numbers today since they have to debounce with
    software timers, which require numbers.


    > Then a irq chip implementation can decide to do it in
    > hardware if the requested debounce delay matches what the hardware can
    > provide.


    I think irq_chip calls should be limited to hardware support.
    Keep them really simple; put layers on top of them if needed.


    > Maybe we should let drivers provide a range of acceptable delays so
    > that the irq chip driver won't have to guess at how long it is
    > acceptable to deviate from the specified delay.


    I can't see it working otherwise. Of course, maybe there should
    just be generic ranges rather than expecting drivers to understand
    how springy contacts might be on a given board, or how dirty they
    may be to cause other kinds of chatter.

    - Dave

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

  10. Re: [PATCH/RFC] hardware irq debouncing support

    David Brownell wrote:
    > On Wednesday 08 October 2008, Haavard Skinnemoen wrote:
    > >
    > > Ok, so the limitations of various chips vary a lot...which means that
    > > it's difficult to predict what IRQF_DEBOUNCED actually does.

    >
    > The specific QOS achieved is system-specific; the term for
    > that kind of mechanism is "hinting". It's very clearly defined
    > what the hint means .. but a given system might not use it.


    I know that, but is "hinting" really what drivers need?

    > The madvise(2) system call is a userspace example of hinting.


    That's different. Ignoring madvise() hints might hurt performance
    slightly, but it won't result in any fundamentally different behaviour.

    On the other hand, lack of debouncing might cause the gpio_keys driver
    to report 1000 keypresses instead of one when the user pushes a button.
    That's much more harmful.

    So if someone goes and replaces the debounce timer in gpio_keys with
    this IRQF_DEBOUNCE flag, it might work very well on hardware which
    happens to use a 30 ms debounce interval, but will break horribly on
    hardware with shorter debounce intervals.

    > > > What would you think about advice to debounce by default on the
    > > > order of one millisecond, where hardware allows? Later, ways
    > > > to query and update that QOS could be defined.

    > >
    > > I suppose a good start would be to add a comment saying that.
    > >
    > > On the other hand, I don't see how drivers can even tell if the
    > > hardware supports IRQF_DEBOUNCED at all...

    >
    > That is, "On the other hand, 'later' is not yet..." ?


    Right.

    > Are you suggesting that debouncing support shouldn't
    > be provided without QOS query/update support?


    Sort of. I'm just wondering how drivers can rely on a feature which
    provides such vague promises.

    > > Just a comment, really. But I realize that it's difficult to specify
    > > any guarantees since hardware may give you anything from a few
    > > nanoseconds to 30 ms...

    >
    > Done: "as close to 1 msec as hardware allows". (I think less
    > than that is probably too little, and more would likely be OK.)


    It could be zero on hardware which doesn't support debouncing at all.

    > > > > What would be perhaps even more useful is generic software debouncing
    > > > > support. Something like
    > > > >
    > > > > int request_debounced_irq(int irq, unsigned long debounce_us,
    > > > > void (*handler)(void *data), void *data);

    >
    > Note by the way what I think is a problematic assumption there:
    > that this *exact* debounce period matters. It seems to be more
    > usual that it just fit into a range of reasonable values; a bit
    > more or less won't matter, almost by definition.


    Yes, I actually came to that conclusion myself, as you've already seen
    below.

    > (And also, that routine is less functional than request_irq ...)


    I guess we could add a "flags" parameter if that's what you mean.

    > > > > which would set up a handler which disables the interrupt and sets up a
    > > > > timer which will ack/unmask the interrupt and run the handler.
    > > >
    > > > Why require "software debouncing" if perhaps the hardware could do
    > > > it all for you?

    > >
    > > Because of the "perhaps" part of your sentence.

    >
    > I'm not sure which sentence you refer too, but the first
    > "perhaps" above is yours!


    I mean that it's difficult to rely on hardware that "perhaps" can do
    debouncing for you. I think many drivers need to know for sure.

    > > Maybe we should just add this interface and drop the flag?

    >
    > What I like about the flag is that it's really simple, a
    > "fire and forget" model. Easy for drivers to use. And it
    > need not be incompatible with a fancier interface...


    Could be. But I think the examples you provided (gpio_keys and
    gpio_mouse) need stronger guarantees before they can drop their
    debouncing timers.

    > The debounce() method might usefully be changed to take the
    > requested delay in microseconds, instead of a boolean. And
    > to return the actual delay. That would make it easier to
    > support fancier calls later, maybe just exposing the raw
    > irq_chip call like
    >
    > usecs = set_irq_debounce(irq, range_spec);


    Maybe that's better. Just request the interrupt as normal, and then try
    to enable debouncing. If the actual delay is too short, the driver can
    set up its own timer.

    > The notion of a request_debounced_irq() needs more cooking
    > yet though, IMO.


    Yes, it was only a suggestion, I didn't mean to provide a fully-cooked
    interface. But I think something like that (or set_irq_debounce) would
    be useful to many drivers.

    > > A flag will
    > > never be able to convey some important parameters like how long to
    > > debounce.

    >
    > But how important *is* that detail to most drivers? In practice.
    > I susct pethey pick numbers today since they have to debounce with
    > software timers, which require numbers.


    I think it's very important to drivers that have to deal with
    mechanical buttons and switches. In many cases, only the board code
    knows what kind of switch is hooked up to the interrupt, so the driver
    just passes on that information from the platform data.

    > > Then a irq chip implementation can decide to do it in
    > > hardware if the requested debounce delay matches what the hardware can
    > > provide.

    >
    > I think irq_chip calls should be limited to hardware support.
    > Keep them really simple; put layers on top of them if needed.


    Ok, so let's just pass the range spec to the debounce() method, and if
    it fails, the genirq core can decide to do debouncing in software.

    > > Maybe we should let drivers provide a range of acceptable delays so
    > > that the irq chip driver won't have to guess at how long it is
    > > acceptable to deviate from the specified delay.

    >
    > I can't see it working otherwise. Of course, maybe there should
    > just be generic ranges rather than expecting drivers to understand
    > how springy contacts might be on a given board, or how dirty they
    > may be to cause other kinds of chatter.


    Maybe. I'd prefer real numbers to generic-sounding names like
    low/medium/high, but if someone can come up with a good way to express
    this, I guess that could work.

    And I don't really expect drivers to understand this, but drivers can
    get help from the platform or board code.

    Haavard
    --
    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/

  11. Re: [PATCH/RFC] hardware irq debouncing support

    Hi!

    > > > Ok, so the limitations of various chips vary a lot...which means that
    > > > it's difficult to predict what IRQF_DEBOUNCED actually does.

    > >
    > > The specific QOS achieved is system-specific; the term for
    > > that kind of mechanism is "hinting". It's very clearly defined
    > > what the hint means .. but a given system might not use it.

    >
    > I know that, but is "hinting" really what drivers need?
    >
    > > The madvise(2) system call is a userspace example of hinting.

    >
    > That's different. Ignoring madvise() hints might hurt performance
    > slightly, but it won't result in any fundamentally different behaviour.
    >
    > On the other hand, lack of debouncing might cause the gpio_keys driver
    > to report 1000 keypresses instead of one when the user pushes a button.
    > That's much more harmful.
    >
    > So if someone goes and replaces the debounce timer in gpio_keys with
    > this IRQF_DEBOUNCE flag, it might work very well on hardware which
    > happens to use a 30 ms debounce interval, but will break horribly on
    > hardware with shorter debounce intervals.


    Right. So you don't _replace_ debounce timer, you just do both timer
    and IRQF_.

    > > > > Why require "software debouncing" if perhaps the hardware could do
    > > > > it all for you?
    > > >
    > > > Because of the "perhaps" part of your sentence.

    > >
    > > I'm not sure which sentence you refer too, but the first
    > > "perhaps" above is yours!

    >
    > I mean that it's difficult to rely on hardware that "perhaps" can do
    > debouncing for you. I think many drivers need to know for sure.


    Why? You write code as if no debouncing exist...


    --
    (english) http://www.livejournal.com/~pavelmachek
    (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pav...rses/blog.html
    --
    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/

  12. Re: [PATCH/RFC] hardware irq debouncing support

    On Thursday 09 October 2008, Haavard Skinnemoen wrote:
    >
    > On the other hand, lack of debouncing might cause the gpio_keys driver
    > to report 1000 keypresses instead of one when the user pushes a button.
    > That's much more harmful.
    >
    > So if someone goes and replaces the debounce timer in gpio_keys with
    > this IRQF_DEBOUNCE flag, it might work very well on hardware which
    > happens to use a 30 ms debounce interval, but will break horribly on
    > hardware with shorter debounce intervals.


    Agreed that if you want to _replace_ software debouncing with a flag
    that's a pure hint, it wouldn't work. But I didn't suggest that...

    Instead, if you just added IRQF_DEBOUNCE to the existing gpio_keys,
    what you'd get is a reduction in useless IRQs received. Which is
    a behavioral improvement, coming at essentially zero system cost.
    (After discounting the cost of "how to do it" discussions!)

    (As Pavel also observed.)


    > > > > What would you think about advice to debounce by default on the
    > > > > order of one millisecond, where hardware allows? Later, ways
    > > > > to query and update that QOS could be defined.
    > > >
    > > > I suppose a good start would be to add a comment saying that.
    > > >
    > > > On the other hand, I don't see how drivers can even tell if the
    > > > hardware supports IRQF_DEBOUNCED at all...

    > >
    > > That is, "On the other hand, 'later' is not yet..." ?

    >
    > Right.
    >
    > > Are you suggesting that debouncing support shouldn't
    > > be provided without QOS query/update support?

    >
    > Sort of. I'm just wondering how drivers can rely on a feature which
    > provides such vague promises.


    As with madvise(), by just hinting "here's a way to improve
    behavior a bit, if hardware allows". As you noted, ignoring
    such hints would mean some missed optimizations ... but the
    system would still behave correctly.


    > > The debounce() method might usefully be changed to take the
    > > requested delay in microseconds, instead of a boolean. And
    > > to return the actual delay. That would make it easier to
    > > support fancier calls later, maybe just exposing the raw
    > > irq_chip call like
    > >
    > > usecs = set_irq_debounce(irq, range_spec);

    >
    > Maybe that's better. Just request the interrupt as normal, and then try
    > to enable debouncing. If the actual delay is too short, the driver can
    > set up its own timer.


    So we seem to be thinking about two mechanisms:

    - I focussed on hinting, as a lightweight way to kick in a
    common hardware mechanism to improve system behavior;

    - You focussed on abstracting the whole debouncing problem,
    leveraging those mechanisms more highly and (implicitly)
    helping improve much messy software debounce logic.

    Still seems to me there should be no problem having both
    mechanisms, supported by one irq_chip.debounce() hook
    that's a little different from the one in $PATCH. And I
    think you agreed with that.

    Specifically with respect to the Atmel GPIO modules, the
    hinting would eliminate a few glitches but would not be as
    powerful as on some other hardware. (As you had observed
    earlier.) So if I were wearing the same corporate hat you
    are, I'd want Linux to support that second mechanism too!!


    > > > A flag will
    > > > never be able to convey some important parameters like how long to
    > > > debounce.

    > >
    > > But how important *is* that detail to most drivers? In practice.
    > > I suspect they pick numbers today since they have to debounce with
    > > software timers, which require numbers.

    >
    > I think it's very important to drivers that have to deal with
    > mechanical buttons and switches. In many cases, only the board code
    > knows what kind of switch is hooked up to the interrupt, so the driver
    > just passes on that information from the platform data.


    If the platform setup code knows the hardware debounce will
    be requested, it can adjust its platform_data debounce parameter
    accordingly.

    - Dave

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

  13. Re: [PATCH/RFC] hardware irq debouncing support

    David Brownell wrote:
    > On Thursday 09 October 2008, Haavard Skinnemoen wrote:
    > >
    > > On the other hand, lack of debouncing might cause the gpio_keys driver
    > > to report 1000 keypresses instead of one when the user pushes a button.
    > > That's much more harmful.
    > >
    > > So if someone goes and replaces the debounce timer in gpio_keys with
    > > this IRQF_DEBOUNCE flag, it might work very well on hardware which
    > > happens to use a 30 ms debounce interval, but will break horribly on
    > > hardware with shorter debounce intervals.

    >
    > Agreed that if you want to _replace_ software debouncing with a flag
    > that's a pure hint, it wouldn't work. But I didn't suggest that...
    >
    > Instead, if you just added IRQF_DEBOUNCE to the existing gpio_keys,
    > what you'd get is a reduction in useless IRQs received. Which is
    > a behavioral improvement, coming at essentially zero system cost.
    > (After discounting the cost of "how to do it" discussions!)


    Ok, I see. I didn't realize that gpio_keys doesn't disable the
    interrupt while debouncing.

    > > Sort of. I'm just wondering how drivers can rely on a feature which
    > > provides such vague promises.

    >
    > As with madvise(), by just hinting "here's a way to improve
    > behavior a bit, if hardware allows". As you noted, ignoring
    > such hints would mean some missed optimizations ... but the
    > system would still behave correctly.


    Right.

    > > Maybe that's better. Just request the interrupt as normal, and then try
    > > to enable debouncing. If the actual delay is too short, the driver can
    > > set up its own timer.

    >
    > So we seem to be thinking about two mechanisms:
    >
    > - I focussed on hinting, as a lightweight way to kick in a
    > common hardware mechanism to improve system behavior;
    >
    > - You focussed on abstracting the whole debouncing problem,
    > leveraging those mechanisms more highly and (implicitly)
    > helping improve much messy software debounce logic.


    Yes, you're right.

    > Still seems to me there should be no problem having both
    > mechanisms, supported by one irq_chip.debounce() hook
    > that's a little different from the one in $PATCH. And I
    > think you agreed with that.
    >
    > Specifically with respect to the Atmel GPIO modules, the
    > hinting would eliminate a few glitches but would not be as
    > powerful as on some other hardware. (As you had observed
    > earlier.) So if I were wearing the same corporate hat you
    > are, I'd want Linux to support that second mechanism too!!


    I just want a mechanism where you'll end up with the same behaviour
    regardless of what the hardware supports, at the cost of a timer if the
    hardware doesn't support the requested debounce delay. As an added
    benefit, this would eliminate the need for drivers to set up their own
    debounce timers. It would also make such "more powerful" hardware
    features easier to utilize from generic drivers, wouldn't it?

    But you've convinced me that your IRQF_DEBOUNCE hint might be useful
    too.

    > > I think it's very important to drivers that have to deal with
    > > mechanical buttons and switches. In many cases, only the board code
    > > knows what kind of switch is hooked up to the interrupt, so the driver
    > > just passes on that information from the platform data.

    >
    > If the platform setup code knows the hardware debounce will
    > be requested, it can adjust its platform_data debounce parameter
    > accordingly.


    I guess it could, but why would it do that? The debounce delay
    shouldn't depend on the mechanism used to implement it, should it?

    Or are you thinking about compensating for any delays introduced by
    IRQF_DEBOUNCE?

    Haavard
    --
    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/

  14. Re: [PATCH/RFC] hardware irq debouncing support

    On Sunday 12 October 2008, Haavard Skinnemoen wrote:
    > I just want a mechanism where you'll end up with the same behaviour
    > regardless of what the hardware supports, at the cost of a timer if the
    > hardware doesn't support the requested debounce delay. As an added
    > benefit, this would eliminate the need for drivers to set up their own
    > debounce timers. It would also make such "more powerful" hardware
    > features easier to utilize from generic drivers, wouldn't it?
    >
    > But you've convinced me that your IRQF_DEBOUNCE hint might be useful
    > too.


    Just for the record ... I'm withdrawing this proposal, in the sense
    that I won't be pursuing it any further. A lighter weight solution
    is enough for now.

    I think the framework we discussed -- lowlevel irq_chip mechanism
    that can kick in hardware debouncing and report the duration of its
    debounce delay, paired with upper level code that can kick in longer
    software timers as needed -- is probably the right direction to go,
    if anyone really needs this.


    > > If the platform setup code knows the hardware debounce will
    > > be requested, it can adjust its platform_data debounce parameter
    > > accordingly.

    >
    > I guess it could, but why would it do that? The debounce delay
    > shouldn't depend on the mechanism used to implement it, should it?


    The overall delay shouldn't matter, no. But if an 10 msec hardware
    debounce kicks in and you wanted 20 msec total, you'd want something
    to conclude that a 10 msec software timer is more appropriate than
    a 20 msec one. I was pointing out that in one construction, that
    "something" would be platform setup code which knows more about how
    the system is set up than a "dumb" software timer in that driver.

    - Dave

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

+ Reply to Thread