| Message ID | 20170320035831.10762-1-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 20, 2017 at 11:58:31AM +0800, Kai-Heng Feng wrote: > v3: > Implicit conversion instead of tenary operator. > > v2: > Use 'HP Power' instead of individual power events. As covered in SubmittingPatches this should come after the ---, it doesn't need to end up in the changelogs. > + switch (event) { > + case SND_SOC_DAPM_PRE_PMD: > + case SND_SOC_DAPM_POST_PMD: > + case SND_SOC_DAPM_POST_PMU: > + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE); > + break; > + case SND_SOC_DAPM_PRE_PMU: To repeat what I said last time: | After power up we mute the amplifier? That's worthy of a comment... Please don't ignore review comments, people are generally making them for a reason and are likely to have the same concerns if issues remain unaddressed. Having to repeat the same comments can get repetitive and make people question the value of time spent reviewing. If you disagree with the review comments that's fine but you need to reply and discuss your concerns so that the reviewer can understand your decisions.
On Mon, Mar 20, 2017 at 11:08 PM Mark Brown <broonie@kernel.org> wrote: > On Mon, Mar 20, 2017 at 11:58:31AM +0800, Kai-Heng Feng wrote: > > > v3: > > Implicit conversion instead of tenary operator. > > > > v2: > > Use 'HP Power' instead of individual power events. > > As covered in SubmittingPatches this should come after the ---, it > doesn't need to end up in the changelogs. > Do you mean https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L197 ? I didn't find any hard rules regarding this, but I'll keep it in mind. > > > + switch (event) { > > + case SND_SOC_DAPM_PRE_PMD: > > + case SND_SOC_DAPM_POST_PMD: > > + case SND_SOC_DAPM_POST_PMU: > > + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE); > > + break; > > + case SND_SOC_DAPM_PRE_PMU: > > To repeat what I said last time: > > | After power up we mute the amplifier? That's worthy of a comment... > Please don't ignore review comments, people are generally making them > for a reason and are likely to have the same concerns if issues remain > unaddressed. Having to repeat the same comments can get repetitive and > make people question the value of time spent reviewing. If you disagree > with the review comments that's fine but you need to reply and discuss > your concerns so that the reviewer can understand your decisions. > IIUC, HPO Power's _POST_PMU is triggered right before power down (_PRE_PMD), hence it's pretty logical to mute the amplifier at this stage. I can't quite see anything wrong here. So no I didn't ignore your comment, I simply misinterpreted what you meant. Because of the logical assumption, I thought you were talking the unmute part in _PRE_PMU, which I did add in the changelog. Again, what's the issue here?
On Mon, Mar 20, 2017 at 03:46:13PM +0000, Kai-Heng Feng wrote: > On Mon, Mar 20, 2017 at 11:08 PM Mark Brown <broonie@kernel.org> wrote: > > As covered in SubmittingPatches this should come after the ---, it > > doesn't need to end up in the changelogs. > Do you mean > https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L197 > ? > I didn't find any hard rules regarding this, but I'll keep it in mind. As it says there "...and inserted automatically following the three dash line". > > > SND_SOC_DAPM_POST_PMD: + case SND_SOC_DAPM_POST_PMU: + > > > snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE); + > > > break; + case SND_SOC_DAPM_PRE_PMU: Please fix your mail client not to completely mangle quoted patches when replying. > > To repeat what I said last time: > > | After power up we mute the amplifier? That's worthy of a > > comment... > IIUC, HPO Power's _POST_PMU is triggered right before power down > (_PRE_PMD), hence it's pretty logical to mute the amplifier at this > stage. I can't quite see anything wrong here. No, that is not the case - I'm not sure what would lead you to believe that it is. _POST_PMU is triggered as the last step of powering up the widget as the name might suggest. Has this code been tested at all? > So no I didn't ignore your comment, I simply misinterpreted what you > meant. Because of the logical assumption, I thought you were talking > the unmute part in _PRE_PMU, which I did add in the changelog. You didn't reply to my review comment and you sent the same code again. That looks an awful lot like being ignored.
On Tue, Mar 21, 2017 at 12:06 AM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Mar 20, 2017 at 03:46:13PM +0000, Kai-Heng Feng wrote: >> On Mon, Mar 20, 2017 at 11:08 PM Mark Brown <broonie@kernel.org> wrote: > >> > As covered in SubmittingPatches this should come after the ---, it >> > doesn't need to end up in the changelogs. > >> Do you mean >> https://github.com/git/git/blob/master/Documentation/SubmittingPatches#L197 >> ? >> I didn't find any hard rules regarding this, but I'll keep it in mind. > > As it says there "...and inserted automatically following the three dash > line". I saw iteration changelog in git log all over the place, maybe add a rule section for each subsystem? > >> > > SND_SOC_DAPM_POST_PMD: + case SND_SOC_DAPM_POST_PMU: + >> > > snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE); + >> > > break; + case SND_SOC_DAPM_PRE_PMU: > > Please fix your mail client not to completely mangle quoted patches when > replying. Okay. I was toying with Google Inbox. > >> > To repeat what I said last time: > >> > | After power up we mute the amplifier? That's worthy of a >> > comment... > >> IIUC, HPO Power's _POST_PMU is triggered right before power down >> (_PRE_PMD), hence it's pretty logical to mute the amplifier at this >> stage. I can't quite see anything wrong here. > > No, that is not the case - I'm not sure what would lead you to believe > that it is. _POST_PMU is triggered as the last step of powering up the > widget as the name might suggest. Has this code been tested at all? I had the same thought originally, but printk under each case suggests otherwise - _POST_PMU is triggered not right after _PRE_PMU but right before _PRE_PMD. And yes, the patch was tested on a real machine. > >> So no I didn't ignore your comment, I simply misinterpreted what you >> meant. Because of the logical assumption, I thought you were talking >> the unmute part in _PRE_PMU, which I did add in the changelog. > > You didn't reply to my review comment and you sent the same code again. > That looks an awful lot like being ignored. Fair enough, I thought changelog is sufficient.
On Tue, Mar 21, 2017 at 12:23:53AM +0800, Kai-Heng Feng wrote: > On Tue, Mar 21, 2017 at 12:06 AM, Mark Brown <broonie@kernel.org> wrote: > > As it says there "...and inserted automatically following the three dash > > line". > I saw iteration changelog in git log all over the place, maybe add a > rule section for each subsystem? Some people won't push back, the only people who insist on anything different are the graphics people. > I had the same thought originally, but printk under each case suggests > otherwise - _POST_PMU is triggered not right after _PRE_PMU but right > before _PRE_PMD. Then you've broken something else on your system, that is obviously completely nonsensical and would break anything that relies on having a _POST_PMU event. Why would we have two events that run at the same time one of which is obviously misnamed? > > > You didn't reply to my review comment and you sent the same code > > > again. > > That looks an awful lot like being ignored. > Fair enough, I thought changelog is sufficient. I'm not seeing anything in the changelog that addresses this.
> -----Original Message----- > From: Kai-Heng Feng [mailto:kai.heng.feng@canonical.com] > Sent: Monday, March 20, 2017 11:59 AM > To: broonie@kernel.org > Cc: lgirdwood@gmail.com; Bard Liao; Oder Chiou; > alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Kai-Heng Feng > Subject: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell XPS > 9343 I2S mode > > + switch (event) { > + case SND_SOC_DAPM_PRE_PMD: > + case SND_SOC_DAPM_POST_PMD: > + case SND_SOC_DAPM_POST_PMU: > + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, > AMP_OUT_MUTE); > + break; > + case SND_SOC_DAPM_PRE_PMU: > + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, > AMP_OUT_UNMUTE); > + break; Besides Mark's comment, I have question here. It seems you want to mute HPO before "HP Power" is powered up and after "HP Power" is powered down. But "HPO L" and "HPO R" are autodisable. And "HP Power" is only connect to "HPO L" and "HPO R". From my understanding, HPO will mute if "HP Power" is powered down. Any specific reason for muting HPO again before "HP Power" is powered up? Will HPO be unmuted before "HP Power" is powered up on your system? Or should the event be associated with "LDO1"? Which power will cause the click noise?
On Tue, Mar 21, 2017 at 1:26 AM, Mark Brown <broonie@kernel.org> wrote: > On Tue, Mar 21, 2017 at 12:23:53AM +0800, Kai-Heng Feng wrote: >> On Tue, Mar 21, 2017 at 12:06 AM, Mark Brown <broonie@kernel.org> wrote: > >> > As it says there "...and inserted automatically following the three dash >> > line". > >> I saw iteration changelog in git log all over the place, maybe add a >> rule section for each subsystem? > > Some people won't push back, the only people who insist on anything > different are the graphics people. Got it. I think the way you suggested is better. > >> I had the same thought originally, but printk under each case suggests >> otherwise - _POST_PMU is triggered not right after _PRE_PMU but right >> before _PRE_PMD. > > Then you've broken something else on your system, that is obviously > completely nonsensical and would break anything that relies on having a > _POST_PMU event. Why would we have two events that run at the same time > one of which is obviously misnamed? Hmm, that's weird though. I did the same test to rt286_spk_event() (without applying the patch I sent), what I observed was the same: _POST_PMU was triggered right after I stopped play sound, i.e. right before _PRE_PMD not right after _PRE_PMU. > >> > > You didn't reply to my review comment and you sent the same code >> > > again. >> > That looks an awful lot like being ignored. > >> Fair enough, I thought changelog is sufficient. > > I'm not seeing anything in the changelog that addresses this.
On Tue, Mar 21, 2017 at 11:07 AM, Bard Liao <bardliao@realtek.com> wrote: >> -----Original Message----- >> From: Kai-Heng Feng [mailto:kai.heng.feng@canonical.com] >> Sent: Monday, March 20, 2017 11:59 AM >> To: broonie@kernel.org >> Cc: lgirdwood@gmail.com; Bard Liao; Oder Chiou; >> alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Kai-Heng Feng >> Subject: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell XPS >> 9343 I2S mode >> >> + switch (event) { >> + case SND_SOC_DAPM_PRE_PMD: >> + case SND_SOC_DAPM_POST_PMD: >> + case SND_SOC_DAPM_POST_PMU: >> + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, >> AMP_OUT_MUTE); >> + break; >> + case SND_SOC_DAPM_PRE_PMU: >> + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, >> AMP_OUT_UNMUTE); >> + break; > > Besides Mark's comment, I have question here. It seems you want to mute > HPO before "HP Power" is powered up and after "HP Power" is powered down. > But "HPO L" and "HPO R" are autodisable. And "HP Power" is only connect to What I really want to do is something rt5670's rt5670_hp_event(), maybe autodisable is not enough sometimes? > "HPO L" and "HPO R". From my understanding, HPO will mute if "HP Power" > is powered down. Any specific reason for muting HPO again before "HP Power" > is powered up? You are right. Either one of them should be sufficient. > Will HPO be unmuted before "HP Power" is powered up on your system? Yes. I am no audio expert here - but from what I read from HDA, there's actually no AMP unmute counterpart to AMP mute. > Or should the event be associated with "LDO1"? Which power will > cause the click noise? I found that the effect is most noticeable if the mute callback is associated with "LDO2" and "HP Power". But again, this is just what I observed. > >
> -----Original Message----- > From: Kai-Heng Feng [mailto:kai.heng.feng@canonical.com] > Sent: Tuesday, March 21, 2017 1:39 PM > To: Bard Liao > Cc: broonie@kernel.org; lgirdwood@gmail.com; Oder Chiou; > alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell > XPS 9343 I2S mode > >> > >> + switch (event) { > >> + case SND_SOC_DAPM_PRE_PMD: > >> + case SND_SOC_DAPM_POST_PMD: > >> + case SND_SOC_DAPM_POST_PMU: > >> + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, > >> AMP_OUT_MUTE); > >> + break; > >> + case SND_SOC_DAPM_PRE_PMU: > >> + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, > >> AMP_OUT_UNMUTE); > >> + break; > > > > Besides Mark's comment, I have question here. It seems you want to mute > > HPO before "HP Power" is powered up and after "HP Power" is powered > down. > > But "HPO L" and "HPO R" are autodisable. And "HP Power" is only connect to > > What I really want to do is something rt5670's rt5670_hp_event(), > maybe autodisable is not enough sometimes? It is different. rt5670_hp_event() is doing depop sequence for headphone. And there is no other mute/unmute controls on other dapm widgets. For me, what you do here is not different from "HPO L" and "HPO R" do. > > > "HPO L" and "HPO R". From my understanding, HPO will mute if "HP Power" > > is powered down. Any specific reason for muting HPO again before "HP > Power" > > is powered up? > > You are right. Either one of them should be sufficient. My point is that you seem to do things that driver is already done. But why and how it can reduce the click noise? > > > Will HPO be unmuted before "HP Power" is powered up on your system? > > Yes. > I am no audio expert here - but from what I read from HDA, there's > actually no AMP unmute counterpart to AMP mute. I didn't get it. How did you check if HPO is muted? > > > Or should the event be associated with "LDO1"? Which power will > > cause the click noise? > > I found that the effect is most noticeable if the mute callback is > associated with "LDO2" and "HP Power". > But again, this is just what I observed. Could you try only associated with "LDO2"? It makes sense that will reduce the noise if a jack is plugged in/out when HPO is already powered up. I have question about the code below + /* Fix headphone click noise */ + if (dmi_check_system(dmi_dell_dino)) + regmap_write(rt286->regmap, + RT286_MIC1_DET_CTRL, 0x0020); + What does this for? How did you get the value 0x0020? I just checked with Kailang, but he have no idea about that. > > > > > > > ------Please consider the environment before printing this e-mail.
> -----Original Message----- > From: Kai-Heng Feng [mailto:kai.heng.feng@canonical.com] > Sent: Tuesday, March 21, 2017 1:26 PM > To: Mark Brown > Cc: Liam Girdwood; Bard Liao; Oder Chiou; alsa-devel@alsa-project.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell > XPS 9343 I2S mode > > > >> I had the same thought originally, but printk under each case suggests > >> otherwise - _POST_PMU is triggered not right after _PRE_PMU but right > >> before _PRE_PMD. > > > > Then you've broken something else on your system, that is obviously > > completely nonsensical and would break anything that relies on having a > > _POST_PMU event. Why would we have two events that run at the same > time > > one of which is obviously misnamed? > > Hmm, that's weird though. I did the same test to rt286_spk_event() > (without applying the patch I sent), what I observed was the same: > _POST_PMU was triggered right after I stopped play sound, i.e. right > before _PRE_PMD not right after _PRE_PMU. > Although I don't think it will happen on my side, but I still ran the same test as you. The result is just as what we expected, _PRE_PMU and _POST_PMU run on powering up stage and _PRE_PMD and _POST_PMD run on powering down stage. Please check what's going on with your system. > ------Please consider the environment before printing this e-mail.
On Tue, Mar 21, 2017 at 4:59 PM, Bard Liao <bardliao@realtek.com> wrote: >> -----Original Message----- >> From: Kai-Heng Feng [mailto:kai.heng.feng@canonical.com] >> Sent: Tuesday, March 21, 2017 1:39 PM >> To: Bard Liao >> Cc: broonie@kernel.org; lgirdwood@gmail.com; Oder Chiou; >> alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell >> XPS 9343 I2S mode >> >> >> >> + switch (event) { >> >> + case SND_SOC_DAPM_PRE_PMD: >> >> + case SND_SOC_DAPM_POST_PMD: >> >> + case SND_SOC_DAPM_POST_PMU: >> >> + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, >> >> AMP_OUT_MUTE); >> >> + break; >> >> + case SND_SOC_DAPM_PRE_PMU: >> >> + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, >> >> AMP_OUT_UNMUTE); >> >> + break; >> > >> > Besides Mark's comment, I have question here. It seems you want to mute >> > HPO before "HP Power" is powered up and after "HP Power" is powered >> down. >> > But "HPO L" and "HPO R" are autodisable. And "HP Power" is only connect to >> >> What I really want to do is something rt5670's rt5670_hp_event(), >> maybe autodisable is not enough sometimes? > > It is different. rt5670_hp_event() is doing depop sequence for > headphone. And there is no other mute/unmute controls on other > dapm widgets. For me, what you do here is not different from > "HPO L" and "HPO R" do. There are two issues - background click noise and the cracking pop noise. Depop is exactly what I want to do here. > >> >> > "HPO L" and "HPO R". From my understanding, HPO will mute if "HP Power" >> > is powered down. Any specific reason for muting HPO again before "HP >> Power" >> > is powered up? >> >> You are right. Either one of them should be sufficient. > > My point is that you seem to do things that driver is already done. > But why and how it can reduce the click noise? This is for the crack (pop) noise not click noise - see below. > >> >> > Will HPO be unmuted before "HP Power" is powered up on your system? >> >> Yes. >> I am no audio expert here - but from what I read from HDA, there's >> actually no AMP unmute counterpart to AMP mute. > > I didn't get it. How did you check if HPO is muted? I didn't. Now sure why do we need to check that? > >> >> > Or should the event be associated with "LDO1"? Which power will >> > cause the click noise? >> >> I found that the effect is most noticeable if the mute callback is >> associated with "LDO2" and "HP Power". >> But again, this is just what I observed. > > Could you try only associated with "LDO2"? > It makes sense that will reduce the noise if a jack is plugged in/out > when HPO is already powered up. Does it also help to reduce noise at other power events? > > I have question about the code below > + /* Fix headphone click noise */ > + if (dmi_check_system(dmi_dell_dino)) > + regmap_write(rt286->regmap, > + RT286_MIC1_DET_CTRL, 0x0020); > + > > What does this for? How did you get the value 0x0020? > I just checked with Kailang, but he have no idea about that. It's PIN_VREFHIZ. It's from commit 3e1b0c4a9d56. > >> >> > >> > >> >> ------Please consider the environment before printing this e-mail.
On Tue, Mar 21, 2017 at 5:15 PM, Bard Liao <bardliao@realtek.com> wrote: >> -----Original Message----- >> From: Kai-Heng Feng [mailto:kai.heng.feng@canonical.com] >> Sent: Tuesday, March 21, 2017 1:26 PM >> To: Mark Brown >> Cc: Liam Girdwood; Bard Liao; Oder Chiou; alsa-devel@alsa-project.org; >> linux-kernel@vger.kernel.org >> Subject: Re: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell >> XPS 9343 I2S mode >> > >> >> I had the same thought originally, but printk under each case suggests >> >> otherwise - _POST_PMU is triggered not right after _PRE_PMU but right >> >> before _PRE_PMD. >> > >> > Then you've broken something else on your system, that is obviously >> > completely nonsensical and would break anything that relies on having a >> > _POST_PMU event. Why would we have two events that run at the same >> time >> > one of which is obviously misnamed? >> >> Hmm, that's weird though. I did the same test to rt286_spk_event() >> (without applying the patch I sent), what I observed was the same: >> _POST_PMU was triggered right after I stopped play sound, i.e. right >> before _PRE_PMD not right after _PRE_PMU. >> > > Although I don't think it will happen on my side, but I still > ran the same test as you. The result is just as what we > expected, _PRE_PMU and _POST_PMU run on powering > up stage and _PRE_PMD and _POST_PMD run on powering > down stage. Please check what's going on with your system. > Maybe mine is broken, maybe other XPS 9343 share the same behavior. I guess we need more users to provide information to continue the discussion. > >> ------Please consider the environment before printing this e-mail.
> -----Original Message----- > From: Kai-Heng Feng [mailto:kai.heng.feng@canonical.com] > Sent: Wednesday, March 22, 2017 1:37 PM > To: Bard Liao > Cc: broonie@kernel.org; lgirdwood@gmail.com; Oder Chiou; > alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell > XPS 9343 I2S mode > > >> What I really want to do is something rt5670's rt5670_hp_event(), > >> maybe autodisable is not enough sometimes? > > > > It is different. rt5670_hp_event() is doing depop sequence for > > headphone. And there is no other mute/unmute controls on other > > dapm widgets. For me, what you do here is not different from > > "HPO L" and "HPO R" do. > > There are two issues - background click noise and the cracking pop noise. > Depop is exactly what I want to do here. Let me explain it in more detail. rt5670 need to set a serious of registers to prevent the pop noise of powering up/down muting/ unmuting headphone. That's what rt5670_hp_event() does. But, what rt286_hp_power_event do is only mute/unmute headphone which is done by "HPO L" and "HPO R" widget. > > > > >> > >> > "HPO L" and "HPO R". From my understanding, HPO will mute if "HP > Power" > >> > is powered down. Any specific reason for muting HPO again before "HP > >> Power" > >> > is powered up? > >> > >> You are right. Either one of them should be sufficient. > > > > My point is that you seem to do things that driver is already done. > > But why and how it can reduce the click noise? > > This is for the crack (pop) noise not click noise - see below. > > > > >> > >> > Will HPO be unmuted before "HP Power" is powered up on your system? > >> > >> Yes. > >> I am no audio expert here - but from what I read from HDA, there's > >> actually no AMP unmute counterpart to AMP mute. > > > > I didn't get it. How did you check if HPO is muted? > > I didn't. Now sure why do we need to check that? If HPO is already muted as what we expected, it means "HPO L" and "HPO R" work properly. And there is no reason we create an event to do the same thing. > >> > >> I found that the effect is most noticeable if the mute callback is > >> associated with "LDO2" and "HP Power". > >> But again, this is just what I observed. > > > > Could you try only associated with "LDO2"? > > It makes sense that will reduce the noise if a jack is plugged in/out > > when HPO is already powered up. > > Does it also help to reduce noise at other power events? I don't know. In theory, you shouldn't hear any sound when HPO is muted. If you need our help for debugging, please send a mail to our FAE and cc me. > > > > > I have question about the code below > > + /* Fix headphone click noise */ > > + if (dmi_check_system(dmi_dell_dino)) > > + regmap_write(rt286->regmap, > > + RT286_MIC1_DET_CTRL, > 0x0020); > > + > > > > What does this for? How did you get the value 0x0020? > > I just checked with Kailang, but he have no idea about that. > > It's PIN_VREFHIZ. It's from commit 3e1b0c4a9d56. snd_hda_codec_set_pin_target(codec, 0x19, PIN_VREFHIZ); 0x19 here means nid 0x19. But if you write 0x19 in rt286.c means write a hidden register with index 0x19. It is totally different. The corresponding code on rt286.c will be rt286->regmap(rt286->regmap, VERB_CMD(AC_VERB_SET_PIN_WIDGET_CONTROL, 0x19, 0x20)); > > > > >> > >> > > >> > > >> > >> ------Please consider the environment before printing this e-mail.
[snip] > Let me explain it in more detail. rt5670 need to set a serious of > registers to prevent the pop noise of powering up/down muting/ > unmuting headphone. That's what rt5670_hp_event() does. But, > what rt286_hp_power_event do is only mute/unmute headphone > which is done by "HPO L" and "HPO R" widget. Thanks for the explanation. [snip] > If HPO is already muted as what we expected, it means "HPO L" and "HPO R" > work properly. And there is no reason we create an event to do the same > thing. Can you advise me how to do a simple check on HPO L&R mute status? > >> >> >> >> I found that the effect is most noticeable if the mute callback is >> >> associated with "LDO2" and "HP Power". >> >> But again, this is just what I observed. >> > >> > Could you try only associated with "LDO2"? >> > It makes sense that will reduce the noise if a jack is plugged in/out >> > when HPO is already powered up. >> >> Does it also help to reduce noise at other power events? > > I don't know. In theory, you shouldn't hear any sound when HPO > is muted. If you need our help for debugging, please send a mail > to our FAE and cc me. Unfortunately it did happen. AMP mute did well for me and another user - please check the bug report link. > >> >> > >> > I have question about the code below >> > + /* Fix headphone click noise */ >> > + if (dmi_check_system(dmi_dell_dino)) >> > + regmap_write(rt286->regmap, >> > + RT286_MIC1_DET_CTRL, >> 0x0020); >> > + >> > >> > What does this for? How did you get the value 0x0020? >> > I just checked with Kailang, but he have no idea about that. >> >> It's PIN_VREFHIZ. It's from commit 3e1b0c4a9d56. > > snd_hda_codec_set_pin_target(codec, 0x19, PIN_VREFHIZ); > 0x19 here means nid 0x19. But if you write 0x19 in rt286.c means > write a hidden register with index 0x19. It is totally different. > The corresponding code on rt286.c will be > rt286->regmap(rt286->regmap, > VERB_CMD(AC_VERB_SET_PIN_WIDGET_CONTROL, 0x19, 0x20)); Understood, will use it instead. >> >> > >> >> >> >> > >> >> > >> >> >> >> ------Please consider the environment before printing this e-mail.
> -----Original Message----- > From: Kai-Heng Feng [mailto:kai.heng.feng@canonical.com] > Sent: Thursday, March 23, 2017 12:42 PM > To: Bard Liao > Cc: broonie@kernel.org; lgirdwood@gmail.com; Oder Chiou; > alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell > XPS 9343 I2S mode > > > If HPO is already muted as what we expected, it means "HPO L" and "HPO R" > > work properly. And there is no reason we create an event to do the same > > thing. > > Can you advise me how to do a simple check on HPO L&R mute status? You can cat /sys/kernel/debug/regmap/<bus name>/registers And check the registers of 0x2139000 for HPOR and 0x213a000 for HPOL. bit 15 = 1 for muted and 0 for unmuted. for example Mute: 2139000: 00000080 213a000: 00000080 UnMute: 2139000: 00000000 213a000: 00000000 > > > > >> >> > >> >> I found that the effect is most noticeable if the mute callback is > >> >> associated with "LDO2" and "HP Power". > >> >> But again, this is just what I observed. > >> > > >> > Could you try only associated with "LDO2"? > >> > It makes sense that will reduce the noise if a jack is plugged in/out > >> > when HPO is already powered up. > >> > >> Does it also help to reduce noise at other power events? > > > > I don't know. In theory, you shouldn't hear any sound when HPO > > is muted. If you need our help for debugging, please send a mail > > to our FAE and cc me. > > Unfortunately it did happen. AMP mute did well for me and another user > - please check the bug report link. I know it happens. But it works fine on my Intel Ultrabook Development System with upstream driver. So I need our FAE's help to check what happened on Dell XPS. According to our company policy, you should report the bug to Dell and Dell will contact our FAE if needed.
diff --git a/sound/soc/codecs/rt286.c b/sound/soc/codecs/rt286.c index 9c365a7f758d..97b52697f974 100644 --- a/sound/soc/codecs/rt286.c +++ b/sound/soc/codecs/rt286.c @@ -36,6 +36,9 @@ #define RT286_VENDOR_ID 0x10ec0286 #define RT288_VENDOR_ID 0x10ec0288 +#define AMP_OUT_MUTE 0xb080 +#define AMP_OUT_UNMUTE 0xb000 + struct rt286_priv { struct reg_default *index_cache; int index_cache_size; @@ -47,6 +50,7 @@ struct rt286_priv { struct delayed_work jack_detect_work; int sys_clk; int clk_id; + bool is_dell_dino; }; static const struct reg_default rt286_index_def[] = { @@ -472,6 +476,32 @@ static int rt286_set_dmic1_event(struct snd_soc_dapm_widget *w, return 0; } +/* Power event function to workaround headphone crack noise */ +static int rt286_hp_power_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->is_dell_dino) + return 0; + + switch (event) { + case SND_SOC_DAPM_PRE_PMD: + case SND_SOC_DAPM_POST_PMD: + case SND_SOC_DAPM_POST_PMU: + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE); + break; + case SND_SOC_DAPM_PRE_PMU: + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_UNMUTE); + break; + default: + return 0; + } + + return 0; +} + static int rt286_ldo2_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol, int event) { @@ -578,7 +608,9 @@ static const struct snd_soc_dapm_widget rt286_dapm_widgets[] = { SND_SOC_DAPM_MUX("HPO Mux", SND_SOC_NOPM, 0, 0, &rt286_hpo_mux), SND_SOC_DAPM_SUPPLY("HP Power", RT286_SET_PIN_HPO, - RT286_SET_PIN_SFT, 0, NULL, 0), + RT286_SET_PIN_SFT, 0, rt286_hp_power_event, + SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_PRE_PMU | + SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD), /* Output Mixer */ SND_SOC_DAPM_MIXER("Front", RT286_SET_POWER(RT286_DAC_OUT1), 0, 1, @@ -1175,8 +1207,10 @@ static int rt286_i2c_probe(struct i2c_client *i2c, if (pdata) rt286->pdata = *pdata; + rt286->is_dell_dino = dmi_check_system(dmi_dell_dino); + if (dmi_check_system(force_combo_jack_table) || - dmi_check_system(dmi_dell_dino)) + rt286->is_dell_dino) rt286->pdata.cbj_en = true; regmap_write(rt286->regmap, RT286_SET_AUDIO_POWER, AC_PWRST_D3); @@ -1192,6 +1226,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 (rt286->is_dell_dino) + regmap_write(rt286->regmap, + RT286_MIC1_DET_CTRL, 0x0020); + regmap_update_bits(rt286->regmap, RT286_CBJ_CTRL1, 0xf000, 0x5000); } @@ -1215,7 +1254,7 @@ static int rt286_i2c_probe(struct i2c_client *i2c, regmap_update_bits(rt286->regmap, RT286_DEPOP_CTRL3, 0xf777, 0x4737); regmap_update_bits(rt286->regmap, RT286_DEPOP_CTRL4, 0x00ff, 0x003f); - if (dmi_check_system(dmi_dell_dino)) { + if (rt286->is_dell_dino) { regmap_update_bits(rt286->regmap, RT286_SET_GPIO_MASK, 0x40, 0x40); regmap_update_bits(rt286->regmap, @@ -1224,6 +1263,9 @@ 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); + /* Workaound headphone crack noise when probing */ + regmap_write(rt286->regmap, RT286_SET_AMP_GAIN_HPO, + AMP_OUT_MUTE); } if (rt286->i2c->irq) {
HDA mode fixed the issue by these two commits: '9476d369d7b3 ALSA: hda - Mute headphone pin on suspend on XPS13 9333' '3e1b0c4a9d56 ALSA: hda - Fix click noise at start on Dell XPS13' Apply the same workarounds to rt286 can solve the issue. When jack is plugged, it rapidly generates I2C interrupts, which triggers rt286_irq() and rt286_jack_detect(), which produces the click noise. alc_fixup_dell_xps13() in patch_realtek.c sets up a pin that can stop the frantic interrupts, hence avoids 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. Set AMP_OUT_MUTE before power events can make the noise less noticeable. Unmute the AMP when the power is up. v3: Implicit conversion instead of tenary operator. v2: Use 'HP Power' instead of individual power events. 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 | 48 +++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 3 deletions(-)