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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxx>
> Cc: Perry Yuan <Perry.Yuan@xxxxxxxx>
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>

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@xxxxxxxx>
> + */
> +
> +#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"
> 




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux