| Message ID | 20170123053514.6193-1-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > >
> -----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.
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
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.
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.
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.
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.
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) {
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(-)