[PATCH 0/3] ASoC: tlv320aic3x: Pop noise reductions

[PATCH 0/3] ASoC: tlv320aic3x: Pop noise reductions

[Peter Ujfalusi]

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(+)

-- 
2.11.0

[PATCH 1/3] ASoC: tlv320aic3x: Mark the RESET register as volatile

[Peter Ujfalusi]

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,
 };
 
-- 
2.11.0

[PATCH 2/3] ASoC: tlv320aic3x: Add delay after power on and register sync

[Peter Ujfalusi]

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
-- 
2.11.0

[PATCH 3/3] ASoC: tlv320aic3x: Add controls for selecting HP power down modes

[Peter Ujfalusi]

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.
-- 
2.11.0

Re: [PATCH 1/3] ASoC: tlv320aic3x: Mark the RESET register as volatile

[Jarkko Nikula]

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>

Re: [PATCH 3/3] ASoC: tlv320aic3x: Add controls for selecting HP power down modes

[Jarkko Nikula]

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?

-- 
Jarkko

Re: [PATCH 3/3] ASoC: tlv320aic3x: Add controls for selecting HP power down modes

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 871 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Applied "ASoC: tlv320aic3x: Add delay after power on and register sync" to the asoc tree

[Mark Brown]

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
-- 
2.11.0

Applied "ASoC: tlv320aic3x: Mark the RESET register as volatile" to the asoc tree

[Mark Brown]

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,
 };
 
-- 
2.11.0

Re: [PATCH 1/3] ASoC: tlv320aic3x: Mark the RESET register as volatile

[Peter Ujfalusi]

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>
> 

-- 
Péter