[PATCHv2 0/2] ASoC: TWL4030: APLL/AIF ordering and DAPM updates

[PATCHv2 0/2] ASoC: TWL4030: APLL/AIF ordering and DAPM updates

[Peter Ujfalusi]

Hello,

Correct the APLL/AIF powering sequence.
Also introducing DAPM outputs and input for keeping APLL/AIF
enabled in case when no valid DAPM route selected during audio
activity.

The second patch removes the legacy OUTL/R.

As a note:
The APLL/AIF patch replaces the same patch in Liam's series.
The OUTL/R removal can be applied after the OUTL/R reference
has been removed from machine drivers (as in Lima's series).

---
Peter Ujfalusi (2):
  ASoC: TWL4030: AIF/APLL fix in DAPM domain
  ASoC: TWL4030: Remove OUTL/R outputs

 sound/soc/codecs/twl4030.c |   91 ++++++++++++++++++++++++++++++++++----------
 1 files changed, 71 insertions(+), 20 deletions(-)

[PATCHv2 1/2] ASoC: TWL4030: AIF/APLL fix in DAPM domain

[Peter Ujfalusi]

This patch orders the APLL and AIF power sequence in
case of HiFi (audio in TWL4030 terms) playback.

We also need to make sure that the AIF is running during
playback/capture, when there is no valid DAPM route
available.

The apll and aif reference counting code has been lifted,
and reused from Liam Girdwood's earlier patch.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
 sound/soc/codecs/twl4030.c |   87 ++++++++++++++++++++++++++++++++++++--------
 1 files changed, 71 insertions(+), 16 deletions(-)

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index 2e025a3..78a53e7 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -123,7 +123,10 @@ struct twl4030_priv {
 	struct snd_soc_codec codec;
 
 	unsigned int codec_powered;
+
+	/* reference counts of AIF/APLL users */
 	unsigned int apll_enabled;
+	unsigned int aif_enabled;
 
 	struct snd_pcm_substream *master_substream;
 	struct snd_pcm_substream *slave_substream;
@@ -259,22 +262,49 @@ static void twl4030_init_chip(struct snd_soc_codec *codec)
 static void twl4030_apll_enable(struct snd_soc_codec *codec, int enable)
 {
 	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
-	int status;
-
-	if (enable == twl4030->apll_enabled)
-		return;
+	int status = -1;
 
 	if (enable)
+		twl4030->apll_enabled++;
+	else
+		twl4030->apll_enabled--;
+
+	if (enable && twl4030->apll_enabled == 1)
 		/* Enable PLL */
 		status = twl4030_codec_enable_resource(TWL4030_CODEC_RES_APLL);
-	else
+	else if (!enable && twl4030->apll_enabled == 0)
 		/* Disable PLL */
 		status = twl4030_codec_disable_resource(TWL4030_CODEC_RES_APLL);
 
 	if (status >= 0)
 		twl4030_write_reg_cache(codec, TWL4030_REG_APLL_CTL, status);
+}
 
-	twl4030->apll_enabled = enable;
+static void twl4030_aif_enable(struct snd_soc_codec *codec, int enable)
+{
+	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
+	u8 audio_if;
+
+	/* update the user count */
+	if (enable)
+		twl4030->aif_enabled++;
+	else
+		twl4030->aif_enabled--;
+
+	audio_if = twl4030_read_reg_cache(codec, TWL4030_REG_AUDIO_IF);
+	if (enable && twl4030->aif_enabled == 1) {
+		/* Enable AIF */
+		/* enable the PLL before we use it to clock the DAI */
+		twl4030_apll_enable(codec, 1);
+
+		twl4030_write(codec, TWL4030_REG_AUDIO_IF,
+						audio_if | TWL4030_AIF_EN);
+	} else if (!enable && twl4030->aif_enabled == 0) {
+		/* disable the DAI before we stop it's source PLL */
+		twl4030_write(codec, TWL4030_REG_AUDIO_IF,
+						audio_if &  ~TWL4030_AIF_EN);
+		twl4030_apll_enable(codec, 0);
+	}
 }
 
 static void twl4030_power_up(struct snd_soc_codec *codec)
@@ -672,6 +702,20 @@ static int apll_event(struct snd_soc_dapm_widget *w,
 	return 0;
 }
 
+static int aif_event(struct snd_soc_dapm_widget *w,
+		struct snd_kcontrol *kcontrol, int event)
+{
+	switch (event) {
+	case SND_SOC_DAPM_PRE_PMU:
+		twl4030_aif_enable(w->codec, 1);
+		break;
+	case SND_SOC_DAPM_POST_PMD:
+		twl4030_aif_enable(w->codec, 0);
+		break;
+	}
+	return 0;
+}
+
 static void headset_ramp(struct snd_soc_codec *codec, int ramp)
 {
 	struct snd_soc_device *socdev = codec->socdev;
@@ -1180,6 +1224,11 @@ static const struct snd_soc_dapm_widget twl4030_dapm_widgets[] = {
 	SND_SOC_DAPM_OUTPUT("HFR"),
 	SND_SOC_DAPM_OUTPUT("VIBRA"),
 
+	/* AIF and APLL clocks for running DAIs (including loopback) */
+	SND_SOC_DAPM_OUTPUT("AIF DAC"),
+	SND_SOC_DAPM_INPUT("AIF ADC"),
+	SND_SOC_DAPM_OUTPUT("APLL"),
+
 	/* DACs */
 	SND_SOC_DAPM_DAC("DAC Right1", "Right Front HiFi Playback",
 			SND_SOC_NOPM, 0, 0),
@@ -1243,7 +1292,8 @@ static const struct snd_soc_dapm_widget twl4030_dapm_widgets[] = {
 	SND_SOC_DAPM_SUPPLY("APLL Enable", SND_SOC_NOPM, 0, 0, apll_event,
 			    SND_SOC_DAPM_PRE_PMU|SND_SOC_DAPM_POST_PMD),
 
-	SND_SOC_DAPM_SUPPLY("AIF Enable", TWL4030_REG_AUDIO_IF, 0, 0, NULL, 0),
+	SND_SOC_DAPM_SUPPLY("AIF Enable", SND_SOC_NOPM, 0, 0, aif_event,
+			    SND_SOC_DAPM_PRE_PMU|SND_SOC_DAPM_POST_PMD),
 
 	/* Output MIXER controls */
 	/* Earpiece */
@@ -1373,10 +1423,6 @@ static const struct snd_soc_dapm_route intercon[] = {
 	{"Digital Voice Playback Mixer", NULL, "DAC Voice"},
 
 	/* Supply for the digital part (APLL) */
-	{"Digital R1 Playback Mixer", NULL, "APLL Enable"},
-	{"Digital L1 Playback Mixer", NULL, "APLL Enable"},
-	{"Digital R2 Playback Mixer", NULL, "APLL Enable"},
-	{"Digital L2 Playback Mixer", NULL, "APLL Enable"},
 	{"Digital Voice Playback Mixer", NULL, "APLL Enable"},
 
 	{"Digital R1 Playback Mixer", NULL, "AIF Enable"},
@@ -1450,6 +1496,14 @@ static const struct snd_soc_dapm_route intercon[] = {
 	{"Vibra Mux", "AudioR2", "DAC Right2"},
 
 	/* outputs */
+	/* Must be always connected (for AIF and APLL) */
+	{"AIF DAC", NULL, "Digital L1 Playback Mixer"},
+	{"AIF DAC", NULL, "Digital R1 Playback Mixer"},
+	{"AIF DAC", NULL, "Digital L2 Playback Mixer"},
+	{"AIF DAC", NULL, "Digital R2 Playback Mixer"},
+	/* Must be always connected (for APLL) */
+	{"APLL", NULL, "Digital Voice Playback Mixer"},
+	/* Physical outputs */
 	{"OUTL", NULL, "Analog L2 Playback Mixer"},
 	{"OUTR", NULL, "Analog R2 Playback Mixer"},
 	{"EARPIECE", NULL, "Earpiece PGA"},
@@ -1465,6 +1519,12 @@ static const struct snd_soc_dapm_route intercon[] = {
 	{"VIBRA", NULL, "Vibra Route"},
 
 	/* Capture path */
+	/* Must be always connected (for AIF and APLL) */
+	{"ADC Virtual Left1", NULL, "AIF ADC"},
+	{"ADC Virtual Right1", NULL, "AIF ADC"},
+	{"ADC Virtual Left2", NULL, "AIF ADC"},
+	{"ADC Virtual Right2", NULL, "AIF ADC"},
+	/* Physical inputs */
 	{"Analog Left", "Main Mic Capture Switch", "MAINMIC"},
 	{"Analog Left", "Headset Mic Capture Switch", "HSMIC"},
 	{"Analog Left", "AUXL Capture Switch", "AUXL"},
@@ -1497,11 +1557,6 @@ static const struct snd_soc_dapm_route intercon[] = {
 	{"ADC Virtual Left2", NULL, "TX2 Capture Route"},
 	{"ADC Virtual Right2", NULL, "TX2 Capture Route"},
 
-	{"ADC Virtual Left1", NULL, "APLL Enable"},
-	{"ADC Virtual Right1", NULL, "APLL Enable"},
-	{"ADC Virtual Left2", NULL, "APLL Enable"},
-	{"ADC Virtual Right2", NULL, "APLL Enable"},
-
 	{"ADC Virtual Left1", NULL, "AIF Enable"},
 	{"ADC Virtual Right1", NULL, "AIF Enable"},
 	{"ADC Virtual Left2", NULL, "AIF Enable"},
-- 
1.7.0.4

Re: [PATCHv2 1/2] ASoC: TWL4030: AIF/APLL fix in DAPM domain

[Mark Brown]

On Wed, Apr 28, 2010 at 03:50:14PM +0300, Peter Ujfalusi wrote:

> +	if (enable && twl4030->apll_enabled == 1)
>  		/* Enable PLL */
>  		status = twl4030_codec_enable_resource(TWL4030_CODEC_RES_APLL);
> -	else
> +	else if (!enable && twl4030->apll_enabled == 0)

This logic looks funny, especially the test for !enable in the second
case (which should always be true).  I think the intention here is to
look for "apll_enabled has done an interesting transition (0->1 or
1->0)" - it'd probably be clearer to look for that directly.

> +	/* AIF and APLL clocks for running DAIs (including loopback) */
> +	SND_SOC_DAPM_OUTPUT("AIF DAC"),
> +	SND_SOC_DAPM_INPUT("AIF ADC"),
> +	SND_SOC_DAPM_OUTPUT("APLL"),

The use of INPUT and OUTPUT widgets here looks really odd - I'd at least
except the AIF widgets to be actual AIF widgets.

Re: [PATCHv2 1/2] ASoC: TWL4030: AIF/APLL fix in DAPM domain

[Peter Ujfalusi]

Hi,

On Wednesday 28 April 2010 16:12:35 ext Mark Brown wrote:
> On Wed, Apr 28, 2010 at 03:50:14PM +0300, Peter Ujfalusi wrote:
> > +	if (enable && twl4030->apll_enabled == 1)
> > 
> >  		/* Enable PLL */
> >  		status = twl4030_codec_enable_resource(TWL4030_CODEC_RES_APLL);
> > 
> > -	else
> > +	else if (!enable && twl4030->apll_enabled == 0)
> 
> This logic looks funny, especially the test for !enable in the second
> case (which should always be true).  I think the intention here is to
> look for "apll_enabled has done an interesting transition (0->1 or
> 1->0)" - it'd probably be clearer to look for that directly.

Yeah, I have not bothered to change this from Liam's patch, but yes. It does 
look funny.
Probably something like this would be nicer:
if (enable) {
	twl4030->apll_enabled++;
	if (twl4030->apll_enabled == 1)
		status = twl4030_codec_enable_resource(TWL4030_CODEC_RES_APLL);
} else {
	twl4030->apll_enabled--;
	if (!twl4030->apll_enabled)
		status = twl4030_codec_disable_resource(TWL4030_CODEC_RES_APLL);
}

After taking a deeper look, the aif does not need any refcounting, since DAPM 
already takes care of that.
So I will simplify that as well.

> 
> > +	/* AIF and APLL clocks for running DAIs (including loopback) */
> > +	SND_SOC_DAPM_OUTPUT("AIF DAC"),
> > +	SND_SOC_DAPM_INPUT("AIF ADC"),
> > +	SND_SOC_DAPM_OUTPUT("APLL"),
> 
> The use of INPUT and OUTPUT widgets here looks really odd - I'd at least
> except the AIF widgets to be actual AIF widgets.

I agree.
These supposed to be 'virtual' outputs, and input.
Is it helps, if I name them as:
SND_SOC_DAPM_OUTPUT("Virtual HiFi OUT"),
SND_SOC_DAPM_INPUT("Virtual HiFi IN"),
SND_SOC_DAPM_OUTPUT("Virtual Voice OUT"),

These are needed to enable the AIF, and/or APLL whenever they are needed by 
DAPM.

I'm switching the AIF/APLL within the DAPM (with DAPM_SUPPLY).
The problem is that if we don't have complete DAPM route (in playback or in 
capture), than DAPM will not enable the AIF/APLL. If the twl is master, than it 
means that the clocks will not run on the serial bus. This means that no data is 
going to be sent or received on the host side -> broken audio.

With these in place, I can make sure that we have complete route all the time, 
so AIF/APLL is enabled.

I know that other codecs (like the tlv320dac33) is enabling the AIF in 
pcm_prepare, or in other places, but with twl4030 those will not work in all 
cases:
If I use pcm_prepare for this, than I have a problem with the digital loopback, 
since that also needs the AIF/APLL enabled.
If I use the DAPM's set_bias_level, than I have two problems:
1. The analog loopback does not need the AIF/APLL enabled, so I have to handle 
that differently.
2. When there is no complete DAPM route, the codec will not change to BIAS_ON, 
so I still end up with dead interface.

The only solution so far is to introduce these virtual outputs/input, and handle 
the AIF/APLL within DAPM.

As a note: I'm already looking at the AIF widget usage, but I'm not sure how it 
supposed to be mapped in twl4030 case.
TWL has basically 4 channel interface:
Playback
AIF		Stereo mode	TDM mode
SDRL2	Left (1)		Ch 1
SDRL1	---			Ch 2
SDRR2	Right (2)		Ch 3
SDRR1	---			Ch 4

Capture
AIF		Stereo mode	TDM mode
ATXL1	Left (1)		Ch 1
AVTXL2	---			Ch 2
ATXR1	Right (2)		Ch 3
AVTXR2	---			Ch 4

How to map these with the AIF interface in Stereo and TDM (4 channel mode) in a 
consistent way?

I will rename the DAPM_OUTPUT and DAPM_INPUT widgets to the "Virtual *" 
convention, which I think sounds better also.


-- 
Péter

[PATCHv2 2/2] ASoC: TWL4030: Remove OUTL/R outputs

[Peter Ujfalusi]

OUTL/R are leftovers from the original driver, and they
are no longer needed.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
 sound/soc/codecs/twl4030.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index 78a53e7..816a995 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -1211,8 +1211,6 @@ static const struct snd_soc_dapm_widget twl4030_dapm_widgets[] = {
 	SND_SOC_DAPM_INPUT("DIGIMIC1"),
 
 	/* Outputs */
-	SND_SOC_DAPM_OUTPUT("OUTL"),
-	SND_SOC_DAPM_OUTPUT("OUTR"),
 	SND_SOC_DAPM_OUTPUT("EARPIECE"),
 	SND_SOC_DAPM_OUTPUT("PREDRIVEL"),
 	SND_SOC_DAPM_OUTPUT("PREDRIVER"),
@@ -1504,8 +1502,6 @@ static const struct snd_soc_dapm_route intercon[] = {
 	/* Must be always connected (for APLL) */
 	{"APLL", NULL, "Digital Voice Playback Mixer"},
 	/* Physical outputs */
-	{"OUTL", NULL, "Analog L2 Playback Mixer"},
-	{"OUTR", NULL, "Analog R2 Playback Mixer"},
 	{"EARPIECE", NULL, "Earpiece PGA"},
 	{"PREDRIVEL", NULL, "PredriveL PGA"},
 	{"PREDRIVER", NULL, "PredriveR PGA"},
-- 
1.7.0.4

Re: [PATCHv2 2/2] ASoC: TWL4030: Remove OUTL/R outputs

[Mark Brown]

On Wed, Apr 28, 2010 at 03:50:15PM +0300, Peter Ujfalusi wrote:
> OUTL/R are leftovers from the original driver, and they
> are no longer needed.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>