[PATCH 0/5] ALSA: control - add generic LED trigger code
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 exported to the user space and eventually the user space can create sound controls which can belong to a LED group.
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 (SoundWire ASoC codecs).
# lsmod | grep snd_ctl_led snd_ctl_led 16384 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
Original RFC: https://lore.kernel.org/alsa-devel/20210207201157.869972-1-perex@perex.cz/
Cc: Hans de Goede hdegoede@redhat.com Cc: Perry Yuan Perry.Yuan@dell.com
Jaroslav Kysela (5): 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
include/sound/control.h | 27 ++- include/uapi/sound/asound.h | 6 +- sound/core/Kconfig | 6 + sound/core/Makefile | 2 + sound/core/control.c | 173 ++++++++++++-- sound/core/control_led.c | 407 ++++++++++++++++++++++++++++++++ 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 +- 15 files changed, 638 insertions(+), 263 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 | 104 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 114 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..19aa19d8ff92 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,84 @@ 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; + for (card_number = 0; card_number < SNDRV_CARDS; card_number++) { + card = snd_card_ref(card_number); + if (card) { + lops->lregister(card); + snd_card_unref(card); + } + } + up_write(&snd_ctl_layer_rwsem); +} +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 +2105,18 @@ 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(&snd_ctl_layer_rwsem); + for (lops = snd_ctl_layer; lops; lops = lops->next) + lops->lregister(card); + up_read(&snd_ctl_layer_rwsem); + return 0; }
/* @@ -2032,6 +2126,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 +2136,11 @@ static int snd_ctl_dev_disconnect(struct snd_device *device) } read_unlock_irqrestore(&card->ctl_files_rwlock, flags);
+ down_read(&snd_ctl_layer_rwsem); + for (lops = snd_ctl_layer; lops; lops = lops->next) + lops->ldisconnect(card); + up_read(&snd_ctl_layer_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 | 11 ++ include/uapi/sound/asound.h | 6 +- sound/core/Kconfig | 6 + sound/core/Makefile | 2 + sound/core/control.c | 1 + sound/core/control_led.c | 276 ++++++++++++++++++++++++++++++++++++ 6 files changed, 301 insertions(+), 1 deletion(-) create mode 100644 sound/core/control_led.c
diff --git a/include/sound/control.h b/include/sound/control.h index 175610bfa8c8..e88f2651fee0 100644 --- a/include/sound/control.h +++ b/include/sound/control.h @@ -265,6 +265,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/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 535a7229e1d9..a1aff0a8ec31 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -936,7 +936,7 @@ struct snd_timer_tread { * * ****************************************************************************/
-#define SNDRV_CTL_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 8) +#define SNDRV_CTL_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 9)
struct snd_ctl_card_info { int card; /* card number */ @@ -982,6 +982,10 @@ typedef int __bitwise snd_ctl_elem_iface_t; #define SNDRV_CTL_ELEM_ACCESS_INACTIVE (1<<8) /* control does actually nothing, but may be updated */ #define SNDRV_CTL_ELEM_ACCESS_LOCK (1<<9) /* write lock */ #define SNDRV_CTL_ELEM_ACCESS_OWNER (1<<10) /* write lock owner */ +#define SNDRV_CTL_ELEM_ACCESS_LED_SHIFT 11 +#define SNDRV_CTL_ELEM_ACCESS_LED_MASK (7<<11) /* three bits - LED group */ +#define SNDRV_CTL_ELEM_ACCESS_SPK_LED (1<<11) /* speaker (output) LED flag */ +#define SNDRV_CTL_ELEM_ACCESS_MIC_LED (2<<11) /* microphone (input) LED flag */ #define SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK (1<<28) /* kernel use a TLV callback */ #define SNDRV_CTL_ELEM_ACCESS_USER (1<<29) /* user space element */ /* bits 30 and 31 are obsoleted (for indirect access) */ diff --git a/sound/core/Kconfig b/sound/core/Kconfig index a4050f87f230..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 19aa19d8ff92..4647b3cd41e8 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); diff --git a/sound/core/control_led.c b/sound/core/control_led.c new file mode 100644 index 000000000000..49b78c12e0e9 --- /dev/null +++ b/sound/core/control_led.c @@ -0,0 +1,276 @@ +// 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; + 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; +} + +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 int snd_ctl_led_get_lock(struct snd_ctl_led *lctl) +{ + struct snd_card *card = lctl->card; + int route; + + down_read(&card->controls_rwsem); + route = snd_ctl_led_get(lctl); + up_read(&card->controls_rwsem); + return route; +} + +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_lock(lctl)); + } + if (!found && kctl && card) { + lctl = kzalloc(sizeof(*lctl), GFP_KERNEL); + if (lctl) { + lctl->card = card; + lctl->kctl = kctl; + lctl->index_offset = ioff; + list_add(&lctl->list, controls); + UPDATE_ROUTE(route, snd_ctl_led_get_lock(lctl)); + } + } + mutex_unlock(&snd_ctl_led_mutex); + if (route >= 0) + ledtrig_audio_set(led_trigger_type, route ? LED_OFF : LED_ON); +} + +static void snd_ctl_led_remove(struct snd_kcontrol *kctl, unsigned int ioff) +{ + struct snd_kcontrol_volatile *vd; + struct list_head *controls; + struct snd_ctl_led *lctl; + + vd = &kctl->vd[ioff]; + controls = snd_ctl_led_controls_by_access(vd->access); + if (!controls) + return; + mutex_lock(&snd_ctl_led_mutex); + list_for_each_entry(lctl, controls, list) + if (lctl->kctl == kctl && lctl->index_offset == ioff) { + list_del(&lctl->list); + kfree(lctl); + break; + } + mutex_unlock(&snd_ctl_led_mutex); +} + +static void snd_ctl_led_notify(struct snd_card *card, unsigned int mask, + struct snd_kcontrol *kctl, unsigned int ioff) +{ + if (mask == SNDRV_CTL_EVENT_MASK_REMOVE) { + snd_ctl_led_remove(kctl, ioff); + } else if ((mask & (SNDRV_CTL_EVENT_MASK_INFO | + SNDRV_CTL_EVENT_MASK_ADD | + SNDRV_CTL_EVENT_MASK_VALUE)) != 0) { + struct snd_kcontrol_volatile *vd = &kctl->vd[ioff]; + unsigned int access = vd->access & SNDRV_CTL_ELEM_ACCESS_LED_MASK; + if (access) + snd_ctl_led_set_state(card, access, kctl, ioff); + else if ((mask & SNDRV_CTL_EVENT_MASK_INFO) != 0) + snd_ctl_led_remove(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 rwsem for controls */ + 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(); +} + +/** + * snd_ctl_led_hello - kernel module reference helper + * + * Call this helper in the module init function when the control LED + * code should be activated for the given driver. + */ +void snd_ctl_led_hello(void) +{ +} +EXPORT_SYMBOL(snd_ctl_led_hello); + +/* + * 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 9b755062d841..961750b4d56f 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 5e40944e7342..d1979ec39d3e 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_dha_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 = 0; - 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_dha_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 5beb8aa44ecd..06d4486a4961 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 7e62aed172a9..bbcef0869a59 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 52907506e16e..bc1b268a5289 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);
Hi Jaroslav,
I love your patch! Perhaps something to improve:
[auto build test WARNING on sound/for-next] [also build test WARNING on v5.11-rc7 next-20210125] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jaroslav-Kysela/ALSA-control-add-ge... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next config: m68k-allmodconfig (attached as .config) compiler: m68k-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/a7fc56df5a000d97b1b201a3f8a59218fba1... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Jaroslav-Kysela/ALSA-control-add-generic-LED-trigger-code/20210211-192035 git checkout a7fc56df5a000d97b1b201a3f8a59218fba1ee49 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
sound/core/control_led.c:49:19: warning: no previous prototype for 'snd_ctl_led_controls_by_access' [-Wmissing-prototypes] 49 | struct list_head *snd_ctl_led_controls_by_access(unsigned int access) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sound/core/control_led.c:244:6: warning: no previous prototype for 'snd_ctl_led_hello' [-Wmissing-prototypes]
244 | void snd_ctl_led_hello(void) | ^~~~~~~~~~~~~~~~~
vim +/snd_ctl_led_hello +244 sound/core/control_led.c
300372c2ee4ebb Jaroslav Kysela 2021-02-11 237 300372c2ee4ebb Jaroslav Kysela 2021-02-11 238 /** 300372c2ee4ebb Jaroslav Kysela 2021-02-11 239 * snd_ctl_led_hello - kernel module reference helper 300372c2ee4ebb Jaroslav Kysela 2021-02-11 240 * 300372c2ee4ebb Jaroslav Kysela 2021-02-11 241 * Call this helper in the module init function when the control LED 300372c2ee4ebb Jaroslav Kysela 2021-02-11 242 * code should be activated for the given driver. 300372c2ee4ebb Jaroslav Kysela 2021-02-11 243 */ 300372c2ee4ebb Jaroslav Kysela 2021-02-11 @244 void snd_ctl_led_hello(void) 300372c2ee4ebb Jaroslav Kysela 2021-02-11 245 { 300372c2ee4ebb Jaroslav Kysela 2021-02-11 246 } 300372c2ee4ebb Jaroslav Kysela 2021-02-11 247 EXPORT_SYMBOL(snd_ctl_led_hello); 300372c2ee4ebb Jaroslav Kysela 2021-02-11 248
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Dne 11. 02. 21 v 13:19 kernel test robot napsal(a):
Hi Jaroslav,
I love your patch! Perhaps something to improve:
All warnings (new ones prefixed by >>):
sound/core/control_led.c:49:19: warning: no previous prototype for 'snd_ctl_led_controls_by_access' [-Wmissing-prototypes] 49 | struct list_head *snd_ctl_led_controls_by_access(unsigned int access) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sound/core/control_led.c:244:6: warning: no previous prototype for 'snd_ctl_led_hello' [-Wmissing-prototypes]
244 | void snd_ctl_led_hello(void) | ^~~~~~~~~~~~~~~~~
I'll address both issues in v2 of the patchset (snd_ctl_led_controls_by_access function should be static and the snd_ctl_led_hello() is obsolete and it should be removed - a piece from an internal version of my code)
Jaroslav
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: a7fc56df5a000d97b1b201a3f8a59218fba1ee49 ("[PATCH 4/5] ALSA: HDA - remove the custom implementation for the audio LED trigger") url: https://github.com/0day-ci/linux/commits/Jaroslav-Kysela/ALSA-control-add-ge... base: https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git for-next
in testcase: kernel-selftests version: kernel-selftests-x86_64-b553cffa-1_20210122 with following parameters:
group: group-02 ucode: 0xe2
test-description: The kernel contains a set of "self tests" under the tools/testing/selftests/ directory. These are intended to be small unit tests to exercise individual code paths in the kernel. test-url: https://www.kernel.org/doc/Documentation/kselftest.txt
on test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz with 32G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag Reported-by: kernel test robot oliver.sang@intel.com
[ 43.984220] WARNING: possible circular locking dependency detected [ 43.990344] 5.11.0-rc4-00124-ga7fc56df5a00 #1 Not tainted [ 43.995694] ------------------------------------------------------ [ 44.001816] kworker/3:3/257 is trying to acquire lock: [ 44.006908] ffff888816929718 (&card->controls_rwsem){++++}-{3:3}, at: snd_ctl_led_set_state (kbuild/src/consumer/sound/core/control_led.c:96 kbuild/src/consumer/sound/core/control_led.c:140) snd_ctl_led [ 44.017421] [ 44.017421] but task is already holding lock: [ 44.023196] ffffffffc006d0a8 (snd_ctl_led_mutex){+.+.}-{3:3}, at: snd_ctl_led_set_state (kbuild/src/consumer/sound/core/control_led.c:124) snd_ctl_led [ 44.033281] [ 44.033281] which lock already depends on the new lock. [ 44.033281] [ 44.041382] [ 44.041382] the existing dependency chain (in reverse order) is: [ 44.048795] [ 44.048795] -> #2 (snd_ctl_led_mutex){+.+.}-{3:3}: [ 44.055006] __lock_acquire (kbuild/src/consumer/kernel/locking/lockdep.c:4832) [ 44.059326] lock_acquire (kbuild/src/consumer/kernel/locking/lockdep.c:437 kbuild/src/consumer/kernel/locking/lockdep.c:5439 kbuild/src/consumer/kernel/locking/lockdep.c:5402) [ 44.063385] __mutex_lock (kbuild/src/consumer/arch/x86/include/asm/atomic64_64.h:22 kbuild/src/consumer/include/asm-generic/atomic-instrumented.h:838 kbuild/src/consumer/include/asm-generic/atomic-long.h:29 kbuild/src/consumer/kernel/locking/mutex.c:111 kbuild/src/consumer/kernel/locking/mutex.c:152 kbuild/src/consumer/kernel/locking/mutex.c:958 kbuild/src/consumer/kernel/locking/mutex.c:1103) [ 44.067443] snd_ctl_led_set_state (kbuild/src/consumer/sound/core/control_led.c:124) snd_ctl_led [ 44.073481] snd_ctl_notify_one (kbuild/src/consumer/sound/core/control.c:206 (discriminator 3)) snd [ 44.078578] __snd_ctl_add_replace (kbuild/src/consumer/sound/core/control.c:407 (discriminator 3)) snd [ 44.084018] snd_ctl_add_replace (kbuild/src/consumer/sound/core/control.c:426) snd [ 44.089115] snd_hda_ctl_add (kbuild/src/consumer/sound/pci/hda/hda_codec.c:1678) snd_hda_codec [ 44.094730] snd_hda_add_new_ctls (kbuild/src/consumer/sound/pci/hda/hda_codec.c:3315) snd_hda_codec [ 44.100861] snd_hda_gen_build_controls (kbuild/src/consumer/sound/pci/hda/hda_generic.c:5151) snd_hda_codec_generic [ 44.108278] alc_build_controls (kbuild/src/consumer/sound/pci/hda/patch_realtek.c:839) snd_hda_codec_realtek [ 44.114748] snd_hda_codec_build_controls (kbuild/src/consumer/sound/pci/hda/hda_codec.c:3028) snd_hda_codec [ 44.121568] hda_codec_driver_probe (kbuild/src/consumer/sound/pci/hda/hda_bind.c:120) snd_hda_codec [ 44.127871] really_probe (kbuild/src/consumer/drivers/base/dd.c:565) [ 44.132018] driver_probe_device (kbuild/src/consumer/drivers/base/dd.c:745) [ 44.136675] device_driver_attach (kbuild/src/consumer/drivers/base/dd.c:1020) [ 44.141334] __driver_attach (kbuild/src/consumer/drivers/base/dd.c:1099) [ 44.145652] bus_for_each_dev (kbuild/src/consumer/drivers/base/bus.c:305) [ 44.149969] bus_add_driver (kbuild/src/consumer/drivers/base/bus.c:623) [ 44.154286] driver_register (kbuild/src/consumer/drivers/base/driver.c:171) [ 44.158520] do_one_initcall (kbuild/src/consumer/init/main.c:1217) [ 44.162847] do_init_module (kbuild/src/consumer/kernel/module.c:3672) [ 44.167086] load_module (kbuild/src/consumer/kernel/module.c:4043) [ 44.171326] __do_sys_finit_module (kbuild/src/consumer/kernel/module.c:4133) [ 44.176170] do_syscall_64 (kbuild/src/consumer/arch/x86/entry/common.c:46) [ 44.180245] entry_SYSCALL_64_after_hwframe (kbuild/src/consumer/arch/x86/entry/entry_64.S:127) [ 44.185777] [ 44.185777] -> #1 (snd_ctl_layer_rwsem){++++}-{3:3}: [ 44.192165] __lock_acquire (kbuild/src/consumer/kernel/locking/lockdep.c:4832) [ 44.196483] lock_acquire (kbuild/src/consumer/kernel/locking/lockdep.c:437 kbuild/src/consumer/kernel/locking/lockdep.c:5439 kbuild/src/consumer/kernel/locking/lockdep.c:5402) [ 44.200543] down_read (kbuild/src/consumer/arch/x86/include/asm/atomic64_64.h:160 kbuild/src/consumer/include/asm-generic/atomic-instrumented.h:893 kbuild/src/consumer/include/asm-generic/atomic-long.h:65 kbuild/src/consumer/kernel/locking/rwsem.c:237 kbuild/src/consumer/kernel/locking/rwsem.c:1212 kbuild/src/consumer/kernel/locking/rwsem.c:1222 kbuild/src/consumer/kernel/locking/rwsem.c:1355) [ 44.204345] snd_ctl_notify_one (kbuild/src/consumer/sound/core/control.c:206) snd [ 44.209441] __snd_ctl_add_replace (kbuild/src/consumer/sound/core/control.c:407 (discriminator 3)) snd [ 44.214879] snd_ctl_add_replace (kbuild/src/consumer/sound/core/control.c:426) snd [ 44.219975] snd_hda_ctl_add (kbuild/src/consumer/sound/pci/hda/hda_codec.c:1678) snd_hda_codec [ 44.225590] snd_hda_add_new_ctls (kbuild/src/consumer/sound/pci/hda/hda_codec.c:3315) snd_hda_codec [ 44.231722] snd_hda_gen_build_controls (kbuild/src/consumer/sound/pci/hda/hda_generic.c:5151) snd_hda_codec_generic [ 44.239138] alc_build_controls (kbuild/src/consumer/sound/pci/hda/patch_realtek.c:839) snd_hda_codec_realtek [ 44.245610] snd_hda_codec_build_controls (kbuild/src/consumer/sound/pci/hda/hda_codec.c:3028) snd_hda_codec [ 44.252430] hda_codec_driver_probe (kbuild/src/consumer/sound/pci/hda/hda_bind.c:120) snd_hda_codec [ 44.258732] really_probe (kbuild/src/consumer/drivers/base/dd.c:565) [ 44.262877] driver_probe_device (kbuild/src/consumer/drivers/base/dd.c:745) [ 44.267540] device_driver_attach (kbuild/src/consumer/drivers/base/dd.c:1020) [ 44.272204] __driver_attach (kbuild/src/consumer/drivers/base/dd.c:1099) [ 44.276523] bus_for_each_dev (kbuild/src/consumer/drivers/base/bus.c:305) [ 44.280839] bus_add_driver (kbuild/src/consumer/drivers/base/bus.c:623) [ 44.285156] driver_register (kbuild/src/consumer/drivers/base/driver.c:171) [ 44.289390] do_one_initcall (kbuild/src/consumer/init/main.c:1217) [ 44.293709] do_init_module (kbuild/src/consumer/kernel/module.c:3672) [ 44.297940] load_module (kbuild/src/consumer/kernel/module.c:4043) [ 44.302175] __do_sys_finit_module (kbuild/src/consumer/kernel/module.c:4133) [ 44.307010] do_syscall_64 (kbuild/src/consumer/arch/x86/entry/common.c:46) [ 44.311071] entry_SYSCALL_64_after_hwframe (kbuild/src/consumer/arch/x86/entry/entry_64.S:127) [ 44.316594] [ 44.316594] -> #0 (&card->controls_rwsem){++++}-{3:3}: [ 44.323148] check_prev_add (kbuild/src/consumer/kernel/locking/lockdep.c:2869) [ 44.327380] validate_chain (kbuild/src/consumer/kernel/locking/lockdep.c:2994 kbuild/src/consumer/kernel/locking/lockdep.c:3608) [ 44.331782] __lock_acquire (kbuild/src/consumer/kernel/locking/lockdep.c:4832) [ 44.336099] lock_acquire (kbuild/src/consumer/kernel/locking/lockdep.c:437 kbuild/src/consumer/kernel/locking/lockdep.c:5439 kbuild/src/consumer/kernel/locking/lockdep.c:5402) [ 44.340157] down_read (kbuild/src/consumer/arch/x86/include/asm/atomic64_64.h:160 kbuild/src/consumer/include/asm-generic/atomic-instrumented.h:893 kbuild/src/consumer/include/asm-generic/atomic-long.h:65 kbuild/src/consumer/kernel/locking/rwsem.c:237 kbuild/src/consumer/kernel/locking/rwsem.c:1212 kbuild/src/consumer/kernel/locking/rwsem.c:1222 kbuild/src/consumer/kernel/locking/rwsem.c:1355) [ 44.343959] snd_ctl_led_set_state (kbuild/src/consumer/sound/core/control_led.c:96 kbuild/src/consumer/sound/core/control_led.c:140) snd_ctl_led [ 44.350083] snd_ctl_led_register (kbuild/src/consumer/sound/core/control_led.c:224 (discriminator 3)) snd_ctl_led [ 44.355947] snd_ctl_dev_register (kbuild/src/consumer/sound/core/control.c:2117 (discriminator 3)) snd [ 44.361128] __snd_device_register+0x1b/0x40 snd [ 44.366996] snd_device_register_all (kbuild/src/consumer/sound/core/device.c:198) snd [ 44.372433] snd_card_register (kbuild/src/consumer/sound/core/init.c:775) snd [ 44.377441] azx_probe_continue (kbuild/src/consumer/sound/pci/hda/hda_intel.c:2345) snd_hda_intel [ 44.383393] process_one_work (kbuild/src/consumer/arch/x86/include/asm/jump_label.h:25 kbuild/src/consumer/include/linux/jump_label.h:200 kbuild/src/consumer/include/trace/events/workqueue.h:108 kbuild/src/consumer/kernel/workqueue.c:2280) [ 44.387882] worker_thread (kbuild/src/consumer/include/linux/list.h:282 kbuild/src/consumer/kernel/workqueue.c:2422) [ 44.392025] kthread (kbuild/src/consumer/kernel/kthread.c:292) [ 44.395740] ret_from_fork (kbuild/src/consumer/arch/x86/entry/entry_64.S:302) [ 44.399808] [ 44.399808] other info that might help us debug this: [ 44.399808] [ 44.407745] Chain exists of: [ 44.407745] &card->controls_rwsem --> snd_ctl_layer_rwsem --> snd_ctl_led_mutex [ 44.407745] [ 44.419479] Possible unsafe locking scenario: [ 44.419479] [ 44.425350] CPU0 CPU1 [ 44.429838] ---- ---- [ 44.434326] lock(snd_ctl_led_mutex); [ 44.438043] lock(snd_ctl_layer_rwsem); [ 44.444424] lock(snd_ctl_led_mutex); [ 44.450633] lock(&card->controls_rwsem); [ 44.454690]
To reproduce:
git clone https://github.com/intel/lkp-tests.git cd lkp-tests bin/lkp install job.yaml # job file is attached in this email bin/lkp split-job --compatible job.yaml bin/lkp run compatible-job.yaml
Thanks, Oliver Sang
Create SYSFS/devices/virtual/sound/ctl-led tree (with SYSFS/class/sound/ctl-led symlink).
speaker/ +-- mode +-- state mic/ +-- mode +-- state
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 | 193 ++++++++++++++++++++++++++++++++------- 1 file changed, 162 insertions(+), 31 deletions(-)
diff --git a/sound/core/control_led.c b/sound/core/control_led.c index 49b78c12e0e9..638808e397fe 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; struct snd_kcontrol *kctl; @@ -25,8 +41,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 { \ @@ -46,15 +75,15 @@ static inline unsigned int group_to_access(unsigned int group) return (group + 1) << SNDRV_CTL_ELEM_ACCESS_LED_SHIFT; }
-struct list_head *snd_ctl_led_controls_by_access(unsigned int access) +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; @@ -87,7 +116,7 @@ static int snd_ctl_led_get(struct snd_ctl_led *lctl) return 0; }
-static int snd_ctl_led_get_lock(struct snd_ctl_led *lctl) +static int snd_ctl_led_get_lock(struct snd_ctl_led_ctl *lctl) { struct snd_card *card = lctl->card; int route; @@ -101,22 +130,14 @@ static int snd_ctl_led_get_lock(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); @@ -125,7 +146,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_lock(lctl)); @@ -136,27 +157,33 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access, lctl->card = card; 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_lock(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 void snd_ctl_led_remove(struct snd_kcontrol *kctl, unsigned int ioff) { struct snd_kcontrol_volatile *vd; - struct list_head *controls; - struct snd_ctl_led *lctl; + struct snd_ctl_led *led; + struct snd_ctl_led_ctl *lctl;
vd = &kctl->vd[ioff]; - controls = snd_ctl_led_controls_by_access(vd->access); - if (!controls) + led = snd_ctl_led_get_by_access(vd->access); + if (!led) return; mutex_lock(&snd_ctl_led_mutex); - list_for_each_entry(lctl, controls, list) + list_for_each_entry(lctl, &led->controls, list) if (lctl->kctl == kctl && lctl->index_offset == ioff) { list_del(&lctl->list); kfree(lctl); @@ -193,13 +220,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); @@ -246,6 +273,90 @@ void snd_ctl_led_hello(void) } EXPORT_SYMBOL(snd_ctl_led_hello);
+/* + * 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_state(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct snd_ctl_led *led = container_of(dev, struct snd_ctl_led, dev); + enum led_brightness b; + const char *str; + + b = ledtrig_audio_get(led->trigger_type); + switch (b) { + case LED_ON: str = "on"; break; + case LED_OFF: str = "off"; break; + default: str = "?"; break; + } + return sprintf(buf, "%s\n", str); +} + +static DEVICE_ATTR(mode, 0644, show_mode, store_mode); +static DEVICE_ATTR(state, 0444, show_state, NULL); + +static struct attribute *snd_ctl_led_dev_attrs[] = { + &dev_attr_mode.attr, + &dev_attr_state.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 */ @@ -258,16 +369,36 @@ 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); + 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) { + device_del(&snd_ctl_led_dev); snd_ctl_disconnect_layer(&snd_ctl_led_lops); snd_ctl_led_clean(NULL); }
Hi Jaroslav,
I love your patch! Perhaps something to improve:
[auto build test WARNING on sound/for-next] [also build test WARNING on v5.11-rc7 next-20210125] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jaroslav-Kysela/ALSA-control-add-ge... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next config: m68k-allmodconfig (attached as .config) compiler: m68k-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/050a577337d6c36f0ff05c450d004d776704... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Jaroslav-Kysela/ALSA-control-add-generic-LED-trigger-code/20210211-192035 git checkout 050a577337d6c36f0ff05c450d004d7767045f20 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
sound/core/control_led.c:78:21: warning: no previous prototype for 'snd_ctl_led_get_by_access' [-Wmissing-prototypes]
78 | struct snd_ctl_led *snd_ctl_led_get_by_access(unsigned int access) | ^~~~~~~~~~~~~~~~~~~~~~~~~ sound/core/control_led.c:271:6: warning: no previous prototype for 'snd_ctl_led_hello' [-Wmissing-prototypes] 271 | void snd_ctl_led_hello(void) | ^~~~~~~~~~~~~~~~~
vim +/snd_ctl_led_get_by_access +78 sound/core/control_led.c
77
78 struct snd_ctl_led *snd_ctl_led_get_by_access(unsigned int access)
79 { 80 unsigned int group = access_to_group(access); 81 if (group >= MAX_LED) 82 return NULL; 83 return &snd_ctl_leds[group]; 84 } 85
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Thu, 11 Feb 2021 12:13:55 +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 exported to the user space and eventually the user space can create sound controls which can belong to a LED group.
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 (SoundWire ASoC codecs).
# lsmod | grep snd_ctl_led snd_ctl_led 16384 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
Original RFC: https://lore.kernel.org/alsa-devel/20210207201157.869972-1-perex@perex.cz/
Cc: Hans de Goede hdegoede@redhat.com Cc: Perry Yuan Perry.Yuan@dell.com
Jaroslav Kysela (5): 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
Thanks for the patch.
I'm afraid that it's a bit too late for 5.12, as the merge window will be likely closed soon. For 5.13, we'll have enough time for get a consensus about the design. Whether this is the best way to go, or we should rather consider user-space solution as Sakamoto-san mentioned: that has to be decided.
Back to the design: your new implementation allows the separation and the dynamic opt-in, which is nice. Thanks for that. This looks generic and may be extended for other purposes in future, too.
One thing I still miss from the picture is how to deal with the case like AMD ACP. It has no mixer control to bundle with the LED trigger. Your idea is to make a (dummy) user element and tie the LED trigger with it?
Another slight concern is the possible regression: by moving the mute-LED mode enum stuff into the sysfs, user will get incompatibilities after the kernel update. And it's not that trivial to change the sysfs entry as default for each user. It needs some detailed documentation or some temporary workaround (e.g. keep providing the controls for now but warns if the value is changed from the default value via the controls).
thanks,
Takashi
Dne 11. 02. 21 v 18:15 Takashi Iwai napsal(a):
Jaroslav Kysela (5): 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
One thing I still miss from the picture is how to deal with the case like AMD ACP. It has no mixer control to bundle with the LED trigger. Your idea is to make a (dummy) user element and tie the LED trigger with it?
Yes, the user-space code which guarantee the silence stream should create an user space control with the appropriate LED group access bits. The alsa-lib's softvol PCM plugin can do this silencing for example.
Another slight concern is the possible regression: by moving the mute-LED mode enum stuff into the sysfs, user will get incompatibilities after the kernel update. And it's not that trivial to change the sysfs entry as default for each user. It needs some detailed documentation or some temporary workaround (e.g. keep providing the controls for now but warns if the value is changed from the default value via the controls).
I don't think that we have a user space application which is using those controls (Pulseaudio or so..) in an abstract way. I think that it's really minor issue. We should probably concentrate for the main designed purpose (notify about the mute / silent state) and handle those add-on features as an experimental stuff.
Jaroslav
On Thu, 11 Feb 2021 18:53:20 +0100, Jaroslav Kysela wrote:
Dne 11. 02. 21 v 18:15 Takashi Iwai napsal(a):
Jaroslav Kysela (5): 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
One thing I still miss from the picture is how to deal with the case like AMD ACP. It has no mixer control to bundle with the LED trigger. Your idea is to make a (dummy) user element and tie the LED trigger with it?
Yes, the user-space code which guarantee the silence stream should create an user space control with the appropriate LED group access bits. The alsa-lib's softvol PCM plugin can do this silencing for example.
What control would it create? In the case of softvol, it's a volume control that really changes the volume. For the mute LED, it's a control turn on/off the mute? If so, I wonder what makes better than creating it from the kernel driver. (Of course, we can list up like "flexibility", etc, but it has a flip side of "complexity" and "fragility"...)
Another slight concern is the possible regression: by moving the mute-LED mode enum stuff into the sysfs, user will get incompatibilities after the kernel update. And it's not that trivial to change the sysfs entry as default for each user. It needs some detailed documentation or some temporary workaround (e.g. keep providing the controls for now but warns if the value is changed from the default value via the controls).
I don't think that we have a user space application which is using those controls (Pulseaudio or so..) in an abstract way. I think that it's really minor issue. We should probably concentrate for the main designed purpose (notify about the mute / silent state) and handle those add-on features as an experimental stuff.
I'm sure that there are users of the reverse mic-mute LED ("follow capture" mode); the feature was added because of the explicit request from my colleague, and this mode works no matter whether ALSA native or PA is used. Not sure about "on" and "off" mode; maybe there can be some users who want to disable the LED.
But, yes, this is a minor issue and should be in a lower priority. It's just as a reminder.
thanks,
Takashi
Dne 12. 02. 21 v 10:23 Takashi Iwai napsal(a):
On Thu, 11 Feb 2021 18:53:20 +0100, Jaroslav Kysela wrote:
Dne 11. 02. 21 v 18:15 Takashi Iwai napsal(a):
Jaroslav Kysela (5): 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
One thing I still miss from the picture is how to deal with the case like AMD ACP. It has no mixer control to bundle with the LED trigger. Your idea is to make a (dummy) user element and tie the LED trigger with it?
Yes, the user-space code which guarantee the silence stream should create an user space control with the appropriate LED group access bits. The alsa-lib's softvol PCM plugin can do this silencing for example.
What control would it create? In the case of softvol, it's a volume control that really changes the volume. For the mute LED, it's a control turn on/off the mute? If so, I wonder what makes better than creating it from the kernel driver. (Of course, we can list up like "flexibility", etc, but it has a flip side of "complexity" and "fragility"...)
The current code handles both switch / volume for the marked control (assuming that the minimal value - usually zero - is full mute). And actually, there are snd_pcm_areas_silence() calls in the softvol plugin, so we know that the PCM stream is not passed to the application at this point.
Condition:
if (info.type == SNDRV_CTL_ELEM_TYPE_BOOLEAN || info.type == SNDRV_CTL_ELEM_TYPE_INTEGER) ... value.value.integer.value[i] != info.value.integer.min
The softvol plugin may be extended to add the mute switch control, of course.
Another slight concern is the possible regression: by moving the mute-LED mode enum stuff into the sysfs, user will get incompatibilities after the kernel update. And it's not that trivial to change the sysfs entry as default for each user. It needs some detailed documentation or some temporary workaround (e.g. keep providing the controls for now but warns if the value is changed from the default value via the controls).
I don't think that we have a user space application which is using those controls (Pulseaudio or so..) in an abstract way. I think that it's really minor issue. We should probably concentrate for the main designed purpose (notify about the mute / silent state) and handle those add-on features as an experimental stuff.
I'm sure that there are users of the reverse mic-mute LED ("follow capture" mode); the feature was added because of the explicit request from my colleague, and this mode works no matter whether ALSA native or PA is used.
I see. It looks like a corner case. The proposed sysfs based code is also user space independent. The issue with the HDA code is that it's card based, but the system wide LEDs should not be tied to one sound card. We are seeing that the hardware designers became very creative :-)
Jaroslav
On Fri, 12 Feb 2021 11:32:38 +0100, Jaroslav Kysela wrote:
Dne 12. 02. 21 v 10:23 Takashi Iwai napsal(a):
On Thu, 11 Feb 2021 18:53:20 +0100, Jaroslav Kysela wrote:
Dne 11. 02. 21 v 18:15 Takashi Iwai napsal(a):
Jaroslav Kysela (5): 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
One thing I still miss from the picture is how to deal with the case like AMD ACP. It has no mixer control to bundle with the LED trigger. Your idea is to make a (dummy) user element and tie the LED trigger with it?
Yes, the user-space code which guarantee the silence stream should create an user space control with the appropriate LED group access bits. The alsa-lib's softvol PCM plugin can do this silencing for example.
What control would it create? In the case of softvol, it's a volume control that really changes the volume. For the mute LED, it's a control turn on/off the mute? If so, I wonder what makes better than creating it from the kernel driver. (Of course, we can list up like "flexibility", etc, but it has a flip side of "complexity" and "fragility"...)
The current code handles both switch / volume for the marked control (assuming that the minimal value - usually zero - is full mute). And actually, there are snd_pcm_areas_silence() calls in the softvol plugin, so we know that the PCM stream is not passed to the application at this point.
Condition:
if (info.type == SNDRV_CTL_ELEM_TYPE_BOOLEAN || info.type == SNDRV_CTL_ELEM_TYPE_INTEGER) ... value.value.integer.value[i] != info.value.integer.minThe softvol plugin may be extended to add the mute switch control, of course.
Well, my question was what kind of mixer control will be added at all, although the chip does neither volume nor mute function. Would we add a fake volume/switch like softvol, or would we add rather a control that is directly tied with the LED state?
Another slight concern is the possible regression: by moving the mute-LED mode enum stuff into the sysfs, user will get incompatibilities after the kernel update. And it's not that trivial to change the sysfs entry as default for each user. It needs some detailed documentation or some temporary workaround (e.g. keep providing the controls for now but warns if the value is changed from the default value via the controls).
I don't think that we have a user space application which is using those controls (Pulseaudio or so..) in an abstract way. I think that it's really minor issue. We should probably concentrate for the main designed purpose (notify about the mute / silent state) and handle those add-on features as an experimental stuff.
I'm sure that there are users of the reverse mic-mute LED ("follow capture" mode); the feature was added because of the explicit request from my colleague, and this mode works no matter whether ALSA native or PA is used.
I see. It looks like a corner case. The proposed sysfs based code is also user space independent. The issue with the HDA code is that it's card based, but the system wide LEDs should not be tied to one sound card. We are seeing that the hardware designers became very creative :-)
Heh, the flexiblity of the existing LED subsystem is already allowing to be creative, although I haven't seen much exotic usage in the reality (maybe the exception is the blinking at panic; IIRC, there is Morse code conversion, too? :)
Takashi
Dne 12. 02. 21 v 13:28 Takashi Iwai napsal(a):
On Fri, 12 Feb 2021 11:32:38 +0100, Jaroslav Kysela wrote:
Dne 12. 02. 21 v 10:23 Takashi Iwai napsal(a):
On Thu, 11 Feb 2021 18:53:20 +0100, Jaroslav Kysela wrote:
Dne 11. 02. 21 v 18:15 Takashi Iwai napsal(a):
Jaroslav Kysela (5): 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
One thing I still miss from the picture is how to deal with the case like AMD ACP. It has no mixer control to bundle with the LED trigger. Your idea is to make a (dummy) user element and tie the LED trigger with it?
Yes, the user-space code which guarantee the silence stream should create an user space control with the appropriate LED group access bits. The alsa-lib's softvol PCM plugin can do this silencing for example.
What control would it create? In the case of softvol, it's a volume control that really changes the volume. For the mute LED, it's a control turn on/off the mute? If so, I wonder what makes better than creating it from the kernel driver. (Of course, we can list up like "flexibility", etc, but it has a flip side of "complexity" and "fragility"...)
The current code handles both switch / volume for the marked control (assuming that the minimal value - usually zero - is full mute). And actually, there are snd_pcm_areas_silence() calls in the softvol plugin, so we know that the PCM stream is not passed to the application at this point.
Condition:
if (info.type == SNDRV_CTL_ELEM_TYPE_BOOLEAN || info.type == SNDRV_CTL_ELEM_TYPE_INTEGER) ... value.value.integer.value[i] != info.value.integer.minThe softvol plugin may be extended to add the mute switch control, of course.
Well, my question was what kind of mixer control will be added at all, although the chip does neither volume nor mute function. Would we add a fake volume/switch like softvol, or would we add rather a control that is directly tied with the LED state?
I don't understand your question. If the user space marks the own vol/sw control with the LED group, then it's tied with the LED state. I believe that the control should be created in the code which make sure that the PCM stream is silenced (like alsa-lib's softvol plugin).
Jaroslav
On Sun, 14 Feb 2021 19:55:21 +0100, Jaroslav Kysela wrote:
Dne 12. 02. 21 v 13:28 Takashi Iwai napsal(a):
On Fri, 12 Feb 2021 11:32:38 +0100, Jaroslav Kysela wrote:
Dne 12. 02. 21 v 10:23 Takashi Iwai napsal(a):
On Thu, 11 Feb 2021 18:53:20 +0100, Jaroslav Kysela wrote:
Dne 11. 02. 21 v 18:15 Takashi Iwai napsal(a):
> Jaroslav Kysela (5): > 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
One thing I still miss from the picture is how to deal with the case like AMD ACP. It has no mixer control to bundle with the LED trigger. Your idea is to make a (dummy) user element and tie the LED trigger with it?
Yes, the user-space code which guarantee the silence stream should create an user space control with the appropriate LED group access bits. The alsa-lib's softvol PCM plugin can do this silencing for example.
What control would it create? In the case of softvol, it's a volume control that really changes the volume. For the mute LED, it's a control turn on/off the mute? If so, I wonder what makes better than creating it from the kernel driver. (Of course, we can list up like "flexibility", etc, but it has a flip side of "complexity" and "fragility"...)
The current code handles both switch / volume for the marked control (assuming that the minimal value - usually zero - is full mute). And actually, there are snd_pcm_areas_silence() calls in the softvol plugin, so we know that the PCM stream is not passed to the application at this point.
Condition:
if (info.type == SNDRV_CTL_ELEM_TYPE_BOOLEAN || info.type == SNDRV_CTL_ELEM_TYPE_INTEGER) ... value.value.integer.value[i] != info.value.integer.minThe softvol plugin may be extended to add the mute switch control, of course.
Well, my question was what kind of mixer control will be added at all, although the chip does neither volume nor mute function. Would we add a fake volume/switch like softvol, or would we add rather a control that is directly tied with the LED state?
I don't understand your question. If the user space marks the own vol/sw control with the LED group, then it's tied with the LED state. I believe that the control should be created in the code which make sure that the PCM stream is silenced (like alsa-lib's softvol plugin).
The softvol (or similar effect) is to be ignored by PA, as it's not suitable with the timer-scheduled type of PCM streaming. So the control shouldn't have any actual effect of PCM stream itself but merely for controlling the LED state. If that's the case, it shouldn't be named like "XXX Switch" or "XXX Volume", but it's a control like "Mic Mute LED Status" -- and ironically, that's a kind of thing we didn't want to take in the kernel side implementation...
That said, I see the value of a generic code for the LED control that is tied with the existing volume/switch; there is already such an implementation in HD-audio, and applying to the other drivers makes sense. OTOH, for the case for AMD ACP, I'm still not sure what's the best way.
thanks,
Takashi
Takashi
Jaroslav-- Jaroslav Kysela perex@perex.cz Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
Dne 15. 02. 21 v 8:50 Takashi Iwai napsal(a):
On Sun, 14 Feb 2021 19:55:21 +0100, Jaroslav Kysela wrote:
Dne 12. 02. 21 v 13:28 Takashi Iwai napsal(a):
On Fri, 12 Feb 2021 11:32:38 +0100, Jaroslav Kysela wrote:
Dne 12. 02. 21 v 10:23 Takashi Iwai napsal(a):
On Thu, 11 Feb 2021 18:53:20 +0100, Jaroslav Kysela wrote:
Dne 11. 02. 21 v 18:15 Takashi Iwai napsal(a):
>> Jaroslav Kysela (5): >> 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
> One thing I still miss from the picture is how to deal with the case > like AMD ACP. It has no mixer control to bundle with the LED trigger. > Your idea is to make a (dummy) user element and tie the LED trigger > with it?
Yes, the user-space code which guarantee the silence stream should create an user space control with the appropriate LED group access bits. The alsa-lib's softvol PCM plugin can do this silencing for example.
What control would it create? In the case of softvol, it's a volume control that really changes the volume. For the mute LED, it's a control turn on/off the mute? If so, I wonder what makes better than creating it from the kernel driver. (Of course, we can list up like "flexibility", etc, but it has a flip side of "complexity" and "fragility"...)
The current code handles both switch / volume for the marked control (assuming that the minimal value - usually zero - is full mute). And actually, there are snd_pcm_areas_silence() calls in the softvol plugin, so we know that the PCM stream is not passed to the application at this point.
Condition:
if (info.type == SNDRV_CTL_ELEM_TYPE_BOOLEAN || info.type == SNDRV_CTL_ELEM_TYPE_INTEGER) ... value.value.integer.value[i] != info.value.integer.minThe softvol plugin may be extended to add the mute switch control, of course.
Well, my question was what kind of mixer control will be added at all, although the chip does neither volume nor mute function. Would we add a fake volume/switch like softvol, or would we add rather a control that is directly tied with the LED state?
I don't understand your question. If the user space marks the own vol/sw control with the LED group, then it's tied with the LED state. I believe that the control should be created in the code which make sure that the PCM stream is silenced (like alsa-lib's softvol plugin).
The softvol (or similar effect) is to be ignored by PA, as it's not suitable with the timer-scheduled type of PCM streaming. So the control shouldn't have any actual effect of PCM stream itself but merely for controlling the LED state. If that's the case, it shouldn't be named like "XXX Switch" or "XXX Volume", but it's a control like "Mic Mute LED Status" -- and ironically, that's a kind of thing we didn't want to take in the kernel side implementation...
I see your point now. We can force softvol switch (not volume) for this kind of hardware (AMD ACP) even for PA, so we know, that there's a code which modifies the PCM stream in the chain.
Regarding time-scheduled I/O - for the capture, we're fine, because we can silence the samples before they're processed in the user space. We also even know, that the standard use for the microphone input is real-time, so the buffering is really short. For playback, yes, the driver queued samples cannot be altered, but it's usually no big security problem. The latency may be seconds (or a bit more) in the corner case. If we need a proper solution, the kernel PCM driver should be improved to take care (software based silence) or the buffering must be reduced. I don't think that we need to change something for ACP right now.
Also note that PA does the own stream silencing when the mixer path is muted, so it's double win here (alsa-lib + PA) and the latency should be identical.
Jaroslav
Hi,
On 2/11/21 12:13 PM, 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 exported to the user space and eventually the user space can create sound controls which can belong to a LED group.
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 (SoundWire ASoC codecs).
# lsmod | grep snd_ctl_led snd_ctl_led 16384 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
So I've been running some tests with this,combined with writing UCM profiles for hw volume control, for some Intel Bay- and CherryTrail devices using Intel's Low Power Engine (LPE) for audio, which uses the ASoC framework.
My work / experiments for this are progressing a bit slower then I would like, but that is not the fault of this patch-set, but rather an issue with hw-volume control mapping, see below for details.
Leaving the ASoC implementation details aside, this patch-set works quite nicely to get the speaker mute-LED to work.
There is one issue, I'm running my kernels with lockdep and this patchset triggers a lockdep warning:
[ 24.487200] input: sof-bytcht rt5672 Headset as /devices/platform/80860F28:00/cht-bsw-rt5672/sound/card1/input19
[ 24.532006] ====================================================== [ 24.532010] WARNING: possible circular locking dependency detected [ 24.532015] 5.11.0-rc7+ #248 Tainted: G E [ 24.532019] ------------------------------------------------------ [ 24.532022] systemd-udevd/394 is trying to acquire lock: [ 24.532027] ffff922888ead720 (&card->controls_rwsem){++++}-{3:3}, at: snd_ctl_led_hello+0x65a/0x9f0 [snd_ctl_led] [ 24.532048] but task is already holding lock: [ 24.532051] ffffffffc0b0eb88 (snd_ctl_led_mutex){+.+.}-{3:3}, at: snd_ctl_led_hello+0x453/0x9f0 [snd_ctl_led] [ 24.532066] which lock already depends on the new lock.
[ 24.532069] the existing dependency chain (in reverse order) is: [ 24.532072] -> #2 (snd_ctl_led_mutex){+.+.}-{3:3}: [ 24.532083] __mutex_lock+0xb8/0x7f0 [ 24.532094] snd_ctl_led_hello+0x8fd/0x9f0 [snd_ctl_led] [ 24.532100] snd_ctl_register_layer+0x48/0x360 [snd] [ 24.532112] 0xffffffffc078f149 [ 24.532119] do_one_initcall+0x6e/0x2e0 [ 24.532127] do_init_module+0x5c/0x260 [ 24.532135] load_module+0x2570/0x27e0 [ 24.532142] __do_sys_init_module+0x130/0x190 [ 24.532148] do_syscall_64+0x33/0x40 [ 24.532154] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 24.532161] -> #1 (snd_ctl_layer_rwsem){++++}-{3:3}: [ 24.532172] down_read+0x40/0x50 [ 24.532177] snd_ctl_notify_one+0x8d/0x150 [snd] [ 24.532188] snd_ctl_find_id+0x24e/0x350 [snd] [ 24.532197] snd_ctl_find_id+0x2f3/0x350 [snd] [ 24.532207] 0xffffffffc0786bab [ 24.532212] platform_probe+0x3f/0x90 [ 24.532219] really_probe+0xf2/0x440 [ 24.532225] driver_probe_device+0xe1/0x150 [ 24.532231] device_driver_attach+0xa8/0xb0 [ 24.532237] __driver_attach+0x8c/0x150 [ 24.532243] bus_for_each_dev+0x78/0xa0 [ 24.532249] bus_add_driver+0x12e/0x1f0 [ 24.532254] driver_register+0x8f/0xe0 [ 24.532260] do_one_initcall+0x6e/0x2e0 [ 24.532266] do_init_module+0x5c/0x260 [ 24.532272] load_module+0x2570/0x27e0 [ 24.532278] __do_sys_init_module+0x130/0x190 [ 24.532285] do_syscall_64+0x33/0x40 [ 24.532290] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 24.532296] -> #0 (&card->controls_rwsem){++++}-{3:3}: [ 24.532307] __lock_acquire+0x113d/0x1e10 [ 24.532314] lock_acquire+0xe4/0x390 [ 24.532320] down_read+0x40/0x50 [ 24.532325] snd_ctl_led_hello+0x65a/0x9f0 [snd_ctl_led] [ 24.532331] snd_ctl_led_hello+0x959/0x9f0 [snd_ctl_led] [ 24.532337] snd_ctl_register_layer+0x183/0x360 [snd] [ 24.532347] snd_device_register_all+0x4c/0x60 [snd] [ 24.532357] snd_card_register+0x74/0x1d0 [snd] [ 24.532366] snd_soc_set_dmi_name+0xb2a/0xc30 [snd_soc_core] [ 24.532393] devm_snd_soc_register_card+0x43/0x90 [snd_soc_core] [ 24.532412] 0xffffffffc0cf3579 [ 24.532419] platform_probe+0x3f/0x90 [ 24.532424] really_probe+0xf2/0x440 [ 24.532430] driver_probe_device+0xe1/0x150 [ 24.532436] device_driver_attach+0xa8/0xb0 [ 24.532442] __driver_attach+0x8c/0x150 [ 24.532447] bus_for_each_dev+0x78/0xa0 [ 24.532453] bus_add_driver+0x12e/0x1f0 [ 24.532459] driver_register+0x8f/0xe0 [ 24.532465] do_one_initcall+0x6e/0x2e0 [ 24.532471] do_init_module+0x5c/0x260 [ 24.532477] load_module+0x2570/0x27e0 [ 24.532483] __do_sys_init_module+0x130/0x190 [ 24.532489] do_syscall_64+0x33/0x40 [ 24.532494] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 24.532501] other info that might help us debug this:
[ 24.532504] Chain exists of: &card->controls_rwsem --> snd_ctl_layer_rwsem --> snd_ctl_led_mutex
[ 24.532517] Possible unsafe locking scenario:
[ 24.532520] CPU0 CPU1 [ 24.532523] ---- ---- [ 24.532526] lock(snd_ctl_led_mutex); [ 24.532532] lock(snd_ctl_layer_rwsem); [ 24.532538] lock(snd_ctl_led_mutex); [ 24.532544] lock(&card->controls_rwsem); [ 24.532550] *** DEADLOCK ***
[ 24.532553] 5 locks held by systemd-udevd/394: [ 24.532558] #0: ffff922883ec2988 (&dev->mutex){....}-{3:3}, at: device_driver_attach+0x3b/0xb0 [ 24.532574] #1: ffffffffc088e1c8 (client_mutex){+.+.}-{3:3}, at: snd_soc_set_dmi_name+0x2f8/0xc30 [snd_soc_core] [ 24.532604] #2: ffffffffc0cf71f0 (&card->mutex){+.+.}-{3:3}, at: snd_soc_set_dmi_name+0x30e/0xc30 [snd_soc_core] [ 24.532632] #3: ffffffffc07562b0 (snd_ctl_layer_rwsem){++++}-{3:3}, at: snd_ctl_register_layer+0x16b/0x360 [snd] [ 24.532652] #4: ffffffffc0b0eb88 (snd_ctl_led_mutex){+.+.}-{3:3}, at: snd_ctl_led_hello+0x453/0x9f0 [snd_ctl_led] [ 24.532668] stack backtrace: [ 24.532672] CPU: 2 PID: 394 Comm: systemd-udevd Tainted: G E 5.11.0-rc7+ #248 [ 24.532679] Hardware name: LENOVO 20C1000VMH/20C1000VMH, BIOS GUET86WW (1.86) 10/21/2019 [ 24.532683] Call Trace: [ 24.532691] dump_stack+0x8b/0xb0 [ 24.532702] check_noncircular+0xfb/0x110 [ 24.532713] __lock_acquire+0x113d/0x1e10 [ 24.532722] ? lock_acquire+0xe4/0x390 [ 24.532730] lock_acquire+0xe4/0x390 [ 24.532737] ? snd_ctl_led_hello+0x65a/0x9f0 [snd_ctl_led] [ 24.532747] down_read+0x40/0x50 [ 24.532754] ? snd_ctl_led_hello+0x65a/0x9f0 [snd_ctl_led] [ 24.532761] snd_ctl_led_hello+0x65a/0x9f0 [snd_ctl_led] [ 24.532771] snd_ctl_led_hello+0x959/0x9f0 [snd_ctl_led] [ 24.532779] snd_ctl_register_layer+0x183/0x360 [snd] [ 24.532791] snd_device_register_all+0x4c/0x60 [snd] [ 24.532803] snd_card_register+0x74/0x1d0 [snd] [ 24.532816] snd_soc_set_dmi_name+0xb2a/0xc30 [snd_soc_core] [ 24.532838] devm_snd_soc_register_card+0x43/0x90 [snd_soc_core] [ 24.532860] 0xffffffffc0cf3579 [ 24.532869] platform_probe+0x3f/0x90 [ 24.532876] really_probe+0xf2/0x440 [ 24.532885] driver_probe_device+0xe1/0x150 [ 24.532893] device_driver_attach+0xa8/0xb0 [ 24.532901] __driver_attach+0x8c/0x150 [ 24.532908] ? device_driver_attach+0xb0/0xb0 [ 24.532914] ? device_driver_attach+0xb0/0xb0 [ 24.532922] bus_for_each_dev+0x78/0xa0 [ 24.532930] bus_add_driver+0x12e/0x1f0 [ 24.532937] driver_register+0x8f/0xe0 [ 24.532944] ? 0xffffffffc0cfa000 [ 24.532950] do_one_initcall+0x6e/0x2e0 [ 24.532961] do_init_module+0x5c/0x260 [ 24.532970] load_module+0x2570/0x27e0 [ 24.532988] ? __do_sys_init_module+0x130/0x190 [ 24.532995] __do_sys_init_module+0x130/0x190 [ 24.533007] do_syscall_64+0x33/0x40 [ 24.533014] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 24.533022] RIP: 0033:0x7f5d8ed4640e [ 24.533030] Code: 48 8b 0d 65 1a 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 32 1a 0c 00 f7 d8 64 89 01 48 [ 24.533037] RSP: 002b:00007ffc37771de8 EFLAGS: 00000246 ORIG_RAX: 00000000000000af [ 24.533044] RAX: ffffffffffffffda RBX: 000056459db8f3f0 RCX: 00007f5d8ed4640e [ 24.533049] RDX: 00007f5d8eea632c RSI: 00000000000064d0 RDI: 000056459dd220b0 [ 24.533054] RBP: 000056459dd220b0 R08: 000056459dced5f0 R09: 00007ffc3776e160 [ 24.533058] R10: 00005640f99694ad R11: 0000000000000246 R12: 00007f5d8eea632c [ 24.533062] R13: 000056459dbe48c0 R14: 0000000000000007 R15: 000056459dd16a30
Regards,
Hans
p.s.
The promised details of the issues which I'm hitting in implementing this on Intel devs using the ASoC framework:
E.g. on the rt5672 the speaker amplifier has no volume control. So I need to use the DAC1 digital volume control, which has no mute bits. I have this working now, but due to there not being enough steps in the hw-vol-control, it reaches 0 when the GNOME UI is displaying that the sound is soft, but not off/muted. Yes the mute-led goes on because the control reaches it lowest value. So I need to fake a mute control in the codec driver for the LED to work reliable it seems...
Hi,
On 2/12/21 9:31 PM, Hans de Goede wrote:
On 2/11/21 12:13 PM, Jaroslav Kysela wrote:
<snip>
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
So I've been running some tests with this,combined with writing UCM profiles for hw volume control, for some Intel Bay- and CherryTrail devices using Intel's Low Power Engine (LPE) for audio, which uses the ASoC framework.
My work / experiments for this are progressing a bit slower then I would like, but that is not the fault of this patch-set, but rather an issue with hw-volume control mapping, see below for details.
Leaving the ASoC implementation details aside, this patch-set works quite nicely to get the speaker mute-LED to work.
I've spend some more time this weekend playing with this and I've also added mic MUTE LED support for the ASoC rt5672 codec driver now using this.
I will post a RFC patch series with the ASoC rt5672 codec driver LED support soon, as adding an extra use-case for this will hopefully help with reviewing this.
FWIW there were some challenges, but those were not related to the driver API this patch set adds. The driver API works well for ASoC codec drivers.
Regards,
Hans
p.s.
One open issue is the lockdep issue which I reported in my previous email.
Dne 15. 02. 21 v 13:02 Hans de Goede napsal(a):
Hi,
On 2/12/21 9:31 PM, Hans de Goede wrote:
On 2/11/21 12:13 PM, Jaroslav Kysela wrote:
<snip>
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
So I've been running some tests with this,combined with writing UCM profiles for hw volume control, for some Intel Bay- and CherryTrail devices using Intel's Low Power Engine (LPE) for audio, which uses the ASoC framework.
My work / experiments for this are progressing a bit slower then I would like, but that is not the fault of this patch-set, but rather an issue with hw-volume control mapping, see below for details.
Leaving the ASoC implementation details aside, this patch-set works quite nicely to get the speaker mute-LED to work.
I've spend some more time this weekend playing with this and I've also added mic MUTE LED support for the ASoC rt5672 codec driver now using this.
I will post a RFC patch series with the ASoC rt5672 codec driver LED support soon, as adding an extra use-case for this will hopefully help with reviewing this.
FWIW there were some challenges, but those were not related to the driver API this patch set adds. The driver API works well for ASoC codec drivers.
Regards,
Hans
p.s.
One open issue is the lockdep issue which I reported in my previous email.
Thank you for tests, Hans. I'm working on the lockdep issue - I'll send v2 of the LED patchset soon.
The actual diff (I'd like to do more tests):
diff --git a/sound/core/control.c b/sound/core/control.c index 4647b3cd41e8..c9f062fada0a 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -2047,6 +2047,7 @@ void snd_ctl_register_layer(struct snd_ctl_layer_ops *lops) 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) { @@ -2054,7 +2055,6 @@ void snd_ctl_register_layer(struct snd_ctl_layer_ops *lops) snd_card_unref(card); } } - up_write(&snd_ctl_layer_rwsem); } EXPORT_SYMBOL_GPL(snd_ctl_register_layer);
diff --git a/sound/core/control_led.c b/sound/core/control_led.c index 638808e397fe..093dce721024 100644 --- a/sound/core/control_led.c +++ b/sound/core/control_led.c @@ -75,7 +75,7 @@ static inline unsigned int group_to_access(unsigned int group) return (group + 1) << SNDRV_CTL_ELEM_ACCESS_LED_SHIFT; }
-struct snd_ctl_led *snd_ctl_led_get_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) @@ -116,15 +116,20 @@ static int snd_ctl_led_get(struct snd_ctl_led_ctl *lctl) return 0; }
-static int snd_ctl_led_get_lock(struct snd_ctl_led_ctl *lctl) +static int snd_ctl_led_get_lock(struct snd_card *ncard, struct snd_ctl_led_ctl *lctl) { struct snd_card *card = lctl->card; int route;
- down_read(&card->controls_rwsem); - route = snd_ctl_led_get(lctl); - up_read(&card->controls_rwsem); - return route; + /* the rwsem is already taken for the notification card */ + if (ncard != card) { + down_read(&card->controls_rwsem); + route = snd_ctl_led_get(lctl); + up_read(&card->controls_rwsem); + return route; + } else { + return snd_ctl_led_get(lctl); + } }
static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access, @@ -149,7 +154,7 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access, 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_lock(lctl)); + UPDATE_ROUTE(route, snd_ctl_led_get_lock(card, lctl)); } if (!found && kctl && card) { lctl = kzalloc(sizeof(*lctl), GFP_KERNEL); @@ -158,7 +163,7 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access, lctl->kctl = kctl; lctl->index_offset = ioff; list_add(&lctl->list, &led->controls); - UPDATE_ROUTE(route, snd_ctl_led_get_lock(lctl)); + UPDATE_ROUTE(route, snd_ctl_led_get_lock(card, lctl)); } } mutex_unlock(&snd_ctl_led_mutex); @@ -246,10 +251,11 @@ static void snd_ctl_led_register(struct snd_card *card) 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 rwsem for controls */ + down_read(&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); + up_read(&card->controls_rwsem); snd_ctl_led_refresh(); }
@@ -262,17 +268,6 @@ static void snd_ctl_led_disconnect(struct snd_card *card) snd_ctl_led_refresh(); }
-/** - * snd_ctl_led_hello - kernel module reference helper - * - * Call this helper in the module init function when the control LED - * code should be activated for the given driver. - */ -void snd_ctl_led_hello(void) -{ -} -EXPORT_SYMBOL(snd_ctl_led_hello); - /* * sysfs */
Jaroslav
participants (5)
-
Hans de Goede -
Jaroslav Kysela -
kernel test robot -
kernel test robot -
Takashi Iwai