[alsa-devel] [PATCH 0/3] ASoC: tlv320aic3x: Pop noise reductions
Hi,
When we don't have any active component between the output of the codec and the speaker/jack the codec emmits pop noise due to several factors: - RESET register is toggled by regcache_sync (patch 1) - After regcache_sync we need to wait a bit to allow the codec to settle down: the codec will be soft reseted when it is powered down, so when we power it on and run regcache_sync, we need to wait for the flushed settings to be valid. (patch 2) - The HP ouptut power-down mode can be changed to be driven weakly to VCM, this can reduce the pop noise as well, with slight increase in powere consumption.
These factors were not taken care as in n810 and n900 we have external active amplifiers and they will remove any artifacts coming from the codec, but on boards w/o active external components they can reach the output.
Regards, Peter --- Peter Ujfalusi (3): ASoC: tlv320aic3x: Mark the RESET register as volatile ASoC: tlv320aic3x: Add delay after power on and register sync ASoC: tlv320aic3x: Add controls for selecting HP power down modes
sound/soc/codecs/tlv320aic3x.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
The RESET register only have one self clearing bit and it should not be cached. If it is cached, when we sync the registers back to the chip we will initiate a software reset as well, which is not desirable.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/codecs/tlv320aic3x.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c index 216f74084c6a..7b6924e19021 100644 --- a/sound/soc/codecs/tlv320aic3x.c +++ b/sound/soc/codecs/tlv320aic3x.c @@ -126,6 +126,16 @@ static const struct reg_default aic3x_reg[] = { { 108, 0x00 }, { 109, 0x00 }, };
+static bool aic3x_volatile_reg(struct device *dev, unsigned int reg) +{ + switch (reg) { + case AIC3X_RESET: + return true; + default: + return false; + } +} + static const struct regmap_config aic3x_regmap = { .reg_bits = 8, .val_bits = 8, @@ -133,6 +143,9 @@ static const struct regmap_config aic3x_regmap = { .max_register = DAC_ICC_ADJ, .reg_defaults = aic3x_reg, .num_reg_defaults = ARRAY_SIZE(aic3x_reg), + + .volatile_reg = aic3x_volatile_reg, + .cache_type = REGCACHE_RBTREE, };
Hi
On 12/23/2016 11:21 AM, Peter Ujfalusi wrote:
The RESET register only have one self clearing bit and it should not be cached. If it is cached, when we sync the registers back to the chip we will initiate a software reset as well, which is not desirable.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com
sound/soc/codecs/tlv320aic3x.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c index 216f74084c6a..7b6924e19021 100644 --- a/sound/soc/codecs/tlv320aic3x.c +++ b/sound/soc/codecs/tlv320aic3x.c @@ -126,6 +126,16 @@ static const struct reg_default aic3x_reg[] = { { 108, 0x00 }, { 109, 0x00 }, };
+static bool aic3x_volatile_reg(struct device *dev, unsigned int reg) +{
- switch (reg) {
- case AIC3X_RESET:
return true;- default:
return false;- }
+}
You mentioned offline you tracked this into my commit 9fb352b18b11 ("ASoC: tlv320aic3x: Do soft reset to codec when going to bias off state") but was it by bisecting or by debugging? I think I tried to cover it in a commit before 508b76864c18 ("ASoC: tlv320aic3x: Don't sync first two registers from register cache").
If you found it by debugging can it be that pop noise came because of 2a6fedec195b ("ASoC: tlv320aic3x: Convert to direct regmap API usage")?
Just thinking if there's a need to have this into stable.
Reviewed-by: Jarkko Nikula jarkko.nikula@bitmer.com
Jarkko,
On 12/23/2016 08:27 PM, Jarkko Nikula wrote:
Hi
On 12/23/2016 11:21 AM, Peter Ujfalusi wrote:
The RESET register only have one self clearing bit and it should not be cached. If it is cached, when we sync the registers back to the chip we will initiate a software reset as well, which is not desirable.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com
sound/soc/codecs/tlv320aic3x.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c index 216f74084c6a..7b6924e19021 100644 --- a/sound/soc/codecs/tlv320aic3x.c +++ b/sound/soc/codecs/tlv320aic3x.c @@ -126,6 +126,16 @@ static const struct reg_default aic3x_reg[] = { { 108, 0x00 }, { 109, 0x00 }, };
+static bool aic3x_volatile_reg(struct device *dev, unsigned int reg) +{
- switch (reg) {
- case AIC3X_RESET:
return true;- default:
return false;- }
+}
You mentioned offline you tracked this into my commit 9fb352b18b11 ("ASoC: tlv320aic3x: Do soft reset to codec when going to bias off state") but was it by bisecting or by debugging? I think I tried to cover it in a commit before 508b76864c18 ("ASoC: tlv320aic3x: Don't sync first two registers from register cache").
If you found it by debugging can it be that pop noise came because of 2a6fedec195b ("ASoC: tlv320aic3x: Convert to direct regmap API usage")?
Noticed this when debugging. The conversion to regmap did introduced this.
Just thinking if there's a need to have this into stable.
Right, it might be a good idea.
Reviewed-by: Jarkko Nikula jarkko.nikula@bitmer.com
The patch
ASoC: tlv320aic3x: Mark the RESET register as volatile
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 63c3194b82530bd71fd49db84eb7ab656b8d404a Mon Sep 17 00:00:00 2001
From: Peter Ujfalusi peter.ujfalusi@ti.com Date: Fri, 23 Dec 2016 11:21:10 +0200 Subject: [PATCH] ASoC: tlv320aic3x: Mark the RESET register as volatile
The RESET register only have one self clearing bit and it should not be cached. If it is cached, when we sync the registers back to the chip we will initiate a software reset as well, which is not desirable.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com Reviewed-by: Jarkko Nikula jarkko.nikula@bitmer.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/codecs/tlv320aic3x.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c index 8877b74b0510..bb94d50052d7 100644 --- a/sound/soc/codecs/tlv320aic3x.c +++ b/sound/soc/codecs/tlv320aic3x.c @@ -126,6 +126,16 @@ static const struct reg_default aic3x_reg[] = { { 108, 0x00 }, { 109, 0x00 }, };
+static bool aic3x_volatile_reg(struct device *dev, unsigned int reg) +{ + switch (reg) { + case AIC3X_RESET: + return true; + default: + return false; + } +} + static const struct regmap_config aic3x_regmap = { .reg_bits = 8, .val_bits = 8, @@ -133,6 +143,9 @@ static const struct regmap_config aic3x_regmap = { .max_register = DAC_ICC_ADJ, .reg_defaults = aic3x_reg, .num_reg_defaults = ARRAY_SIZE(aic3x_reg), + + .volatile_reg = aic3x_volatile_reg, + .cache_type = REGCACHE_RBTREE, };
When the codec is powered on, it's registers are in reset state as the power off will do a soft reset of the codec.
After the register sync we need to add delay to remove the pop-noise on stream start.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/codecs/tlv320aic3x.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c index 7b6924e19021..bc220f83dd1f 100644 --- a/sound/soc/codecs/tlv320aic3x.c +++ b/sound/soc/codecs/tlv320aic3x.c @@ -1393,6 +1393,12 @@ static int aic3x_set_power(struct snd_soc_codec *codec, int power) snd_soc_write(codec, AIC3X_PLL_PROGC_REG, pll_c); snd_soc_write(codec, AIC3X_PLL_PROGD_REG, pll_d); } + + /* + * Delay is needed to reduce pop-noise after syncing back the + * registers + */ + mdelay(50); } else { /* * Do soft reset to this codec instance in order to clear
The patch
ASoC: tlv320aic3x: Add delay after power on and register sync
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 03303da5243f394f5cc5e530a8bc9f26ad2c79cb Mon Sep 17 00:00:00 2001
From: Peter Ujfalusi peter.ujfalusi@ti.com Date: Fri, 23 Dec 2016 11:21:11 +0200 Subject: [PATCH] ASoC: tlv320aic3x: Add delay after power on and register sync
When the codec is powered on, it's registers are in reset state as the power off will do a soft reset of the codec.
After the register sync we need to add delay to remove the pop-noise on stream start.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/codecs/tlv320aic3x.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c index bb94d50052d7..29bf8c81ae02 100644 --- a/sound/soc/codecs/tlv320aic3x.c +++ b/sound/soc/codecs/tlv320aic3x.c @@ -1393,6 +1393,12 @@ static int aic3x_set_power(struct snd_soc_codec *codec, int power) snd_soc_write(codec, AIC3X_PLL_PROGC_REG, pll_c); snd_soc_write(codec, AIC3X_PLL_PROGD_REG, pll_d); } + + /* + * Delay is needed to reduce pop-noise after syncing back the + * registers + */ + mdelay(50); } else { /* * Do soft reset to this codec instance in order to clear
When the HP drivers are powered down, their output can be placed in high-impedance state or it can be weakly driven to the VCM level. High-impedance mode provides better power saving in the expense of pop noise when powering up the HP. When the output is driven to VCM level the artifacts can be minimized, but the power down power consumption will be slightly higher.
The patch adds way to select the HPL/R power-down mode.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/codecs/tlv320aic3x.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c index bc220f83dd1f..3e66dedeafac 100644 --- a/sound/soc/codecs/tlv320aic3x.c +++ b/sound/soc/codecs/tlv320aic3x.c @@ -305,6 +305,13 @@ static const char * const aic3x_rampup_step[] = { "0ms", "1ms", "2ms", "4ms" }; static SOC_ENUM_SINGLE_DECL(aic3x_rampup_step_enum, HPOUT_POP_REDUCTION, 2, aic3x_rampup_step);
+static const char * const aic3x_output_pdown_mode[] = { + "Drive to a common-mode", "High-impedance mode"}; +static SOC_ENUM_SINGLE_DECL(aic3x_hpl_pdown_mode_enum, HPLOUT_CTRL, 2, + aic3x_output_pdown_mode); +static SOC_ENUM_SINGLE_DECL(aic3x_hpr_pdown_mode_enum, HPROUT_CTRL, 2, + aic3x_output_pdown_mode); + /* * DAC digital volumes. From -63.5 to 0 dB in 0.5 dB steps */ @@ -392,6 +399,9 @@ static const struct snd_kcontrol_new aic3x_snd_controls[] = { SOC_DOUBLE_R("HPCOM Playback Switch", HPLCOM_CTRL, HPRCOM_CTRL, 3, 0x01, 0),
+ SOC_ENUM("Left HP Power Down mode", aic3x_hpl_pdown_mode_enum), + SOC_ENUM("Right HP Power Down mode", aic3x_hpr_pdown_mode_enum), + /* * Note: enable Automatic input Gain Controller with care. It can * adjust PGA to max value when ADC is on and will never go back.
On 12/23/2016 11:21 AM, Peter Ujfalusi wrote:
When the HP drivers are powered down, their output can be placed in high-impedance state or it can be weakly driven to the VCM level. High-impedance mode provides better power saving in the expense of pop noise when powering up the HP. When the output is driven to VCM level the artifacts can be minimized, but the power down power consumption will be slightly higher.
The patch adds way to select the HPL/R power-down mode.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com
sound/soc/codecs/tlv320aic3x.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
This certainly gives more possibilities to control this depending on use-case and device idleness. However I think does it find use or will it end up being a random choice?
Alternative could be some high-impedance widget that defines the output mode and what a machine driver wanting lower power consumption can connect instead of HP outputs. Another idea that comes to my mind do you hear the pop if output is let to be in high-impedance but is driven to VCM with some delay before powering up?
On Fri, Dec 23, 2016 at 08:47:07PM +0200, Jarkko Nikula wrote:
This certainly gives more possibilities to control this depending on use-case and device idleness. However I think does it find use or will it end up being a random choice?
I suspect most people won't understand it but OTOH I can see some people wanting to use it so...
Alternative could be some high-impedance widget that defines the output mode and what a machine driver wanting lower power consumption can connect instead of HP outputs. Another idea that comes to my mind do you hear the pop if output is let to be in high-impedance but is driven to VCM with some delay before powering up?
I can also see a system tying it to jack detection so you get the pop reduction if the headphones are physically connected and low power if they're not there and nothing is going to care anyway.
participants (3)
-
Jarkko Nikula -
Mark Brown -
Peter Ujfalusi