[PATCH/RFC] hardware irq debouncing support

[PATCH/RFC] hardware irq debouncing support

[David Brownell]

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

Re: [PATCH/RFC] hardware irq debouncing support

[Tony Lindgren]

* David Brownell <david-b@pacbell.net> [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 <tony@atomide.com>

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

Re: [PATCH/RFC] hardware irq debouncing support

[David Brownell]

On Friday 03 October 2008, Tony Lindgren wrote:
> * David Brownell <david-b@pacbell.net> [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.  ;)

- 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 <tony@atomide.com>

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

> 
> > 
> > ---
> >  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(-)
> > 

Re: [PATCH/RFC] hardware irq debouncing support

[Tony Lindgren]

* David Brownell <david-b@pacbell.net> [081003 11:45]:
> On Friday 03 October 2008, Tony Lindgren wrote:
> > * David Brownell <david-b@pacbell.net> [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 <tony@atomide.com>
> 
> 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(-)
> > > 

Re: [PATCH/RFC] hardware irq debouncing support

[Haavard Skinnemoen]

David Brownell <david-b@pacbell.net> 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

Re: [PATCH/RFC] hardware irq debouncing support

[David Brownell]

On Friday 03 October 2008, Haavard Skinnemoen wrote:
> David Brownell <david-b@pacbell.net> 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

Re: [PATCH/RFC] hardware irq debouncing support

[Haavard Skinnemoen]

David Brownell <david-b@pacbell.net> wrote:
> On Friday 03 October 2008, Haavard Skinnemoen wrote:
> > David Brownell <david-b@pacbell.net> 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

Re: [PATCH/RFC] hardware irq debouncing support

[David Brownell]

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

Re: [PATCH/RFC] hardware irq debouncing support

[Haavard Skinnemoen]

David Brownell <david-b@pacbell.net> 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

Re: [PATCH/RFC] hardware irq debouncing support

[Pavel Machek]

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/~pavel/picture/horses/blog.html

Re: [PATCH/RFC] hardware irq debouncing support

[David Brownell]

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

Re: [PATCH/RFC] hardware irq debouncing support

[Haavard Skinnemoen]

David Brownell <david-b@pacbell.net> 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

Re: [PATCH/RFC] hardware irq debouncing support

[David Brownell]

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

Re: [PATCH/RFC] hardware irq debouncing support

[Pavel Machek]

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/~pavel/picture/horses/blog.html

Re: [PATCH/RFC] hardware irq debouncing support

[David Brownell]

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