[alsa-devel] [PATCH 0/5] ASoC: tpa6130a2: pop removal (mostly) changes
Hello,
The following series improves the tpa driver to have better pop filtering capabilities. New stereo DAPM route can be used to reduce left/right power time differences. Machine drivers can opt to not use the built in DAPM routes, and use direct call to enable the stereo path.
--- Peter Ujfalusi (5): ASoC: tpa6130a2: Simplify power state management ASoC: tpa6130a2: Defer SW enable from power enable ASoC: tpa6130a2: Use one event handler for PGA_E ASoC: tpa6130a2: Add stereo DAPM path ASoC: tpa6130a2: Make DAPM registration optional, and direct interface
sound/soc/codecs/tpa6130a2.c | 77 ++++++++++++++++++++++++------------------ sound/soc/codecs/tpa6130a2.h | 3 +- 2 files changed, 46 insertions(+), 34 deletions(-)
Use simpler way to avoid setting the same power state for the amplifier. Simplifies the check introduced by patch: ASoC: tpa6130a2: Fix unbalanced regulator disables
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com Cc: Jarkko Nikula jhnikula@gmail.com --- sound/soc/codecs/tpa6130a2.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/tpa6130a2.c b/sound/soc/codecs/tpa6130a2.c index 9d61a1d..199edf0 100644 --- a/sound/soc/codecs/tpa6130a2.c +++ b/sound/soc/codecs/tpa6130a2.c @@ -41,7 +41,7 @@ struct tpa6130a2_data { unsigned char regs[TPA6130A2_CACHEREGNUM]; struct regulator *supply; int power_gpio; - unsigned char power_state; + u8 power_state:1; enum tpa_model id; };
@@ -116,7 +116,7 @@ static int tpa6130a2_initialize(void) return ret; }
-static int tpa6130a2_power(int power) +static int tpa6130a2_power(u8 power) { struct tpa6130a2_data *data; u8 val; @@ -126,8 +126,10 @@ static int tpa6130a2_power(int power) data = i2c_get_clientdata(tpa6130a2_client);
mutex_lock(&data->mutex); - if (power && !data->power_state) { + if (power == data->power_state) + goto exit;
+ if (power) { ret = regulator_enable(data->supply); if (ret != 0) { dev_err(&tpa6130a2_client->dev, @@ -154,7 +156,7 @@ static int tpa6130a2_power(int power) val = tpa6130a2_read(TPA6130A2_REG_CONTROL); val &= ~TPA6130A2_SWS; tpa6130a2_i2c_write(TPA6130A2_REG_CONTROL, val); - } else if (!power && data->power_state) { + } else { /* set SWS */ val = tpa6130a2_read(TPA6130A2_REG_CONTROL); val |= TPA6130A2_SWS;
On Tue, Nov 30, 2010 at 04:00:00PM +0200, Peter Ujfalusi wrote:
Use simpler way to avoid setting the same power state for the amplifier. Simplifies the check introduced by patch: ASoC: tpa6130a2: Fix unbalanced regulator disables
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com
Acked-by: Mark Brown broonie@opensource.wolfsonmicro.com
Do not enable the amplifier right after the power has been restored to the amplifier. The DAPM_SUPPLY widget turns on the amp early in the DAPM power walk, and the unmuting of channel happens quite late. Keeping the amp in SW reset state ensures better muting. In this way the pop noise coming from other components (codec) can be filtered out.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com --- sound/soc/codecs/tpa6130a2.c | 6 +----- 1 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/sound/soc/codecs/tpa6130a2.c b/sound/soc/codecs/tpa6130a2.c index 199edf0..42887ae 100644 --- a/sound/soc/codecs/tpa6130a2.c +++ b/sound/soc/codecs/tpa6130a2.c @@ -151,11 +151,6 @@ static int tpa6130a2_power(u8 power) data->power_state = 0; goto exit; } - - /* Clear SWS */ - val = tpa6130a2_read(TPA6130A2_REG_CONTROL); - val &= ~TPA6130A2_SWS; - tpa6130a2_i2c_write(TPA6130A2_REG_CONTROL, val); } else { /* set SWS */ val = tpa6130a2_read(TPA6130A2_REG_CONTROL); @@ -301,6 +296,7 @@ static void tpa6130a2_channel_enable(u8 channel, int enable) /* Enable amplifier */ val = tpa6130a2_read(TPA6130A2_REG_CONTROL); val |= channel; + val &= ~TPA6130A2_SWS; tpa6130a2_i2c_write(TPA6130A2_REG_CONTROL, val);
/* Unmute channel */
On Tue, Nov 30, 2010 at 04:00:01PM +0200, Peter Ujfalusi wrote:
Do not enable the amplifier right after the power has been restored to the amplifier. The DAPM_SUPPLY widget turns on the amp early in the DAPM power walk, and the unmuting of channel happens quite late. Keeping the amp in SW reset state ensures better muting. In this way the pop noise coming from other components (codec) can be filtered out.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com
Acked-by: Mark Brown broonie@opensource.wolfsonmicro.com
Reduce the amount of duplicated code by using single event handler for PGA_E to enable the needed channel. Use the w->shift to pass the channel information to the handler function.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com --- sound/soc/codecs/tpa6130a2.c | 24 +++++------------------- 1 files changed, 5 insertions(+), 19 deletions(-)
diff --git a/sound/soc/codecs/tpa6130a2.c b/sound/soc/codecs/tpa6130a2.c index 42887ae..4c77a82 100644 --- a/sound/soc/codecs/tpa6130a2.c +++ b/sound/soc/codecs/tpa6130a2.c @@ -317,29 +317,15 @@ static void tpa6130a2_channel_enable(u8 channel, int enable) } }
-static int tpa6130a2_left_event(struct snd_soc_dapm_widget *w, +static int tpa6130a2_pga_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol, int event) { switch (event) { case SND_SOC_DAPM_POST_PMU: - tpa6130a2_channel_enable(TPA6130A2_HP_EN_L, 1); + tpa6130a2_channel_enable(w->shift, 1); break; case SND_SOC_DAPM_POST_PMD: - tpa6130a2_channel_enable(TPA6130A2_HP_EN_L, 0); - break; - } - return 0; -} - -static int tpa6130a2_right_event(struct snd_soc_dapm_widget *w, - struct snd_kcontrol *kcontrol, int event) -{ - switch (event) { - case SND_SOC_DAPM_POST_PMU: - tpa6130a2_channel_enable(TPA6130A2_HP_EN_R, 1); - break; - case SND_SOC_DAPM_POST_PMD: - tpa6130a2_channel_enable(TPA6130A2_HP_EN_R, 0); + tpa6130a2_channel_enable(w->shift, 0); break; } return 0; @@ -363,10 +349,10 @@ static int tpa6130a2_supply_event(struct snd_soc_dapm_widget *w,
static const struct snd_soc_dapm_widget tpa6130a2_dapm_widgets[] = { SND_SOC_DAPM_PGA_E("TPA6130A2 Left", SND_SOC_NOPM, - 0, 0, NULL, 0, tpa6130a2_left_event, + TPA6130A2_HP_EN_L, 0, NULL, 0, tpa6130a2_pga_event, SND_SOC_DAPM_POST_PMU|SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_PGA_E("TPA6130A2 Right", SND_SOC_NOPM, - 0, 0, NULL, 0, tpa6130a2_right_event, + TPA6130A2_HP_EN_R, 0, NULL, 0, tpa6130a2_pga_event, SND_SOC_DAPM_POST_PMU|SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_SUPPLY("TPA6130A2 Enable", SND_SOC_NOPM, 0, 0, tpa6130a2_supply_event,
On Tue, Nov 30, 2010 at 04:00:02PM +0200, Peter Ujfalusi wrote:
Reduce the amount of duplicated code by using single event handler for PGA_E to enable the needed channel. Use the w->shift to pass the channel information to the handler function.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com
Acked-by: Mark Brown broonie@opensource.wolfsonmicro.com
New DAPM widgets, and paths to enable both channels at the same time (for stereo output). With this path the switch time difference can be avoided between left and right channels. The original DAPM paths can be still used, if only one of TPA's output has been connected to a speaker, but for most of the cases, switching to the stereo path is better.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com --- sound/soc/codecs/tpa6130a2.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/tpa6130a2.c b/sound/soc/codecs/tpa6130a2.c index 4c77a82..c97badf 100644 --- a/sound/soc/codecs/tpa6130a2.c +++ b/sound/soc/codecs/tpa6130a2.c @@ -354,20 +354,27 @@ static const struct snd_soc_dapm_widget tpa6130a2_dapm_widgets[] = { SND_SOC_DAPM_PGA_E("TPA6130A2 Right", SND_SOC_NOPM, TPA6130A2_HP_EN_R, 0, NULL, 0, tpa6130a2_pga_event, SND_SOC_DAPM_POST_PMU|SND_SOC_DAPM_POST_PMD), + SND_SOC_DAPM_PGA_E("TPA6130A2 Stereo", SND_SOC_NOPM, + TPA6130A2_HP_EN_L | TPA6130A2_HP_EN_R, 0, NULL, 0, + tpa6130a2_pga_event, + SND_SOC_DAPM_POST_PMU|SND_SOC_DAPM_POST_PMD), SND_SOC_DAPM_SUPPLY("TPA6130A2 Enable", SND_SOC_NOPM, 0, 0, tpa6130a2_supply_event, SND_SOC_DAPM_POST_PMU|SND_SOC_DAPM_POST_PMD), /* Outputs */ SND_SOC_DAPM_OUTPUT("TPA6130A2 Headphone Left"), SND_SOC_DAPM_OUTPUT("TPA6130A2 Headphone Right"), + SND_SOC_DAPM_OUTPUT("TPA6130A2 Headphone Stereo"), };
static const struct snd_soc_dapm_route audio_map[] = { {"TPA6130A2 Headphone Left", NULL, "TPA6130A2 Left"}, {"TPA6130A2 Headphone Right", NULL, "TPA6130A2 Right"}, + {"TPA6130A2 Headphone Stereo", NULL, "TPA6130A2 Stereo"},
{"TPA6130A2 Headphone Left", NULL, "TPA6130A2 Enable"}, {"TPA6130A2 Headphone Right", NULL, "TPA6130A2 Enable"}, + {"TPA6130A2 Headphone Stereo", NULL, "TPA6130A2 Enable"}, };
int tpa6130a2_add_controls(struct snd_soc_codec *codec)
On Tue, Nov 30, 2010 at 04:00:03PM +0200, Peter Ujfalusi wrote:
New DAPM widgets, and paths to enable both channels at the same time (for stereo output). With this path the switch time difference can be avoided between left and right channels. The original DAPM paths can be still used, if only one of TPA's output has been connected to a speaker, but for most of the cases, switching to the stereo path is better.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com
Acked-by: Mark Brown broonie@opensource.wolfsonmicro.com
Users can choose to not add the DAPM routes provided by the amp driver, but use the direct enable/disable interface from machine driver with SND_SOC_DAPM_HP's event callback. In some cases this method must be used to make the audio path pop noise free.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com --- sound/soc/codecs/tpa6130a2.c | 30 +++++++++++++++++++++++++----- sound/soc/codecs/tpa6130a2.h | 3 ++- 2 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/sound/soc/codecs/tpa6130a2.c b/sound/soc/codecs/tpa6130a2.c index c97badf..5a38372 100644 --- a/sound/soc/codecs/tpa6130a2.c +++ b/sound/soc/codecs/tpa6130a2.c @@ -377,7 +377,26 @@ static const struct snd_soc_dapm_route audio_map[] = { {"TPA6130A2 Headphone Stereo", NULL, "TPA6130A2 Enable"}, };
-int tpa6130a2_add_controls(struct snd_soc_codec *codec) +int tpa6130a2_stereo_enable(int enable) +{ + int ret = 0; + if (enable) { + ret = tpa6130a2_power(1); + if (ret < 0) + return ret; + tpa6130a2_channel_enable(TPA6130A2_HP_EN_R | TPA6130A2_HP_EN_L, + 1); + } else { + tpa6130a2_channel_enable(TPA6130A2_HP_EN_R | TPA6130A2_HP_EN_L, + 0); + ret = tpa6130a2_power(0); + } + + return ret; +} +EXPORT_SYMBOL_GPL(tpa6130a2_stereo_enable); + +int tpa6130a2_add_controls(struct snd_soc_codec *codec, int with_dapm) { struct tpa6130a2_data *data; struct snd_soc_dapm_context *dapm = &codec->dapm; @@ -387,10 +406,11 @@ int tpa6130a2_add_controls(struct snd_soc_codec *codec)
data = i2c_get_clientdata(tpa6130a2_client);
- snd_soc_dapm_new_controls(dapm, tpa6130a2_dapm_widgets, - ARRAY_SIZE(tpa6130a2_dapm_widgets)); - - snd_soc_dapm_add_routes(dapm, audio_map, ARRAY_SIZE(audio_map)); + if (with_dapm) { + snd_soc_dapm_new_controls(dapm, tpa6130a2_dapm_widgets, + ARRAY_SIZE(tpa6130a2_dapm_widgets)); + snd_soc_dapm_add_routes(dapm, audio_map, ARRAY_SIZE(audio_map)); + }
if (data->id == TPA6140A2) return snd_soc_add_controls(codec, tpa6140a2_controls, diff --git a/sound/soc/codecs/tpa6130a2.h b/sound/soc/codecs/tpa6130a2.h index 57e867f..e3150d9 100644 --- a/sound/soc/codecs/tpa6130a2.h +++ b/sound/soc/codecs/tpa6130a2.h @@ -56,6 +56,7 @@ /* TPA6130A2_REG_VERSION (0x04) */ #define TPA6130A2_VERSION_MASK (0x0f)
-extern int tpa6130a2_add_controls(struct snd_soc_codec *codec); +extern int tpa6130a2_add_controls(struct snd_soc_codec *codec, int with_dapm); +extern int tpa6130a2_stereo_enable(int enable);
#endif /* __TPA6130A2_H__ */
On Tue, Nov 30, 2010 at 04:00:04PM +0200, Peter Ujfalusi wrote:
Users can choose to not add the DAPM routes provided by the amp driver, but use the direct enable/disable interface from machine driver with SND_SOC_DAPM_HP's event callback. In some cases this method must be used to make the audio path pop noise free.
Is there any situation where it would undesirable to do this? If not it'd seem better to just make the driver do this always.
+int tpa6130a2_stereo_enable(int enable) +{
It'd be much nicer if this took a CODEC as an argument - even if the implementation doesn't actually use it yet it'd be better to have an interface which has an idea that there may be multiple instances of the device.
On Tuesday 30 November 2010 16:30:13 ext Mark Brown wrote:
On Tue, Nov 30, 2010 at 04:00:04PM +0200, Peter Ujfalusi wrote:
Users can choose to not add the DAPM routes provided by the amp driver, but use the direct enable/disable interface from machine driver with SND_SOC_DAPM_HP's event callback. In some cases this method must be used to make the audio path pop noise free.
Is there any situation where it would undesirable to do this? If not it'd seem better to just make the driver do this always.
You mean to not have DAPM widgets/routes in the tpa6130a2 driver, and only have a function, which can be used to turn on/off the amp? The original [1] (first version) of the tpa6130a2 driver only had DAPM_HP widget. It has been changed based on the comments.
+int tpa6130a2_stereo_enable(int enable) +{
It'd be much nicer if this took a CODEC as an argument - even if the implementation doesn't actually use it yet it'd be better to have an interface which has an idea that there may be multiple instances of the device.
Sure, I will do that.
[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2009-October/022034.htm...
Hi,
On Wednesday 01 December 2010 08:54:20 Ujfalusi Peter (Nokia-MS/Tampere) wrote:
On Tuesday 30 November 2010 16:30:13 ext Mark Brown wrote:
On Tue, Nov 30, 2010 at 04:00:04PM +0200, Peter Ujfalusi wrote:
Users can choose to not add the DAPM routes provided by the amp driver, but use the direct enable/disable interface from machine driver with SND_SOC_DAPM_HP's event callback. In some cases this method must be used to make the audio path pop noise free.
Is there any situation where it would undesirable to do this? If not it'd seem better to just make the driver do this always.
You mean to not have DAPM widgets/routes in the tpa6130a2 driver, and only have a function, which can be used to turn on/off the amp? The original [1] (first version) of the tpa6130a2 driver only had DAPM_HP widget. It has been changed based on the comments.
I think the simplest thing would be to actually remove all DAPM widgets, routings from this driver, and provide only the funtion, which enable the amp in stereo mode. As it is now, we do not have user for this amp in upstream, so I'm not that worried about braking machine drivers ;) The problem with tpa6130a2 provided DAPM_HP is that at least in one machine, I have to introduce delay before I enable the amp, otherwise I have really bad pop at stream start. The codec in this case alone does not produce that bad pop, but it seams the sequence, and timing in the whole system makes the pop bad. On other setups I have not encountered such a problem.
So: I would remove all dapm related things from the tpa6130a2 driver, and offer only the: int tpa6130a2_stereo_enable(snd_soc_codec, *codec, int enable); Function for users (machine drivers).
If there's a need I can re-introduce tpa6130a2 provided DAPM_HP widget to reduce duplicated code in machine drivers (when we have enough users for it).
What do you think?
On Wed, Dec 01, 2010 at 10:07:57AM +0200, Peter Ujfalusi wrote:
On Wednesday 01 December 2010 08:54:20 Ujfalusi Peter (Nokia-MS/Tampere) wrote:
On Tuesday 30 November 2010 16:30:13 ext Mark Brown wrote:
Is there any situation where it would undesirable to do this? If not it'd seem better to just make the driver do this always.
You mean to not have DAPM widgets/routes in the tpa6130a2 driver, and only have a function, which can be used to turn on/off the amp? The original [1] (first version) of the tpa6130a2 driver only had DAPM_HP widget. It has been changed based on the comments.
You didn't mention any pop/click issues in the original patch or the discussion following it as far as I remember - in your original patch there was nothing motivating doing this
If there's a need I can re-introduce tpa6130a2 provided DAPM_HP widget to reduce duplicated code in machine drivers (when we have enough users for it).
What do you think?
I think this is a better approach since it solves an actual problem. We may want to consider introducing a new widget type for edge PGAs, though I can see that getting overused.
On Wednesday 01 December 2010 13:44:28 ext Mark Brown wrote:
You didn't mention any pop/click issues in the original patch or the discussion following it as far as I remember - in your original patch there was nothing motivating doing this
Correct. At that time I did not noticed this problem, and with that HW setup it was OK. With different components however the ordering is a problem. I only had chance/time to look for pop noise recently...
I think this is a better approach since it solves an actual problem. We may want to consider introducing a new widget type for edge PGAs, though I can see that getting overused.
Thanks, I'll send a patch to remove the DAPM things from the tpa driver. It's a shame, sicne the DAPM things inside of the driver looks quite nice after the series ;)
On Tue, 2010-11-30 at 15:59 +0200, Peter Ujfalusi wrote:
Hello,
The following series improves the tpa driver to have better pop filtering capabilities. New stereo DAPM route can be used to reduce left/right power time differences. Machine drivers can opt to not use the built in DAPM routes, and use direct call to enable the stereo path.
Peter Ujfalusi (5): ASoC: tpa6130a2: Simplify power state management ASoC: tpa6130a2: Defer SW enable from power enable ASoC: tpa6130a2: Use one event handler for PGA_E ASoC: tpa6130a2: Add stereo DAPM path ASoC: tpa6130a2: Make DAPM registration optional, and direct interface
sound/soc/codecs/tpa6130a2.c | 77 ++++++++++++++++++++++++------------------ sound/soc/codecs/tpa6130a2.h | 3 +- 2 files changed, 46 insertions(+), 34 deletions(-)
1 - 4 Applied.
Thanks
Liam
participants (3)
-
Liam Girdwood -
Mark Brown -
Peter Ujfalusi