[PATCH] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[PATCH] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Kai-Heng Feng]

HDA mode does not have these issues because necessary workarounds in
linux/sound/pci/hda/patch_realtek.c are already applied. So we can
leverage these workaournds into rt286.

When jack is plugged, it rapidly generates I2C interrupts, which
triggers rt286_irq() and rt286_jack_detect(), which causes the noise.
alc_fixup_dell_xps13() in patch_realtek.c sets up a pin that can stop
the frantic interrupts, hence fixes the click noise.

When rt286 is under powersaving state, play a sound with headphone or
plug a headphone in will produce a loud crack sound.
alc_shutup_dell_xps13() patch_realtek.c mutes the headphone at plugging.
Apply the same workaround to LDO2 power event can make the loud crack
sound to a more tolerable pop sound. I found that unconditionally apply
the workaround to all related power event can help a little further.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=112611
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1313434
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 sound/soc/codecs/rt286.c | 67 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 63 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/rt286.c b/sound/soc/codecs/rt286.c
index 74c0e4e..d041937 100644
--- a/sound/soc/codecs/rt286.c
+++ b/sound/soc/codecs/rt286.c
@@ -36,6 +36,8 @@
 #define RT286_VENDOR_ID 0x10ec0286
 #define RT288_VENDOR_ID 0x10ec0288
 
+#define AMP_OUT_MUTE 0xb080
+
 struct rt286_priv {
 	struct reg_default *index_cache;
 	int index_cache_size;
@@ -47,6 +49,7 @@ struct rt286_priv {
 	struct delayed_work jack_detect_work;
 	int sys_clk;
 	int clk_id;
+	void (*mute_hpo_func)(struct snd_soc_codec *codec);
 };
 
 static const struct reg_default rt286_index_def[] = {
@@ -472,10 +475,51 @@ static int rt286_set_dmic1_event(struct snd_soc_dapm_widget *w,
 	return 0;
 }
 
+/* Add HV, VREF and LDO1 event functions to workaround headphone crack noise */
+static int rt286_hv_event(struct snd_soc_dapm_widget *w,
+			     struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
+	struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
+
+	if (rt286->mute_hpo_func)
+		rt286->mute_hpo_func(codec);
+
+	return 0;
+}
+
+static int rt286_vref_event(struct snd_soc_dapm_widget *w,
+			     struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
+	struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
+
+	if (rt286->mute_hpo_func)
+		rt286->mute_hpo_func(codec);
+
+	return 0;
+}
+
+static int rt286_ldo1_event(struct snd_soc_dapm_widget *w,
+			     struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
+	struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
+
+	if (rt286->mute_hpo_func)
+		rt286->mute_hpo_func(codec);
+
+	return 0;
+}
+
 static int rt286_ldo2_event(struct snd_soc_dapm_widget *w,
 			     struct snd_kcontrol *kcontrol, int event)
 {
 	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
+	struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
+
+	if (rt286->mute_hpo_func)
+		rt286->mute_hpo_func(codec);
 
 	switch (event) {
 	case SND_SOC_DAPM_POST_PMU:
@@ -516,16 +560,24 @@ static int rt286_mic1_event(struct snd_soc_dapm_widget *w,
 	return 0;
 }
 
+static void dell_dino_mute_hpo(struct snd_soc_codec *codec)
+{
+	snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE);
+}
+
 static const struct snd_soc_dapm_widget rt286_dapm_widgets[] = {
 	SND_SOC_DAPM_SUPPLY_S("HV", 1, RT286_POWER_CTRL1,
-		12, 1, NULL, 0),
+		12, 1, rt286_hv_event, SND_SOC_DAPM_PRE_PMD |
+		SND_SOC_DAPM_PRE_PMU),
 	SND_SOC_DAPM_SUPPLY("VREF", RT286_POWER_CTRL1,
-		0, 1, NULL, 0),
+		0, 1, rt286_vref_event, SND_SOC_DAPM_PRE_PMD |
+		SND_SOC_DAPM_PRE_PMU),
 	SND_SOC_DAPM_SUPPLY_S("LDO1", 1, RT286_POWER_CTRL2,
-		2, 0, NULL, 0),
+		2, 0, rt286_ldo1_event, SND_SOC_DAPM_PRE_PMD |
+		SND_SOC_DAPM_PRE_PMU),
 	SND_SOC_DAPM_SUPPLY_S("LDO2", 2, RT286_POWER_CTRL1,
 		13, 1, rt286_ldo2_event, SND_SOC_DAPM_PRE_PMD |
-		SND_SOC_DAPM_POST_PMU),
+		SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU),
 	SND_SOC_DAPM_SUPPLY("MCLK MODE", RT286_PLL_CTRL1,
 		5, 0, NULL, 0),
 	SND_SOC_DAPM_SUPPLY("MIC1 Input Buffer", SND_SOC_NOPM,
@@ -1190,6 +1242,11 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
 		regmap_update_bits(rt286->regmap,
 					RT286_CBJ_CTRL1, 0xf000, 0xb000);
 	} else {
+		/* Fix headphone click noise */
+		if (dmi_check_system(dmi_dell_dino))
+			regmap_write(rt286->regmap,
+					RT286_MIC1_DET_CTRL, 0x0020);
+
 		regmap_update_bits(rt286->regmap,
 					RT286_CBJ_CTRL1, 0xf000, 0x5000);
 	}
@@ -1204,6 +1261,7 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
 	mdelay(10);
 
 	regmap_write(rt286->regmap, RT286_MISC_CTRL1, 0x0000);
+
 	/* Power down LDO, VREF */
 	regmap_update_bits(rt286->regmap, RT286_POWER_CTRL2, 0xc, 0x0);
 	regmap_update_bits(rt286->regmap, RT286_POWER_CTRL1, 0x1001, 0x1001);
@@ -1222,6 +1280,7 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
 			RT286_SET_GPIO_DATA, 0x40, 0x40);
 		regmap_update_bits(rt286->regmap,
 			RT286_GPIO_CTRL, 0xc, 0x8);
+		rt286->mute_hpo_func = dell_dino_mute_hpo;
 	}
 
 	if (rt286->i2c->irq) {
-- 
2.10.0

Re: [PATCH] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Kai-Heng Feng]

On Mon, Jan 23, 2017 at 1:35 PM Kai-Heng Feng <kai.heng.feng@canonical.com>
wrote:

> HDA mode does not have these issues because necessary workarounds in
> linux/sound/pci/hda/patch_realtek.c are already applied. So we can
> leverage these workaournds into rt286.
>
> When jack is plugged, it rapidly generates I2C interrupts, which
> triggers rt286_irq() and rt286_jack_detect(), which causes the noise.
> alc_fixup_dell_xps13() in patch_realtek.c sets up a pin that can stop
> the frantic interrupts, hence fixes the click noise.
>
> When rt286 is under powersaving state, play a sound with headphone or
> plug a headphone in will produce a loud crack sound.
> alc_shutup_dell_xps13() patch_realtek.c mutes the headphone at plugging.
> Apply the same workaround to LDO2 power event can make the loud crack
> sound to a more tolerable pop sound. I found that unconditionally apply
> the workaround to all related power event can help a little further.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=112611
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1313434
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>

Ping.
Can anyone help reviewing this?


> ---
>  sound/soc/codecs/rt286.c | 67
> +++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 63 insertions(+), 4 deletions(-)
>
> diff --git a/sound/soc/codecs/rt286.c b/sound/soc/codecs/rt286.c
> index 74c0e4e..d041937 100644
> --- a/sound/soc/codecs/rt286.c
> +++ b/sound/soc/codecs/rt286.c
> @@ -36,6 +36,8 @@
>  #define RT286_VENDOR_ID 0x10ec0286
>  #define RT288_VENDOR_ID 0x10ec0288
>
> +#define AMP_OUT_MUTE 0xb080
> +
>  struct rt286_priv {
>         struct reg_default *index_cache;
>         int index_cache_size;
> @@ -47,6 +49,7 @@ struct rt286_priv {
>         struct delayed_work jack_detect_work;
>         int sys_clk;
>         int clk_id;
> +       void (*mute_hpo_func)(struct snd_soc_codec *codec);
>  };
>
>  static const struct reg_default rt286_index_def[] = {
> @@ -472,10 +475,51 @@ static int rt286_set_dmic1_event(struct
> snd_soc_dapm_widget *w,
>         return 0;
>  }
>
> +/* Add HV, VREF and LDO1 event functions to workaround headphone crack
> noise */
> +static int rt286_hv_event(struct snd_soc_dapm_widget *w,
> +                            struct snd_kcontrol *kcontrol, int event)
> +{
> +       struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> +       struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
> +
> +       if (rt286->mute_hpo_func)
> +               rt286->mute_hpo_func(codec);
> +
> +       return 0;
> +}
> +
> +static int rt286_vref_event(struct snd_soc_dapm_widget *w,
> +                            struct snd_kcontrol *kcontrol, int event)
> +{
> +       struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> +       struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
> +
> +       if (rt286->mute_hpo_func)
> +               rt286->mute_hpo_func(codec);
> +
> +       return 0;
> +}
> +
> +static int rt286_ldo1_event(struct snd_soc_dapm_widget *w,
> +                            struct snd_kcontrol *kcontrol, int event)
> +{
> +       struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> +       struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
> +
> +       if (rt286->mute_hpo_func)
> +               rt286->mute_hpo_func(codec);
> +
> +       return 0;
> +}
> +
>  static int rt286_ldo2_event(struct snd_soc_dapm_widget *w,
>                              struct snd_kcontrol *kcontrol, int event)
>  {
>         struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> +       struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
> +
> +       if (rt286->mute_hpo_func)
> +               rt286->mute_hpo_func(codec);
>
>         switch (event) {
>         case SND_SOC_DAPM_POST_PMU:
> @@ -516,16 +560,24 @@ static int rt286_mic1_event(struct
> snd_soc_dapm_widget *w,
>         return 0;
>  }
>
> +static void dell_dino_mute_hpo(struct snd_soc_codec *codec)
> +{
> +       snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE);
> +}
> +
>  static const struct snd_soc_dapm_widget rt286_dapm_widgets[] = {
>         SND_SOC_DAPM_SUPPLY_S("HV", 1, RT286_POWER_CTRL1,
> -               12, 1, NULL, 0),
> +               12, 1, rt286_hv_event, SND_SOC_DAPM_PRE_PMD |
> +               SND_SOC_DAPM_PRE_PMU),
>         SND_SOC_DAPM_SUPPLY("VREF", RT286_POWER_CTRL1,
> -               0, 1, NULL, 0),
> +               0, 1, rt286_vref_event, SND_SOC_DAPM_PRE_PMD |
> +               SND_SOC_DAPM_PRE_PMU),
>         SND_SOC_DAPM_SUPPLY_S("LDO1", 1, RT286_POWER_CTRL2,
> -               2, 0, NULL, 0),
> +               2, 0, rt286_ldo1_event, SND_SOC_DAPM_PRE_PMD |
> +               SND_SOC_DAPM_PRE_PMU),
>         SND_SOC_DAPM_SUPPLY_S("LDO2", 2, RT286_POWER_CTRL1,
>                 13, 1, rt286_ldo2_event, SND_SOC_DAPM_PRE_PMD |
> -               SND_SOC_DAPM_POST_PMU),
> +               SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU),
>         SND_SOC_DAPM_SUPPLY("MCLK MODE", RT286_PLL_CTRL1,
>                 5, 0, NULL, 0),
>         SND_SOC_DAPM_SUPPLY("MIC1 Input Buffer", SND_SOC_NOPM,
> @@ -1190,6 +1242,11 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
>                 regmap_update_bits(rt286->regmap,
>                                         RT286_CBJ_CTRL1, 0xf000, 0xb000);
>         } else {
> +               /* Fix headphone click noise */
> +               if (dmi_check_system(dmi_dell_dino))
> +                       regmap_write(rt286->regmap,
> +                                       RT286_MIC1_DET_CTRL, 0x0020);
> +
>                 regmap_update_bits(rt286->regmap,
>                                         RT286_CBJ_CTRL1, 0xf000, 0x5000);
>         }
> @@ -1204,6 +1261,7 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
>         mdelay(10);
>
>         regmap_write(rt286->regmap, RT286_MISC_CTRL1, 0x0000);
> +
>         /* Power down LDO, VREF */
>         regmap_update_bits(rt286->regmap, RT286_POWER_CTRL2, 0xc, 0x0);
>         regmap_update_bits(rt286->regmap, RT286_POWER_CTRL1, 0x1001,
> 0x1001);
> @@ -1222,6 +1280,7 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
>                         RT286_SET_GPIO_DATA, 0x40, 0x40);
>                 regmap_update_bits(rt286->regmap,
>                         RT286_GPIO_CTRL, 0xc, 0x8);
> +               rt286->mute_hpo_func = dell_dino_mute_hpo;
>         }
>
>         if (rt286->i2c->irq) {
> --
> 2.10.0
>
>

Re: [PATCH] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Bard Liao]

> -----Original Message-----
> From: Kai-Heng Feng [mailto:kai.heng.feng@canonical.com]
> Sent: Monday, January 23, 2017 1:35 PM
> To: Bard Liao
> Cc: Oder Chiou; lgirdwood@gmail.com; broonie@kernel.org;
> alsa-devel@alsa-project.org; Kai-Heng Feng
> Subject: [PATCH] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343
> I2S mode
> 
> HDA mode does not have these issues because necessary workarounds in
> linux/sound/pci/hda/patch_realtek.c are already applied. So we can
> leverage these workaournds into rt286.
> 
> When jack is plugged, it rapidly generates I2C interrupts, which
> triggers rt286_irq() and rt286_jack_detect(), which causes the noise.
> alc_fixup_dell_xps13() in patch_realtek.c sets up a pin that can stop
> the frantic interrupts, hence fixes the click noise.
> 
> When rt286 is under powersaving state, play a sound with headphone or
> plug a headphone in will produce a loud crack sound.
> alc_shutup_dell_xps13() patch_realtek.c mutes the headphone at plugging.
> Apply the same workaround to LDO2 power event can make the loud crack
> sound to a more tolerable pop sound. I found that unconditionally apply
> the workaround to all related power event can help a little further.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=112611
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1313434
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  sound/soc/codecs/rt286.c | 67
> +++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 63 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/codecs/rt286.c b/sound/soc/codecs/rt286.c
> index 74c0e4e..d041937 100644
> --- a/sound/soc/codecs/rt286.c
> +++ b/sound/soc/codecs/rt286.c
> @@ -36,6 +36,8 @@
>  #define RT286_VENDOR_ID 0x10ec0286
>  #define RT288_VENDOR_ID 0x10ec0288
> 
> +#define AMP_OUT_MUTE 0xb080
> +
>  struct rt286_priv {
>  	struct reg_default *index_cache;
>  	int index_cache_size;
> @@ -47,6 +49,7 @@ struct rt286_priv {
>  	struct delayed_work jack_detect_work;
>  	int sys_clk;
>  	int clk_id;
> +	void (*mute_hpo_func)(struct snd_soc_codec *codec);
>  };
> 
>  static const struct reg_default rt286_index_def[] = {
> @@ -472,10 +475,51 @@ static int rt286_set_dmic1_event(struct
> snd_soc_dapm_widget *w,
>  	return 0;
>  }
> 
> +/* Add HV, VREF and LDO1 event functions to workaround headphone crack
> noise */
> +static int rt286_hv_event(struct snd_soc_dapm_widget *w,
> +			     struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> +	struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
> +
> +	if (rt286->mute_hpo_func)
> +		rt286->mute_hpo_func(codec);
> +
> +	return 0;
> +}
> +
> +static int rt286_vref_event(struct snd_soc_dapm_widget *w,
> +			     struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> +	struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
> +
> +	if (rt286->mute_hpo_func)
> +		rt286->mute_hpo_func(codec);
> +
> +	return 0;
> +}
> +
> +static int rt286_ldo1_event(struct snd_soc_dapm_widget *w,
> +			     struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> +	struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
> +
> +	if (rt286->mute_hpo_func)
> +		rt286->mute_hpo_func(codec);
> +
> +	return 0;
> +}
> +
>  static int rt286_ldo2_event(struct snd_soc_dapm_widget *w,
>  			     struct snd_kcontrol *kcontrol, int event)
>  {
>  	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> +	struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
> +
> +	if (rt286->mute_hpo_func)
> +		rt286->mute_hpo_func(codec);
> 
>  	switch (event) {
>  	case SND_SOC_DAPM_POST_PMU:
> @@ -516,16 +560,24 @@ static int rt286_mic1_event(struct
> snd_soc_dapm_widget *w,
>  	return 0;
>  }
> 
> +static void dell_dino_mute_hpo(struct snd_soc_codec *codec)
> +{
> +	snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE);
> +}
> +

I didn't see where will headphone be unmute. There is already a
headphone mute control on the driver. See
	SND_SOC_DAPM_SWITCH("HPO L", SND_SOC_NOPM, 0, 0,
		&hpol_enable_control),
	SND_SOC_DAPM_SWITCH("HPO R", SND_SOC_NOPM, 0, 0,
		&hpor_enable_control),


>  	SND_SOC_DAPM_SUPPLY_S("LDO2", 2, RT286_POWER_CTRL1,
>  		13, 1, rt286_ldo2_event, SND_SOC_DAPM_PRE_PMD |
> -		SND_SOC_DAPM_POST_PMU),
> +		SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU),

Is POST_PMU really what you want?

> @@ -1190,6 +1242,11 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
>  		regmap_update_bits(rt286->regmap,
>  					RT286_CBJ_CTRL1, 0xf000, 0xb000);
>  	} else {
> +		/* Fix headphone click noise */
> +		if (dmi_check_system(dmi_dell_dino))
> +			regmap_write(rt286->regmap,
> +					RT286_MIC1_DET_CTRL, 0x0020);
> +

I need to figure out how 0x0020 works.

Re: [PATCH] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Kai-Heng Feng]

On Thu, Feb 16, 2017 at 2:53 PM, Bard Liao <bardliao@realtek.com> wrote:
>> -----Original Message-----
>> From: Kai-Heng Feng [mailto:kai.heng.feng@canonical.com]
>> Sent: Monday, January 23, 2017 1:35 PM
>> To: Bard Liao
>> Cc: Oder Chiou; lgirdwood@gmail.com; broonie@kernel.org;
>> alsa-devel@alsa-project.org; Kai-Heng Feng
>> Subject: [PATCH] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343
>> I2S mode
>>
>> HDA mode does not have these issues because necessary workarounds in
>> linux/sound/pci/hda/patch_realtek.c are already applied. So we can
>> leverage these workaournds into rt286.
>>
>> When jack is plugged, it rapidly generates I2C interrupts, which
>> triggers rt286_irq() and rt286_jack_detect(), which causes the noise.
>> alc_fixup_dell_xps13() in patch_realtek.c sets up a pin that can stop
>> the frantic interrupts, hence fixes the click noise.
>>
>> When rt286 is under powersaving state, play a sound with headphone or
>> plug a headphone in will produce a loud crack sound.
>> alc_shutup_dell_xps13() patch_realtek.c mutes the headphone at plugging.
>> Apply the same workaround to LDO2 power event can make the loud crack
>> sound to a more tolerable pop sound. I found that unconditionally apply
>> the workaround to all related power event can help a little further.
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=112611
>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1313434
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>>  sound/soc/codecs/rt286.c | 67
>> +++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 63 insertions(+), 4 deletions(-)
>>
>> diff --git a/sound/soc/codecs/rt286.c b/sound/soc/codecs/rt286.c
>> index 74c0e4e..d041937 100644
>> --- a/sound/soc/codecs/rt286.c
>> +++ b/sound/soc/codecs/rt286.c
>> @@ -36,6 +36,8 @@
>>  #define RT286_VENDOR_ID 0x10ec0286
>>  #define RT288_VENDOR_ID 0x10ec0288
>>
>> +#define AMP_OUT_MUTE 0xb080
>> +
>>  struct rt286_priv {
>>       struct reg_default *index_cache;
>>       int index_cache_size;
>> @@ -47,6 +49,7 @@ struct rt286_priv {
>>       struct delayed_work jack_detect_work;
>>       int sys_clk;
>>       int clk_id;
>> +     void (*mute_hpo_func)(struct snd_soc_codec *codec);
>>  };
>>
>>  static const struct reg_default rt286_index_def[] = {
>> @@ -472,10 +475,51 @@ static int rt286_set_dmic1_event(struct
>> snd_soc_dapm_widget *w,
>>       return 0;
>>  }
>>
>> +/* Add HV, VREF and LDO1 event functions to workaround headphone crack
>> noise */
>> +static int rt286_hv_event(struct snd_soc_dapm_widget *w,
>> +                          struct snd_kcontrol *kcontrol, int event)
>> +{
>> +     struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
>> +     struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
>> +
>> +     if (rt286->mute_hpo_func)
>> +             rt286->mute_hpo_func(codec);
>> +
>> +     return 0;
>> +}
>> +
>> +static int rt286_vref_event(struct snd_soc_dapm_widget *w,
>> +                          struct snd_kcontrol *kcontrol, int event)
>> +{
>> +     struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
>> +     struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
>> +
>> +     if (rt286->mute_hpo_func)
>> +             rt286->mute_hpo_func(codec);
>> +
>> +     return 0;
>> +}
>> +
>> +static int rt286_ldo1_event(struct snd_soc_dapm_widget *w,
>> +                          struct snd_kcontrol *kcontrol, int event)
>> +{
>> +     struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
>> +     struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
>> +
>> +     if (rt286->mute_hpo_func)
>> +             rt286->mute_hpo_func(codec);
>> +
>> +     return 0;
>> +}
>> +
>>  static int rt286_ldo2_event(struct snd_soc_dapm_widget *w,
>>                            struct snd_kcontrol *kcontrol, int event)
>>  {
>>       struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
>> +     struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
>> +
>> +     if (rt286->mute_hpo_func)
>> +             rt286->mute_hpo_func(codec);
>>
>>       switch (event) {
>>       case SND_SOC_DAPM_POST_PMU:
>> @@ -516,16 +560,24 @@ static int rt286_mic1_event(struct
>> snd_soc_dapm_widget *w,
>>       return 0;
>>  }
>>
>> +static void dell_dino_mute_hpo(struct snd_soc_codec *codec)
>> +{
>> +     snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE);
>> +}
>> +
>
> I didn't see where will headphone be unmute. There is already a
> headphone mute control on the driver. See
>         SND_SOC_DAPM_SWITCH("HPO L", SND_SOC_NOPM, 0, 0,
>                 &hpol_enable_control),
>         SND_SOC_DAPM_SWITCH("HPO R", SND_SOC_NOPM, 0, 0,
>                 &hpor_enable_control),

This is a direct mirror to function alc_shutup_dell_xps13() in
sound/pci/hda/patch_realtek.c.

I'll try to use what you suggest here.

>
>
>>       SND_SOC_DAPM_SUPPLY_S("LDO2", 2, RT286_POWER_CTRL1,
>>               13, 1, rt286_ldo2_event, SND_SOC_DAPM_PRE_PMD |
>> -             SND_SOC_DAPM_POST_PMU),
>> +             SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU),
>
> Is POST_PMU really what you want?

I found that mute output before any power event has better result than
mute after.

>
>> @@ -1190,6 +1242,11 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
>>               regmap_update_bits(rt286->regmap,
>>                                       RT286_CBJ_CTRL1, 0xf000, 0xb000);
>>       } else {
>> +             /* Fix headphone click noise */
>> +             if (dmi_check_system(dmi_dell_dino))
>> +                     regmap_write(rt286->regmap,
>> +                                     RT286_MIC1_DET_CTRL, 0x0020);
>> +
>
> I need to figure out how 0x0020 works.

It's from commit 3e1b0c4a9d563d7fc6e22dc92613cd3237bb5ce0

>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Mark Brown]

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

On Thu, Feb 16, 2017 at 05:10:06AM +0000, Kai-Heng Feng wrote:

> Ping.
> Can anyone help reviewing this?

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, though there are some other maintainers who like them - if in
doubt look at how patches for the subsystem are normally handled.

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

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

Re: [PATCH] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Mark Brown]

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

On Mon, Jan 23, 2017 at 01:35:14PM +0800, Kai-Heng Feng wrote:

> +static void dell_dino_mute_hpo(struct snd_soc_codec *codec)
> +{
> +	snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE);
> +}
> +

How does this ever get unmuted, should it be an _AUTODISABLE control
instead?

> @@ -1204,6 +1261,7 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
>  	mdelay(10);
>  
>  	regmap_write(rt286->regmap, RT286_MISC_CTRL1, 0x0000);
> +
>  	/* Power down LDO, VREF */
>  	regmap_update_bits(rt286->regmap, RT286_POWER_CTRL2, 0xc, 0x0);
>  	regmap_update_bits(rt286->regmap, RT286_POWER_CTRL1, 0x1001, 0x1001);

Random whitespace change.

> @@ -1222,6 +1280,7 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
>  			RT286_SET_GPIO_DATA, 0x40, 0x40);
>  		regmap_update_bits(rt286->regmap,
>  			RT286_GPIO_CTRL, 0xc, 0x8);
> +		rt286->mute_hpo_func = dell_dino_mute_hpo;
>  	}

Why would we only do this on some machines, does this break others?

This change does appear to be two different changes merged into one,
there's the GPIO setup and this automute thing and they don't seem to
overlap AFAICT - they should be split into separate patches unless
there's some reason why they overlap but I'm not seeing that.

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

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

Re: [PATCH] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Kai-Heng Feng]

On Fri, Feb 17, 2017 at 3:22 AM Mark Brown <broonie@kernel.org> wrote:

> On Mon, Jan 23, 2017 at 01:35:14PM +0800, Kai-Heng Feng wrote:
>
> > +static void dell_dino_mute_hpo(struct snd_soc_codec *codec)
> > +{
> > +     snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE);
> > +}
> > +
>
> How does this ever get unmuted, should it be an _AUTODISABLE control
> instead?
>

I think the AMP is overrode by another value when playing sound, but I am
not sure.

Can there be multiple callbacks hooked to the same widget?
In this case, in addition to hpol_enable_control(), can I register another
callback to "HPO L"?


>
> > @@ -1204,6 +1261,7 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
> >       mdelay(10);
> >
> >       regmap_write(rt286->regmap, RT286_MISC_CTRL1, 0x0000);
> > +
> >       /* Power down LDO, VREF */
> >       regmap_update_bits(rt286->regmap, RT286_POWER_CTRL2, 0xc, 0x0);
> >       regmap_update_bits(rt286->regmap, RT286_POWER_CTRL1, 0x1001,
> 0x1001);
>
> Random whitespace change.
>
> > @@ -1222,6 +1280,7 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
> >                       RT286_SET_GPIO_DATA, 0x40, 0x40);
> >               regmap_update_bits(rt286->regmap,
> >                       RT286_GPIO_CTRL, 0xc, 0x8);
> > +             rt286->mute_hpo_func = dell_dino_mute_hpo;
> >       }
>
> Why would we only do this on some machines, does this break others?
>

Because there's already an existing dmi quirk "dmi_dell_dino", I think I
should just follow.
I don't know if this may break other machines or not, I only have XPS 9343
to test.


>
> This change does appear to be two different changes merged into one,
> there's the GPIO setup and this automute thing and they don't seem to
> overlap AFAICT - they should be split into separate patches unless
> there's some reason why they overlap but I'm not seeing that.
>

Understood. I'll split them in later version.

Re: [PATCH] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Mark Brown]

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

On Mon, Mar 13, 2017 at 08:41:41AM +0000, Kai-Heng Feng wrote:
> On Fri, Feb 17, 2017 at 3:22 AM Mark Brown <broonie@kernel.org> wrote:

> > On Mon, Jan 23, 2017 at 01:35:14PM +0800, Kai-Heng Feng wrote:

> > > +static void dell_dino_mute_hpo(struct snd_soc_codec *codec)
> > > +{
> > > +     snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE);
> > > +}
> > > +

> > How does this ever get unmuted, should it be an _AUTODISABLE control
> > instead?

> I think the AMP is overrode by another value when playing sound, but I am
> not sure.

That's bad, we shouldn't have multiple inconsistent ways of controlling
the same thing - this tends to lead to bugs as they need to be
coordinated and things might change in one or both mechanisms.

> Can there be multiple callbacks hooked to the same widget?
> In this case, in addition to hpol_enable_control(), can I register another
> callback to "HPO L"?

You can have a callback that calls other callbacks to share code, you
can't do this directly in the framework itself.

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

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

Re: [PATCH] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode

[Garrett Thornburg]

This is my first time posting in any type of mailing list, so
hopefully I'm doing this right. I recently encountered the audio
trouble with I2S mode via the headphone jack on my Dell XPS 9343 as
well and found your patch.

I applied the patch to the latest stable kernel (4.10) on arch linux
mostly using their tools and the repeated clicking sound is fixed when
no sound is playing. However, sometimes when sound is playing, the
clicking returns but it's much more mild. If I pause/ resume music,
for example, it might "click.. click.. click.." very quietly, but not
always. It's audible when I turn the volume level down low, but
overall it's much better.

I do still get somewhat loud pops when resuming music after the sound
card has been idle for a while, or a pop when the card switches to an
idle state.

This patch has really improved my situation though, so thank you :).