[PATCH] ASoC: Jack: add configurable option for irq_flag

[PATCH] ASoC: Jack: add configurable option for irq_flag

[Omair Mohammed Abdullah]

From: Vaibhav Agarwal <vaibhav.agarwal@intel.com>

For a jack using GPIO based detection, the framework sets it to trigger on both
edges (rising and falling). Some codecs may provide only a falling or rising edge
interrupt but not both.

This patch provides a configurable option for irq_flag during
snd_soc_jack_add_gpios(). If not configured, it sets the interrupt line to
trigger on both edges.

Signed-off-by: Vaibhav Agarwal <vaibhav.agarwal@intel.com>
Signed-off-by: Omair Mohammed Abdullah <omair.m.abdullah@linux.intel.com>
---
 include/sound/soc.h  |    2 ++
 sound/soc/soc-jack.c |    7 +++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index a6a059c..d0286ad 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -585,6 +585,7 @@ struct snd_soc_jack_zone {
  * @report:       value to report when jack detected
  * @invert:       report presence in low state
  * @debouce_time: debouce time in ms
+ * @irq_flag:     Interrupt flags for GPIO-Irq line
  * @wake:	  enable as wake source
  * @jack_status_check: callback function which overrides the detection
  *		       to provide more complex checks (eg, reading an
@@ -597,6 +598,7 @@ struct snd_soc_jack_gpio {
 	int report;
 	int invert;
 	int debounce_time;
+	unsigned long irq_flags;
 	bool wake;
 
 	struct snd_soc_jack *jack;
diff --git a/sound/soc/soc-jack.c b/sound/soc/soc-jack.c
index 0bb5ccc..c9f7953 100644
--- a/sound/soc/soc-jack.c
+++ b/sound/soc/soc-jack.c
@@ -318,10 +318,13 @@ int snd_soc_jack_add_gpios(struct snd_soc_jack *jack, int count,
 		INIT_DELAYED_WORK(&gpios[i].work, gpio_work);
 		gpios[i].jack = jack;
 
+		if (!gpios[i].irq_flags)
+			gpios[i].irq_flags =
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
+
 		ret = request_any_context_irq(gpio_to_irq(gpios[i].gpio),
 					      gpio_handler,
-					      IRQF_TRIGGER_RISING |
-					      IRQF_TRIGGER_FALLING,
+					      gpios[i].irq_flags,
 					      gpios[i].name,
 					      &gpios[i]);
 		if (ret < 0)
-- 
1.7.4.1

Re: [PATCH] ASoC: Jack: add configurable option for irq_flag

[Mark Brown]

On Tue, Feb 12, 2013 at 11:07:02AM +0530, Omair Mohammed Abdullah wrote:
> From: Vaibhav Agarwal <vaibhav.agarwal@intel.com>

> For a jack using GPIO based detection, the framework sets it to trigger on both
> edges (rising and falling). Some codecs may provide only a falling or rising edge
> interrupt but not both.

If the device only interrupts on one edge how does it detect jack
removal?

Re: [PATCH] ASoC: Jack: add configurable option for irq_flag

[Ramesh Babu]

>> For a jack using GPIO based detection, the framework sets it to trigger
>> on both
>> edges (rising and falling). Some codecs may provide only a falling or
>> rising edge
>> interrupt but not both.
>
> If the device only interrupts on one edge how does it detect jack
> removal?
Some codecs clears it's interrupt line when driver reads the codec status
register.
In such cases, the interupt line will go back to normal after driver
reads status register.  During removal, interrupt line will change and
triggers on same edge interrupt.

Re: [PATCH] ASoC: Jack: add configurable option for irq_flag

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 701 bytes --]

On Wed, Feb 13, 2013 at 11:09:57AM +0530, Ramesh Babu wrote:

> > If the device only interrupts on one edge how does it detect jack
> > removal?

> Some codecs clears it's interrupt line when driver reads the codec status
> register.
> In such cases, the interupt line will go back to normal after driver
> reads status register.  During removal, interrupt line will change and
> triggers on same edge interrupt.

Are you sure this is a GPIO that should be used directly (rather than
handled as part of the CODEC driver) and are you sure that the code
handles things like debounce well?

If this really is a GPIO it feels like the controller out to be exposing
itself as having dual edged interrupts.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: Jack: add configurable option for irq_flag

[Omair M. Abdullah]

On Wed, Feb 13, 2013 at 01:19:17PM +0000, Mark Brown wrote:
> On Wed, Feb 13, 2013 at 11:09:57AM +0530, Ramesh Babu wrote:
> 
> > > If the device only interrupts on one edge how does it detect jack removal?
> 
This patch is mainly to handle GPIOs where there is some toggling of the GPIO
lines due to the switch bouncing, and the debounce time cannot be increased
further due to other constraints. In such cases, providing the specific
edge on which to trigger the interrupt helps increase the robustness.

     ___   _                _   _________
e.g.    |_| |______________| |_|
insert followed by removal, where we want to trigger on the falling edge in both
cases.

Re: [PATCH] ASoC: Jack: add configurable option for irq_flag

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1041 bytes --]

On Fri, Feb 15, 2013 at 12:40:00PM +0530, Omair M. Abdullah wrote:

> This patch is mainly to handle GPIOs where there is some toggling of the GPIO
> lines due to the switch bouncing, and the debounce time cannot be increased
> further due to other constraints. In such cases, providing the specific
> edge on which to trigger the interrupt helps increase the robustness.

>      ___   _                _   _________
> e.g.    |_| |______________| |_|

> insert followed by removal, where we want to trigger on the falling edge in both
> cases.

This doesn't make much sense to me, it's a *very* non-obvious change and
it doesn't reflect what's actually happening well.  If you happen to be
lucky and get no bounce it'll fail.  If it's working on your system
there is a fair element of luck in there.

It sounds like all you're looking for here is a better debounce
algorithm, for example one that delays for a bit then starts polling the
GPIO state at a higher rate and declares a result when the GPIO state
doesn't change for a few polls.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: Jack: add configurable option for irq_flag

[Omair M. Abdullah]

On Fri, Feb 15, 2013 at 12:17:27PM +0000, Mark Brown wrote:
> On Fri, Feb 15, 2013 at 12:40:00PM +0530, Omair M. Abdullah wrote:
> 
> > This patch is mainly to handle GPIOs where there is some toggling of the GPIO
> > lines due to the switch bouncing, and the debounce time cannot be increased
> > further due to other constraints. In such cases, providing the specific
> > edge on which to trigger the interrupt helps increase the robustness.
> 
> >      ___   _                _   _________
> > e.g.    |_| |______________| |_|
> 
> > insert followed by removal, where we want to trigger on the falling edge in both
> > cases.
> 
> This doesn't make much sense to me, it's a *very* non-obvious change and
> it doesn't reflect what's actually happening well.  If you happen to be
> lucky and get no bounce it'll fail.  If it's working on your system
> there is a fair element of luck in there.
> 
> It sounds like all you're looking for here is a better debounce
> algorithm, for example one that delays for a bit then starts polling the
> GPIO state at a higher rate and declares a result when the GPIO state
> doesn't change for a few polls.
We are using a polling mechanism in our system to check the jack state a few
times. But what we observed is that we always get a bounce.

Also, we do have a system where we are using the snd_soc_jack_gpio code for a
codec interrupt through a GPIO line, like Ramesh mentioned - even if it is just
for re-using the software debounce mechanism. In such cases, the interrupt would
be triggered on one edge only. Maybe that is not the original intent the of that
code?

Re: [PATCH] ASoC: Jack: add configurable option for irq_flag

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1263 bytes --]

On Sun, Feb 17, 2013 at 12:07:25PM +0530, Omair M. Abdullah wrote:
> On Fri, Feb 15, 2013 at 12:17:27PM +0000, Mark Brown wrote:

> > It sounds like all you're looking for here is a better debounce
> > algorithm, for example one that delays for a bit then starts polling the
> > GPIO state at a higher rate and declares a result when the GPIO state
> > doesn't change for a few polls.

> We are using a polling mechanism in our system to check the jack state a few
> times. But what we observed is that we always get a bounce.

If you're already doing that then this is at best redundant and at worst
will make things worse if you do happen to hit a case where you don't
see any bounce for some reason.

> Also, we do have a system where we are using the snd_soc_jack_gpio code for a
> codec interrupt through a GPIO line, like Ramesh mentioned - even if it is just
> for re-using the software debounce mechanism. In such cases, the interrupt would
> be triggered on one edge only. Maybe that is not the original intent the of that
> code?

What you're describing does not sound at all sane, the GPIO jack code is
there for managing GPIO jacks not for providing a generic debounce
mechanism for interrupts.  I'm not sure how you'd actually go about
doing this...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: Jack: add configurable option for irq_flag

[Omair M. Abdullah]

On Sun, Feb 17, 2013 at 06:11:45PM +0000, Mark Brown wrote:
> On Sun, Feb 17, 2013 at 12:07:25PM +0530, Omair M. Abdullah wrote:
> > On Fri, Feb 15, 2013 at 12:17:27PM +0000, Mark Brown wrote:
> 
> > > It sounds like all you're looking for here is a better debounce
> > > algorithm, for example one that delays for a bit then starts polling the
> > > GPIO state at a higher rate and declares a result when the GPIO state
> > > doesn't change for a few polls.
> 
> > We are using a polling mechanism in our system to check the jack state a few
> > times. But what we observed is that we always get a bounce.
> 
> If you're already doing that then this is at best redundant and at worst
> will make things worse if you do happen to hit a case where you don't
> see any bounce for some reason.
> 
> > Also, we do have a system where we are using the snd_soc_jack_gpio code for a
> > codec interrupt through a GPIO line, like Ramesh mentioned - even if it is just
> > for re-using the software debounce mechanism. In such cases, the interrupt would
> > be triggered on one edge only. Maybe that is not the original intent the of that
> > code?
> 
> What you're describing does not sound at all sane, the GPIO jack code is
> there for managing GPIO jacks not for providing a generic debounce
> mechanism for interrupts.  I'm not sure how you'd actually go about
> doing this...
If you have the codec interrupt line which is connected to the SoC through a
GPIO then this is possible.

Re: [PATCH] ASoC: Jack: add configurable option for irq_flag

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 615 bytes --]

On Mon, Feb 18, 2013 at 06:25:17PM +0530, Omair M. Abdullah wrote:
> On Sun, Feb 17, 2013 at 06:11:45PM +0000, Mark Brown wrote:

> > What you're describing does not sound at all sane, the GPIO jack code is
> > there for managing GPIO jacks not for providing a generic debounce
> > mechanism for interrupts.  I'm not sure how you'd actually go about
> > doing this...

> If you have the codec interrupt line which is connected to the SoC through a
> GPIO then this is possible.

No, that doesn't make much sense.  How is the CODEC IRQ ever going to
get delivered?  And in general this doesn't seem sensible anyway.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]