[alsa-devel] [PATCH 3/4] ASoC: msm8916-wcd-analog: Simplify MIC BIAS Internal

[Srinivas Kandagatla]

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 at gmail.com> # longcheer-l8150
>>> Cc: Srinivas Kandagatla <srinivas.kandagatla at linaro.org>
>>> Signed-off-by: Stephan Gerhold <stephan at 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),
>>>