[alsa-devel] [PATCH 0/4] ALSA: hda beep clean up
Hi,
this is a series of small cleanups of HD-audio beep code. Since the keyboard driver already handles the simultaneous outputs from the multiple input beep instances, there is no merit to have beep_mode=2 option to switch the beep input device.
Takashi
The beep_mode=2 option was introduced to make the beep mixer controlling the beep input allocation/deallocation dynamically, so that a user can switch between HD-audio codec digital beep and the system beep only via mixer API. This was necessary because the keyboard driver took only the first input beep instance at that time.
However, the recent keyboard driver already processes the multiple input instances, thus there is no point to keep this mode.
Let's remove it.
Signed-off-by: Takashi Iwai tiwai@suse.de --- Documentation/sound/alsa/ALSA-Configuration.txt | 3 +- sound/pci/hda/Kconfig | 7 ++--- sound/pci/hda/hda_beep.c | 37 +---------------------- sound/pci/hda/hda_beep.h | 4 --- sound/pci/hda/hda_intel.c | 6 ++-- 5 files changed, 8 insertions(+), 49 deletions(-)
diff --git a/Documentation/sound/alsa/ALSA-Configuration.txt b/Documentation/sound/alsa/ALSA-Configuration.txt index 221b810..4e4d0bc 100644 --- a/Documentation/sound/alsa/ALSA-Configuration.txt +++ b/Documentation/sound/alsa/ALSA-Configuration.txt @@ -875,8 +875,7 @@ Prior to version 0.9.0rc4 options had a 'snd_' prefix. This was removed. setup before initializing the codecs. This option is available only when CONFIG_SND_HDA_PATCH_LOADER=y is set. See HD-Audio.txt for details. - beep_mode - Selects the beep registration mode (0=off, 1=on, 2= - dynamic registration via mute switch on/off); the default + beep_mode - Selects the beep registration mode (0=off, 1=on); default value is set via CONFIG_SND_HDA_INPUT_BEEP_MODE kconfig.
[Single (global) options] diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index d030797..194d625 100644 --- a/sound/pci/hda/Kconfig +++ b/sound/pci/hda/Kconfig @@ -53,15 +53,14 @@ config SND_HDA_INPUT_BEEP driver. This interface is used to generate digital beeps.
config SND_HDA_INPUT_BEEP_MODE - int "Digital beep registration mode (0=off, 1=on, 2=mute sw on/off)" + int "Digital beep registration mode (0=off, 1=on)" depends on SND_HDA_INPUT_BEEP=y default "1" - range 0 2 + range 0 1 help Set 0 to disable the digital beep interface for HD-audio by default. Set 1 to always enable the digital beep interface for HD-audio by - default. Set 2 to control the beep device registration to input - layer using a "Beep Switch" in mixer applications. + default.
config SND_HDA_INPUT_JACK bool "Support jack plugging notification via input layer" diff --git a/sound/pci/hda/hda_beep.c b/sound/pci/hda/hda_beep.c index 60738e5..662de6e 100644 --- a/sound/pci/hda/hda_beep.c +++ b/sound/pci/hda/hda_beep.c @@ -162,28 +162,6 @@ static int snd_hda_do_attach(struct hda_beep *beep) return 0; }
-static void snd_hda_do_register(struct work_struct *work) -{ - struct hda_beep *beep = - container_of(work, struct hda_beep, register_work); - - mutex_lock(&beep->mutex); - if (beep->enabled && !beep->dev) - snd_hda_do_attach(beep); - mutex_unlock(&beep->mutex); -} - -static void snd_hda_do_unregister(struct work_struct *work) -{ - struct hda_beep *beep = - container_of(work, struct hda_beep, unregister_work.work); - - mutex_lock(&beep->mutex); - if (!beep->enabled && beep->dev) - snd_hda_do_detach(beep); - mutex_unlock(&beep->mutex); -} - int snd_hda_enable_beep_device(struct hda_codec *codec, int enable) { struct hda_beep *beep = codec->beep; @@ -197,15 +175,6 @@ int snd_hda_enable_beep_device(struct hda_codec *codec, int enable) snd_hda_codec_write(beep->codec, beep->nid, 0, AC_VERB_SET_BEEP_CONTROL, 0); } - if (beep->mode == HDA_BEEP_MODE_SWREG) { - if (enable) { - cancel_delayed_work(&beep->unregister_work); - schedule_work(&beep->register_work); - } else { - schedule_delayed_work(&beep->unregister_work, - HZ); - } - } return 1; } return 0; @@ -235,12 +204,10 @@ int snd_hda_attach_beep_device(struct hda_codec *codec, int nid) beep->mode = codec->beep_mode; codec->beep = beep;
- INIT_WORK(&beep->register_work, &snd_hda_do_register); - INIT_DELAYED_WORK(&beep->unregister_work, &snd_hda_do_unregister); INIT_WORK(&beep->beep_work, &snd_hda_generate_beep); mutex_init(&beep->mutex);
- if (beep->mode == HDA_BEEP_MODE_ON) { + if (beep->mode) { int err = snd_hda_do_attach(beep); if (err < 0) { kfree(beep); @@ -257,8 +224,6 @@ void snd_hda_detach_beep_device(struct hda_codec *codec) { struct hda_beep *beep = codec->beep; if (beep) { - cancel_work_sync(&beep->register_work); - cancel_delayed_work(&beep->unregister_work); if (beep->dev) snd_hda_do_detach(beep); codec->beep = NULL; diff --git a/sound/pci/hda/hda_beep.h b/sound/pci/hda/hda_beep.h index 55f0647..30e79d1 100644 --- a/sound/pci/hda/hda_beep.h +++ b/sound/pci/hda/hda_beep.h @@ -26,7 +26,6 @@
#define HDA_BEEP_MODE_OFF 0 #define HDA_BEEP_MODE_ON 1 -#define HDA_BEEP_MODE_SWREG 2
/* beep information */ struct hda_beep { @@ -37,10 +36,7 @@ struct hda_beep { int tone; hda_nid_t nid; unsigned int enabled:1; - unsigned int request_enable:1; unsigned int linear_tone:1; /* linear tone for IDT/STAC codec */ - struct work_struct register_work; /* registration work */ - struct delayed_work unregister_work; /* unregistration work */ struct work_struct beep_work; /* scheduled task for beep event */ struct mutex mutex; }; diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 7757536..334c0ba 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -72,7 +72,7 @@ static int enable_msi = -1; static char *patch[SNDRV_CARDS]; #endif #ifdef CONFIG_SND_HDA_INPUT_BEEP -static int beep_mode[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] = +static bool beep_mode[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] = CONFIG_SND_HDA_INPUT_BEEP_MODE}; #endif
@@ -103,9 +103,9 @@ module_param_array(patch, charp, NULL, 0444); MODULE_PARM_DESC(patch, "Patch file for Intel HD audio interface."); #endif #ifdef CONFIG_SND_HDA_INPUT_BEEP -module_param_array(beep_mode, int, NULL, 0444); +module_param_array(beep_mode, bool, NULL, 0444); MODULE_PARM_DESC(beep_mode, "Select HDA Beep registration mode " - "(0=off, 1=on, 2=mute switch on/off) (default=1)."); + "(0=off, 1=on) (default=1)."); #endif
#ifdef CONFIG_SND_HDA_POWER_SAVE
Move snd_hda_mixer_amp_switch_put_beep() to hda_beep.c as a clean up to remove one more ifdef.
Also add the corresponding get callback to return consistently the digital beep state independently from the mixer amp value.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_beep.c | 28 ++++++++++++++++++++++++++++ sound/pci/hda/hda_codec.c | 19 ------------------- sound/pci/hda/hda_local.h | 4 +++- 3 files changed, 31 insertions(+), 20 deletions(-)
diff --git a/sound/pci/hda/hda_beep.c b/sound/pci/hda/hda_beep.c index 662de6e..336b4b3 100644 --- a/sound/pci/hda/hda_beep.c +++ b/sound/pci/hda/hda_beep.c @@ -231,3 +231,31 @@ void snd_hda_detach_beep_device(struct hda_codec *codec) } } EXPORT_SYMBOL_HDA(snd_hda_detach_beep_device); + +/* 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) { + ucontrol->value.integer.value[0] = + ucontrol->value.integer.value[1] = + beep->enabled; + return 0; + } + return snd_hda_mixer_amp_switch_get(kcontrol, ucontrol); +} +EXPORT_SYMBOL_HDA(snd_hda_mixer_amp_switch_get_beep); + +int snd_hda_mixer_amp_switch_put_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) + snd_hda_enable_beep_device(codec, + *ucontrol->value.integer.value); + 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_codec.c b/sound/pci/hda/hda_codec.c index 51cb2a2..ddac428 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -2676,25 +2676,6 @@ int snd_hda_mixer_amp_switch_put(struct snd_kcontrol *kcontrol, } EXPORT_SYMBOL_HDA(snd_hda_mixer_amp_switch_put);
-#ifdef CONFIG_SND_HDA_INPUT_BEEP -/** - * snd_hda_mixer_amp_switch_put_beep - Put callback for a beep AMP switch - * - * This function calls snd_hda_enable_beep_device(), which behaves differently - * depending on beep_mode option. - */ -int snd_hda_mixer_amp_switch_put_beep(struct snd_kcontrol *kcontrol, - struct snd_ctl_elem_value *ucontrol) -{ - struct hda_codec *codec = snd_kcontrol_chip(kcontrol); - long *valp = ucontrol->value.integer.value; - - snd_hda_enable_beep_device(codec, *valp); - return snd_hda_mixer_amp_switch_put(kcontrol, ucontrol); -} -EXPORT_SYMBOL_HDA(snd_hda_mixer_amp_switch_put_beep); -#endif /* CONFIG_SND_HDA_INPUT_BEEP */ - /* * bound volume controls * diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 9a096a8..1b4c129 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -89,7 +89,7 @@ { .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, .index = xcidx, \ .subdevice = HDA_SUBDEV_AMP_FLAG, \ .info = snd_hda_mixer_amp_switch_info, \ - .get = snd_hda_mixer_amp_switch_get, \ + .get = snd_hda_mixer_amp_switch_get_beep, \ .put = snd_hda_mixer_amp_switch_put_beep, \ .private_value = HDA_COMPOSE_AMP_VAL(nid, channel, xindex, direction) } #else @@ -121,6 +121,8 @@ int snd_hda_mixer_amp_switch_get(struct snd_kcontrol *kcontrol, int snd_hda_mixer_amp_switch_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol); #ifdef CONFIG_SND_HDA_INPUT_BEEP +int snd_hda_mixer_amp_switch_get_beep(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol); int snd_hda_mixer_amp_switch_put_beep(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol); #endif
It's no longer necessary since beep_mode=2 option was dropped. It can be checked simply via codec->beep != NULL.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_beep.c | 14 ++++++-------- sound/pci/hda/hda_beep.h | 1 - 2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/sound/pci/hda/hda_beep.c b/sound/pci/hda/hda_beep.c index 336b4b3..e6cf2a2 100644 --- a/sound/pci/hda/hda_beep.c +++ b/sound/pci/hda/hda_beep.c @@ -184,6 +184,7 @@ EXPORT_SYMBOL_HDA(snd_hda_enable_beep_device); int snd_hda_attach_beep_device(struct hda_codec *codec, int nid) { struct hda_beep *beep; + int err;
if (!snd_hda_get_bool_hint(codec, "beep")) return 0; /* disabled explicitly by hints */ @@ -201,19 +202,16 @@ int snd_hda_attach_beep_device(struct hda_codec *codec, int nid)
beep->nid = nid; beep->codec = codec; - beep->mode = codec->beep_mode; codec->beep = beep;
INIT_WORK(&beep->beep_work, &snd_hda_generate_beep); mutex_init(&beep->mutex);
- if (beep->mode) { - int err = snd_hda_do_attach(beep); - if (err < 0) { - kfree(beep); - codec->beep = NULL; - return err; - } + err = snd_hda_do_attach(beep); + if (err < 0) { + kfree(beep); + codec->beep = NULL; + return err; }
return 0; diff --git a/sound/pci/hda/hda_beep.h b/sound/pci/hda/hda_beep.h index 30e79d1..4dc6933 100644 --- a/sound/pci/hda/hda_beep.h +++ b/sound/pci/hda/hda_beep.h @@ -31,7 +31,6 @@ struct hda_beep { struct input_dev *dev; struct hda_codec *codec; - unsigned int mode; char phys[32]; int tone; hda_nid_t nid;
Call cancel_work_sync() when turning off the beep switch so that the mute call is executed in a proper order.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_beep.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/hda_beep.c b/sound/pci/hda/hda_beep.c index e6cf2a2..0bc2315 100644 --- a/sound/pci/hda/hda_beep.c +++ b/sound/pci/hda/hda_beep.c @@ -165,12 +165,13 @@ static int snd_hda_do_attach(struct hda_beep *beep) int snd_hda_enable_beep_device(struct hda_codec *codec, int enable) { struct hda_beep *beep = codec->beep; - enable = !!enable; - if (beep == NULL) + if (!beep) return 0; + enable = !!enable; if (beep->enabled != enable) { beep->enabled = enable; if (!enable) { + cancel_work_sync(&beep->beep_work); /* turn off beep */ snd_hda_codec_write(beep->codec, beep->nid, 0, AC_VERB_SET_BEEP_CONTROL, 0);
Date 3.7.2012 18:03, Takashi Iwai wrote:
Hi,
this is a series of small cleanups of HD-audio beep code. Since the keyboard driver already handles the simultaneous outputs from the multiple input beep instances, there is no merit to have beep_mode=2 option to switch the beep input device.
Acked-by: Jaroslav Kysela perex@perex.cz
Date 3.7.2012 18:31, Jaroslav Kysela wrote:
Date 3.7.2012 18:03, Takashi Iwai wrote:
Hi,
this is a series of small cleanups of HD-audio beep code. Since the keyboard driver already handles the simultaneous outputs from the multiple input beep instances, there is no merit to have beep_mode=2 option to switch the beep input device.
Acked-by: Jaroslav Kysela perex@perex.cz
While I acked this, I vaguely remember now, that the event duplication (simultaneous outputs) caused some issues on some Lenovo notebooks.
When the pcspkr and HDA beep generator is connected to the same output (integrated speakers), it may cause some issues (bad beep quality - frequency etc.).
I looked to the input code, and the kd_mksound() calls input_handler_for_each_handle(), thus the event is duplicated to all beep generators, right?
The beep_mode=2 was introduced to let users to help with this issue with a comfortable way.
Jaroslav
At Tue, 03 Jul 2012 22:01:45 +0200, Jaroslav Kysela wrote:
Date 3.7.2012 18:31, Jaroslav Kysela wrote:
Date 3.7.2012 18:03, Takashi Iwai wrote:
Hi,
this is a series of small cleanups of HD-audio beep code. Since the keyboard driver already handles the simultaneous outputs from the multiple input beep instances, there is no merit to have beep_mode=2 option to switch the beep input device.
Acked-by: Jaroslav Kysela perex@perex.cz
While I acked this, I vaguely remember now, that the event duplication (simultaneous outputs) caused some issues on some Lenovo notebooks.
When the pcspkr and HDA beep generator is connected to the same output (integrated speakers), it may cause some issues (bad beep quality - frequency etc.).
I looked to the input code, and the kd_mksound() calls input_handler_for_each_handle(), thus the event is duplicated to all beep generators, right?
Right.
The beep_mode=2 was introduced to let users to help with this issue with a comfortable way.
Not really. At the time this was introduced, the keyboard driver took only a single input handle exclusively, namely only the last registered one. Thus playing on both had never happened until recently.
Takashi
Date 4.7.2012 07:16, Takashi Iwai wrote:
At Tue, 03 Jul 2012 22:01:45 +0200, Jaroslav Kysela wrote:
Date 3.7.2012 18:31, Jaroslav Kysela wrote:
Date 3.7.2012 18:03, Takashi Iwai wrote:
Hi,
this is a series of small cleanups of HD-audio beep code. Since the keyboard driver already handles the simultaneous outputs from the multiple input beep instances, there is no merit to have beep_mode=2 option to switch the beep input device.
Acked-by: Jaroslav Kysela perex@perex.cz
While I acked this, I vaguely remember now, that the event duplication (simultaneous outputs) caused some issues on some Lenovo notebooks.
When the pcspkr and HDA beep generator is connected to the same output (integrated speakers), it may cause some issues (bad beep quality - frequency etc.).
I looked to the input code, and the kd_mksound() calls input_handler_for_each_handle(), thus the event is duplicated to all beep generators, right?
Right.
The beep_mode=2 was introduced to let users to help with this issue with a comfortable way.
Not really. At the time this was introduced, the keyboard driver took only a single input handle exclusively, namely only the last registered one. Thus playing on both had never happened until recently.
OK, thanks for the clarification. The question is, if it's the desired behaviour. Multiple outs can cause some harmonic effects. It would be nice, if the input layer can turn on/off (filter) the beep events at runtime separately for all registered drivers. The setup can be handled using sysfs or so..
Jaroslav
At Wed, 04 Jul 2012 09:43:23 +0200, Jaroslav Kysela wrote:
Date 4.7.2012 07:16, Takashi Iwai wrote:
At Tue, 03 Jul 2012 22:01:45 +0200, Jaroslav Kysela wrote:
Date 3.7.2012 18:31, Jaroslav Kysela wrote:
Date 3.7.2012 18:03, Takashi Iwai wrote:
Hi,
this is a series of small cleanups of HD-audio beep code. Since the keyboard driver already handles the simultaneous outputs from the multiple input beep instances, there is no merit to have beep_mode=2 option to switch the beep input device.
Acked-by: Jaroslav Kysela perex@perex.cz
While I acked this, I vaguely remember now, that the event duplication (simultaneous outputs) caused some issues on some Lenovo notebooks.
When the pcspkr and HDA beep generator is connected to the same output (integrated speakers), it may cause some issues (bad beep quality - frequency etc.).
I looked to the input code, and the kd_mksound() calls input_handler_for_each_handle(), thus the event is duplicated to all beep generators, right?
Right.
The beep_mode=2 was introduced to let users to help with this issue with a comfortable way.
Not really. At the time this was introduced, the keyboard driver took only a single input handle exclusively, namely only the last registered one. Thus playing on both had never happened until recently.
OK, thanks for the clarification. The question is, if it's the desired behaviour. Multiple outs can cause some harmonic effects. It would be nice, if the input layer can turn on/off (filter) the beep events at runtime separately for all registered drivers. The setup can be handled using sysfs or so..
Yeah, it's an issue in the input layer, or maybe specific to keyboard driver. It should be relatively easy to implement.
Meanwhile, you can simply mute "Beep" volume. That is effectively as same as deregistering the HD-audio input handle. The beep code in HD-audio driver doesn't emit the verb any longer when it's muted.
Getting rid of the system beep is another question. But it could be done via setterm or whatever, too.
Takashi
participants (2)
-
Jaroslav Kysela -
Takashi Iwai