[PATCH 1/3] ASoC: TWL4030: Enable voice filters for voice sidetone

[PATCH 1/3] ASoC: TWL4030: Enable voice filters for voice sidetone

[Lopez Cruz, Misael]

Digital voice loopback (sidetone) requires voice filters
to be enabled: VTXL, VTXR, VRX.

Signed-off-by: Misael Lopez Cruz <x0052729@ti.com>
---
 sound/soc/codecs/twl4030.c |   67 +++++++++++++++++++++++++-------------------
 1 files changed, 38 insertions(+), 29 deletions(-)

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index 4dbb853..780217a 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -332,6 +332,28 @@ static void twl4030_power_down(struct snd_soc_codec *codec)
 	twl4030_codec_enable(codec, 0);
 }
 
+/* In case of voice mode, the RX1 L(VRX) for downlink and the TX2 L/R
+ * (VTXL, VTXR) for uplink has to be enabled/disabled. */
+static void twl4030_voice_enable(struct snd_soc_codec *codec, int direction,
+				int enable)
+{
+	u8 reg, mask;
+
+	reg = twl4030_read_reg_cache(codec, TWL4030_REG_OPTION);
+
+	if (direction == SNDRV_PCM_STREAM_PLAYBACK)
+		mask = TWL4030_ARXL1_VRX_EN;
+	else
+		mask = TWL4030_ATXL2_VTXL_EN | TWL4030_ATXR2_VTXR_EN;
+
+	if (enable)
+		reg |= mask;
+	else
+		reg &= ~mask;
+
+	twl4030_write(codec, TWL4030_REG_OPTION, reg);
+}
+
 /* Earpiece */
 static const struct snd_kcontrol_new twl4030_dapm_earpiece_controls[] = {
 	SOC_DAPM_SINGLE("Voice", TWL4030_REG_EAR_CTL, 0, 1, 0),
@@ -712,7 +734,22 @@ static int bypass_event(struct snd_soc_dapm_widget *w,
 
 	reg = twl4030_read_reg_cache(w->codec, m->reg);
 
-	if (m->reg <= TWL4030_REG_ARXR2_APGA_CTL) {
+	if (m->reg == TWL4030_REG_VSTPGA) {
+		/* Voice digital bypass */
+		if (reg) {
+			twl4030->bypass_state |= (1 << 5);
+			twl4030_voice_enable(w->codec,
+					SNDRV_PCM_STREAM_PLAYBACK, 1);
+			twl4030_voice_enable(w->codec,
+					SNDRV_PCM_STREAM_CAPTURE, 1);
+		} else {
+			twl4030->bypass_state &= ~(1 << 5);
+			twl4030_voice_enable(w->codec,
+					SNDRV_PCM_STREAM_PLAYBACK, 0);
+			twl4030_voice_enable(w->codec,
+					SNDRV_PCM_STREAM_CAPTURE, 0);
+		}
+	} else if (m->reg <= TWL4030_REG_ARXR2_APGA_CTL) {
 		/* Analog bypass */
 		if (reg & (1 << m->shift))
 			twl4030->bypass_state |=
@@ -726,12 +763,6 @@ static int bypass_event(struct snd_soc_dapm_widget *w,
 			twl4030->bypass_state |= (1 << 4);
 		else
 			twl4030->bypass_state &= ~(1 << 4);
-	} else if (m->reg == TWL4030_REG_VSTPGA) {
-		/* Voice digital bypass */
-		if (reg)
-			twl4030->bypass_state |= (1 << 5);
-		else
-			twl4030->bypass_state &= ~(1 << 5);
 	} else {
 		/* Digital bypass */
 		if (reg & (0x7 << m->shift))
@@ -1806,28 +1837,6 @@ static int twl4030_set_dai_fmt(struct snd_soc_dai *codec_dai,
 	return 0;
 }
 
-/* In case of voice mode, the RX1 L(VRX) for downlink and the TX2 L/R
- * (VTXL, VTXR) for uplink has to be enabled/disabled. */
-static void twl4030_voice_enable(struct snd_soc_codec *codec, int direction,
-				int enable)
-{
-	u8 reg, mask;
-
-	reg = twl4030_read_reg_cache(codec, TWL4030_REG_OPTION);
-
-	if (direction == SNDRV_PCM_STREAM_PLAYBACK)
-		mask = TWL4030_ARXL1_VRX_EN;
-	else
-		mask = TWL4030_ATXL2_VTXL_EN | TWL4030_ATXR2_VTXR_EN;
-
-	if (enable)
-		reg |= mask;
-	else
-		reg &= ~mask;
-
-	twl4030_write(codec, TWL4030_REG_OPTION, reg);
-}
-
 static int twl4030_voice_startup(struct snd_pcm_substream *substream,
 		struct snd_soc_dai *dai)
 {
-- 
1.5.4.3

Re: [PATCH 1/3] ASoC: TWL4030: Enable voice filters for voice sidetone

[Mark Brown]

On Fri, Jun 19, 2009 at 03:23:41AM -0500, Lopez Cruz, Misael wrote:
> Digital voice loopback (sidetone) requires voice filters
> to be enabled: VTXL, VTXR, VRX.

Is it possible to handle this by inserting DAPM widgets for these
filters into the bypass paths and letting DAPM power them up?  I'm also
wondering if with the new bias level management stuff which keeps the
codec in BIAS_ON while any DAPM widgets are powered some of the bypass
mode stuff can be removed or simplified?

I'm OK with the change if Peter acks it, I've not got detailed knowledge
of the TWL4030.

Re: [PATCH 1/3] ASoC: TWL4030: Enable voice filters for voice sidetone

[Peter Ujfalusi]

On Friday 19 June 2009 11:23:41 ext Lopez Cruz, Misael wrote:
> Digital voice loopback (sidetone) requires voice filters
> to be enabled: VTXL, VTXR, VRX.
>
> Signed-off-by: Misael Lopez Cruz <x0052729@ti.com>
> ---
>  sound/soc/codecs/twl4030.c |   67
> +++++++++++++++++++++++++------------------- 1 files changed, 38
> insertions(+), 29 deletions(-)
>
> diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
> index 4dbb853..780217a 100644
> --- a/sound/soc/codecs/twl4030.c
> +++ b/sound/soc/codecs/twl4030.c
> @@ -332,6 +332,28 @@ static void twl4030_power_down(struct snd_soc_codec
> *codec) twl4030_codec_enable(codec, 0);
>  }
>
> +/* In case of voice mode, the RX1 L(VRX) for downlink and the TX2 L/R
> + * (VTXL, VTXR) for uplink has to be enabled/disabled. */
> +static void twl4030_voice_enable(struct snd_soc_codec *codec, int
> direction, +				int enable)
> +{
> +	u8 reg, mask;
> +
> +	reg = twl4030_read_reg_cache(codec, TWL4030_REG_OPTION);
> +
> +	if (direction == SNDRV_PCM_STREAM_PLAYBACK)
> +		mask = TWL4030_ARXL1_VRX_EN;
> +	else
> +		mask = TWL4030_ATXL2_VTXL_EN | TWL4030_ATXR2_VTXR_EN;
> +
> +	if (enable)
> +		reg |= mask;
> +	else
> +		reg &= ~mask;
> +
> +	twl4030_write(codec, TWL4030_REG_OPTION, reg);
> +}
> +
>  /* Earpiece */
>  static const struct snd_kcontrol_new twl4030_dapm_earpiece_controls[] = {
>  	SOC_DAPM_SINGLE("Voice", TWL4030_REG_EAR_CTL, 0, 1, 0),
> @@ -712,7 +734,22 @@ static int bypass_event(struct snd_soc_dapm_widget *w,
>
>  	reg = twl4030_read_reg_cache(w->codec, m->reg);
>
> -	if (m->reg <= TWL4030_REG_ARXR2_APGA_CTL) {
> +	if (m->reg == TWL4030_REG_VSTPGA) {
> +		/* Voice digital bypass */
> +		if (reg) {
> +			twl4030->bypass_state |= (1 << 5);
> +			twl4030_voice_enable(w->codec,
> +					SNDRV_PCM_STREAM_PLAYBACK, 1);
> +			twl4030_voice_enable(w->codec,
> +					SNDRV_PCM_STREAM_CAPTURE, 1);
> +		} else {
> +			twl4030->bypass_state &= ~(1 << 5);
> +			twl4030_voice_enable(w->codec,
> +					SNDRV_PCM_STREAM_PLAYBACK, 0);
> +			twl4030_voice_enable(w->codec,
> +					SNDRV_PCM_STREAM_CAPTURE, 0);
> +		}
> +	} else if (m->reg <= TWL4030_REG_ARXR2_APGA_CTL) {

This is not going to work correctly, consider the following scenario (for 
example):
1. You start recording/playback on the voice interface
  This will enables VTXL, VTXR/VRX on start.
2. you enable the Voice bypass
  This will enables (again) VTXL, VTXR and VRX.
3. you disable the Voice bypass
  This will disables VTXL, VTXR/VRX.
4. The ongoing recording/playback will break, since the VTXL, VTXR/VRX has 
been disabled...

I think for the Voice bypass it is feasible (as Mark pointed out) to inject 
DAPM widgets into the Voice playback and capture route (to a place, where the 
playback will enable the VRX, capture will enable the VTXL/VTXR, than make the 
bypass connection that it will enable both).


>  		/* Analog bypass */
>  		if (reg & (1 << m->shift))
>  			twl4030->bypass_state |=
> @@ -726,12 +763,6 @@ static int bypass_event(struct snd_soc_dapm_widget *w,
>  			twl4030->bypass_state |= (1 << 4);
>  		else
>  			twl4030->bypass_state &= ~(1 << 4);
> -	} else if (m->reg == TWL4030_REG_VSTPGA) {
> -		/* Voice digital bypass */
> -		if (reg)
> -			twl4030->bypass_state |= (1 << 5);
> -		else
> -			twl4030->bypass_state &= ~(1 << 5);
>  	} else {
>  		/* Digital bypass */
>  		if (reg & (0x7 << m->shift))
> @@ -1806,28 +1837,6 @@ static int twl4030_set_dai_fmt(struct snd_soc_dai
> *codec_dai, return 0;
>  }
>
> -/* In case of voice mode, the RX1 L(VRX) for downlink and the TX2 L/R
> - * (VTXL, VTXR) for uplink has to be enabled/disabled. */
> -static void twl4030_voice_enable(struct snd_soc_codec *codec, int
> direction, -				int enable)
> -{
> -	u8 reg, mask;
> -
> -	reg = twl4030_read_reg_cache(codec, TWL4030_REG_OPTION);
> -
> -	if (direction == SNDRV_PCM_STREAM_PLAYBACK)
> -		mask = TWL4030_ARXL1_VRX_EN;
> -	else
> -		mask = TWL4030_ATXL2_VTXL_EN | TWL4030_ATXR2_VTXR_EN;
> -
> -	if (enable)
> -		reg |= mask;
> -	else
> -		reg &= ~mask;
> -
> -	twl4030_write(codec, TWL4030_REG_OPTION, reg);
> -}
> -
>  static int twl4030_voice_startup(struct snd_pcm_substream *substream,
>  		struct snd_soc_dai *dai)
>  {

-- 
Péter

Re: [PATCH 1/3] ASoC: TWL4030: Enable voice filters for voice sidetone

[Peter Ujfalusi]

On Friday 19 June 2009 12:58:08 ext Mark Brown wrote:
> On Fri, Jun 19, 2009 at 03:23:41AM -0500, Lopez Cruz, Misael wrote:
> > Digital voice loopback (sidetone) requires voice filters
> > to be enabled: VTXL, VTXR, VRX.
>
> Is it possible to handle this by inserting DAPM widgets for these
> filters into the bypass paths and letting DAPM power them up? 

I think for the Voice interface this is feasible.

> I'm also wondering if with the new bias level management stuff which keeps
> the codec in BIAS_ON while any DAPM widgets are powered some of the bypass
> mode stuff can be removed or simplified?

I have been wondering about the same, but in the HiFi path it is not that 
simple. Most probably it is possible to add DAPM widget for the Audio Filter 
L1/R1 (which is at the moment always enabled), but for the L2/R2 it is not 
that simple.
Simplifying the twl4030_mute function should be possible at this point, I 
think, by moving those output mutes as DAPM_PGA_E widgets on the paths. This 
is in my to-do list... Although I have to check that such a change should not 
cause any 'clicks' on the outputs.

>
> I'm OK with the change if Peter acks it, I've not got detailed knowledge
> of the TWL4030.

I think this patch is not good. See my comment on the patch.

-- 
Péter

Re: [PATCH 1/3] ASoC: TWL4030: Enable voice filters for voice sidetone

[Lopez Cruz, Misael]

> On Friday 19 June 2009 12:58:08 ext Mark Brown wrote:
> > On Fri, Jun 19, 2009 at 03:23:41AM -0500, Lopez Cruz, Misael wrote:
> > > Digital voice loopback (sidetone) requires voice filters
> > > to be enabled: VTXL, VTXR, VRX.

> > Is it possible to handle this by inserting DAPM widgets for these
> > filters into the bypass paths and letting DAPM power them up? 

> I think for the Voice interface this is feasible.

I don't see any problem with either voice or HiFi interfaces
in playback path. I think that by inserting filter widgets between
Digital and Analog Playback Mixers should be enough. That way
filters get enabled in digital bypass (as widget will be after
Digital Playback Mixer) and get disabled in analog bypass (new
widgets are before Analog Playback Mixer)

> > I'm also wondering if with the new bias level management 
> > stuff which keeps the codec in BIAS_ON while any DAPM widgets
> > are powered some of the bypass mode stuff can be removed
> > or simplified?

> I have been wondering about the same, but in the HiFi path it 
> is not that simple. Most probably it is possible to add DAPM
> widget for the Audio Filter L1/R1 (which is at the moment always
> enabled), but for the L2/R2 it is not that simple.

Could you please clarify why it's not that simple?

I see more problems in capture path because TXR2, TXL2 paths are
shared in voice and tdm scenarios.

> Simplifying the twl4030_mute function should be possible at 
> this point, I think, by moving those output mutes as DAPM_PGA_E
> widgets on the paths. This is in my to-do list... Although I
> have to check that such a change should not cause any 'clicks'
> on the outputs.

-Misa

Re: [PATCH 1/3] ASoC: TWL4030: Enable voice filters for voice sidetone

[Peter Ujfalusi]

On Tuesday 23 June 2009 04:52:10 ext Lopez Cruz, Misael wrote:
> > On Friday 19 June 2009 12:58:08 ext Mark Brown wrote:
> > > On Fri, Jun 19, 2009 at 03:23:41AM -0500, Lopez Cruz, Misael wrote:
> > > > Digital voice loopback (sidetone) requires voice filters
> > > > to be enabled: VTXL, VTXR, VRX.
> > >
> > > Is it possible to handle this by inserting DAPM widgets for these
> > > filters into the bypass paths and letting DAPM power them up?
> >
> > I think for the Voice interface this is feasible.
>
> I don't see any problem with either voice or HiFi interfaces
> in playback path. I think that by inserting filter widgets between
> Digital and Analog Playback Mixers should be enough. That way
> filters get enabled in digital bypass (as widget will be after
> Digital Playback Mixer) and get disabled in analog bypass (new
> widgets are before Analog Playback Mixer)

I don't think it should be so problematic either.
We have already separated the analog and digital bypasses, so adding widgets 
for the digital filters (to the correct places) should not be that difficult..

>
> > > I'm also wondering if with the new bias level management
> > > stuff which keeps the codec in BIAS_ON while any DAPM widgets
> > > are powered some of the bypass mode stuff can be removed
> > > or simplified?
> >
> > I have been wondering about the same, but in the HiFi path it
> > is not that simple. Most probably it is possible to add DAPM
> > widget for the Audio Filter L1/R1 (which is at the moment always
> > enabled), but for the L2/R2 it is not that simple.
>
> Could you please clarify why it's not that simple?
>
> I see more problems in capture path because TXR2, TXL2 paths are
> shared in voice and tdm scenarios.

Also ARXL1 and VRX are shared on the playback case (ARXR1 is only valid for 
Option1).

So we have three bits, which operates differently in Option1 and Option2.
In theory this should not be that big of a problem, but in reality it can 
cause quite interesting cases:
The codec is in Option2.
One starts playback on the Voice interface (VRX).
Than application sets up the audio routing for HeadsetL from AudioL1.
Now if you start an audio playback (which will enable the ARXL1, even if we 
are in Option2), than you stop it: the ARXL1 (which is VRX in Option2) bit 
will be turned off... Now I'm not sure how this will be handled by the ASoC 
core.
What I mean is: One path enabled this bit, than another path also enables the 
same bit, than it disables it. Will the first path notice it? Or will it think 
that the bit is still enabled?
One thing is sure: we can not use the _same_ widget for the Voice and HiFi 
paths, they have to be different widgets.

I think that is what I meant, when I said that it is not going to be simple.

>
> > Simplifying the twl4030_mute function should be possible at
> > this point, I think, by moving those output mutes as DAPM_PGA_E
> > widgets on the paths. This is in my to-do list... Although I
> > have to check that such a change should not cause any 'clicks'
> > on the outputs.
>
> -Misa

-- 
Péter

Re: [PATCH 1/3] ASoC: TWL4030: Enable voice filters for voice sidetone

[Mark Brown]

On Tue, Jun 23, 2009 at 08:38:20AM +0300, Peter Ujfalusi wrote:

> Now if you start an audio playback (which will enable the ARXL1, even if we 
> are in Option2), than you stop it: the ARXL1 (which is VRX in Option2) bit 
> will be turned off... Now I'm not sure how this will be handled by the ASoC 
> core.
> What I mean is: One path enabled this bit, than another path also enables the 
> same bit, than it disables it. Will the first path notice it? Or will it think 
> that the bit is still enabled?

Sounds like a possible case for a supply widget, though the fact that
you've got both option 1 and option 2 is fun.  If the driver were a
proper platform driver you could select between option 1 and option 2 in
platform data (assuming that's sensible) and register different widgets
depending on which was in use.

Re: [PATCH 1/3] ASoC: TWL4030: Enable voice filters for voice sidetone

[Peter Ujfalusi]

On Tuesday 23 June 2009 13:16:16 ext Mark Brown wrote:
> On Tue, Jun 23, 2009 at 08:38:20AM +0300, Peter Ujfalusi wrote:
> > Now if you start an audio playback (which will enable the ARXL1, even if
> > we are in Option2), than you stop it: the ARXL1 (which is VRX in Option2)
> > bit will be turned off... Now I'm not sure how this will be handled by
> > the ASoC core.
> > What I mean is: One path enabled this bit, than another path also enables
> > the same bit, than it disables it. Will the first path notice it? Or will
> > it think that the bit is still enabled?
>
> Sounds like a possible case for a supply widget, though the fact that
> you've got both option 1 and option 2 is fun.
Well I use the fun word for a bit different things...
I have to check what actually the supply widget supposed to be doing and what 
is it for.

> If the driver were a
> proper platform driver you could select between option 1 and option 2 in
> platform data (assuming that's sensible) and register different widgets
> depending on which was in use.
I have been looking at this. It does not seamed that complicated. I had 
written some of the code, but than it turned out a bit more complicated:
In twl4030 series the Vibra interface also reside inside of the CODEC part.
in short: twl4030 is a MFD, than the TWL4030 codec itself also and MFD..
So in order to get this in a proper way, this should be handled somehow 
(another 'fake' MFD for the twl4030 codec/vibra perhaps).

This is still in my list, but certainly need more time to get a reasonable 
good implementation. 

-- 
Péter

Re: [PATCH 1/3] ASoC: TWL4030: Enable voice filters for voice sidetone

[Lopez Cruz, Misael]

> On Tuesday 23 June 2009 13:16:16 ext Mark Brown wrote:
> > On Tue, Jun 23, 2009 at 08:38:20AM +0300, Peter Ujfalusi wrote:
> > > Now if you start an audio playback (which will enable the 
> > > ARXL1, even if we are in Option2), than you stop it: the
> > > ARXL1 (which is VRX in Option2) bit will be turned off...
> > > Now I'm not sure how this will be handled by
> > > the ASoC core.
> > > What I mean is: One path enabled this bit, than another 
> > > path also enables the same bit, than it disables it. Will
> > > the first path notice it? Or will it think that the bit
> > > is still enabled?

> > Sounds like a possible case for a supply widget, though the 
> > fact that you've got both option 1 and option 2 is fun.

> Well I use the fun word for a bit different things...
> I have to check what actually the supply widget supposed to 
> be doing and what is it for.

> > If the driver were a proper platform driver you could select
> > between option 1 and option 2 in platform data (assuming
> > that's sensible) and register different widgets
> > depending on which was in use.

If we register widgets based on an option passed through platform
data, then we won't be able to switch between options, right?
I think switching between option1/2 during runtime is needed,
otherwise 4-channels and voice support will be mutually exclusive.

> I have been looking at this. It does not seamed that 
> complicated. I had written some of the code, but than it
> turned out a bit more complicated:
> In twl4030 series the Vibra interface also reside inside of 
> the CODEC part.
> in short: twl4030 is a MFD, than the TWL4030 codec itself 
> also and MFD..
> So in order to get this in a proper way, this should be 
> handled somehow (another 'fake' MFD for the twl4030 codec/vibra
> perhaps).

> This is still in my list, but certainly need more time to get 
> a reasonable good implementation. 

> -- 
> Péter

Re: [PATCH 1/3] ASoC: TWL4030: Enable voice filters for voice sidetone

[Mark Brown]

On Tue, Jun 23, 2009 at 02:45:02PM -0500, Lopez Cruz, Misael wrote:

> If we register widgets based on an option passed through platform
> data, then we won't be able to switch between options, right?
> I think switching between option1/2 during runtime is needed,
> otherwise 4-channels and voice support will be mutually exclusive.

Is it realistic that a platform will want to switch between those two
cases at runtime?

Re: [PATCH 1/3] ASoC: TWL4030: Enable voice filters for voice sidetone

[Peter Ujfalusi]

On Wednesday 24 June 2009 00:51:20 ext Mark Brown wrote:
> On Tue, Jun 23, 2009 at 02:45:02PM -0500, Lopez Cruz, Misael wrote:
> > If we register widgets based on an option passed through platform
> > data, then we won't be able to switch between options, right?
> > I think switching between option1/2 during runtime is needed,
> > otherwise 4-channels and voice support will be mutually exclusive.
>
> Is it realistic that a platform will want to switch between those two
> cases at runtime?

Realistic or not, I don't know. I don't know if it is feasible at the moment, 
but I can't predict the future.
I think having the ability to switch between Option1 and Option2 at runtime is 
a nice feature.

We could as well create two codec drivers (with shared core code) for Option1 
and Option2 as well, having compile time selection between the two. It would 
be even more easier and cleaner.

If we can not short out things these are backup plans for the future, but I 
would really like to see the runtime selection to be the 'master plan'

-- 
Péter