[alsa-devel] Broken headphone and speaker amplifier supplies for the rt5640 driver
Hi,
The rt5640 driver sets for both the headphone and speaker amplifier widget the shift to SND_SOC_NOPM, which is -1. This doesn't make much sense and breaks compilation with some changes I've made.
I guess for the heaphone amplifier the right fix is to set the shift to RT5640_PWR_HA_BIT. For the speaker amplifier I'd assume that the right bit is RT5640_PWR_CLS_D_BIT. But the speaker amplifier widget also as an event callback in which it already seems to set/clear the bit. I think the right fix is to remove the register change from the event callback and set the shift to RT5640_PWR_CLS_D_BIT. But I neither have the hardware nor the datasheet, so this is all just an educated guess.
- Lars
On 07/26/2013 09:06 AM, Lars-Peter Clausen wrote:
Hi,
The rt5640 driver sets for both the headphone and speaker amplifier widget the shift to SND_SOC_NOPM, which is -1. This doesn't make much sense and breaks compilation with some changes I've made.
I guess for the heaphone amplifier the right fix is to set the shift to RT5640_PWR_HA_BIT ...
If I make those changes (see below for the exact patch I tried), then both headphones and speakers do appear to still work fine. Bard will have to comment on whether the changes are actually correct though:-)
diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c index ce585e3..e3ab0b2 100644 --- a/sound/soc/codecs/rt5640.c +++ b/sound/soc/codecs/rt5640.c @@ -899,8 +899,6 @@ static int spk_event(struct snd_soc_dapm_widget *w,
switch (event) { case SND_SOC_DAPM_POST_PMU:
regmap_update_bits(rt5640->regmap, RT5640_PWR_DIG1,0x0001, 0x0001);@@ -908,8 +906,6 @@ static int spk_event(struct snd_soc_dapm_widget *w, case SND_SOC_DAPM_PRE_PMD: regmap_update_bits(rt5640->regmap, RT5640_PR_BASE + 0x1c, 0xf000, 0x0000);
regmap_update_bits(rt5640->regmap, RT5640_PWR_DIG1,0x0001, 0x0000);break;
default:
@@ -1159,13 +1155,13 @@ static const struct snd_soc_dapm_widget rt5640_dapm_widgets[] = { SND_SOC_DAPM_SUPPLY("Improve MONO Amp Drv", RT5640_PWR_ANLG1, RT5640_PWR_MA_BIT, 0, NULL, 0), SND_SOC_DAPM_SUPPLY("Improve HP Amp Drv", RT5640_PWR_ANLG1,
SND_SOC_NOPM, 0, NULL, 0),
RT5640_PWR_HA_BIT, 0, NULL, 0),
SND_SOC_NOPM, 0, spk_event,
RT5640_PWR_CLS_D_BIT, 0, spk_event,
On Fri, Jul 26, 2013 at 05:06:12PM +0200, Lars-Peter Clausen wrote:
I guess for the heaphone amplifier the right fix is to set the shift to RT5640_PWR_HA_BIT. For the speaker amplifier I'd assume that the right bit is RT5640_PWR_CLS_D_BIT. But the speaker amplifier widget also as an event callback in which it already seems to set/clear the bit. I think the right fix is to remove the register change from the event callback and set the shift to RT5640_PWR_CLS_D_BIT. But I neither have the hardware nor the datasheet, so this is all just an educated guess.
Or set the register to SND_SOC_NOPM allowing the manual sequencing which is (I presume) needed? If we're going to need an event to do the sequencing we may as well have it do both writes.
-----Original Message----- From: Mark Brown [mailto:broonie@kernel.org] Sent: Sunday, July 28, 2013 1:35 AM To: Lars-Peter Clausen Cc: Stephen Warren; Bard Liao; Linux-ALSA Subject: Re: [alsa-devel] Broken headphone and speaker amplifier supplies forthe rt5640 driver
On Fri, Jul 26, 2013 at 05:06:12PM +0200, Lars-Peter Clausen wrote:
I guess for the heaphone amplifier the right fix is to set the shift to RT5640_PWR_HA_BIT. For the speaker amplifier I'd assume that the right bit is RT5640_PWR_CLS_D_BIT. But the speaker amplifier widget also as an event callback in which it already seems to set/clear the bit. I think the right fix is to remove the register change from the event callback and set the shift to RT5640_PWR_CLS_D_BIT. But I neither have the hardware nor the datasheet, so this is all just an educated
guess.
Or set the register to SND_SOC_NOPM allowing the manual sequencing which is (I presume) needed? If we're going to need an event to do the sequencing we may as well have it do both writes.
We do need a sequence for headphone depop. Another question about it. Can I do mute/unmute in the widget event? The mute/unmute control is written in rt5640_snd_controls[] now. It allows user to unmute speaker or headphone before dapm power on the related power. And it will bring a pop noise. So I prefer to do the unmute/mute step in the widget event. Is that ok?
------Please consider the environment before printing this e-mail.
On Mon, Jul 29, 2013 at 11:04:46AM +0800, Bard Liao wrote:
The mute/unmute control is written in rt5640_snd_controls[] now. It allows user to unmute speaker or headphone before dapm power on the related power. And it will bring a pop noise. So I prefer to do the unmute/mute step in the widget event. Is that ok?
If you need to do that you should really still present the mute control to the user; store the current state in a variable in the private data so that the user always sees the control and then only write the value out while the widget is powered. Mute is expected to be fast and some userspaces like to be able to mute individual outputs.
Ideally the core would be able to do this.
On 07/29/2013 08:07 AM, Mark Brown wrote:
On Mon, Jul 29, 2013 at 11:04:46AM +0800, Bard Liao wrote:
The mute/unmute control is written in rt5640_snd_controls[] now. It allows user to unmute speaker or headphone before dapm power on the related power. And it will bring a pop noise. So I prefer to do the unmute/mute step in the widget event. Is that ok?
If you need to do that you should really still present the mute control to the user; store the current state in a variable in the private data so that the user always sees the control and then only write the value out while the widget is powered. Mute is expected to be fast and some userspaces like to be able to mute individual outputs.
Ideally the core would be able to do this.
Yea, so the stuff I was working on while I stumbled upon this was more or less that. Support for letting DAPM mute controls, which are also exposed to userspace, if necessary in order to avoid clicks and pops. Once muted by DAPM userspace will only see a cached value of the controls state and once DAPM unmutes the control the userspace state will be restored.
- Lars
On Mon, Jul 29, 2013 at 08:14:51AM +0200, Lars-Peter Clausen wrote:
Yea, so the stuff I was working on while I stumbled upon this was more or less that. Support for letting DAPM mute controls, which are also exposed to userspace, if necessary in order to avoid clicks and pops. Once muted by DAPM userspace will only see a cached value of the controls state and once DAPM unmutes the control the userspace state will be restored.
Oh, cool. That's not really been an issue with most ground referenced hardware (at least not normally) but it'd really useful for a large proportion of VMID based devices.
participants (4)
-
Bard Liao -
Lars-Peter Clausen -
Mark Brown -
Stephen Warren