[alsa-devel] [PATCH] ALSA: hda - Fix 'Beep Playback Switch' with no underlying mute switch
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@canonical.com ---
Another Beep fix. It's not that I'm suddenly obsessed with beeping; it's my automated test tool that finds them :-)
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.
Example alsa info: codecs/canonical/cx20590-lenovo-thinkpad-t420-ccert-201102-7230
sound/pci/hda/hda_beep.c | 6 ++++-- sound/pci/hda/patch_conexant.c | 8 +++++++- 2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/sound/pci/hda/hda_beep.c b/sound/pci/hda/hda_beep.c index d26ae65..3a72543 100644 --- a/sound/pci/hda/hda_beep.c +++ b/sound/pci/hda/hda_beep.c @@ -237,9 +237,9 @@ int snd_hda_mixer_amp_switch_get_beep(struct snd_kcontrol *kcontrol, { struct hda_codec *codec = snd_kcontrol_chip(kcontrol); struct hda_beep *beep = codec->beep; - if (beep && !beep->enabled) { + 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); @@ -263,6 +263,8 @@ int snd_hda_mixer_amp_switch_put_beep(struct snd_kcontrol *kcontrol, enable |= *valp; snd_hda_enable_beep_device(codec, enable); } + 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/patch_conexant.c b/sound/pci/hda/patch_conexant.c index 5e22a8f..fa36c5e 100644 --- a/sound/pci/hda/patch_conexant.c +++ b/sound/pci/hda/patch_conexant.c @@ -537,12 +537,18 @@ static int conexant_build_controls(struct hda_codec *codec) /* create beep controls if needed */ if (spec->beep_amp) { const struct snd_kcontrol_new *knew; + unsigned int caps; + caps = snd_hda_param_read(codec, spec->beep_amp, AC_PAR_AMP_OUT_CAP); + for (knew = cxt_beep_mixer; knew->name; knew++) { struct snd_kcontrol *kctl; kctl = snd_ctl_new1(knew, codec); if (!kctl) return -ENOMEM; - kctl->private_value = spec->beep_amp; + if (knew->info == snd_hda_mixer_amp_switch_info && !(caps & AC_AMPCAP_MUTE)) + kctl->private_value = 0x10000; /* Mono channel, no nid */ + else + kctl->private_value = spec->beep_amp; err = snd_hda_ctl_add(codec, 0, kctl); if (err < 0) return err;
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@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 ;)
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.
thanks,
Takashi
Example alsa info: codecs/canonical/cx20590-lenovo-thinkpad-t420-ccert-201102-7230
sound/pci/hda/hda_beep.c | 6 ++++-- sound/pci/hda/patch_conexant.c | 8 +++++++- 2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/sound/pci/hda/hda_beep.c b/sound/pci/hda/hda_beep.c index d26ae65..3a72543 100644 --- a/sound/pci/hda/hda_beep.c +++ b/sound/pci/hda/hda_beep.c @@ -237,9 +237,9 @@ int snd_hda_mixer_amp_switch_get_beep(struct snd_kcontrol *kcontrol, { struct hda_codec *codec = snd_kcontrol_chip(kcontrol); struct hda_beep *beep = codec->beep;
- if (beep && !beep->enabled) {
- 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;@@ -263,6 +263,8 @@ int snd_hda_mixer_amp_switch_put_beep(struct snd_kcontrol *kcontrol, enable |= *valp; snd_hda_enable_beep_device(codec, enable); }
- if (!get_amp_nid(kcontrol))
return 0;} EXPORT_SYMBOL_HDA(snd_hda_mixer_amp_switch_put_beep); diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c index 5e22a8f..fa36c5e 100644 --- a/sound/pci/hda/patch_conexant.c +++ b/sound/pci/hda/patch_conexant.c @@ -537,12 +537,18 @@ static int conexant_build_controls(struct hda_codec *codec) /* create beep controls if needed */ if (spec->beep_amp) { const struct snd_kcontrol_new *knew;
unsigned int caps;caps = snd_hda_param_read(codec, spec->beep_amp, AC_PAR_AMP_OUT_CAP);- for (knew = cxt_beep_mixer; knew->name; knew++) { struct snd_kcontrol *kctl; kctl = snd_ctl_new1(knew, codec); if (!kctl) return -ENOMEM;
kctl->private_value = spec->beep_amp;
if (knew->info == snd_hda_mixer_amp_switch_info && !(caps & AC_AMPCAP_MUTE))kctl->private_value = 0x10000; /* Mono channel, no nid */elsekctl->private_value = spec->beep_amp; err = snd_hda_ctl_add(codec, 0, kctl); if (err < 0) return err;-- 1.7.9.5
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@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?
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@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@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@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;@@ -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;} 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
On 08/14/2012 09:44 AM, Takashi Iwai wrote:
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:
Ok, third version attached.
At Tue, 14 Aug 2012 10:04:05 +0200, David Henningsson wrote:
On 08/14/2012 09:44 AM, Takashi Iwai wrote:
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:
Ok, third version attached.
Thanks, applied now (with a minor optimization).
Takashi
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
participants (2)
-
David Henningsson -
Takashi Iwai