Broken headphone and speaker amplifier supplies for the rt5640 driver

Broken headphone and speaker amplifier supplies for the rt5640 driver

[Lars-Peter Clausen]

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

Re: Broken headphone and speaker amplifier supplies for the rt5640 driver

[Stephen Warren]

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);
>  		regmap_update_bits(rt5640->regmap, RT5640_PR_BASE + 0x1c,
>  					0xf000, 0xf000);
>  		break;
> @@ -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_DAPM_PGA("HP L Amp", RT5640_PWR_ANLG1,
>  		RT5640_PWR_HP_L_BIT, 0, NULL, 0),
>  	SND_SOC_DAPM_PGA("HP R Amp", RT5640_PWR_ANLG1,
>  		RT5640_PWR_HP_R_BIT, 0, NULL, 0),
>  	SND_SOC_DAPM_SUPPLY("Improve SPK Amp Drv", RT5640_PWR_DIG1,
> -		SND_SOC_NOPM, 0, spk_event,
> +		RT5640_PWR_CLS_D_BIT, 0, spk_event,
>  		SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMU),
>  	/* Output Lines */
>  	SND_SOC_DAPM_OUTPUT("SPOLP"),

Re: Broken headphone and speaker amplifier supplies for the rt5640 driver

[Mark Brown]

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

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

Re: Broken headphone and speaker amplifier suppliesforthe rt5640 driver

[Bard Liao]

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

Re: Broken headphone and speaker amplifier suppliesforthe rt5640 driver

[Mark Brown]

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

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

Re: Broken headphone and speaker amplifier suppliesforthe rt5640 driver

[Lars-Peter Clausen]

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

Re: Broken headphone and speaker amplifier suppliesforthe rt5640 driver

[Mark Brown]

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

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

Re: Broken headphone and speaker amplifier suppliesforthe rt5640 driver

[Bard Liao]

> -----Original Message-----
> From: alsa-devel-bounces@alsa-project.org
> [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of Stephen Warren
> Sent: Saturday, July 27, 2013 12:58 AM
> To: Lars-Peter Clausen
> Cc: Bard Liao; Linux-ALSA
> Subject: Re: [alsa-devel] Broken headphone and speaker amplifier suppliesfor
> the rt5640 driver
> 
> 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:-)

The control bits for headphone amplifier and speaker amplifier are both correct.
But rt5640 need a sequence for headphone depop.
So, I would like to do something like this.
- SND_SOC_DAPM_SUPPLY("Improve HP Amp Drv", RT5640_PWR_ANLG1,
- 		SND_SOC_NOPM, 0, NULL, 0),
+ SND_SOC_DAPM_SUPPLY_S("Improve HP Amp Drv", 1, SND_SOC_NOPM,
+		0, 0, rt5640_hp_power_event,
+		SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMU),

I will send a patch ASAP.

Thanks.

> 
> > 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);
> >  		regmap_update_bits(rt5640->regmap, RT5640_PR_BASE + 0x1c,
> >  					0xf000, 0xf000);
> >  		break;
> > @@ -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_DAPM_PGA("HP L Amp", RT5640_PWR_ANLG1,
> >  		RT5640_PWR_HP_L_BIT, 0, NULL, 0),
> >  	SND_SOC_DAPM_PGA("HP R Amp", RT5640_PWR_ANLG1,
> >  		RT5640_PWR_HP_R_BIT, 0, NULL, 0),
> >  	SND_SOC_DAPM_SUPPLY("Improve SPK Amp Drv", RT5640_PWR_DIG1,
> > -		SND_SOC_NOPM, 0, spk_event,
> > +		RT5640_PWR_CLS_D_BIT, 0, spk_event,
> >  		SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMU),
> >  	/* Output Lines */
> >  	SND_SOC_DAPM_OUTPUT("SPOLP"),
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel