[v4,6/6] ALSA: led control - add sysfs kcontrol LED marking layer

Commit Message

Jaroslav Kysela March 17, 2021, 5:29 p.m. UTC

We need to manage the kcontrol entries association for the LED trigger
from the user space. This patch adds a layer to the sysfs tree like:

/sys/devices/virtual/sound/ctl-led/mic
   + card0
   |  + attach
   |  + detach
   |  ...
   + card1
      + attach
      ...

Operations:

  attach and detach
    - amixer style ID is accepted and easy strings for numid and
      simple names
  reset
    - reset all associated kcontrol entries
  list
    - list associated kcontrol entries (numid values only)

Additional symlinks:

/sys/devices/virtual/sound/ctl-led/mic/card0/card ->
  /sys/class/sound/card0

/sys/class/sound/card0/controlC0/led-mic ->
  /sys/devices/virtual/sound/ctl-led/mic/card0

Signed-off-by: Jaroslav Kysela <[e-mail]>
---
 sound/core/control_led.c | 366 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 362 insertions(+), 4 deletions(-)

Comments

Hans de Goede March 19, 2021, 4:34 p.m. UTC | #1

Hi,

On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
> We need to manage the kcontrol entries association for the LED trigger
> from the user space. This patch adds a layer to the sysfs tree like:
> 
> /sys/devices/virtual/sound/ctl-led/mic
>    + card0
>    |  + attach
>    |  + detach
>    |  ...
>    + card1
>       + attach
>       ...
> 
> Operations:
> 
>   attach and detach
>     - amixer style ID is accepted and easy strings for numid and
>       simple names
>   reset
>     - reset all associated kcontrol entries
>   list
>     - list associated kcontrol entries (numid values only)
> 
> Additional symlinks:
> 
> /sys/devices/virtual/sound/ctl-led/mic/card0/card ->
>   /sys/class/sound/card0
> 
> /sys/class/sound/card0/controlC0/led-mic ->
>   /sys/devices/virtual/sound/ctl-led/mic/card0
> 
> Signed-off-by: Jaroslav Kysela <[e-mail]>

Thank you so much for this patch.

I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.

And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute
LED build into the (detachable) USB-keyboard's mute hot-key:

modprobe snd_ctl_led
echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach

This needs to be replaced by some UCM profile code doing the equivalent of course,
but for a proof-of-concept test of the kernel API this introduces the above will do.

Only complaint which I have is the need to add "-n" to the echo commands,
it would be nice if set_led_id() would check if the copy which it stores in buf2
ends with "\n" and if it does if it would then strip that from the copy in buf2.

Regards,

Hans


p.s.

Note this does need my recently listed alsa=lib patches so that these "Channel Switch" controls
get grouped with the "Speaker Playback Volume" / "HP Playback Volume" controls, so that the
volume-hw control code will actually toggle them:

https://lore.kernel.org/alsa-devel/[e-mail]/T/#u

Talking about that series, what is the status of that ? From my POV it
is ready for merging...



> ---
>  sound/core/control_led.c | 366 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 362 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> index dfa51d8461e1..d4fb8b873f34 100644
> --- a/sound/core/control_led.c
> +++ b/sound/core/control_led.c
> @@ -24,6 +24,12 @@ enum snd_ctl_led_mode {
>  	 MODE_ON,
>  };
>  
> +struct snd_ctl_led_card {
> +	struct device dev;
> +	int number;
> +	struct snd_ctl_led *led;
> +};
> +
>  struct snd_ctl_led {
>  	struct device dev;
>  	struct list_head controls;
> @@ -31,6 +37,7 @@ struct snd_ctl_led {
>  	unsigned int group;
>  	enum led_audio trigger_type;
>  	enum snd_ctl_led_mode mode;
> +	struct snd_ctl_led_card *cards[SNDRV_CARDS];
>  };
>  
>  struct snd_ctl_led_ctl {
> @@ -58,6 +65,9 @@ static struct snd_ctl_led snd_ctl_leds[MAX_LED] = {
>  	},
>  };
>  
> +static void snd_ctl_led_sysfs_add(struct snd_card *card);
> +static void snd_ctl_led_sysfs_remove(struct snd_card *card);
> +
>  #define UPDATE_ROUTE(route, cb) \
>  	do { \
>  		int route2 = (cb); \
> @@ -222,6 +232,46 @@ static void snd_ctl_led_notify(struct snd_card *card, unsigned int mask,
>  	}
>  }
>  
> +static int snd_ctl_led_set_id(int card_number, struct snd_ctl_elem_id *id,
> +			      unsigned int group, bool set)
> +{
> +	struct snd_card *card;
> +	struct snd_kcontrol *kctl;
> +	struct snd_kcontrol_volatile *vd;
> +	unsigned int ioff, access, new_access;
> +	int err = 0;
> +
> +	card = snd_card_ref(card_number);
> +	if (card) {
> +		down_write(&card->controls_rwsem);
> +		kctl = snd_ctl_find_id(card, id);
> +		if (kctl) {
> +			ioff = snd_ctl_get_ioff(kctl, id);
> +			vd = &kctl->vd[ioff];
> +			access = vd->access & SNDRV_CTL_ELEM_ACCESS_LED_MASK;
> +			if (access != 0 && access != group_to_access(group)) {
> +				err = -EXDEV;
> +				goto unlock;
> +			}
> +			new_access = vd->access & ~SNDRV_CTL_ELEM_ACCESS_LED_MASK;
> +			if (set)
> +				new_access |= group_to_access(group);
> +			if (new_access != vd->access) {
> +				vd->access = new_access;
> +				snd_ctl_led_notify(card, SNDRV_CTL_EVENT_MASK_INFO, kctl, ioff);
> +			}
> +		} else {
> +			err = -ENOENT;
> +		}
> +unlock:
> +		up_write(&card->controls_rwsem);
> +		snd_card_unref(card);
> +	} else {
> +		err = -ENXIO;
> +	}
> +	return err;
> +}
> +
>  static void snd_ctl_led_refresh(void)
>  {
>  	unsigned int group;
> @@ -230,6 +280,12 @@ static void snd_ctl_led_refresh(void)
>  		snd_ctl_led_set_state(NULL, group_to_access(group), NULL, 0);
>  }
>  
> +static void snd_ctl_led_ctl_destroy(struct snd_ctl_led_ctl *lctl)
> +{
> +	list_del(&lctl->list);
> +	kfree(lctl);
> +}
> +
>  static void snd_ctl_led_clean(struct snd_card *card)
>  {
>  	unsigned int group;
> @@ -241,13 +297,47 @@ static void snd_ctl_led_clean(struct snd_card *card)
>  repeat:
>  		list_for_each_entry(lctl, &led->controls, list)
>  			if (!card || lctl->card == card) {
> -				list_del(&lctl->list);
> -				kfree(lctl);
> +				snd_ctl_led_ctl_destroy(lctl);
>  				goto repeat;
>  			}
>  	}
>  }
>  
> +static int snd_ctl_led_reset(int card_number, unsigned int group)
> +{
> +	struct snd_card *card;
> +	struct snd_ctl_led *led;
> +	struct snd_ctl_led_ctl *lctl;
> +	struct snd_kcontrol_volatile *vd;
> +	bool change = false;
> +
> +	card = snd_card_ref(card_number);
> +	if (!card)
> +		return -ENXIO;
> +
> +	mutex_lock(&snd_ctl_led_mutex);
> +	if (!snd_ctl_led_card_valid[card_number]) {
> +		mutex_unlock(&snd_ctl_led_mutex);
> +		snd_card_unref(card);
> +		return -ENXIO;
> +	}
> +	led = &snd_ctl_leds[group];
> +repeat:
> +	list_for_each_entry(lctl, &led->controls, list)
> +		if (lctl->card == card) {
> +			vd = &lctl->kctl->vd[lctl->index_offset];
> +			vd->access &= ~group_to_access(group);
> +			snd_ctl_led_ctl_destroy(lctl);
> +			change = true;
> +			goto repeat;
> +		}
> +	mutex_unlock(&snd_ctl_led_mutex);
> +	if (change)
> +		snd_ctl_led_set_state(NULL, group_to_access(group), NULL, 0);
> +	snd_card_unref(card);
> +	return 0;
> +}
> +
>  static void snd_ctl_led_register(struct snd_card *card)
>  {
>  	struct snd_kcontrol *kctl;
> @@ -264,10 +354,12 @@ static void snd_ctl_led_register(struct snd_card *card)
>  		for (ioff = 0; ioff < kctl->count; ioff++)
>  			snd_ctl_led_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, kctl, ioff);
>  	snd_ctl_led_refresh();
> +	snd_ctl_led_sysfs_add(card);
>  }
>  
>  static void snd_ctl_led_disconnect(struct snd_card *card)
>  {
> +	snd_ctl_led_sysfs_remove(card);
>  	mutex_lock(&snd_ctl_led_mutex);
>  	snd_ctl_led_card_valid[card->number] = false;
>  	snd_ctl_led_clean(card);
> @@ -349,8 +441,264 @@ static const struct attribute_group *snd_ctl_led_dev_attr_groups[] = {
>  	NULL,
>  };
>  
> +static char *find_eos(char *s)
> +{
> +	while (*s && *s != ',')
> +		s++;
> +	if (*s)
> +		s++;
> +	return s;
> +}
> +
> +static char *parse_uint(char *s, unsigned int *val)
> +{
> +	unsigned long long res;
> +	if (kstrtoull(s, 10, &res))
> +		res = 0;
> +	*val = res;
> +	return find_eos(s);
> +}
> +
> +static char *parse_string(char *s, char *val, size_t val_size)
> +{
> +	if (*s == '"' || *s == '\'') {
> +		char c = *s;
> +		s++;
> +		while (*s && *s != c) {
> +			if (val_size > 1) {
> +				*val++ = *s;
> +				val_size--;
> +			}
> +			s++;
> +		}
> +	} else {
> +		while (*s && *s != ',') {
> +			if (val_size > 1) {
> +				*val++ = *s;
> +				val_size--;
> +			}
> +			s++;
> +		}
> +	}
> +	*val = '\0';
> +	if (*s)
> +		s++;
> +	return s;
> +}
> +
> +static char *parse_iface(char *s, unsigned int *val)
> +{
> +	if (!strncasecmp(s, "card", 4))
> +		*val = SNDRV_CTL_ELEM_IFACE_CARD;
> +	else if (!strncasecmp(s, "mixer", 5))
> +		*val = SNDRV_CTL_ELEM_IFACE_MIXER;
> +	return find_eos(s);
> +}
> +
> +/*
> + * These types of input strings are accepted:
> + *
> + *   unsigned integer - numid (equivaled to numid=UINT)
> + *   string - basic mixer name (equivalent to iface=MIXER,name=STR)
> + *   numid=UINT
> + *   [iface=MIXER,][device=UINT,][subdevice=UINT,]name=STR[,index=UINT]
> + */
> +static ssize_t set_led_id(struct snd_ctl_led_card *led_card, const char *buf, size_t count,
> +			  bool attach)
> +{
> +	char buf2[256], *s;
> +	size_t len = max(sizeof(s) - 1, count);
> +	struct snd_ctl_elem_id id;
> +	int err;
> +
> +	strncpy(buf2, buf, len);
> +	buf2[len] = '\0';
> +	memset(&id, 0, sizeof(id));
> +	id.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
> +	s = buf2;
> +	while (*s) {
> +		if (!strncasecmp(s, "numid=", 6)) {
> +			s = parse_uint(s + 6, &id.numid);
> +		} else if (!strncasecmp(s, "iface=", 6)) {
> +			s = parse_iface(s + 6, &id.iface);
> +		} else if (!strncasecmp(s, "device=", 7)) {
> +			s = parse_uint(s + 7, &id.device);
> +		} else if (!strncasecmp(s, "subdevice=", 10)) {
> +			s = parse_uint(s + 10, &id.subdevice);
> +		} else if (!strncasecmp(s, "name=", 5)) {
> +			s = parse_string(s + 5, id.name, sizeof(id.name));
> +		} else if (!strncasecmp(s, "index=", 6)) {
> +			s = parse_uint(s + 6, &id.index);
> +		} else if (s == buf2) {
> +			while (*s) {
> +				if (*s < '0' || *s > '9')
> +					break;
> +				s++;
> +			}
> +			if (*s == '\0')
> +				parse_uint(buf2, &id.numid);
> +			else {
> +				for (; *s >= ' '; s++);
> +				*s = '\0';
> +				strlcpy(id.name, buf2, sizeof(id.name));
> +			}
> +			break;
> +		}
> +		if (*s == ',')
> +			s++;
> +	}
> +
> +	err = snd_ctl_led_set_id(led_card->number, &id, led_card->led->group, attach);
> +	if (err < 0)
> +		return err;
> +
> +	return count;
> +}
> +
> +static ssize_t parse_attach(struct device *dev, struct device_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
> +	return set_led_id(led_card, buf, count, true);
> +}
> +
> +static ssize_t parse_detach(struct device *dev, struct device_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
> +	return set_led_id(led_card, buf, count, false);
> +}
> +
> +static ssize_t ctl_reset(struct device *dev, struct device_attribute *attr,
> +			 const char *buf, size_t count)
> +{
> +	struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
> +	int err;
> +
> +	if (count > 0 && buf[0] == '1') {
> +		err = snd_ctl_led_reset(led_card->number, led_card->led->group);
> +		if (err < 0)
> +			return err;
> +	}
> +	return count;
> +}
> +
> +static ssize_t ctl_list(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
> +	struct snd_card *card;
> +	struct snd_ctl_led_ctl *lctl;
> +	char *buf2 = buf;
> +	size_t l;
> +
> +	card = snd_card_ref(led_card->number);
> +	if (!card)
> +		return -ENXIO;
> +	down_read(&card->controls_rwsem);
> +	mutex_lock(&snd_ctl_led_mutex);
> +	if (snd_ctl_led_card_valid[led_card->number]) {
> +		list_for_each_entry(lctl, &led_card->led->controls, list)
> +			if (lctl->card == card) {
> +				if (buf2 - buf > PAGE_SIZE - 16)
> +					break;
> +				if (buf2 != buf)
> +					*buf2++ = ' ';
> +				l = scnprintf(buf2, 15, "%u",
> +						lctl->kctl->id.numid +
> +							lctl->index_offset);
> +				buf2[l] = '\0';
> +				buf2 += l + 1;
> +			}
> +	}
> +	mutex_unlock(&snd_ctl_led_mutex);
> +	up_read(&card->controls_rwsem);
> +	snd_card_unref(card);
> +	return buf2 - buf;
> +}
> +
> +static DEVICE_ATTR(attach, 0200, NULL, parse_attach);
> +static DEVICE_ATTR(detach, 0200, NULL, parse_detach);
> +static DEVICE_ATTR(reset, 0200, NULL, ctl_reset);
> +static DEVICE_ATTR(list, 0444, ctl_list, NULL);
> +
> +static struct attribute *snd_ctl_led_card_attrs[] = {
> +	&dev_attr_attach.attr,
> +	&dev_attr_detach.attr,
> +	&dev_attr_reset.attr,
> +	&dev_attr_list.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group snd_ctl_led_card_attr_group = {
> +	.attrs = snd_ctl_led_card_attrs,
> +};
> +
> +static const struct attribute_group *snd_ctl_led_card_attr_groups[] = {
> +	&snd_ctl_led_card_attr_group,
> +	NULL,
> +};
> +
>  static struct device snd_ctl_led_dev;
>  
> +static void snd_ctl_led_sysfs_add(struct snd_card *card)
> +{
> +	unsigned int group;
> +	struct snd_ctl_led_card *led_card;
> +	struct snd_ctl_led *led;
> +	char link_name[32];
> +
> +	for (group = 0; group < MAX_LED; group++) {
> +		led = &snd_ctl_leds[group];
> +		led_card = kzalloc(sizeof(*led_card), GFP_KERNEL);
> +		if (!led_card)
> +			goto cerr2;
> +		led_card->number = card->number;
> +		led_card->led = led;
> +		device_initialize(&led_card->dev);
> +		if (dev_set_name(&led_card->dev, "card%d", card->number) < 0)
> +			goto cerr;
> +		led_card->dev.parent = &led->dev;
> +		led_card->dev.groups = snd_ctl_led_card_attr_groups;
> +		if (device_add(&led_card->dev))
> +			goto cerr;
> +		led->cards[card->number] = led_card;
> +		snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> +		WARN(sysfs_create_link(&card->ctl_dev.kobj, &led_card->dev.kobj, link_name),
> +			"can't create symlink to controlC%i device\n", card->number);
> +		WARN(sysfs_create_link(&led_card->dev.kobj, &card->card_dev.kobj, "card"),
> +			"can't create symlink to card%i\n", card->number);
> +
> +		continue;
> +cerr:
> +		put_device(&led_card->dev);
> +cerr2:
> +		printk(KERN_ERR "snd_ctl_led: unable to add card%d", card->number);
> +		kfree(led_card);
> +	}
> +}
> +
> +static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> +{
> +	unsigned int group;
> +	struct snd_ctl_led_card *led_card;
> +	struct snd_ctl_led *led;
> +	char link_name[32];
> +
> +	for (group = 0; group < MAX_LED; group++) {
> +		led = &snd_ctl_leds[group];
> +		led_card = led->cards[card->number];
> +		if (!led_card)
> +			continue;
> +		snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> +		sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> +		sysfs_remove_link(&led_card->dev.kobj, "card");
> +		device_del(&led_card->dev);
> +		kfree(led_card);
> +		led->cards[card->number] = NULL;
> +	}
> +}
> +
>  /*
>   * Control layer registration
>   */
> @@ -397,14 +745,24 @@ static int __init snd_ctl_led_init(void)
>  static void __exit snd_ctl_led_exit(void)
>  {
>  	struct snd_ctl_led *led;
> -	unsigned int group;
> +	struct snd_card *card;
> +	unsigned int group, card_number;
>  
> +	snd_ctl_disconnect_layer(&snd_ctl_led_lops);
> +	for (card_number = 0; card_number < SNDRV_CARDS; card_number++) {
> +		if (!snd_ctl_led_card_valid[card_number])
> +			continue;
> +		card = snd_card_ref(card_number);
> +		if (card) {
> +			snd_ctl_led_sysfs_remove(card);
> +			snd_card_unref(card);
> +		}
> +	}
>  	for (group = 0; group < MAX_LED; group++) {
>  		led = &snd_ctl_leds[group];
>  		device_del(&led->dev);
>  	}
>  	device_del(&snd_ctl_led_dev);
> -	snd_ctl_disconnect_layer(&snd_ctl_led_lops);
>  	snd_ctl_led_clean(NULL);
>  }
>  
>

Takashi Iwai March 19, 2021, 5:22 p.m. UTC | #2

On Fri, 19 Mar 2021 17:34:39 +0100,
Hans de Goede wrote:
> 
> Hi,
> 
> On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
> > We need to manage the kcontrol entries association for the LED trigger
> > from the user space. This patch adds a layer to the sysfs tree like:
> > 
> > /sys/devices/virtual/sound/ctl-led/mic
> >    + card0
> >    |  + attach
> >    |  + detach
> >    |  ...
> >    + card1
> >       + attach
> >       ...
> > 
> > Operations:
> > 
> >   attach and detach
> >     - amixer style ID is accepted and easy strings for numid and
> >       simple names
> >   reset
> >     - reset all associated kcontrol entries
> >   list
> >     - list associated kcontrol entries (numid values only)
> > 
> > Additional symlinks:
> > 
> > /sys/devices/virtual/sound/ctl-led/mic/card0/card ->
> >   /sys/class/sound/card0
> > 
> > /sys/class/sound/card0/controlC0/led-mic ->
> >   /sys/devices/virtual/sound/ctl-led/mic/card0
> > 
> > Signed-off-by: Jaroslav Kysela <[e-mail]>
> 
> Thank you so much for this patch.
> 
> I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
> 
> And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute
> LED build into the (detachable) USB-keyboard's mute hot-key:
> 
> modprobe snd_ctl_led
> echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
> echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
> 
> This needs to be replaced by some UCM profile code doing the equivalent of course,
> but for a proof-of-concept test of the kernel API this introduces the above will do.

IMO, that's the question: how we'll enable this in future.  If the
binding of the control/mute mapping is provided via UCM, it's supposed
to be changeable by each user.  Then the current sysfs permission
doesn't fit.  OTOH, if it's 0666, it's accessible to all users even
remotely, which is worse than the access with the normal sound device
file.  Or if it's supposed to be changed via udev stuff or systemd?
Or is it just for debugging?

Through a quick glance over the series, I'm fine to take those
patches, but the only concern is the sysfs entries.  Basically, once
when we use sysfs entries, it's set in stone.  So we should be very
clear about our strategy how to deploy the control/mute mapping
regarding using those sysfs entries.

OTOH, if the interface is thought for debugging or development
purpose, it could be done in debugfs, which we can keep playing in
further development, too.

And, BTW, the mute LED mode setup doesn't have to be sysfs entries;
we'd need primarily only the flags for inverted LED behavior, and
those are only two, so it could be simply module options.  Then it's
even easier for users to set up than tweaking sysfs entries.


thanks,

Takashi

> 
> Only complaint which I have is the need to add "-n" to the echo commands,
> it would be nice if set_led_id() would check if the copy which it stores in buf2
> ends with "\n" and if it does if it would then strip that from the copy in buf2.
> 
> Regards,
> 
> Hans
> 
> 
> p.s.
> 
> Note this does need my recently listed alsa=lib patches so that these "Channel Switch" controls
> get grouped with the "Speaker Playback Volume" / "HP Playback Volume" controls, so that the
> volume-hw control code will actually toggle them:
> 
> https://lore.kernel.org/alsa-devel/[e-mail]/T/#u
> 
> Talking about that series, what is the status of that ? From my POV it
> is ready for merging...
> 
> 
> 
> > ---
> >  sound/core/control_led.c | 366 ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 362 insertions(+), 4 deletions(-)
> > 
> > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > index dfa51d8461e1..d4fb8b873f34 100644
> > --- a/sound/core/control_led.c
> > +++ b/sound/core/control_led.c
> > @@ -24,6 +24,12 @@ enum snd_ctl_led_mode {
> >  	 MODE_ON,
> >  };
> >  
> > +struct snd_ctl_led_card {
> > +	struct device dev;
> > +	int number;
> > +	struct snd_ctl_led *led;
> > +};
> > +
> >  struct snd_ctl_led {
> >  	struct device dev;
> >  	struct list_head controls;
> > @@ -31,6 +37,7 @@ struct snd_ctl_led {
> >  	unsigned int group;
> >  	enum led_audio trigger_type;
> >  	enum snd_ctl_led_mode mode;
> > +	struct snd_ctl_led_card *cards[SNDRV_CARDS];
> >  };
> >  
> >  struct snd_ctl_led_ctl {
> > @@ -58,6 +65,9 @@ static struct snd_ctl_led snd_ctl_leds[MAX_LED] = {
> >  	},
> >  };
> >  
> > +static void snd_ctl_led_sysfs_add(struct snd_card *card);
> > +static void snd_ctl_led_sysfs_remove(struct snd_card *card);
> > +
> >  #define UPDATE_ROUTE(route, cb) \
> >  	do { \
> >  		int route2 = (cb); \
> > @@ -222,6 +232,46 @@ static void snd_ctl_led_notify(struct snd_card *card, unsigned int mask,
> >  	}
> >  }
> >  
> > +static int snd_ctl_led_set_id(int card_number, struct snd_ctl_elem_id *id,
> > +			      unsigned int group, bool set)
> > +{
> > +	struct snd_card *card;
> > +	struct snd_kcontrol *kctl;
> > +	struct snd_kcontrol_volatile *vd;
> > +	unsigned int ioff, access, new_access;
> > +	int err = 0;
> > +
> > +	card = snd_card_ref(card_number);
> > +	if (card) {
> > +		down_write(&card->controls_rwsem);
> > +		kctl = snd_ctl_find_id(card, id);
> > +		if (kctl) {
> > +			ioff = snd_ctl_get_ioff(kctl, id);
> > +			vd = &kctl->vd[ioff];
> > +			access = vd->access & SNDRV_CTL_ELEM_ACCESS_LED_MASK;
> > +			if (access != 0 && access != group_to_access(group)) {
> > +				err = -EXDEV;
> > +				goto unlock;
> > +			}
> > +			new_access = vd->access & ~SNDRV_CTL_ELEM_ACCESS_LED_MASK;
> > +			if (set)
> > +				new_access |= group_to_access(group);
> > +			if (new_access != vd->access) {
> > +				vd->access = new_access;
> > +				snd_ctl_led_notify(card, SNDRV_CTL_EVENT_MASK_INFO, kctl, ioff);
> > +			}
> > +		} else {
> > +			err = -ENOENT;
> > +		}
> > +unlock:
> > +		up_write(&card->controls_rwsem);
> > +		snd_card_unref(card);
> > +	} else {
> > +		err = -ENXIO;
> > +	}
> > +	return err;
> > +}
> > +
> >  static void snd_ctl_led_refresh(void)
> >  {
> >  	unsigned int group;
> > @@ -230,6 +280,12 @@ static void snd_ctl_led_refresh(void)
> >  		snd_ctl_led_set_state(NULL, group_to_access(group), NULL, 0);
> >  }
> >  
> > +static void snd_ctl_led_ctl_destroy(struct snd_ctl_led_ctl *lctl)
> > +{
> > +	list_del(&lctl->list);
> > +	kfree(lctl);
> > +}
> > +
> >  static void snd_ctl_led_clean(struct snd_card *card)
> >  {
> >  	unsigned int group;
> > @@ -241,13 +297,47 @@ static void snd_ctl_led_clean(struct snd_card *card)
> >  repeat:
> >  		list_for_each_entry(lctl, &led->controls, list)
> >  			if (!card || lctl->card == card) {
> > -				list_del(&lctl->list);
> > -				kfree(lctl);
> > +				snd_ctl_led_ctl_destroy(lctl);
> >  				goto repeat;
> >  			}
> >  	}
> >  }
> >  
> > +static int snd_ctl_led_reset(int card_number, unsigned int group)
> > +{
> > +	struct snd_card *card;
> > +	struct snd_ctl_led *led;
> > +	struct snd_ctl_led_ctl *lctl;
> > +	struct snd_kcontrol_volatile *vd;
> > +	bool change = false;
> > +
> > +	card = snd_card_ref(card_number);
> > +	if (!card)
> > +		return -ENXIO;
> > +
> > +	mutex_lock(&snd_ctl_led_mutex);
> > +	if (!snd_ctl_led_card_valid[card_number]) {
> > +		mutex_unlock(&snd_ctl_led_mutex);
> > +		snd_card_unref(card);
> > +		return -ENXIO;
> > +	}
> > +	led = &snd_ctl_leds[group];
> > +repeat:
> > +	list_for_each_entry(lctl, &led->controls, list)
> > +		if (lctl->card == card) {
> > +			vd = &lctl->kctl->vd[lctl->index_offset];
> > +			vd->access &= ~group_to_access(group);
> > +			snd_ctl_led_ctl_destroy(lctl);
> > +			change = true;
> > +			goto repeat;
> > +		}
> > +	mutex_unlock(&snd_ctl_led_mutex);
> > +	if (change)
> > +		snd_ctl_led_set_state(NULL, group_to_access(group), NULL, 0);
> > +	snd_card_unref(card);
> > +	return 0;
> > +}
> > +
> >  static void snd_ctl_led_register(struct snd_card *card)
> >  {
> >  	struct snd_kcontrol *kctl;
> > @@ -264,10 +354,12 @@ static void snd_ctl_led_register(struct snd_card *card)
> >  		for (ioff = 0; ioff < kctl->count; ioff++)
> >  			snd_ctl_led_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, kctl, ioff);
> >  	snd_ctl_led_refresh();
> > +	snd_ctl_led_sysfs_add(card);
> >  }
> >  
> >  static void snd_ctl_led_disconnect(struct snd_card *card)
> >  {
> > +	snd_ctl_led_sysfs_remove(card);
> >  	mutex_lock(&snd_ctl_led_mutex);
> >  	snd_ctl_led_card_valid[card->number] = false;
> >  	snd_ctl_led_clean(card);
> > @@ -349,8 +441,264 @@ static const struct attribute_group *snd_ctl_led_dev_attr_groups[] = {
> >  	NULL,
> >  };
> >  
> > +static char *find_eos(char *s)
> > +{
> > +	while (*s && *s != ',')
> > +		s++;
> > +	if (*s)
> > +		s++;
> > +	return s;
> > +}
> > +
> > +static char *parse_uint(char *s, unsigned int *val)
> > +{
> > +	unsigned long long res;
> > +	if (kstrtoull(s, 10, &res))
> > +		res = 0;
> > +	*val = res;
> > +	return find_eos(s);
> > +}
> > +
> > +static char *parse_string(char *s, char *val, size_t val_size)
> > +{
> > +	if (*s == '"' || *s == '\'') {
> > +		char c = *s;
> > +		s++;
> > +		while (*s && *s != c) {
> > +			if (val_size > 1) {
> > +				*val++ = *s;
> > +				val_size--;
> > +			}
> > +			s++;
> > +		}
> > +	} else {
> > +		while (*s && *s != ',') {
> > +			if (val_size > 1) {
> > +				*val++ = *s;
> > +				val_size--;
> > +			}
> > +			s++;
> > +		}
> > +	}
> > +	*val = '\0';
> > +	if (*s)
> > +		s++;
> > +	return s;
> > +}
> > +
> > +static char *parse_iface(char *s, unsigned int *val)
> > +{
> > +	if (!strncasecmp(s, "card", 4))
> > +		*val = SNDRV_CTL_ELEM_IFACE_CARD;
> > +	else if (!strncasecmp(s, "mixer", 5))
> > +		*val = SNDRV_CTL_ELEM_IFACE_MIXER;
> > +	return find_eos(s);
> > +}
> > +
> > +/*
> > + * These types of input strings are accepted:
> > + *
> > + *   unsigned integer - numid (equivaled to numid=UINT)
> > + *   string - basic mixer name (equivalent to iface=MIXER,name=STR)
> > + *   numid=UINT
> > + *   [iface=MIXER,][device=UINT,][subdevice=UINT,]name=STR[,index=UINT]
> > + */
> > +static ssize_t set_led_id(struct snd_ctl_led_card *led_card, const char *buf, size_t count,
> > +			  bool attach)
> > +{
> > +	char buf2[256], *s;
> > +	size_t len = max(sizeof(s) - 1, count);
> > +	struct snd_ctl_elem_id id;
> > +	int err;
> > +
> > +	strncpy(buf2, buf, len);
> > +	buf2[len] = '\0';
> > +	memset(&id, 0, sizeof(id));
> > +	id.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
> > +	s = buf2;
> > +	while (*s) {
> > +		if (!strncasecmp(s, "numid=", 6)) {
> > +			s = parse_uint(s + 6, &id.numid);
> > +		} else if (!strncasecmp(s, "iface=", 6)) {
> > +			s = parse_iface(s + 6, &id.iface);
> > +		} else if (!strncasecmp(s, "device=", 7)) {
> > +			s = parse_uint(s + 7, &id.device);
> > +		} else if (!strncasecmp(s, "subdevice=", 10)) {
> > +			s = parse_uint(s + 10, &id.subdevice);
> > +		} else if (!strncasecmp(s, "name=", 5)) {
> > +			s = parse_string(s + 5, id.name, sizeof(id.name));
> > +		} else if (!strncasecmp(s, "index=", 6)) {
> > +			s = parse_uint(s + 6, &id.index);
> > +		} else if (s == buf2) {
> > +			while (*s) {
> > +				if (*s < '0' || *s > '9')
> > +					break;
> > +				s++;
> > +			}
> > +			if (*s == '\0')
> > +				parse_uint(buf2, &id.numid);
> > +			else {
> > +				for (; *s >= ' '; s++);
> > +				*s = '\0';
> > +				strlcpy(id.name, buf2, sizeof(id.name));
> > +			}
> > +			break;
> > +		}
> > +		if (*s == ',')
> > +			s++;
> > +	}
> > +
> > +	err = snd_ctl_led_set_id(led_card->number, &id, led_card->led->group, attach);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	return count;
> > +}
> > +
> > +static ssize_t parse_attach(struct device *dev, struct device_attribute *attr,
> > +			    const char *buf, size_t count)
> > +{
> > +	struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
> > +	return set_led_id(led_card, buf, count, true);
> > +}
> > +
> > +static ssize_t parse_detach(struct device *dev, struct device_attribute *attr,
> > +			    const char *buf, size_t count)
> > +{
> > +	struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
> > +	return set_led_id(led_card, buf, count, false);
> > +}
> > +
> > +static ssize_t ctl_reset(struct device *dev, struct device_attribute *attr,
> > +			 const char *buf, size_t count)
> > +{
> > +	struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
> > +	int err;
> > +
> > +	if (count > 0 && buf[0] == '1') {
> > +		err = snd_ctl_led_reset(led_card->number, led_card->led->group);
> > +		if (err < 0)
> > +			return err;
> > +	}
> > +	return count;
> > +}
> > +
> > +static ssize_t ctl_list(struct device *dev,
> > +			struct device_attribute *attr, char *buf)
> > +{
> > +	struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
> > +	struct snd_card *card;
> > +	struct snd_ctl_led_ctl *lctl;
> > +	char *buf2 = buf;
> > +	size_t l;
> > +
> > +	card = snd_card_ref(led_card->number);
> > +	if (!card)
> > +		return -ENXIO;
> > +	down_read(&card->controls_rwsem);
> > +	mutex_lock(&snd_ctl_led_mutex);
> > +	if (snd_ctl_led_card_valid[led_card->number]) {
> > +		list_for_each_entry(lctl, &led_card->led->controls, list)
> > +			if (lctl->card == card) {
> > +				if (buf2 - buf > PAGE_SIZE - 16)
> > +					break;
> > +				if (buf2 != buf)
> > +					*buf2++ = ' ';
> > +				l = scnprintf(buf2, 15, "%u",
> > +						lctl->kctl->id.numid +
> > +							lctl->index_offset);
> > +				buf2[l] = '\0';
> > +				buf2 += l + 1;
> > +			}
> > +	}
> > +	mutex_unlock(&snd_ctl_led_mutex);
> > +	up_read(&card->controls_rwsem);
> > +	snd_card_unref(card);
> > +	return buf2 - buf;
> > +}
> > +
> > +static DEVICE_ATTR(attach, 0200, NULL, parse_attach);
> > +static DEVICE_ATTR(detach, 0200, NULL, parse_detach);
> > +static DEVICE_ATTR(reset, 0200, NULL, ctl_reset);
> > +static DEVICE_ATTR(list, 0444, ctl_list, NULL);
> > +
> > +static struct attribute *snd_ctl_led_card_attrs[] = {
> > +	&dev_attr_attach.attr,
> > +	&dev_attr_detach.attr,
> > +	&dev_attr_reset.attr,
> > +	&dev_attr_list.attr,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group snd_ctl_led_card_attr_group = {
> > +	.attrs = snd_ctl_led_card_attrs,
> > +};
> > +
> > +static const struct attribute_group *snd_ctl_led_card_attr_groups[] = {
> > +	&snd_ctl_led_card_attr_group,
> > +	NULL,
> > +};
> > +
> >  static struct device snd_ctl_led_dev;
> >  
> > +static void snd_ctl_led_sysfs_add(struct snd_card *card)
> > +{
> > +	unsigned int group;
> > +	struct snd_ctl_led_card *led_card;
> > +	struct snd_ctl_led *led;
> > +	char link_name[32];
> > +
> > +	for (group = 0; group < MAX_LED; group++) {
> > +		led = &snd_ctl_leds[group];
> > +		led_card = kzalloc(sizeof(*led_card), GFP_KERNEL);
> > +		if (!led_card)
> > +			goto cerr2;
> > +		led_card->number = card->number;
> > +		led_card->led = led;
> > +		device_initialize(&led_card->dev);
> > +		if (dev_set_name(&led_card->dev, "card%d", card->number) < 0)
> > +			goto cerr;
> > +		led_card->dev.parent = &led->dev;
> > +		led_card->dev.groups = snd_ctl_led_card_attr_groups;
> > +		if (device_add(&led_card->dev))
> > +			goto cerr;
> > +		led->cards[card->number] = led_card;
> > +		snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> > +		WARN(sysfs_create_link(&card->ctl_dev.kobj, &led_card->dev.kobj, link_name),
> > +			"can't create symlink to controlC%i device\n", card->number);
> > +		WARN(sysfs_create_link(&led_card->dev.kobj, &card->card_dev.kobj, "card"),
> > +			"can't create symlink to card%i\n", card->number);
> > +
> > +		continue;
> > +cerr:
> > +		put_device(&led_card->dev);
> > +cerr2:
> > +		printk(KERN_ERR "snd_ctl_led: unable to add card%d", card->number);
> > +		kfree(led_card);
> > +	}
> > +}
> > +
> > +static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > +{
> > +	unsigned int group;
> > +	struct snd_ctl_led_card *led_card;
> > +	struct snd_ctl_led *led;
> > +	char link_name[32];
> > +
> > +	for (group = 0; group < MAX_LED; group++) {
> > +		led = &snd_ctl_leds[group];
> > +		led_card = led->cards[card->number];
> > +		if (!led_card)
> > +			continue;
> > +		snprintf(link_name, sizeof(link_name), "led-%s", led->name);
> > +		sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > +		sysfs_remove_link(&led_card->dev.kobj, "card");
> > +		device_del(&led_card->dev);
> > +		kfree(led_card);
> > +		led->cards[card->number] = NULL;
> > +	}
> > +}
> > +
> >  /*
> >   * Control layer registration
> >   */
> > @@ -397,14 +745,24 @@ static int __init snd_ctl_led_init(void)
> >  static void __exit snd_ctl_led_exit(void)
> >  {
> >  	struct snd_ctl_led *led;
> > -	unsigned int group;
> > +	struct snd_card *card;
> > +	unsigned int group, card_number;
> >  
> > +	snd_ctl_disconnect_layer(&snd_ctl_led_lops);
> > +	for (card_number = 0; card_number < SNDRV_CARDS; card_number++) {
> > +		if (!snd_ctl_led_card_valid[card_number])
> > +			continue;
> > +		card = snd_card_ref(card_number);
> > +		if (card) {
> > +			snd_ctl_led_sysfs_remove(card);
> > +			snd_card_unref(card);
> > +		}
> > +	}
> >  	for (group = 0; group < MAX_LED; group++) {
> >  		led = &snd_ctl_leds[group];
> >  		device_del(&led->dev);
> >  	}
> >  	device_del(&snd_ctl_led_dev);
> > -	snd_ctl_disconnect_layer(&snd_ctl_led_lops);
> >  	snd_ctl_led_clean(NULL);
> >  }
> >  
> > 
>

Jaroslav Kysela March 19, 2021, 5:58 p.m. UTC | #3

Dne 19. 03. 21 v 18:22 Takashi Iwai napsal(a):
> On Fri, 19 Mar 2021 17:34:39 +0100,
> Hans de Goede wrote:
>>
>> Hi,
>>
>> On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
>>> We need to manage the kcontrol entries association for the LED trigger
>>> from the user space. This patch adds a layer to the sysfs tree like:
>>>
>>> /sys/devices/virtual/sound/ctl-led/mic
>>>    + card0
>>>    |  + attach
>>>    |  + detach
>>>    |  ...
>>>    + card1
>>>       + attach
>>>       ...
>>>
>>> Operations:
>>>
>>>   attach and detach
>>>     - amixer style ID is accepted and easy strings for numid and
>>>       simple names
>>>   reset
>>>     - reset all associated kcontrol entries
>>>   list
>>>     - list associated kcontrol entries (numid values only)
>>>
>>> Additional symlinks:
>>>
>>> /sys/devices/virtual/sound/ctl-led/mic/card0/card ->
>>>   /sys/class/sound/card0
>>>
>>> /sys/class/sound/card0/controlC0/led-mic ->
>>>   /sys/devices/virtual/sound/ctl-led/mic/card0
>>>
>>> Signed-off-by: Jaroslav Kysela <[e-mail]>
>>
>> Thank you so much for this patch.
>>
>> I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
>>
>> And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute
>> LED build into the (detachable) USB-keyboard's mute hot-key:
>>
>> modprobe snd_ctl_led
>> echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
>> echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
>>
>> This needs to be replaced by some UCM profile code doing the equivalent of course,
>> but for a proof-of-concept test of the kernel API this introduces the above will do.
> 
> IMO, that's the question: how we'll enable this in future.  If the
> binding of the control/mute mapping is provided via UCM, it's supposed
> to be changeable by each user.  Then the current sysfs permission

Nope. We have two UCM boot sequences which are called from "alsactl init" only
now. So, respecting the security concerns, only root should "fiddle" with this
settings. So yes, udev + alsactl (or any script) executed as root.

> Through a quick glance over the series, I'm fine to take those
> patches, but the only concern is the sysfs entries.  Basically, once
> when we use sysfs entries, it's set in stone.  So we should be very
> clear about our strategy how to deploy the control/mute mapping
> regarding using those sysfs entries.
> 
> OTOH, if the interface is thought for debugging or development
> purpose, it could be done in debugfs, which we can keep playing in
> further development, too.

We need attach / detach (reset and list operations are optional, but nice to
have). If you have any other idea, let me know.

> And, BTW, the mute LED mode setup doesn't have to be sysfs entries;
> we'd need primarily only the flags for inverted LED behavior, and
> those are only two, so it could be simply module options.  Then it's
> even easier for users to set up than tweaking sysfs entries.

I don't insist to control this over sysfs. But if sysfs is in the game, it's
nice to have the runtime control for this. The module parameter may be added
to modify the default value.

						Jaroslav

Jaroslav Kysela March 19, 2021, 6:11 p.m. UTC | #4

Dne 19. 03. 21 v 17:34 Hans de Goede napsal(a):
> Hi,
> 
> On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
>> We need to manage the kcontrol entries association for the LED trigger
>> from the user space. This patch adds a layer to the sysfs tree like:
>>
>> /sys/devices/virtual/sound/ctl-led/mic
>>    + card0
>>    |  + attach
>>    |  + detach
>>    |  ...
>>    + card1
>>       + attach
>>       ...
>>
>> Operations:
>>
>>   attach and detach
>>     - amixer style ID is accepted and easy strings for numid and
>>       simple names
>>   reset
>>     - reset all associated kcontrol entries
>>   list
>>     - list associated kcontrol entries (numid values only)
>>
>> Additional symlinks:
>>
>> /sys/devices/virtual/sound/ctl-led/mic/card0/card ->
>>   /sys/class/sound/card0
>>
>> /sys/class/sound/card0/controlC0/led-mic ->
>>   /sys/devices/virtual/sound/ctl-led/mic/card0
>>
>> Signed-off-by: Jaroslav Kysela <[e-mail]>
> 
> Thank you so much for this patch.
> 
> I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
> 
> And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute
> LED build into the (detachable) USB-keyboard's mute hot-key:
> 
> modprobe snd_ctl_led
> echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
> echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
> 
> This needs to be replaced by some UCM profile code doing the equivalent of course,
> but for a proof-of-concept test of the kernel API this introduces the above will do.

I added already the FixedBootSequence support to alsa-lib and alsactl and the "sysset" sequence command. But looking to this command now, it may be better to rename it to "sysw" ("double s" does not look so great).

> Only complaint which I have is the need to add "-n" to the echo commands,
> it would be nice if set_led_id() would check if the copy which it stores in buf2
> ends with "\n" and if it does if it would then strip that from the copy in buf2.

Yes, I will fix that. It's possible to use the shorter string:

echo "Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach


				Jaroslav

Hans de Goede March 19, 2021, 10:08 p.m. UTC | #5

Hi,

On 3/19/21 6:22 PM, Takashi Iwai wrote:
> On Fri, 19 Mar 2021 17:34:39 +0100,
> Hans de Goede wrote:
>>
>> Hi,
>>
>> On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
>>> We need to manage the kcontrol entries association for the LED trigger
>>> from the user space. This patch adds a layer to the sysfs tree like:
>>>
>>> /sys/devices/virtual/sound/ctl-led/mic
>>>    + card0
>>>    |  + attach
>>>    |  + detach
>>>    |  ...
>>>    + card1
>>>       + attach
>>>       ...
>>>
>>> Operations:
>>>
>>>   attach and detach
>>>     - amixer style ID is accepted and easy strings for numid and
>>>       simple names
>>>   reset
>>>     - reset all associated kcontrol entries
>>>   list
>>>     - list associated kcontrol entries (numid values only)
>>>
>>> Additional symlinks:
>>>
>>> /sys/devices/virtual/sound/ctl-led/mic/card0/card ->
>>>   /sys/class/sound/card0
>>>
>>> /sys/class/sound/card0/controlC0/led-mic ->
>>>   /sys/devices/virtual/sound/ctl-led/mic/card0
>>>
>>> Signed-off-by: Jaroslav Kysela <[e-mail]>
>>
>> Thank you so much for this patch.
>>
>> I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
>>
>> And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute
>> LED build into the (detachable) USB-keyboard's mute hot-key:
>>
>> modprobe snd_ctl_led
>> echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
>> echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
>>
>> This needs to be replaced by some UCM profile code doing the equivalent of course,
>> but for a proof-of-concept test of the kernel API this introduces the above will do.
> 
> IMO, that's the question: how we'll enable this in future.  If the
> binding of the control/mute mapping is provided via UCM, it's supposed
> to be changeable by each user.  Then the current sysfs permission
> doesn't fit.  OTOH, if it's 0666, it's accessible to all users even
> remotely, which is worse than the access with the normal sound device
> file.  Or if it's supposed to be changed via udev stuff or systemd?
> Or is it just for debugging?
> 
> Through a quick glance over the series, I'm fine to take those
> patches, but the only concern is the sysfs entries.  Basically, once
> when we use sysfs entries, it's set in stone.  So we should be very
> clear about our strategy how to deploy the control/mute mapping
> regarding using those sysfs entries.
> 
> OTOH, if the interface is thought for debugging or development
> purpose, it could be done in debugfs, which we can keep playing in
> further development, too.
> 
> And, BTW, the mute LED mode setup doesn't have to be sysfs entries;
> we'd need primarily only the flags for inverted LED behavior, and
> those are only two, so it could be simply module options.  Then it's
> even easier for users to set up than tweaking sysfs entries.

The flexibility offered by this new sysfs API is necessary for the ASoC
codec drivers, because Mark does not want to have which controls are
tied to the LED triggers hard-coded inside the codec drivers.

So as Jaroslav mentions in his reply, the plan is to have the UCM
profiles contain commands to setup the LED triggers to this new
sysfs API.

Regards,

Hans

Hans de Goede March 19, 2021, 10:13 p.m. UTC | #6

Hi,

On 3/19/21 7:11 PM, Jaroslav Kysela wrote:
> Dne 19. 03. 21 v 17:34 Hans de Goede napsal(a):
>> Hi,
>>
>> On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
>>> We need to manage the kcontrol entries association for the LED trigger
>>> from the user space. This patch adds a layer to the sysfs tree like:
>>>
>>> /sys/devices/virtual/sound/ctl-led/mic
>>>    + card0
>>>    |  + attach
>>>    |  + detach
>>>    |  ...
>>>    + card1
>>>       + attach
>>>       ...
>>>
>>> Operations:
>>>
>>>   attach and detach
>>>     - amixer style ID is accepted and easy strings for numid and
>>>       simple names
>>>   reset
>>>     - reset all associated kcontrol entries
>>>   list
>>>     - list associated kcontrol entries (numid values only)
>>>
>>> Additional symlinks:
>>>
>>> /sys/devices/virtual/sound/ctl-led/mic/card0/card ->
>>>   /sys/class/sound/card0
>>>
>>> /sys/class/sound/card0/controlC0/led-mic ->
>>>   /sys/devices/virtual/sound/ctl-led/mic/card0
>>>
>>> Signed-off-by: Jaroslav Kysela <[e-mail]>
>>
>> Thank you so much for this patch.
>>
>> I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
>>
>> And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute
>> LED build into the (detachable) USB-keyboard's mute hot-key:
>>
>> modprobe snd_ctl_led
>> echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
>> echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
>>
>> This needs to be replaced by some UCM profile code doing the equivalent of course,
>> but for a proof-of-concept test of the kernel API this introduces the above will do.
> 
> I added already the FixedBootSequence support to alsa-lib and alsactl and the "sysset" sequence command.

Oh, nice.

I'll look into testing the FixedBootSequence + sysset changes to replace my rc.local hack.

One thing still seems to missing from the puzzle though, what about the "modprobe snd_ctl_led",
I guess we only want to do that on systems where that module is necessary, which means also having
a command for that in the FixedBootSequence ?

I guess we can use the "exec" keyword in the FixedBootSequence to do the modprobe ?

> But looking to this command now, it may be better to rename it to "sysw" ("double s" does not look so great).

"sysset" is fine by me, but so is "sysw" .

Regards,

Hans

Takashi Iwai March 20, 2021, 7:41 a.m. UTC | #7

On Fri, 19 Mar 2021 23:08:33 +0100,
Hans de Goede wrote:
> 
> Hi,
> 
> On 3/19/21 6:22 PM, Takashi Iwai wrote:
> > On Fri, 19 Mar 2021 17:34:39 +0100,
> > Hans de Goede wrote:
> >>
> >> Hi,
> >>
> >> On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
> >>> We need to manage the kcontrol entries association for the LED trigger
> >>> from the user space. This patch adds a layer to the sysfs tree like:
> >>>
> >>> /sys/devices/virtual/sound/ctl-led/mic
> >>>    + card0
> >>>    |  + attach
> >>>    |  + detach
> >>>    |  ...
> >>>    + card1
> >>>       + attach
> >>>       ...
> >>>
> >>> Operations:
> >>>
> >>>   attach and detach
> >>>     - amixer style ID is accepted and easy strings for numid and
> >>>       simple names
> >>>   reset
> >>>     - reset all associated kcontrol entries
> >>>   list
> >>>     - list associated kcontrol entries (numid values only)
> >>>
> >>> Additional symlinks:
> >>>
> >>> /sys/devices/virtual/sound/ctl-led/mic/card0/card ->
> >>>   /sys/class/sound/card0
> >>>
> >>> /sys/class/sound/card0/controlC0/led-mic ->
> >>>   /sys/devices/virtual/sound/ctl-led/mic/card0
> >>>
> >>> Signed-off-by: Jaroslav Kysela <[e-mail]>
> >>
> >> Thank you so much for this patch.
> >>
> >> I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
> >>
> >> And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute
> >> LED build into the (detachable) USB-keyboard's mute hot-key:
> >>
> >> modprobe snd_ctl_led
> >> echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
> >> echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
> >>
> >> This needs to be replaced by some UCM profile code doing the equivalent of course,
> >> but for a proof-of-concept test of the kernel API this introduces the above will do.
> > 
> > IMO, that's the question: how we'll enable this in future.  If the
> > binding of the control/mute mapping is provided via UCM, it's supposed
> > to be changeable by each user.  Then the current sysfs permission
> > doesn't fit.  OTOH, if it's 0666, it's accessible to all users even
> > remotely, which is worse than the access with the normal sound device
> > file.  Or if it's supposed to be changed via udev stuff or systemd?
> > Or is it just for debugging?
> > 
> > Through a quick glance over the series, I'm fine to take those
> > patches, but the only concern is the sysfs entries.  Basically, once
> > when we use sysfs entries, it's set in stone.  So we should be very
> > clear about our strategy how to deploy the control/mute mapping
> > regarding using those sysfs entries.
> > 
> > OTOH, if the interface is thought for debugging or development
> > purpose, it could be done in debugfs, which we can keep playing in
> > further development, too.
> > 
> > And, BTW, the mute LED mode setup doesn't have to be sysfs entries;
> > we'd need primarily only the flags for inverted LED behavior, and
> > those are only two, so it could be simply module options.  Then it's
> > even easier for users to set up than tweaking sysfs entries.
> 
> The flexibility offered by this new sysfs API is necessary for the ASoC
> codec drivers, because Mark does not want to have which controls are
> tied to the LED triggers hard-coded inside the codec drivers.

The hard-coded mapping itself isn't always bad things, IMO.  Of
course, it's a question whether to be done in the codec driver in a
fixed routing.  A machine driver would fit well, instead; i.e. instead
of the control-access bit flag, just bind statically from the machine
driver after instantiating the kctl objects like sysfs does.

> So as Jaroslav mentions in his reply, the plan is to have the UCM
> profiles contain commands to setup the LED triggers to this new
> sysfs API.

IIUC, this won't be only UCM but also the combination of udev +
alsactl + UCM, right?

Would other OS can follow a similar pattern?  Let's check that first
(although I myself think this should be feasible).


thanks,

Takashi

Hans de Goede March 20, 2021, 9:17 a.m. UTC | #8

Hi,

On 3/20/21 8:41 AM, Takashi Iwai wrote:
> On Fri, 19 Mar 2021 23:08:33 +0100,
> Hans de Goede wrote:
>>
>> Hi,
>>
>> On 3/19/21 6:22 PM, Takashi Iwai wrote:
>>> On Fri, 19 Mar 2021 17:34:39 +0100,
>>> Hans de Goede wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
>>>>> We need to manage the kcontrol entries association for the LED trigger
>>>>> from the user space. This patch adds a layer to the sysfs tree like:
>>>>>
>>>>> /sys/devices/virtual/sound/ctl-led/mic
>>>>>    + card0
>>>>>    |  + attach
>>>>>    |  + detach
>>>>>    |  ...
>>>>>    + card1
>>>>>       + attach
>>>>>       ...
>>>>>
>>>>> Operations:
>>>>>
>>>>>   attach and detach
>>>>>     - amixer style ID is accepted and easy strings for numid and
>>>>>       simple names
>>>>>   reset
>>>>>     - reset all associated kcontrol entries
>>>>>   list
>>>>>     - list associated kcontrol entries (numid values only)
>>>>>
>>>>> Additional symlinks:
>>>>>
>>>>> /sys/devices/virtual/sound/ctl-led/mic/card0/card ->
>>>>>   /sys/class/sound/card0
>>>>>
>>>>> /sys/class/sound/card0/controlC0/led-mic ->
>>>>>   /sys/devices/virtual/sound/ctl-led/mic/card0
>>>>>
>>>>> Signed-off-by: Jaroslav Kysela <[e-mail]>
>>>>
>>>> Thank you so much for this patch.
>>>>
>>>> I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
>>>>
>>>> And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute
>>>> LED build into the (detachable) USB-keyboard's mute hot-key:
>>>>
>>>> modprobe snd_ctl_led
>>>> echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
>>>> echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
>>>>
>>>> This needs to be replaced by some UCM profile code doing the equivalent of course,
>>>> but for a proof-of-concept test of the kernel API this introduces the above will do.
>>>
>>> IMO, that's the question: how we'll enable this in future.  If the
>>> binding of the control/mute mapping is provided via UCM, it's supposed
>>> to be changeable by each user.  Then the current sysfs permission
>>> doesn't fit.  OTOH, if it's 0666, it's accessible to all users even
>>> remotely, which is worse than the access with the normal sound device
>>> file.  Or if it's supposed to be changed via udev stuff or systemd?
>>> Or is it just for debugging?
>>>
>>> Through a quick glance over the series, I'm fine to take those
>>> patches, but the only concern is the sysfs entries.  Basically, once
>>> when we use sysfs entries, it's set in stone.  So we should be very
>>> clear about our strategy how to deploy the control/mute mapping
>>> regarding using those sysfs entries.
>>>
>>> OTOH, if the interface is thought for debugging or development
>>> purpose, it could be done in debugfs, which we can keep playing in
>>> further development, too.
>>>
>>> And, BTW, the mute LED mode setup doesn't have to be sysfs entries;
>>> we'd need primarily only the flags for inverted LED behavior, and
>>> those are only two, so it could be simply module options.  Then it's
>>> even easier for users to set up than tweaking sysfs entries.
>>
>> The flexibility offered by this new sysfs API is necessary for the ASoC
>> codec drivers, because Mark does not want to have which controls are
>> tied to the LED triggers hard-coded inside the codec drivers.
> 
> The hard-coded mapping itself isn't always bad things, IMO.  Of
> course, it's a question whether to be done in the codec driver in a
> fixed routing.  A machine driver would fit well, instead; i.e. instead
> of the control-access bit flag, just bind statically from the machine
> driver after instantiating the kctl objects like sysfs does.

Yes setting the new LED-access flags from the machine driver(s) would
work too for the 3 devices (spanning 2 machine drivers) on which
I'm trying to get the mute-LED to work, assuming Mark is going to be
ok with that approach. But those are pretty simple devices.

There also is the recently posted Dell privacy stuff which is
also using LED-triggers on a "full-blown" Intel core series laptop,
which use codecs in much more varied ways. And I've the feeling that
we will see more of this stuff coming up and in those cases the extra
flexibility which going through UCM gives us would be good I think.

I believe that that Dell privacy stuff is actually the reason why
Jaroslav started this whole series, right Jaroslav ?

I'm just piggy backing along with my own use-cases which I had
on my wishlist / itches-list for a while now :)

>> So as Jaroslav mentions in his reply, the plan is to have the UCM
>> profiles contain commands to setup the LED triggers to this new
>> sysfs API.
> 
> IIUC, this won't be only UCM but also the combination of udev +
> alsactl + UCM, right?

Right.

> Would other OS can follow a similar pattern?  Let's check that first
> (although I myself think this should be feasible).

With other OS you mean e.g. Android?  Android has device-specific
init-scripts which can either call alsactl or directly do the
echo-s.

Regards,

Hans

Takashi Iwai March 20, 2021, 9:48 a.m. UTC | #9

On Sat, 20 Mar 2021 10:17:57 +0100,
Hans de Goede wrote:
> 
> Hi,
> 
> On 3/20/21 8:41 AM, Takashi Iwai wrote:
> > On Fri, 19 Mar 2021 23:08:33 +0100,
> > Hans de Goede wrote:
> >>
> >> Hi,
> >>
> >> On 3/19/21 6:22 PM, Takashi Iwai wrote:
> >>> On Fri, 19 Mar 2021 17:34:39 +0100,
> >>> Hans de Goede wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> On 3/17/21 6:29 PM, Jaroslav Kysela wrote:
> >>>>> We need to manage the kcontrol entries association for the LED trigger
> >>>>> from the user space. This patch adds a layer to the sysfs tree like:
> >>>>>
> >>>>> /sys/devices/virtual/sound/ctl-led/mic
> >>>>>    + card0
> >>>>>    |  + attach
> >>>>>    |  + detach
> >>>>>    |  ...
> >>>>>    + card1
> >>>>>       + attach
> >>>>>       ...
> >>>>>
> >>>>> Operations:
> >>>>>
> >>>>>   attach and detach
> >>>>>     - amixer style ID is accepted and easy strings for numid and
> >>>>>       simple names
> >>>>>   reset
> >>>>>     - reset all associated kcontrol entries
> >>>>>   list
> >>>>>     - list associated kcontrol entries (numid values only)
> >>>>>
> >>>>> Additional symlinks:
> >>>>>
> >>>>> /sys/devices/virtual/sound/ctl-led/mic/card0/card ->
> >>>>>   /sys/class/sound/card0
> >>>>>
> >>>>> /sys/class/sound/card0/controlC0/led-mic ->
> >>>>>   /sys/devices/virtual/sound/ctl-led/mic/card0
> >>>>>
> >>>>> Signed-off-by: Jaroslav Kysela <[e-mail]>
> >>>>
> >>>> Thank you so much for this patch.
> >>>>
> >>>> I've given this new version a try, dropping my sound/soc/codecs/rt56??.c patches to set the access-flags directly.
> >>>>
> >>>> And with these 3 lines in /etc/rc.d/rc.local I get nicely working control of the mute
> >>>> LED build into the (detachable) USB-keyboard's mute hot-key:
> >>>>
> >>>> modprobe snd_ctl_led
> >>>> echo -n name="Speaker Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
> >>>> echo -n name="HP Channel Switch" > /sys/class/sound/card1/controlC1/led-speaker/attach
> >>>>
> >>>> This needs to be replaced by some UCM profile code doing the equivalent of course,
> >>>> but for a proof-of-concept test of the kernel API this introduces the above will do.
> >>>
> >>> IMO, that's the question: how we'll enable this in future.  If the
> >>> binding of the control/mute mapping is provided via UCM, it's supposed
> >>> to be changeable by each user.  Then the current sysfs permission
> >>> doesn't fit.  OTOH, if it's 0666, it's accessible to all users even
> >>> remotely, which is worse than the access with the normal sound device
> >>> file.  Or if it's supposed to be changed via udev stuff or systemd?
> >>> Or is it just for debugging?
> >>>
> >>> Through a quick glance over the series, I'm fine to take those
> >>> patches, but the only concern is the sysfs entries.  Basically, once
> >>> when we use sysfs entries, it's set in stone.  So we should be very
> >>> clear about our strategy how to deploy the control/mute mapping
> >>> regarding using those sysfs entries.
> >>>
> >>> OTOH, if the interface is thought for debugging or development
> >>> purpose, it could be done in debugfs, which we can keep playing in
> >>> further development, too.
> >>>
> >>> And, BTW, the mute LED mode setup doesn't have to be sysfs entries;
> >>> we'd need primarily only the flags for inverted LED behavior, and
> >>> those are only two, so it could be simply module options.  Then it's
> >>> even easier for users to set up than tweaking sysfs entries.
> >>
> >> The flexibility offered by this new sysfs API is necessary for the ASoC
> >> codec drivers, because Mark does not want to have which controls are
> >> tied to the LED triggers hard-coded inside the codec drivers.
> > 
> > The hard-coded mapping itself isn't always bad things, IMO.  Of
> > course, it's a question whether to be done in the codec driver in a
> > fixed routing.  A machine driver would fit well, instead; i.e. instead
> > of the control-access bit flag, just bind statically from the machine
> > driver after instantiating the kctl objects like sysfs does.
> 
> Yes setting the new LED-access flags from the machine driver(s) would
> work too for the 3 devices (spanning 2 machine drivers) on which
> I'm trying to get the mute-LED to work, assuming Mark is going to be
> ok with that approach. But those are pretty simple devices.
> 
> There also is the recently posted Dell privacy stuff which is
> also using LED-triggers on a "full-blown" Intel core series laptop,
> which use codecs in much more varied ways. And I've the feeling that
> we will see more of this stuff coming up and in those cases the extra
> flexibility which going through UCM gives us would be good I think.
> 
> I believe that that Dell privacy stuff is actually the reason why
> Jaroslav started this whole series, right Jaroslav ?
> 
> I'm just piggy backing along with my own use-cases which I had
> on my wishlist / itches-list for a while now :)
> 
> >> So as Jaroslav mentions in his reply, the plan is to have the UCM
> >> profiles contain commands to setup the LED triggers to this new
> >> sysfs API.
> > 
> > IIUC, this won't be only UCM but also the combination of udev +
> > alsactl + UCM, right?
> 
> Right.
> 
> > Would other OS can follow a similar pattern?  Let's check that first
> > (although I myself think this should be feasible).
> 
> With other OS you mean e.g. Android?  Android has device-specific
> init-scripts which can either call alsactl or directly do the
> echo-s.

Also ChromeOS.  I'd like to get a general consensus before moving
forward.


Takashi

Jaroslav Kysela March 22, 2021, 2:16 p.m. UTC | #10

Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):

>> With other OS you mean e.g. Android?  Android has device-specific
>> init-scripts which can either call alsactl or directly do the
>> echo-s.
> 
> Also ChromeOS.  I'd like to get a general consensus before moving
> forward.

Where are ChromeOS people? They could join to the discussion which is floating
few months now. Perhaps, the gmail's spam filter does not allow them to
communicate with us ;-)

						Jaroslav

Takashi Iwai March 23, 2021, 9:38 a.m. UTC | #11

On Mon, 22 Mar 2021 15:16:30 +0100,
Jaroslav Kysela wrote:
> 
> Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
> 
> >> With other OS you mean e.g. Android?  Android has device-specific
> >> init-scripts which can either call alsactl or directly do the
> >> echo-s.
> > 
> > Also ChromeOS.  I'd like to get a general consensus before moving
> > forward.
> 
> Where are ChromeOS people? They could join to the discussion which is floating
> few months now. Perhaps, the gmail's spam filter does not allow them to
> communicate with us ;-)

Also adding Dylan and Mark to Cc.

FYI, the patch set is:
  https://lore.kernel.org/alsa-devel/[e-mail]/


Takashi

Takashi Iwai March 23, 2021, 9:49 a.m. UTC | #12

On Tue, 23 Mar 2021 10:38:46 +0100,
Takashi Iwai wrote:
> 
> On Mon, 22 Mar 2021 15:16:30 +0100,
> Jaroslav Kysela wrote:
> > 
> > Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
> > 
> > >> With other OS you mean e.g. Android?  Android has device-specific
> > >> init-scripts which can either call alsactl or directly do the
> > >> echo-s.
> > > 
> > > Also ChromeOS.  I'd like to get a general consensus before moving
> > > forward.
> > 
> > Where are ChromeOS people? They could join to the discussion which is floating
> > few months now. Perhaps, the gmail's spam filter does not allow them to
> > communicate with us ;-)
> 
> Also adding Dylan and Mark to Cc.
> 
> FYI, the patch set is:
>   https://lore.kernel.org/alsa-devel/[e-mail]/

... and now back to the topic.

So the primary question is whether we want the sysfs entries to allow
user-space defining the mute-LED vs control binding externally.  With
this, the mute LED is supposed to be set up via udev rules that
triggers some alsactl stuff, and the rest is handled in an extension
in UCM profile.  If this approach is acceptable on all platforms, we
can go for it.  That was the question to other platforms like Android
and ChromeOS.


And, now looking into the details, I have a few more questions:

- The binding with SNDRV_CTL_ELEM_* bit flag is handy for some drivers
  but not for everything; e.g. if we want to add the binding in ASoC
  machine driver, an API like
    snd_ctl_bind_mute_led(card, elem_id, inverted);
  would be easier.  It'd be essentially an internal call of the sysfs
  binding.  (I haven't checked, but might this be also more
  straightforward conversion for HD-audio case, too?)

- The binding in the kernel could (should?) be shown in the sysfs
  output.  Currently it seems handled differently?

- Specifying the numid may the code simpler in kernel side?
  alsactl has already the string parser.

- Do we have to deal with binding with multiple controls to a single
  mute LED?  Might a single exclusive binding make things easier?
  Then we don't have to create sysfs entries per card, and it'll be
  something like
     echo 1:10 > /sys/devices/virtual/sound/ctl-led/mic/bind
  which is equivalent with the API call above.
  If multiple bindings are attempted, it can simply give an error.
  In the driver side, it catches the unexpected binding, too.


thanks,

Takashi

Jaroslav Kysela March 23, 2021, 10:31 a.m. UTC | #13

Dne 23. 03. 21 v 10:49 Takashi Iwai napsal(a):
> On Tue, 23 Mar 2021 10:38:46 +0100,
> Takashi Iwai wrote:
>>
>> On Mon, 22 Mar 2021 15:16:30 +0100,
>> Jaroslav Kysela wrote:
>>>
>>> Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
>>>
>>>>> With other OS you mean e.g. Android?  Android has device-specific
>>>>> init-scripts which can either call alsactl or directly do the
>>>>> echo-s.
>>>>
>>>> Also ChromeOS.  I'd like to get a general consensus before moving
>>>> forward.
>>>
>>> Where are ChromeOS people? They could join to the discussion which is floating
>>> few months now. Perhaps, the gmail's spam filter does not allow them to
>>> communicate with us ;-)
>>
>> Also adding Dylan and Mark to Cc.
>>
>> FYI, the patch set is:
>>   https://lore.kernel.org/alsa-devel/[e-mail]/
> 
> ... and now back to the topic.
> 
> So the primary question is whether we want the sysfs entries to allow
> user-space defining the mute-LED vs control binding externally.  With
> this, the mute LED is supposed to be set up via udev rules that
> triggers some alsactl stuff, and the rest is handled in an extension
> in UCM profile.  If this approach is acceptable on all platforms, we
> can go for it.  That was the question to other platforms like Android
> and ChromeOS.
> 
> 
> And, now looking into the details, I have a few more questions:
> 
> - The binding with SNDRV_CTL_ELEM_* bit flag is handy for some drivers
>   but not for everything; e.g. if we want to add the binding in ASoC
>   machine driver, an API like
>     snd_ctl_bind_mute_led(card, elem_id, inverted);
>   would be easier.  It'd be essentially an internal call of the sysfs
>   binding.

I would probably create more universal helper for the access field. It may be
handy to update other flags like INACTIVE or so. Something like:

  snd_ctl_update_access(card, elem_id, access_mask, access_bits);

If we decide to move this information out of access field, we can replace
those calls with another function.

For ASoC codecs, it may be difficult to do such calls in the init phase,
because the card is not bound to the component. But yes, I agree that this
setting should be handled in the upper layer (machine) than the component layer.

>  (I haven't checked, but might this be also more
>   straightforward conversion for HD-audio case, too?)

I don't think that it brings a simplification. The id composition is more
complex than 'if (codec->led_flag) access |= LED_GROUP'.

> - The binding in the kernel could (should?) be shown in the sysfs
>   output.  Currently it seems handled differently?

It isn't. The LED group is stored in the access field and my implementation
tracks those bits per elements. So, the sysfs LED code updates those bits,
too. The settings is preserved even if you reload the ctl-led module.

> - Specifying the numid may the code simpler in kernel side?
>   alsactl has already the string parser.

Yes, but it's not so handy for scripting / UCM. I can add find-ctl-numid
lookup to UCM, of course, but what about standard shell scripting?

> - Do we have to deal with binding with multiple controls to a single
>   mute LED?  Might a single exclusive binding make things easier?
>   Then we don't have to create sysfs entries per card, and it'll be
>   something like
>      echo 1:10 > /sys/devices/virtual/sound/ctl-led/mic/bind
>   which is equivalent with the API call above.
>   If multiple bindings are attempted, it can simply give an error.
>   In the driver side, it catches the unexpected binding, too.

AMD ACP digital + HDA analog headset microphone. If we follow the standard HDA
behaviour, both inputs should trigger the mic LED. Two cards are in the game.

					Jaroslav

Hans de Goede March 23, 2021, 10:42 a.m. UTC | #14

Hi,

On 3/23/21 11:31 AM, Jaroslav Kysela wrote:
> Dne 23. 03. 21 v 10:49 Takashi Iwai napsal(a):
>> On Tue, 23 Mar 2021 10:38:46 +0100,
>> Takashi Iwai wrote:
>>>
>>> On Mon, 22 Mar 2021 15:16:30 +0100,
>>> Jaroslav Kysela wrote:
>>>>
>>>> Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
>>>>
>>>>>> With other OS you mean e.g. Android?  Android has device-specific
>>>>>> init-scripts which can either call alsactl or directly do the
>>>>>> echo-s.
>>>>>
>>>>> Also ChromeOS.  I'd like to get a general consensus before moving
>>>>> forward.
>>>>
>>>> Where are ChromeOS people? They could join to the discussion which is floating
>>>> few months now. Perhaps, the gmail's spam filter does not allow them to
>>>> communicate with us ;-)
>>>
>>> Also adding Dylan and Mark to Cc.
>>>
>>> FYI, the patch set is:
>>>   https://lore.kernel.org/alsa-devel/[e-mail]/
>>
>> ... and now back to the topic.
>>
>> So the primary question is whether we want the sysfs entries to allow
>> user-space defining the mute-LED vs control binding externally.  With
>> this, the mute LED is supposed to be set up via udev rules that
>> triggers some alsactl stuff, and the rest is handled in an extension
>> in UCM profile.  If this approach is acceptable on all platforms, we
>> can go for it.  That was the question to other platforms like Android
>> and ChromeOS.
>>
>>
>> And, now looking into the details, I have a few more questions:
>>
>> - The binding with SNDRV_CTL_ELEM_* bit flag is handy for some drivers
>>   but not for everything; e.g. if we want to add the binding in ASoC
>>   machine driver, an API like
>>     snd_ctl_bind_mute_led(card, elem_id, inverted);
>>   would be easier.  It'd be essentially an internal call of the sysfs
>>   binding.
> 
> I would probably create more universal helper for the access field. It may be
> handy to update other flags like INACTIVE or so. Something like:
> 
>   snd_ctl_update_access(card, elem_id, access_mask, access_bits);

For the ASoC machine drivers this functions would ideally take an element-name
not the numeric id, because the machine-driver has no idea of the ids and
the ids are not really stable (they may change when e.g. a new mixer
element is added to the codec).

> 
> If we decide to move this information out of access field, we can replace
> those calls with another function.
> 
> For ASoC codecs, it may be difficult to do such calls in the init phase,
> because the card is not bound to the component. But yes, I agree that this
> setting should be handled in the upper layer (machine) than the component layer.
> 
>>  (I haven't checked, but might this be also more
>>   straightforward conversion for HD-audio case, too?)
> 
> I don't think that it brings a simplification. The id composition is more
> complex than 'if (codec->led_flag) access |= LED_GROUP'.
> 
>> - The binding in the kernel could (should?) be shown in the sysfs
>>   output.  Currently it seems handled differently?
> 
> It isn't. The LED group is stored in the access field and my implementation
> tracks those bits per elements. So, the sysfs LED code updates those bits,
> too. The settings is preserved even if you reload the ctl-led module.
> 
>> - Specifying the numid may the code simpler in kernel side?
>>   alsactl has already the string parser.
> 
> Yes, but it's not so handy for scripting / UCM. I can add find-ctl-numid
> lookup to UCM, of course, but what about standard shell scripting?

I would prefer for the sysfs API to accept element-names too, as
I mentioned above that would even be better for in kernel use, let
alone for a userspace API.

Regards,

Hans

Takashi Iwai March 23, 2021, 10:50 a.m. UTC | #15

On Tue, 23 Mar 2021 11:31:30 +0100,
Jaroslav Kysela wrote:
> 
> Dne 23. 03. 21 v 10:49 Takashi Iwai napsal(a):
> > On Tue, 23 Mar 2021 10:38:46 +0100,
> > Takashi Iwai wrote:
> >>
> >> On Mon, 22 Mar 2021 15:16:30 +0100,
> >> Jaroslav Kysela wrote:
> >>>
> >>> Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
> >>>
> >>>>> With other OS you mean e.g. Android?  Android has device-specific
> >>>>> init-scripts which can either call alsactl or directly do the
> >>>>> echo-s.
> >>>>
> >>>> Also ChromeOS.  I'd like to get a general consensus before moving
> >>>> forward.
> >>>
> >>> Where are ChromeOS people? They could join to the discussion which is floating
> >>> few months now. Perhaps, the gmail's spam filter does not allow them to
> >>> communicate with us ;-)
> >>
> >> Also adding Dylan and Mark to Cc.
> >>
> >> FYI, the patch set is:
> >>   https://lore.kernel.org/alsa-devel/[e-mail]/
> > 
> > ... and now back to the topic.
> > 
> > So the primary question is whether we want the sysfs entries to allow
> > user-space defining the mute-LED vs control binding externally.  With
> > this, the mute LED is supposed to be set up via udev rules that
> > triggers some alsactl stuff, and the rest is handled in an extension
> > in UCM profile.  If this approach is acceptable on all platforms, we
> > can go for it.  That was the question to other platforms like Android
> > and ChromeOS.
> > 
> > 
> > And, now looking into the details, I have a few more questions:
> > 
> > - The binding with SNDRV_CTL_ELEM_* bit flag is handy for some drivers
> >   but not for everything; e.g. if we want to add the binding in ASoC
> >   machine driver, an API like
> >     snd_ctl_bind_mute_led(card, elem_id, inverted);
> >   would be easier.  It'd be essentially an internal call of the sysfs
> >   binding.
> 
> I would probably create more universal helper for the access field. It may be
> handy to update other flags like INACTIVE or so. Something like:
> 
>   snd_ctl_update_access(card, elem_id, access_mask, access_bits);
> 
> If we decide to move this information out of access field, we can replace
> those calls with another function.
> 
> For ASoC codecs, it may be difficult to do such calls in the init phase,
> because the card is not bound to the component. But yes, I agree that this
> setting should be handled in the upper layer (machine) than the component layer.
> 
> >  (I haven't checked, but might this be also more
> >   straightforward conversion for HD-audio case, too?)
> 
> I don't think that it brings a simplification. The id composition is more
> complex than 'if (codec->led_flag) access |= LED_GROUP'.

I guess it'll simply replace the existing call of
snd_hda_add_vmaster_hook() with snd_ctl_update_something().
But it's a minor thing and can be refactored later.

> > - The binding in the kernel could (should?) be shown in the sysfs
> >   output.  Currently it seems handled differently?
> 
> It isn't. The LED group is stored in the access field and my implementation
> tracks those bits per elements. So, the sysfs LED code updates those bits,
> too. The settings is preserved even if you reload the ctl-led module.
> 
> > - Specifying the numid may the code simpler in kernel side?
> >   alsactl has already the string parser.
> 
> Yes, but it's not so handy for scripting / UCM. I can add find-ctl-numid
> lookup to UCM, of course, but what about standard shell scripting?

Hmm, would UCM itself touch the sysfs entry?  That sounds a bit awful.

The simpler implementation in the kernel side is always nicer, but of
course only if it works sufficiently.  So it depends on how much we
want to support this feature.  The parse of control name can be done
by scripting, but it's cumbersome for now, indeed, so if the shell
scripting is seen as the major usage, it'd be more convenient if the
kernel parses the string, yeah.

> > - Do we have to deal with binding with multiple controls to a single
> >   mute LED?  Might a single exclusive binding make things easier?
> >   Then we don't have to create sysfs entries per card, and it'll be
> >   something like
> >      echo 1:10 > /sys/devices/virtual/sound/ctl-led/mic/bind
> >   which is equivalent with the API call above.
> >   If multiple bindings are attempted, it can simply give an error.
> >   In the driver side, it catches the unexpected binding, too.
> 
> AMD ACP digital + HDA analog headset microphone. If we follow the standard HDA
> behaviour, both inputs should trigger the mic LED. Two cards are in the game.

And that brings yet another question.  If the Dell privacy thing comes
to play here, for example, the mute LED is tied with the hardware
control of the built-in mic.  Then do we influence on this depending
on the headset mic mute state, too?


thanks,

Takashi

Takashi Iwai March 23, 2021, 11:03 a.m. UTC | #16

On Tue, 23 Mar 2021 11:42:26 +0100,
Hans de Goede wrote:
> 
> Hi,
> 
> On 3/23/21 11:31 AM, Jaroslav Kysela wrote:
> > Dne 23. 03. 21 v 10:49 Takashi Iwai napsal(a):
> >> On Tue, 23 Mar 2021 10:38:46 +0100,
> >> Takashi Iwai wrote:
> >>>
> >>> On Mon, 22 Mar 2021 15:16:30 +0100,
> >>> Jaroslav Kysela wrote:
> >>>>
> >>>> Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
> >>>>
> >>>>>> With other OS you mean e.g. Android?  Android has device-specific
> >>>>>> init-scripts which can either call alsactl or directly do the
> >>>>>> echo-s.
> >>>>>
> >>>>> Also ChromeOS.  I'd like to get a general consensus before moving
> >>>>> forward.
> >>>>
> >>>> Where are ChromeOS people? They could join to the discussion which is floating
> >>>> few months now. Perhaps, the gmail's spam filter does not allow them to
> >>>> communicate with us ;-)
> >>>
> >>> Also adding Dylan and Mark to Cc.
> >>>
> >>> FYI, the patch set is:
> >>>   https://lore.kernel.org/alsa-devel/[e-mail]/
> >>
> >> ... and now back to the topic.
> >>
> >> So the primary question is whether we want the sysfs entries to allow
> >> user-space defining the mute-LED vs control binding externally.  With
> >> this, the mute LED is supposed to be set up via udev rules that
> >> triggers some alsactl stuff, and the rest is handled in an extension
> >> in UCM profile.  If this approach is acceptable on all platforms, we
> >> can go for it.  That was the question to other platforms like Android
> >> and ChromeOS.
> >>
> >>
> >> And, now looking into the details, I have a few more questions:
> >>
> >> - The binding with SNDRV_CTL_ELEM_* bit flag is handy for some drivers
> >>   but not for everything; e.g. if we want to add the binding in ASoC
> >>   machine driver, an API like
> >>     snd_ctl_bind_mute_led(card, elem_id, inverted);
> >>   would be easier.  It'd be essentially an internal call of the sysfs
> >>   binding.
> > 
> > I would probably create more universal helper for the access field. It may be
> > handy to update other flags like INACTIVE or so. Something like:
> > 
> >   snd_ctl_update_access(card, elem_id, access_mask, access_bits);
> 
> For the ASoC machine drivers this functions would ideally take an element-name
> not the numeric id, because the machine-driver has no idea of the ids and
> the ids are not really stable (they may change when e.g. a new mixer
> element is added to the codec).

In the kernel side, what we need is rather a simple helper function
like snd_ctl_find_elem(card, iface, name, index) that returns kcontrol
object.  A similar code has been already implemented everywhere, so
it'd make sense to have a common helper instead.

> > If we decide to move this information out of access field, we can replace
> > those calls with another function.
> > 
> > For ASoC codecs, it may be difficult to do such calls in the init phase,
> > because the card is not bound to the component. But yes, I agree that this
> > setting should be handled in the upper layer (machine) than the component layer.
> > 
> >>  (I haven't checked, but might this be also more
> >>   straightforward conversion for HD-audio case, too?)
> > 
> > I don't think that it brings a simplification. The id composition is more
> > complex than 'if (codec->led_flag) access |= LED_GROUP'.
> > 
> >> - The binding in the kernel could (should?) be shown in the sysfs
> >>   output.  Currently it seems handled differently?
> > 
> > It isn't. The LED group is stored in the access field and my implementation
> > tracks those bits per elements. So, the sysfs LED code updates those bits,
> > too. The settings is preserved even if you reload the ctl-led module.
> > 
> >> - Specifying the numid may the code simpler in kernel side?
> >>   alsactl has already the string parser.
> > 
> > Yes, but it's not so handy for scripting / UCM. I can add find-ctl-numid
> > lookup to UCM, of course, but what about standard shell scripting?
> 
> I would prefer for the sysfs API to accept element-names too, as
> I mentioned above that would even be better for in kernel use, let
> alone for a userspace API.

In the kernel side API, we don't need the string parser for the
(iface, name, index) tuple.  So, the only question is whether the
string parsing is the mandatory for the sysfs interface.

I'm not entirely objecting to it; such a parser could be used in other
places generically, too.  But, looking at the current "list" output,
it shows also only numid, so from the symmetry POV, using the numid
for binding would make sense, too.


thanks,

Takashi

Jaroslav Kysela March 23, 2021, 11:13 a.m. UTC | #17

Dne 23. 03. 21 v 11:50 Takashi Iwai napsal(a):

>>> - Specifying the numid may the code simpler in kernel side?
>>>   alsactl has already the string parser.
>>
>> Yes, but it's not so handy for scripting / UCM. I can add find-ctl-numid
>> lookup to UCM, of course, but what about standard shell scripting?
> 
> Hmm, would UCM itself touch the sysfs entry?  That sounds a bit awful.

I already described that with UCM, the boot initialization sequences can be
put in the top-level UCM configuration file to replace the standard (legacy)
alsactl initialization, because ASoC does not use the standard control names
(so the lagacy init does not work). Those boot sequences are supposed to run
at boot / card initialization phase only. This sysfs setup should be placed
only to those sections. The motivation is to have the card configuration in
the one place.

> The simpler implementation in the kernel side is always nicer, but of
> course only if it works sufficiently.  So it depends on how much we
> want to support this feature.  The parse of control name can be done
> by scripting, but it's cumbersome for now, indeed, so if the shell
> scripting is seen as the major usage, it'd be more convenient if the
> kernel parses the string, yeah.
> 
>>> - Do we have to deal with binding with multiple controls to a single
>>>   mute LED?  Might a single exclusive binding make things easier?
>>>   Then we don't have to create sysfs entries per card, and it'll be
>>>   something like
>>>      echo 1:10 > /sys/devices/virtual/sound/ctl-led/mic/bind
>>>   which is equivalent with the API call above.
>>>   If multiple bindings are attempted, it can simply give an error.
>>>   In the driver side, it catches the unexpected binding, too.
>>
>> AMD ACP digital + HDA analog headset microphone. If we follow the standard HDA
>> behaviour, both inputs should trigger the mic LED. Two cards are in the game.
> 
> And that brings yet another question.  If the Dell privacy thing comes
> to play here, for example, the mute LED is tied with the hardware
> control of the built-in mic.  Then do we influence on this depending
> on the headset mic mute state, too?

What users expect? I think that both scenarios are valid, thus we should allow
them.

					Jaroslav

Takashi Iwai March 23, 2021, 11:34 a.m. UTC | #18

On Tue, 23 Mar 2021 12:13:12 +0100,
Jaroslav Kysela wrote:
> 
> Dne 23. 03. 21 v 11:50 Takashi Iwai napsal(a):
> 
> >>> - Specifying the numid may the code simpler in kernel side?
> >>>   alsactl has already the string parser.
> >>
> >> Yes, but it's not so handy for scripting / UCM. I can add find-ctl-numid
> >> lookup to UCM, of course, but what about standard shell scripting?
> > 
> > Hmm, would UCM itself touch the sysfs entry?  That sounds a bit awful.
> 
> I already described that with UCM, the boot initialization sequences can be
> put in the top-level UCM configuration file to replace the standard (legacy)
> alsactl initialization, because ASoC does not use the standard control names
> (so the lagacy init does not work). Those boot sequences are supposed to run
> at boot / card initialization phase only. This sysfs setup should be placed
> only to those sections. The motivation is to have the card configuration in
> the one place.
> 
> > The simpler implementation in the kernel side is always nicer, but of
> > course only if it works sufficiently.  So it depends on how much we
> > want to support this feature.  The parse of control name can be done
> > by scripting, but it's cumbersome for now, indeed, so if the shell
> > scripting is seen as the major usage, it'd be more convenient if the
> > kernel parses the string, yeah.
> > 
> >>> - Do we have to deal with binding with multiple controls to a single
> >>>   mute LED?  Might a single exclusive binding make things easier?
> >>>   Then we don't have to create sysfs entries per card, and it'll be
> >>>   something like
> >>>      echo 1:10 > /sys/devices/virtual/sound/ctl-led/mic/bind
> >>>   which is equivalent with the API call above.
> >>>   If multiple bindings are attempted, it can simply give an error.
> >>>   In the driver side, it catches the unexpected binding, too.
> >>
> >> AMD ACP digital + HDA analog headset microphone. If we follow the standard HDA
> >> behaviour, both inputs should trigger the mic LED. Two cards are in the game.
> > 
> > And that brings yet another question.  If the Dell privacy thing comes
> > to play here, for example, the mute LED is tied with the hardware
> > control of the built-in mic.  Then do we influence on this depending
> > on the headset mic mute state, too?
> 
> What users expect? I think that both scenarios are valid, thus we should allow
> them.

IMO, this is a hard part.  It's possible that user (or the system)
wants two different scenarios:
- LED indicates the built-in mic mute
- LED indicates the mute state of the currently used input

The current code assumes the latter case, and that might conflict with
the concept of Dell privacy stuff (as the built-in mic is still
allowed while using the headset).

How would be a good way to switch to a different scenario?


thanks,

Takashi

Jaroslav Kysela March 23, 2021, 12:22 p.m. UTC | #19

Dne 23. 03. 21 v 12:34 Takashi Iwai napsal(a):

>>> The simpler implementation in the kernel side is always nicer, but of
>>> course only if it works sufficiently.  So it depends on how much we
>>> want to support this feature.  The parse of control name can be done
>>> by scripting, but it's cumbersome for now, indeed, so if the shell
>>> scripting is seen as the major usage, it'd be more convenient if the
>>> kernel parses the string, yeah.
>>>
>>>>> - Do we have to deal with binding with multiple controls to a single
>>>>>   mute LED?  Might a single exclusive binding make things easier?
>>>>>   Then we don't have to create sysfs entries per card, and it'll be
>>>>>   something like
>>>>>      echo 1:10 > /sys/devices/virtual/sound/ctl-led/mic/bind
>>>>>   which is equivalent with the API call above.
>>>>>   If multiple bindings are attempted, it can simply give an error.
>>>>>   In the driver side, it catches the unexpected binding, too.
>>>>
>>>> AMD ACP digital + HDA analog headset microphone. If we follow the standard HDA
>>>> behaviour, both inputs should trigger the mic LED. Two cards are in the game.
>>>
>>> And that brings yet another question.  If the Dell privacy thing comes
>>> to play here, for example, the mute LED is tied with the hardware
>>> control of the built-in mic.  Then do we influence on this depending
>>> on the headset mic mute state, too?
>>
>> What users expect? I think that both scenarios are valid, thus we should allow
>> them.
> 
> IMO, this is a hard part.  It's possible that user (or the system)
> wants two different scenarios:
> - LED indicates the built-in mic mute
> - LED indicates the mute state of the currently used input
> 
> The current code assumes the latter case, and that might conflict with
> the concept of Dell privacy stuff (as the built-in mic is still
> allowed while using the headset).
> 
> How would be a good way to switch to a different scenario?

[Adding Perry /Dell/ to the discussion]

It's an user space setup. We can manage some conditional settings in UCM and
the shell scripts can be written with conditional parts. Perhaps, a global
configuration file(s) in /etc/alsa may specify the requested scenario.

I would just start with a default behavior (which may be hw specific) and
refine this later.

					Jaroslav

Curtis Malainey March 23, 2021, 9:39 p.m. UTC | #20

On Mon, Mar 22, 2021 at 7:16 AM Jaroslav Kysela <[e-mail]> wrote:

> Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
>
> >> With other OS you mean e.g. Android?  Android has device-specific
> >> init-scripts which can either call alsactl or directly do the
> >> echo-s.
> >
> > Also ChromeOS.  I'd like to get a general consensus before moving
> > forward.
>
> Where are ChromeOS people? They could join to the discussion which is
> floating
> few months now. Perhaps, the gmail's spam filter does not allow them to
> communicate with us ;-)
>
> Hi Sorry, i missed this was directly to dgreid and me. Will try to get up
to speed on this.

Curtis

>                                                 Jaroslav
>
> --
> Jaroslav Kysela <[e-mail]>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
>

Dylan Reid March 23, 2021, 10:49 p.m. UTC | #21

On Tue, Mar 23, 2021 at 2:40 PM Curtis Malainey <[e-mail]>
wrote:

> On Mon, Mar 22, 2021 at 7:16 AM Jaroslav Kysela <[e-mail]> wrote:
>
> > Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
> >
> > >> With other OS you mean e.g. Android?  Android has device-specific
> > >> init-scripts which can either call alsactl or directly do the
> > >> echo-s.
> > >
> > > Also ChromeOS.  I'd like to get a general consensus before moving
> > > forward.
> >
> > Where are ChromeOS people? They could join to the discussion which is
> > floating
> > few months now. Perhaps, the gmail's spam filter does not allow them to
> > communicate with us ;-)
> >
> > Hi Sorry, i missed this was directly to dgreid and me. Will try to get up
> to speed on this.
>

Sorry, this one wasn't gmail's fault, it was my manual filtering of emails
about LEDs:)

Chrome OS is supportive of user space control when possible. We will work
with partners to establish a standard in Chrome OS for mute LED meaning
(built in, headset, usb, etc). Having user space control allows different
ecosystems to make different policy decisions. For us, the UCM-specified
mute on/off will be driven exclusively by the audio daemon.


>
> Curtis
>
> >                                                 Jaroslav
> >
> > --
> > Jaroslav Kysela <[e-mail]>
> > Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
> >
>

Takashi Iwai March 26, 2021, 8:07 a.m. UTC | #22

On Tue, 23 Mar 2021 23:49:40 +0100,
Dylan Reid wrote:
> 
> On Tue, Mar 23, 2021 at 2:40 PM Curtis Malainey <[e-mail]>
> wrote:
> 
>     On Mon, Mar 22, 2021 at 7:16 AM Jaroslav Kysela <[e-mail]> wrote:
>    
>     > Dne 20. 03. 21 v 10:48 Takashi Iwai napsal(a):
>     >
>     > >> With other OS you mean e.g. Android?  Android has device-specific
>     > >> init-scripts which can either call alsactl or directly do the
>     > >> echo-s.
>     > >
>     > > Also ChromeOS.  I'd like to get a general consensus before moving
>     > > forward.
>     >
>     > Where are ChromeOS people? They could join to the discussion which is
>     > floating
>     > few months now. Perhaps, the gmail's spam filter does not allow them to
>     > communicate with us ;-)
>     >
>     > Hi Sorry, i missed this was directly to dgreid and me. Will try to get
>     up
>     to speed on this.
> 
> Sorry, this one wasn't gmail's fault, it was my manual filtering of emails
> about LEDs:)
> 
> Chrome OS is supportive of user space control when possible. We will work with
> partners to establish a standard in Chrome OS for mute LED meaning (built in,
> headset, usb, etc). Having user space control allows different ecosystems to
> make different policy decisions. For us, the UCM-specified mute on/off will be
> driven exclusively by the audio daemon.

OK, thanks for a green signal.  So there doesn't seem any big concerns
about the implementation, so far.

Just to be sure, Mark, do you see any possible issues for Android and
other embedded deployment in this approach (sysfs + some init stuff +
UCM)?


Takashi

Patch

diff --git a/sound/core/control_led.c b/sound/core/control_led.c
index dfa51d8461e1..d4fb8b873f34 100644
--- a/sound/core/control_led.c
+++ b/sound/core/control_led.c
@@ -24,6 +24,12 @@  enum snd_ctl_led_mode {
 	 MODE_ON,
 };
 
+struct snd_ctl_led_card {
+	struct device dev;
+	int number;
+	struct snd_ctl_led *led;
+};
+
 struct snd_ctl_led {
 	struct device dev;
 	struct list_head controls;
@@ -31,6 +37,7 @@  struct snd_ctl_led {
 	unsigned int group;
 	enum led_audio trigger_type;
 	enum snd_ctl_led_mode mode;
+	struct snd_ctl_led_card *cards[SNDRV_CARDS];
 };
 
 struct snd_ctl_led_ctl {
@@ -58,6 +65,9 @@  static struct snd_ctl_led snd_ctl_leds[MAX_LED] = {
 	},
 };
 
+static void snd_ctl_led_sysfs_add(struct snd_card *card);
+static void snd_ctl_led_sysfs_remove(struct snd_card *card);
+
 #define UPDATE_ROUTE(route, cb) \
 	do { \
 		int route2 = (cb); \
@@ -222,6 +232,46 @@  static void snd_ctl_led_notify(struct snd_card *card, unsigned int mask,
 	}
 }
 
+static int snd_ctl_led_set_id(int card_number, struct snd_ctl_elem_id *id,
+			      unsigned int group, bool set)
+{
+	struct snd_card *card;
+	struct snd_kcontrol *kctl;
+	struct snd_kcontrol_volatile *vd;
+	unsigned int ioff, access, new_access;
+	int err = 0;
+
+	card = snd_card_ref(card_number);
+	if (card) {
+		down_write(&card->controls_rwsem);
+		kctl = snd_ctl_find_id(card, id);
+		if (kctl) {
+			ioff = snd_ctl_get_ioff(kctl, id);
+			vd = &kctl->vd[ioff];
+			access = vd->access & SNDRV_CTL_ELEM_ACCESS_LED_MASK;
+			if (access != 0 && access != group_to_access(group)) {
+				err = -EXDEV;
+				goto unlock;
+			}
+			new_access = vd->access & ~SNDRV_CTL_ELEM_ACCESS_LED_MASK;
+			if (set)
+				new_access |= group_to_access(group);
+			if (new_access != vd->access) {
+				vd->access = new_access;
+				snd_ctl_led_notify(card, SNDRV_CTL_EVENT_MASK_INFO, kctl, ioff);
+			}
+		} else {
+			err = -ENOENT;
+		}
+unlock:
+		up_write(&card->controls_rwsem);
+		snd_card_unref(card);
+	} else {
+		err = -ENXIO;
+	}
+	return err;
+}
+
 static void snd_ctl_led_refresh(void)
 {
 	unsigned int group;
@@ -230,6 +280,12 @@  static void snd_ctl_led_refresh(void)
 		snd_ctl_led_set_state(NULL, group_to_access(group), NULL, 0);
 }
 
+static void snd_ctl_led_ctl_destroy(struct snd_ctl_led_ctl *lctl)
+{
+	list_del(&lctl->list);
+	kfree(lctl);
+}
+
 static void snd_ctl_led_clean(struct snd_card *card)
 {
 	unsigned int group;
@@ -241,13 +297,47 @@  static void snd_ctl_led_clean(struct snd_card *card)
 repeat:
 		list_for_each_entry(lctl, &led->controls, list)
 			if (!card || lctl->card == card) {
-				list_del(&lctl->list);
-				kfree(lctl);
+				snd_ctl_led_ctl_destroy(lctl);
 				goto repeat;
 			}
 	}
 }
 
+static int snd_ctl_led_reset(int card_number, unsigned int group)
+{
+	struct snd_card *card;
+	struct snd_ctl_led *led;
+	struct snd_ctl_led_ctl *lctl;
+	struct snd_kcontrol_volatile *vd;
+	bool change = false;
+
+	card = snd_card_ref(card_number);
+	if (!card)
+		return -ENXIO;
+
+	mutex_lock(&snd_ctl_led_mutex);
+	if (!snd_ctl_led_card_valid[card_number]) {
+		mutex_unlock(&snd_ctl_led_mutex);
+		snd_card_unref(card);
+		return -ENXIO;
+	}
+	led = &snd_ctl_leds[group];
+repeat:
+	list_for_each_entry(lctl, &led->controls, list)
+		if (lctl->card == card) {
+			vd = &lctl->kctl->vd[lctl->index_offset];
+			vd->access &= ~group_to_access(group);
+			snd_ctl_led_ctl_destroy(lctl);
+			change = true;
+			goto repeat;
+		}
+	mutex_unlock(&snd_ctl_led_mutex);
+	if (change)
+		snd_ctl_led_set_state(NULL, group_to_access(group), NULL, 0);
+	snd_card_unref(card);
+	return 0;
+}
+
 static void snd_ctl_led_register(struct snd_card *card)
 {
 	struct snd_kcontrol *kctl;
@@ -264,10 +354,12 @@  static void snd_ctl_led_register(struct snd_card *card)
 		for (ioff = 0; ioff < kctl->count; ioff++)
 			snd_ctl_led_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, kctl, ioff);
 	snd_ctl_led_refresh();
+	snd_ctl_led_sysfs_add(card);
 }
 
 static void snd_ctl_led_disconnect(struct snd_card *card)
 {
+	snd_ctl_led_sysfs_remove(card);
 	mutex_lock(&snd_ctl_led_mutex);
 	snd_ctl_led_card_valid[card->number] = false;
 	snd_ctl_led_clean(card);
@@ -349,8 +441,264 @@  static const struct attribute_group *snd_ctl_led_dev_attr_groups[] = {
 	NULL,
 };
 
+static char *find_eos(char *s)
+{
+	while (*s && *s != ',')
+		s++;
+	if (*s)
+		s++;
+	return s;
+}
+
+static char *parse_uint(char *s, unsigned int *val)
+{
+	unsigned long long res;
+	if (kstrtoull(s, 10, &res))
+		res = 0;
+	*val = res;
+	return find_eos(s);
+}
+
+static char *parse_string(char *s, char *val, size_t val_size)
+{
+	if (*s == '"' || *s == '\'') {
+		char c = *s;
+		s++;
+		while (*s && *s != c) {
+			if (val_size > 1) {
+				*val++ = *s;
+				val_size--;
+			}
+			s++;
+		}
+	} else {
+		while (*s && *s != ',') {
+			if (val_size > 1) {
+				*val++ = *s;
+				val_size--;
+			}
+			s++;
+		}
+	}
+	*val = '\0';
+	if (*s)
+		s++;
+	return s;
+}
+
+static char *parse_iface(char *s, unsigned int *val)
+{
+	if (!strncasecmp(s, "card", 4))
+		*val = SNDRV_CTL_ELEM_IFACE_CARD;
+	else if (!strncasecmp(s, "mixer", 5))
+		*val = SNDRV_CTL_ELEM_IFACE_MIXER;
+	return find_eos(s);
+}
+
+/*
+ * These types of input strings are accepted:
+ *
+ *   unsigned integer - numid (equivaled to numid=UINT)
+ *   string - basic mixer name (equivalent to iface=MIXER,name=STR)
+ *   numid=UINT
+ *   [iface=MIXER,][device=UINT,][subdevice=UINT,]name=STR[,index=UINT]
+ */
+static ssize_t set_led_id(struct snd_ctl_led_card *led_card, const char *buf, size_t count,
+			  bool attach)
+{
+	char buf2[256], *s;
+	size_t len = max(sizeof(s) - 1, count);
+	struct snd_ctl_elem_id id;
+	int err;
+
+	strncpy(buf2, buf, len);
+	buf2[len] = '\0';
+	memset(&id, 0, sizeof(id));
+	id.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
+	s = buf2;
+	while (*s) {
+		if (!strncasecmp(s, "numid=", 6)) {
+			s = parse_uint(s + 6, &id.numid);
+		} else if (!strncasecmp(s, "iface=", 6)) {
+			s = parse_iface(s + 6, &id.iface);
+		} else if (!strncasecmp(s, "device=", 7)) {
+			s = parse_uint(s + 7, &id.device);
+		} else if (!strncasecmp(s, "subdevice=", 10)) {
+			s = parse_uint(s + 10, &id.subdevice);
+		} else if (!strncasecmp(s, "name=", 5)) {
+			s = parse_string(s + 5, id.name, sizeof(id.name));
+		} else if (!strncasecmp(s, "index=", 6)) {
+			s = parse_uint(s + 6, &id.index);
+		} else if (s == buf2) {
+			while (*s) {
+				if (*s < '0' || *s > '9')
+					break;
+				s++;
+			}
+			if (*s == '\0')
+				parse_uint(buf2, &id.numid);
+			else {
+				for (; *s >= ' '; s++);
+				*s = '\0';
+				strlcpy(id.name, buf2, sizeof(id.name));
+			}
+			break;
+		}
+		if (*s == ',')
+			s++;
+	}
+
+	err = snd_ctl_led_set_id(led_card->number, &id, led_card->led->group, attach);
+	if (err < 0)
+		return err;
+
+	return count;
+}
+
+static ssize_t parse_attach(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
+	return set_led_id(led_card, buf, count, true);
+}
+
+static ssize_t parse_detach(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
+	return set_led_id(led_card, buf, count, false);
+}
+
+static ssize_t ctl_reset(struct device *dev, struct device_attribute *attr,
+			 const char *buf, size_t count)
+{
+	struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
+	int err;
+
+	if (count > 0 && buf[0] == '1') {
+		err = snd_ctl_led_reset(led_card->number, led_card->led->group);
+		if (err < 0)
+			return err;
+	}
+	return count;
+}
+
+static ssize_t ctl_list(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct snd_ctl_led_card *led_card = container_of(dev, struct snd_ctl_led_card, dev);
+	struct snd_card *card;
+	struct snd_ctl_led_ctl *lctl;
+	char *buf2 = buf;
+	size_t l;
+
+	card = snd_card_ref(led_card->number);
+	if (!card)
+		return -ENXIO;
+	down_read(&card->controls_rwsem);
+	mutex_lock(&snd_ctl_led_mutex);
+	if (snd_ctl_led_card_valid[led_card->number]) {
+		list_for_each_entry(lctl, &led_card->led->controls, list)
+			if (lctl->card == card) {
+				if (buf2 - buf > PAGE_SIZE - 16)
+					break;
+				if (buf2 != buf)
+					*buf2++ = ' ';
+				l = scnprintf(buf2, 15, "%u",
+						lctl->kctl->id.numid +
+							lctl->index_offset);
+				buf2[l] = '\0';
+				buf2 += l + 1;
+			}
+	}
+	mutex_unlock(&snd_ctl_led_mutex);
+	up_read(&card->controls_rwsem);
+	snd_card_unref(card);
+	return buf2 - buf;
+}
+
+static DEVICE_ATTR(attach, 0200, NULL, parse_attach);
+static DEVICE_ATTR(detach, 0200, NULL, parse_detach);
+static DEVICE_ATTR(reset, 0200, NULL, ctl_reset);
+static DEVICE_ATTR(list, 0444, ctl_list, NULL);
+
+static struct attribute *snd_ctl_led_card_attrs[] = {
+	&dev_attr_attach.attr,
+	&dev_attr_detach.attr,
+	&dev_attr_reset.attr,
+	&dev_attr_list.attr,
+	NULL,
+};
+
+static const struct attribute_group snd_ctl_led_card_attr_group = {
+	.attrs = snd_ctl_led_card_attrs,
+};
+
+static const struct attribute_group *snd_ctl_led_card_attr_groups[] = {
+	&snd_ctl_led_card_attr_group,
+	NULL,
+};
+
 static struct device snd_ctl_led_dev;
 
+static void snd_ctl_led_sysfs_add(struct snd_card *card)
+{
+	unsigned int group;
+	struct snd_ctl_led_card *led_card;
+	struct snd_ctl_led *led;
+	char link_name[32];
+
+	for (group = 0; group < MAX_LED; group++) {
+		led = &snd_ctl_leds[group];
+		led_card = kzalloc(sizeof(*led_card), GFP_KERNEL);
+		if (!led_card)
+			goto cerr2;
+		led_card->number = card->number;
+		led_card->led = led;
+		device_initialize(&led_card->dev);
+		if (dev_set_name(&led_card->dev, "card%d", card->number) < 0)
+			goto cerr;
+		led_card->dev.parent = &led->dev;
+		led_card->dev.groups = snd_ctl_led_card_attr_groups;
+		if (device_add(&led_card->dev))
+			goto cerr;
+		led->cards[card->number] = led_card;
+		snprintf(link_name, sizeof(link_name), "led-%s", led->name);
+		WARN(sysfs_create_link(&card->ctl_dev.kobj, &led_card->dev.kobj, link_name),
+			"can't create symlink to controlC%i device\n", card->number);
+		WARN(sysfs_create_link(&led_card->dev.kobj, &card->card_dev.kobj, "card"),
+			"can't create symlink to card%i\n", card->number);
+
+		continue;
+cerr:
+		put_device(&led_card->dev);
+cerr2:
+		printk(KERN_ERR "snd_ctl_led: unable to add card%d", card->number);
+		kfree(led_card);
+	}
+}
+
+static void snd_ctl_led_sysfs_remove(struct snd_card *card)
+{
+	unsigned int group;
+	struct snd_ctl_led_card *led_card;
+	struct snd_ctl_led *led;
+	char link_name[32];
+
+	for (group = 0; group < MAX_LED; group++) {
+		led = &snd_ctl_leds[group];
+		led_card = led->cards[card->number];
+		if (!led_card)
+			continue;
+		snprintf(link_name, sizeof(link_name), "led-%s", led->name);
+		sysfs_remove_link(&card->ctl_dev.kobj, link_name);
+		sysfs_remove_link(&led_card->dev.kobj, "card");
+		device_del(&led_card->dev);
+		kfree(led_card);
+		led->cards[card->number] = NULL;
+	}
+}
+
 /*
  * Control layer registration
  */
@@ -397,14 +745,24 @@  static int __init snd_ctl_led_init(void)
 static void __exit snd_ctl_led_exit(void)
 {
 	struct snd_ctl_led *led;
-	unsigned int group;
+	struct snd_card *card;
+	unsigned int group, card_number;
 
+	snd_ctl_disconnect_layer(&snd_ctl_led_lops);
+	for (card_number = 0; card_number < SNDRV_CARDS; card_number++) {
+		if (!snd_ctl_led_card_valid[card_number])
+			continue;
+		card = snd_card_ref(card_number);
+		if (card) {
+			snd_ctl_led_sysfs_remove(card);
+			snd_card_unref(card);
+		}
+	}
 	for (group = 0; group < MAX_LED; group++) {
 		led = &snd_ctl_leds[group];
 		device_del(&led->dev);
 	}
 	device_del(&snd_ctl_led_dev);
-	snd_ctl_disconnect_layer(&snd_ctl_led_lops);
 	snd_ctl_led_clean(NULL);
 }