| Message ID | 20240104155838.7514-1-mail@bernhard-seibold.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: hda: generic: Always add LED controls | expand |
On Thu, 04 Jan 2024 16:58:38 +0100, Bernhard Seibold wrote: > > LEDs for mute and microphone mute can be added to any system via the > audio-mute and audio-micmute triggers, e.g. via USB HID devices. > Therefore, add the LED controls unconditionally. Hmm... Won't your change lead to the sysfs entries always, i.e. creating sysfs entries even if the device has no LED at all? Takashi > > Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de> > --- > sound/pci/hda/hda_generic.c | 46 ++++++++++++++++++------------------- > sound/pci/hda/hda_generic.h | 2 -- > 2 files changed, 22 insertions(+), 26 deletions(-) > > diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c > index bf685d01259d..e78c0dd17c42 100644 > --- a/sound/pci/hda/hda_generic.c > +++ b/sound/pci/hda/hda_generic.c > @@ -3644,8 +3644,7 @@ static int add_single_cap_ctl(struct hda_codec *codec, const char *label, > return -ENOMEM; > if (is_switch) { > knew->put = cap_single_sw_put; > - if (spec->mic_mute_led) > - knew->access |= SNDRV_CTL_ELEM_ACCESS_MIC_LED; > + knew->access |= SNDRV_CTL_ELEM_ACCESS_MIC_LED; > } > if (!inv_dmic) > return 0; > @@ -3663,8 +3662,7 @@ static int add_single_cap_ctl(struct hda_codec *codec, const char *label, > return -ENOMEM; > if (is_switch) { > knew->put = cap_single_sw_put; > - if (spec->mic_mute_led) > - knew->access |= SNDRV_CTL_ELEM_ACCESS_MIC_LED; > + knew->access |= SNDRV_CTL_ELEM_ACCESS_MIC_LED; > } > return 0; > } > @@ -3706,8 +3704,7 @@ static int create_bind_cap_vol_ctl(struct hda_codec *codec, int idx, > knew->index = idx; > knew->private_value = sw_ctl; > knew->subdevice = HDA_SUBDEV_AMP_FLAG; > - if (spec->mic_mute_led) > - knew->access |= SNDRV_CTL_ELEM_ACCESS_MIC_LED; > + knew->access |= SNDRV_CTL_ELEM_ACCESS_MIC_LED; > } > return 0; > } > @@ -3979,7 +3976,6 @@ int snd_hda_gen_add_mute_led_cdev(struct hda_codec *codec, > if (spec->vmaster_mute.hook) > codec_err(codec, "vmaster hook already present before cdev!\n"); > > - spec->vmaster_mute_led = 1; > return 0; > } > EXPORT_SYMBOL_GPL(snd_hda_gen_add_mute_led_cdev); > @@ -4002,7 +3998,6 @@ int snd_hda_gen_add_micmute_led_cdev(struct hda_codec *codec, > int (*callback)(struct led_classdev *, > enum led_brightness)) > { > - struct hda_gen_spec *spec = codec->spec; > int err; > > if (callback) { > @@ -4013,7 +4008,6 @@ int snd_hda_gen_add_micmute_led_cdev(struct hda_codec *codec, > } > } > > - spec->mic_mute_led = 1; > return 0; > } > EXPORT_SYMBOL_GPL(snd_hda_gen_add_micmute_led_cdev); > @@ -4973,8 +4967,7 @@ int snd_hda_gen_parse_auto_config(struct hda_codec *codec, > > parse_user_hints(codec); > > - if (spec->vmaster_mute_led || spec->mic_mute_led) > - snd_ctl_led_request(); > + snd_ctl_led_request(); > > if (spec->mixer_nid && !spec->mixer_merge_nid) > spec->mixer_merge_nid = spec->mixer_nid; > @@ -5173,6 +5166,7 @@ static const char * const follower_pfxs[] = { > int snd_hda_gen_build_controls(struct hda_codec *codec) > { > struct hda_gen_spec *spec = codec->spec; > + struct snd_kcontrol *kctl; > int err; > > if (spec->kctls.used) { > @@ -5211,19 +5205,23 @@ int snd_hda_gen_build_controls(struct hda_codec *codec) > if (err < 0) > return err; > } > - if (!spec->no_analog && !spec->suppress_vmaster && > - !snd_hda_find_mixer_ctl(codec, "Master Playback Switch")) { > - err = __snd_hda_add_vmaster(codec, "Master Playback Switch", > - NULL, follower_pfxs, > - "Playback Switch", true, > - spec->vmaster_mute_led ? > - SNDRV_CTL_ELEM_ACCESS_SPK_LED : 0, > - &spec->vmaster_mute.sw_kctl); > - if (err < 0) > - return err; > - if (spec->vmaster_mute.hook) { > - snd_hda_add_vmaster_hook(codec, &spec->vmaster_mute); > - snd_hda_sync_vmaster_hook(&spec->vmaster_mute); > + if (!spec->no_analog && !spec->suppress_vmaster) { > + > + kctl = snd_hda_find_mixer_ctl(codec, "Master Playback Switch"); > + if (!kctl) { > + err = __snd_hda_add_vmaster(codec, "Master Playback Switch", > + NULL, follower_pfxs, > + "Playback Switch", true, > + SNDRV_CTL_ELEM_ACCESS_SPK_LED, > + &spec->vmaster_mute.sw_kctl); > + if (err < 0) > + return err; > + if (spec->vmaster_mute.hook) { > + snd_hda_add_vmaster_hook(codec, &spec->vmaster_mute); > + snd_hda_sync_vmaster_hook(&spec->vmaster_mute); > + } > + } else { > + kctl->vd[0].access |= SNDRV_CTL_ELEM_ACCESS_SPK_LED; > } > } > > diff --git a/sound/pci/hda/hda_generic.h b/sound/pci/hda/hda_generic.h > index a8eea8367629..13db36528041 100644 > --- a/sound/pci/hda/hda_generic.h > +++ b/sound/pci/hda/hda_generic.h > @@ -223,8 +223,6 @@ struct hda_gen_spec { > unsigned int inv_dmic_split:1; /* inverted dmic w/a for conexant */ > unsigned int own_eapd_ctl:1; /* set EAPD by own function */ > unsigned int keep_eapd_on:1; /* don't turn off EAPD automatically */ > - unsigned int vmaster_mute_led:1; /* add SPK-LED flag to vmaster mute switch */ > - unsigned int mic_mute_led:1; /* add MIC-LED flag to capture mute switch */ > unsigned int indep_hp:1; /* independent HP supported */ > unsigned int prefer_hp_amp:1; /* enable HP amp for speaker if any */ > unsigned int add_stereo_mix_input:2; /* add aamix as a capture src */ > -- > 2.43.0 >
On Thu, Jan 04, 2024 at 05:41:48PM +0100, Takashi Iwai wrote: > On Thu, 04 Jan 2024 16:58:38 +0100, > Bernhard Seibold wrote: > > > > LEDs for mute and microphone mute can be added to any system via the > > audio-mute and audio-micmute triggers, e.g. via USB HID devices. > > Therefore, add the LED controls unconditionally. > > Hmm... Won't your change lead to the sysfs entries always, > i.e. creating sysfs entries even if the device has no LED at all? This patch is not affecting create_mute_led_cdev, that function is still only called for some special systems via fixups, to create the actual LED classdevs that are present on there. What I'm trying to change is to have the led-ctls always available. They serve as input to the audio-mute and audio-micmute triggers, and these triggers should work on any system with HD Audio, not just a few. It would be even better to move the whole functionality into the sound core so that it works with other drivers as well, but since yesterday was the first time I even looked at sound code, I wouldn't even know where to start. Regard, Bernhard
On 04. 01. 24 20:14, Bernhard Seibold wrote: > On Thu, Jan 04, 2024 at 05:41:48PM +0100, Takashi Iwai wrote: >> On Thu, 04 Jan 2024 16:58:38 +0100, >> Bernhard Seibold wrote: >>> >>> LEDs for mute and microphone mute can be added to any system via the >>> audio-mute and audio-micmute triggers, e.g. via USB HID devices. >>> Therefore, add the LED controls unconditionally. >> >> Hmm... Won't your change lead to the sysfs entries always, >> i.e. creating sysfs entries even if the device has no LED at all? > > This patch is not affecting create_mute_led_cdev, that function is still > only called for some special systems via fixups, to create the actual > LED classdevs that are present on there. > > What I'm trying to change is to have the led-ctls always available. They > serve as input to the audio-mute and audio-micmute triggers, and these > triggers should work on any system with HD Audio, not just a few. > > It would be even better to move the whole functionality into the sound > core so that it works with other drivers as well, but since yesterday was > the first time I even looked at sound code, I wouldn't even know where > to start. You can attach controls using sysfs for special use cases. If the system has audio LED driver controlled in other code, the HDA driver should be notified somehow to create triggers, otherwise the default (current) functionality is fine in my eyes. Jaroslav
On Thu, Jan 04, 2024 at 10:46:04PM +0100, Jaroslav Kysela wrote: > On 04. 01. 24 20:14, Bernhard Seibold wrote: > > On Thu, Jan 04, 2024 at 05:41:48PM +0100, Takashi Iwai wrote: > > > On Thu, 04 Jan 2024 16:58:38 +0100, > > > Bernhard Seibold wrote: > > > > > > > > LEDs for mute and microphone mute can be added to any system via the > > > > audio-mute and audio-micmute triggers, e.g. via USB HID devices. > > > > Therefore, add the LED controls unconditionally. > > > > > > Hmm... Won't your change lead to the sysfs entries always, > > > i.e. creating sysfs entries even if the device has no LED at all? > > > > This patch is not affecting create_mute_led_cdev, that function is still > > only called for some special systems via fixups, to create the actual > > LED classdevs that are present on there. > > > > What I'm trying to change is to have the led-ctls always available. They > > serve as input to the audio-mute and audio-micmute triggers, and these > > triggers should work on any system with HD Audio, not just a few. > > > > It would be even better to move the whole functionality into the sound > > core so that it works with other drivers as well, but since yesterday was > > the first time I even looked at sound code, I wouldn't even know where > > to start. > > You can attach controls using sysfs for special use cases. If the system has > audio LED driver controlled in other code, the HDA driver should be notified > somehow to create triggers, otherwise the default (current) functionality is > fine in my eyes. I've got a USB keyboard that has LEDs for mute and for microphone mute, and these currently do not work as expected. I'm not sure how special that use-case is, but I don't think expecting users to manually fiddle around in sysfs to get their keyboard LEDs working is a good solution. Regards, Bernhard
On 05. 01. 24 0:41, Bernhard Seibold wrote: > On Thu, Jan 04, 2024 at 10:46:04PM +0100, Jaroslav Kysela wrote: >> On 04. 01. 24 20:14, Bernhard Seibold wrote: >>> On Thu, Jan 04, 2024 at 05:41:48PM +0100, Takashi Iwai wrote: >>>> On Thu, 04 Jan 2024 16:58:38 +0100, >>>> Bernhard Seibold wrote: >>>>> >>>>> LEDs for mute and microphone mute can be added to any system via the >>>>> audio-mute and audio-micmute triggers, e.g. via USB HID devices. >>>>> Therefore, add the LED controls unconditionally. >>>> >>>> Hmm... Won't your change lead to the sysfs entries always, >>>> i.e. creating sysfs entries even if the device has no LED at all? >>> >>> This patch is not affecting create_mute_led_cdev, that function is still >>> only called for some special systems via fixups, to create the actual >>> LED classdevs that are present on there. >>> >>> What I'm trying to change is to have the led-ctls always available. They >>> serve as input to the audio-mute and audio-micmute triggers, and these >>> triggers should work on any system with HD Audio, not just a few. >>> >>> It would be even better to move the whole functionality into the sound >>> core so that it works with other drivers as well, but since yesterday was >>> the first time I even looked at sound code, I wouldn't even know where >>> to start. >> >> You can attach controls using sysfs for special use cases. If the system has >> audio LED driver controlled in other code, the HDA driver should be notified >> somehow to create triggers, otherwise the default (current) functionality is >> fine in my eyes. > > I've got a USB keyboard that has LEDs for mute and for microphone mute, > and these currently do not work as expected. I'm not sure how special > that use-case is, but I don't think expecting users to manually fiddle > around in sysfs to get their keyboard LEDs working is a good solution. I see your point (and tracked your name to hid-lenovo.c which creates mute/micmute LED drivers for Lenovo keyboards). The question is, if the audio triggers should be enabled blindly (all time) or when a LED driver exists and is active (runtime). In later case, this can be covered using new udev rules. Obviously, no manual configuration from users is expected. Jaroslav
On Fri, Jan 05, 2024 at 10:27:52AM +0100, Jaroslav Kysela wrote: > On 05. 01. 24 0:41, Bernhard Seibold wrote: > > On Thu, Jan 04, 2024 at 10:46:04PM +0100, Jaroslav Kysela wrote: > > > You can attach controls using sysfs for special use cases. If the system has > > > audio LED driver controlled in other code, the HDA driver should be notified > > > somehow to create triggers, otherwise the default (current) functionality is > > > fine in my eyes. > > > > I've got a USB keyboard that has LEDs for mute and for microphone mute, > > and these currently do not work as expected. I'm not sure how special > > that use-case is, but I don't think expecting users to manually fiddle > > around in sysfs to get their keyboard LEDs working is a good solution. > > I see your point (and tracked your name to hid-lenovo.c which creates > mute/micmute LED drivers for Lenovo keyboards). The question is, if the > audio triggers should be enabled blindly (all time) or when a LED driver > exists and is active (runtime). In later case, this can be covered using new > udev rules. Obviously, no manual configuration from users is expected. That would require a rule for every keyboard or other HID device with a mute or micmute LED. I don't think this is a good idea when it could just work out of the box. The HID specification defines "usages" for these LEDs, so any HID device can contain one, and this works also without device-specific drivers. For "mute", an LED classdev is already being created by the input-leds module. The default trigger is not yet set correctly, but I have a patch for that. Support for "micmute" is still missing, but I'm working on that as well.
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c index bf685d01259d..e78c0dd17c42 100644 --- a/sound/pci/hda/hda_generic.c +++ b/sound/pci/hda/hda_generic.c @@ -3644,8 +3644,7 @@ static int add_single_cap_ctl(struct hda_codec *codec, const char *label, return -ENOMEM; if (is_switch) { knew->put = cap_single_sw_put; - if (spec->mic_mute_led) - knew->access |= SNDRV_CTL_ELEM_ACCESS_MIC_LED; + knew->access |= SNDRV_CTL_ELEM_ACCESS_MIC_LED; } if (!inv_dmic) return 0; @@ -3663,8 +3662,7 @@ static int add_single_cap_ctl(struct hda_codec *codec, const char *label, return -ENOMEM; if (is_switch) { knew->put = cap_single_sw_put; - if (spec->mic_mute_led) - knew->access |= SNDRV_CTL_ELEM_ACCESS_MIC_LED; + knew->access |= SNDRV_CTL_ELEM_ACCESS_MIC_LED; } return 0; } @@ -3706,8 +3704,7 @@ static int create_bind_cap_vol_ctl(struct hda_codec *codec, int idx, knew->index = idx; knew->private_value = sw_ctl; knew->subdevice = HDA_SUBDEV_AMP_FLAG; - if (spec->mic_mute_led) - knew->access |= SNDRV_CTL_ELEM_ACCESS_MIC_LED; + knew->access |= SNDRV_CTL_ELEM_ACCESS_MIC_LED; } return 0; } @@ -3979,7 +3976,6 @@ int snd_hda_gen_add_mute_led_cdev(struct hda_codec *codec, if (spec->vmaster_mute.hook) codec_err(codec, "vmaster hook already present before cdev!\n"); - spec->vmaster_mute_led = 1; return 0; } EXPORT_SYMBOL_GPL(snd_hda_gen_add_mute_led_cdev); @@ -4002,7 +3998,6 @@ int snd_hda_gen_add_micmute_led_cdev(struct hda_codec *codec, int (*callback)(struct led_classdev *, enum led_brightness)) { - struct hda_gen_spec *spec = codec->spec; int err; if (callback) { @@ -4013,7 +4008,6 @@ int snd_hda_gen_add_micmute_led_cdev(struct hda_codec *codec, } } - spec->mic_mute_led = 1; return 0; } EXPORT_SYMBOL_GPL(snd_hda_gen_add_micmute_led_cdev); @@ -4973,8 +4967,7 @@ int snd_hda_gen_parse_auto_config(struct hda_codec *codec, parse_user_hints(codec); - if (spec->vmaster_mute_led || spec->mic_mute_led) - snd_ctl_led_request(); + snd_ctl_led_request(); if (spec->mixer_nid && !spec->mixer_merge_nid) spec->mixer_merge_nid = spec->mixer_nid; @@ -5173,6 +5166,7 @@ static const char * const follower_pfxs[] = { int snd_hda_gen_build_controls(struct hda_codec *codec) { struct hda_gen_spec *spec = codec->spec; + struct snd_kcontrol *kctl; int err; if (spec->kctls.used) { @@ -5211,19 +5205,23 @@ int snd_hda_gen_build_controls(struct hda_codec *codec) if (err < 0) return err; } - if (!spec->no_analog && !spec->suppress_vmaster && - !snd_hda_find_mixer_ctl(codec, "Master Playback Switch")) { - err = __snd_hda_add_vmaster(codec, "Master Playback Switch", - NULL, follower_pfxs, - "Playback Switch", true, - spec->vmaster_mute_led ? - SNDRV_CTL_ELEM_ACCESS_SPK_LED : 0, - &spec->vmaster_mute.sw_kctl); - if (err < 0) - return err; - if (spec->vmaster_mute.hook) { - snd_hda_add_vmaster_hook(codec, &spec->vmaster_mute); - snd_hda_sync_vmaster_hook(&spec->vmaster_mute); + if (!spec->no_analog && !spec->suppress_vmaster) { + + kctl = snd_hda_find_mixer_ctl(codec, "Master Playback Switch"); + if (!kctl) { + err = __snd_hda_add_vmaster(codec, "Master Playback Switch", + NULL, follower_pfxs, + "Playback Switch", true, + SNDRV_CTL_ELEM_ACCESS_SPK_LED, + &spec->vmaster_mute.sw_kctl); + if (err < 0) + return err; + if (spec->vmaster_mute.hook) { + snd_hda_add_vmaster_hook(codec, &spec->vmaster_mute); + snd_hda_sync_vmaster_hook(&spec->vmaster_mute); + } + } else { + kctl->vd[0].access |= SNDRV_CTL_ELEM_ACCESS_SPK_LED; } } diff --git a/sound/pci/hda/hda_generic.h b/sound/pci/hda/hda_generic.h index a8eea8367629..13db36528041 100644 --- a/sound/pci/hda/hda_generic.h +++ b/sound/pci/hda/hda_generic.h @@ -223,8 +223,6 @@ struct hda_gen_spec { unsigned int inv_dmic_split:1; /* inverted dmic w/a for conexant */ unsigned int own_eapd_ctl:1; /* set EAPD by own function */ unsigned int keep_eapd_on:1; /* don't turn off EAPD automatically */ - unsigned int vmaster_mute_led:1; /* add SPK-LED flag to vmaster mute switch */ - unsigned int mic_mute_led:1; /* add MIC-LED flag to capture mute switch */ unsigned int indep_hp:1; /* independent HP supported */ unsigned int prefer_hp_amp:1; /* enable HP amp for speaker if any */ unsigned int add_stereo_mix_input:2; /* add aamix as a capture src */
LEDs for mute and microphone mute can be added to any system via the audio-mute and audio-micmute triggers, e.g. via USB HID devices. Therefore, add the LED controls unconditionally. Signed-off-by: Bernhard Seibold <mail@bernhard-seibold.de> --- sound/pci/hda/hda_generic.c | 46 ++++++++++++++++++------------------- sound/pci/hda/hda_generic.h | 2 -- 2 files changed, 22 insertions(+), 26 deletions(-)