[alsa-devel] [PATCH 1/3] ASoC: TWL4030: Enable voice filters for voice sidetone
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) {
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.
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.
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
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
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.
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.
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
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?
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'
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);elsetwl4030->bypass_state &= ~(1 << 5);@@ -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) {
participants (3)
-
Lopez Cruz, Misael -
Mark Brown -
Peter Ujfalusi