mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [RFC] ASoC: codecs: msm8916-wcd-analog: use btn0 released detection
@ 2017-09-13 20:43 Damien Riegel
  2017-09-18  9:08 ` Srinivas Kandagatla
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Damien Riegel @ 2017-09-13 20:43 UTC (permalink / raw)
  To: alsa-devel, linux-kernel
  Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
	Srinivas Kandagatla, Damien Riegel

msm8916-wcd-analog uses button0 to differentiate between headphone and
headset. Under some circumstances, button pressed and released
interrupts are not fired as the driver expects it.

For instance, with some connectors, there are spurious button-pressed
interrupts when unplugging a headphone, without the corresponding
button-released interrupt. But the codec always alternates between
button pressed and released interrupts, it cannot fire two interrupts of
the same kind in a row. That means that when the headphone is plugged
back, only a button-released interrupt will be fired instead of pressed
then released. This causes the driver to report headphone as headset.

By changing the logic and relying on button 0 release interrupt, the
driver could be made more robust for connectors that differ from the one
used on the Dragonboard's audio mezzanine.

Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
---
 sound/soc/codecs/msm8916-wcd-analog.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
index 549c269acc7d..f562f2d86907 100644
--- a/sound/soc/codecs/msm8916-wcd-analog.c
+++ b/sound/soc/codecs/msm8916-wcd-analog.c
@@ -285,7 +285,7 @@ struct pm8916_wcd_analog_priv {
 	u16 codec_version;
 	bool	mbhc_btn_enabled;
 	/* special event to detect accessory type */
-	bool	mbhc_btn0_pressed;
+	int	mbhc_btn0_released;
 	bool	detect_accessory_type;
 	struct clk *mclk;
 	struct snd_soc_codec *codec;
@@ -483,7 +483,7 @@ static void pm8916_wcd_setup_mbhc(struct pm8916_wcd_analog_priv *wcd)
 
 	snd_soc_update_bits(codec, CDC_D_INT_EN_CLR, int_en_mask, 0);
 	snd_soc_update_bits(codec, CDC_D_INT_EN_SET, int_en_mask, int_en_mask);
-	wcd->mbhc_btn0_pressed = false;
+	wcd->mbhc_btn0_released = false;
 	wcd->detect_accessory_type = true;
 }
 
@@ -950,7 +950,7 @@ static irqreturn_t mbhc_btn_release_irq_handler(int irq, void *arg)
 
 		/* check if its BTN0 thats released */
 		if ((val != -1) && !(val & CDC_A_MBHC_RESULT_1_BTN_RESULT_MASK))
-			priv->mbhc_btn0_pressed = false;
+			priv->mbhc_btn0_released = true;
 
 	} else {
 		snd_soc_jack_report(priv->jack, 0, btn_mask);
@@ -983,9 +983,7 @@ static irqreturn_t mbhc_btn_press_irq_handler(int irq, void *arg)
 		break;
 	case 0x0:
 		/* handle BTN_0 specially for type detection */
-		if (priv->detect_accessory_type)
-			priv->mbhc_btn0_pressed = true;
-		else
+		if (!priv->detect_accessory_type)
 			snd_soc_jack_report(priv->jack,
 					    SND_JACK_BTN_0, btn_mask);
 		break;
@@ -1029,19 +1027,19 @@ static irqreturn_t pm8916_mbhc_switch_irq_handler(int irq, void *arg)
 		 * both press and release event received then its
 		 * a headset.
 		 */
-		if (priv->mbhc_btn0_pressed)
+		if (priv->mbhc_btn0_released)
 			snd_soc_jack_report(priv->jack,
-					    SND_JACK_HEADPHONE, hs_jack_mask);
+					    SND_JACK_HEADSET, hs_jack_mask);
 		else
 			snd_soc_jack_report(priv->jack,
-					    SND_JACK_HEADSET, hs_jack_mask);
+					    SND_JACK_HEADPHONE, hs_jack_mask);
 
 		priv->detect_accessory_type = false;
 
 	} else { /* removal */
 		snd_soc_jack_report(priv->jack, 0, hs_jack_mask);
 		priv->detect_accessory_type = true;
-		priv->mbhc_btn0_pressed = false;
+		priv->mbhc_btn0_released = false;
 	}
 
 	return IRQ_HANDLED;
-- 
2.14.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] ASoC: codecs: msm8916-wcd-analog: use btn0 released detection
  2017-09-13 20:43 [RFC] ASoC: codecs: msm8916-wcd-analog: use btn0 released detection Damien Riegel
@ 2017-09-18  9:08 ` Srinivas Kandagatla
  2017-09-19 12:11   ` Mark Brown
  2017-09-28 18:58 ` Applied "ASoC: codecs: msm8916-wcd-analog: use btn0 released detection" to the asoc tree Mark Brown
  2017-09-28 19:56 ` [RFC] ASoC: codecs: msm8916-wcd-analog: use btn0 released detection Srinivas Kandagatla
  2 siblings, 1 reply; 8+ messages in thread
From: Srinivas Kandagatla @ 2017-09-18  9:08 UTC (permalink / raw)
  To: Damien Riegel, alsa-devel, linux-kernel
  Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai



On 13/09/17 21:43, Damien Riegel wrote:
> msm8916-wcd-analog uses button0 to differentiate between headphone and
> headset. Under some circumstances, button pressed and released
> interrupts are not fired as the driver expects it.
> 

This is what we need to understand to find a right solution,
I would like to understand on what is the difference in the hw layout.

To start with could you share the schematics/connections differences of 
this area on how the lines are wired up, I would like to compare it with

https://www.dropbox.com/s/zqqrqhd9d2yw7pr/Audio%C2%A0Mezzanine%C2%A0Board-SCH-0113.pdf?dl=0


thanks,
srini

> For instance, with some connectors, there are spurious button-pressed
> interrupts when unplugging a headphone, without the corresponding
> button-released interrupt. 
But the codec always alternates between
> button pressed and released interrupts, it cannot fire two interrupts of
> the same kind in a row. That means that when the headphone is plugged
> back, only a button-released interrupt will be fired instead of pressed
> then released. This causes the driver to report headphone as headset.
> 
> By changing the logic and relying on button 0 release interrupt, the
> driver could be made more robust for connectors that differ from the one
> used on the Dragonboard's audio mezzanine.
> 
> Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> ---
>   sound/soc/codecs/msm8916-wcd-analog.c | 18 ++++++++----------
>   1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
> index 549c269acc7d..f562f2d86907 100644
> --- a/sound/soc/codecs/msm8916-wcd-analog.c
> +++ b/sound/soc/codecs/msm8916-wcd-analog.c
> @@ -285,7 +285,7 @@ struct pm8916_wcd_analog_priv {
>   	u16 codec_version;
>   	bool	mbhc_btn_enabled;
>   	/* special event to detect accessory type */
> -	bool	mbhc_btn0_pressed;
> +	int	mbhc_btn0_released;
>   	bool	detect_accessory_type;
>   	struct clk *mclk;
>   	struct snd_soc_codec *codec;
> @@ -483,7 +483,7 @@ static void pm8916_wcd_setup_mbhc(struct pm8916_wcd_analog_priv *wcd)
>   
>   	snd_soc_update_bits(codec, CDC_D_INT_EN_CLR, int_en_mask, 0);
>   	snd_soc_update_bits(codec, CDC_D_INT_EN_SET, int_en_mask, int_en_mask);
> -	wcd->mbhc_btn0_pressed = false;
> +	wcd->mbhc_btn0_released = false;
>   	wcd->detect_accessory_type = true;
>   }
>   
> @@ -950,7 +950,7 @@ static irqreturn_t mbhc_btn_release_irq_handler(int irq, void *arg)
>   
>   		/* check if its BTN0 thats released */
>   		if ((val != -1) && !(val & CDC_A_MBHC_RESULT_1_BTN_RESULT_MASK))
> -			priv->mbhc_btn0_pressed = false;
> +			priv->mbhc_btn0_released = true;
>   
>   	} else {
>   		snd_soc_jack_report(priv->jack, 0, btn_mask);
> @@ -983,9 +983,7 @@ static irqreturn_t mbhc_btn_press_irq_handler(int irq, void *arg)
>   		break;
>   	case 0x0:
>   		/* handle BTN_0 specially for type detection */
> -		if (priv->detect_accessory_type)
> -			priv->mbhc_btn0_pressed = true;
> -		else
> +		if (!priv->detect_accessory_type)
>   			snd_soc_jack_report(priv->jack,
>   					    SND_JACK_BTN_0, btn_mask);
>   		break;
> @@ -1029,19 +1027,19 @@ static irqreturn_t pm8916_mbhc_switch_irq_handler(int irq, void *arg)
>   		 * both press and release event received then its
>   		 * a headset.
>   		 */
> -		if (priv->mbhc_btn0_pressed)
> +		if (priv->mbhc_btn0_released)
>   			snd_soc_jack_report(priv->jack,
> -					    SND_JACK_HEADPHONE, hs_jack_mask);
> +					    SND_JACK_HEADSET, hs_jack_mask);
>   		else
>   			snd_soc_jack_report(priv->jack,
> -					    SND_JACK_HEADSET, hs_jack_mask);
> +					    SND_JACK_HEADPHONE, hs_jack_mask);
>   
>   		priv->detect_accessory_type = false;
>   
>   	} else { /* removal */
>   		snd_soc_jack_report(priv->jack, 0, hs_jack_mask);
>   		priv->detect_accessory_type = true;
> -		priv->mbhc_btn0_pressed = false;
> +		priv->mbhc_btn0_released = false;
>   	}
>   
>   	return IRQ_HANDLED;
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] ASoC: codecs: msm8916-wcd-analog: use btn0 released detection
  2017-09-18  9:08 ` Srinivas Kandagatla
@ 2017-09-19 12:11   ` Mark Brown
  2017-09-19 13:22     ` Damien Riegel
  0 siblings, 1 reply; 8+ messages in thread
From: Mark Brown @ 2017-09-19 12:11 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Damien Riegel, alsa-devel, linux-kernel, Liam Girdwood,
	Jaroslav Kysela, Takashi Iwai

[-- Attachment #1: Type: text/plain, Size: 614 bytes --]

On Mon, Sep 18, 2017 at 10:08:04AM +0100, Srinivas Kandagatla wrote:
> On 13/09/17 21:43, Damien Riegel wrote:
> > msm8916-wcd-analog uses button0 to differentiate between headphone and
> > headset. Under some circumstances, button pressed and released
> > interrupts are not fired as the driver expects it.

> This is what we need to understand to find a right solution,
> I would like to understand on what is the difference in the hw layout.

These sorts of problems can also occur because of differences in test
process - pressure at different angles, different speeds of insertion
and a million other things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] ASoC: codecs: msm8916-wcd-analog: use btn0 released detection
  2017-09-19 12:11   ` Mark Brown
@ 2017-09-19 13:22     ` Damien Riegel
  2017-09-28 18:18       ` Srinivas Kandagatla
  0 siblings, 1 reply; 8+ messages in thread
From: Damien Riegel @ 2017-09-19 13:22 UTC (permalink / raw)
  To: Mark Brown
  Cc: Srinivas Kandagatla, alsa-devel, linux-kernel, Liam Girdwood,
	Jaroslav Kysela, Takashi Iwai

On Tue, Sep 19, 2017 at 01:11:47PM +0100, Mark Brown wrote:
> On Mon, Sep 18, 2017 at 10:08:04AM +0100, Srinivas Kandagatla wrote:
> > On 13/09/17 21:43, Damien Riegel wrote:
> > > msm8916-wcd-analog uses button0 to differentiate between headphone and
> > > headset. Under some circumstances, button pressed and released
> > > interrupts are not fired as the driver expects it.
> 
> > This is what we need to understand to find a right solution,
> > I would like to understand on what is the difference in the hw layout.

I asked the hardware team and the design is exactly the same, but we use
different mechanical parts (ie. the jack connector).

We've started comparing electrical signals between "Android on
Intrinsyc's eval kit" vs. "Linux on our device", just to get a broad
idea of what might be happening, and today we'll compare the two
hardware with the same software on it. I'll get back to you as soon as I
have more information.

> These sorts of problems can also occur because of differences in test
> process - pressure at different angles, different speeds of insertion
> and a million other things.

Definitely. I also noticed an issue with the first very detection
because the micbias will only be set the first time the driver gets a
mechanical insertion interrupt. Following snippet seems to solve the
problem:

diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
index f562f2d86907..5045dabd9ea9 100644
--- a/sound/soc/codecs/msm8916-wcd-analog.c
+++ b/sound/soc/codecs/msm8916-wcd-analog.c
@@ -446,6 +446,7 @@ static int pm8916_wcd_analog_enable_micbias_int1(struct
 static void pm8916_wcd_setup_mbhc(struct pm8916_wcd_analog_priv *wcd)
 {
        struct snd_soc_codec *codec = wcd->codec;
+       bool micbias_enabled = false;
        u32 plug_type = 0;
        u32 int_en_mask;

@@ -477,6 +478,11 @@ static void pm8916_wcd_setup_mbhc(struct pm8916_wcd_analog_priv *wcd)
                            DIG_CLK_CTL_D_MBHC_CLK_EN_MASK,
                            DIG_CLK_CTL_D_MBHC_CLK_EN);

+       if (snd_soc_read(codec, CDC_A_MICB_2_EN) & CDC_A_MICB_2_EN_ENABLE)
+               micbias_enabled = true;
+
+       pm8916_mbhc_configure_bias(priv, micbias_enabled);
+
        int_en_mask = MBHC_SWITCH_INT;
        if (wcd->mbhc_btn_enabled)
                int_en_mask |= MBHC_BUTTON_PRESS_DET | MBHC_BUTTON_RELEASE_DET;

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] ASoC: codecs: msm8916-wcd-analog: use btn0 released detection
  2017-09-19 13:22     ` Damien Riegel
@ 2017-09-28 18:18       ` Srinivas Kandagatla
  2017-09-28 19:10         ` Damien Riegel
  0 siblings, 1 reply; 8+ messages in thread
From: Srinivas Kandagatla @ 2017-09-28 18:18 UTC (permalink / raw)
  To: Damien Riegel, Mark Brown
  Cc: alsa-devel, linux-kernel, Liam Girdwood, Jaroslav Kysela, Takashi Iwai



On 19/09/17 06:22, Damien Riegel wrote:
> Definitely. I also noticed an issue with the first very detection
> because the micbias will only be set the first time the driver gets a
> mechanical insertion interrupt. Following snippet seems to solve the
> problem:
Does that mean that you do not need the "ASoC: codecs: 
msm8916-wcd-analog: use btn0 released detection" patch anymore?

Below changes seems to fine as it is.

Acked-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>


> diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
> index f562f2d86907..5045dabd9ea9 100644
> --- a/sound/soc/codecs/msm8916-wcd-analog.c
> +++ b/sound/soc/codecs/msm8916-wcd-analog.c
> @@ -446,6 +446,7 @@ static int pm8916_wcd_analog_enable_micbias_int1(struct
>   static void pm8916_wcd_setup_mbhc(struct pm8916_wcd_analog_priv *wcd)
>   {
>          struct snd_soc_codec *codec = wcd->codec;
> +       bool micbias_enabled = false;
>          u32 plug_type = 0;
>          u32 int_en_mask;
> 
> @@ -477,6 +478,11 @@ static void pm8916_wcd_setup_mbhc(struct pm8916_wcd_analog_priv *wcd)
>                              DIG_CLK_CTL_D_MBHC_CLK_EN_MASK,
>                              DIG_CLK_CTL_D_MBHC_CLK_EN);
> 
> +       if (snd_soc_read(codec, CDC_A_MICB_2_EN) & CDC_A_MICB_2_EN_ENABLE)
> +               micbias_enabled = true;
> +
> +       pm8916_mbhc_configure_bias(priv, micbias_enabled);
> +
>          int_en_mask = MBHC_SWITCH_INT;
>          if (wcd->mbhc_btn_enabled)
>                  int_en_mask |= MBHC_BUTTON_PRESS_DET | MBHC_BUTTON_RELEASE_DET;
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Applied "ASoC: codecs: msm8916-wcd-analog: use btn0 released detection" to the asoc tree
  2017-09-13 20:43 [RFC] ASoC: codecs: msm8916-wcd-analog: use btn0 released detection Damien Riegel
  2017-09-18  9:08 ` Srinivas Kandagatla
@ 2017-09-28 18:58 ` Mark Brown
  2017-09-28 19:56 ` [RFC] ASoC: codecs: msm8916-wcd-analog: use btn0 released detection Srinivas Kandagatla
  2 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2017-09-28 18:58 UTC (permalink / raw)
  To: Damien Riegel
  Cc: Mark Brown, alsa-devel, linux-kernel, Takashi Iwai,
	Liam Girdwood, Mark Brown, Srinivas Kandagatla, alsa-devel

The patch

   ASoC: codecs: msm8916-wcd-analog: use btn0 released detection

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From d430a7e3abbf98b4bcb16522cab8e4591775883e Mon Sep 17 00:00:00 2001
From: Damien Riegel <damien.riegel@savoirfairelinux.com>
Date: Wed, 13 Sep 2017 16:43:55 -0400
Subject: [PATCH] ASoC: codecs: msm8916-wcd-analog: use btn0 released detection

msm8916-wcd-analog uses button0 to differentiate between headphone and
headset. Under some circumstances, button pressed and released
interrupts are not fired as the driver expects it.

For instance, with some connectors, there are spurious button-pressed
interrupts when unplugging a headphone, without the corresponding
button-released interrupt. But the codec always alternates between
button pressed and released interrupts, it cannot fire two interrupts of
the same kind in a row. That means that when the headphone is plugged
back, only a button-released interrupt will be fired instead of pressed
then released. This causes the driver to report headphone as headset.

By changing the logic and relying on button 0 release interrupt, the
driver could be made more robust for connectors that differ from the one
used on the Dragonboard's audio mezzanine.

Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/msm8916-wcd-analog.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
index 549c269acc7d..f562f2d86907 100644
--- a/sound/soc/codecs/msm8916-wcd-analog.c
+++ b/sound/soc/codecs/msm8916-wcd-analog.c
@@ -285,7 +285,7 @@ struct pm8916_wcd_analog_priv {
 	u16 codec_version;
 	bool	mbhc_btn_enabled;
 	/* special event to detect accessory type */
-	bool	mbhc_btn0_pressed;
+	int	mbhc_btn0_released;
 	bool	detect_accessory_type;
 	struct clk *mclk;
 	struct snd_soc_codec *codec;
@@ -483,7 +483,7 @@ static void pm8916_wcd_setup_mbhc(struct pm8916_wcd_analog_priv *wcd)
 
 	snd_soc_update_bits(codec, CDC_D_INT_EN_CLR, int_en_mask, 0);
 	snd_soc_update_bits(codec, CDC_D_INT_EN_SET, int_en_mask, int_en_mask);
-	wcd->mbhc_btn0_pressed = false;
+	wcd->mbhc_btn0_released = false;
 	wcd->detect_accessory_type = true;
 }
 
@@ -950,7 +950,7 @@ static irqreturn_t mbhc_btn_release_irq_handler(int irq, void *arg)
 
 		/* check if its BTN0 thats released */
 		if ((val != -1) && !(val & CDC_A_MBHC_RESULT_1_BTN_RESULT_MASK))
-			priv->mbhc_btn0_pressed = false;
+			priv->mbhc_btn0_released = true;
 
 	} else {
 		snd_soc_jack_report(priv->jack, 0, btn_mask);
@@ -983,9 +983,7 @@ static irqreturn_t mbhc_btn_press_irq_handler(int irq, void *arg)
 		break;
 	case 0x0:
 		/* handle BTN_0 specially for type detection */
-		if (priv->detect_accessory_type)
-			priv->mbhc_btn0_pressed = true;
-		else
+		if (!priv->detect_accessory_type)
 			snd_soc_jack_report(priv->jack,
 					    SND_JACK_BTN_0, btn_mask);
 		break;
@@ -1029,19 +1027,19 @@ static irqreturn_t pm8916_mbhc_switch_irq_handler(int irq, void *arg)
 		 * both press and release event received then its
 		 * a headset.
 		 */
-		if (priv->mbhc_btn0_pressed)
+		if (priv->mbhc_btn0_released)
 			snd_soc_jack_report(priv->jack,
-					    SND_JACK_HEADPHONE, hs_jack_mask);
+					    SND_JACK_HEADSET, hs_jack_mask);
 		else
 			snd_soc_jack_report(priv->jack,
-					    SND_JACK_HEADSET, hs_jack_mask);
+					    SND_JACK_HEADPHONE, hs_jack_mask);
 
 		priv->detect_accessory_type = false;
 
 	} else { /* removal */
 		snd_soc_jack_report(priv->jack, 0, hs_jack_mask);
 		priv->detect_accessory_type = true;
-		priv->mbhc_btn0_pressed = false;
+		priv->mbhc_btn0_released = false;
 	}
 
 	return IRQ_HANDLED;
-- 
2.14.1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] ASoC: codecs: msm8916-wcd-analog: use btn0 released detection
  2017-09-28 18:18       ` Srinivas Kandagatla
@ 2017-09-28 19:10         ` Damien Riegel
  0 siblings, 0 replies; 8+ messages in thread
From: Damien Riegel @ 2017-09-28 19:10 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Mark Brown, alsa-devel, linux-kernel, Liam Girdwood,
	Jaroslav Kysela, Takashi Iwai

Mark,


I think Srinivas' Ack was only for the snippet quoted below, not for the
original patch of this thread. I don't know if you still want to take
that patch without his Ack or not.

On Thu, Sep 28, 2017 at 11:18:26AM -0700, Srinivas Kandagatla wrote:
> 
> 
> On 19/09/17 06:22, Damien Riegel wrote:
> > Definitely. I also noticed an issue with the first very detection
> > because the micbias will only be set the first time the driver gets a
> > mechanical insertion interrupt. Following snippet seems to solve the
> > problem:
> Does that mean that you do not need the "ASoC: codecs: msm8916-wcd-analog:
> use btn0 released detection" patch anymore?

No, configuring the bias in setup_mbhc only fixes an issue with the very
first detection, we are dealing with two different issues. But I haven't
had the time recently to capture the signal on our board to show you how
our hardware behaves.

> Below changes seems to fine as it is.
> 
> Acked-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Thanks, I'll resend a proper patch with a real commit message then.

-- 
Damien

> 
> 
> > diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
> > index f562f2d86907..5045dabd9ea9 100644
> > --- a/sound/soc/codecs/msm8916-wcd-analog.c
> > +++ b/sound/soc/codecs/msm8916-wcd-analog.c
> > @@ -446,6 +446,7 @@ static int pm8916_wcd_analog_enable_micbias_int1(struct
> >   static void pm8916_wcd_setup_mbhc(struct pm8916_wcd_analog_priv *wcd)
> >   {
> >          struct snd_soc_codec *codec = wcd->codec;
> > +       bool micbias_enabled = false;
> >          u32 plug_type = 0;
> >          u32 int_en_mask;
> > 
> > @@ -477,6 +478,11 @@ static void pm8916_wcd_setup_mbhc(struct pm8916_wcd_analog_priv *wcd)
> >                              DIG_CLK_CTL_D_MBHC_CLK_EN_MASK,
> >                              DIG_CLK_CTL_D_MBHC_CLK_EN);
> > 
> > +       if (snd_soc_read(codec, CDC_A_MICB_2_EN) & CDC_A_MICB_2_EN_ENABLE)
> > +               micbias_enabled = true;
> > +
> > +       pm8916_mbhc_configure_bias(priv, micbias_enabled);
> > +
> >          int_en_mask = MBHC_SWITCH_INT;
> >          if (wcd->mbhc_btn_enabled)
> >                  int_en_mask |= MBHC_BUTTON_PRESS_DET | MBHC_BUTTON_RELEASE_DET;
> > 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC] ASoC: codecs: msm8916-wcd-analog: use btn0 released detection
  2017-09-13 20:43 [RFC] ASoC: codecs: msm8916-wcd-analog: use btn0 released detection Damien Riegel
  2017-09-18  9:08 ` Srinivas Kandagatla
  2017-09-28 18:58 ` Applied "ASoC: codecs: msm8916-wcd-analog: use btn0 released detection" to the asoc tree Mark Brown
@ 2017-09-28 19:56 ` Srinivas Kandagatla
  2 siblings, 0 replies; 8+ messages in thread
From: Srinivas Kandagatla @ 2017-09-28 19:56 UTC (permalink / raw)
  To: Damien Riegel, alsa-devel, linux-kernel
  Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai



On 13/09/17 13:43, Damien Riegel wrote:
> msm8916-wcd-analog uses button0 to differentiate between headphone and
> headset. Under some circumstances, button pressed and released
> interrupts are not fired as the driver expects it.
> 
> For instance, with some connectors, there are spurious button-pressed
> interrupts when unplugging a headphone, without the corresponding
> button-released interrupt. But the codec always alternates between
> button pressed and released interrupts, it cannot fire two interrupts of
> the same kind in a row. That means that when the headphone is plugged
> back, only a button-released interrupt will be fired instead of pressed
> then released. This causes the driver to report headphone as headset.
> 
> By changing the logic and relying on button 0 release interrupt, the
> driver could be made more robust for connectors that differ from the one
> used on the Dragonboard's audio mezzanine.
> 
> Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>


Acked-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> ---


>   sound/soc/codecs/msm8916-wcd-analog.c | 18 ++++++++----------
>   1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
> index 549c269acc7d..f562f2d86907 100644
> --- a/sound/soc/codecs/msm8916-wcd-analog.c
> +++ b/sound/soc/codecs/msm8916-wcd-analog.c
> @@ -285,7 +285,7 @@ struct pm8916_wcd_analog_priv {
>   	u16 codec_version;
>   	bool	mbhc_btn_enabled;
>   	/* special event to detect accessory type */
> -	bool	mbhc_btn0_pressed;
> +	int	mbhc_btn0_released;
>   	bool	detect_accessory_type;
>   	struct clk *mclk;
>   	struct snd_soc_codec *codec;
> @@ -483,7 +483,7 @@ static void pm8916_wcd_setup_mbhc(struct pm8916_wcd_analog_priv *wcd)
>   
>   	snd_soc_update_bits(codec, CDC_D_INT_EN_CLR, int_en_mask, 0);
>   	snd_soc_update_bits(codec, CDC_D_INT_EN_SET, int_en_mask, int_en_mask);
> -	wcd->mbhc_btn0_pressed = false;
> +	wcd->mbhc_btn0_released = false;
>   	wcd->detect_accessory_type = true;
>   }
>   
> @@ -950,7 +950,7 @@ static irqreturn_t mbhc_btn_release_irq_handler(int irq, void *arg)
>   
>   		/* check if its BTN0 thats released */
>   		if ((val != -1) && !(val & CDC_A_MBHC_RESULT_1_BTN_RESULT_MASK))
> -			priv->mbhc_btn0_pressed = false;
> +			priv->mbhc_btn0_released = true;
>   
>   	} else {
>   		snd_soc_jack_report(priv->jack, 0, btn_mask);
> @@ -983,9 +983,7 @@ static irqreturn_t mbhc_btn_press_irq_handler(int irq, void *arg)
>   		break;
>   	case 0x0:
>   		/* handle BTN_0 specially for type detection */
> -		if (priv->detect_accessory_type)
> -			priv->mbhc_btn0_pressed = true;
> -		else
> +		if (!priv->detect_accessory_type)
>   			snd_soc_jack_report(priv->jack,
>   					    SND_JACK_BTN_0, btn_mask);
>   		break;
> @@ -1029,19 +1027,19 @@ static irqreturn_t pm8916_mbhc_switch_irq_handler(int irq, void *arg)
>   		 * both press and release event received then its
>   		 * a headset.
>   		 */
> -		if (priv->mbhc_btn0_pressed)
> +		if (priv->mbhc_btn0_released)
>   			snd_soc_jack_report(priv->jack,
> -					    SND_JACK_HEADPHONE, hs_jack_mask);
> +					    SND_JACK_HEADSET, hs_jack_mask);
>   		else
>   			snd_soc_jack_report(priv->jack,
> -					    SND_JACK_HEADSET, hs_jack_mask);
> +					    SND_JACK_HEADPHONE, hs_jack_mask);
>   
>   		priv->detect_accessory_type = false;
>   
>   	} else { /* removal */
>   		snd_soc_jack_report(priv->jack, 0, hs_jack_mask);
>   		priv->detect_accessory_type = true;
> -		priv->mbhc_btn0_pressed = false;
> +		priv->mbhc_btn0_released = false;
>   	}
>   
>   	return IRQ_HANDLED;
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-09-28 19:56 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-13 20:43 [RFC] ASoC: codecs: msm8916-wcd-analog: use btn0 released detection Damien Riegel
2017-09-18  9:08 ` Srinivas Kandagatla
2017-09-19 12:11   ` Mark Brown
2017-09-19 13:22     ` Damien Riegel
2017-09-28 18:18       ` Srinivas Kandagatla
2017-09-28 19:10         ` Damien Riegel
2017-09-28 18:58 ` Applied "ASoC: codecs: msm8916-wcd-analog: use btn0 released detection" to the asoc tree Mark Brown
2017-09-28 19:56 ` [RFC] ASoC: codecs: msm8916-wcd-analog: use btn0 released detection Srinivas Kandagatla

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®