[alsa-devel] [PATCH] ALSA: hda - Fix 'Beep Playback Switch' with no underlying mute switch

[Takashi Iwai]

At Tue, 14 Aug 2012 09:02:38 +0200,
David Henningsson wrote:
> 
> On 08/13/2012 05:39 PM, Takashi Iwai wrote:
> > At Mon, 13 Aug 2012 17:30:13 +0200,
> > David Henningsson wrote:
> >>
> >> Some conexant devices has no mute capability on their Beep widgets.
> >> This patch makes sure we don't try setting mutes on those widgets.
> >>
> >> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> >> ---
> >>
> >> Another Beep fix. It's not that I'm suddenly obsessed with beeping; it's my automated
> >> test tool that finds them :-)
> >
> > Good that you can demonstrate the results ;)
> 
> Yes, I'm still working on the test tool. Got to have something to show 
> at Plumber's :-)
> 
> >> I admit "knew->info == snd_hda_mixer_amp_switch_info" and "kctl->private_value = 0x10000"
> >> looks a bit hacky, feel free to suggest something more elegant if you wish.
> >
> > Checking the amp out caps in snd_hda_mixer_amp_switch_*_beep() would
> > be better, IMO.  It's not necessarily limited to patch_conexant.c.
> 
> Ok, here comes a second version of the patch. What do you think?

Better to use query_amp_caps().  It's cached, so the succeeding call
is cheap.  For example:

static bool ctl_has_mute(struct snd_kcontrol *kcontrol)
{
	struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
	return query_amp_caps(codec, get_amp_nid(kcontrol),
			      get_amp_direction(kcontrol)) & AC_AMPCAP_MUTE;
}

....
int snd_hda_mixer_amp_switch_get_beep(struct snd_kcontrol *kcontrol,
				      struct snd_ctl_elem_value *ucontrol)
{
	....
	if (beep && (!beep->enabled || !ctl_has_mute(kcontrol)) {
		ucontrol->value.integer.value[0] =
			ucontrol->value.integer.value[1] = beep->enabled;
		return 0;
	}
	....
}


thanks,

Takashi

> 
> 
> -- 
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
> [2 0001-ALSA-hda-Fix-Beep-Playback-Switch-with-no-underlying.patch <text/x-patch (7bit)>]
> >From 82710da74aa51e3fa1c19f1d575cbe124327e501 Mon Sep 17 00:00:00 2001
> From: David Henningsson <david.henningsson at canonical.com>
> Date: Mon, 13 Aug 2012 17:10:46 +0200
> Subject: [PATCH] ALSA: hda - Fix 'Beep Playback Switch' with no underlying
>  mute switch
> 
> Some Conexant devices (e g CX20590) have no mute capability on
> their Beep widgets.
> This patch makes sure we don't try setting mutes on those widgets.
> 
> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> ---
>  sound/pci/hda/hda_beep.c |   26 ++++++++++++++++++++++++--
>  sound/pci/hda/hda_beep.h |    1 +
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_beep.c b/sound/pci/hda/hda_beep.c
> index d26ae65..815dc88 100644
> --- a/sound/pci/hda/hda_beep.c
> +++ b/sound/pci/hda/hda_beep.c
> @@ -231,15 +231,33 @@ void snd_hda_detach_beep_device(struct hda_codec *codec)
>  }
>  EXPORT_SYMBOL_HDA(snd_hda_detach_beep_device);
>  
> +static void verify_amp_has_mute(struct snd_kcontrol *kcontrol)
> +{
> +	unsigned int caps;
> +	int dir;
> +	struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
> +	struct hda_beep *beep = codec->beep;
> +	if (snd_BUG_ON(!beep))
> +		return;
> +
> +	dir = get_amp_direction(kcontrol) == HDA_INPUT ? AC_PAR_AMP_IN_CAP : AC_PAR_AMP_OUT_CAP;
> +	caps = snd_hda_param_read(codec, get_amp_nid(kcontrol), dir);
> +	if (!(caps & AC_AMPCAP_MUTE))
> +		kcontrol->private_value &= 0xffff0000; /* No NID to set */
> +	beep->amp_verified = 1;
> +}
> +
>  /* get/put callbacks for beep mute mixer switches */
>  int snd_hda_mixer_amp_switch_get_beep(struct snd_kcontrol *kcontrol,
>  				      struct snd_ctl_elem_value *ucontrol)
>  {
>  	struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
>  	struct hda_beep *beep = codec->beep;
> -	if (beep && !beep->enabled) {
> +	if (beep && !beep->amp_verified)
> +		verify_amp_has_mute(kcontrol);
> +	if (beep && (!get_amp_nid(kcontrol) || !beep->enabled)) {
>  		ucontrol->value.integer.value[0] =
> -			ucontrol->value.integer.value[1] = 0;
> +			ucontrol->value.integer.value[1] = beep->enabled;
>  		return 0;
>  	}
>  	return snd_hda_mixer_amp_switch_get(kcontrol, ucontrol);
> @@ -262,7 +280,11 @@ int snd_hda_mixer_amp_switch_put_beep(struct snd_kcontrol *kcontrol,
>  		if (chs & 2)
>  			enable |= *valp;
>  		snd_hda_enable_beep_device(codec, enable);
> +		if (!beep->amp_verified)
> +			verify_amp_has_mute(kcontrol);
>  	}
> +	if (!get_amp_nid(kcontrol))
> +		return 0;
>  	return snd_hda_mixer_amp_switch_put(kcontrol, ucontrol);
>  }
>  EXPORT_SYMBOL_HDA(snd_hda_mixer_amp_switch_put_beep);
> diff --git a/sound/pci/hda/hda_beep.h b/sound/pci/hda/hda_beep.h
> index 4dc6933..923e91a 100644
> --- a/sound/pci/hda/hda_beep.h
> +++ b/sound/pci/hda/hda_beep.h
> @@ -36,6 +36,7 @@ struct hda_beep {
>  	hda_nid_t nid;
>  	unsigned int enabled:1;
>  	unsigned int linear_tone:1;	/* linear tone for IDT/STAC codec */
> +	unsigned int amp_verified:1;  /* Have we verified that the node has a mute switch? */
>  	struct work_struct beep_work; /* scheduled task for beep event */
>  	struct mutex mutex;
>  };
> -- 
> 1.7.9.5
>