Re: [PATCH] [RFC] ALSA: control - add generic LED API to sound core routines

Hi Jaroslav,

On 2/7/21 9:11 PM, Jaroslav Kysela wrote:

[DO NOT MERGE!] [This is just an early proposal for comments] [The code is not tested / debugged]

The recent laptops have usually two LEDs assigned to reflect the speaker and microphone mute state. This implementation adds a tiny layer on top of the control API which calculates the state for those LEDs using the driver callbacks.

Two new access flags are introduced to describe the controls which affects the audio path settings (an easy path for drivers).

The LED resource can be shared with multiple sound cards with this code. The user space controls may be added to the state chain, too.

This code should replace the LED code in the HDA driver and add a possibility to easy extend the other drivers (ASoC codecs etc.).

Note: The snd_ctl_notify_one() changes should be moved to a separate patch. I will do this, when this proposal is agreed.

Signed-off-by: Jaroslav Kysela perex@perex.cz Cc: Perry Yuan Perry.Yuan@dell.com Cc: Hans de Goede hdegoede@redhat.com

Nice I think that having this handled in a generic manner would be a great improvement.

I have 2 HP x2 models which are Bay Trail resp. Cherry Trail based, which means they use Intel's LPE (Low Power Engine) for audio which uses ASoC.

These come with detachable USB keyboard-docks which have a speaker mute LED. I have already added LED-class support to the HID driver for these, but I never got around to hooking up a trigger from the ASoC code.

So if I understand things correctly, then if I add this patch to a kernel and in the ASoC codec driver add SNDRV_CTL_ELEM_ACCESS_SPK_LED to one or more of the controls, and add .default_trigger = "audio-mute" to the led_class_dev for the LED, then if I toggle the control on / off in alsamixer this should then control the LED ?

If I got that right I will certainly give this a try.

Any advice for which control would be the best to use? Looking at the code, it assumes that if a control is enabled that then there will be a route, which makes sense if there are e.g. headphones and speaker and lineout controls. But what about chained controls, e.g. separate volume + mute controls or multiple volume controls chained. With ASoC this is not unheard of. I guess the answer is to pick the control which we will also want to use for hw volume-control if/when UCM profiles grow hw volume-control support ?

Regards,

Hans

p.s. This would only / at least add proper support for these LEDs at the kernel level. ATM these devices which use UCM profiles don't actually use any kind of hw volume-control. So we would also need to fix that (in userspace) to get these LEDs to really work when an user hits the mute key, or lowers the volume to NIL.

---

include/sound/control.h | 12 +++ include/uapi/sound/asound.h | 2 + sound/core/Kconfig | 6 ++ sound/core/Makefile | 1 + sound/core/control.c | 83 ++++++++++++++------- sound/core/control_led.c | 144 ++++++++++++++++++++++++++++++++++++ sound/pci/hda/Kconfig | 3 +- 7 files changed, 225 insertions(+), 26 deletions(-) create mode 100644 sound/core/control_led.c

diff --git a/include/sound/control.h b/include/sound/control.h index 77d9fa10812d..ed967b18ffc9 100644 --- a/include/sound/control.h +++ b/include/sound/control.h @@ -261,4 +261,16 @@ snd_kctl_jack_new(const char *name, struct snd_card *card); void snd_kctl_jack_report(struct snd_card *card, struct snd_kcontrol *kctl, bool status);

+#if IS_ENABLED(CONFIG_SND_LED) +void snd_ctl_led_notify(struct snd_card *card, unsigned int mask,

• 	struct snd_kcontrol *kctl, unsigned int ioff);
  

+void snd_ctl_led_register(struct snd_card *card); +void snd_ctl_led_disconnect(struct snd_card *card); +#else +static inline void snd_ctl_led_notify(struct snd_card *card, unsigned int mask,

• 		      struct snd_kcontrol *kctl, unsigned int ioff) {}
  

+static inline void snd_ctl_led_register(struct snd_card *card) {} +static inline void snd_ctl_led_disconnect(struct snd_card *card) {} +#endif

#endif /* __SOUND_CONTROL_H */ diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 535a7229e1d9..3faacdf5eaea 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -982,6 +982,8 @@ typedef int __bitwise snd_ctl_elem_iface_t; #define SNDRV_CTL_ELEM_ACCESS_INACTIVE (1<<8) /* control does actually nothing, but may be updated */ #define SNDRV_CTL_ELEM_ACCESS_LOCK (1<<9) /* write lock */ #define SNDRV_CTL_ELEM_ACCESS_OWNER (1<<10) /* write lock owner */ +#define SNDRV_CTL_ELEM_ACCESS_SPK_LED (1<<11) /* speaker (output) LED flag */ +#define SNDRV_CTL_ELEM_ACCESS_MIC_LED (1<<12) /* microphone (input) LED flag */ #define SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK (1<<28) /* kernel use a TLV callback */ #define SNDRV_CTL_ELEM_ACCESS_USER (1<<29) /* user space element */ /* bits 30 and 31 are obsoleted (for indirect access) */ diff --git a/sound/core/Kconfig b/sound/core/Kconfig index a4050f87f230..1bde494fa1fe 100644 --- a/sound/core/Kconfig +++ b/sound/core/Kconfig @@ -203,4 +203,10 @@ config SND_DMA_SGBUF def_bool y depends on X86

+config SND_LED

• bool
• select NEW_LEDS if SND_LED
• select LEDS_TRIGGERS if SND_LED
• select LEDS_TRIGGER_AUDIO if SND_LED

source "sound/core/seq/Kconfig" diff --git a/sound/core/Makefile b/sound/core/Makefile index ee4a4a6b99ba..81b33877a03d 100644 --- a/sound/core/Makefile +++ b/sound/core/Makefile @@ -13,6 +13,7 @@ snd-$(CONFIG_ISA_DMA_API) += isadma.o snd-$(CONFIG_SND_OSSEMUL) += sound_oss.o snd-$(CONFIG_SND_VMASTER) += vmaster.o snd-$(CONFIG_SND_JACK) += ctljack.o jack.o +snd-$(CONFIG_SND_LED) += control_led.o

snd-pcm-y := pcm.o pcm_native.o pcm_lib.o pcm_misc.o \ pcm_memory.o memalloc.o diff --git a/sound/core/control.c b/sound/core/control.c index 5165741a8400..3171e7f2798e 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -181,6 +181,28 @@ void snd_ctl_notify(struct snd_card *card, unsigned int mask, } EXPORT_SYMBOL(snd_ctl_notify);

+/**

• • snd_ctl_notify_one - Send notification to user-space for a control change
• • @card: the card to send notification
• • @mask: the event mask, SNDRV_CTL_EVENT_*
• • @kctl: the pointer with the control instance
• • @ioff: the additional offset to the control index
• • This function calls snd_ctl_notify() and does additional jobs
• • like LED state changes.
• */

+void snd_ctl_notify_one(struct snd_card *card, unsigned int mask,

• 	struct snd_kcontrol *kctl, unsigned int ioff)
  

+{

• struct snd_ctl_elem_id id = kctl->id;
• id.index += ioff;
• id.numid += ioff;
• snd_ctl_led_notify(card, mask, kctl, ioff);
• snd_ctl_notify(card, mask, &id);

+} +EXPORT_SYMBOL(snd_ctl_notify_one);

/**

• snd_ctl_new - create a new control instance with some elements
• @kctl: the pointer to store new control instance

@@ -250,6 +272,8 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol, SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE | SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND | SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK |

•    SNDRV_CTL_ELEM_ACCESS_SPK_LED |
  

•    SNDRV_CTL_ELEM_ACCESS_MIC_LED |
  

  SNDRV_CTL_ELEM_ACCESS_SKIP_CHECK);

  err = snd_ctl_new(&kctl, count, access, NULL);

@@ -342,7 +366,6 @@ static int __snd_ctl_add_replace(struct snd_card *card, { struct snd_ctl_elem_id id; unsigned int idx;

• unsigned int count; struct snd_kcontrol *old; int err;

@@ -376,10 +399,8 @@ static int __snd_ctl_add_replace(struct snd_card *card, kcontrol->id.numid = card->last_numid + 1; card->last_numid += kcontrol->count;

• id = kcontrol->id;
• count = kcontrol->count;
• for (idx = 0; idx < count; idx++, id.index++, id.numid++)

• snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_ADD, &id);
  

• for (idx = 0; idx < kcontrol->count; idx++)

• snd_ctl_notify_one(card, SNDRV_CTL_EVENT_MASK_ADD, kcontrol, idx);
  
  

  return 0;

} @@ -462,16 +483,15 @@ EXPORT_SYMBOL(snd_ctl_replace); */ int snd_ctl_remove(struct snd_card *card, struct snd_kcontrol *kcontrol) {

• struct snd_ctl_elem_id id; unsigned int idx;

  if (snd_BUG_ON(!card || !kcontrol)) return -EINVAL; list_del(&kcontrol->list); card->controls_count -= kcontrol->count;

• id = kcontrol->id;

• for (idx = 0; idx < kcontrol->count; idx++, id.index++, id.numid++)

• snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_REMOVE, &id);
  

• for (idx = 0; idx < kcontrol->count; idx++)

• snd_ctl_notify_one(card, SNDRV_CTL_EVENT_MASK_REMOVE, kcontrol, idx);
  

  snd_ctl_free_one(kcontrol); return 0;

} @@ -584,11 +604,13 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id, vd->access |= SNDRV_CTL_ELEM_ACCESS_INACTIVE; } snd_ctl_build_ioff(id, kctl, index_offset);

• ret = 1;

• downgrade_write(&card->controls_rwsem);
• snd_ctl_notify_one(card, SNDRV_CTL_EVENT_MASK_INFO, kctl, index_offset);
• up_read(&card->controls_rwsem);
• return 1;
• unlock: up_write(&card->controls_rwsem);

• if (ret > 0)

• snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);
  

  return ret;

} EXPORT_SYMBOL_GPL(snd_ctl_activate_id); @@ -1110,25 +1132,34 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file, unsigned int index_offset; int result;

• down_write(&card->controls_rwsem); kctl = snd_ctl_find_id(card, &control->id);

• if (kctl == NULL)

• if (kctl == NULL) {

• up_write(&card->controls_rwsem);
  

  return -ENOENT;

• }

  index_offset = snd_ctl_get_ioff(kctl, &control->id); vd = &kctl->vd[index_offset]; if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_WRITE) || kctl->put == NULL || (file && vd->owner && vd->owner != file)) {

• up_write(&card->controls_rwsem);
  

  return -EPERM; }

  snd_ctl_build_ioff(&control->id, kctl, index_offset); result = kctl->put(kctl, control);

• if (result < 0)

• if (result < 0) {

• up_write(&card->controls_rwsem);
  

  return result;

• }

  if (result > 0) {

• struct snd_ctl_elem_id id = control->id;
  

• snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &id);
  

• downgrade_write(&card->controls_rwsem);
  

• snd_ctl_notify_one(card, SNDRV_CTL_EVENT_MASK_VALUE, kctl, index_offset);
  

• up_read(&card->controls_rwsem);
  

• } else {

• up_write(&card->controls_rwsem);
  

  }

  return 0;

@@ -1150,9 +1181,7 @@ static int snd_ctl_elem_write_user(struct snd_ctl_file *file, if (result < 0) goto error;

• down_write(&card->controls_rwsem); result = snd_ctl_elem_write(card, file, control);
• up_write(&card->controls_rwsem); if (result < 0) goto error;

@@ -1301,7 +1330,6 @@ static int replace_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf, { struct user_element *ue = kctl->private_data; unsigned int *container;

• struct snd_ctl_elem_id id; unsigned int mask = 0; int i; int change;

@@ -1333,10 +1361,8 @@ static int replace_user_tlv(struct snd_kcontrol *kctl, unsigned int __user *buf, ue->tlv_data_size = size;

mask |= SNDRV_CTL_EVENT_MASK_TLV;

• for (i = 0; i < kctl->count; ++i) {

• snd_ctl_build_ioff(&id, kctl, i);
  

• snd_ctl_notify(ue->card, mask, &id);
  

• }

• for (i = 0; i < kctl->count; ++i)

• snd_ctl_notify_one(ue->card, mask, kctl, i);
  
  

  return change;

} @@ -1998,9 +2024,14 @@ static const struct file_operations snd_ctl_f_ops = static int snd_ctl_dev_register(struct snd_device *device) { struct snd_card *card = device->device_data;

• int err;

• return snd_register_device(SNDRV_DEVICE_TYPE_CONTROL, card, -1,

• 		   &snd_ctl_f_ops, card, &card->ctl_dev);
  

• err = snd_register_device(SNDRV_DEVICE_TYPE_CONTROL, card, -1,

• 		  &snd_ctl_f_ops, card, &card->ctl_dev);
  

• if (err < 0)

• return err;
  

• snd_ctl_led_register(card);
• return 0;

}

/* @@ -2019,6 +2050,8 @@ static int snd_ctl_dev_disconnect(struct snd_device *device) } read_unlock_irqrestore(&card->ctl_files_rwlock, flags);

• snd_ctl_led_disconnect(card);
• return snd_unregister_device(&card->ctl_dev);

}

diff --git a/sound/core/control_led.c b/sound/core/control_led.c new file mode 100644 index 000000000000..e70855ea54d1 --- /dev/null +++ b/sound/core/control_led.c @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/*

• • LED state routines for driver control interface
• • Copyright (c) 2021 by Jaroslav Kysela perex@perex.cz
• */

+#include <linux/leds.h> +#include <sound/core.h> +#include <sound/control.h>

+static DEFINE_MUTEX(snd_control_led_mutex); +static unsigned int snd_control_led_last_card; +static struct snd_card *snd_control_led_cards[SNDRV_CARDS];

+#define UPDATE_ROUTE(route, cb) \

• do { \

• int route2 = (cb); \
  

• if (route2 >= 0) \
  

• 	route = route < 0 ? route2 : (route | route2); \
  

• } while (0)

+static int snd_ctl_led_get(struct snd_card *card,

• 	   struct snd_kcontrol *kctl, unsigned int ioff)
  

+{

• struct snd_ctl_elem_info info;
• struct snd_ctl_elem_value value;
• unsigned int i;
• int result;
• memset(&info, 0, sizeof(info));
• info.id = kctl->id;
• info.id.index += ioff;
• info.id.numid += ioff;
• result = kctl->info(kctl, &info);
• if (result < 0)

• return -1;
  

• memset(&value, 0, sizeof(value));
• value.id = info.id;
• result = kctl->get(kctl, &value);
• if (result < 0)

• return -1;
  

• if (info.type == SNDRV_CTL_ELEM_TYPE_BOOLEAN ||

•    info.type == SNDRV_CTL_ELEM_TYPE_INTEGER) {
  

• for (i = 0; i < info.count; i++)
  

• 	if (value.value.integer.value[i] != info.value.integer.min)
  

• 		return 1;
  

• } else if (info.type == SNDRV_CTL_ELEM_TYPE_INTEGER64) {

• for (i = 0; i < info.count; i++)
  

• 	if (value.value.integer64.value[i] != info.value.integer64.min)
  

• 		return 1;
  

• }
• return 0;

+}

+static int snd_ctl_led_set_state_for_card(int card_number, unsigned int group) +{

• struct snd_card *card = snd_control_led_cards[card_number];
• struct snd_kcontrol *kctl;
• struct snd_kcontrol_volatile *vd;
• unsigned int ioff;
• int route = -1;
• down_read(&card->controls_rwsem);
• list_for_each_entry(kctl, &card->controls, list) {

• for (ioff = 0; ioff < kctl->count; ioff++) {
  

• 	vd = &kctl->vd[ioff];
  

• 	if (vd->access & group)
  

• 		UPDATE_ROUTE(route, snd_ctl_led_get(card, kctl, ioff));
  

• }
  

• }
• up_read(&card->controls_rwsem);
• return route;

+}

+static void snd_ctl_led_set_state(struct snd_card *card, unsigned int group) +{

• int card_number;
• enum led_audio led_trigger_type;
• int route;
• if (group == SNDRV_CTL_ELEM_ACCESS_SPK_LED) {

• led_trigger_type = LED_AUDIO_MUTE;
  

• } else if (group == SNDRV_CTL_ELEM_ACCESS_MIC_LED) {

• led_trigger_type = LED_AUDIO_MICMUTE;
  

• } else {

• return;
  

• }
• route = -1;
• mutex_lock(&snd_control_led_mutex);
• /* the card may not be registered (active) at this point */
• if (snd_control_led_cards[card->number] == NULL) {

• mutex_unlock(&snd_control_led_mutex);
  

• return;
  

• }
• for (card_number = 0; card_number <= snd_control_led_last_card; card_number++)

• UPDATE_ROUTE(route, snd_ctl_led_set_state_for_card(card_number, group));
  

• mutex_unlock(&snd_control_led_mutex);
• if (route >= 0)

• ledtrig_audio_set(led_trigger_type, route ? LED_OFF : LED_ON);
  

+}

+void snd_ctl_led_notify(struct snd_card *card, unsigned int mask,

• 	struct snd_kcontrol *kctl, unsigned int ioff)
  

+{

• if (mask == SNDRV_CTL_EVENT_MASK_REMOVE ||

•    (mask & (SNDRV_CTL_EVENT_MASK_INFO |
  

•      SNDRV_CTL_EVENT_MASK_ADD |
  

•      SNDRV_CTL_EVENT_MASK_VALUE)) != 0) {
  

• struct snd_kcontrol_volatile *vd = &kctl->vd[ioff];
  

• if (vd->access & SNDRV_CTL_ELEM_ACCESS_SPK_LED)
  

• 	snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_SPK_LED);
  

• if (vd->access & SNDRV_CTL_ELEM_ACCESS_MIC_LED)
  

• 	snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_MIC_LED);
  

• }

+}

+void snd_ctl_led_register(struct snd_card *card) +{

• if (snd_BUG_ON(card->number < 0 ||

•        card->number >= ARRAY_SIZE(snd_control_led_cards)))
  

• return;
  

• mutex_lock(&snd_control_led_mutex);
• snd_control_led_cards[card->number] = card;
• if (card->number > snd_control_led_last_card)

• snd_control_led_last_card = card->number;
  

• mutex_unlock(&snd_control_led_mutex);
• snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_SPK_LED);
• snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_MIC_LED);

+}

+void snd_ctl_led_disconnect(struct snd_card *card) +{

• mutex_lock(&snd_control_led_mutex);
• if (snd_control_led_last_card == card->number) {

• while (snd_control_led_last_card > 0)
  

• 	if (snd_control_led_cards[--snd_control_led_last_card])
  

• 		break;
  

• }
• snd_control_led_cards[card->number] = NULL;
• mutex_unlock(&snd_control_led_mutex);
• snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_SPK_LED);
• snd_ctl_led_set_state(card, SNDRV_CTL_ELEM_ACCESS_MIC_LED);

+} diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index 90759391cbac..ccf788ede8ba 100644 --- a/sound/pci/hda/Kconfig +++ b/sound/pci/hda/Kconfig @@ -9,7 +9,8 @@ config SND_HDA select SND_HDA_CORE

config SND_HDA_GENERIC_LEDS

•   bool
  

• bool
• select SND_LED # just for test, ignore, please!

config SND_HDA_INTEL tristate "HD Audio PCI"