[PATCH v4 0/6] ALSA: control - add generic LED API
This patchset tries to resolve the diversity in the audio LED control among the ALSA drivers. A new control layer registration is introduced which allows to run additional operations on top of the elementary ALSA sound controls.
A new control access group (three bits in the access flags) was introduced to carry the LED group information for the sound controls. The low-level sound drivers can just mark those controls using this access group. This information is not exported to the user space, but user space can manage the LED sound control associations through sysfs (last patch) per Mark's request. It makes things fully configurable in the kernel and user space (UCM).
The actual state ('route') evaluation is really easy (the minimal value check for all channels / controls / cards). If there's more complicated logic for a given hardware, the card driver may eventually export a new read-only sound control for the LED group and do the logic itself.
The new LED trigger control code is completely separated and possibly optional (there's no symbol dependency). The full code separation allows eventually to move this LED trigger control to the user space in future. Actually it replaces the already present functionality in the kernel space (HDA drivers) and allows a quick adoption for the recent hardware (ASoC codecs including SoundWire).
# lsmod | grep snd_ctl_led snd_ctl_led 24576 0
The sound driver implementation is really easy:
1) call snd_ctl_led_request() when control LED layer should be automatically activated / it calls module_request("snd-ctl-led") on demand / 2) mark all related kcontrols with SNDRV_CTL_ELEM_ACCESS_SPK_LED or SNDRV_CTL_ELEM_ACCESS_MIC_LED
v4 changes: - the LED access flags are private to kernel now (no user space API change) - fixes (kctl management, sysfs cleanup) - add the sysfs LED marking kcontrol management - https://lore.kernel.org/alsa-devel/28fffebd-1ce9-7480-0f2f-ed8369abddf1@pere... v3 changes: - reorder the controls_rwsem use to fix the remaining mutex issue card->controls_rwsem <-> snd_ctl_layer_rwsem v2 changes: - fix the locking - remove the controls_rwsem read lock in the element get (the consistency is already protected with the global snd_ctl_led_mutex and possible partial value writes are catched with the next value change notification callback) - rename state to brightness and show the brightness unsigned integer value instead the text on/off string (sync with the LED core routines) - remove snd_ctl_led_hello() function (CI warning) - make snd_ctl_led_get_by_access() function static (CI warning) - move snd_ctl_layer_rwsem lock before the registraction callback call in snd_ctl_register_layer() - optimization v1: - https://lore.kernel.org/alsa-devel/20210211111400.1131020-1-perex@perex.cz/ Original RFC: - https://lore.kernel.org/alsa-devel/20210207201157.869972-1-perex@perex.cz/
Cc: Hans de Goede hdegoede@redhat.com
Jaroslav Kysela (6): ALSA: control - introduce snd_ctl_notify_one() helper ALSA: control - add layer registration routines ALSA: control - add generic LED trigger module as the new control layer ALSA: HDA - remove the custom implementation for the audio LED trigger ALSA: control - add sysfs support to the LED trigger module ALSA: led control - add sysfs kcontrol LED marking layer
include/sound/control.h | 35 +- sound/core/Kconfig | 6 + sound/core/Makefile | 2 + sound/core/control.c | 182 ++++++-- sound/core/control_led.c | 770 ++++++++++++++++++++++++++++++++ sound/pci/hda/Kconfig | 4 +- sound/pci/hda/hda_codec.c | 69 +-- sound/pci/hda/hda_generic.c | 162 ++----- sound/pci/hda/hda_generic.h | 15 +- sound/pci/hda/hda_local.h | 16 +- sound/pci/hda/patch_ca0132.c | 4 +- sound/pci/hda/patch_realtek.c | 2 +- sound/pci/hda/patch_sigmatel.c | 6 +- sound/pci/hda/thinkpad_helper.c | 2 +- 14 files changed, 1011 insertions(+), 264 deletions(-) create mode 100644 sound/core/control_led.c
This helper is required for the following generic LED mute patch. The helper also simplifies some other functions.
Signed-off-by: Jaroslav Kysela perex@perex.cz --- include/sound/control.h | 4 +-- sound/core/control.c | 68 +++++++++++++++++++++++++++-------------- 2 files changed, 47 insertions(+), 25 deletions(-)
diff --git a/include/sound/control.h b/include/sound/control.h index 77d9fa10812d..22f3d48163ff 100644 --- a/include/sound/control.h +++ b/include/sound/control.h @@ -115,6 +115,7 @@ typedef int (*snd_kctl_ioctl_func_t) (struct snd_card * card, unsigned int cmd, unsigned long arg);
void snd_ctl_notify(struct snd_card * card, unsigned int mask, struct snd_ctl_elem_id * id); +void snd_ctl_notify_one(struct snd_card * card, unsigned int mask, struct snd_kcontrol * kctl, unsigned int ioff);
struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new * kcontrolnew, void * private_data); void snd_ctl_free_one(struct snd_kcontrol * kcontrol); @@ -123,8 +124,7 @@ int snd_ctl_remove(struct snd_card * card, struct snd_kcontrol * kcontrol); int snd_ctl_replace(struct snd_card *card, struct snd_kcontrol *kcontrol, bool add_on_replace); int snd_ctl_remove_id(struct snd_card * card, struct snd_ctl_elem_id *id); int snd_ctl_rename_id(struct snd_card * card, struct snd_ctl_elem_id *src_id, struct snd_ctl_elem_id *dst_id); -int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id, - int active); +int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id, int active); struct snd_kcontrol *snd_ctl_find_numid(struct snd_card * card, unsigned int numid); struct snd_kcontrol *snd_ctl_find_id(struct snd_card * card, struct snd_ctl_elem_id *id);
diff --git a/sound/core/control.c b/sound/core/control.c index 5165741a8400..8a5cedb0a4be 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -181,6 +181,27 @@ 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_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 @@ -342,7 +363,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 +396,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 +480,14 @@ 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 +600,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 +1128,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 +1177,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 +1326,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 +1357,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; }
The layer registration allows to handle an extra functionality on top of the control API. It can be used for the audio LED control for example.
Signed-off-by: Jaroslav Kysela perex@perex.cz --- include/sound/control.h | 12 +++++ sound/core/control.c | 110 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 120 insertions(+), 2 deletions(-)
diff --git a/include/sound/control.h b/include/sound/control.h index 22f3d48163ff..175610bfa8c8 100644 --- a/include/sound/control.h +++ b/include/sound/control.h @@ -108,6 +108,14 @@ struct snd_ctl_file { struct list_head events; /* waiting events for read */ };
+struct snd_ctl_layer_ops { + struct snd_ctl_layer_ops *next; + const char *module_name; + void (*lregister)(struct snd_card *card); + void (*ldisconnect)(struct snd_card *card); + void (*lnotify)(struct snd_card *card, unsigned int mask, struct snd_kcontrol *kctl, unsigned int ioff); +}; + #define snd_ctl_file(n) list_entry(n, struct snd_ctl_file, list)
typedef int (*snd_kctl_ioctl_func_t) (struct snd_card * card, @@ -140,6 +148,10 @@ int snd_ctl_unregister_ioctl_compat(snd_kctl_ioctl_func_t fcn); #define snd_ctl_unregister_ioctl_compat(fcn) #endif
+int snd_ctl_request_layer(const char *module_name); +void snd_ctl_register_layer(struct snd_ctl_layer_ops *lops); +void snd_ctl_disconnect_layer(struct snd_ctl_layer_ops *lops); + int snd_ctl_get_preferred_subdevice(struct snd_card *card, int type);
static inline unsigned int snd_ctl_get_ioffnum(struct snd_kcontrol *kctl, struct snd_ctl_elem_id *id) diff --git a/sound/core/control.c b/sound/core/control.c index 8a5cedb0a4be..87630021e434 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -28,10 +28,12 @@ struct snd_kctl_ioctl { };
static DECLARE_RWSEM(snd_ioctl_rwsem); +static DECLARE_RWSEM(snd_ctl_layer_rwsem); static LIST_HEAD(snd_control_ioctls); #ifdef CONFIG_COMPAT static LIST_HEAD(snd_control_compat_ioctls); #endif +static struct snd_ctl_layer_ops *snd_ctl_layer;
static int snd_ctl_open(struct inode *inode, struct file *file) { @@ -195,10 +197,15 @@ 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; + struct snd_ctl_layer_ops *lops;
id.index += ioff; id.numid += ioff; snd_ctl_notify(card, mask, &id); + down_read(&snd_ctl_layer_rwsem); + for (lops = snd_ctl_layer; lops; lops = lops->next) + lops->lnotify(card, mask, kctl, ioff); + up_read(&snd_ctl_layer_rwsem); } EXPORT_SYMBOL(snd_ctl_notify_one);
@@ -1997,6 +2004,86 @@ EXPORT_SYMBOL_GPL(snd_ctl_get_preferred_subdevice); #define snd_ctl_ioctl_compat NULL #endif
+/* + * control layers (audio LED etc.) + */ + +/** + * snd_ctl_request_layer - request to use the layer + * @module_name: Name of the kernel module (NULL == build-in) + * + * Return an error code when the module cannot be loaded. + */ +int snd_ctl_request_layer(const char *module_name) +{ + struct snd_ctl_layer_ops *lops; + + if (module_name == NULL) + return 0; + down_read(&snd_ctl_layer_rwsem); + for (lops = snd_ctl_layer; lops; lops = lops->next) + if (strcmp(lops->module_name, module_name) == 0) + break; + up_read(&snd_ctl_layer_rwsem); + if (lops) + return 0; + return request_module(module_name); +} +EXPORT_SYMBOL_GPL(snd_ctl_request_layer); + +/** + * snd_ctl_register_layer - register new control layer + * @lops: operation structure + * + * The new layer can track all control elements and do additional + * operations on top (like audio LED handling). + */ +void snd_ctl_register_layer(struct snd_ctl_layer_ops *lops) +{ + struct snd_card *card; + int card_number; + + down_write(&snd_ctl_layer_rwsem); + lops->next = snd_ctl_layer; + snd_ctl_layer = lops; + up_write(&snd_ctl_layer_rwsem); + for (card_number = 0; card_number < SNDRV_CARDS; card_number++) { + card = snd_card_ref(card_number); + if (card) { + down_read(&card->controls_rwsem); + lops->lregister(card); + up_read(&card->controls_rwsem); + snd_card_unref(card); + } + } +} +EXPORT_SYMBOL_GPL(snd_ctl_register_layer); + +/** + * snd_ctl_disconnect_layer - disconnect control layer + * @lops: operation structure + * + * It is expected that the information about tracked cards + * is freed before this call (the disconnect callback is + * not called here). + */ +void snd_ctl_disconnect_layer(struct snd_ctl_layer_ops *lops) +{ + struct snd_ctl_layer_ops *lops2, *prev_lops2; + + down_write(&snd_ctl_layer_rwsem); + for (lops2 = snd_ctl_layer, prev_lops2 = NULL; lops2; lops2 = lops2->next) + if (lops2 == lops) { + if (!prev_lops2) + snd_ctl_layer = lops->next; + else + prev_lops2->next = lops->next; + break; + } + up_write(&snd_ctl_layer_rwsem); +} +EXPORT_SYMBOL_GPL(snd_ctl_disconnect_layer); + /* * INIT PART */ @@ -2020,9 +2107,20 @@ 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; + struct snd_ctl_layer_ops *lops; + 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; + down_read(&card->controls_rwsem); + down_read(&snd_ctl_layer_rwsem); + for (lops = snd_ctl_layer; lops; lops = lops->next) + lops->lregister(card); + up_read(&snd_ctl_layer_rwsem); + up_read(&card->controls_rwsem); + return 0; }
/* @@ -2032,6 +2130,7 @@ static int snd_ctl_dev_disconnect(struct snd_device *device) { struct snd_card *card = device->device_data; struct snd_ctl_file *ctl; + struct snd_ctl_layer_ops *lops; unsigned long flags;
read_lock_irqsave(&card->ctl_files_rwlock, flags); @@ -2041,6 +2140,13 @@ static int snd_ctl_dev_disconnect(struct snd_device *device) } read_unlock_irqrestore(&card->ctl_files_rwlock, flags);
+ down_read(&card->controls_rwsem); + down_read(&snd_ctl_layer_rwsem); + for (lops = snd_ctl_layer; lops; lops = lops->next) + lops->ldisconnect(card); + up_read(&snd_ctl_layer_rwsem); + up_read(&card->controls_rwsem); + return snd_unregister_device(&card->ctl_dev); }
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 code change 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 on demand, 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.).
Signed-off-by: Jaroslav Kysela perex@perex.cz --- include/sound/control.h | 19 ++- sound/core/Kconfig | 6 + sound/core/Makefile | 2 + sound/core/control.c | 4 +- sound/core/control_led.c | 278 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 307 insertions(+), 2 deletions(-) create mode 100644 sound/core/control_led.c
diff --git a/include/sound/control.h b/include/sound/control.h index 175610bfa8c8..985c51a8fb74 100644 --- a/include/sound/control.h +++ b/include/sound/control.h @@ -24,7 +24,7 @@ typedef int (snd_kcontrol_tlv_rw_t)(struct snd_kcontrol *kcontrol,
/* internal flag for skipping validations */ #ifdef CONFIG_SND_CTL_VALIDATION -#define SNDRV_CTL_ELEM_ACCESS_SKIP_CHECK (1 << 27) +#define SNDRV_CTL_ELEM_ACCESS_SKIP_CHECK (1 << 24) #define snd_ctl_skip_validation(info) \ ((info)->access & SNDRV_CTL_ELEM_ACCESS_SKIP_CHECK) #else @@ -32,6 +32,12 @@ typedef int (snd_kcontrol_tlv_rw_t)(struct snd_kcontrol *kcontrol, #define snd_ctl_skip_validation(info) true #endif
+/* kernel only - LED bits */ +#define SNDRV_CTL_ELEM_ACCESS_LED_SHIFT 25 +#define SNDRV_CTL_ELEM_ACCESS_LED_MASK (7<<25) /* kernel three bits - LED group */ +#define SNDRV_CTL_ELEM_ACCESS_SPK_LED (1<<25) /* kernel speaker (output) LED flag */ +#define SNDRV_CTL_ELEM_ACCESS_MIC_LED (2<<25) /* kernel microphone (input) LED flag */ + enum { SNDRV_CTL_TLV_OP_READ = 0, SNDRV_CTL_TLV_OP_WRITE = 1, @@ -265,6 +271,17 @@ int snd_ctl_apply_vmaster_followers(struct snd_kcontrol *kctl, void *arg), void *arg);
+/* + * Control LED trigger layer + */ +#define SND_CTL_LAYER_MODULE_LED "snd-ctl-led" + +#if IS_MODULE(CONFIG_SND_CTL_LED) +static inline int snd_ctl_led_request(void) { return snd_ctl_request_layer(SND_CTL_LAYER_MODULE_LED); } +#else +static inline int snd_ctl_led_request(void) { return 0; } +#endif + /* * Helper functions for jack-detection controls */ diff --git a/sound/core/Kconfig b/sound/core/Kconfig index a4050f87f230..db2e3c63ff41 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_CTL_LED + tristate + select NEW_LEDS if SND_CTL_LED + select LEDS_TRIGGERS if SND_CTL_LED + select LEDS_TRIGGER_AUDIO if SND_CTL_LED + source "sound/core/seq/Kconfig" diff --git a/sound/core/Makefile b/sound/core/Makefile index ee4a4a6b99ba..d774792850f3 100644 --- a/sound/core/Makefile +++ b/sound/core/Makefile @@ -27,6 +27,7 @@ CFLAGS_pcm_native.o := -I$(src)
snd-pcm-dmaengine-objs := pcm_dmaengine.o
+snd-ctl-led-objs := control_led.o snd-rawmidi-objs := rawmidi.o snd-timer-objs := timer.o snd-hrtimer-objs := hrtimer.o @@ -37,6 +38,7 @@ snd-seq-device-objs := seq_device.o snd-compress-objs := compress_offload.o
obj-$(CONFIG_SND) += snd.o +obj-$(CONFIG_SND_CTL_LED) += snd-ctl-led.o obj-$(CONFIG_SND_HWDEP) += snd-hwdep.o obj-$(CONFIG_SND_TIMER) += snd-timer.o obj-$(CONFIG_SND_HRTIMER) += snd-hrtimer.o diff --git a/sound/core/control.c b/sound/core/control.c index 87630021e434..6825ca75daf5 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -278,6 +278,7 @@ 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_LED_MASK | SNDRV_CTL_ELEM_ACCESS_SKIP_CHECK);
err = snd_ctl_new(&kctl, count, access, NULL); @@ -1047,7 +1048,8 @@ static int snd_ctl_elem_info_user(struct snd_ctl_file *ctl, if (result < 0) return result; /* drop internal access flags */ - info.access &= ~SNDRV_CTL_ELEM_ACCESS_SKIP_CHECK; + info.access &= ~(SNDRV_CTL_ELEM_ACCESS_SKIP_CHECK| + SNDRV_CTL_ELEM_ACCESS_LED_MASK); if (copy_to_user(_info, &info, sizeof(info))) return -EFAULT; return result; diff --git a/sound/core/control_led.c b/sound/core/control_led.c new file mode 100644 index 000000000000..3e2c3c485c5c --- /dev/null +++ b/sound/core/control_led.c @@ -0,0 +1,278 @@ +// 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/slab.h> +#include <linux/module.h> +#include <linux/leds.h> +#include <sound/core.h> +#include <sound/control.h> + +MODULE_AUTHOR("Jaroslav Kysela perex@perex.cz"); +MODULE_DESCRIPTION("ALSA control interface to LED trigger code."); +MODULE_LICENSE("GPL"); + +#define MAX_LED (((SNDRV_CTL_ELEM_ACCESS_MIC_LED - SNDRV_CTL_ELEM_ACCESS_SPK_LED) \ + >> SNDRV_CTL_ELEM_ACCESS_LED_SHIFT) + 1) + +struct snd_ctl_led { + struct list_head list; + struct snd_card *card; + unsigned int access; + struct snd_kcontrol *kctl; + unsigned int index_offset; +}; + +static DEFINE_MUTEX(snd_ctl_led_mutex); +static struct list_head snd_ctl_led_controls[MAX_LED]; +static bool snd_ctl_led_card_valid[SNDRV_CARDS]; + +#define UPDATE_ROUTE(route, cb) \ + do { \ + int route2 = (cb); \ + if (route2 >= 0) \ + route = route < 0 ? route2 : (route | route2); \ + } while (0) + +static inline unsigned int access_to_group(unsigned int access) +{ + return ((access & SNDRV_CTL_ELEM_ACCESS_LED_MASK) >> + SNDRV_CTL_ELEM_ACCESS_LED_SHIFT) - 1; +} + +static inline unsigned int group_to_access(unsigned int group) +{ + return (group + 1) << SNDRV_CTL_ELEM_ACCESS_LED_SHIFT; +} + +static struct list_head *snd_ctl_led_controls_by_access(unsigned int access) +{ + unsigned int group = access_to_group(access); + if (group >= MAX_LED) + return NULL; + return &snd_ctl_led_controls[group]; +} + +static int snd_ctl_led_get(struct snd_ctl_led *lctl) +{ + struct snd_kcontrol *kctl = lctl->kctl; + 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 += lctl->index_offset; + info.id.numid += lctl->index_offset; + 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 void snd_ctl_led_set_state(struct snd_card *card, unsigned int access, + struct snd_kcontrol *kctl, unsigned int ioff) +{ + struct list_head *controls; + struct snd_ctl_led *lctl; + enum led_audio led_trigger_type; + int route; + bool found; + + controls = snd_ctl_led_controls_by_access(access); + if (!controls) + return; + if (access == SNDRV_CTL_ELEM_ACCESS_SPK_LED) { + led_trigger_type = LED_AUDIO_MUTE; + } else if (access == SNDRV_CTL_ELEM_ACCESS_MIC_LED) { + led_trigger_type = LED_AUDIO_MICMUTE; + } else { + return; + } + route = -1; + found = false; + mutex_lock(&snd_ctl_led_mutex); + /* the card may not be registered (active) at this point */ + if (card && !snd_ctl_led_card_valid[card->number]) { + mutex_unlock(&snd_ctl_led_mutex); + return; + } + list_for_each_entry(lctl, controls, list) { + if (lctl->kctl == kctl && lctl->index_offset == ioff) + found = true; + UPDATE_ROUTE(route, snd_ctl_led_get(lctl)); + } + if (!found && kctl && card) { + lctl = kzalloc(sizeof(*lctl), GFP_KERNEL); + if (lctl) { + lctl->card = card; + lctl->access = access; + lctl->kctl = kctl; + lctl->index_offset = ioff; + list_add(&lctl->list, controls); + UPDATE_ROUTE(route, snd_ctl_led_get(lctl)); + } + } + mutex_unlock(&snd_ctl_led_mutex); + if (route >= 0) + ledtrig_audio_set(led_trigger_type, route ? LED_OFF : LED_ON); +} + +static struct snd_ctl_led *snd_ctl_led_find(struct snd_kcontrol *kctl, unsigned int ioff) +{ + struct list_head *controls; + struct snd_ctl_led *lctl; + unsigned int group; + + for (group = 0; group < MAX_LED; group++) { + controls = &snd_ctl_led_controls[group]; + list_for_each_entry(lctl, controls, list) + if (lctl->kctl == kctl && lctl->index_offset == ioff) + return lctl; + } + return NULL; +} + +static unsigned int snd_ctl_led_remove(struct snd_kcontrol *kctl, unsigned int ioff, + unsigned int access) +{ + struct snd_ctl_led *lctl; + unsigned int ret = 0; + + mutex_lock(&snd_ctl_led_mutex); + lctl = snd_ctl_led_find(kctl, ioff); + if (lctl && (access == 0 || access != lctl->access)) { + ret = lctl->access; + list_del(&lctl->list); + kfree(lctl); + } + mutex_unlock(&snd_ctl_led_mutex); + return ret; +} + +static void snd_ctl_led_notify(struct snd_card *card, unsigned int mask, + struct snd_kcontrol *kctl, unsigned int ioff) +{ + struct snd_kcontrol_volatile *vd; + unsigned int access, access2; + + if (mask == SNDRV_CTL_EVENT_MASK_REMOVE) { + access = snd_ctl_led_remove(kctl, ioff, 0); + if (access) + snd_ctl_led_set_state(card, access, NULL, 0); + } else if (mask & SNDRV_CTL_EVENT_MASK_INFO) { + vd = &kctl->vd[ioff]; + access = vd->access & SNDRV_CTL_ELEM_ACCESS_LED_MASK; + access2 = snd_ctl_led_remove(kctl, ioff, access); + if (access2) + snd_ctl_led_set_state(card, access2, NULL, 0); + if (access) + snd_ctl_led_set_state(card, access, kctl, ioff); + } else if ((mask & (SNDRV_CTL_EVENT_MASK_ADD | + SNDRV_CTL_EVENT_MASK_VALUE)) != 0) { + vd = &kctl->vd[ioff]; + access = vd->access & SNDRV_CTL_ELEM_ACCESS_LED_MASK; + if (access) + snd_ctl_led_set_state(card, access, kctl, ioff); + } +} + +static void snd_ctl_led_refresh(void) +{ + unsigned int group; + + for (group = 0; group < MAX_LED; group++) + snd_ctl_led_set_state(NULL, group_to_access(group), NULL, 0); +} + +static void snd_ctl_led_clean(struct snd_card *card) +{ + unsigned int group; + struct list_head *controls; + struct snd_ctl_led *lctl; + + for (group = 0; group < MAX_LED; group++) { + controls = &snd_ctl_led_controls[group]; +repeat: + list_for_each_entry(lctl, controls, list) + if (!card || lctl->card == card) { + list_del(&lctl->list); + kfree(lctl); + goto repeat; + } + } +} + +static void snd_ctl_led_register(struct snd_card *card) +{ + struct snd_kcontrol *kctl; + unsigned int ioff; + + if (snd_BUG_ON(card->number < 0 || + card->number >= ARRAY_SIZE(snd_ctl_led_card_valid))) + return; + mutex_lock(&snd_ctl_led_mutex); + snd_ctl_led_card_valid[card->number] = true; + mutex_unlock(&snd_ctl_led_mutex); + /* the register callback is already called with held card->controls_rwsem */ + list_for_each_entry(kctl, &card->controls, list) + for (ioff = 0; ioff < kctl->count; ioff++) + snd_ctl_led_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, kctl, ioff); + snd_ctl_led_refresh(); +} + +static void snd_ctl_led_disconnect(struct snd_card *card) +{ + mutex_lock(&snd_ctl_led_mutex); + snd_ctl_led_card_valid[card->number] = false; + snd_ctl_led_clean(card); + mutex_unlock(&snd_ctl_led_mutex); + snd_ctl_led_refresh(); +} + +/* + * Control layer registration + */ +static struct snd_ctl_layer_ops snd_ctl_led_lops = { + .module_name = SND_CTL_LAYER_MODULE_LED, + .lregister = snd_ctl_led_register, + .ldisconnect = snd_ctl_led_disconnect, + .lnotify = snd_ctl_led_notify, +}; + +static int __init snd_ctl_led_init(void) +{ + unsigned int group; + + for (group = 0; group < MAX_LED; group++) + INIT_LIST_HEAD(&snd_ctl_led_controls[group]); + snd_ctl_register_layer(&snd_ctl_led_lops); + return 0; +} + +static void __exit snd_ctl_led_exit(void) +{ + snd_ctl_disconnect_layer(&snd_ctl_led_lops); + snd_ctl_led_clean(NULL); +} + +module_init(snd_ctl_led_init) +module_exit(snd_ctl_led_exit)
With the new snd-ctl-led module, we have a generic way to trigger audio LEDs based on the sound control changes.
Remove the custom implementation from the HDA driver.
Move the LED initialization before snd_hda_gen_parse_auto_config() call in all drivers to create marked controls there.
Signed-off-by: Jaroslav Kysela perex@perex.cz --- sound/pci/hda/Kconfig | 4 +- sound/pci/hda/hda_codec.c | 69 ++------------ sound/pci/hda/hda_generic.c | 162 +++++--------------------------- sound/pci/hda/hda_generic.h | 15 +-- sound/pci/hda/hda_local.h | 16 +--- sound/pci/hda/patch_ca0132.c | 4 +- sound/pci/hda/patch_realtek.c | 2 +- sound/pci/hda/patch_sigmatel.c | 6 +- sound/pci/hda/thinkpad_helper.c | 2 +- 9 files changed, 45 insertions(+), 235 deletions(-)
diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index 90759391cbac..c4360cdbc728 100644 --- a/sound/pci/hda/Kconfig +++ b/sound/pci/hda/Kconfig @@ -221,10 +221,8 @@ comment "Set to Y if you want auto-loading the codec driver"
config SND_HDA_GENERIC tristate "Enable generic HD-audio codec parser" - select NEW_LEDS if SND_HDA_GENERIC_LEDS + select SND_CTL_LED if SND_HDA_GENERIC_LEDS select LEDS_CLASS if SND_HDA_GENERIC_LEDS - select LEDS_TRIGGERS if SND_HDA_GENERIC_LEDS - select LEDS_TRIGGER_AUDIO if SND_HDA_GENERIC_LEDS help Say Y or M here to enable the generic HD-audio codec parser in snd-hda-intel driver. diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 2026f1ccaf5a..e54e39b35709 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -1952,7 +1952,7 @@ static int add_follower(struct hda_codec *codec, int __snd_hda_add_vmaster(struct hda_codec *codec, char *name, unsigned int *tlv, const char * const *followers, const char *suffix, bool init_follower_vol, - struct snd_kcontrol **ctl_ret) + unsigned int access, struct snd_kcontrol **ctl_ret) { struct snd_kcontrol *kctl; int err; @@ -1968,6 +1968,7 @@ int __snd_hda_add_vmaster(struct hda_codec *codec, char *name, kctl = snd_ctl_make_virtual_master(name, tlv); if (!kctl) return -ENOMEM; + kctl->vd[0].access |= access; err = snd_hda_ctl_add(codec, 0, kctl); if (err < 0) return err; @@ -1994,87 +1995,29 @@ int __snd_hda_add_vmaster(struct hda_codec *codec, char *name, } EXPORT_SYMBOL_GPL(__snd_hda_add_vmaster);
-/* - * mute-LED control using vmaster - */ -static int vmaster_mute_mode_info(struct snd_kcontrol *kcontrol, - struct snd_ctl_elem_info *uinfo) -{ - static const char * const texts[] = { - "On", "Off", "Follow Master" - }; - - return snd_ctl_enum_info(uinfo, 1, 3, texts); -} - -static int vmaster_mute_mode_get(struct snd_kcontrol *kcontrol, - struct snd_ctl_elem_value *ucontrol) -{ - struct hda_vmaster_mute_hook *hook = snd_kcontrol_chip(kcontrol); - ucontrol->value.enumerated.item[0] = hook->mute_mode; - return 0; -} - -static int vmaster_mute_mode_put(struct snd_kcontrol *kcontrol, - struct snd_ctl_elem_value *ucontrol) -{ - struct hda_vmaster_mute_hook *hook = snd_kcontrol_chip(kcontrol); - unsigned int old_mode = hook->mute_mode; - - hook->mute_mode = ucontrol->value.enumerated.item[0]; - if (hook->mute_mode > HDA_VMUTE_FOLLOW_MASTER) - hook->mute_mode = HDA_VMUTE_FOLLOW_MASTER; - if (old_mode == hook->mute_mode) - return 0; - snd_hda_sync_vmaster_hook(hook); - return 1; -} - -static const struct snd_kcontrol_new vmaster_mute_mode = { - .iface = SNDRV_CTL_ELEM_IFACE_MIXER, - .name = "Mute-LED Mode", - .info = vmaster_mute_mode_info, - .get = vmaster_mute_mode_get, - .put = vmaster_mute_mode_put, -}; - /* meta hook to call each driver's vmaster hook */ static void vmaster_hook(void *private_data, int enabled) { struct hda_vmaster_mute_hook *hook = private_data;
- if (hook->mute_mode != HDA_VMUTE_FOLLOW_MASTER) - enabled = hook->mute_mode; hook->hook(hook->codec, enabled); }
/** - * snd_hda_add_vmaster_hook - Add a vmaster hook for mute-LED + * snd_hda_add_vmaster_hook - Add a vmaster hw specific hook * @codec: the HDA codec * @hook: the vmaster hook object - * @expose_enum_ctl: flag to create an enum ctl * - * Add a mute-LED hook with the given vmaster switch kctl. - * When @expose_enum_ctl is set, "Mute-LED Mode" control is automatically - * created and associated with the given hook. + * Add a hw specific hook (like EAPD) with the given vmaster switch kctl. */ int snd_hda_add_vmaster_hook(struct hda_codec *codec, - struct hda_vmaster_mute_hook *hook, - bool expose_enum_ctl) + struct hda_vmaster_mute_hook *hook) { - struct snd_kcontrol *kctl; - if (!hook->hook || !hook->sw_kctl) return 0; hook->codec = codec; - hook->mute_mode = HDA_VMUTE_FOLLOW_MASTER; snd_ctl_add_vmaster_hook(hook->sw_kctl, vmaster_hook, hook); - if (!expose_enum_ctl) - return 0; - kctl = snd_ctl_new1(&vmaster_mute_mode, hook); - if (!kctl) - return -ENOMEM; - return snd_hda_ctl_add(codec, 0, kctl); + return 0; } EXPORT_SYMBOL_GPL(snd_hda_add_vmaster_hook);
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c index f5cba7afd1c6..3998e1771805 100644 --- a/sound/pci/hda/hda_generic.c +++ b/sound/pci/hda/hda_generic.c @@ -981,6 +981,8 @@ add_control(struct hda_gen_spec *spec, int type, const char *name, knew->index = cidx; if (get_amp_nid_(val)) knew->subdevice = HDA_SUBDEV_AMP_FLAG; + if (knew->access == 0) + knew->access = SNDRV_CTL_ELEM_ACCESS_READWRITE; knew->private_value = val; return knew; } @@ -3618,8 +3620,11 @@ static int add_single_cap_ctl(struct hda_codec *codec, const char *label, amp_val_replace_channels(ctl, chs)); if (!knew) return -ENOMEM; - if (is_switch) + if (is_switch) { knew->put = cap_single_sw_put; + if (spec->mic_mute_led) + knew->access |= SNDRV_CTL_ELEM_ACCESS_MIC_LED; + } if (!inv_dmic) return 0;
@@ -3634,8 +3639,11 @@ static int add_single_cap_ctl(struct hda_codec *codec, const char *label, amp_val_replace_channels(ctl, 2)); if (!knew) return -ENOMEM; - if (is_switch) + if (is_switch) { knew->put = cap_single_sw_put; + if (spec->mic_mute_led) + knew->access |= SNDRV_CTL_ELEM_ACCESS_MIC_LED; + } return 0; }
@@ -3676,6 +3684,8 @@ 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; } return 0; } @@ -3917,11 +3927,6 @@ static int create_mute_led_cdev(struct hda_codec *codec, return devm_led_classdev_register(&codec->core.dev, cdev); }
-static void vmaster_update_mute_led(void *private_data, int enabled) -{ - ledtrig_audio_set(LED_AUDIO_MUTE, enabled ? LED_OFF : LED_ON); -} - /** * snd_hda_gen_add_mute_led_cdev - Create a LED classdev and enable as vmaster mute LED * @codec: the HDA codec @@ -3945,134 +3950,11 @@ 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.hook = vmaster_update_mute_led; - spec->vmaster_mute_enum = 1; + spec->vmaster_mute_led = 1; return 0; } EXPORT_SYMBOL_GPL(snd_hda_gen_add_mute_led_cdev);
-/* - * mic mute LED hook helpers - */ -enum { - MICMUTE_LED_ON, - MICMUTE_LED_OFF, - MICMUTE_LED_FOLLOW_CAPTURE, - MICMUTE_LED_FOLLOW_MUTE, -}; - -static void call_micmute_led_update(struct hda_codec *codec) -{ - struct hda_gen_spec *spec = codec->spec; - unsigned int val; - - switch (spec->micmute_led.led_mode) { - case MICMUTE_LED_ON: - val = 1; - break; - case MICMUTE_LED_OFF: - val = 0; - break; - case MICMUTE_LED_FOLLOW_CAPTURE: - val = !!spec->micmute_led.capture; - break; - case MICMUTE_LED_FOLLOW_MUTE: - default: - val = !spec->micmute_led.capture; - break; - } - - if (val == spec->micmute_led.led_value) - return; - spec->micmute_led.led_value = val; - ledtrig_audio_set(LED_AUDIO_MICMUTE, - spec->micmute_led.led_value ? LED_ON : LED_OFF); -} - -static void update_micmute_led(struct hda_codec *codec, - struct snd_kcontrol *kcontrol, - struct snd_ctl_elem_value *ucontrol) -{ - struct hda_gen_spec *spec = codec->spec; - unsigned int mask; - - if (spec->micmute_led.old_hook) - spec->micmute_led.old_hook(codec, kcontrol, ucontrol); - - if (!ucontrol) - return; - mask = 1U << snd_ctl_get_ioffidx(kcontrol, &ucontrol->id); - if (!strcmp("Capture Switch", ucontrol->id.name)) { - /* TODO: How do I verify if it's a mono or stereo here? */ - if (ucontrol->value.integer.value[0] || - ucontrol->value.integer.value[1]) - spec->micmute_led.capture |= mask; - else - spec->micmute_led.capture &= ~mask; - call_micmute_led_update(codec); - } -} - -static int micmute_led_mode_info(struct snd_kcontrol *kcontrol, - struct snd_ctl_elem_info *uinfo) -{ - static const char * const texts[] = { - "On", "Off", "Follow Capture", "Follow Mute", - }; - - return snd_ctl_enum_info(uinfo, 1, ARRAY_SIZE(texts), texts); -} - -static int micmute_led_mode_get(struct snd_kcontrol *kcontrol, - struct snd_ctl_elem_value *ucontrol) -{ - struct hda_codec *codec = snd_kcontrol_chip(kcontrol); - struct hda_gen_spec *spec = codec->spec; - - ucontrol->value.enumerated.item[0] = spec->micmute_led.led_mode; - return 0; -} - -static int micmute_led_mode_put(struct snd_kcontrol *kcontrol, - struct snd_ctl_elem_value *ucontrol) -{ - struct hda_codec *codec = snd_kcontrol_chip(kcontrol); - struct hda_gen_spec *spec = codec->spec; - unsigned int mode; - - mode = ucontrol->value.enumerated.item[0]; - if (mode > MICMUTE_LED_FOLLOW_MUTE) - mode = MICMUTE_LED_FOLLOW_MUTE; - if (mode == spec->micmute_led.led_mode) - return 0; - spec->micmute_led.led_mode = mode; - call_micmute_led_update(codec); - return 1; -} - -static const struct snd_kcontrol_new micmute_led_mode_ctl = { - .iface = SNDRV_CTL_ELEM_IFACE_MIXER, - .name = "Mic Mute-LED Mode", - .info = micmute_led_mode_info, - .get = micmute_led_mode_get, - .put = micmute_led_mode_put, -}; - -/* Set up the capture sync hook for controlling the mic-mute LED */ -static int add_micmute_led_hook(struct hda_codec *codec) -{ - struct hda_gen_spec *spec = codec->spec; - - spec->micmute_led.led_mode = MICMUTE_LED_FOLLOW_MUTE; - spec->micmute_led.capture = 0; - spec->micmute_led.led_value = -1; - spec->micmute_led.old_hook = spec->cap_sync_hook; - spec->cap_sync_hook = update_micmute_led; - if (!snd_hda_gen_add_kctl(spec, NULL, &micmute_led_mode_ctl)) - return -ENOMEM; - return 0; -} - /** * snd_hda_gen_add_micmute_led_cdev - Create a LED classdev and enable as mic-mute LED * @codec: the HDA codec @@ -4091,6 +3973,7 @@ 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) { @@ -4101,7 +3984,8 @@ int snd_hda_gen_add_micmute_led_cdev(struct hda_codec *codec, } }
- return add_micmute_led_hook(codec); + spec->mic_mute_led = 1; + return 0; } EXPORT_SYMBOL_GPL(snd_hda_gen_add_micmute_led_cdev); #endif /* CONFIG_SND_HDA_GENERIC_LEDS */ @@ -5060,6 +4944,9 @@ 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(); + if (spec->mixer_nid && !spec->mixer_merge_nid) spec->mixer_merge_nid = spec->mixer_nid;
@@ -5291,7 +5178,7 @@ int snd_hda_gen_build_controls(struct hda_codec *codec) !snd_hda_find_mixer_ctl(codec, "Master Playback Volume")) { err = snd_hda_add_vmaster(codec, "Master Playback Volume", spec->vmaster_tlv, follower_pfxs, - "Playback Volume"); + "Playback Volume", 0); if (err < 0) return err; } @@ -5299,13 +5186,14 @@ int snd_hda_gen_build_controls(struct hda_codec *codec) !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.sw_kctl); + "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, - spec->vmaster_mute_enum); + snd_hda_add_vmaster_hook(codec, &spec->vmaster_mute); snd_hda_sync_vmaster_hook(&spec->vmaster_mute); } } diff --git a/sound/pci/hda/hda_generic.h b/sound/pci/hda/hda_generic.h index 0886bc81f40b..d4dd1b8a2e7e 100644 --- a/sound/pci/hda/hda_generic.h +++ b/sound/pci/hda/hda_generic.h @@ -84,15 +84,6 @@ struct badness_table { extern const struct badness_table hda_main_out_badness; extern const struct badness_table hda_extra_out_badness;
-struct hda_micmute_hook { - unsigned int led_mode; - unsigned int capture; - unsigned int led_value; - void (*old_hook)(struct hda_codec *codec, - struct snd_kcontrol *kcontrol, - struct snd_ctl_elem_value *ucontrol); -}; - struct hda_gen_spec { char stream_name_analog[32]; /* analog PCM stream */ const struct hda_pcm_stream *stream_analog_playback; @@ -229,7 +220,8 @@ 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_enum:1; /* add vmaster mute mode enum */ + 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 */ @@ -285,9 +277,6 @@ struct hda_gen_spec { struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol);
- /* mic mute LED hook; called via cap_sync_hook */ - struct hda_micmute_hook micmute_led; - /* PCM hooks */ void (*pcm_playback_hook)(struct hda_pcm_stream *hinfo, struct hda_codec *codec, diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 317245a5585d..4c5589c10f1d 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -131,21 +131,15 @@ struct snd_kcontrol *snd_hda_find_mixer_ctl(struct hda_codec *codec, int __snd_hda_add_vmaster(struct hda_codec *codec, char *name, unsigned int *tlv, const char * const *followers, const char *suffix, bool init_follower_vol, - struct snd_kcontrol **ctl_ret); -#define snd_hda_add_vmaster(codec, name, tlv, followers, suffix) \ - __snd_hda_add_vmaster(codec, name, tlv, followers, suffix, true, NULL) + unsigned int access, struct snd_kcontrol **ctl_ret); +#define snd_hda_add_vmaster(codec, name, tlv, followers, suffix, access) \ + __snd_hda_add_vmaster(codec, name, tlv, followers, suffix, true, access, NULL) int snd_hda_codec_reset(struct hda_codec *codec); void snd_hda_codec_register(struct hda_codec *codec); void snd_hda_codec_cleanup_for_unbind(struct hda_codec *codec);
#define snd_hda_regmap_sync(codec) snd_hdac_regmap_sync(&(codec)->core)
-enum { - HDA_VMUTE_OFF, - HDA_VMUTE_ON, - HDA_VMUTE_FOLLOW_MASTER, -}; - struct hda_vmaster_mute_hook { /* below two fields must be filled by the caller of * snd_hda_add_vmaster_hook() beforehand @@ -153,13 +147,11 @@ struct hda_vmaster_mute_hook { struct snd_kcontrol *sw_kctl; void (*hook)(void *, int); /* below are initialized automatically */ - unsigned int mute_mode; /* HDA_VMUTE_XXX */ struct hda_codec *codec; };
int snd_hda_add_vmaster_hook(struct hda_codec *codec, - struct hda_vmaster_mute_hook *hook, - bool expose_enum_ctl); + struct hda_vmaster_mute_hook *hook); void snd_hda_sync_vmaster_hook(struct hda_vmaster_mute_hook *hook);
/* amp value bits */ diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c index b2b620f6c832..49b4fdd2feab 100644 --- a/sound/pci/hda/patch_ca0132.c +++ b/sound/pci/hda/patch_ca0132.c @@ -7041,11 +7041,11 @@ static int ca0132_build_controls(struct hda_codec *codec) spec->tlv); snd_hda_add_vmaster(codec, "Master Playback Volume", spec->tlv, ca0132_alt_follower_pfxs, - "Playback Volume"); + "Playback Volume", 0); err = __snd_hda_add_vmaster(codec, "Master Playback Switch", NULL, ca0132_alt_follower_pfxs, "Playback Switch", - true, &spec->vmaster_mute.sw_kctl); + true, 0, &spec->vmaster_mute.sw_kctl); if (err < 0) return err; } diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 8239e5efc12d..6d95d9e04723 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -292,7 +292,7 @@ static void alc_fixup_gpio4(struct hda_codec *codec, static void alc_fixup_micmute_led(struct hda_codec *codec, const struct hda_fixup *fix, int action) { - if (action == HDA_FIXUP_ACT_PROBE) + if (action == HDA_FIXUP_ACT_PRE_PROBE) snd_hda_gen_add_micmute_led_cdev(codec, NULL); }
diff --git a/sound/pci/hda/patch_sigmatel.c b/sound/pci/hda/patch_sigmatel.c index c662431bf13a..3bd592e126a3 100644 --- a/sound/pci/hda/patch_sigmatel.c +++ b/sound/pci/hda/patch_sigmatel.c @@ -4277,6 +4277,9 @@ static int stac_parse_auto_config(struct hda_codec *codec)
spec->gen.automute_hook = stac_update_outputs;
+ if (spec->gpio_led) + snd_hda_gen_add_mute_led_cdev(codec, stac_vmaster_hook); + err = snd_hda_gen_parse_auto_config(codec, &spec->gen.autocfg); if (err < 0) return err; @@ -4318,9 +4321,6 @@ static int stac_parse_auto_config(struct hda_codec *codec) } #endif
- if (spec->gpio_led) - snd_hda_gen_add_mute_led_cdev(codec, stac_vmaster_hook); - if (spec->aloopback_ctl && snd_hda_get_bool_hint(codec, "loopback") == 1) { unsigned int wr_verb = diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c index 6698ae241efc..de4d8deed102 100644 --- a/sound/pci/hda/thinkpad_helper.c +++ b/sound/pci/hda/thinkpad_helper.c @@ -18,7 +18,7 @@ static bool is_thinkpad(struct hda_codec *codec) static void hda_fixup_thinkpad_acpi(struct hda_codec *codec, const struct hda_fixup *fix, int action) { - if (action == HDA_FIXUP_ACT_PROBE) { + if (action == HDA_FIXUP_ACT_PRE_PROBE) { if (!is_thinkpad(codec)) return; snd_hda_gen_add_mute_led_cdev(codec, NULL);
Create SYSFS/devices/virtual/sound/ctl-led tree (with SYSFS/class/sound/ctl-led symlink).
speaker/ +-- mode +-- brightness mic/ +-- mode +-- brightness
Copy the idea from the HDA driver and allow to set the audio LEDs based on the various modes:
- follow mute - follow moute (inverted to follow mute) - off - on
Also, the actual LED state is exposed.
Signed-off-by: Jaroslav Kysela perex@perex.cz --- sound/core/control_led.c | 192 +++++++++++++++++++++++++++++++++------ 1 file changed, 163 insertions(+), 29 deletions(-)
diff --git a/sound/core/control_led.c b/sound/core/control_led.c index 3e2c3c485c5c..dfa51d8461e1 100644 --- a/sound/core/control_led.c +++ b/sound/core/control_led.c @@ -17,7 +17,23 @@ MODULE_LICENSE("GPL"); #define MAX_LED (((SNDRV_CTL_ELEM_ACCESS_MIC_LED - SNDRV_CTL_ELEM_ACCESS_SPK_LED) \ >> SNDRV_CTL_ELEM_ACCESS_LED_SHIFT) + 1)
+enum snd_ctl_led_mode { + MODE_FOLLOW_MUTE = 0, + MODE_FOLLOW_ROUTE, + MODE_OFF, + MODE_ON, +}; + struct snd_ctl_led { + struct device dev; + struct list_head controls; + const char *name; + unsigned int group; + enum led_audio trigger_type; + enum snd_ctl_led_mode mode; +}; + +struct snd_ctl_led_ctl { struct list_head list; struct snd_card *card; unsigned int access; @@ -26,8 +42,21 @@ struct snd_ctl_led { };
static DEFINE_MUTEX(snd_ctl_led_mutex); -static struct list_head snd_ctl_led_controls[MAX_LED]; static bool snd_ctl_led_card_valid[SNDRV_CARDS]; +static struct snd_ctl_led snd_ctl_leds[MAX_LED] = { + { + .name = "speaker", + .group = (SNDRV_CTL_ELEM_ACCESS_SPK_LED >> SNDRV_CTL_ELEM_ACCESS_LED_SHIFT) - 1, + .trigger_type = LED_AUDIO_MUTE, + .mode = MODE_FOLLOW_MUTE, + }, + { + .name = "mic", + .group = (SNDRV_CTL_ELEM_ACCESS_MIC_LED >> SNDRV_CTL_ELEM_ACCESS_LED_SHIFT) - 1, + .trigger_type = LED_AUDIO_MICMUTE, + .mode = MODE_FOLLOW_MUTE, + }, +};
#define UPDATE_ROUTE(route, cb) \ do { \ @@ -47,15 +76,15 @@ static inline unsigned int group_to_access(unsigned int group) return (group + 1) << SNDRV_CTL_ELEM_ACCESS_LED_SHIFT; }
-static struct list_head *snd_ctl_led_controls_by_access(unsigned int access) +static struct snd_ctl_led *snd_ctl_led_get_by_access(unsigned int access) { unsigned int group = access_to_group(access); if (group >= MAX_LED) return NULL; - return &snd_ctl_led_controls[group]; + return &snd_ctl_leds[group]; }
-static int snd_ctl_led_get(struct snd_ctl_led *lctl) +static int snd_ctl_led_get(struct snd_ctl_led_ctl *lctl) { struct snd_kcontrol *kctl = lctl->kctl; struct snd_ctl_elem_info info; @@ -91,22 +120,14 @@ static int snd_ctl_led_get(struct snd_ctl_led *lctl) static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access, struct snd_kcontrol *kctl, unsigned int ioff) { - struct list_head *controls; - struct snd_ctl_led *lctl; - enum led_audio led_trigger_type; + struct snd_ctl_led *led; + struct snd_ctl_led_ctl *lctl; int route; bool found;
- controls = snd_ctl_led_controls_by_access(access); - if (!controls) + led = snd_ctl_led_get_by_access(access); + if (!led) return; - if (access == SNDRV_CTL_ELEM_ACCESS_SPK_LED) { - led_trigger_type = LED_AUDIO_MUTE; - } else if (access == SNDRV_CTL_ELEM_ACCESS_MIC_LED) { - led_trigger_type = LED_AUDIO_MICMUTE; - } else { - return; - } route = -1; found = false; mutex_lock(&snd_ctl_led_mutex); @@ -115,7 +136,7 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access, mutex_unlock(&snd_ctl_led_mutex); return; } - list_for_each_entry(lctl, controls, list) { + list_for_each_entry(lctl, &led->controls, list) { if (lctl->kctl == kctl && lctl->index_offset == ioff) found = true; UPDATE_ROUTE(route, snd_ctl_led_get(lctl)); @@ -127,23 +148,29 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access, lctl->access = access; lctl->kctl = kctl; lctl->index_offset = ioff; - list_add(&lctl->list, controls); + list_add(&lctl->list, &led->controls); UPDATE_ROUTE(route, snd_ctl_led_get(lctl)); } } mutex_unlock(&snd_ctl_led_mutex); + switch (led->mode) { + case MODE_OFF: route = 1; break; + case MODE_ON: route = 0; break; + case MODE_FOLLOW_ROUTE: if (route >= 0) route ^= 1; break; + case MODE_FOLLOW_MUTE: /* noop */ break; + } if (route >= 0) - ledtrig_audio_set(led_trigger_type, route ? LED_OFF : LED_ON); + ledtrig_audio_set(led->trigger_type, route ? LED_OFF : LED_ON); }
-static struct snd_ctl_led *snd_ctl_led_find(struct snd_kcontrol *kctl, unsigned int ioff) +static struct snd_ctl_led_ctl *snd_ctl_led_find(struct snd_kcontrol *kctl, unsigned int ioff) { struct list_head *controls; - struct snd_ctl_led *lctl; + struct snd_ctl_led_ctl *lctl; unsigned int group;
for (group = 0; group < MAX_LED; group++) { - controls = &snd_ctl_led_controls[group]; + controls = &snd_ctl_leds[group].controls; list_for_each_entry(lctl, controls, list) if (lctl->kctl == kctl && lctl->index_offset == ioff) return lctl; @@ -154,7 +181,7 @@ static struct snd_ctl_led *snd_ctl_led_find(struct snd_kcontrol *kctl, unsigned static unsigned int snd_ctl_led_remove(struct snd_kcontrol *kctl, unsigned int ioff, unsigned int access) { - struct snd_ctl_led *lctl; + struct snd_ctl_led_ctl *lctl; unsigned int ret = 0;
mutex_lock(&snd_ctl_led_mutex); @@ -206,13 +233,13 @@ static void snd_ctl_led_refresh(void) static void snd_ctl_led_clean(struct snd_card *card) { unsigned int group; - struct list_head *controls; - struct snd_ctl_led *lctl; + struct snd_ctl_led *led; + struct snd_ctl_led_ctl *lctl;
for (group = 0; group < MAX_LED; group++) { - controls = &snd_ctl_led_controls[group]; + led = &snd_ctl_leds[group]; repeat: - list_for_each_entry(lctl, controls, list) + list_for_each_entry(lctl, &led->controls, list) if (!card || lctl->card == card) { list_del(&lctl->list); kfree(lctl); @@ -248,6 +275,82 @@ static void snd_ctl_led_disconnect(struct snd_card *card) snd_ctl_led_refresh(); }
+/* + * sysfs + */ + +static ssize_t show_mode(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct snd_ctl_led *led = container_of(dev, struct snd_ctl_led, dev); + const char *str; + + switch (led->mode) { + case MODE_FOLLOW_MUTE: str = "follow-mute"; break; + case MODE_FOLLOW_ROUTE: str = "follow-route"; break; + case MODE_ON: str = "on"; break; + case MODE_OFF: str = "off"; break; + } + return sprintf(buf, "%s\n", str); +} + +static ssize_t store_mode(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct snd_ctl_led *led = container_of(dev, struct snd_ctl_led, dev); + char _buf[16]; + size_t l = min(count, sizeof(_buf) - 1) + 1; + enum snd_ctl_led_mode mode; + + memcpy(_buf, buf, l); + _buf[l] = '\0'; + if (strstr(_buf, "mute")) + mode = MODE_FOLLOW_MUTE; + else if (strstr(_buf, "route")) + mode = MODE_FOLLOW_ROUTE; + else if (strncmp(_buf, "off", 3) == 0 || strncmp(_buf, "0", 1) == 0) + mode = MODE_OFF; + else if (strncmp(_buf, "on", 2) == 0 || strncmp(_buf, "1", 1) == 0) + mode = MODE_ON; + else + return count; + + mutex_lock(&snd_ctl_led_mutex); + led->mode = mode; + mutex_unlock(&snd_ctl_led_mutex); + + snd_ctl_led_set_state(NULL, group_to_access(led->group), NULL, 0); + return count; +} + +static ssize_t show_brightness(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct snd_ctl_led *led = container_of(dev, struct snd_ctl_led, dev); + + return sprintf(buf, "%u\n", ledtrig_audio_get(led->trigger_type)); +} + +static DEVICE_ATTR(mode, 0644, show_mode, store_mode); +static DEVICE_ATTR(brightness, 0444, show_brightness, NULL); + +static struct attribute *snd_ctl_led_dev_attrs[] = { + &dev_attr_mode.attr, + &dev_attr_brightness.attr, + NULL, +}; + +static const struct attribute_group snd_ctl_led_dev_attr_group = { + .attrs = snd_ctl_led_dev_attrs, +}; + +static const struct attribute_group *snd_ctl_led_dev_attr_groups[] = { + &snd_ctl_led_dev_attr_group, + NULL, +}; + +static struct device snd_ctl_led_dev; + /* * Control layer registration */ @@ -260,16 +363,47 @@ static struct snd_ctl_layer_ops snd_ctl_led_lops = {
static int __init snd_ctl_led_init(void) { + struct snd_ctl_led *led; unsigned int group;
- for (group = 0; group < MAX_LED; group++) - INIT_LIST_HEAD(&snd_ctl_led_controls[group]); + device_initialize(&snd_ctl_led_dev); + snd_ctl_led_dev.class = sound_class; + dev_set_name(&snd_ctl_led_dev, "ctl-led"); + if (device_add(&snd_ctl_led_dev)) { + put_device(&snd_ctl_led_dev); + return -ENOMEM; + } + for (group = 0; group < MAX_LED; group++) { + led = &snd_ctl_leds[group]; + INIT_LIST_HEAD(&led->controls); + device_initialize(&led->dev); + led->dev.parent = &snd_ctl_led_dev; + led->dev.groups = snd_ctl_led_dev_attr_groups; + dev_set_name(&led->dev, led->name); + if (device_add(&led->dev)) { + put_device(&led->dev); + for (; group > 0; group--) { + led = &snd_ctl_leds[group]; + device_del(&led->dev); + } + device_del(&snd_ctl_led_dev); + return -ENOMEM; + } + } snd_ctl_register_layer(&snd_ctl_led_lops); return 0; }
static void __exit snd_ctl_led_exit(void) { + struct snd_ctl_led *led; + unsigned int group; + + for (group = 0; group < MAX_LED; group++) { + led = &snd_ctl_leds[group]; + device_del(&led->dev); + } + device_del(&snd_ctl_led_dev); snd_ctl_disconnect_layer(&snd_ctl_led_lops); snd_ctl_led_clean(NULL); }
We need to manage the kcontrol entries association for the LED trigger from the user space. This patch adds a layer to the sysfs tree like:
/sys/devices/virtual/sound/ctl-led/mic + card0 | + attach | + detach | ... + card1 + attach ...
Operations:
attach and detach - amixer style ID is accepted and easy strings for numid and simple names reset - reset all associated kcontrol entries list - list associated kcontrol entries (numid values only)
Additional symlinks:
/sys/devices/virtual/sound/ctl-led/mic/card0/card -> /sys/class/sound/card0
/sys/class/sound/card0/controlC0/led-mic -> /sys/devices/virtual/sound/ctl-led/mic/card0
Signed-off-by: Jaroslav Kysela perex@perex.cz --- sound/core/control_led.c | 366 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 362 insertions(+), 4 deletions(-)
diff --git a/sound/core/control_led.c b/sound/core/control_led.c index dfa51d8461e1..d4fb8b873f34 100644 --- a/sound/core/control_led.c +++ b/sound/core/control_led.c @@ -24,6 +24,12 @@ enum snd_ctl_led_mode { MODE_ON, };
+struct snd_ctl_led_card { + struct device dev; + int number; + struct snd_ctl_led *led; +}; + struct snd_ctl_led { struct device dev; struct list_head controls; @@ -31,6 +37,7 @@ struct snd_ctl_led { unsigned int group; enum led_audio trigger_type; enum snd_ctl_led_mode mode; + struct snd_ctl_led_card *cards[SNDRV_CARDS]; };
struct snd_ctl_led_ctl { @@ -58,6 +65,9 @@ static struct snd_ctl_led snd_ctl_leds[MAX_LED] = { }, };
+static void snd_ctl_led_sysfs_add(struct snd_card *card); +static void snd_ctl_led_sysfs_remove(struct snd_card *card); + #define UPDATE_ROUTE(route, cb) \ do { \ int route2 = (cb); \ @@ -222,6 +232,46 @@ static void snd_ctl_led_notify(struct snd_card *card, unsigned int mask, } }
+static int snd_ctl_led_set_id(int card_number, struct snd_ctl_elem_id *id, + unsigned int group, bool set) +{ + struct snd_card *card; + struct snd_kcontrol *kctl; + struct snd_kcontrol_volatile *vd; + unsigned int ioff, access, new_access; + int err = 0; + + card = snd_card_ref(card_number); + if (card) { + down_write(&card->controls_rwsem); + kctl = snd_ctl_find_id(card, id); + if (kctl) { + ioff = snd_ctl_get_ioff(kctl, id); + vd = &kctl->vd[ioff]; + access = vd->access & SNDRV_CTL_ELEM_ACCESS_LED_MASK; + if (access != 0 && access != group_to_access(group)) { + err = -EXDEV; + goto unlock; + } + new_access = vd->access & ~SNDRV_CTL_ELEM_ACCESS_LED_MASK; + if (set) + new_access |= group_to_access(group); + if (new_access != vd->access) { + vd->access = new_access; + snd_ctl_led_notify(card, SNDRV_CTL_EVENT_MASK_INFO, kctl, ioff); + } + } else { + err = -ENOENT; + } +unlock: + up_write(&card->controls_rwsem); + snd_card_unref(card); + } else { + err = -ENXIO; + } + return err; +} + static void snd_ctl_led_refresh(void) { unsigned int group; @@ -230,6 +280,12 @@ static void snd_ctl_led_refresh(void) snd_ctl_led_set_state(NULL, group_to_access(group), NULL, 0); }
+static void snd_ctl_led_ctl_destroy(struct snd_ctl_led_ctl *lctl) +{ + list_del(&lctl->list); + kfree(lctl); +} + static void snd_ctl_led_clean(struct snd_card *card) { unsigned int group; @@ -241,13 +297,47 @@ static void snd_ctl_led_clean(struct snd_card *card) repeat: list_for_each_entry(lctl, &led->controls, list) if (!card || lctl->card == card) { - list_del(&lctl->list); - kfree(lctl); + snd_ctl_led_ctl_destroy(lctl); goto repeat; } } }
+static int snd_ctl_led_reset(int card_number, unsigned int group) +{ + struct snd_card *card; + struct snd_ctl_led *led; + struct snd_ctl_led_ctl *lctl; + struct snd_kcontrol_volatile *vd; + bool change = false; + + card = snd_card_ref(card_number); + if (!card) + return -ENXIO; + + mutex_lock(&snd_ctl_led_mutex); + if (!snd_ctl_led_card_valid[card_number]) { + mutex_unlock(&snd_ctl_led_mutex); + snd_card_unref(card); + return -ENXIO; + } + led = &snd_ctl_leds[group]; +repeat: + list_for_each_entry(lctl, &led->controls, list) + if (lctl->card == card) { + vd = &lctl->kctl->vd[lctl->index_offset]; + vd->access &= ~group_to_access(group); + snd_ctl_led_ctl_destroy(lctl); + change = true; + goto repeat; + } + mutex_unlock(&snd_ctl_led_mutex); + if (change) + snd_ctl_led_set_state(NULL, group_to_access(group), NULL, 0); + snd_card_unref(card); + return 0; +} + static void snd_ctl_led_register(struct snd_card *card) { struct snd_kcontrol *kctl; @@ -264,10 +354,12 @@ static void snd_ctl_led_register(struct snd_card *card) for (ioff = 0; ioff < kctl->count; ioff++) snd_ctl_led_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, kctl, ioff); snd_ctl_led_refresh(); + snd_ctl_led_sysfs_add(card); }
static void snd_ctl_led_disconnect(struct snd_card *card) { + snd_ctl_led_sysfs_remove(card); mutex_lock(&snd_ctl_led_mutex); snd_ctl_led_card_valid[card->number] = false; snd_ctl_led_clean(card); @@ -349,8 +441,264 @@ static const struct attribute_group *snd_ctl_led_dev_attr_groups[] = { NULL, };
+static char *find_eos(char *s) +{ + while (*s && *s != ',') + s++; + if (*s) + s++; + return s; +} + +static char *parse_uint(char *s, unsigned int *val) +{ + unsigned long long res; + if (kstrtoull(s, 10, &res)) + res = 0; + *val = res; + return find_eos(s); +} + +static char *parse_string(char *s, char *val, size_t val_size) +{ + if (*s == '"' || *s == ''') { + char c = *s; + s++; + while (*s && *s != c) { + if (val_size > 1) { + *val++ = *s; + val_size--; + } + s++; + } + } else { + while (*s && *s != ',') { + if (val_size > 1) { + *val++ = *s; + val_size--; + } + s++; + } + } + *val = '\0'; + if (*s) + s++; + return s; +} + +static char *parse_iface(char *s, unsigned int *val) +{ + if (!strncasecmp(s, "card", 4)) + *val = SNDRV_CTL_ELEM_IFACE_CARD; + else if (!strncasecmp(s, "mixer", 5)) + *val = SNDRV_CTL_ELEM_IFACE_MIXER; + return find_eos(s); +} + +/* + * These types of input strings are accepted: + * + * unsigned integer - numid (equivaled to numid=UINT) + * string - basic mixer name (equivalent to iface=MIXER,name=STR) + * numid=UINT + * [iface=MIXER,][device=UINT,][subdevice=UINT,]name=STR[,index=UINT] + */ +static ssize_t set_led_id(struct snd_ctl_led_card *led_card, const char *buf, size_t count, + bool attach) +{ + char buf2[256], *s; + size_t len = max(sizeof(s) - 1, count); + struct snd_ctl_elem_id id; + int err; + + strncpy(buf2, buf, len); + buf2[len] = '\0'; + memset(&id, 0, sizeof(id)); + id.iface = SNDRV_CTL_ELEM_IFACE_MIXER; + s = buf2; + while (*s) { + if (!strncasecmp(s, "numid=", 6)) { + s = parse_uint(s + 6, &id.numid); + } else if (!strncasecmp(s, "iface=", 6)) { + s = parse_iface(s + 6, &id.iface); + } else if (!strncasecmp(s, "device=", 7)) { + s = parse_uint(s + 7, &id.device); + } else if (!strncasecmp(s, "subdevice=", 10)) { + s = parse_uint(s + 10, &id.subdevice); + } else if (!strncasecmp(s, "name=", 5)) { + s = parse_string(s + 5, id.name, sizeof(id.name)); + } else if (!strncasecmp(s, "index=", 6)) { + s = parse_uint(s + 6, &id.index); + } else if (s == buf2) { + while (*s) { + if (*s < '0' || *s > '9') + break; + s++; + } + if (*s == '\0') + parse_uint(buf2, &id.numid); + else { + for (; *s >= ' '; s++); + *s = '\0'; + strlcpy(id.name, buf2, sizeof(id.name)); + } + break; + } + if (*s == ',') + s++; + } + + err = snd_ctl_led_set_id(led_card->number, &id, led_card->led->group, attach); + if (err < 0) + return err; + + return count; +} + +static ssize_t parse_attach(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev); + return set_led_id(led_card, buf, count, true); +} + +static ssize_t parse_detach(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev); + return set_led_id(led_card, buf, count, false); +} + +static ssize_t ctl_reset(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev); + int err; + + if (count > 0 && buf[0] == '1') { + err = snd_ctl_led_reset(led_card->number, led_card->led->group); + if (err < 0) + return err; + } + return count; +} + +static ssize_t ctl_list(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev); + struct snd_card *card; + struct snd_ctl_led_ctl *lctl; + char *buf2 = buf; + size_t l; + + card = snd_card_ref(led_card->number); + if (!card) + return -ENXIO; + down_read(&card->controls_rwsem); + mutex_lock(&snd_ctl_led_mutex); + if (snd_ctl_led_card_valid[led_card->number]) { + list_for_each_entry(lctl, &led_card->led->controls, list) + if (lctl->card == card) { + if (buf2 - buf > PAGE_SIZE - 16) + break; + if (buf2 != buf) + *buf2++ = ' '; + l = scnprintf(buf2, 15, "%u", + lctl->kctl->id.numid + + lctl->index_offset); + buf2[l] = '\0'; + buf2 += l + 1; + } + } + mutex_unlock(&snd_ctl_led_mutex); + up_read(&card->controls_rwsem); + snd_card_unref(card); + return buf2 - buf; +} + +static DEVICE_ATTR(attach, 0200, NULL, parse_attach); +static DEVICE_ATTR(detach, 0200, NULL, parse_detach); +static DEVICE_ATTR(reset, 0200, NULL, ctl_reset); +static DEVICE_ATTR(list, 0444, ctl_list, NULL); + +static struct attribute *snd_ctl_led_card_attrs[] = { + &dev_attr_attach.attr, + &dev_attr_detach.attr, + &dev_attr_reset.attr, + &dev_attr_list.attr, + NULL, +}; + +static const struct attribute_group snd_ctl_led_card_attr_group = { + .attrs = snd_ctl_led_card_attrs, +}; + +static const struct attribute_group *snd_ctl_led_card_attr_groups[] = { + &snd_ctl_led_card_attr_group, + NULL, +}; + static struct device snd_ctl_led_dev;
+static void snd_ctl_led_sysfs_add(struct snd_card *card) +{ + unsigned int group; + struct snd_ctl_led_card *led_card; + struct snd_ctl_led *led; + char link_name[32]; + + for (group = 0; group < MAX_LED; group++) { + led = &snd_ctl_leds[group]; + led_card = kzalloc(sizeof(*led_card), GFP_KERNEL); + if (!led_card) + goto cerr2; + led_card->number = card->number; + led_card->led = led; + device_initialize(&led_card->dev); + if (dev_set_name(&led_card->dev, "card%d", card->number) < 0) + goto cerr; + led_card->dev.parent = &led->dev; + led_card->dev.groups = snd_ctl_led_card_attr_groups; + if (device_add(&led_card->dev)) + goto cerr; + led->cards[card->number] = led_card; + snprintf(link_name, sizeof(link_name), "led-%s", led->name); + WARN(sysfs_create_link(&card->ctl_dev.kobj, &led_card->dev.kobj, link_name), + "can't create symlink to controlC%i device\n", card->number); + WARN(sysfs_create_link(&led_card->dev.kobj, &card->card_dev.kobj, "card"), + "can't create symlink to card%i\n", card->number); + + continue; +cerr: + put_device(&led_card->dev); +cerr2: + printk(KERN_ERR "snd_ctl_led: unable to add card%d", card->number); + kfree(led_card); + } +} + +static void snd_ctl_led_sysfs_remove(struct snd_card *card) +{ + unsigned int group; + struct snd_ctl_led_card *led_card; + struct snd_ctl_led *led; + char link_name[32]; + + for (group = 0; group < MAX_LED; group++) { + led = &snd_ctl_leds[group]; + led_card = led->cards[card->number]; + if (!led_card) + continue; + snprintf(link_name, sizeof(link_name), "led-%s", led->name); + sysfs_remove_link(&card->ctl_dev.kobj, link_name); + sysfs_remove_link(&led_card->dev.kobj, "card"); + device_del(&led_card->dev); + kfree(led_card); + led->cards[card->number] = NULL; + } +} + /* * Control layer registration */ @@ -397,14 +745,24 @@ static int __init snd_ctl_led_init(void) static void __exit snd_ctl_led_exit(void) { struct snd_ctl_led *led; - unsigned int group; + struct snd_card *card; + unsigned int group, card_number;
+ snd_ctl_disconnect_layer(&snd_ctl_led_lops); + for (card_number = 0; card_number < SNDRV_CARDS; card_number++) { + if (!snd_ctl_led_card_valid[card_number]) + continue; + card = snd_card_ref(card_number); + if (card) { + snd_ctl_led_sysfs_remove(card); + snd_card_unref(card); + } + } for (group = 0; group < MAX_LED; group++) { led = &snd_ctl_leds[group]; device_del(&led->dev); } device_del(&snd_ctl_led_dev); - snd_ctl_disconnect_layer(&snd_ctl_led_lops); snd_ctl_led_clean(NULL); }
Hi,
On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
We need to manage the kcontrol entries association for the LED trigger from the user space. This patch adds a layer to the sysfs tree like:
/sys/devices/virtual/sound/ctl-led/mic
- card0
| + attach | + detach | ...
- card1
...
- attach
Operations:
attach and detach - amixer style ID is accepted and easy strings for numid and simple names reset - reset all associated kcontrol entries list - list associated kcontrol entries (numid values only)
Additional symlinks:
/sys/devices/virtual/sound/ctl-led/mic/card0/card -> /sys/class/sound/card0
/sys/class/sound/card0/controlC0/led-mic -> /sys/devices/virtual/sound/ctl-led/mic/card0
Signed-off-by: Jaroslav Kysela perex@perex.cz
Thank you so much for this patch.
I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute LED build into the (detachable) USB-keyboard's mute hot-key:
modprobe snd_ctl_led echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
This needs to be replaced by some UCM profile code doing the equivalent of course, but for a proof-of-concept test of the kernel API this introduces the above will do.
Only complaint which I have is the need to add "-n" to the echo commands, it would be nice if set_led_id() would check if the copy which it stores in buf2 ends with "\n" and if it does if it would then strip that from the copy in buf2.
Regards,
Hans
p.s.
Note this does need my recently listed alsa=lib patches so that these "Channel Switch" controls get grouped with the "Speaker Playback Volume" / "HP Playback Volume" controls, so that the volume-hw control code will actually toggle them:
https://lore.kernel.org/alsa-devel/20210307133005.30801-1-hdegoede@redhat.co...
Talking about that series, what is the status of that ? From my POV it is ready for merging...
sound/core/control_led.c | 366 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 362 insertions(+), 4 deletions(-)
diff --git a/sound/core/control_led.c b/sound/core/control_led.c index dfa51d8461e1..d4fb8b873f34 100644 --- a/sound/core/control_led.c +++ b/sound/core/control_led.c @@ -24,6 +24,12 @@ enum snd_ctl_led_mode { MODE_ON, };
+struct snd_ctl_led_card {
- struct device dev;
- int number;
- struct snd_ctl_led *led;
+};
struct snd_ctl_led { struct device dev; struct list_head controls; @@ -31,6 +37,7 @@ struct snd_ctl_led { unsigned int group; enum led_audio trigger_type; enum snd_ctl_led_mode mode;
- struct snd_ctl_led_card *cards[SNDRV_CARDS];
};
struct snd_ctl_led_ctl { @@ -58,6 +65,9 @@ static struct snd_ctl_led snd_ctl_leds[MAX_LED] = { }, };
+static void snd_ctl_led_sysfs_add(struct snd_card *card); +static void snd_ctl_led_sysfs_remove(struct snd_card *card);
#define UPDATE_ROUTE(route, cb) \ do { \ int route2 = (cb); \ @@ -222,6 +232,46 @@ static void snd_ctl_led_notify(struct snd_card *card, unsigned int mask, } }
+static int snd_ctl_led_set_id(int card_number, struct snd_ctl_elem_id *id,
unsigned int group, bool set)+{
- struct snd_card *card;
- struct snd_kcontrol *kctl;
- struct snd_kcontrol_volatile *vd;
- unsigned int ioff, access, new_access;
- int err = 0;
- card = snd_card_ref(card_number);
- if (card) {
down_write(&card->controls_rwsem);kctl = snd_ctl_find_id(card, id);if (kctl) {ioff = snd_ctl_get_ioff(kctl, id);vd = &kctl->vd[ioff];access = vd->access & SNDRV_CTL_ELEM_ACCESS_LED_MASK;if (access != 0 && access != group_to_access(group)) {err = -EXDEV;goto unlock;}new_access = vd->access & ~SNDRV_CTL_ELEM_ACCESS_LED_MASK;if (set)new_access |= group_to_access(group);if (new_access != vd->access) {vd->access = new_access;snd_ctl_led_notify(card, SNDRV_CTL_EVENT_MASK_INFO, kctl, ioff);}} else {err = -ENOENT;}+unlock:
up_write(&card->controls_rwsem);snd_card_unref(card);- } else {
err = -ENXIO;- }
- return err;
+}
static void snd_ctl_led_refresh(void) { unsigned int group; @@ -230,6 +280,12 @@ static void snd_ctl_led_refresh(void) snd_ctl_led_set_state(NULL, group_to_access(group), NULL, 0); }
+static void snd_ctl_led_ctl_destroy(struct snd_ctl_led_ctl *lctl) +{
- list_del(&lctl->list);
- kfree(lctl);
+}
static void snd_ctl_led_clean(struct snd_card *card) { unsigned int group; @@ -241,13 +297,47 @@ static void snd_ctl_led_clean(struct snd_card *card) repeat: list_for_each_entry(lctl, &led->controls, list) if (!card || lctl->card == card) {
list_del(&lctl->list);kfree(lctl);
snd_ctl_led_ctl_destroy(lctl); goto repeat; }}
+static int snd_ctl_led_reset(int card_number, unsigned int group) +{
- struct snd_card *card;
- struct snd_ctl_led *led;
- struct snd_ctl_led_ctl *lctl;
- struct snd_kcontrol_volatile *vd;
- bool change = false;
- card = snd_card_ref(card_number);
- if (!card)
return -ENXIO;- mutex_lock(&snd_ctl_led_mutex);
- if (!snd_ctl_led_card_valid[card_number]) {
mutex_unlock(&snd_ctl_led_mutex);snd_card_unref(card);return -ENXIO;- }
- led = &snd_ctl_leds[group];
+repeat:
- list_for_each_entry(lctl, &led->controls, list)
if (lctl->card == card) {vd = &lctl->kctl->vd[lctl->index_offset];vd->access &= ~group_to_access(group);snd_ctl_led_ctl_destroy(lctl);change = true;goto repeat;}- mutex_unlock(&snd_ctl_led_mutex);
- if (change)
snd_ctl_led_set_state(NULL, group_to_access(group), NULL, 0);- snd_card_unref(card);
- return 0;
+}
static void snd_ctl_led_register(struct snd_card *card) { struct snd_kcontrol *kctl; @@ -264,10 +354,12 @@ static void snd_ctl_led_register(struct snd_card *card) for (ioff = 0; ioff < kctl->count; ioff++) snd_ctl_led_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, kctl, ioff); snd_ctl_led_refresh();
- snd_ctl_led_sysfs_add(card);
}
static void snd_ctl_led_disconnect(struct snd_card *card) {
- snd_ctl_led_sysfs_remove(card); mutex_lock(&snd_ctl_led_mutex); snd_ctl_led_card_valid[card->number] = false; snd_ctl_led_clean(card);
@@ -349,8 +441,264 @@ static const struct attribute_group *snd_ctl_led_dev_attr_groups[] = { NULL, };
+static char *find_eos(char *s) +{
- while (*s && *s != ',')
s++;- if (*s)
s++;- return s;
+}
+static char *parse_uint(char *s, unsigned int *val) +{
- unsigned long long res;
- if (kstrtoull(s, 10, &res))
res = 0;- *val = res;
- return find_eos(s);
+}
+static char *parse_string(char *s, char *val, size_t val_size) +{
- if (*s == '"' || *s == ''') {
char c = *s;s++;while (*s && *s != c) {if (val_size > 1) {*val++ = *s;val_size--;}s++;}- } else {
while (*s && *s != ',') {if (val_size > 1) {*val++ = *s;val_size--;}s++;}- }
- *val = '\0';
- if (*s)
s++;- return s;
+}
+static char *parse_iface(char *s, unsigned int *val) +{
- if (!strncasecmp(s, "card", 4))
*val = SNDRV_CTL_ELEM_IFACE_CARD;- else if (!strncasecmp(s, "mixer", 5))
*val = SNDRV_CTL_ELEM_IFACE_MIXER;- return find_eos(s);
+}
+/*
- These types of input strings are accepted:
- unsigned integer - numid (equivaled to numid=UINT)
- string - basic mixer name (equivalent to iface=MIXER,name=STR)
- numid=UINT
- [iface=MIXER,][device=UINT,][subdevice=UINT,]name=STR[,index=UINT]
- */
+static ssize_t set_led_id(struct snd_ctl_led_card *led_card, const char *buf, size_t count,
bool attach)+{
- char buf2[256], *s;
- size_t len = max(sizeof(s) - 1, count);
- struct snd_ctl_elem_id id;
- int err;
- strncpy(buf2, buf, len);
- buf2[len] = '\0';
- memset(&id, 0, sizeof(id));
- id.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
- s = buf2;
- while (*s) {
if (!strncasecmp(s, "numid=", 6)) {s = parse_uint(s + 6, &id.numid);} else if (!strncasecmp(s, "iface=", 6)) {s = parse_iface(s + 6, &id.iface);} else if (!strncasecmp(s, "device=", 7)) {s = parse_uint(s + 7, &id.device);} else if (!strncasecmp(s, "subdevice=", 10)) {s = parse_uint(s + 10, &id.subdevice);} else if (!strncasecmp(s, "name=", 5)) {s = parse_string(s + 5, id.name, sizeof(id.name));} else if (!strncasecmp(s, "index=", 6)) {s = parse_uint(s + 6, &id.index);} else if (s == buf2) {while (*s) {if (*s < '0' || *s > '9')break;s++;}if (*s == '\0')parse_uint(buf2, &id.numid);else {for (; *s >= ' '; s++);*s = '\0';strlcpy(id.name, buf2, sizeof(id.name));}break;}if (*s == ',')s++;- }
- err = snd_ctl_led_set_id(led_card->number, &id, led_card->led->group, attach);
- if (err < 0)
return err;- return count;
+}
+static ssize_t parse_attach(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)+{
- struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
- return set_led_id(led_card, buf, count, true);
+}
+static ssize_t parse_detach(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)+{
- struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
- return set_led_id(led_card, buf, count, false);
+}
+static ssize_t ctl_reset(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)+{
- struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
- int err;
- if (count > 0 && buf[0] == '1') {
err = snd_ctl_led_reset(led_card->number, led_card->led->group);if (err < 0)return err;- }
- return count;
+}
+static ssize_t ctl_list(struct device *dev,
struct device_attribute *attr, char *buf)+{
- struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
- struct snd_card *card;
- struct snd_ctl_led_ctl *lctl;
- char *buf2 = buf;
- size_t l;
- card = snd_card_ref(led_card->number);
- if (!card)
return -ENXIO;- down_read(&card->controls_rwsem);
- mutex_lock(&snd_ctl_led_mutex);
- if (snd_ctl_led_card_valid[led_card->number]) {
list_for_each_entry(lctl, &led_card->led->controls, list)if (lctl->card == card) {if (buf2 - buf > PAGE_SIZE - 16)break;if (buf2 != buf)*buf2++ = ' ';l = scnprintf(buf2, 15, "%u",lctl->kctl->id.numid +lctl->index_offset);buf2[l] = '\0';buf2 += l + 1;}- }
- mutex_unlock(&snd_ctl_led_mutex);
- up_read(&card->controls_rwsem);
- snd_card_unref(card);
- return buf2 - buf;
+}
+static DEVICE_ATTR(attach, 0200, NULL, parse_attach); +static DEVICE_ATTR(detach, 0200, NULL, parse_detach); +static DEVICE_ATTR(reset, 0200, NULL, ctl_reset); +static DEVICE_ATTR(list, 0444, ctl_list, NULL);
+static struct attribute *snd_ctl_led_card_attrs[] = {
- &dev_attr_attach.attr,
- &dev_attr_detach.attr,
- &dev_attr_reset.attr,
- &dev_attr_list.attr,
- NULL,
+};
+static const struct attribute_group snd_ctl_led_card_attr_group = {
- .attrs = snd_ctl_led_card_attrs,
+};
+static const struct attribute_group *snd_ctl_led_card_attr_groups[] = {
- &snd_ctl_led_card_attr_group,
- NULL,
+};
static struct device snd_ctl_led_dev;
+static void snd_ctl_led_sysfs_add(struct snd_card *card) +{
- unsigned int group;
- struct snd_ctl_led_card *led_card;
- struct snd_ctl_led *led;
- char link_name[32];
- for (group = 0; group < MAX_LED; group++) {
led = &snd_ctl_leds[group];led_card = kzalloc(sizeof(*led_card), GFP_KERNEL);if (!led_card)goto cerr2;led_card->number = card->number;led_card->led = led;device_initialize(&led_card->dev);if (dev_set_name(&led_card->dev, "card%d", card->number) < 0)goto cerr;led_card->dev.parent = &led->dev;led_card->dev.groups = snd_ctl_led_card_attr_groups;if (device_add(&led_card->dev))goto cerr;led->cards[card->number] = led_card;snprintf(link_name, sizeof(link_name), "led-%s", led->name);WARN(sysfs_create_link(&card->ctl_dev.kobj, &led_card->dev.kobj, link_name),"can't create symlink to controlC%i device\n", card->number);WARN(sysfs_create_link(&led_card->dev.kobj, &card->card_dev.kobj, "card"),"can't create symlink to card%i\n", card->number);continue;+cerr:
put_device(&led_card->dev);+cerr2:
printk(KERN_ERR "snd_ctl_led: unable to add card%d", card->number);kfree(led_card);- }
+}
+static void snd_ctl_led_sysfs_remove(struct snd_card *card) +{
- unsigned int group;
- struct snd_ctl_led_card *led_card;
- struct snd_ctl_led *led;
- char link_name[32];
- for (group = 0; group < MAX_LED; group++) {
led = &snd_ctl_leds[group];led_card = led->cards[card->number];if (!led_card)continue;snprintf(link_name, sizeof(link_name), "led-%s", led->name);sysfs_remove_link(&card->ctl_dev.kobj, link_name);sysfs_remove_link(&led_card->dev.kobj, "card");device_del(&led_card->dev);kfree(led_card);led->cards[card->number] = NULL;- }
+}
/*
- Control layer registration
*/ @@ -397,14 +745,24 @@ static int __init snd_ctl_led_init(void) static void __exit snd_ctl_led_exit(void) { struct snd_ctl_led *led;
- unsigned int group;
struct snd_card *card;
unsigned int group, card_number;
snd_ctl_disconnect_layer(&snd_ctl_led_lops);
for (card_number = 0; card_number < SNDRV_CARDS; card_number++) {
if (!snd_ctl_led_card_valid[card_number])continue;card = snd_card_ref(card_number);if (card) {snd_ctl_led_sysfs_remove(card);snd_card_unref(card);}} for (group = 0; group < MAX_LED; group++) { led = &snd_ctl_leds[group]; device_del(&led->dev); } device_del(&snd_ctl_led_dev);
- snd_ctl_disconnect_layer(&snd_ctl_led_lops); snd_ctl_led_clean(NULL);
}
On Fri, 19 Mar 2021 17:34:39 +0100, Hans de Goede wrote:
Hi,
On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
We need to manage the kcontrol entries association for the LED trigger from the user space. This patch adds a layer to the sysfs tree like:
/sys/devices/virtual/sound/ctl-led/mic
- card0
| + attach | + detach | ...
- card1
...
- attach
Operations:
attach and detach - amixer style ID is accepted and easy strings for numid and simple names reset - reset all associated kcontrol entries list - list associated kcontrol entries (numid values only)
Additional symlinks:
/sys/devices/virtual/sound/ctl-led/mic/card0/card -> /sys/class/sound/card0
/sys/class/sound/card0/controlC0/led-mic -> /sys/devices/virtual/sound/ctl-led/mic/card0
Signed-off-by: Jaroslav Kysela perex@perex.cz
Thank you so much for this patch.
I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute LED build into the (detachable) USB-keyboard's mute hot-key:
modprobe snd_ctl_led echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
This needs to be replaced by some UCM profile code doing the equivalent of course, but for a proof-of-concept test of the kernel API this introduces the above will do.
IMO, that's the question: how we'll enable this in future. If the binding of the control/mute mapping is provided via UCM, it's supposed to be changeable by each user. Then the current sysfs permission doesn't fit. OTOH, if it's 0666, it's accessible to all users even remotely, which is worse than the access with the normal sound device file. Or if it's supposed to be changed via udev stuff or systemd? Or is it just for debugging?
Through a quick glance over the series, I'm fine to take those patches, but the only concern is the sysfs entries. Basically, once when we use sysfs entries, it's set in stone. So we should be very clear about our strategy how to deploy the control/mute mapping regarding using those sysfs entries.
OTOH, if the interface is thought for debugging or development purpose, it could be done in debugfs, which we can keep playing in further development, too.
And, BTW, the mute LED mode setup doesn't have to be sysfs entries; we'd need primarily only the flags for inverted LED behavior, and those are only two, so it could be simply module options. Then it's even easier for users to set up than tweaking sysfs entries.
thanks,
Takashi
Only complaint which I have is the need to add "-n" to the echo commands, it would be nice if set_led_id() would check if the copy which it stores in buf2 ends with "\n" and if it does if it would then strip that from the copy in buf2.
Regards,
Hans
p.s.
Note this does need my recently listed alsa=lib patches so that these "Channel Switch" controls get grouped with the "Speaker Playback Volume" / "HP Playback Volume" controls, so that the volume-hw control code will actually toggle them:
https://lore.kernel.org/alsa-devel/20210307133005.30801-1-hdegoede@redhat.co...
Talking about that series, what is the status of that ? From my POV it is ready for merging...
sound/core/control_led.c | 366 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 362 insertions(+), 4 deletions(-)
diff --git a/sound/core/control_led.c b/sound/core/control_led.c index dfa51d8461e1..d4fb8b873f34 100644 --- a/sound/core/control_led.c +++ b/sound/core/control_led.c @@ -24,6 +24,12 @@ enum snd_ctl_led_mode { MODE_ON, };
+struct snd_ctl_led_card {
- struct device dev;
- int number;
- struct snd_ctl_led *led;
+};
struct snd_ctl_led { struct device dev; struct list_head controls; @@ -31,6 +37,7 @@ struct snd_ctl_led { unsigned int group; enum led_audio trigger_type; enum snd_ctl_led_mode mode;
- struct snd_ctl_led_card *cards[SNDRV_CARDS];
};
struct snd_ctl_led_ctl { @@ -58,6 +65,9 @@ static struct snd_ctl_led snd_ctl_leds[MAX_LED] = { }, };
+static void snd_ctl_led_sysfs_add(struct snd_card *card); +static void snd_ctl_led_sysfs_remove(struct snd_card *card);
#define UPDATE_ROUTE(route, cb) \ do { \ int route2 = (cb); \ @@ -222,6 +232,46 @@ static void snd_ctl_led_notify(struct snd_card *card, unsigned int mask, } }
+static int snd_ctl_led_set_id(int card_number, struct snd_ctl_elem_id *id,
unsigned int group, bool set)+{
- struct snd_card *card;
- struct snd_kcontrol *kctl;
- struct snd_kcontrol_volatile *vd;
- unsigned int ioff, access, new_access;
- int err = 0;
- card = snd_card_ref(card_number);
- if (card) {
down_write(&card->controls_rwsem);kctl = snd_ctl_find_id(card, id);if (kctl) {ioff = snd_ctl_get_ioff(kctl, id);vd = &kctl->vd[ioff];access = vd->access & SNDRV_CTL_ELEM_ACCESS_LED_MASK;if (access != 0 && access != group_to_access(group)) {err = -EXDEV;goto unlock;}new_access = vd->access & ~SNDRV_CTL_ELEM_ACCESS_LED_MASK;if (set)new_access |= group_to_access(group);if (new_access != vd->access) {vd->access = new_access;snd_ctl_led_notify(card, SNDRV_CTL_EVENT_MASK_INFO, kctl, ioff);}} else {err = -ENOENT;}+unlock:
up_write(&card->controls_rwsem);snd_card_unref(card);- } else {
err = -ENXIO;- }
- return err;
+}
static void snd_ctl_led_refresh(void) { unsigned int group; @@ -230,6 +280,12 @@ static void snd_ctl_led_refresh(void) snd_ctl_led_set_state(NULL, group_to_access(group), NULL, 0); }
+static void snd_ctl_led_ctl_destroy(struct snd_ctl_led_ctl *lctl) +{
- list_del(&lctl->list);
- kfree(lctl);
+}
static void snd_ctl_led_clean(struct snd_card *card) { unsigned int group; @@ -241,13 +297,47 @@ static void snd_ctl_led_clean(struct snd_card *card) repeat: list_for_each_entry(lctl, &led->controls, list) if (!card || lctl->card == card) {
list_del(&lctl->list);kfree(lctl);
snd_ctl_led_ctl_destroy(lctl); goto repeat; }}
+static int snd_ctl_led_reset(int card_number, unsigned int group) +{
- struct snd_card *card;
- struct snd_ctl_led *led;
- struct snd_ctl_led_ctl *lctl;
- struct snd_kcontrol_volatile *vd;
- bool change = false;
- card = snd_card_ref(card_number);
- if (!card)
return -ENXIO;- mutex_lock(&snd_ctl_led_mutex);
- if (!snd_ctl_led_card_valid[card_number]) {
mutex_unlock(&snd_ctl_led_mutex);snd_card_unref(card);return -ENXIO;- }
- led = &snd_ctl_leds[group];
+repeat:
- list_for_each_entry(lctl, &led->controls, list)
if (lctl->card == card) {vd = &lctl->kctl->vd[lctl->index_offset];vd->access &= ~group_to_access(group);snd_ctl_led_ctl_destroy(lctl);change = true;goto repeat;}- mutex_unlock(&snd_ctl_led_mutex);
- if (change)
snd_ctl_led_set_state(NULL, group_to_access(group), NULL, 0);- snd_card_unref(card);
- return 0;
+}
static void snd_ctl_led_register(struct snd_card *card) { struct snd_kcontrol *kctl; @@ -264,10 +354,12 @@ static void snd_ctl_led_register(struct snd_card *card) for (ioff = 0; ioff < kctl->count; ioff++) snd_ctl_led_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, kctl, ioff); snd_ctl_led_refresh();
- snd_ctl_led_sysfs_add(card);
}
static void snd_ctl_led_disconnect(struct snd_card *card) {
- snd_ctl_led_sysfs_remove(card); mutex_lock(&snd_ctl_led_mutex); snd_ctl_led_card_valid[card->number] = false; snd_ctl_led_clean(card);
@@ -349,8 +441,264 @@ static const struct attribute_group *snd_ctl_led_dev_attr_groups[] = { NULL, };
+static char *find_eos(char *s) +{
- while (*s && *s != ',')
s++;- if (*s)
s++;- return s;
+}
+static char *parse_uint(char *s, unsigned int *val) +{
- unsigned long long res;
- if (kstrtoull(s, 10, &res))
res = 0;- *val = res;
- return find_eos(s);
+}
+static char *parse_string(char *s, char *val, size_t val_size) +{
- if (*s == '"' || *s == ''') {
char c = *s;s++;while (*s && *s != c) {if (val_size > 1) {*val++ = *s;val_size--;}s++;}- } else {
while (*s && *s != ',') {if (val_size > 1) {*val++ = *s;val_size--;}s++;}- }
- *val = '\0';
- if (*s)
s++;- return s;
+}
+static char *parse_iface(char *s, unsigned int *val) +{
- if (!strncasecmp(s, "card", 4))
*val = SNDRV_CTL_ELEM_IFACE_CARD;- else if (!strncasecmp(s, "mixer", 5))
*val = SNDRV_CTL_ELEM_IFACE_MIXER;- return find_eos(s);
+}
+/*
- These types of input strings are accepted:
- unsigned integer - numid (equivaled to numid=UINT)
- string - basic mixer name (equivalent to iface=MIXER,name=STR)
- numid=UINT
- [iface=MIXER,][device=UINT,][subdevice=UINT,]name=STR[,index=UINT]
- */
+static ssize_t set_led_id(struct snd_ctl_led_card *led_card, const char *buf, size_t count,
bool attach)+{
- char buf2[256], *s;
- size_t len = max(sizeof(s) - 1, count);
- struct snd_ctl_elem_id id;
- int err;
- strncpy(buf2, buf, len);
- buf2[len] = '\0';
- memset(&id, 0, sizeof(id));
- id.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
- s = buf2;
- while (*s) {
if (!strncasecmp(s, "numid=", 6)) {s = parse_uint(s + 6, &id.numid);} else if (!strncasecmp(s, "iface=", 6)) {s = parse_iface(s + 6, &id.iface);} else if (!strncasecmp(s, "device=", 7)) {s = parse_uint(s + 7, &id.device);} else if (!strncasecmp(s, "subdevice=", 10)) {s = parse_uint(s + 10, &id.subdevice);} else if (!strncasecmp(s, "name=", 5)) {s = parse_string(s + 5, id.name, sizeof(id.name));} else if (!strncasecmp(s, "index=", 6)) {s = parse_uint(s + 6, &id.index);} else if (s == buf2) {while (*s) {if (*s < '0' || *s > '9')break;s++;}if (*s == '\0')parse_uint(buf2, &id.numid);else {for (; *s >= ' '; s++);*s = '\0';strlcpy(id.name, buf2, sizeof(id.name));}break;}if (*s == ',')s++;- }
- err = snd_ctl_led_set_id(led_card->number, &id, led_card->led->group, attach);
- if (err < 0)
return err;- return count;
+}
+static ssize_t parse_attach(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)+{
- struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
- return set_led_id(led_card, buf, count, true);
+}
+static ssize_t parse_detach(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)+{
- struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
- return set_led_id(led_card, buf, count, false);
+}
+static ssize_t ctl_reset(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)+{
- struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
- int err;
- if (count > 0 && buf[0] == '1') {
err = snd_ctl_led_reset(led_card->number, led_card->led->group);if (err < 0)return err;- }
- return count;
+}
+static ssize_t ctl_list(struct device *dev,
struct device_attribute *attr, char *buf)+{
- struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
- struct snd_card *card;
- struct snd_ctl_led_ctl *lctl;
- char *buf2 = buf;
- size_t l;
- card = snd_card_ref(led_card->number);
- if (!card)
return -ENXIO;- down_read(&card->controls_rwsem);
- mutex_lock(&snd_ctl_led_mutex);
- if (snd_ctl_led_card_valid[led_card->number]) {
list_for_each_entry(lctl, &led_card->led->controls, list)if (lctl->card == card) {if (buf2 - buf > PAGE_SIZE - 16)break;if (buf2 != buf)*buf2++ = ' ';l = scnprintf(buf2, 15, "%u",lctl->kctl->id.numid +lctl->index_offset);buf2[l] = '\0';buf2 += l + 1;}- }
- mutex_unlock(&snd_ctl_led_mutex);
- up_read(&card->controls_rwsem);
- snd_card_unref(card);
- return buf2 - buf;
+}
+static DEVICE_ATTR(attach, 0200, NULL, parse_attach); +static DEVICE_ATTR(detach, 0200, NULL, parse_detach); +static DEVICE_ATTR(reset, 0200, NULL, ctl_reset); +static DEVICE_ATTR(list, 0444, ctl_list, NULL);
+static struct attribute *snd_ctl_led_card_attrs[] = {
- &dev_attr_attach.attr,
- &dev_attr_detach.attr,
- &dev_attr_reset.attr,
- &dev_attr_list.attr,
- NULL,
+};
+static const struct attribute_group snd_ctl_led_card_attr_group = {
- .attrs = snd_ctl_led_card_attrs,
+};
+static const struct attribute_group *snd_ctl_led_card_attr_groups[] = {
- &snd_ctl_led_card_attr_group,
- NULL,
+};
static struct device snd_ctl_led_dev;
+static void snd_ctl_led_sysfs_add(struct snd_card *card) +{
- unsigned int group;
- struct snd_ctl_led_card *led_card;
- struct snd_ctl_led *led;
- char link_name[32];
- for (group = 0; group < MAX_LED; group++) {
led = &snd_ctl_leds[group];led_card = kzalloc(sizeof(*led_card), GFP_KERNEL);if (!led_card)goto cerr2;led_card->number = card->number;led_card->led = led;device_initialize(&led_card->dev);if (dev_set_name(&led_card->dev, "card%d", card->number) < 0)goto cerr;led_card->dev.parent = &led->dev;led_card->dev.groups = snd_ctl_led_card_attr_groups;if (device_add(&led_card->dev))goto cerr;led->cards[card->number] = led_card;snprintf(link_name, sizeof(link_name), "led-%s", led->name);WARN(sysfs_create_link(&card->ctl_dev.kobj, &led_card->dev.kobj, link_name),"can't create symlink to controlC%i device\n", card->number);WARN(sysfs_create_link(&led_card->dev.kobj, &card->card_dev.kobj, "card"),"can't create symlink to card%i\n", card->number);continue;+cerr:
put_device(&led_card->dev);+cerr2:
printk(KERN_ERR "snd_ctl_led: unable to add card%d", card->number);kfree(led_card);- }
+}
+static void snd_ctl_led_sysfs_remove(struct snd_card *card) +{
- unsigned int group;
- struct snd_ctl_led_card *led_card;
- struct snd_ctl_led *led;
- char link_name[32];
- for (group = 0; group < MAX_LED; group++) {
led = &snd_ctl_leds[group];led_card = led->cards[card->number];if (!led_card)continue;snprintf(link_name, sizeof(link_name), "led-%s", led->name);sysfs_remove_link(&card->ctl_dev.kobj, link_name);sysfs_remove_link(&led_card->dev.kobj, "card");device_del(&led_card->dev);kfree(led_card);led->cards[card->number] = NULL;- }
+}
/*
- Control layer registration
*/ @@ -397,14 +745,24 @@ static int __init snd_ctl_led_init(void) static void __exit snd_ctl_led_exit(void) { struct snd_ctl_led *led;
- unsigned int group;
struct snd_card *card;
unsigned int group, card_number;
snd_ctl_disconnect_layer(&snd_ctl_led_lops);
for (card_number = 0; card_number < SNDRV_CARDS; card_number++) {
if (!snd_ctl_led_card_valid[card_number])continue;card = snd_card_ref(card_number);if (card) {snd_ctl_led_sysfs_remove(card);snd_card_unref(card);}} for (group = 0; group < MAX_LED; group++) { led = &snd_ctl_leds[group]; device_del(&led->dev); } device_del(&snd_ctl_led_dev);
- snd_ctl_disconnect_layer(&snd_ctl_led_lops); snd_ctl_led_clean(NULL);
}
Dne 19. 03. 21 v 18:22 Takashi Iwai napsal(a):
On Fri, 19 Mar 2021 17:34:39 +0100, Hans de Goede wrote:
Hi,
On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
We need to manage the kcontrol entries association for the LED trigger from the user space. This patch adds a layer to the sysfs tree like:
/sys/devices/virtual/sound/ctl-led/mic
- card0
| + attach | + detach | ...
- card1
...
- attach
Operations:
attach and detach - amixer style ID is accepted and easy strings for numid and simple names reset - reset all associated kcontrol entries list - list associated kcontrol entries (numid values only)
Additional symlinks:
/sys/devices/virtual/sound/ctl-led/mic/card0/card -> /sys/class/sound/card0
/sys/class/sound/card0/controlC0/led-mic -> /sys/devices/virtual/sound/ctl-led/mic/card0
Signed-off-by: Jaroslav Kysela perex@perex.cz
Thank you so much for this patch.
I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute LED build into the (detachable) USB-keyboard's mute hot-key:
modprobe snd_ctl_led echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
This needs to be replaced by some UCM profile code doing the equivalent of course, but for a proof-of-concept test of the kernel API this introduces the above will do.
IMO, that's the question: how we'll enable this in future. If the binding of the control/mute mapping is provided via UCM, it's supposed to be changeable by each user. Then the current sysfs permission
Nope. We have two UCM boot sequences which are called from "alsactl init" only now. So, respecting the security concerns, only root should "fiddle" with this settings. So yes, udev + alsactl (or any script) executed as root.
Through a quick glance over the series, I'm fine to take those patches, but the only concern is the sysfs entries. Basically, once when we use sysfs entries, it's set in stone. So we should be very clear about our strategy how to deploy the control/mute mapping regarding using those sysfs entries.
OTOH, if the interface is thought for debugging or development purpose, it could be done in debugfs, which we can keep playing in further development, too.
We need attach / detach (reset and list operations are optional, but nice to have). If you have any other idea, let me know.
And, BTW, the mute LED mode setup doesn't have to be sysfs entries; we'd need primarily only the flags for inverted LED behavior, and those are only two, so it could be simply module options. Then it's even easier for users to set up than tweaking sysfs entries.
I don't insist to control this over sysfs. But if sysfs is in the game, it's nice to have the runtime control for this. The module parameter may be added to modify the default value.
Jaroslav
Hi,
On 3/19/21 6:22 PM, Takashi Iwai wrote:
On Fri, 19 Mar 2021 17:34:39 +0100, Hans de Goede wrote:
Hi,
On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
We need to manage the kcontrol entries association for the LED trigger from the user space. This patch adds a layer to the sysfs tree like:
/sys/devices/virtual/sound/ctl-led/mic
- card0
| + attach | + detach | ...
- card1
...
- attach
Operations:
attach and detach - amixer style ID is accepted and easy strings for numid and simple names reset - reset all associated kcontrol entries list - list associated kcontrol entries (numid values only)
Additional symlinks:
/sys/devices/virtual/sound/ctl-led/mic/card0/card -> /sys/class/sound/card0
/sys/class/sound/card0/controlC0/led-mic -> /sys/devices/virtual/sound/ctl-led/mic/card0
Signed-off-by: Jaroslav Kysela perex@perex.cz
Thank you so much for this patch.
I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute LED build into the (detachable) USB-keyboard's mute hot-key:
modprobe snd_ctl_led echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
This needs to be replaced by some UCM profile code doing the equivalent of course, but for a proof-of-concept test of the kernel API this introduces the above will do.
IMO, that's the question: how we'll enable this in future. If the binding of the control/mute mapping is provided via UCM, it's supposed to be changeable by each user. Then the current sysfs permission doesn't fit. OTOH, if it's 0666, it's accessible to all users even remotely, which is worse than the access with the normal sound device file. Or if it's supposed to be changed via udev stuff or systemd? Or is it just for debugging?
Through a quick glance over the series, I'm fine to take those patches, but the only concern is the sysfs entries. Basically, once when we use sysfs entries, it's set in stone. So we should be very clear about our strategy how to deploy the control/mute mapping regarding using those sysfs entries.
OTOH, if the interface is thought for debugging or development purpose, it could be done in debugfs, which we can keep playing in further development, too.
And, BTW, the mute LED mode setup doesn't have to be sysfs entries; we'd need primarily only the flags for inverted LED behavior, and those are only two, so it could be simply module options. Then it's even easier for users to set up than tweaking sysfs entries.
The flexibility offered by this new sysfs API is necessary for the ASoC codec drivers, because Mark does not want to have which controls are tied to the LED triggers hard-coded inside the codec drivers.
So as Jaroslav mentions in his reply, the plan is to have the UCM profiles contain commands to setup the LED triggers to this new sysfs API.
Regards,
Hans
On Fri, 19 Mar 2021 23:08:33 +0100, Hans de Goede wrote:
Hi,
On 3/19/21 6:22 PM, Takashi Iwai wrote:
On Fri, 19 Mar 2021 17:34:39 +0100, Hans de Goede wrote:
Hi,
On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
We need to manage the kcontrol entries association for the LED trigger from the user space. This patch adds a layer to the sysfs tree like:
/sys/devices/virtual/sound/ctl-led/mic
- card0
| + attach | + detach | ...
- card1
...
- attach
Operations:
attach and detach - amixer style ID is accepted and easy strings for numid and simple names reset - reset all associated kcontrol entries list - list associated kcontrol entries (numid values only)
Additional symlinks:
/sys/devices/virtual/sound/ctl-led/mic/card0/card -> /sys/class/sound/card0
/sys/class/sound/card0/controlC0/led-mic -> /sys/devices/virtual/sound/ctl-led/mic/card0
Signed-off-by: Jaroslav Kysela perex@perex.cz
Thank you so much for this patch.
I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute LED build into the (detachable) USB-keyboard's mute hot-key:
modprobe snd_ctl_led echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
This needs to be replaced by some UCM profile code doing the equivalent of course, but for a proof-of-concept test of the kernel API this introduces the above will do.
IMO, that's the question: how we'll enable this in future. If the binding of the control/mute mapping is provided via UCM, it's supposed to be changeable by each user. Then the current sysfs permission doesn't fit. OTOH, if it's 0666, it's accessible to all users even remotely, which is worse than the access with the normal sound device file. Or if it's supposed to be changed via udev stuff or systemd? Or is it just for debugging?
Through a quick glance over the series, I'm fine to take those patches, but the only concern is the sysfs entries. Basically, once when we use sysfs entries, it's set in stone. So we should be very clear about our strategy how to deploy the control/mute mapping regarding using those sysfs entries.
OTOH, if the interface is thought for debugging or development purpose, it could be done in debugfs, which we can keep playing in further development, too.
And, BTW, the mute LED mode setup doesn't have to be sysfs entries; we'd need primarily only the flags for inverted LED behavior, and those are only two, so it could be simply module options. Then it's even easier for users to set up than tweaking sysfs entries.
The flexibility offered by this new sysfs API is necessary for the ASoC codec drivers, because Mark does not want to have which controls are tied to the LED triggers hard-coded inside the codec drivers.
The hard-coded mapping itself isn't always bad things, IMO. Of course, it's a question whether to be done in the codec driver in a fixed routing. A machine driver would fit well, instead; i.e. instead of the control-access bit flag, just bind statically from the machine driver after instantiating the kctl objects like sysfs does.
So as Jaroslav mentions in his reply, the plan is to have the UCM profiles contain commands to setup the LED triggers to this new sysfs API.
IIUC, this won't be only UCM but also the combination of udev + alsactl + UCM, right?
Would other OS can follow a similar pattern? Let's check that first (although I myself think this should be feasible).
thanks,
Takashi
Hi,
On 3/20/21 8:41 AM, Takashi Iwai wrote:
On Fri, 19 Mar 2021 23:08:33 +0100, Hans de Goede wrote:
Hi,
On 3/19/21 6:22 PM, Takashi Iwai wrote:
On Fri, 19 Mar 2021 17:34:39 +0100, Hans de Goede wrote:
Hi,
On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
We need to manage the kcontrol entries association for the LED trigger from the user space. This patch adds a layer to the sysfs tree like:
/sys/devices/virtual/sound/ctl-led/mic
- card0
| + attach | + detach | ...
- card1
...
- attach
Operations:
attach and detach - amixer style ID is accepted and easy strings for numid and simple names reset - reset all associated kcontrol entries list - list associated kcontrol entries (numid values only)
Additional symlinks:
/sys/devices/virtual/sound/ctl-led/mic/card0/card -> /sys/class/sound/card0
/sys/class/sound/card0/controlC0/led-mic -> /sys/devices/virtual/sound/ctl-led/mic/card0
Signed-off-by: Jaroslav Kysela perex@perex.cz
Thank you so much for this patch.
I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute LED build into the (detachable) USB-keyboard's mute hot-key:
modprobe snd_ctl_led echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
This needs to be replaced by some UCM profile code doing the equivalent of course, but for a proof-of-concept test of the kernel API this introduces the above will do.
IMO, that's the question: how we'll enable this in future. If the binding of the control/mute mapping is provided via UCM, it's supposed to be changeable by each user. Then the current sysfs permission doesn't fit. OTOH, if it's 0666, it's accessible to all users even remotely, which is worse than the access with the normal sound device file. Or if it's supposed to be changed via udev stuff or systemd? Or is it just for debugging?
Through a quick glance over the series, I'm fine to take those patches, but the only concern is the sysfs entries. Basically, once when we use sysfs entries, it's set in stone. So we should be very clear about our strategy how to deploy the control/mute mapping regarding using those sysfs entries.
OTOH, if the interface is thought for debugging or development purpose, it could be done in debugfs, which we can keep playing in further development, too.
And, BTW, the mute LED mode setup doesn't have to be sysfs entries; we'd need primarily only the flags for inverted LED behavior, and those are only two, so it could be simply module options. Then it's even easier for users to set up than tweaking sysfs entries.
The flexibility offered by this new sysfs API is necessary for the ASoC codec drivers, because Mark does not want to have which controls are tied to the LED triggers hard-coded inside the codec drivers.
The hard-coded mapping itself isn't always bad things, IMO. Of course, it's a question whether to be done in the codec driver in a fixed routing. A machine driver would fit well, instead; i.e. instead of the control-access bit flag, just bind statically from the machine driver after instantiating the kctl objects like sysfs does.
Yes setting the new LED-access flags from the machine driver(s) would work too for the 3 devices (spanning 2 machine drivers) on which I'm trying to get the mute-LED to work, assuming Mark is going to be ok with that approach. But those are pretty simple devices.
There also is the recently posted Dell privacy stuff which is also using LED-triggers on a "full-blown" Intel core series laptop, which use codecs in much more varied ways. And I've the feeling that we will see more of this stuff coming up and in those cases the extra flexibility which going through UCM gives us would be good I think.
I believe that that Dell privacy stuff is actually the reason why Jaroslav started this whole series, right Jaroslav ?
I'm just piggy backing along with my own use-cases which I had on my wishlist / itches-list for a while now :)
So as Jaroslav mentions in his reply, the plan is to have the UCM profiles contain commands to setup the LED triggers to this new sysfs API.
IIUC, this won't be only UCM but also the combination of udev + alsactl + UCM, right?
Right.
Would other OS can follow a similar pattern? Let's check that first (although I myself think this should be feasible).
With other OS you mean e.g. Android? Android has device-specific init-scripts which can either call alsactl or directly do the echo-s.
Regards,
Hans
On Sat, 20 Mar 2021 10:17:57 +0100, Hans de Goede wrote:
Hi,
On 3/20/21 8:41 AM, Takashi Iwai wrote:
On Fri, 19 Mar 2021 23:08:33 +0100, Hans de Goede wrote:
Hi,
On 3/19/21 6:22 PM, Takashi Iwai wrote:
On Fri, 19 Mar 2021 17:34:39 +0100, Hans de Goede wrote:
Hi,
On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
We need to manage the kcontrol entries association for the LED trigger from the user space. This patch adds a layer to the sysfs tree like:
/sys/devices/virtual/sound/ctl-led/mic
- card0
| + attach | + detach | ...
- card1
...
- attach
Operations:
attach and detach - amixer style ID is accepted and easy strings for numid and simple names reset - reset all associated kcontrol entries list - list associated kcontrol entries (numid values only)
Additional symlinks:
/sys/devices/virtual/sound/ctl-led/mic/card0/card -> /sys/class/sound/card0
/sys/class/sound/card0/controlC0/led-mic -> /sys/devices/virtual/sound/ctl-led/mic/card0
Signed-off-by: Jaroslav Kysela perex@perex.cz
Thank you so much for this patch.
I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute LED build into the (detachable) USB-keyboard's mute hot-key:
modprobe snd_ctl_led echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
This needs to be replaced by some UCM profile code doing the equivalent of course, but for a proof-of-concept test of the kernel API this introduces the above will do.
IMO, that's the question: how we'll enable this in future. If the binding of the control/mute mapping is provided via UCM, it's supposed to be changeable by each user. Then the current sysfs permission doesn't fit. OTOH, if it's 0666, it's accessible to all users even remotely, which is worse than the access with the normal sound device file. Or if it's supposed to be changed via udev stuff or systemd? Or is it just for debugging?
Through a quick glance over the series, I'm fine to take those patches, but the only concern is the sysfs entries. Basically, once when we use sysfs entries, it's set in stone. So we should be very clear about our strategy how to deploy the control/mute mapping regarding using those sysfs entries.
OTOH, if the interface is thought for debugging or development purpose, it could be done in debugfs, which we can keep playing in further development, too.
And, BTW, the mute LED mode setup doesn't have to be sysfs entries; we'd need primarily only the flags for inverted LED behavior, and those are only two, so it could be simply module options. Then it's even easier for users to set up than tweaking sysfs entries.
The flexibility offered by this new sysfs API is necessary for the ASoC codec drivers, because Mark does not want to have which controls are tied to the LED triggers hard-coded inside the codec drivers.
The hard-coded mapping itself isn't always bad things, IMO. Of course, it's a question whether to be done in the codec driver in a fixed routing. A machine driver would fit well, instead; i.e. instead of the control-access bit flag, just bind statically from the machine driver after instantiating the kctl objects like sysfs does.
Yes setting the new LED-access flags from the machine driver(s) would work too for the 3 devices (spanning 2 machine drivers) on which I'm trying to get the mute-LED to work, assuming Mark is going to be ok with that approach. But those are pretty simple devices.
There also is the recently posted Dell privacy stuff which is also using LED-triggers on a "full-blown" Intel core series laptop, which use codecs in much more varied ways. And I've the feeling that we will see more of this stuff coming up and in those cases the extra flexibility which going through UCM gives us would be good I think.
I believe that that Dell privacy stuff is actually the reason why Jaroslav started this whole series, right Jaroslav ?
I'm just piggy backing along with my own use-cases which I had on my wishlist / itches-list for a while now :)
So as Jaroslav mentions in his reply, the plan is to have the UCM profiles contain commands to setup the LED triggers to this new sysfs API.
IIUC, this won't be only UCM but also the combination of udev + alsactl + UCM, right?
Right.
Would other OS can follow a similar pattern? Let's check that first (although I myself think this should be feasible).
With other OS you mean e.g. Android? Android has device-specific init-scripts which can either call alsactl or directly do the echo-s.
Also ChromeOS. I'd like to get a general consensus before moving forward.
Takashi
Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
With other OS you mean e.g. Android? Android has device-specific init-scripts which can either call alsactl or directly do the echo-s.
Also ChromeOS. I'd like to get a general consensus before moving forward.
Where are ChromeOS people? They could join to the discussion which is floating few months now. Perhaps, the gmail's spam filter does not allow them to communicate with us ;-)
Jaroslav
On Mon, 22 Mar 2021 15:16:30 +0100, Jaroslav Kysela wrote:
Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
With other OS you mean e.g. Android? Android has device-specific init-scripts which can either call alsactl or directly do the echo-s.
Also ChromeOS. I'd like to get a general consensus before moving forward.
Where are ChromeOS people? They could join to the discussion which is floating few months now. Perhaps, the gmail's spam filter does not allow them to communicate with us ;-)
Also adding Dylan and Mark to Cc.
FYI, the patch set is: https://lore.kernel.org/alsa-devel/20210317172945.842280-1-perex@perex.cz/
Takashi
On Tue, 23 Mar 2021 10:38:46 +0100, Takashi Iwai wrote:
On Mon, 22 Mar 2021 15:16:30 +0100, Jaroslav Kysela wrote:
Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
With other OS you mean e.g. Android? Android has device-specific init-scripts which can either call alsactl or directly do the echo-s.
Also ChromeOS. I'd like to get a general consensus before moving forward.
Where are ChromeOS people? They could join to the discussion which is floating few months now. Perhaps, the gmail's spam filter does not allow them to communicate with us ;-)
Also adding Dylan and Mark to Cc.
FYI, the patch set is: https://lore.kernel.org/alsa-devel/20210317172945.842280-1-perex@perex.cz/
... and now back to the topic.
So the primary question is whether we want the sysfs entries to allow user-space defining the mute-LED vs control binding externally. With this, the mute LED is supposed to be set up via udev rules that triggers some alsactl stuff, and the rest is handled in an extension in UCM profile. If this approach is acceptable on all platforms, we can go for it. That was the question to other platforms like Android and ChromeOS.
And, now looking into the details, I have a few more questions:
- The binding with SNDRV_CTL_ELEM_* bit flag is handy for some drivers but not for everything; e.g. if we want to add the binding in ASoC machine driver, an API like snd_ctl_bind_mute_led(card, elem_id, inverted); would be easier. It'd be essentially an internal call of the sysfs binding. (I haven't checked, but might this be also more straightforward conversion for HD-audio case, too?)
- The binding in the kernel could (should?) be shown in the sysfs output. Currently it seems handled differently?
- Specifying the numid may the code simpler in kernel side? alsactl has already the string parser.
- Do we have to deal with binding with multiple controls to a single mute LED? Might a single exclusive binding make things easier? Then we don't have to create sysfs entries per card, and it'll be something like echo 1:10 > /sys/devices/virtual/sound/ctl-led/mic/bind which is equivalent with the API call above. If multiple bindings are attempted, it can simply give an error. In the driver side, it catches the unexpected binding, too.
thanks,
Takashi
Dne 23. 03. 21 v 10:49 Takashi Iwai napsal(a):
On Tue, 23 Mar 2021 10:38:46 +0100, Takashi Iwai wrote:
On Mon, 22 Mar 2021 15:16:30 +0100, Jaroslav Kysela wrote:
Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
With other OS you mean e.g. Android? Android has device-specific init-scripts which can either call alsactl or directly do the echo-s.
Also ChromeOS. I'd like to get a general consensus before moving forward.
Where are ChromeOS people? They could join to the discussion which is floating few months now. Perhaps, the gmail's spam filter does not allow them to communicate with us ;-)
Also adding Dylan and Mark to Cc.
FYI, the patch set is: https://lore.kernel.org/alsa-devel/20210317172945.842280-1-perex@perex.cz/
... and now back to the topic.
So the primary question is whether we want the sysfs entries to allow user-space defining the mute-LED vs control binding externally. With this, the mute LED is supposed to be set up via udev rules that triggers some alsactl stuff, and the rest is handled in an extension in UCM profile. If this approach is acceptable on all platforms, we can go for it. That was the question to other platforms like Android and ChromeOS.
And, now looking into the details, I have a few more questions:
- The binding with SNDRV_CTL_ELEM_* bit flag is handy for some drivers but not for everything; e.g. if we want to add the binding in ASoC machine driver, an API like snd_ctl_bind_mute_led(card, elem_id, inverted); would be easier. It'd be essentially an internal call of the sysfs binding.
I would probably create more universal helper for the access field. It may be handy to update other flags like INACTIVE or so. Something like:
snd_ctl_update_access(card, elem_id, access_mask, access_bits);
If we decide to move this information out of access field, we can replace those calls with another function.
For ASoC codecs, it may be difficult to do such calls in the init phase, because the card is not bound to the component. But yes, I agree that this setting should be handled in the upper layer (machine) than the component layer.
(I haven't checked, but might this be also more straightforward conversion for HD-audio case, too?)
I don't think that it brings a simplification. The id composition is more complex than 'if (codec->led_flag) access |= LED_GROUP'.
- The binding in the kernel could (should?) be shown in the sysfs output. Currently it seems handled differently?
It isn't. The LED group is stored in the access field and my implementation tracks those bits per elements. So, the sysfs LED code updates those bits, too. The settings is preserved even if you reload the ctl-led module.
- Specifying the numid may the code simpler in kernel side? alsactl has already the string parser.
Yes, but it's not so handy for scripting / UCM. I can add find-ctl-numid lookup to UCM, of course, but what about standard shell scripting?
- Do we have to deal with binding with multiple controls to a single mute LED? Might a single exclusive binding make things easier? Then we don't have to create sysfs entries per card, and it'll be something like echo 1:10 > /sys/devices/virtual/sound/ctl-led/mic/bind which is equivalent with the API call above. If multiple bindings are attempted, it can simply give an error. In the driver side, it catches the unexpected binding, too.
AMD ACP digital + HDA analog headset microphone. If we follow the standard HDA behaviour, both inputs should trigger the mic LED. Two cards are in the game.
Jaroslav
Hi,
On 3/23/21 11:31 AM, Jaroslav Kysela wrote:
Dne 23. 03. 21 v 10:49 Takashi Iwai napsal(a):
On Tue, 23 Mar 2021 10:38:46 +0100, Takashi Iwai wrote:
On Mon, 22 Mar 2021 15:16:30 +0100, Jaroslav Kysela wrote:
Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
With other OS you mean e.g. Android? Android has device-specific init-scripts which can either call alsactl or directly do the echo-s.
Also ChromeOS. I'd like to get a general consensus before moving forward.
Where are ChromeOS people? They could join to the discussion which is floating few months now. Perhaps, the gmail's spam filter does not allow them to communicate with us ;-)
Also adding Dylan and Mark to Cc.
FYI, the patch set is: https://lore.kernel.org/alsa-devel/20210317172945.842280-1-perex@perex.cz/
... and now back to the topic.
So the primary question is whether we want the sysfs entries to allow user-space defining the mute-LED vs control binding externally. With this, the mute LED is supposed to be set up via udev rules that triggers some alsactl stuff, and the rest is handled in an extension in UCM profile. If this approach is acceptable on all platforms, we can go for it. That was the question to other platforms like Android and ChromeOS.
And, now looking into the details, I have a few more questions:
- The binding with SNDRV_CTL_ELEM_* bit flag is handy for some drivers but not for everything; e.g. if we want to add the binding in ASoC machine driver, an API like snd_ctl_bind_mute_led(card, elem_id, inverted); would be easier. It'd be essentially an internal call of the sysfs binding.
I would probably create more universal helper for the access field. It may be handy to update other flags like INACTIVE or so. Something like:
snd_ctl_update_access(card, elem_id, access_mask, access_bits);
For the ASoC machine drivers this functions would ideally take an element-name not the numeric id, because the machine-driver has no idea of the ids and the ids are not really stable (they may change when e.g. a new mixer element is added to the codec).
If we decide to move this information out of access field, we can replace those calls with another function.
For ASoC codecs, it may be difficult to do such calls in the init phase, because the card is not bound to the component. But yes, I agree that this setting should be handled in the upper layer (machine) than the component layer.
(I haven't checked, but might this be also more straightforward conversion for HD-audio case, too?)
I don't think that it brings a simplification. The id composition is more complex than 'if (codec->led_flag) access |= LED_GROUP'.
- The binding in the kernel could (should?) be shown in the sysfs output. Currently it seems handled differently?
It isn't. The LED group is stored in the access field and my implementation tracks those bits per elements. So, the sysfs LED code updates those bits, too. The settings is preserved even if you reload the ctl-led module.
- Specifying the numid may the code simpler in kernel side? alsactl has already the string parser.
Yes, but it's not so handy for scripting / UCM. I can add find-ctl-numid lookup to UCM, of course, but what about standard shell scripting?
I would prefer for the sysfs API to accept element-names too, as I mentioned above that would even be better for in kernel use, let alone for a userspace API.
Regards,
Hans
On Tue, 23 Mar 2021 11:42:26 +0100, Hans de Goede wrote:
Hi,
On 3/23/21 11:31 AM, Jaroslav Kysela wrote:
Dne 23. 03. 21 v 10:49 Takashi Iwai napsal(a):
On Tue, 23 Mar 2021 10:38:46 +0100, Takashi Iwai wrote:
On Mon, 22 Mar 2021 15:16:30 +0100, Jaroslav Kysela wrote:
Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
> With other OS you mean e.g. Android? Android has device-specific > init-scripts which can either call alsactl or directly do the > echo-s.
Also ChromeOS. I'd like to get a general consensus before moving forward.
Where are ChromeOS people? They could join to the discussion which is floating few months now. Perhaps, the gmail's spam filter does not allow them to communicate with us ;-)
Also adding Dylan and Mark to Cc.
FYI, the patch set is: https://lore.kernel.org/alsa-devel/20210317172945.842280-1-perex@perex.cz/
... and now back to the topic.
So the primary question is whether we want the sysfs entries to allow user-space defining the mute-LED vs control binding externally. With this, the mute LED is supposed to be set up via udev rules that triggers some alsactl stuff, and the rest is handled in an extension in UCM profile. If this approach is acceptable on all platforms, we can go for it. That was the question to other platforms like Android and ChromeOS.
And, now looking into the details, I have a few more questions:
- The binding with SNDRV_CTL_ELEM_* bit flag is handy for some drivers but not for everything; e.g. if we want to add the binding in ASoC machine driver, an API like snd_ctl_bind_mute_led(card, elem_id, inverted); would be easier. It'd be essentially an internal call of the sysfs binding.
I would probably create more universal helper for the access field. It may be handy to update other flags like INACTIVE or so. Something like:
snd_ctl_update_access(card, elem_id, access_mask, access_bits);
For the ASoC machine drivers this functions would ideally take an element-name not the numeric id, because the machine-driver has no idea of the ids and the ids are not really stable (they may change when e.g. a new mixer element is added to the codec).
In the kernel side, what we need is rather a simple helper function like snd_ctl_find_elem(card, iface, name, index) that returns kcontrol object. A similar code has been already implemented everywhere, so it'd make sense to have a common helper instead.
If we decide to move this information out of access field, we can replace those calls with another function.
For ASoC codecs, it may be difficult to do such calls in the init phase, because the card is not bound to the component. But yes, I agree that this setting should be handled in the upper layer (machine) than the component layer.
(I haven't checked, but might this be also more straightforward conversion for HD-audio case, too?)
I don't think that it brings a simplification. The id composition is more complex than 'if (codec->led_flag) access |= LED_GROUP'.
- The binding in the kernel could (should?) be shown in the sysfs output. Currently it seems handled differently?
It isn't. The LED group is stored in the access field and my implementation tracks those bits per elements. So, the sysfs LED code updates those bits, too. The settings is preserved even if you reload the ctl-led module.
- Specifying the numid may the code simpler in kernel side? alsactl has already the string parser.
Yes, but it's not so handy for scripting / UCM. I can add find-ctl-numid lookup to UCM, of course, but what about standard shell scripting?
I would prefer for the sysfs API to accept element-names too, as I mentioned above that would even be better for in kernel use, let alone for a userspace API.
In the kernel side API, we don't need the string parser for the (iface, name, index) tuple. So, the only question is whether the string parsing is the mandatory for the sysfs interface.
I'm not entirely objecting to it; such a parser could be used in other places generically, too. But, looking at the current "list" output, it shows also only numid, so from the symmetry POV, using the numid for binding would make sense, too.
thanks,
Takashi
On Tue, 23 Mar 2021 11:31:30 +0100, Jaroslav Kysela wrote:
Dne 23. 03. 21 v 10:49 Takashi Iwai napsal(a):
On Tue, 23 Mar 2021 10:38:46 +0100, Takashi Iwai wrote:
On Mon, 22 Mar 2021 15:16:30 +0100, Jaroslav Kysela wrote:
Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
With other OS you mean e.g. Android? Android has device-specific init-scripts which can either call alsactl or directly do the echo-s.
Also ChromeOS. I'd like to get a general consensus before moving forward.
Where are ChromeOS people? They could join to the discussion which is floating few months now. Perhaps, the gmail's spam filter does not allow them to communicate with us ;-)
Also adding Dylan and Mark to Cc.
FYI, the patch set is: https://lore.kernel.org/alsa-devel/20210317172945.842280-1-perex@perex.cz/
... and now back to the topic.
So the primary question is whether we want the sysfs entries to allow user-space defining the mute-LED vs control binding externally. With this, the mute LED is supposed to be set up via udev rules that triggers some alsactl stuff, and the rest is handled in an extension in UCM profile. If this approach is acceptable on all platforms, we can go for it. That was the question to other platforms like Android and ChromeOS.
And, now looking into the details, I have a few more questions:
- The binding with SNDRV_CTL_ELEM_* bit flag is handy for some drivers but not for everything; e.g. if we want to add the binding in ASoC machine driver, an API like snd_ctl_bind_mute_led(card, elem_id, inverted); would be easier. It'd be essentially an internal call of the sysfs binding.
I would probably create more universal helper for the access field. It may be handy to update other flags like INACTIVE or so. Something like:
snd_ctl_update_access(card, elem_id, access_mask, access_bits);
If we decide to move this information out of access field, we can replace those calls with another function.
For ASoC codecs, it may be difficult to do such calls in the init phase, because the card is not bound to the component. But yes, I agree that this setting should be handled in the upper layer (machine) than the component layer.
(I haven't checked, but might this be also more straightforward conversion for HD-audio case, too?)
I don't think that it brings a simplification. The id composition is more complex than 'if (codec->led_flag) access |= LED_GROUP'.
I guess it'll simply replace the existing call of snd_hda_add_vmaster_hook() with snd_ctl_update_something(). But it's a minor thing and can be refactored later.
- The binding in the kernel could (should?) be shown in the sysfs output. Currently it seems handled differently?
It isn't. The LED group is stored in the access field and my implementation tracks those bits per elements. So, the sysfs LED code updates those bits, too. The settings is preserved even if you reload the ctl-led module.
- Specifying the numid may the code simpler in kernel side? alsactl has already the string parser.
Yes, but it's not so handy for scripting / UCM. I can add find-ctl-numid lookup to UCM, of course, but what about standard shell scripting?
Hmm, would UCM itself touch the sysfs entry? That sounds a bit awful.
The simpler implementation in the kernel side is always nicer, but of course only if it works sufficiently. So it depends on how much we want to support this feature. The parse of control name can be done by scripting, but it's cumbersome for now, indeed, so if the shell scripting is seen as the major usage, it'd be more convenient if the kernel parses the string, yeah.
- Do we have to deal with binding with multiple controls to a single mute LED? Might a single exclusive binding make things easier? Then we don't have to create sysfs entries per card, and it'll be something like echo 1:10 > /sys/devices/virtual/sound/ctl-led/mic/bind which is equivalent with the API call above. If multiple bindings are attempted, it can simply give an error. In the driver side, it catches the unexpected binding, too.
AMD ACP digital + HDA analog headset microphone. If we follow the standard HDA behaviour, both inputs should trigger the mic LED. Two cards are in the game.
And that brings yet another question. If the Dell privacy thing comes to play here, for example, the mute LED is tied with the hardware control of the built-in mic. Then do we influence on this depending on the headset mic mute state, too?
thanks,
Takashi
Dne 23. 03. 21 v 11:50 Takashi Iwai napsal(a):
- Specifying the numid may the code simpler in kernel side? alsactl has already the string parser.
Yes, but it's not so handy for scripting / UCM. I can add find-ctl-numid lookup to UCM, of course, but what about standard shell scripting?
Hmm, would UCM itself touch the sysfs entry? That sounds a bit awful.
I already described that with UCM, the boot initialization sequences can be put in the top-level UCM configuration file to replace the standard (legacy) alsactl initialization, because ASoC does not use the standard control names (so the lagacy init does not work). Those boot sequences are supposed to run at boot / card initialization phase only. This sysfs setup should be placed only to those sections. The motivation is to have the card configuration in the one place.
The simpler implementation in the kernel side is always nicer, but of course only if it works sufficiently. So it depends on how much we want to support this feature. The parse of control name can be done by scripting, but it's cumbersome for now, indeed, so if the shell scripting is seen as the major usage, it'd be more convenient if the kernel parses the string, yeah.
- Do we have to deal with binding with multiple controls to a single mute LED? Might a single exclusive binding make things easier? Then we don't have to create sysfs entries per card, and it'll be something like echo 1:10 > /sys/devices/virtual/sound/ctl-led/mic/bind which is equivalent with the API call above. If multiple bindings are attempted, it can simply give an error. In the driver side, it catches the unexpected binding, too.
AMD ACP digital + HDA analog headset microphone. If we follow the standard HDA behaviour, both inputs should trigger the mic LED. Two cards are in the game.
And that brings yet another question. If the Dell privacy thing comes to play here, for example, the mute LED is tied with the hardware control of the built-in mic. Then do we influence on this depending on the headset mic mute state, too?
What users expect? I think that both scenarios are valid, thus we should allow them.
Jaroslav
On Tue, 23 Mar 2021 12:13:12 +0100, Jaroslav Kysela wrote:
Dne 23. 03. 21 v 11:50 Takashi Iwai napsal(a):
- Specifying the numid may the code simpler in kernel side? alsactl has already the string parser.
Yes, but it's not so handy for scripting / UCM. I can add find-ctl-numid lookup to UCM, of course, but what about standard shell scripting?
Hmm, would UCM itself touch the sysfs entry? That sounds a bit awful.
I already described that with UCM, the boot initialization sequences can be put in the top-level UCM configuration file to replace the standard (legacy) alsactl initialization, because ASoC does not use the standard control names (so the lagacy init does not work). Those boot sequences are supposed to run at boot / card initialization phase only. This sysfs setup should be placed only to those sections. The motivation is to have the card configuration in the one place.
The simpler implementation in the kernel side is always nicer, but of course only if it works sufficiently. So it depends on how much we want to support this feature. The parse of control name can be done by scripting, but it's cumbersome for now, indeed, so if the shell scripting is seen as the major usage, it'd be more convenient if the kernel parses the string, yeah.
- Do we have to deal with binding with multiple controls to a single mute LED? Might a single exclusive binding make things easier? Then we don't have to create sysfs entries per card, and it'll be something like echo 1:10 > /sys/devices/virtual/sound/ctl-led/mic/bind which is equivalent with the API call above. If multiple bindings are attempted, it can simply give an error. In the driver side, it catches the unexpected binding, too.
AMD ACP digital + HDA analog headset microphone. If we follow the standard HDA behaviour, both inputs should trigger the mic LED. Two cards are in the game.
And that brings yet another question. If the Dell privacy thing comes to play here, for example, the mute LED is tied with the hardware control of the built-in mic. Then do we influence on this depending on the headset mic mute state, too?
What users expect? I think that both scenarios are valid, thus we should allow them.
IMO, this is a hard part. It's possible that user (or the system) wants two different scenarios: - LED indicates the built-in mic mute - LED indicates the mute state of the currently used input
The current code assumes the latter case, and that might conflict with the concept of Dell privacy stuff (as the built-in mic is still allowed while using the headset).
How would be a good way to switch to a different scenario?
thanks,
Takashi
Dne 23. 03. 21 v 12:34 Takashi Iwai napsal(a):
The simpler implementation in the kernel side is always nicer, but of course only if it works sufficiently. So it depends on how much we want to support this feature. The parse of control name can be done by scripting, but it's cumbersome for now, indeed, so if the shell scripting is seen as the major usage, it'd be more convenient if the kernel parses the string, yeah.
- Do we have to deal with binding with multiple controls to a single mute LED? Might a single exclusive binding make things easier? Then we don't have to create sysfs entries per card, and it'll be something like echo 1:10 > /sys/devices/virtual/sound/ctl-led/mic/bind which is equivalent with the API call above. If multiple bindings are attempted, it can simply give an error. In the driver side, it catches the unexpected binding, too.
AMD ACP digital + HDA analog headset microphone. If we follow the standard HDA behaviour, both inputs should trigger the mic LED. Two cards are in the game.
And that brings yet another question. If the Dell privacy thing comes to play here, for example, the mute LED is tied with the hardware control of the built-in mic. Then do we influence on this depending on the headset mic mute state, too?
What users expect? I think that both scenarios are valid, thus we should allow them.
IMO, this is a hard part. It's possible that user (or the system) wants two different scenarios:
- LED indicates the built-in mic mute
- LED indicates the mute state of the currently used input
The current code assumes the latter case, and that might conflict with the concept of Dell privacy stuff (as the built-in mic is still allowed while using the headset).
How would be a good way to switch to a different scenario?
[Adding Perry /Dell/ to the discussion]
It's an user space setup. We can manage some conditional settings in UCM and the shell scripts can be written with conditional parts. Perhaps, a global configuration file(s) in /etc/alsa may specify the requested scenario.
I would just start with a default behavior (which may be hw specific) and refine this later.
Jaroslav
On Mon, Mar 22, 2021 at 7:16 AM Jaroslav Kysela perex@perex.cz wrote:
Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
With other OS you mean e.g. Android? Android has device-specific init-scripts which can either call alsactl or directly do the echo-s.
Also ChromeOS. I'd like to get a general consensus before moving forward.
Where are ChromeOS people? They could join to the discussion which is floating few months now. Perhaps, the gmail's spam filter does not allow them to communicate with us ;-)
Hi Sorry, i missed this was directly to dgreid and me. Will try to get up
to speed on this.
Curtis
Jaroslav-- Jaroslav Kysela perex@perex.cz Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
On Tue, Mar 23, 2021 at 2:40 PM Curtis Malainey cujomalainey@google.com wrote:
On Mon, Mar 22, 2021 at 7:16 AM Jaroslav Kysela perex@perex.cz wrote:
Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
With other OS you mean e.g. Android? Android has device-specific init-scripts which can either call alsactl or directly do the echo-s.
Also ChromeOS. I'd like to get a general consensus before moving forward.
Where are ChromeOS people? They could join to the discussion which is floating few months now. Perhaps, the gmail's spam filter does not allow them to communicate with us ;-)
Hi Sorry, i missed this was directly to dgreid and me. Will try to get up
to speed on this.
Sorry, this one wasn't gmail's fault, it was my manual filtering of emails about LEDs:)
Chrome OS is supportive of user space control when possible. We will work with partners to establish a standard in Chrome OS for mute LED meaning (built in, headset, usb, etc). Having user space control allows different ecosystems to make different policy decisions. For us, the UCM-specified mute on/off will be driven exclusively by the audio daemon.
Curtis
Jaroslav-- Jaroslav Kysela perex@perex.cz Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
On Tue, 23 Mar 2021 23:49:40 +0100, Dylan Reid wrote:
On Tue, Mar 23, 2021 at 2:40 PM Curtis Malainey cujomalainey@google.com wrote:
On Mon, Mar 22, 2021 at 7:16 AM Jaroslav Kysela <perex@perex.cz> wrote: > Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a): > > >> With other OS you mean e.g. Android? Android has device-specific > >> init-scripts which can either call alsactl or directly do the > >> echo-s. > > > > Also ChromeOS. I'd like to get a general consensus before moving > > forward. > > Where are ChromeOS people? They could join to the discussion which is > floating > few months now. Perhaps, the gmail's spam filter does not allow them to > communicate with us ;-) > > Hi Sorry, i missed this was directly to dgreid and me. Will try to get up to speed on this.Sorry, this one wasn't gmail's fault, it was my manual filtering of emails about LEDs:)
Chrome OS is supportive of user space control when possible. We will work with partners to establish a standard in Chrome OS for mute LED meaning (built in, headset, usb, etc). Having user space control allows different ecosystems to make different policy decisions. For us, the UCM-specified mute on/off will be driven exclusively by the audio daemon.
OK, thanks for a green signal. So there doesn't seem any big concerns about the implementation, so far.
Just to be sure, Mark, do you see any possible issues for Android and other embedded deployment in this approach (sysfs + some init stuff + UCM)?
Takashi
Dne 19. 03. 21 v 17:34 Hans de Goede napsal(a):
Hi,
On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
We need to manage the kcontrol entries association for the LED trigger from the user space. This patch adds a layer to the sysfs tree like:
/sys/devices/virtual/sound/ctl-led/mic
- card0
| + attach | + detach | ...
- card1
...
- attach
Operations:
attach and detach - amixer style ID is accepted and easy strings for numid and simple names reset - reset all associated kcontrol entries list - list associated kcontrol entries (numid values only)
Additional symlinks:
/sys/devices/virtual/sound/ctl-led/mic/card0/card -> /sys/class/sound/card0
/sys/class/sound/card0/controlC0/led-mic -> /sys/devices/virtual/sound/ctl-led/mic/card0
Signed-off-by: Jaroslav Kysela perex@perex.cz
Thank you so much for this patch.
I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute LED build into the (detachable) USB-keyboard's mute hot-key:
modprobe snd_ctl_led echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
This needs to be replaced by some UCM profile code doing the equivalent of course, but for a proof-of-concept test of the kernel API this introduces the above will do.
I added already the FixedBootSequence support to alsa-lib and alsactl and the "sysset" sequence command. But looking to this command now, it may be better to rename it to "sysw" ("double s" does not look so great).
Only complaint which I have is the need to add "-n" to the echo commands, it would be nice if set_led_id() would check if the copy which it stores in buf2 ends with "\n" and if it does if it would then strip that from the copy in buf2.
Yes, I will fix that. It's possible to use the shorter string:
echo "Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
Jaroslav
Hi,
On 3/19/21 7:11 PM, Jaroslav Kysela wrote:
Dne 19. 03. 21 v 17:34 Hans de Goede napsal(a):
Hi,
On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
We need to manage the kcontrol entries association for the LED trigger from the user space. This patch adds a layer to the sysfs tree like:
/sys/devices/virtual/sound/ctl-led/mic
- card0
| + attach | + detach | ...
- card1
...
- attach
Operations:
attach and detach - amixer style ID is accepted and easy strings for numid and simple names reset - reset all associated kcontrol entries list - list associated kcontrol entries (numid values only)
Additional symlinks:
/sys/devices/virtual/sound/ctl-led/mic/card0/card -> /sys/class/sound/card0
/sys/class/sound/card0/controlC0/led-mic -> /sys/devices/virtual/sound/ctl-led/mic/card0
Signed-off-by: Jaroslav Kysela perex@perex.cz
Thank you so much for this patch.
I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute LED build into the (detachable) USB-keyboard's mute hot-key:
modprobe snd_ctl_led echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
This needs to be replaced by some UCM profile code doing the equivalent of course, but for a proof-of-concept test of the kernel API this introduces the above will do.
I added already the FixedBootSequence support to alsa-lib and alsactl and the "sysset" sequence command.
Oh, nice.
I'll look into testing the FixedBootSequence + sysset changes to replace my rc.local hack.
One thing still seems to missing from the puzzle though, what about the "modprobe snd_ctl_led", I guess we only want to do that on systems where that module is necessary, which means also having a command for that in the FixedBootSequence ?
I guess we can use the "exec" keyword in the FixedBootSequence to do the modprobe ?
But looking to this command now, it may be better to rename it to "sysw" ("double s" does not look so great).
"sysset" is fine by me, but so is "sysw" .
Regards,
Hans
On Wed, 17 Mar 2021 18:29:39 +0100, Jaroslav Kysela wrote:
This patchset tries to resolve the diversity in the audio LED control among the ALSA drivers. A new control layer registration is introduced which allows to run additional operations on top of the elementary ALSA sound controls.
A new control access group (three bits in the access flags) was introduced to carry the LED group information for the sound controls. The low-level sound drivers can just mark those controls using this access group. This information is not exported to the user space, but user space can manage the LED sound control associations through sysfs (last patch) per Mark's request. It makes things fully configurable in the kernel and user space (UCM).
The actual state ('route') evaluation is really easy (the minimal value check for all channels / controls / cards). If there's more complicated logic for a given hardware, the card driver may eventually export a new read-only sound control for the LED group and do the logic itself.
The new LED trigger control code is completely separated and possibly optional (there's no symbol dependency). The full code separation allows eventually to move this LED trigger control to the user space in future. Actually it replaces the already present functionality in the kernel space (HDA drivers) and allows a quick adoption for the recent hardware (ASoC codecs including SoundWire).
# lsmod | grep snd_ctl_led snd_ctl_led 24576 0
The sound driver implementation is really easy:
- call snd_ctl_led_request() when control LED layer should be automatically activated / it calls module_request("snd-ctl-led") on demand /
- mark all related kcontrols with SNDRV_CTL_ELEM_ACCESS_SPK_LED or SNDRV_CTL_ELEM_ACCESS_MIC_LED
v4 changes:
- the LED access flags are private to kernel now (no user space API change)
- fixes (kctl management, sysfs cleanup)
- add the sysfs LED marking kcontrol management
v3 changes:
- reorder the controls_rwsem use to fix the remaining mutex issue card->controls_rwsem <-> snd_ctl_layer_rwsem
v2 changes:
- fix the locking - remove the controls_rwsem read lock in the element get (the consistency is already protected with the global snd_ctl_led_mutex and possible partial value writes are catched with the next value change notification callback)
- rename state to brightness and show the brightness unsigned integer value instead the text on/off string (sync with the LED core routines)
- remove snd_ctl_led_hello() function (CI warning)
- make snd_ctl_led_get_by_access() function static (CI warning)
- move snd_ctl_layer_rwsem lock before the registraction callback call in snd_ctl_register_layer() - optimization
v1:
Original RFC:
Cc: Hans de Goede hdegoede@redhat.com
Jaroslav Kysela (6): ALSA: control - introduce snd_ctl_notify_one() helper ALSA: control - add layer registration routines ALSA: control - add generic LED trigger module as the new control layer ALSA: HDA - remove the custom implementation for the audio LED trigger ALSA: control - add sysfs support to the LED trigger module ALSA: led control - add sysfs kcontrol LED marking layer
As we seem agreeing with this approach, I merged this patch set as is now for 5.13. For the further development, let's put changes over there.
An immutable branch was created on top of the clean 5.12-rc5, and tagged as mute-led-rework, too.
Mark, please pull from there if you need to patch further ASoC changes that are relevant with the feature.
Thanks!
Takashi
participants (5)
-
Curtis Malainey -
Dylan Reid -
Hans de Goede -
Jaroslav Kysela -
Takashi Iwai