[PATCH] [RFC] ALSA: control - add generic LED API to sound core routines
[DO NOT MERGE!] [This is just an early proposal for comments] [The code is not tested / debugged]
The recent laptops have usually two LEDs assigned to reflect the speaker and microphone mute state. This implementation adds a tiny layer on top of the control API which calculates the state for those LEDs using the driver callbacks.
Two new access flags are introduced to describe the controls which affects the audio path settings (an easy path for drivers).
The LED resource can be shared with multiple sound cards with this code. The user space controls may be added to the state chain, too.
This code should replace the LED code in the HDA driver and add a possibility to easy extend the other drivers (ASoC codecs etc.).
Note: The snd_ctl_notify_one() changes should be moved to a separate patch. I will do this, when this proposal is agreed.
Signed-off-by: Jaroslav Kysela perex@perex.cz Cc: Perry Yuan Perry.Yuan@dell.com Cc: Hans de Goede hdegoede@redhat.com --- include/sound/control.h | 12 +++ include/uapi/sound/asound.h | 2 + sound/core/Kconfig | 6 ++ sound/core/Makefile | 1 + sound/core/control.c | 83 ++++++++++++++------- sound/core/control_led.c | 144 ++++++++++++++++++++++++++++++++++++ sound/pci/hda/Kconfig | 3 +- 7 files changed, 225 insertions(+), 26 deletions(-) create mode 100644 sound/core/control_led.c
diff --git a/include/sound/control.h b/include/sound/control.h index 77d9fa10812d..ed967b18ffc9 100644 --- a/include/sound/control.h +++ b/include/sound/control.h @@ -261,4 +261,16 @@ snd_kctl_jack_new(const char *name, struct snd_card *card); void snd_kctl_jack_report(struct snd_card *card, struct snd_kcontrol *kctl, bool status);
+#if IS_ENABLED(CONFIG_SND_LED) +void snd_ctl_led_notify(struct snd_card *card, unsigned int mask, + struct snd_kcontrol *kctl, unsigned int ioff); +void snd_ctl_led_register(struct snd_card *card); +void snd_ctl_led_disconnect(struct snd_card *card); +#else +static inline void snd_ctl_led_notify(struct snd_card *card, unsigned int mask, + struct snd_kcontrol *kctl, unsigned int ioff) {} +static inline void snd_ctl_led_register(struct snd_card *card) {} +static inline void snd_ctl_led_disconnect(struct snd_card *card) {} +#endif + #endif /* __SOUND_CONTROL_H */ diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 535a7229e1d9..3faacdf5eaea 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -982,6 +982,8 @@ typedef int __bitwise snd_ctl_elem_iface_t; #define SNDRV_CTL_ELEM_ACCESS_INACTIVE (1<<8) /* control does actually nothing, but may be updated */ #define SNDRV_CTL_ELEM_ACCESS_LOCK (1<<9) /* write lock */ #define SNDRV_CTL_ELEM_ACCESS_OWNER (1<<10) /* write lock owner */ +#define SNDRV_CTL_ELEM_ACCESS_SPK_LED (1<<11) /* speaker (output) LED flag */ +#define SNDRV_CTL_ELEM_ACCESS_MIC_LED (1<<12) /* microphone (input) LED flag */ #define SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK (1<<28) /* kernel use a TLV callback */ #define SNDRV_CTL_ELEM_ACCESS_USER (1<<29) /* user space element */ /* bits 30 and 31 are obsoleted (for indirect access) */ diff --git a/sound/core/Kconfig b/sound/core/Kconfig index a4050f87f230..1bde494fa1fe 100644 --- a/sound/core/Kconfig +++ b/sound/core/Kconfig @@ -203,4 +203,10 @@ config SND_DMA_SGBUF def_bool y depends on X86
+config SND_LED + bool + select NEW_LEDS if SND_LED + select LEDS_TRIGGERS if SND_LED + select LEDS_TRIGGER_AUDIO if SND_LED + source "sound/core/seq/Kconfig" diff --git a/sound/core/Makefile b/sound/core/Makefile index ee4a4a6b99ba..81b33877a03d 100644 --- a/sound/core/Makefile +++ b/sound/core/Makefile @@ -13,6 +13,7 @@ snd-$(CONFIG_ISA_DMA_API) += isadma.o snd-$(CONFIG_SND_OSSEMUL) += sound_oss.o snd-$(CONFIG_SND_VMASTER) += vmaster.o snd-$(CONFIG_SND_JACK) += ctljack.o jack.o +snd-$(CONFIG_SND_LED) += control_led.o
snd-pcm-y := pcm.o pcm_native.o pcm_lib.o pcm_misc.o \ pcm_memory.o memalloc.o diff --git a/sound/core/control.c b/sound/core/control.c index 5165741a8400..3171e7f2798e 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -181,6 +181,28 @@ void snd_ctl_notify(struct snd_card *card, unsigned int mask, } EXPORT_SYMBOL(snd_ctl_notify);
+/** + * snd_ctl_notify_one - Send notification to user-space for a control change + * @card: the card to send notification + * @mask: the event mask, SNDRV_CTL_EVENT_* + * @kctl: the pointer with the control instance + * @ioff: the additional offset to the control index + * + * This function calls snd_ctl_notify() and does additional jobs + * like LED state changes. + */ +void snd_ctl_notify_one(struct snd_card *card, unsigned int mask, + struct snd_kcontrol *kctl, unsigned int ioff) +{ + struct snd_ctl_elem_id id = kctl->id; + + id.index += ioff; + id.numid += ioff; + snd_ctl_led_notify(card, mask, kctl, ioff); + snd_ctl_notify(card, mask, &id); +} +EXPORT_SYMBOL(snd_ctl_notify_one); + /** * snd_ctl_new - create a new control instance with some elements * @kctl: the pointer to store new control instance @@ -250,6 +272,8 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol, SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE | SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND | SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK | + SNDRV_CTL_ELEM_ACCESS_SPK_LED | + SNDRV_CTL_ELEM_ACCESS_MIC_LED | SNDRV_CTL_ELEM_ACCESS_SKIP_CHECK);
err = snd_ctl_new(&kctl, count, access, NULL); @@ -342,7 +366,6 @@ static int __snd_ctl_add_replace(struct snd_card *card, { struct snd_ctl_elem_id id; unsigned int idx; - unsigned int count; struct snd_kcontrol *old; int err;
@@ -376,10 +399,8 @@ static int __snd_ctl_add_replace(struct snd_card *card, kcontrol->id.numid = card->last_numid + 1; card->last_numid += kcontrol->count;
- id = kcontrol->id; - count = kcontrol->count; - for (idx = 0; idx < count; idx++, id.index++, id.numid++) - snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id); + for (idx = 0; idx < kcontrol->count; idx++) + snd_ctl_notify_one(card, SNDRV_CTL_EVENT_MASK_ADD, kcontrol, idx);
return 0; } @@ -462,16 +483,15 @@ EXPORT_SYMBOL(snd_ctl_replace); */ int snd_ctl_remove(struct snd_card *card, struct snd_kcontrol *kcontrol) { - struct snd_ctl_elem_id id; unsigned int idx;
if (snd_BUG_ON(!card || !kcontrol)) return -EINVAL; list_del(&kcontrol->list); card->controls_count -= kcontrol->count; - id = kcontrol->id; - for (idx = 0; idx < kcontrol->count; idx++, id.index++, id.numid++) - snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_REMOVE, &id); + + for (idx = 0; idx < kcontrol->count; idx++) + snd_ctl_notify_one(card, SNDRV_CTL_EVENT_MASK_REMOVE, kcontrol, idx); snd_ctl_free_one(kcontrol); return 0; } @@ -584,11 +604,13 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id, vd->access |= SNDRV_CTL_ELEM_ACCESS_INACTIVE; } snd_ctl_build_ioff(id, kctl, index_offset); - ret = 1; + downgrade_write(&card->controls_rwsem); + snd_ctl_notify_one(card, SNDRV_CTL_EVENT_MASK_INFO, kctl, index_offset); + up_read(&card->controls_rwsem); + return 1; + unlock: up_write(&card->controls_rwsem); - if (ret > 0) - snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id); return ret; } EXPORT_SYMBOL_GPL(snd_ctl_activate_id); @@ -1110,25 +1132,34 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file, unsigned int index_offset; int result;
+ down_write(&card->controls_rwsem); kctl = snd_ctl_find_id(card, &control->id); - if (kctl == NULL) + if (kctl == NULL) { + up_write(&card->controls_rwsem); return -ENOENT; + }
index_offset = snd_ctl_get_ioff(kctl, &control->id); vd = &kctl->vd[index_offset]; if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_WRITE) || kctl->put == NULL || (file && vd->owner && vd->owner != file)) { + up_write(&card->controls_rwsem); return -EPERM; }
snd_ctl_build_ioff(&control->id, kctl, index_offset); result = kctl->put(kctl, control); - if (result < 0) + if (result < 0) { + up_write(&card->controls_rwsem); return result; + }
if (result > 0) { - struct snd_ctl_elem_id id = control->id; - snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &id); + downgrade_write(&card->controls_rwsem); + snd_ctl_notify_one(card, SNDRV_CTL_EVENT_MASK_VALUE, kctl, index_offset); + up_read(&card->controls_rwsem); + } else { + up_write(&card->controls_rwsem); }
return 0; @@ -1150,9 +1181,7 @@ static int snd_ctl_elem_write_user(struct snd_ctl_file *file, if (result < 0) goto error;
- down_write(&card->controls_rwsem); result = snd_ctl_elem_write(card, file, control); - up_write(&card->controls_rwsem); if (result < 0) goto error;
@@ -1301,7 +1330,6 @@ static int replace_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf, { struct user_element *ue = kctl->private_data; unsigned int *container; - struct snd_ctl_elem_id id; unsigned int mask = 0; int i; int change; @@ -1333,10 +1361,8 @@ static int replace_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf, ue->tlv_data_size = size;
mask |= SNDRV_CTL_EVENT_MASK_TLV; - for (i = 0; i < kctl->count; ++i) { - snd_ctl_build_ioff(&id, kctl, i); - snd_ctl_notify(ue->card, mask, &id); - } + for (i = 0; i < kctl->count; ++i) + snd_ctl_notify_one(ue->card, mask, kctl, i);
return change; } @@ -1998,9 +2024,14 @@ static const struct file_operations snd_ctl_f_ops = static int snd_ctl_dev_register(struct snd_device *device) { struct snd_card *card = device->device_data; + int err;
- return snd_register_device(SNDRV_DEVICE_TYPE_CONTROL, card, -1, - &snd_ctl_f_ops, card, &card->ctl_dev); + err = snd_register_device(SNDRV_DEVICE_TYPE_CONTROL, card, -1, + &snd_ctl_f_ops, card, &card->ctl_dev); + if (err < 0) + return err; + snd_ctl_led_register(card); + return 0; }
/* @@ -2019,6 +2050,8 @@ static int snd_ctl_dev_disconnect(struct snd_device *device) } read_unlock_irqrestore(&card->ctl_files_rwlock, flags);
+ snd_ctl_led_disconnect(card); + return snd_unregister_device(&card->ctl_dev); }
diff --git a/sound/core/control_led.c b/sound/core/control_led.c new file mode 100644 index 000000000000..e70855ea54d1 --- /dev/null +++ b/sound/core/control_led.c @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * LED state routines for driver control interface + * Copyright (c) 2021 by Jaroslav Kysela perex@perex.cz + */ + +#include <linux/leds.h> +#include <sound/core.h> +#include <sound/control.h> + +static DEFINE_MUTEX(snd_control_led_mutex); +static unsigned int snd_control_led_last_card; +static struct snd_card *snd_control_led_cards[SNDRV_CARDS]; + +#define UPDATE_ROUTE(route, cb) \ + do { \ + int route2 = (cb); \ + if (route2 >= 0) \ + route = route < 0 ? route2 : (route | route2); \ + } while (0) + +static int snd_ctl_led_get(struct snd_card *card, + struct snd_kcontrol *kctl, unsigned int ioff) +{ + struct snd_ctl_elem_info info; + struct snd_ctl_elem_value value; + unsigned int i; + int result; + + memset(&info, 0, sizeof(info)); + info.id = kctl->id; + info.id.index += ioff; + info.id.numid += ioff; + result = kctl->info(kctl, &info); + if (result < 0) + return -1; + memset(&value, 0, sizeof(value)); + value.id = info.id; + result = kctl->get(kctl, &value); + if (result < 0) + return -1; + if (info.type == SNDRV_CTL_ELEM_TYPE_BOOLEAN || + info.type == SNDRV_CTL_ELEM_TYPE_INTEGER) { + for (i = 0; i < info.count; i++) + if (value.value.integer.value[i] != info.value.integer.min) + return 1; + } else if (info.type == SNDRV_CTL_ELEM_TYPE_INTEGER64) { + for (i = 0; i < info.count; i++) + if (value.value.integer64.value[i] != info.value.integer64.min) + return 1; + } + return 0; +} + +static int snd_ctl_led_set_state_for_card(int card_number, unsigned int group) +{ + struct snd_card *card = snd_control_led_cards[card_number]; + struct snd_kcontrol *kctl; + struct snd_kcontrol_volatile *vd; + unsigned int ioff; + int route = -1; + + down_read(&card->controls_rwsem); + list_for_each_entry(kctl, &card->controls, list) { + for (ioff = 0; ioff < kctl->count; ioff++) { + vd = &kctl->vd[ioff]; + if (vd->access & group) + UPDATE_ROUTE(route, snd_ctl_led_get(card, kctl, ioff)); + } + } + up_read(&card->controls_rwsem); + return route; +} + +static void snd_ctl_led_set_state(struct snd_card *card, unsigned int group) +{ + int card_number; + enum led_audio led_trigger_type; + int route; + + if (group == SNDRV_CTL_ELEM_ACCESS_SPK_LED) { + led_trigger_type = LED_AUDIO_MUTE; + } else if (group == SNDRV_CTL_ELEM_ACCESS_MIC_LED) { + led_trigger_type = LED_AUDIO_MICMUTE; + } else { + return; + } + route = -1; + mutex_lock(&snd_control_led_mutex); + /* the card may not be registered (active) at this point */ + if (snd_control_led_cards[card->number] == NULL) { + mutex_unlock(&snd_control_led_mutex); + return; + } + for (card_number = 0; card_number <= snd_control_led_last_card; card_number++) + UPDATE_ROUTE(route, snd_ctl_led_set_state_for_card(card_number, group)); + mutex_unlock(&snd_control_led_mutex); + if (route >= 0) + ledtrig_audio_set(led_trigger_type, route ? LED_OFF : LED_ON); + +} + +void snd_ctl_led_notify(struct snd_card *card, unsigned int mask, + struct snd_kcontrol *kctl, unsigned int ioff) +{ + if (mask == SNDRV_CTL_EVENT_MASK_REMOVE || + (mask & (SNDRV_CTL_EVENT_MASK_INFO | + SNDRV_CTL_EVENT_MASK_ADD | + SNDRV_CTL_EVENT_MASK_VALUE)) != 0) { + struct snd_kcontrol_volatile *vd = &kctl->vd[ioff]; + if (vd->access & SNDRV_CTL_ELEM_ACCESS_SPK_LED) + snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_SPK_LED); + if (vd->access & SNDRV_CTL_ELEM_ACCESS_MIC_LED) + snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_MIC_LED); + } +} + +void snd_ctl_led_register(struct snd_card *card) +{ + if (snd_BUG_ON(card->number < 0 || + card->number >= ARRAY_SIZE(snd_control_led_cards))) + return; + mutex_lock(&snd_control_led_mutex); + snd_control_led_cards[card->number] = card; + if (card->number > snd_control_led_last_card) + snd_control_led_last_card = card->number; + mutex_unlock(&snd_control_led_mutex); + snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_SPK_LED); + snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_MIC_LED); +} + +void snd_ctl_led_disconnect(struct snd_card *card) +{ + mutex_lock(&snd_control_led_mutex); + if (snd_control_led_last_card == card->number) { + while (snd_control_led_last_card > 0) + if (snd_control_led_cards[--snd_control_led_last_card]) + break; + } + snd_control_led_cards[card->number] = NULL; + mutex_unlock(&snd_control_led_mutex); + snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_SPK_LED); + snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_MIC_LED); +} diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index 90759391cbac..ccf788ede8ba 100644 --- a/sound/pci/hda/Kconfig +++ b/sound/pci/hda/Kconfig @@ -9,7 +9,8 @@ config SND_HDA select SND_HDA_CORE
config SND_HDA_GENERIC_LEDS - bool + bool + select SND_LED # just for test, ignore, please!
config SND_HDA_INTEL tristate "HD Audio PCI"
Hi Jaroslav,
On 2/7/21 9:11 PM, Jaroslav Kysela wrote:
[DO NOT MERGE!] [This is just an early proposal for comments] [The code is not tested / debugged]
The recent laptops have usually two LEDs assigned to reflect the speaker and microphone mute state. This implementation adds a tiny layer on top of the control API which calculates the state for those LEDs using the driver callbacks.
Two new access flags are introduced to describe the controls which affects the audio path settings (an easy path for drivers).
The LED resource can be shared with multiple sound cards with this code. The user space controls may be added to the state chain, too.
This code should replace the LED code in the HDA driver and add a possibility to easy extend the other drivers (ASoC codecs etc.).
Note: The snd_ctl_notify_one() changes should be moved to a separate patch. I will do this, when this proposal is agreed.
Signed-off-by: Jaroslav Kysela perex@perex.cz Cc: Perry Yuan Perry.Yuan@dell.com Cc: Hans de Goede hdegoede@redhat.com
Nice I think that having this handled in a generic manner would be a great improvement.
I have 2 HP x2 models which are Bay Trail resp. Cherry Trail based, which means they use Intel's LPE (Low Power Engine) for audio which uses ASoC.
These come with detachable USB keyboard-docks which have a speaker mute LED. I have already added LED-class support to the HID driver for these, but I never got around to hooking up a trigger from the ASoC code.
So if I understand things correctly, then if I add this patch to a kernel and in the ASoC codec driver add SNDRV_CTL_ELEM_ACCESS_SPK_LED to one or more of the controls, and add .default_trigger = "audio-mute" to the led_class_dev for the LED, then if I toggle the control on / off in alsamixer this should then control the LED ?
If I got that right I will certainly give this a try.
Any advice for which control would be the best to use? Looking at the code, it assumes that if a control is enabled that then there will be a route, which makes sense if there are e.g. headphones and speaker and lineout controls. But what about chained controls, e.g. separate volume + mute controls or multiple volume controls chained. With ASoC this is not unheard of. I guess the answer is to pick the control which we will also want to use for hw volume-control if/when UCM profiles grow hw volume-control support ?
Regards,
Hans
p.s. This would only / at least add proper support for these LEDs at the kernel level. ATM these devices which use UCM profiles don't actually use any kind of hw volume-control. So we would also need to fix that (in userspace) to get these LEDs to really work when an user hits the mute key, or lowers the volume to NIL.
include/sound/control.h | 12 +++ include/uapi/sound/asound.h | 2 + sound/core/Kconfig | 6 ++ sound/core/Makefile | 1 + sound/core/control.c | 83 ++++++++++++++------- sound/core/control_led.c | 144 ++++++++++++++++++++++++++++++++++++ sound/pci/hda/Kconfig | 3 +- 7 files changed, 225 insertions(+), 26 deletions(-) create mode 100644 sound/core/control_led.c
diff --git a/include/sound/control.h b/include/sound/control.h index 77d9fa10812d..ed967b18ffc9 100644 --- a/include/sound/control.h +++ b/include/sound/control.h @@ -261,4 +261,16 @@ snd_kctl_jack_new(const char *name, struct snd_card *card); void snd_kctl_jack_report(struct snd_card *card, struct snd_kcontrol *kctl, bool status);
+#if IS_ENABLED(CONFIG_SND_LED) +void snd_ctl_led_notify(struct snd_card *card, unsigned int mask,
struct snd_kcontrol *kctl, unsigned int ioff);+void snd_ctl_led_register(struct snd_card *card); +void snd_ctl_led_disconnect(struct snd_card *card); +#else +static inline void snd_ctl_led_notify(struct snd_card *card, unsigned int mask,
struct snd_kcontrol *kctl, unsigned int ioff) {}+static inline void snd_ctl_led_register(struct snd_card *card) {} +static inline void snd_ctl_led_disconnect(struct snd_card *card) {} +#endif
#endif /* __SOUND_CONTROL_H */ diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 535a7229e1d9..3faacdf5eaea 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -982,6 +982,8 @@ typedef int __bitwise snd_ctl_elem_iface_t; #define SNDRV_CTL_ELEM_ACCESS_INACTIVE (1<<8) /* control does actually nothing, but may be updated */ #define SNDRV_CTL_ELEM_ACCESS_LOCK (1<<9) /* write lock */ #define SNDRV_CTL_ELEM_ACCESS_OWNER (1<<10) /* write lock owner */ +#define SNDRV_CTL_ELEM_ACCESS_SPK_LED (1<<11) /* speaker (output) LED flag */ +#define SNDRV_CTL_ELEM_ACCESS_MIC_LED (1<<12) /* microphone (input) LED flag */ #define SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK (1<<28) /* kernel use a TLV callback */ #define SNDRV_CTL_ELEM_ACCESS_USER (1<<29) /* user space element */ /* bits 30 and 31 are obsoleted (for indirect access) */ diff --git a/sound/core/Kconfig b/sound/core/Kconfig index a4050f87f230..1bde494fa1fe 100644 --- a/sound/core/Kconfig +++ b/sound/core/Kconfig @@ -203,4 +203,10 @@ config SND_DMA_SGBUF def_bool y depends on X86
+config SND_LED
- bool
- select NEW_LEDS if SND_LED
- select LEDS_TRIGGERS if SND_LED
- select LEDS_TRIGGER_AUDIO if SND_LED
source "sound/core/seq/Kconfig" diff --git a/sound/core/Makefile b/sound/core/Makefile index ee4a4a6b99ba..81b33877a03d 100644 --- a/sound/core/Makefile +++ b/sound/core/Makefile @@ -13,6 +13,7 @@ snd-$(CONFIG_ISA_DMA_API) += isadma.o snd-$(CONFIG_SND_OSSEMUL) += sound_oss.o snd-$(CONFIG_SND_VMASTER) += vmaster.o snd-$(CONFIG_SND_JACK) += ctljack.o jack.o +snd-$(CONFIG_SND_LED) += control_led.o
snd-pcm-y := pcm.o pcm_native.o pcm_lib.o pcm_misc.o \ pcm_memory.o memalloc.o diff --git a/sound/core/control.c b/sound/core/control.c index 5165741a8400..3171e7f2798e 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -181,6 +181,28 @@ void snd_ctl_notify(struct snd_card *card, unsigned int mask, } EXPORT_SYMBOL(snd_ctl_notify);
+/**
- snd_ctl_notify_one - Send notification to user-space for a control change
- @card: the card to send notification
- @mask: the event mask, SNDRV_CTL_EVENT_*
- @kctl: the pointer with the control instance
- @ioff: the additional offset to the control index
- This function calls snd_ctl_notify() and does additional jobs
- like LED state changes.
- */
+void snd_ctl_notify_one(struct snd_card *card, unsigned int mask,
struct snd_kcontrol *kctl, unsigned int ioff)+{
- struct snd_ctl_elem_id id = kctl->id;
- id.index += ioff;
- id.numid += ioff;
- snd_ctl_led_notify(card, mask, kctl, ioff);
- snd_ctl_notify(card, mask, &id);
+} +EXPORT_SYMBOL(snd_ctl_notify_one);
/**
- snd_ctl_new - create a new control instance with some elements
- @kctl: the pointer to store new control instance
@@ -250,6 +272,8 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol, SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE | SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND | SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK |
SNDRV_CTL_ELEM_ACCESS_SPK_LED |SNDRV_CTL_ELEM_ACCESS_MIC_LED |SNDRV_CTL_ELEM_ACCESS_SKIP_CHECK);
err = snd_ctl_new(&kctl, count, access, NULL);
@@ -342,7 +366,6 @@ static int __snd_ctl_add_replace(struct snd_card *card, { struct snd_ctl_elem_id id; unsigned int idx;
- unsigned int count; struct snd_kcontrol *old; int err;
@@ -376,10 +399,8 @@ static int __snd_ctl_add_replace(struct snd_card *card, kcontrol->id.numid = card->last_numid + 1; card->last_numid += kcontrol->count;
- id = kcontrol->id;
- count = kcontrol->count;
- for (idx = 0; idx < count; idx++, id.index++, id.numid++)
snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id);
for (idx = 0; idx < kcontrol->count; idx++)
snd_ctl_notify_one(card, SNDRV_CTL_EVENT_MASK_ADD, kcontrol, idx);return 0;
} @@ -462,16 +483,15 @@ EXPORT_SYMBOL(snd_ctl_replace); */ int snd_ctl_remove(struct snd_card *card, struct snd_kcontrol *kcontrol) {
struct snd_ctl_elem_id id; unsigned int idx;
if (snd_BUG_ON(!card || !kcontrol)) return -EINVAL; list_del(&kcontrol->list); card->controls_count -= kcontrol->count;
id = kcontrol->id;
for (idx = 0; idx < kcontrol->count; idx++, id.index++, id.numid++)
snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_REMOVE, &id);
- for (idx = 0; idx < kcontrol->count; idx++)
snd_ctl_notify_one(card, SNDRV_CTL_EVENT_MASK_REMOVE, kcontrol, idx);} @@ -584,11 +604,13 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id, vd->access |= SNDRV_CTL_ELEM_ACCESS_INACTIVE; } snd_ctl_build_ioff(id, kctl, index_offset);
- ret = 1;
- downgrade_write(&card->controls_rwsem);
- snd_ctl_notify_one(card, SNDRV_CTL_EVENT_MASK_INFO, kctl, index_offset);
- up_read(&card->controls_rwsem);
- return 1;
- unlock: up_write(&card->controls_rwsem);
- if (ret > 0)
snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);} EXPORT_SYMBOL_GPL(snd_ctl_activate_id); @@ -1110,25 +1132,34 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file, unsigned int index_offset; int result;
- down_write(&card->controls_rwsem); kctl = snd_ctl_find_id(card, &control->id);
- if (kctl == NULL)
if (kctl == NULL) {
up_write(&card->controls_rwsem);return -ENOENT;
}
index_offset = snd_ctl_get_ioff(kctl, &control->id); vd = &kctl->vd[index_offset]; if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_WRITE) || kctl->put == NULL || (file && vd->owner && vd->owner != file)) {
up_write(&card->controls_rwsem);return -EPERM; }
snd_ctl_build_ioff(&control->id, kctl, index_offset); result = kctl->put(kctl, control);
- if (result < 0)
if (result < 0) {
up_write(&card->controls_rwsem);return result;
}
if (result > 0) {
struct snd_ctl_elem_id id = control->id;snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &id);
downgrade_write(&card->controls_rwsem);snd_ctl_notify_one(card, SNDRV_CTL_EVENT_MASK_VALUE, kctl, index_offset);up_read(&card->controls_rwsem);} else {
up_write(&card->controls_rwsem);}
return 0;
@@ -1150,9 +1181,7 @@ static int snd_ctl_elem_write_user(struct snd_ctl_file *file, if (result < 0) goto error;
- down_write(&card->controls_rwsem); result = snd_ctl_elem_write(card, file, control);
- up_write(&card->controls_rwsem); if (result < 0) goto error;
@@ -1301,7 +1330,6 @@ static int replace_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf, { struct user_element *ue = kctl->private_data; unsigned int *container;
- struct snd_ctl_elem_id id; unsigned int mask = 0; int i; int change;
@@ -1333,10 +1361,8 @@ static int replace_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf, ue->tlv_data_size = size;
mask |= SNDRV_CTL_EVENT_MASK_TLV;
- for (i = 0; i < kctl->count; ++i) {
snd_ctl_build_ioff(&id, kctl, i);snd_ctl_notify(ue->card, mask, &id);- }
for (i = 0; i < kctl->count; ++i)
snd_ctl_notify_one(ue->card, mask, kctl, i);return change;
} @@ -1998,9 +2024,14 @@ static const struct file_operations snd_ctl_f_ops = static int snd_ctl_dev_register(struct snd_device *device) { struct snd_card *card = device->device_data;
- int err;
- return snd_register_device(SNDRV_DEVICE_TYPE_CONTROL, card, -1,
&snd_ctl_f_ops, card, &card->ctl_dev);
- err = snd_register_device(SNDRV_DEVICE_TYPE_CONTROL, card, -1,
&snd_ctl_f_ops, card, &card->ctl_dev);- if (err < 0)
return err;- snd_ctl_led_register(card);
- return 0;
}
/* @@ -2019,6 +2050,8 @@ static int snd_ctl_dev_disconnect(struct snd_device *device) } read_unlock_irqrestore(&card->ctl_files_rwlock, flags);
- snd_ctl_led_disconnect(card);
- return snd_unregister_device(&card->ctl_dev);
}
diff --git a/sound/core/control_led.c b/sound/core/control_led.c new file mode 100644 index 000000000000..e70855ea54d1 --- /dev/null +++ b/sound/core/control_led.c @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/*
- LED state routines for driver control interface
- Copyright (c) 2021 by Jaroslav Kysela perex@perex.cz
- */
+#include <linux/leds.h> +#include <sound/core.h> +#include <sound/control.h>
+static DEFINE_MUTEX(snd_control_led_mutex); +static unsigned int snd_control_led_last_card; +static struct snd_card *snd_control_led_cards[SNDRV_CARDS];
+#define UPDATE_ROUTE(route, cb) \
- do { \
int route2 = (cb); \if (route2 >= 0) \route = route < 0 ? route2 : (route | route2); \- } while (0)
+static int snd_ctl_led_get(struct snd_card *card,
struct snd_kcontrol *kctl, unsigned int ioff)+{
- struct snd_ctl_elem_info info;
- struct snd_ctl_elem_value value;
- unsigned int i;
- int result;
- memset(&info, 0, sizeof(info));
- info.id = kctl->id;
- info.id.index += ioff;
- info.id.numid += ioff;
- result = kctl->info(kctl, &info);
- if (result < 0)
return -1;- memset(&value, 0, sizeof(value));
- value.id = info.id;
- result = kctl->get(kctl, &value);
- if (result < 0)
return -1;- if (info.type == SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
info.type == SNDRV_CTL_ELEM_TYPE_INTEGER) {for (i = 0; i < info.count; i++)if (value.value.integer.value[i] != info.value.integer.min)return 1;- } else if (info.type == SNDRV_CTL_ELEM_TYPE_INTEGER64) {
for (i = 0; i < info.count; i++)if (value.value.integer64.value[i] != info.value.integer64.min)return 1;- }
- return 0;
+}
+static int snd_ctl_led_set_state_for_card(int card_number, unsigned int group) +{
- struct snd_card *card = snd_control_led_cards[card_number];
- struct snd_kcontrol *kctl;
- struct snd_kcontrol_volatile *vd;
- unsigned int ioff;
- int route = -1;
- down_read(&card->controls_rwsem);
- list_for_each_entry(kctl, &card->controls, list) {
for (ioff = 0; ioff < kctl->count; ioff++) {vd = &kctl->vd[ioff];if (vd->access & group)UPDATE_ROUTE(route, snd_ctl_led_get(card, kctl, ioff));}- }
- up_read(&card->controls_rwsem);
- return route;
+}
+static void snd_ctl_led_set_state(struct snd_card *card, unsigned int group) +{
- int card_number;
- enum led_audio led_trigger_type;
- int route;
- if (group == SNDRV_CTL_ELEM_ACCESS_SPK_LED) {
led_trigger_type = LED_AUDIO_MUTE;- } else if (group == SNDRV_CTL_ELEM_ACCESS_MIC_LED) {
led_trigger_type = LED_AUDIO_MICMUTE;- } else {
return;- }
- route = -1;
- mutex_lock(&snd_control_led_mutex);
- /* the card may not be registered (active) at this point */
- if (snd_control_led_cards[card->number] == NULL) {
mutex_unlock(&snd_control_led_mutex);return;- }
- for (card_number = 0; card_number <= snd_control_led_last_card; card_number++)
UPDATE_ROUTE(route, snd_ctl_led_set_state_for_card(card_number, group));- mutex_unlock(&snd_control_led_mutex);
- if (route >= 0)
ledtrig_audio_set(led_trigger_type, route ? LED_OFF : LED_ON);+}
+void snd_ctl_led_notify(struct snd_card *card, unsigned int mask,
struct snd_kcontrol *kctl, unsigned int ioff)+{
- if (mask == SNDRV_CTL_EVENT_MASK_REMOVE ||
(mask & (SNDRV_CTL_EVENT_MASK_INFO |SNDRV_CTL_EVENT_MASK_ADD |SNDRV_CTL_EVENT_MASK_VALUE)) != 0) {struct snd_kcontrol_volatile *vd = &kctl->vd[ioff];if (vd->access & SNDRV_CTL_ELEM_ACCESS_SPK_LED)snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_SPK_LED);if (vd->access & SNDRV_CTL_ELEM_ACCESS_MIC_LED)snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_MIC_LED);- }
+}
+void snd_ctl_led_register(struct snd_card *card) +{
- if (snd_BUG_ON(card->number < 0 ||
card->number >= ARRAY_SIZE(snd_control_led_cards)))return;- mutex_lock(&snd_control_led_mutex);
- snd_control_led_cards[card->number] = card;
- if (card->number > snd_control_led_last_card)
snd_control_led_last_card = card->number;- mutex_unlock(&snd_control_led_mutex);
- snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_SPK_LED);
- snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_MIC_LED);
+}
+void snd_ctl_led_disconnect(struct snd_card *card) +{
- mutex_lock(&snd_control_led_mutex);
- if (snd_control_led_last_card == card->number) {
while (snd_control_led_last_card > 0)if (snd_control_led_cards[--snd_control_led_last_card])break;- }
- snd_control_led_cards[card->number] = NULL;
- mutex_unlock(&snd_control_led_mutex);
- snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_SPK_LED);
- snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_MIC_LED);
+} diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index 90759391cbac..ccf788ede8ba 100644 --- a/sound/pci/hda/Kconfig +++ b/sound/pci/hda/Kconfig @@ -9,7 +9,8 @@ config SND_HDA select SND_HDA_CORE
config SND_HDA_GENERIC_LEDS
bool
- bool
- select SND_LED # just for test, ignore, please!
config SND_HDA_INTEL tristate "HD Audio PCI"
Dne 07. 02. 21 v 21:52 Hans de Goede napsal(a):
Hi Jaroslav,
On 2/7/21 9:11 PM, Jaroslav Kysela wrote:
[DO NOT MERGE!] [This is just an early proposal for comments] [The code is not tested / debugged]
The recent laptops have usually two LEDs assigned to reflect the speaker and microphone mute state. This implementation adds a tiny layer on top of the control API which calculates the state for those LEDs using the driver callbacks.
Two new access flags are introduced to describe the controls which affects the audio path settings (an easy path for drivers).
The LED resource can be shared with multiple sound cards with this code. The user space controls may be added to the state chain, too.
This code should replace the LED code in the HDA driver and add a possibility to easy extend the other drivers (ASoC codecs etc.).
Note: The snd_ctl_notify_one() changes should be moved to a separate patch. I will do this, when this proposal is agreed.
Signed-off-by: Jaroslav Kysela perex@perex.cz Cc: Perry Yuan Perry.Yuan@dell.com Cc: Hans de Goede hdegoede@redhat.com
Nice I think that having this handled in a generic manner would be a great improvement.
I have 2 HP x2 models which are Bay Trail resp. Cherry Trail based, which means they use Intel's LPE (Low Power Engine) for audio which uses ASoC.
These come with detachable USB keyboard-docks which have a speaker mute LED. I have already added LED-class support to the HID driver for these, but I never got around to hooking up a trigger from the ASoC code.
So if I understand things correctly, then if I add this patch to a kernel and in the ASoC codec driver add SNDRV_CTL_ELEM_ACCESS_SPK_LED to one or more of the controls, and add .default_trigger = "audio-mute" to the led_class_dev for the LED, then if I toggle the control on / off in alsamixer this should then control the LED ?
Yes, the ALSA control code should do just the LED trigger. The LED class driver is a separate thing.
If I got that right I will certainly give this a try.
Any advice for which control would be the best to use? Looking at the code, it assumes that if a control is enabled that then there will be a route, which makes sense if there are e.g. headphones and speaker and lineout controls. But what about chained controls, e.g. separate volume + mute controls or multiple volume controls chained. With ASoC this is not unheard of. I guess the answer is to pick the control which we will also want to use for hw volume-control if/when UCM profiles grow hw volume-control support ?
The proposed implementation just goes through all marked controls and if any of the marked controls is set to value greater than the minimal value, then the path is evaluated as "on" (thus the mute LED should be set to off).
The controls tied to the PCM stream (DAC / DMA) should be marked. The physical inputs behind a multiplexer without a "global" volume / switch controls will require a bit different handling. I would just start with something simple and we can add the more complex things on demand.
Regards,
Hans
p.s. This would only / at least add proper support for these LEDs at the kernel level. ATM these devices which use UCM profiles don't actually use any kind of hw volume-control. So we would also need to fix that (in userspace) to get these LEDs to really work when an user hits the mute key, or lowers the volume to NIL.
There's already the hw volume support in UCM and in PA (I added this to PA 14.0). Some drivers like SOF HDA / SoundWire already set the PlaybackMixerElem / CaptureMixerElem values. It's true that the hw volume controls are missing for the Intel SST hardware.
The similar situation is for the SoundWire codecs (missing LED controls, missing hw volume configuration in UCM).
Jaroslav
Hi Jaroslav
-----Original Message----- From: Jaroslav Kysela perex@perex.cz Sent: 2021年2月8日 19:42 To: Hans de Goede; ALSA development Cc: Takashi Iwai; Yuan, Perry Subject: Re: [PATCH] [RFC] ALSA: control - add generic LED API to sound core routines
[EXTERNAL EMAIL]
Dne 07. 02. 21 v 21:52 Hans de Goede napsal(a):
Hi Jaroslav,
On 2/7/21 9:11 PM, Jaroslav Kysela wrote:
[DO NOT MERGE!] [This is just an early proposal for comments] [The code is not tested / debugged]
The recent laptops have usually two LEDs assigned to reflect the speaker and microphone mute state. This implementation adds a tiny layer on top of the control API which calculates the state for those LEDs using the driver callbacks.
Two new access flags are introduced to describe the controls which affects the audio path settings (an easy path for drivers).
The LED resource can be shared with multiple sound cards with this code. The user space controls may be added to the state chain, too.
This code should replace the LED code in the HDA driver and add a possibility to easy extend the other drivers (ASoC codecs etc.).
Note: The snd_ctl_notify_one() changes should be moved to a separate patch. I will do this, when this proposal is agreed.
Signed-off-by: Jaroslav Kysela perex@perex.cz Cc: Perry Yuan Perry.Yuan@dell.com Cc: Hans de Goede hdegoede@redhat.com
Nice I think that having this handled in a generic manner would be a great improvement.
I have 2 HP x2 models which are Bay Trail resp. Cherry Trail based, which means they use Intel's LPE (Low Power Engine) for audio which uses ASoC.
These come with detachable USB keyboard-docks which have a speaker mute LED. I have already added LED-class support to the HID driver for these, but I never got around to hooking up a trigger from the ASoC code.
So if I understand things correctly, then if I add this patch to a kernel and in the ASoC codec driver add
SNDRV_CTL_ELEM_ACCESS_SPK_LED
to one or more of the controls, and add .default_trigger = "audio-mute" to the led_class_dev for the LED, then if I toggle the control on / off in alsamixer this should then control the LED ?
Yes, the ALSA control code should do just the LED trigger. The LED class driver is a separate thing.
If I got that right I will certainly give this a try.
Any advice for which control would be the best to use? Looking at the code, it assumes that if a control is enabled that then there will be a route, which makes sense if there are e.g. headphones and speaker and lineout controls. But what about chained controls, e.g. separate volume + mute controls or multiple volume controls chained. With ASoC this is not unheard of. I guess the answer is to pick the control which we will also want to use for hw volume-control if/when UCM profiles grow hw volume-control support ?
The proposed implementation just goes through all marked controls and if any of the marked controls is set to value greater than the minimal value, then the path is evaluated as "on" (thus the mute LED should be set to off).
The controls tied to the PCM stream (DAC / DMA) should be marked. The physical inputs behind a multiplexer without a "global" volume / switch controls will require a bit different handling. I would just start with something simple and we can add the more complex things on demand.
Regards,
Hans
p.s. This would only / at least add proper support for these LEDs at the kernel level. ATM these devices which use UCM profiles don't actually use any kind of hw volume-control. So we would also need to fix that (in userspace) to get these LEDs to really work when an user hits the mute key, or lowers the volume to NIL.
There's already the hw volume support in UCM and in PA (I added this to PA 14.0). Some drivers like SOF HDA / SoundWire already set the PlaybackMixerElem / CaptureMixerElem values. It's true that the hw volume controls are missing for the Intel SST hardware.
The similar situation is for the SoundWire codecs (missing LED controls, missing hw volume configuration in UCM).
Jaroslav-- Jaroslav Kysela perex@perex.cz Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
Will this new audio led control implementation be supporting SOF/Soundwire subsystem ? If so, I can help to do some testing rebase my privacy driver which needs the audio led control.
Perry
On Sun, 07 Feb 2021 21:11:57 +0100, Jaroslav Kysela wrote:
[DO NOT MERGE!] [This is just an early proposal for comments] [The code is not tested / debugged]
The recent laptops have usually two LEDs assigned to reflect the speaker and microphone mute state. This implementation adds a tiny layer on top of the control API which calculates the state for those LEDs using the driver callbacks.
Two new access flags are introduced to describe the controls which affects the audio path settings (an easy path for drivers).
The LED resource can be shared with multiple sound cards with this code. The user space controls may be added to the state chain, too.
This code should replace the LED code in the HDA driver and add a possibility to easy extend the other drivers (ASoC codecs etc.).
Having a common helper in the ALSA core sounds like a good way to go.
My slight concern is that this will end up having the dependency on LEDS stuff unconditionally when CONFIG_SND_LED=y.
Also, are those new access flags exposed to user-space intentionally, so that user-space gets some information?
Last but not least: I'm not sure whether we should limit to only two LEDs (mic and spk). I'm afraid that there will be more LEDs in future; people love decorations :)
thanks,
Takashi
Dne 08. 02. 21 v 16:11 Takashi Iwai napsal(a):
On Sun, 07 Feb 2021 21:11:57 +0100, Jaroslav Kysela wrote:
[DO NOT MERGE!] [This is just an early proposal for comments] [The code is not tested / debugged]
The recent laptops have usually two LEDs assigned to reflect the speaker and microphone mute state. This implementation adds a tiny layer on top of the control API which calculates the state for those LEDs using the driver callbacks.
Two new access flags are introduced to describe the controls which affects the audio path settings (an easy path for drivers).
The LED resource can be shared with multiple sound cards with this code. The user space controls may be added to the state chain, too.
This code should replace the LED code in the HDA driver and add a possibility to easy extend the other drivers (ASoC codecs etc.).
Having a common helper in the ALSA core sounds like a good way to go.
My slight concern is that this will end up having the dependency on LEDS stuff unconditionally when CONFIG_SND_LED=y.
You probably mean that the LEDs subsystem is activated even if we don't have audio LED class driver connected, right?
I think that the HDA way (select conditionally the LED code) in the low-level driver Kconfig is good, but I'm open for any other suggestions.
Also, are those new access flags exposed to user-space intentionally, so that user-space gets some information?
Yes, it's one benefit, the second benefit is that we can create user space controls for hardware which does not have any switch / volume controls for the given path.
An example is the AMD ACP bridge with the simple digital microphones. We can use alsa-lib's softvol plugin to control the volume for this and it would be nice to mark this user space control with the mic mute LED flag.
Last but not least: I'm not sure whether we should limit to only two LEDs (mic and spk). I'm afraid that there will be more LEDs in future; people love decorations :)
We have some more free bits in the access field. If the LED count will increase in future for the standard hardware, we should reconsider the implementation (info callback or so).
Perhaps, it may be clever to reserve three bits from the access flags now (to create a three bit value not a mask). In this case, we can carry information for 7 LED types (assuming that one control element can be assigned only to one LED type).
Jaroslav
On Mon, 08 Feb 2021 17:17:29 +0100, Jaroslav Kysela wrote:
Dne 08. 02. 21 v 16:11 Takashi Iwai napsal(a):
On Sun, 07 Feb 2021 21:11:57 +0100, Jaroslav Kysela wrote:
[DO NOT MERGE!] [This is just an early proposal for comments] [The code is not tested / debugged]
The recent laptops have usually two LEDs assigned to reflect the speaker and microphone mute state. This implementation adds a tiny layer on top of the control API which calculates the state for those LEDs using the driver callbacks.
Two new access flags are introduced to describe the controls which affects the audio path settings (an easy path for drivers).
The LED resource can be shared with multiple sound cards with this code. The user space controls may be added to the state chain, too.
This code should replace the LED code in the HDA driver and add a possibility to easy extend the other drivers (ASoC codecs etc.).
Having a common helper in the ALSA core sounds like a good way to go.
My slight concern is that this will end up having the dependency on LEDS stuff unconditionally when CONFIG_SND_LED=y.
You probably mean that the LEDs subsystem is activated even if we don't have audio LED class driver connected, right?
Yes.
I think that the HDA way (select conditionally the LED code) in the low-level driver Kconfig is good, but I'm open for any other suggestions.
Well, in the case of HD-audio, it's only for HD-audio. But with this change, LEDS class will be always loaded on distro kernels no matter which sound driver is actually used.
I guess we can split the LED-support code to another module? The problem would be the registration from the control core. The other parts look already isolated enough.
Also, are those new access flags exposed to user-space intentionally, so that user-space gets some information?
Yes, it's one benefit, the second benefit is that we can create user space controls for hardware which does not have any switch / volume controls for the given path.
An example is the AMD ACP bridge with the simple digital microphones. We can use alsa-lib's softvol plugin to control the volume for this and it would be nice to mark this user space control with the mic mute LED flag.
OK, makes sense.
Last but not least: I'm not sure whether we should limit to only two LEDs (mic and spk). I'm afraid that there will be more LEDs in future; people love decorations :)
We have some more free bits in the access field. If the LED count will increase in future for the standard hardware, we should reconsider the implementation (info callback or so).
Perhaps, it may be clever to reserve three bits from the access flags now (to create a three bit value not a mask). In this case, we can carry information for 7 LED types (assuming that one control element can be assigned only to one LED type).
Sounds like a good idea. I guess 4 types would suffice for now.
thanks,
Takashi
Hi,
On Mon, Feb 08, 2021 at 05:33:02PM +0100, Takashi Iwai wrote:
Also, are those new access flags exposed to user-space intentionally, so that user-space gets some information?
Yes, it's one benefit, the second benefit is that we can create user space controls for hardware which does not have any switch / volume controls for the given path.
An example is the AMD ACP bridge with the simple digital microphones. We can use alsa-lib's softvol plugin to control the volume for this and it would be nice to mark this user space control with the mic mute LED flag.
OK, makes sense.
I have a concern about the usage of access flag for such kind of hardware specific stuffs (LED dedicated to specific audio control) since it's not enough hardware abstraction.
In my opinion, for the case, developers for in-kernel driver tend to use specific name for control elements (or prefix/suffix of the name). Adding new access flags for it seems to be overengineering against the original purpose.
The patch itself includes some remarkable ideas that: - introduction of association between control elements - analyzing current status of the association (then operate LEDs) - communication to userspace stuffs about the association
(here I carefully avoid usage of word 'topology'.)
The association itself seems to be useful when cooperating use case manager of alsa-lib. I guess that the kind of framework designed for the association is preferable instead of the patch tight-coupled to LED stuffs. (And some subsystem already attempts to implement such framework into kernel land, e.g. media controller devices in media subsystem.)
In another side, I guess that the reason to supply the association to kernel land is to use 'ledtrig_audio_set()' kernel API. If userspace stuffs find target LEDs and operate them via userspace interface, the association could be in userspace. I think it better to investigate for the direction since I can imagine that the introduction of association to kernel land brings much codes into kernel land to support wide-variety of hardware (and going to be obsoleted according to lifetime of actual hardware sooner or later).
Regards
Takashi Sakamoto
On Mon, 08 Feb 2021 23:34:43 +0100, Takashi Sakamoto wrote:
Hi,
On Mon, Feb 08, 2021 at 05:33:02PM +0100, Takashi Iwai wrote:
Also, are those new access flags exposed to user-space intentionally, so that user-space gets some information?
Yes, it's one benefit, the second benefit is that we can create user space controls for hardware which does not have any switch / volume controls for the given path.
An example is the AMD ACP bridge with the simple digital microphones. We can use alsa-lib's softvol plugin to control the volume for this and it would be nice to mark this user space control with the mic mute LED flag.
OK, makes sense.
I have a concern about the usage of access flag for such kind of hardware specific stuffs (LED dedicated to specific audio control) since it's not enough hardware abstraction.
In my opinion, for the case, developers for in-kernel driver tend to use specific name for control elements (or prefix/suffix of the name). Adding new access flags for it seems to be overengineering against the original purpose.
The patch itself includes some remarkable ideas that:
- introduction of association between control elements
- analyzing current status of the association (then operate LEDs)
- communication to userspace stuffs about the association
(here I carefully avoid usage of word 'topology'.)
The association itself seems to be useful when cooperating use case manager of alsa-lib. I guess that the kind of framework designed for the association is preferable instead of the patch tight-coupled to LED stuffs. (And some subsystem already attempts to implement such framework into kernel land, e.g. media controller devices in media subsystem.)
In another side, I guess that the reason to supply the association to kernel land is to use 'ledtrig_audio_set()' kernel API. If userspace stuffs find target LEDs and operate them via userspace interface, the association could be in userspace. I think it better to investigate for the direction since I can imagine that the introduction of association to kernel land brings much codes into kernel land to support wide-variety of hardware (and going to be obsoleted according to lifetime of actual hardware sooner or later).
Sakamoto-san's comments made me reconsidering of the situation again. The user-space access like via sysfs was my original idea when the mic mute LED issue came up for AMD ACP driver in the past. One problem is the permission. The r/w control over sysfs is for root, and we want for a normal user. This might be solvable via loginctl or such and adding the dynamic permission via ACL. I didn't investigate enough yet.
Takashi
Dne 08. 02. 21 v 23:34 Takashi Sakamoto napsal(a):
Hi,
On Mon, Feb 08, 2021 at 05:33:02PM +0100, Takashi Iwai wrote:
Also, are those new access flags exposed to user-space intentionally, so that user-space gets some information?
Yes, it's one benefit, the second benefit is that we can create user space controls for hardware which does not have any switch / volume controls for the given path.
An example is the AMD ACP bridge with the simple digital microphones. We can use alsa-lib's softvol plugin to control the volume for this and it would be nice to mark this user space control with the mic mute LED flag.
OK, makes sense.
I have a concern about the usage of access flag for such kind of hardware specific stuffs (LED dedicated to specific audio control) since it's not enough hardware abstraction.
In my opinion, for the case, developers for in-kernel driver tend to use specific name for control elements (or prefix/suffix of the name). Adding new access flags for it seems to be overengineering against the original purpose.
Unfortunately, the ASoC drivers do not take care about any abstract naming. They mostly follow hw (codec) register naming from datasheets. So this rule is no longer true.
The patch itself includes some remarkable ideas that:
- introduction of association between control elements
- analyzing current status of the association (then operate LEDs)
- communication to userspace stuffs about the association
(here I carefully avoid usage of word 'topology'.)
The association itself seems to be useful when cooperating use case manager of alsa-lib. I guess that the kind of framework designed for the association is preferable instead of the patch tight-coupled to LED stuffs. (And some subsystem already attempts to implement such framework into kernel land, e.g. media controller devices in media subsystem.)
In another side, I guess that the reason to supply the association to kernel land is to use 'ledtrig_audio_set()' kernel API. If userspace stuffs find target LEDs and operate them via userspace interface, the association could be in userspace. I think it better to investigate for the direction since I can imagine that the introduction of association to kernel land brings much codes into kernel land to support wide-variety of hardware (and going to be obsoleted according to lifetime of actual hardware sooner or later).
My goal is to:
1) reduce the code required to the LED support in the drivers 2) handle security - see the Takashi follow up; for very secure kernel configurations, the user space (non-root) should not "touch" the LED settings at all to have the right feedback; also UCM is a bit another layer on top of the other APIs
The user space solution was already a bit denied when the LED support was added to the HDA driver (we can see the similarity for the vmaster code which is another layer in the control code etc.).
I almost finished the complete implementation in the separate kernel module and the current snd.ko (control) code has minimal modifications just to redirect the necessary things to make the LED layer operational. It seems that we can save the code in the HDA driver and we can do really light changes in other drivers (set flags and do module load) just to get LED working without any user space intervention.
If we come with another framework or solution in the future, we can remove this layer (it's just one small file / module). At least, we will have marked (the new access flags) the related controls / drivers / hardware.
Jaroslav
participants (5)
-
Hans de Goede -
Jaroslav Kysela -
Takashi Iwai -
Takashi Sakamoto -
Yuan, Perry