alsa-devel.alsa-project.org archive mirror
 help / color / mirror / Atom feed
* [alsa-devel] [PATCH 0/4] ASoC: msm8916-wcd-analog: MIC BIAS fixes/additions
@ 2020-01-11 16:40 Stephan Gerhold
  2020-01-11 16:40 ` [alsa-devel] [PATCH 1/4] ASoC: msm8916-wcd-analog: Fix selected events for MIC BIAS External1 Stephan Gerhold
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Stephan Gerhold @ 2020-01-11 16:40 UTC (permalink / raw)
  To: Mark Brown
  Cc: alsa-devel, Stephan Gerhold, Takashi Iwai, Liam Girdwood,
	Srinivas Kandagatla, ~postmarketos/upstreaming

This patch series fixes some bugs in the MIC BIAS implementation
in msm8916-wcd-analog. Finally, it adds support for MIC BIAS Internal3,
which is needed for msm8916-longcheer-l8150.

Stephan Gerhold (4):
  ASoC: msm8916-wcd-analog: Fix selected events for MIC BIAS External1
  ASoC: msm8916-wcd-analog: Fix MIC BIAS Internal1
  ASoC: msm8916-wcd-analog: Simplify MIC BIAS Internal
  ASoC: msm8916-wcd-analog: Add MIC BIAS Internal3

 sound/soc/codecs/msm8916-wcd-analog.c | 59 ++++++++++-----------------
 1 file changed, 21 insertions(+), 38 deletions(-)

-- 
2.24.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] [PATCH 1/4] ASoC: msm8916-wcd-analog: Fix selected events for MIC BIAS External1
  2020-01-11 16:40 [alsa-devel] [PATCH 0/4] ASoC: msm8916-wcd-analog: MIC BIAS fixes/additions Stephan Gerhold
@ 2020-01-11 16:40 ` Stephan Gerhold
  2020-01-13 11:32   ` Srinivas Kandagatla
  2020-01-13 15:13   ` [alsa-devel] Applied "ASoC: msm8916-wcd-analog: Fix selected events for MIC BIAS External1" to the asoc tree Mark Brown
  2020-01-11 16:40 ` [alsa-devel] [PATCH 2/4] ASoC: msm8916-wcd-analog: Fix MIC BIAS Internal1 Stephan Gerhold
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 13+ messages in thread
From: Stephan Gerhold @ 2020-01-11 16:40 UTC (permalink / raw)
  To: Mark Brown
  Cc: alsa-devel, Stephan Gerhold, Takashi Iwai, Liam Girdwood,
	Srinivas Kandagatla, ~postmarketos/upstreaming

MIC BIAS External1 sets pm8916_wcd_analog_enable_micbias_ext1()
as event handler, which ends up in pm8916_wcd_analog_enable_micbias_ext().

But pm8916_wcd_analog_enable_micbias_ext() only handles the POST_PMU
event, which is not specified in the event flags for MIC BIAS External1.
This means that the code in the event handler is never actually run.

Set SND_SOC_DAPM_POST_PMU as the only event for the handler to fix this.

Fixes: 585e881e5b9e ("ASoC: codecs: Add msm8916-wcd analog codec")
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 sound/soc/codecs/msm8916-wcd-analog.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
index f53235be77d9..30b19f12fabc 100644
--- a/sound/soc/codecs/msm8916-wcd-analog.c
+++ b/sound/soc/codecs/msm8916-wcd-analog.c
@@ -938,10 +938,10 @@ static const struct snd_soc_dapm_widget pm8916_wcd_analog_dapm_widgets[] = {
 
 	SND_SOC_DAPM_SUPPLY("MIC BIAS External1", CDC_A_MICB_1_EN, 7, 0,
 			    pm8916_wcd_analog_enable_micbias_ext1,
-			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
+			    SND_SOC_DAPM_POST_PMU),
 	SND_SOC_DAPM_SUPPLY("MIC BIAS External2", CDC_A_MICB_2_EN, 7, 0,
 			    pm8916_wcd_analog_enable_micbias_ext2,
-			    SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),
+			    SND_SOC_DAPM_POST_PMU),
 
 	SND_SOC_DAPM_ADC_E("ADC1", NULL, CDC_A_TX_1_EN, 7, 0,
 			   pm8916_wcd_analog_enable_adc,
-- 
2.24.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] [PATCH 2/4] ASoC: msm8916-wcd-analog: Fix MIC BIAS Internal1
  2020-01-11 16:40 [alsa-devel] [PATCH 0/4] ASoC: msm8916-wcd-analog: MIC BIAS fixes/additions Stephan Gerhold
  2020-01-11 16:40 ` [alsa-devel] [PATCH 1/4] ASoC: msm8916-wcd-analog: Fix selected events for MIC BIAS External1 Stephan Gerhold
@ 2020-01-11 16:40 ` Stephan Gerhold
  2020-01-13 14:08   ` Srinivas Kandagatla
  2020-01-13 15:13   ` [alsa-devel] Applied "ASoC: msm8916-wcd-analog: Fix MIC BIAS Internal1" to the asoc tree Mark Brown
  2020-01-11 16:40 ` [alsa-devel] [PATCH 3/4] ASoC: msm8916-wcd-analog: Simplify MIC BIAS Internal Stephan Gerhold
  2020-01-11 16:40 ` [alsa-devel] [PATCH 4/4] ASoC: msm8916-wcd-analog: Add MIC BIAS Internal3 Stephan Gerhold
  3 siblings, 2 replies; 13+ messages in thread
From: Stephan Gerhold @ 2020-01-11 16:40 UTC (permalink / raw)
  To: Mark Brown
  Cc: alsa-devel, Stephan Gerhold, Takashi Iwai, Liam Girdwood,
	Srinivas Kandagatla, ~postmarketos/upstreaming

MIC BIAS Internal1 is broken at the moment because we always
enable the internal rbias resistor to the TX2 line (connected to
the headset microphone), rather than enabling the resistor connected
to TX1.

Move the RBIAS code to pm8916_wcd_analog_enable_micbias_int1/2()
to fix this.

Fixes: 585e881e5b9e ("ASoC: codecs: Add msm8916-wcd analog codec")
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 sound/soc/codecs/msm8916-wcd-analog.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
index 30b19f12fabc..1f7964beb20c 100644
--- a/sound/soc/codecs/msm8916-wcd-analog.c
+++ b/sound/soc/codecs/msm8916-wcd-analog.c
@@ -396,9 +396,6 @@ static int pm8916_wcd_analog_enable_micbias_int(struct snd_soc_component
 
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
-		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
-				    MICB_1_INT_TX2_INT_RBIAS_EN_MASK,
-				    MICB_1_INT_TX2_INT_RBIAS_EN_ENABLE);
 		snd_soc_component_update_bits(component, reg, MICB_1_EN_PULL_DOWN_EN_MASK, 0);
 		snd_soc_component_update_bits(component, CDC_A_MICB_1_EN,
 				    MICB_1_EN_OPA_STG2_TAIL_CURR_MASK,
@@ -448,6 +445,14 @@ static int pm8916_wcd_analog_enable_micbias_int1(struct
 	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
 	struct pm8916_wcd_analog_priv *wcd = snd_soc_component_get_drvdata(component);
 
+	switch (event) {
+	case SND_SOC_DAPM_PRE_PMU:
+		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
+				    MICB_1_INT_TX1_INT_RBIAS_EN_MASK,
+				    MICB_1_INT_TX1_INT_RBIAS_EN_ENABLE);
+		break;
+	}
+
 	return pm8916_wcd_analog_enable_micbias_int(component, event, w->reg,
 						     wcd->micbias1_cap_mode);
 }
@@ -558,6 +563,11 @@ static int pm8916_wcd_analog_enable_micbias_int2(struct
 	struct pm8916_wcd_analog_priv *wcd = snd_soc_component_get_drvdata(component);
 
 	switch (event) {
+	case SND_SOC_DAPM_PRE_PMU:
+		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
+				    MICB_1_INT_TX2_INT_RBIAS_EN_MASK,
+				    MICB_1_INT_TX2_INT_RBIAS_EN_ENABLE);
+		break;
 	case SND_SOC_DAPM_POST_PMU:
 		pm8916_mbhc_configure_bias(wcd, true);
 		break;
-- 
2.24.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] [PATCH 3/4] ASoC: msm8916-wcd-analog: Simplify MIC BIAS Internal
  2020-01-11 16:40 [alsa-devel] [PATCH 0/4] ASoC: msm8916-wcd-analog: MIC BIAS fixes/additions Stephan Gerhold
  2020-01-11 16:40 ` [alsa-devel] [PATCH 1/4] ASoC: msm8916-wcd-analog: Fix selected events for MIC BIAS External1 Stephan Gerhold
  2020-01-11 16:40 ` [alsa-devel] [PATCH 2/4] ASoC: msm8916-wcd-analog: Fix MIC BIAS Internal1 Stephan Gerhold
@ 2020-01-11 16:40 ` Stephan Gerhold
  2020-01-14 10:54   ` Srinivas Kandagatla
  2020-01-11 16:40 ` [alsa-devel] [PATCH 4/4] ASoC: msm8916-wcd-analog: Add MIC BIAS Internal3 Stephan Gerhold
  3 siblings, 1 reply; 13+ messages in thread
From: Stephan Gerhold @ 2020-01-11 16:40 UTC (permalink / raw)
  To: Mark Brown
  Cc: alsa-devel, Stephan Gerhold, Takashi Iwai, Liam Girdwood,
	Srinivas Kandagatla, ~postmarketos/upstreaming, Nikita Travkin

At the moment, MIC BIAS Internal* and MIC BIAS External* both reference
the same register, and have a part of their initialization sequence
duplicated.

In general, the sequence for enabling MIC BIAS InternalX seems to be:
  1. Enable MIC BIAS ExternalX
  2. Enable internal RBIAS resistor

This means that we can simplify the code by modelling MIC BIAS InternalX
as supply connected to MIC BIAS ExternalX. MIC BIAS InternalX is only
responsible for enabling the internal RBIAS resistor (2). The DAPM enable
sequence will automatically enable MIC BIAS ExternalX (1).

This makes it much easier to add support for MIC BIAS Internal3
as a next step.

Tested-by: Nikita Travkin <nikitos.tr@gmail.com> # longcheer-l8150
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 sound/soc/codecs/msm8916-wcd-analog.c | 57 ++++++---------------------
 1 file changed, 13 insertions(+), 44 deletions(-)

diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
index 1f7964beb20c..930baae6eff5 100644
--- a/sound/soc/codecs/msm8916-wcd-analog.c
+++ b/sound/soc/codecs/msm8916-wcd-analog.c
@@ -389,23 +389,17 @@ static int pm8916_wcd_analog_enable_micbias_ext(struct snd_soc_component
 	return 0;
 }
 
-static int pm8916_wcd_analog_enable_micbias_int(struct snd_soc_component
-						 *component, int event,
-						 int reg, u32 cap_mode)
+static int pm8916_wcd_analog_enable_micbias_int(struct snd_soc_dapm_widget *w,
+						struct snd_kcontrol *kcontrol,
+						int event)
 {
+	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
 
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
-		snd_soc_component_update_bits(component, reg, MICB_1_EN_PULL_DOWN_EN_MASK, 0);
 		snd_soc_component_update_bits(component, CDC_A_MICB_1_EN,
 				    MICB_1_EN_OPA_STG2_TAIL_CURR_MASK,
 				    MICB_1_EN_OPA_STG2_TAIL_CURR_1_60UA);
-
-		break;
-	case SND_SOC_DAPM_POST_PMU:
-		pm8916_wcd_analog_micbias_enable(component);
-		snd_soc_component_update_bits(component, CDC_A_MICB_1_EN,
-				    MICB_1_EN_BYP_CAP_MASK, cap_mode);
 		break;
 	}
 
@@ -437,26 +431,6 @@ static int pm8916_wcd_analog_enable_micbias_ext2(struct
 
 }
 
-static int pm8916_wcd_analog_enable_micbias_int1(struct
-						  snd_soc_dapm_widget
-						  *w, struct snd_kcontrol
-						  *kcontrol, int event)
-{
-	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
-	struct pm8916_wcd_analog_priv *wcd = snd_soc_component_get_drvdata(component);
-
-	switch (event) {
-	case SND_SOC_DAPM_PRE_PMU:
-		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
-				    MICB_1_INT_TX1_INT_RBIAS_EN_MASK,
-				    MICB_1_INT_TX1_INT_RBIAS_EN_ENABLE);
-		break;
-	}
-
-	return pm8916_wcd_analog_enable_micbias_int(component, event, w->reg,
-						     wcd->micbias1_cap_mode);
-}
-
 static int pm8916_mbhc_configure_bias(struct pm8916_wcd_analog_priv *priv,
 				      bool micbias2_enabled)
 {
@@ -564,9 +538,8 @@ static int pm8916_wcd_analog_enable_micbias_int2(struct
 
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
-		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
-				    MICB_1_INT_TX2_INT_RBIAS_EN_MASK,
-				    MICB_1_INT_TX2_INT_RBIAS_EN_ENABLE);
+		snd_soc_component_update_bits(component, CDC_A_MICB_2_EN,
+					      CDC_A_MICB_2_PULL_DOWN_EN_MASK, 0);
 		break;
 	case SND_SOC_DAPM_POST_PMU:
 		pm8916_mbhc_configure_bias(wcd, true);
@@ -576,8 +549,7 @@ static int pm8916_wcd_analog_enable_micbias_int2(struct
 		break;
 	}
 
-	return pm8916_wcd_analog_enable_micbias_int(component, event, w->reg,
-						     wcd->micbias2_cap_mode);
+	return pm8916_wcd_analog_enable_micbias_int(w, kcontrol, event);
 }
 
 static int pm8916_wcd_analog_enable_adc(struct snd_soc_dapm_widget *w,
@@ -878,12 +850,10 @@ static const struct snd_soc_dapm_route pm8916_wcd_analog_audio_map[] = {
 	{"SPK PA", NULL, "SPK DAC"},
 	{"SPK DAC", "Switch", "PDM_RX3"},
 
-	{"MIC BIAS Internal1", NULL, "INT_LDO_H"},
-	{"MIC BIAS Internal2", NULL, "INT_LDO_H"},
+	{"MIC BIAS Internal1", NULL, "MIC BIAS External1"},
+	{"MIC BIAS Internal2", NULL, "MIC BIAS External2"},
 	{"MIC BIAS External1", NULL, "INT_LDO_H"},
 	{"MIC BIAS External2", NULL, "INT_LDO_H"},
-	{"MIC BIAS Internal1", NULL, "vdd-micbias"},
-	{"MIC BIAS Internal2", NULL, "vdd-micbias"},
 	{"MIC BIAS External1", NULL, "vdd-micbias"},
 	{"MIC BIAS External2", NULL, "vdd-micbias"},
 };
@@ -937,11 +907,10 @@ static const struct snd_soc_dapm_widget pm8916_wcd_analog_dapm_widgets[] = {
 	SND_SOC_DAPM_SUPPLY("RX_BIAS", CDC_A_RX_COM_BIAS_DAC, 7, 0, NULL, 0),
 
 	/* TX */
-	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal1", CDC_A_MICB_1_EN, 7, 0,
-			    pm8916_wcd_analog_enable_micbias_int1,
-			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
-			    SND_SOC_DAPM_POST_PMD),
-	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal2", CDC_A_MICB_2_EN, 7, 0,
+	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal1", CDC_A_MICB_1_INT_RBIAS, 7, 0,
+			    pm8916_wcd_analog_enable_micbias_int,
+			    SND_SOC_DAPM_PRE_PMU),
+	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal2", CDC_A_MICB_1_INT_RBIAS, 4, 0,
 			    pm8916_wcd_analog_enable_micbias_int2,
 			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
 			    SND_SOC_DAPM_POST_PMD),
-- 
2.24.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] [PATCH 4/4] ASoC: msm8916-wcd-analog: Add MIC BIAS Internal3
  2020-01-11 16:40 [alsa-devel] [PATCH 0/4] ASoC: msm8916-wcd-analog: MIC BIAS fixes/additions Stephan Gerhold
                   ` (2 preceding siblings ...)
  2020-01-11 16:40 ` [alsa-devel] [PATCH 3/4] ASoC: msm8916-wcd-analog: Simplify MIC BIAS Internal Stephan Gerhold
@ 2020-01-11 16:40 ` Stephan Gerhold
  3 siblings, 0 replies; 13+ messages in thread
From: Stephan Gerhold @ 2020-01-11 16:40 UTC (permalink / raw)
  To: Mark Brown
  Cc: alsa-devel, Stephan Gerhold, Takashi Iwai, Liam Girdwood,
	Srinivas Kandagatla, ~postmarketos/upstreaming, Nikita Travkin

PM8916 has three TX inputs that each have an (optional) internal
RBIAS resistor. MIC BIAS Internal1/2 (for TX1/2) are already supported.
TX3 does not have its own MIC BIAS supply, instead it is also supplied
from MIC BIAS1.

Now that we have simplified the MIC BIAS Internal* implementation
we can easily add support for it:

Add a MIC BIAS Internal3 supply that enables the internal RBIAS
resistor on TX3, and make sure to enable MIC BIAS External1.

Tested-by: Nikita Travkin <nikitos.tr@gmail.com> # longcheer-l8150
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 sound/soc/codecs/msm8916-wcd-analog.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
index 930baae6eff5..5e452b2457b2 100644
--- a/sound/soc/codecs/msm8916-wcd-analog.c
+++ b/sound/soc/codecs/msm8916-wcd-analog.c
@@ -852,6 +852,7 @@ static const struct snd_soc_dapm_route pm8916_wcd_analog_audio_map[] = {
 
 	{"MIC BIAS Internal1", NULL, "MIC BIAS External1"},
 	{"MIC BIAS Internal2", NULL, "MIC BIAS External2"},
+	{"MIC BIAS Internal3", NULL, "MIC BIAS External1"},
 	{"MIC BIAS External1", NULL, "INT_LDO_H"},
 	{"MIC BIAS External2", NULL, "INT_LDO_H"},
 	{"MIC BIAS External1", NULL, "vdd-micbias"},
@@ -914,6 +915,9 @@ static const struct snd_soc_dapm_widget pm8916_wcd_analog_dapm_widgets[] = {
 			    pm8916_wcd_analog_enable_micbias_int2,
 			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
 			    SND_SOC_DAPM_POST_PMD),
+	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal3", CDC_A_MICB_1_INT_RBIAS, 1, 0,
+			    pm8916_wcd_analog_enable_micbias_int,
+			    SND_SOC_DAPM_PRE_PMU),
 
 	SND_SOC_DAPM_SUPPLY("MIC BIAS External1", CDC_A_MICB_1_EN, 7, 0,
 			    pm8916_wcd_analog_enable_micbias_ext1,
-- 
2.24.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH 1/4] ASoC: msm8916-wcd-analog: Fix selected events for MIC BIAS External1
  2020-01-11 16:40 ` [alsa-devel] [PATCH 1/4] ASoC: msm8916-wcd-analog: Fix selected events for MIC BIAS External1 Stephan Gerhold
@ 2020-01-13 11:32   ` Srinivas Kandagatla
  2020-01-13 15:13   ` [alsa-devel] Applied "ASoC: msm8916-wcd-analog: Fix selected events for MIC BIAS External1" to the asoc tree Mark Brown
  1 sibling, 0 replies; 13+ messages in thread
From: Srinivas Kandagatla @ 2020-01-13 11:32 UTC (permalink / raw)
  To: Stephan Gerhold, Mark Brown
  Cc: alsa-devel, Takashi Iwai, Liam Girdwood, ~postmarketos/upstreaming



On 11/01/2020 16:40, Stephan Gerhold wrote:
> MIC BIAS External1 sets pm8916_wcd_analog_enable_micbias_ext1()
> as event handler, which ends up in pm8916_wcd_analog_enable_micbias_ext().
> 
> But pm8916_wcd_analog_enable_micbias_ext() only handles the POST_PMU
> event, which is not specified in the event flags for MIC BIAS External1.
> This means that the code in the event handler is never actually run.
> 
> Set SND_SOC_DAPM_POST_PMU as the only event for the handler to fix this.
> 
> Fixes: 585e881e5b9e ("ASoC: codecs: Add msm8916-wcd analog codec")
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
Thanks for testing Ext mic bias and the fix.

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

>   sound/soc/codecs/msm8916-wcd-analog.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
> index f53235be77d9..30b19f12fabc 100644
> --- a/sound/soc/codecs/msm8916-wcd-analog.c
> +++ b/sound/soc/codecs/msm8916-wcd-analog.c
> @@ -938,10 +938,10 @@ static const struct snd_soc_dapm_widget pm8916_wcd_analog_dapm_widgets[] = {
>   
>   	SND_SOC_DAPM_SUPPLY("MIC BIAS External1", CDC_A_MICB_1_EN, 7, 0,
>   			    pm8916_wcd_analog_enable_micbias_ext1,
> -			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
> +			    SND_SOC_DAPM_POST_PMU),
>   	SND_SOC_DAPM_SUPPLY("MIC BIAS External2", CDC_A_MICB_2_EN, 7, 0,
>   			    pm8916_wcd_analog_enable_micbias_ext2,
> -			    SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),
> +			    SND_SOC_DAPM_POST_PMU),
>   
>   	SND_SOC_DAPM_ADC_E("ADC1", NULL, CDC_A_TX_1_EN, 7, 0,
>   			   pm8916_wcd_analog_enable_adc,
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH 2/4] ASoC: msm8916-wcd-analog: Fix MIC BIAS Internal1
  2020-01-11 16:40 ` [alsa-devel] [PATCH 2/4] ASoC: msm8916-wcd-analog: Fix MIC BIAS Internal1 Stephan Gerhold
@ 2020-01-13 14:08   ` Srinivas Kandagatla
  2020-01-13 15:13   ` [alsa-devel] Applied "ASoC: msm8916-wcd-analog: Fix MIC BIAS Internal1" to the asoc tree Mark Brown
  1 sibling, 0 replies; 13+ messages in thread
From: Srinivas Kandagatla @ 2020-01-13 14:08 UTC (permalink / raw)
  To: Stephan Gerhold, Mark Brown
  Cc: alsa-devel, Takashi Iwai, Liam Girdwood, ~postmarketos/upstreaming



On 11/01/2020 16:40, Stephan Gerhold wrote:
> MIC BIAS Internal1 is broken at the moment because we always
> enable the internal rbias resistor to the TX2 line (connected to
> the headset microphone), rather than enabling the resistor connected
> to TX1.
> 
> Move the RBIAS code to pm8916_wcd_analog_enable_micbias_int1/2()
> to fix this.
> 
> Fixes: 585e881e5b9e ("ASoC: codecs: Add msm8916-wcd analog codec")
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

LGTM,

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


> ---
>   sound/soc/codecs/msm8916-wcd-analog.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
> index 30b19f12fabc..1f7964beb20c 100644
> --- a/sound/soc/codecs/msm8916-wcd-analog.c
> +++ b/sound/soc/codecs/msm8916-wcd-analog.c
> @@ -396,9 +396,6 @@ static int pm8916_wcd_analog_enable_micbias_int(struct snd_soc_component
>   
>   	switch (event) {
>   	case SND_SOC_DAPM_PRE_PMU:
> -		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
> -				    MICB_1_INT_TX2_INT_RBIAS_EN_MASK,
> -				    MICB_1_INT_TX2_INT_RBIAS_EN_ENABLE);
>   		snd_soc_component_update_bits(component, reg, MICB_1_EN_PULL_DOWN_EN_MASK, 0);
>   		snd_soc_component_update_bits(component, CDC_A_MICB_1_EN,
>   				    MICB_1_EN_OPA_STG2_TAIL_CURR_MASK,
> @@ -448,6 +445,14 @@ static int pm8916_wcd_analog_enable_micbias_int1(struct
>   	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
>   	struct pm8916_wcd_analog_priv *wcd = snd_soc_component_get_drvdata(component);
>   
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
> +				    MICB_1_INT_TX1_INT_RBIAS_EN_MASK,
> +				    MICB_1_INT_TX1_INT_RBIAS_EN_ENABLE);
> +		break;
> +	}
> +
>   	return pm8916_wcd_analog_enable_micbias_int(component, event, w->reg,
>   						     wcd->micbias1_cap_mode);
>   }
> @@ -558,6 +563,11 @@ static int pm8916_wcd_analog_enable_micbias_int2(struct
>   	struct pm8916_wcd_analog_priv *wcd = snd_soc_component_get_drvdata(component);
>   
>   	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
> +				    MICB_1_INT_TX2_INT_RBIAS_EN_MASK,
> +				    MICB_1_INT_TX2_INT_RBIAS_EN_ENABLE);
> +		break;
>   	case SND_SOC_DAPM_POST_PMU:
>   		pm8916_mbhc_configure_bias(wcd, true);
>   		break;
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] Applied "ASoC: msm8916-wcd-analog: Fix MIC BIAS Internal1" to the asoc tree
  2020-01-11 16:40 ` [alsa-devel] [PATCH 2/4] ASoC: msm8916-wcd-analog: Fix MIC BIAS Internal1 Stephan Gerhold
  2020-01-13 14:08   ` Srinivas Kandagatla
@ 2020-01-13 15:13   ` Mark Brown
  1 sibling, 0 replies; 13+ messages in thread
From: Mark Brown @ 2020-01-13 15:13 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: alsa-devel, Takashi Iwai, Liam Girdwood, Mark Brown,
	Srinivas Kandagatla, ~postmarketos/upstreaming

The patch

   ASoC: msm8916-wcd-analog: Fix MIC BIAS Internal1

has been applied to the asoc tree at

   https://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 057efcf9faea4769cf1020677d93d040db9b23f3 Mon Sep 17 00:00:00 2001
From: Stephan Gerhold <stephan@gerhold.net>
Date: Sat, 11 Jan 2020 17:40:04 +0100
Subject: [PATCH] ASoC: msm8916-wcd-analog: Fix MIC BIAS Internal1

MIC BIAS Internal1 is broken at the moment because we always
enable the internal rbias resistor to the TX2 line (connected to
the headset microphone), rather than enabling the resistor connected
to TX1.

Move the RBIAS code to pm8916_wcd_analog_enable_micbias_int1/2()
to fix this.

Fixes: 585e881e5b9e ("ASoC: codecs: Add msm8916-wcd analog codec")
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
Link: https://lore.kernel.org/r/20200111164006.43074-3-stephan@gerhold.net
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/msm8916-wcd-analog.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
index 30b19f12fabc..1f7964beb20c 100644
--- a/sound/soc/codecs/msm8916-wcd-analog.c
+++ b/sound/soc/codecs/msm8916-wcd-analog.c
@@ -396,9 +396,6 @@ static int pm8916_wcd_analog_enable_micbias_int(struct snd_soc_component
 
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
-		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
-				    MICB_1_INT_TX2_INT_RBIAS_EN_MASK,
-				    MICB_1_INT_TX2_INT_RBIAS_EN_ENABLE);
 		snd_soc_component_update_bits(component, reg, MICB_1_EN_PULL_DOWN_EN_MASK, 0);
 		snd_soc_component_update_bits(component, CDC_A_MICB_1_EN,
 				    MICB_1_EN_OPA_STG2_TAIL_CURR_MASK,
@@ -448,6 +445,14 @@ static int pm8916_wcd_analog_enable_micbias_int1(struct
 	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
 	struct pm8916_wcd_analog_priv *wcd = snd_soc_component_get_drvdata(component);
 
+	switch (event) {
+	case SND_SOC_DAPM_PRE_PMU:
+		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
+				    MICB_1_INT_TX1_INT_RBIAS_EN_MASK,
+				    MICB_1_INT_TX1_INT_RBIAS_EN_ENABLE);
+		break;
+	}
+
 	return pm8916_wcd_analog_enable_micbias_int(component, event, w->reg,
 						     wcd->micbias1_cap_mode);
 }
@@ -558,6 +563,11 @@ static int pm8916_wcd_analog_enable_micbias_int2(struct
 	struct pm8916_wcd_analog_priv *wcd = snd_soc_component_get_drvdata(component);
 
 	switch (event) {
+	case SND_SOC_DAPM_PRE_PMU:
+		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
+				    MICB_1_INT_TX2_INT_RBIAS_EN_MASK,
+				    MICB_1_INT_TX2_INT_RBIAS_EN_ENABLE);
+		break;
 	case SND_SOC_DAPM_POST_PMU:
 		pm8916_mbhc_configure_bias(wcd, true);
 		break;
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] Applied "ASoC: msm8916-wcd-analog: Fix selected events for MIC BIAS External1" to the asoc tree
  2020-01-11 16:40 ` [alsa-devel] [PATCH 1/4] ASoC: msm8916-wcd-analog: Fix selected events for MIC BIAS External1 Stephan Gerhold
  2020-01-13 11:32   ` Srinivas Kandagatla
@ 2020-01-13 15:13   ` Mark Brown
  1 sibling, 0 replies; 13+ messages in thread
From: Mark Brown @ 2020-01-13 15:13 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: alsa-devel, Takashi Iwai, Liam Girdwood, Mark Brown,
	Srinivas Kandagatla, ~postmarketos/upstreaming

The patch

   ASoC: msm8916-wcd-analog: Fix selected events for MIC BIAS External1

has been applied to the asoc tree at

   https://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 e0beec88397b163c7c4ea6fcfb67e8e07a2671dc Mon Sep 17 00:00:00 2001
From: Stephan Gerhold <stephan@gerhold.net>
Date: Sat, 11 Jan 2020 17:40:03 +0100
Subject: [PATCH] ASoC: msm8916-wcd-analog: Fix selected events for MIC BIAS
 External1

MIC BIAS External1 sets pm8916_wcd_analog_enable_micbias_ext1()
as event handler, which ends up in pm8916_wcd_analog_enable_micbias_ext().

But pm8916_wcd_analog_enable_micbias_ext() only handles the POST_PMU
event, which is not specified in the event flags for MIC BIAS External1.
This means that the code in the event handler is never actually run.

Set SND_SOC_DAPM_POST_PMU as the only event for the handler to fix this.

Fixes: 585e881e5b9e ("ASoC: codecs: Add msm8916-wcd analog codec")
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
Link: https://lore.kernel.org/r/20200111164006.43074-2-stephan@gerhold.net
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/msm8916-wcd-analog.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
index f53235be77d9..30b19f12fabc 100644
--- a/sound/soc/codecs/msm8916-wcd-analog.c
+++ b/sound/soc/codecs/msm8916-wcd-analog.c
@@ -938,10 +938,10 @@ static const struct snd_soc_dapm_widget pm8916_wcd_analog_dapm_widgets[] = {
 
 	SND_SOC_DAPM_SUPPLY("MIC BIAS External1", CDC_A_MICB_1_EN, 7, 0,
 			    pm8916_wcd_analog_enable_micbias_ext1,
-			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
+			    SND_SOC_DAPM_POST_PMU),
 	SND_SOC_DAPM_SUPPLY("MIC BIAS External2", CDC_A_MICB_2_EN, 7, 0,
 			    pm8916_wcd_analog_enable_micbias_ext2,
-			    SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),
+			    SND_SOC_DAPM_POST_PMU),
 
 	SND_SOC_DAPM_ADC_E("ADC1", NULL, CDC_A_TX_1_EN, 7, 0,
 			   pm8916_wcd_analog_enable_adc,
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH 3/4] ASoC: msm8916-wcd-analog: Simplify MIC BIAS Internal
  2020-01-11 16:40 ` [alsa-devel] [PATCH 3/4] ASoC: msm8916-wcd-analog: Simplify MIC BIAS Internal Stephan Gerhold
@ 2020-01-14 10:54   ` Srinivas Kandagatla
  2020-01-14 12:08     ` Stephan Gerhold
  0 siblings, 1 reply; 13+ messages in thread
From: Srinivas Kandagatla @ 2020-01-14 10:54 UTC (permalink / raw)
  To: Stephan Gerhold, Mark Brown
  Cc: alsa-devel, Takashi Iwai, Liam Girdwood,
	~postmarketos/upstreaming, Nikita Travkin



On 11/01/2020 16:40, Stephan Gerhold wrote:
> At the moment, MIC BIAS Internal* and MIC BIAS External* both reference
> the same register, and have a part of their initialization sequence
> duplicated.
> 
> In general, the sequence for enabling MIC BIAS InternalX seems to be:
>    1. Enable MIC BIAS ExternalX
>    2. Enable internal RBIAS resistor

Does not sound correct to me.

What external means here is if the MICBIAS has external pull up resistor 
or not. And this is very much board specific. In order for driver to 
enable/disable internal pull up resistor in such cases we use two dapm 
paths to differentiate it.


--srini




> 
> This means that we can simplify the code by modelling MIC BIAS InternalX
> as supply connected to MIC BIAS ExternalX. MIC BIAS InternalX is only
> responsible for enabling the internal RBIAS resistor (2). The DAPM enable
> sequence will automatically enable MIC BIAS ExternalX (1).
> 
> This makes it much easier to add support for MIC BIAS Internal3
> as a next step.
> 
> Tested-by: Nikita Travkin <nikitos.tr@gmail.com> # longcheer-l8150
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>   sound/soc/codecs/msm8916-wcd-analog.c | 57 ++++++---------------------
>   1 file changed, 13 insertions(+), 44 deletions(-)
> 
> diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
> index 1f7964beb20c..930baae6eff5 100644
> --- a/sound/soc/codecs/msm8916-wcd-analog.c
> +++ b/sound/soc/codecs/msm8916-wcd-analog.c
> @@ -389,23 +389,17 @@ static int pm8916_wcd_analog_enable_micbias_ext(struct snd_soc_component
>   	return 0;
>   }
>   
> -static int pm8916_wcd_analog_enable_micbias_int(struct snd_soc_component
> -						 *component, int event,
> -						 int reg, u32 cap_mode)
> +static int pm8916_wcd_analog_enable_micbias_int(struct snd_soc_dapm_widget *w,
> +						struct snd_kcontrol *kcontrol,
> +						int event)
>   {
> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
>   
>   	switch (event) {
>   	case SND_SOC_DAPM_PRE_PMU:
> -		snd_soc_component_update_bits(component, reg, MICB_1_EN_PULL_DOWN_EN_MASK, 0);
>   		snd_soc_component_update_bits(component, CDC_A_MICB_1_EN,
>   				    MICB_1_EN_OPA_STG2_TAIL_CURR_MASK,
>   				    MICB_1_EN_OPA_STG2_TAIL_CURR_1_60UA);
> -
> -		break;
> -	case SND_SOC_DAPM_POST_PMU:
> -		pm8916_wcd_analog_micbias_enable(component);
> -		snd_soc_component_update_bits(component, CDC_A_MICB_1_EN,
> -				    MICB_1_EN_BYP_CAP_MASK, cap_mode);
>   		break;
>   	}
>   
> @@ -437,26 +431,6 @@ static int pm8916_wcd_analog_enable_micbias_ext2(struct
>   
>   }
>   
> -static int pm8916_wcd_analog_enable_micbias_int1(struct
> -						  snd_soc_dapm_widget
> -						  *w, struct snd_kcontrol
> -						  *kcontrol, int event)
> -{
> -	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> -	struct pm8916_wcd_analog_priv *wcd = snd_soc_component_get_drvdata(component);
> -
> -	switch (event) {
> -	case SND_SOC_DAPM_PRE_PMU:
> -		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
> -				    MICB_1_INT_TX1_INT_RBIAS_EN_MASK,
> -				    MICB_1_INT_TX1_INT_RBIAS_EN_ENABLE);
> -		break;
> -	}
> -
> -	return pm8916_wcd_analog_enable_micbias_int(component, event, w->reg,
> -						     wcd->micbias1_cap_mode);
> -}
> -
>   static int pm8916_mbhc_configure_bias(struct pm8916_wcd_analog_priv *priv,
>   				      bool micbias2_enabled)
>   {
> @@ -564,9 +538,8 @@ static int pm8916_wcd_analog_enable_micbias_int2(struct
>   
>   	switch (event) {
>   	case SND_SOC_DAPM_PRE_PMU:
> -		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
> -				    MICB_1_INT_TX2_INT_RBIAS_EN_MASK,
> -				    MICB_1_INT_TX2_INT_RBIAS_EN_ENABLE);
> +		snd_soc_component_update_bits(component, CDC_A_MICB_2_EN,
> +					      CDC_A_MICB_2_PULL_DOWN_EN_MASK, 0);
>   		break;
>   	case SND_SOC_DAPM_POST_PMU:
>   		pm8916_mbhc_configure_bias(wcd, true);
> @@ -576,8 +549,7 @@ static int pm8916_wcd_analog_enable_micbias_int2(struct
>   		break;
>   	}
>   
> -	return pm8916_wcd_analog_enable_micbias_int(component, event, w->reg,
> -						     wcd->micbias2_cap_mode);
> +	return pm8916_wcd_analog_enable_micbias_int(w, kcontrol, event);
>   }
>   
>   static int pm8916_wcd_analog_enable_adc(struct snd_soc_dapm_widget *w,
> @@ -878,12 +850,10 @@ static const struct snd_soc_dapm_route pm8916_wcd_analog_audio_map[] = {
>   	{"SPK PA", NULL, "SPK DAC"},
>   	{"SPK DAC", "Switch", "PDM_RX3"},
>   
> -	{"MIC BIAS Internal1", NULL, "INT_LDO_H"},
> -	{"MIC BIAS Internal2", NULL, "INT_LDO_H"},
> +	{"MIC BIAS Internal1", NULL, "MIC BIAS External1"},
> +	{"MIC BIAS Internal2", NULL, "MIC BIAS External2"},
>   	{"MIC BIAS External1", NULL, "INT_LDO_H"},
>   	{"MIC BIAS External2", NULL, "INT_LDO_H"},
> -	{"MIC BIAS Internal1", NULL, "vdd-micbias"},
> -	{"MIC BIAS Internal2", NULL, "vdd-micbias"},
>   	{"MIC BIAS External1", NULL, "vdd-micbias"},
>   	{"MIC BIAS External2", NULL, "vdd-micbias"},
>   };
> @@ -937,11 +907,10 @@ static const struct snd_soc_dapm_widget pm8916_wcd_analog_dapm_widgets[] = {
>   	SND_SOC_DAPM_SUPPLY("RX_BIAS", CDC_A_RX_COM_BIAS_DAC, 7, 0, NULL, 0),
>   
>   	/* TX */
> -	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal1", CDC_A_MICB_1_EN, 7, 0,
> -			    pm8916_wcd_analog_enable_micbias_int1,
> -			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
> -			    SND_SOC_DAPM_POST_PMD),
> -	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal2", CDC_A_MICB_2_EN, 7, 0,
> +	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal1", CDC_A_MICB_1_INT_RBIAS, 7, 0,
> +			    pm8916_wcd_analog_enable_micbias_int,
> +			    SND_SOC_DAPM_PRE_PMU),
> +	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal2", CDC_A_MICB_1_INT_RBIAS, 4, 0,
>   			    pm8916_wcd_analog_enable_micbias_int2,
>   			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
>   			    SND_SOC_DAPM_POST_PMD),
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH 3/4] ASoC: msm8916-wcd-analog: Simplify MIC BIAS Internal
  2020-01-14 10:54   ` Srinivas Kandagatla
@ 2020-01-14 12:08     ` Stephan Gerhold
  2020-01-14 13:03       ` Srinivas Kandagatla
  0 siblings, 1 reply; 13+ messages in thread
From: Stephan Gerhold @ 2020-01-14 12:08 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: alsa-devel, Takashi Iwai, Liam Girdwood, Mark Brown,
	~postmarketos/upstreaming, Nikita Travkin

On Tue, Jan 14, 2020 at 10:54:44AM +0000, Srinivas Kandagatla wrote:
> 
> 
> On 11/01/2020 16:40, Stephan Gerhold wrote:
> > At the moment, MIC BIAS Internal* and MIC BIAS External* both reference
> > the same register, and have a part of their initialization sequence
> > duplicated.
> > 
> > In general, the sequence for enabling MIC BIAS InternalX seems to be:
> >    1. Enable MIC BIAS ExternalX
> >    2. Enable internal RBIAS resistor
> 
> Does not sound correct to me.
> 
> What external means here is if the MICBIAS has external pull up resistor or
> not. And this is very much board specific. In order for driver to
> enable/disable internal pull up resistor in such cases we use two dapm paths
> to differentiate it.
> 

You are right. Maybe the naming is a bit confusing here.
Let me try to clarify it:

If I understand it correctly, setting the MICB_EN bit in CDC_A_MICB_1_EN
enables the MIC_BIAS1 supply. This supply can be either used with an
external pull up resistor ("MIC BIAS External1") or with the internal
pull up resistor ("MIC BIAS Internal1"). Which one of them, is board-specific.

To use the internal pull up resistor, we need to set the TX1_INT_RBIAS_EN
bit in CDC_A_MICB_1_INT_RBIAS additionally.

In other words, the sequence for enabling MIC BIAS Internal1 is:
  I1. Enable MIC_BIAS1 supply (MICB_EN bit in CDC_A_MICB_1_EN)
  I2. Enable internal RBIAS (TX1_INT_RBIAS_EN bit in CDC_A_MICB_1_INT_RBIAS)

The sequence for enabling MIC BIAS External1 is:
  E1. Enable MIC_BIAS1 supply (MICB_EN bit in CDC_A_MICB_1_EN)

Right now we have:
  SND_SOC_DAPM_SUPPLY("MIC BIAS Internal1", CDC_A_MICB_1_EN, 7, 0, ...) // I1
  SND_SOC_DAPM_SUPPLY("MIC BIAS External1", CDC_A_MICB_1_EN, 7, 0, ...) // E1

I2 is done in the PM event handler (pm8916_wcd_analog_enable_micbias_int1).

The idea of this patch is to simplify this, and use one DAPM supply
that handles the common (I1/E1), and one DAPM supply that handles (I2).

Translated to code we get:
  SND_SOC_DAPM_SUPPLY("MIC BIAS1", CDC_A_MICB_1_EN, 7, 0, ...) // I1/E1
  SND_SOC_DAPM_SUPPLY("MIC BIAS Internal1", CDC_A_MICB_1_INT_RBIAS, 7, 0, ...) // I2
  SND_SOC_DAPM_SUPPLY("MIC BIAS External1", SND_SOC_NOPM, ...) // dummy

And the routes:
  {"MIC BIAS Internal1", NULL, "MIC BIAS1"} // If I2, also do I1
  {"MIC BIAS External1", NULL, "MIC BIAS1"} // Only do E1

As you can see, for "MIC BIAS External1" we just do "MIC BIAS1".
The confusing element of this patch might be that I simplified it a bit
further and combined "MIC BIAS1" with "MIC BIAS External1".
This works because we don't do anything extra for "MIC BIAS External1".

Does that make sense?

Thanks for taking the time to review all my patches.
Stephan

> 
> --srini
> 
> 
> 
> 
> > 
> > This means that we can simplify the code by modelling MIC BIAS InternalX
> > as supply connected to MIC BIAS ExternalX. MIC BIAS InternalX is only
> > responsible for enabling the internal RBIAS resistor (2). The DAPM enable
> > sequence will automatically enable MIC BIAS ExternalX (1).
> > 
> > This makes it much easier to add support for MIC BIAS Internal3
> > as a next step.
> > 
> > Tested-by: Nikita Travkin <nikitos.tr@gmail.com> # longcheer-l8150
> > Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> >   sound/soc/codecs/msm8916-wcd-analog.c | 57 ++++++---------------------
> >   1 file changed, 13 insertions(+), 44 deletions(-)
> > 
> > diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
> > index 1f7964beb20c..930baae6eff5 100644
> > --- a/sound/soc/codecs/msm8916-wcd-analog.c
> > +++ b/sound/soc/codecs/msm8916-wcd-analog.c
> > @@ -389,23 +389,17 @@ static int pm8916_wcd_analog_enable_micbias_ext(struct snd_soc_component
> >   	return 0;
> >   }
> > -static int pm8916_wcd_analog_enable_micbias_int(struct snd_soc_component
> > -						 *component, int event,
> > -						 int reg, u32 cap_mode)
> > +static int pm8916_wcd_analog_enable_micbias_int(struct snd_soc_dapm_widget *w,
> > +						struct snd_kcontrol *kcontrol,
> > +						int event)
> >   {
> > +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> >   	switch (event) {
> >   	case SND_SOC_DAPM_PRE_PMU:
> > -		snd_soc_component_update_bits(component, reg, MICB_1_EN_PULL_DOWN_EN_MASK, 0);
> >   		snd_soc_component_update_bits(component, CDC_A_MICB_1_EN,
> >   				    MICB_1_EN_OPA_STG2_TAIL_CURR_MASK,
> >   				    MICB_1_EN_OPA_STG2_TAIL_CURR_1_60UA);
> > -
> > -		break;
> > -	case SND_SOC_DAPM_POST_PMU:
> > -		pm8916_wcd_analog_micbias_enable(component);
> > -		snd_soc_component_update_bits(component, CDC_A_MICB_1_EN,
> > -				    MICB_1_EN_BYP_CAP_MASK, cap_mode);
> >   		break;
> >   	}
> > @@ -437,26 +431,6 @@ static int pm8916_wcd_analog_enable_micbias_ext2(struct
> >   }
> > -static int pm8916_wcd_analog_enable_micbias_int1(struct
> > -						  snd_soc_dapm_widget
> > -						  *w, struct snd_kcontrol
> > -						  *kcontrol, int event)
> > -{
> > -	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> > -	struct pm8916_wcd_analog_priv *wcd = snd_soc_component_get_drvdata(component);
> > -
> > -	switch (event) {
> > -	case SND_SOC_DAPM_PRE_PMU:
> > -		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
> > -				    MICB_1_INT_TX1_INT_RBIAS_EN_MASK,
> > -				    MICB_1_INT_TX1_INT_RBIAS_EN_ENABLE);
> > -		break;
> > -	}
> > -
> > -	return pm8916_wcd_analog_enable_micbias_int(component, event, w->reg,
> > -						     wcd->micbias1_cap_mode);
> > -}
> > -
> >   static int pm8916_mbhc_configure_bias(struct pm8916_wcd_analog_priv *priv,
> >   				      bool micbias2_enabled)
> >   {
> > @@ -564,9 +538,8 @@ static int pm8916_wcd_analog_enable_micbias_int2(struct
> >   	switch (event) {
> >   	case SND_SOC_DAPM_PRE_PMU:
> > -		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
> > -				    MICB_1_INT_TX2_INT_RBIAS_EN_MASK,
> > -				    MICB_1_INT_TX2_INT_RBIAS_EN_ENABLE);
> > +		snd_soc_component_update_bits(component, CDC_A_MICB_2_EN,
> > +					      CDC_A_MICB_2_PULL_DOWN_EN_MASK, 0);
> >   		break;
> >   	case SND_SOC_DAPM_POST_PMU:
> >   		pm8916_mbhc_configure_bias(wcd, true);
> > @@ -576,8 +549,7 @@ static int pm8916_wcd_analog_enable_micbias_int2(struct
> >   		break;
> >   	}
> > -	return pm8916_wcd_analog_enable_micbias_int(component, event, w->reg,
> > -						     wcd->micbias2_cap_mode);
> > +	return pm8916_wcd_analog_enable_micbias_int(w, kcontrol, event);
> >   }
> >   static int pm8916_wcd_analog_enable_adc(struct snd_soc_dapm_widget *w,
> > @@ -878,12 +850,10 @@ static const struct snd_soc_dapm_route pm8916_wcd_analog_audio_map[] = {
> >   	{"SPK PA", NULL, "SPK DAC"},
> >   	{"SPK DAC", "Switch", "PDM_RX3"},
> > -	{"MIC BIAS Internal1", NULL, "INT_LDO_H"},
> > -	{"MIC BIAS Internal2", NULL, "INT_LDO_H"},
> > +	{"MIC BIAS Internal1", NULL, "MIC BIAS External1"},
> > +	{"MIC BIAS Internal2", NULL, "MIC BIAS External2"},
> >   	{"MIC BIAS External1", NULL, "INT_LDO_H"},
> >   	{"MIC BIAS External2", NULL, "INT_LDO_H"},
> > -	{"MIC BIAS Internal1", NULL, "vdd-micbias"},
> > -	{"MIC BIAS Internal2", NULL, "vdd-micbias"},
> >   	{"MIC BIAS External1", NULL, "vdd-micbias"},
> >   	{"MIC BIAS External2", NULL, "vdd-micbias"},
> >   };
> > @@ -937,11 +907,10 @@ static const struct snd_soc_dapm_widget pm8916_wcd_analog_dapm_widgets[] = {
> >   	SND_SOC_DAPM_SUPPLY("RX_BIAS", CDC_A_RX_COM_BIAS_DAC, 7, 0, NULL, 0),
> >   	/* TX */
> > -	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal1", CDC_A_MICB_1_EN, 7, 0,
> > -			    pm8916_wcd_analog_enable_micbias_int1,
> > -			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
> > -			    SND_SOC_DAPM_POST_PMD),
> > -	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal2", CDC_A_MICB_2_EN, 7, 0,
> > +	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal1", CDC_A_MICB_1_INT_RBIAS, 7, 0,
> > +			    pm8916_wcd_analog_enable_micbias_int,
> > +			    SND_SOC_DAPM_PRE_PMU),
> > +	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal2", CDC_A_MICB_1_INT_RBIAS, 4, 0,
> >   			    pm8916_wcd_analog_enable_micbias_int2,
> >   			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
> >   			    SND_SOC_DAPM_POST_PMD),
> > 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH 3/4] ASoC: msm8916-wcd-analog: Simplify MIC BIAS Internal
  2020-01-14 12:08     ` Stephan Gerhold
@ 2020-01-14 13:03       ` Srinivas Kandagatla
  2020-01-14 16:54         ` Stephan Gerhold
  0 siblings, 1 reply; 13+ messages in thread
From: Srinivas Kandagatla @ 2020-01-14 13:03 UTC (permalink / raw)
  To: Stephan Gerhold
  Cc: alsa-devel, Takashi Iwai, Liam Girdwood, Mark Brown,
	~postmarketos/upstreaming, Nikita Travkin



On 14/01/2020 12:08, Stephan Gerhold wrote:
> On Tue, Jan 14, 2020 at 10:54:44AM +0000, Srinivas Kandagatla wrote:
>>
>>
>> On 11/01/2020 16:40, Stephan Gerhold wrote:
>>> At the moment, MIC BIAS Internal* and MIC BIAS External* both reference
>>> the same register, and have a part of their initialization sequence
>>> duplicated.
>>>
>>> In general, the sequence for enabling MIC BIAS InternalX seems to be:
>>>     1. Enable MIC BIAS ExternalX
>>>     2. Enable internal RBIAS resistor
>>
>> Does not sound correct to me.
>>
>> What external means here is if the MICBIAS has external pull up resistor or
>> not. And this is very much board specific. In order for driver to
>> enable/disable internal pull up resistor in such cases we use two dapm paths
>> to differentiate it.
>>
> 
> You are right. Maybe the naming is a bit confusing here.
> Let me try to clarify it:
> 
> If I understand it correctly, setting the MICB_EN bit in CDC_A_MICB_1_EN
> enables the MIC_BIAS1 supply. This supply can be either used with an
> external pull up resistor ("MIC BIAS External1") or with the internal
> pull up resistor ("MIC BIAS Internal1"). Which one of them, is board-specific.
> 
> To use the internal pull up resistor, we need to set the TX1_INT_RBIAS_EN
> bit in CDC_A_MICB_1_INT_RBIAS additionally.
> 
> In other words, the sequence for enabling MIC BIAS Internal1 is:
>    I1. Enable MIC_BIAS1 supply (MICB_EN bit in CDC_A_MICB_1_EN)
>    I2. Enable internal RBIAS (TX1_INT_RBIAS_EN bit in CDC_A_MICB_1_INT_RBIAS)
> 
> The sequence for enabling MIC BIAS External1 is:
>    E1. Enable MIC_BIAS1 supply (MICB_EN bit in CDC_A_MICB_1_EN)

In theory, We should also disable the internal pull up here. if not we 
end up in two parallel resistors, effectively 1/2 the value of external 
resistor.



> 
> Right now we have:
>    SND_SOC_DAPM_SUPPLY("MIC BIAS Internal1", CDC_A_MICB_1_EN, 7, 0, ...) // I1
>    SND_SOC_DAPM_SUPPLY("MIC BIAS External1", CDC_A_MICB_1_EN, 7, 0, ...) // E1
> 
> I2 is done in the PM event handler (pm8916_wcd_analog_enable_micbias_int1).
> 
> The idea of this patch is to simplify this, and use one DAPM supply
> that handles the common (I1/E1), and one DAPM supply that handles (I2).
> 
> Translated to code we get:
>    SND_SOC_DAPM_SUPPLY("MIC BIAS1", CDC_A_MICB_1_EN, 7, 0, ...) // I1/E1
>    SND_SOC_DAPM_SUPPLY("MIC BIAS Internal1", CDC_A_MICB_1_INT_RBIAS, 7, 0, ...) // I2
>    SND_SOC_DAPM_SUPPLY("MIC BIAS External1", SND_SOC_NOPM, ...) // dummy
> 
> And the routes:
>    {"MIC BIAS Internal1", NULL, "MIC BIAS1"} // If I2, also do I1
>    {"MIC BIAS External1", NULL, "MIC BIAS1"} // Only do E1

This should work! and makes it much clear too.


> 
> As you can see, for "MIC BIAS External1" we just do "MIC BIAS1".
> The confusing element of this patch might be that I simplified it a bit
> further and combined "MIC BIAS1" with "MIC BIAS External1".
> This works because we don't do anything extra for "MIC BIAS External1".
Should we not make sure that internal resistor switch is disabled here?

--srini

> 
> Does that make sense?
> 
> Thanks for taking the time to review all my patches.
> Stephan
> 
>>
>> --srini
>>
>>
>>
>>
>>>
>>> This means that we can simplify the code by modelling MIC BIAS InternalX
>>> as supply connected to MIC BIAS ExternalX. MIC BIAS InternalX is only
>>> responsible for enabling the internal RBIAS resistor (2). The DAPM enable
>>> sequence will automatically enable MIC BIAS ExternalX (1).
>>>
>>> This makes it much easier to add support for MIC BIAS Internal3
>>> as a next step.
>>>
>>> Tested-by: Nikita Travkin <nikitos.tr@gmail.com> # longcheer-l8150
>>> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
>>> ---
>>>    sound/soc/codecs/msm8916-wcd-analog.c | 57 ++++++---------------------
>>>    1 file changed, 13 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
>>> index 1f7964beb20c..930baae6eff5 100644
>>> --- a/sound/soc/codecs/msm8916-wcd-analog.c
>>> +++ b/sound/soc/codecs/msm8916-wcd-analog.c
>>> @@ -389,23 +389,17 @@ static int pm8916_wcd_analog_enable_micbias_ext(struct snd_soc_component
>>>    	return 0;
>>>    }
>>> -static int pm8916_wcd_analog_enable_micbias_int(struct snd_soc_component
>>> -						 *component, int event,
>>> -						 int reg, u32 cap_mode)
>>> +static int pm8916_wcd_analog_enable_micbias_int(struct snd_soc_dapm_widget *w,
>>> +						struct snd_kcontrol *kcontrol,
>>> +						int event)
>>>    {
>>> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
>>>    	switch (event) {
>>>    	case SND_SOC_DAPM_PRE_PMU:
>>> -		snd_soc_component_update_bits(component, reg, MICB_1_EN_PULL_DOWN_EN_MASK, 0);
>>>    		snd_soc_component_update_bits(component, CDC_A_MICB_1_EN,
>>>    				    MICB_1_EN_OPA_STG2_TAIL_CURR_MASK,
>>>    				    MICB_1_EN_OPA_STG2_TAIL_CURR_1_60UA);
>>> -
>>> -		break;
>>> -	case SND_SOC_DAPM_POST_PMU:
>>> -		pm8916_wcd_analog_micbias_enable(component);
>>> -		snd_soc_component_update_bits(component, CDC_A_MICB_1_EN,
>>> -				    MICB_1_EN_BYP_CAP_MASK, cap_mode);
>>>    		break;
>>>    	}
>>> @@ -437,26 +431,6 @@ static int pm8916_wcd_analog_enable_micbias_ext2(struct
>>>    }
>>> -static int pm8916_wcd_analog_enable_micbias_int1(struct
>>> -						  snd_soc_dapm_widget
>>> -						  *w, struct snd_kcontrol
>>> -						  *kcontrol, int event)
>>> -{
>>> -	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
>>> -	struct pm8916_wcd_analog_priv *wcd = snd_soc_component_get_drvdata(component);
>>> -
>>> -	switch (event) {
>>> -	case SND_SOC_DAPM_PRE_PMU:
>>> -		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
>>> -				    MICB_1_INT_TX1_INT_RBIAS_EN_MASK,
>>> -				    MICB_1_INT_TX1_INT_RBIAS_EN_ENABLE);
>>> -		break;
>>> -	}
>>> -
>>> -	return pm8916_wcd_analog_enable_micbias_int(component, event, w->reg,
>>> -						     wcd->micbias1_cap_mode);
>>> -}
>>> -
>>>    static int pm8916_mbhc_configure_bias(struct pm8916_wcd_analog_priv *priv,
>>>    				      bool micbias2_enabled)
>>>    {
>>> @@ -564,9 +538,8 @@ static int pm8916_wcd_analog_enable_micbias_int2(struct
>>>    	switch (event) {
>>>    	case SND_SOC_DAPM_PRE_PMU:
>>> -		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
>>> -				    MICB_1_INT_TX2_INT_RBIAS_EN_MASK,
>>> -				    MICB_1_INT_TX2_INT_RBIAS_EN_ENABLE);
>>> +		snd_soc_component_update_bits(component, CDC_A_MICB_2_EN,
>>> +					      CDC_A_MICB_2_PULL_DOWN_EN_MASK, 0);
>>>    		break;
>>>    	case SND_SOC_DAPM_POST_PMU:
>>>    		pm8916_mbhc_configure_bias(wcd, true);
>>> @@ -576,8 +549,7 @@ static int pm8916_wcd_analog_enable_micbias_int2(struct
>>>    		break;
>>>    	}
>>> -	return pm8916_wcd_analog_enable_micbias_int(component, event, w->reg,
>>> -						     wcd->micbias2_cap_mode);
>>> +	return pm8916_wcd_analog_enable_micbias_int(w, kcontrol, event);
>>>    }
>>>    static int pm8916_wcd_analog_enable_adc(struct snd_soc_dapm_widget *w,
>>> @@ -878,12 +850,10 @@ static const struct snd_soc_dapm_route pm8916_wcd_analog_audio_map[] = {
>>>    	{"SPK PA", NULL, "SPK DAC"},
>>>    	{"SPK DAC", "Switch", "PDM_RX3"},
>>> -	{"MIC BIAS Internal1", NULL, "INT_LDO_H"},
>>> -	{"MIC BIAS Internal2", NULL, "INT_LDO_H"},
>>> +	{"MIC BIAS Internal1", NULL, "MIC BIAS External1"},
>>> +	{"MIC BIAS Internal2", NULL, "MIC BIAS External2"},
>>>    	{"MIC BIAS External1", NULL, "INT_LDO_H"},
>>>    	{"MIC BIAS External2", NULL, "INT_LDO_H"},
>>> -	{"MIC BIAS Internal1", NULL, "vdd-micbias"},
>>> -	{"MIC BIAS Internal2", NULL, "vdd-micbias"},
>>>    	{"MIC BIAS External1", NULL, "vdd-micbias"},
>>>    	{"MIC BIAS External2", NULL, "vdd-micbias"},
>>>    };
>>> @@ -937,11 +907,10 @@ static const struct snd_soc_dapm_widget pm8916_wcd_analog_dapm_widgets[] = {
>>>    	SND_SOC_DAPM_SUPPLY("RX_BIAS", CDC_A_RX_COM_BIAS_DAC, 7, 0, NULL, 0),
>>>    	/* TX */
>>> -	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal1", CDC_A_MICB_1_EN, 7, 0,
>>> -			    pm8916_wcd_analog_enable_micbias_int1,
>>> -			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
>>> -			    SND_SOC_DAPM_POST_PMD),
>>> -	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal2", CDC_A_MICB_2_EN, 7, 0,
>>> +	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal1", CDC_A_MICB_1_INT_RBIAS, 7, 0,
>>> +			    pm8916_wcd_analog_enable_micbias_int,
>>> +			    SND_SOC_DAPM_PRE_PMU),
>>> +	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal2", CDC_A_MICB_1_INT_RBIAS, 4, 0,
>>>    			    pm8916_wcd_analog_enable_micbias_int2,
>>>    			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
>>>    			    SND_SOC_DAPM_POST_PMD),
>>>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH 3/4] ASoC: msm8916-wcd-analog: Simplify MIC BIAS Internal
  2020-01-14 13:03       ` Srinivas Kandagatla
@ 2020-01-14 16:54         ` Stephan Gerhold
  0 siblings, 0 replies; 13+ messages in thread
From: Stephan Gerhold @ 2020-01-14 16:54 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: alsa-devel, Takashi Iwai, Liam Girdwood, Mark Brown,
	~postmarketos/upstreaming, Nikita Travkin

On Tue, Jan 14, 2020 at 01:03:22PM +0000, Srinivas Kandagatla wrote:
> 
> 
> On 14/01/2020 12:08, Stephan Gerhold wrote:
> > On Tue, Jan 14, 2020 at 10:54:44AM +0000, Srinivas Kandagatla wrote:
> > > 
> > > 
> > > On 11/01/2020 16:40, Stephan Gerhold wrote:
> > > > At the moment, MIC BIAS Internal* and MIC BIAS External* both reference
> > > > the same register, and have a part of their initialization sequence
> > > > duplicated.
> > > > 
> > > > In general, the sequence for enabling MIC BIAS InternalX seems to be:
> > > >     1. Enable MIC BIAS ExternalX
> > > >     2. Enable internal RBIAS resistor
> > > 
> > > Does not sound correct to me.
> > > 
> > > What external means here is if the MICBIAS has external pull up resistor or
> > > not. And this is very much board specific. In order for driver to
> > > enable/disable internal pull up resistor in such cases we use two dapm paths
> > > to differentiate it.
> > > 
> > 
> > You are right. Maybe the naming is a bit confusing here.
> > Let me try to clarify it:
> > 
> > If I understand it correctly, setting the MICB_EN bit in CDC_A_MICB_1_EN
> > enables the MIC_BIAS1 supply. This supply can be either used with an
> > external pull up resistor ("MIC BIAS External1") or with the internal
> > pull up resistor ("MIC BIAS Internal1"). Which one of them, is board-specific.
> > 
> > To use the internal pull up resistor, we need to set the TX1_INT_RBIAS_EN
> > bit in CDC_A_MICB_1_INT_RBIAS additionally.
> > 
> > In other words, the sequence for enabling MIC BIAS Internal1 is:
> >    I1. Enable MIC_BIAS1 supply (MICB_EN bit in CDC_A_MICB_1_EN)
> >    I2. Enable internal RBIAS (TX1_INT_RBIAS_EN bit in CDC_A_MICB_1_INT_RBIAS)
> > 
> > The sequence for enabling MIC BIAS External1 is:
> >    E1. Enable MIC_BIAS1 supply (MICB_EN bit in CDC_A_MICB_1_EN)
> 
> In theory, We should also disable the internal pull up here. if not we end
> up in two parallel resistors, effectively 1/2 the value of external
> resistor.
>

I think this should not happen in practice with this patch.

The internal pull up is a separate DAPM supply with my changes.
As such, the ASoC core will disable it whenever it is not needed.

Therefore, the only way to trigger this situation is to specifically
craft broken DAPM routes in the device tree, e.g.

	qcom,audio-routing =
		"AMIC1", "MIC BIAS Internal1",
		"AMIC1", "MIC BIAS External1";

Which is obviously wrong, because you should select either Internal1
*or* External1, depending on your board design.

> 
> 
> > 
> > Right now we have:
> >    SND_SOC_DAPM_SUPPLY("MIC BIAS Internal1", CDC_A_MICB_1_EN, 7, 0, ...) // I1
> >    SND_SOC_DAPM_SUPPLY("MIC BIAS External1", CDC_A_MICB_1_EN, 7, 0, ...) // E1
> > 
> > I2 is done in the PM event handler (pm8916_wcd_analog_enable_micbias_int1).
> > 
> > The idea of this patch is to simplify this, and use one DAPM supply
> > that handles the common (I1/E1), and one DAPM supply that handles (I2).
> > 
> > Translated to code we get:
> >    SND_SOC_DAPM_SUPPLY("MIC BIAS1", CDC_A_MICB_1_EN, 7, 0, ...) // I1/E1
> >    SND_SOC_DAPM_SUPPLY("MIC BIAS Internal1", CDC_A_MICB_1_INT_RBIAS, 7, 0, ...) // I2
> >    SND_SOC_DAPM_SUPPLY("MIC BIAS External1", SND_SOC_NOPM, ...) // dummy
> > 
> > And the routes:
> >    {"MIC BIAS Internal1", NULL, "MIC BIAS1"} // If I2, also do I1
> >    {"MIC BIAS External1", NULL, "MIC BIAS1"} // Only do E1
> 
> This should work! and makes it much clear too.
> 

I will submit a v2 with this setup. I think I simplified it a bit too
much in this patch.

> 
> > 
> > As you can see, for "MIC BIAS External1" we just do "MIC BIAS1".
> > The confusing element of this patch might be that I simplified it a bit
> > further and combined "MIC BIAS1" with "MIC BIAS External1".
> > This works because we don't do anything extra for "MIC BIAS External1".
> Should we not make sure that internal resistor switch is disabled here?
> 

See above. (I don't mind sending a v3 if we decide that we really need to
disable the internal rbias explicitly.)

> --srini
> 
> > 
> > Does that make sense?
> > 
> > Thanks for taking the time to review all my patches.
> > Stephan
> > 
> > > 
> > > --srini
> > > 
> > > 
> > > 
> > > 
> > > > 
> > > > This means that we can simplify the code by modelling MIC BIAS InternalX
> > > > as supply connected to MIC BIAS ExternalX. MIC BIAS InternalX is only
> > > > responsible for enabling the internal RBIAS resistor (2). The DAPM enable
> > > > sequence will automatically enable MIC BIAS ExternalX (1).
> > > > 
> > > > This makes it much easier to add support for MIC BIAS Internal3
> > > > as a next step.
> > > > 
> > > > Tested-by: Nikita Travkin <nikitos.tr@gmail.com> # longcheer-l8150
> > > > Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > > > ---
> > > >    sound/soc/codecs/msm8916-wcd-analog.c | 57 ++++++---------------------
> > > >    1 file changed, 13 insertions(+), 44 deletions(-)
> > > > 
> > > > diff --git a/sound/soc/codecs/msm8916-wcd-analog.c b/sound/soc/codecs/msm8916-wcd-analog.c
> > > > index 1f7964beb20c..930baae6eff5 100644
> > > > --- a/sound/soc/codecs/msm8916-wcd-analog.c
> > > > +++ b/sound/soc/codecs/msm8916-wcd-analog.c
> > > > @@ -389,23 +389,17 @@ static int pm8916_wcd_analog_enable_micbias_ext(struct snd_soc_component
> > > >    	return 0;
> > > >    }
> > > > -static int pm8916_wcd_analog_enable_micbias_int(struct snd_soc_component
> > > > -						 *component, int event,
> > > > -						 int reg, u32 cap_mode)
> > > > +static int pm8916_wcd_analog_enable_micbias_int(struct snd_soc_dapm_widget *w,
> > > > +						struct snd_kcontrol *kcontrol,
> > > > +						int event)
> > > >    {
> > > > +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> > > >    	switch (event) {
> > > >    	case SND_SOC_DAPM_PRE_PMU:
> > > > -		snd_soc_component_update_bits(component, reg, MICB_1_EN_PULL_DOWN_EN_MASK, 0);
> > > >    		snd_soc_component_update_bits(component, CDC_A_MICB_1_EN,
> > > >    				    MICB_1_EN_OPA_STG2_TAIL_CURR_MASK,
> > > >    				    MICB_1_EN_OPA_STG2_TAIL_CURR_1_60UA);
> > > > -
> > > > -		break;
> > > > -	case SND_SOC_DAPM_POST_PMU:
> > > > -		pm8916_wcd_analog_micbias_enable(component);
> > > > -		snd_soc_component_update_bits(component, CDC_A_MICB_1_EN,
> > > > -				    MICB_1_EN_BYP_CAP_MASK, cap_mode);
> > > >    		break;
> > > >    	}
> > > > @@ -437,26 +431,6 @@ static int pm8916_wcd_analog_enable_micbias_ext2(struct
> > > >    }
> > > > -static int pm8916_wcd_analog_enable_micbias_int1(struct
> > > > -						  snd_soc_dapm_widget
> > > > -						  *w, struct snd_kcontrol
> > > > -						  *kcontrol, int event)
> > > > -{
> > > > -	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> > > > -	struct pm8916_wcd_analog_priv *wcd = snd_soc_component_get_drvdata(component);
> > > > -
> > > > -	switch (event) {
> > > > -	case SND_SOC_DAPM_PRE_PMU:
> > > > -		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
> > > > -				    MICB_1_INT_TX1_INT_RBIAS_EN_MASK,
> > > > -				    MICB_1_INT_TX1_INT_RBIAS_EN_ENABLE);
> > > > -		break;
> > > > -	}
> > > > -
> > > > -	return pm8916_wcd_analog_enable_micbias_int(component, event, w->reg,
> > > > -						     wcd->micbias1_cap_mode);
> > > > -}
> > > > -
> > > >    static int pm8916_mbhc_configure_bias(struct pm8916_wcd_analog_priv *priv,
> > > >    				      bool micbias2_enabled)
> > > >    {
> > > > @@ -564,9 +538,8 @@ static int pm8916_wcd_analog_enable_micbias_int2(struct
> > > >    	switch (event) {
> > > >    	case SND_SOC_DAPM_PRE_PMU:
> > > > -		snd_soc_component_update_bits(component, CDC_A_MICB_1_INT_RBIAS,
> > > > -				    MICB_1_INT_TX2_INT_RBIAS_EN_MASK,
> > > > -				    MICB_1_INT_TX2_INT_RBIAS_EN_ENABLE);
> > > > +		snd_soc_component_update_bits(component, CDC_A_MICB_2_EN,
> > > > +					      CDC_A_MICB_2_PULL_DOWN_EN_MASK, 0);
> > > >    		break;
> > > >    	case SND_SOC_DAPM_POST_PMU:
> > > >    		pm8916_mbhc_configure_bias(wcd, true);
> > > > @@ -576,8 +549,7 @@ static int pm8916_wcd_analog_enable_micbias_int2(struct
> > > >    		break;
> > > >    	}
> > > > -	return pm8916_wcd_analog_enable_micbias_int(component, event, w->reg,
> > > > -						     wcd->micbias2_cap_mode);
> > > > +	return pm8916_wcd_analog_enable_micbias_int(w, kcontrol, event);
> > > >    }
> > > >    static int pm8916_wcd_analog_enable_adc(struct snd_soc_dapm_widget *w,
> > > > @@ -878,12 +850,10 @@ static const struct snd_soc_dapm_route pm8916_wcd_analog_audio_map[] = {
> > > >    	{"SPK PA", NULL, "SPK DAC"},
> > > >    	{"SPK DAC", "Switch", "PDM_RX3"},
> > > > -	{"MIC BIAS Internal1", NULL, "INT_LDO_H"},
> > > > -	{"MIC BIAS Internal2", NULL, "INT_LDO_H"},
> > > > +	{"MIC BIAS Internal1", NULL, "MIC BIAS External1"},
> > > > +	{"MIC BIAS Internal2", NULL, "MIC BIAS External2"},
> > > >    	{"MIC BIAS External1", NULL, "INT_LDO_H"},
> > > >    	{"MIC BIAS External2", NULL, "INT_LDO_H"},
> > > > -	{"MIC BIAS Internal1", NULL, "vdd-micbias"},
> > > > -	{"MIC BIAS Internal2", NULL, "vdd-micbias"},
> > > >    	{"MIC BIAS External1", NULL, "vdd-micbias"},
> > > >    	{"MIC BIAS External2", NULL, "vdd-micbias"},
> > > >    };
> > > > @@ -937,11 +907,10 @@ static const struct snd_soc_dapm_widget pm8916_wcd_analog_dapm_widgets[] = {
> > > >    	SND_SOC_DAPM_SUPPLY("RX_BIAS", CDC_A_RX_COM_BIAS_DAC, 7, 0, NULL, 0),
> > > >    	/* TX */
> > > > -	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal1", CDC_A_MICB_1_EN, 7, 0,
> > > > -			    pm8916_wcd_analog_enable_micbias_int1,
> > > > -			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
> > > > -			    SND_SOC_DAPM_POST_PMD),
> > > > -	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal2", CDC_A_MICB_2_EN, 7, 0,
> > > > +	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal1", CDC_A_MICB_1_INT_RBIAS, 7, 0,
> > > > +			    pm8916_wcd_analog_enable_micbias_int,
> > > > +			    SND_SOC_DAPM_PRE_PMU),
> > > > +	SND_SOC_DAPM_SUPPLY("MIC BIAS Internal2", CDC_A_MICB_1_INT_RBIAS, 4, 0,
> > > >    			    pm8916_wcd_analog_enable_micbias_int2,
> > > >    			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
> > > >    			    SND_SOC_DAPM_POST_PMD),
> > > > 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

end of thread, other threads:[~2020-01-14 16:55 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-11 16:40 [alsa-devel] [PATCH 0/4] ASoC: msm8916-wcd-analog: MIC BIAS fixes/additions Stephan Gerhold
2020-01-11 16:40 ` [alsa-devel] [PATCH 1/4] ASoC: msm8916-wcd-analog: Fix selected events for MIC BIAS External1 Stephan Gerhold
2020-01-13 11:32   ` Srinivas Kandagatla
2020-01-13 15:13   ` [alsa-devel] Applied "ASoC: msm8916-wcd-analog: Fix selected events for MIC BIAS External1" to the asoc tree Mark Brown
2020-01-11 16:40 ` [alsa-devel] [PATCH 2/4] ASoC: msm8916-wcd-analog: Fix MIC BIAS Internal1 Stephan Gerhold
2020-01-13 14:08   ` Srinivas Kandagatla
2020-01-13 15:13   ` [alsa-devel] Applied "ASoC: msm8916-wcd-analog: Fix MIC BIAS Internal1" to the asoc tree Mark Brown
2020-01-11 16:40 ` [alsa-devel] [PATCH 3/4] ASoC: msm8916-wcd-analog: Simplify MIC BIAS Internal Stephan Gerhold
2020-01-14 10:54   ` Srinivas Kandagatla
2020-01-14 12:08     ` Stephan Gerhold
2020-01-14 13:03       ` Srinivas Kandagatla
2020-01-14 16:54         ` Stephan Gerhold
2020-01-11 16:40 ` [alsa-devel] [PATCH 4/4] ASoC: msm8916-wcd-analog: Add MIC BIAS Internal3 Stephan Gerhold

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).