Re: [PATCH v4 5/6] ALSA: control - add sysfs support to the LED trigger module

Re: [PATCH v4 5/6] ALSA: control - add sysfs support to the LED trigger module

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2872 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210317172945.842280-6-perex@perex.cz>
References: <20210317172945.842280-6-perex@perex.cz>
TO: Jaroslav Kysela <perex@perex.cz>
TO: ALSA development <alsa-devel@alsa-project.org>
CC: Takashi Iwai <tiwai@suse.de>
CC: Hans de Goede <hdegoede@redhat.com>

Hi Jaroslav,

I love your patch! Perhaps something to improve:

[auto build test WARNING on next-20210317]
[cannot apply to sound/for-next v5.12-rc3 v5.12-rc2 v5.12-rc1 v5.12-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jaroslav-Kysela/ALSA-control-add-generic-LED-API/20210318-013608
base:    fa903833ae344e4f4d798a8b85ba3ef0c5ce96c9
:::::: branch date: 3 hours ago
:::::: commit date: 3 hours ago
config: x86_64-randconfig-m001-20210317 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
sound/core/control_led.c:294 show_mode() error: uninitialized symbol 'str'.

vim +/str +294 sound/core/control_led.c

086bb76b418eff Jaroslav Kysela 2021-03-17  277  
94a1a03a2f377e Jaroslav Kysela 2021-03-17  278  /*
94a1a03a2f377e Jaroslav Kysela 2021-03-17  279   * sysfs
94a1a03a2f377e Jaroslav Kysela 2021-03-17  280   */
94a1a03a2f377e Jaroslav Kysela 2021-03-17  281  
94a1a03a2f377e Jaroslav Kysela 2021-03-17  282  static ssize_t show_mode(struct device *dev,
94a1a03a2f377e Jaroslav Kysela 2021-03-17  283  			 struct device_attribute *attr, char *buf)
94a1a03a2f377e Jaroslav Kysela 2021-03-17  284  {
94a1a03a2f377e Jaroslav Kysela 2021-03-17  285  	struct snd_ctl_led *led = container_of(dev, struct snd_ctl_led, dev);
94a1a03a2f377e Jaroslav Kysela 2021-03-17  286  	const char *str;
94a1a03a2f377e Jaroslav Kysela 2021-03-17  287  
94a1a03a2f377e Jaroslav Kysela 2021-03-17  288  	switch (led->mode) {
94a1a03a2f377e Jaroslav Kysela 2021-03-17  289  	case MODE_FOLLOW_MUTE:	str = "follow-mute"; break;
94a1a03a2f377e Jaroslav Kysela 2021-03-17  290  	case MODE_FOLLOW_ROUTE:	str = "follow-route"; break;
94a1a03a2f377e Jaroslav Kysela 2021-03-17  291  	case MODE_ON:		str = "on"; break;
94a1a03a2f377e Jaroslav Kysela 2021-03-17  292  	case MODE_OFF:		str = "off"; break;
94a1a03a2f377e Jaroslav Kysela 2021-03-17  293  	}
94a1a03a2f377e Jaroslav Kysela 2021-03-17 @294  	return sprintf(buf, "%s\n", str);
94a1a03a2f377e Jaroslav Kysela 2021-03-17  295  }
94a1a03a2f377e Jaroslav Kysela 2021-03-17  296  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 34752 bytes --]

[PATCH v4 5/6] ALSA: control - add sysfs support to the LED trigger module

[Jaroslav Kysela]

Create SYSFS/devices/virtual/sound/ctl-led tree
(with SYSFS/class/sound/ctl-led symlink).

  speaker/
    +-- mode
    +-- brightness
  mic/
    +-- mode
    +-- brightness

Copy the idea from the HDA driver and allow to set the audio
LEDs based on the various modes:

- follow mute
- follow moute (inverted to follow mute)
- off
- on

Also, the actual LED state is exposed.

Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
 sound/core/control_led.c | 192 +++++++++++++++++++++++++++++++++------
 1 file changed, 163 insertions(+), 29 deletions(-)

diff --git a/sound/core/control_led.c b/sound/core/control_led.c
index 3e2c3c485c5c..dfa51d8461e1 100644
--- a/sound/core/control_led.c
+++ b/sound/core/control_led.c
@@ -17,7 +17,23 @@ MODULE_LICENSE("GPL");
 #define MAX_LED (((SNDRV_CTL_ELEM_ACCESS_MIC_LED - SNDRV_CTL_ELEM_ACCESS_SPK_LED) \
 			>> SNDRV_CTL_ELEM_ACCESS_LED_SHIFT) + 1)
 
+enum snd_ctl_led_mode {
+	 MODE_FOLLOW_MUTE = 0,
+	 MODE_FOLLOW_ROUTE,
+	 MODE_OFF,
+	 MODE_ON,
+};
+
 struct snd_ctl_led {
+	struct device dev;
+	struct list_head controls;
+	const char *name;
+	unsigned int group;
+	enum led_audio trigger_type;
+	enum snd_ctl_led_mode mode;
+};
+
+struct snd_ctl_led_ctl {
 	struct list_head list;
 	struct snd_card *card;
 	unsigned int access;
@@ -26,8 +42,21 @@ struct snd_ctl_led {
 };
 
 static DEFINE_MUTEX(snd_ctl_led_mutex);
-static struct list_head snd_ctl_led_controls[MAX_LED];
 static bool snd_ctl_led_card_valid[SNDRV_CARDS];
+static struct snd_ctl_led snd_ctl_leds[MAX_LED] = {
+	{
+		.name = "speaker",
+		.group = (SNDRV_CTL_ELEM_ACCESS_SPK_LED >> SNDRV_CTL_ELEM_ACCESS_LED_SHIFT) - 1,
+		.trigger_type = LED_AUDIO_MUTE,
+		.mode = MODE_FOLLOW_MUTE,
+	},
+	{
+		.name = "mic",
+		.group = (SNDRV_CTL_ELEM_ACCESS_MIC_LED >> SNDRV_CTL_ELEM_ACCESS_LED_SHIFT) - 1,
+		.trigger_type = LED_AUDIO_MICMUTE,
+		.mode = MODE_FOLLOW_MUTE,
+	},
+};
 
 #define UPDATE_ROUTE(route, cb) \
 	do { \
@@ -47,15 +76,15 @@ static inline unsigned int group_to_access(unsigned int group)
 	return (group + 1) << SNDRV_CTL_ELEM_ACCESS_LED_SHIFT;
 }
 
-static struct list_head *snd_ctl_led_controls_by_access(unsigned int access)
+static struct snd_ctl_led *snd_ctl_led_get_by_access(unsigned int access)
 {
 	unsigned int group = access_to_group(access);
 	if (group >= MAX_LED)
 		return NULL;
-	return &snd_ctl_led_controls[group];
+	return &snd_ctl_leds[group];
 }
 
-static int snd_ctl_led_get(struct snd_ctl_led *lctl)
+static int snd_ctl_led_get(struct snd_ctl_led_ctl *lctl)
 {
 	struct snd_kcontrol *kctl = lctl->kctl;
 	struct snd_ctl_elem_info info;
@@ -91,22 +120,14 @@ static int snd_ctl_led_get(struct snd_ctl_led *lctl)
 static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
 				  struct snd_kcontrol *kctl, unsigned int ioff)
 {
-	struct list_head *controls;
-	struct snd_ctl_led *lctl;
-	enum led_audio led_trigger_type;
+	struct snd_ctl_led *led;
+	struct snd_ctl_led_ctl *lctl;
 	int route;
 	bool found;
 
-	controls = snd_ctl_led_controls_by_access(access);
-	if (!controls)
+	led = snd_ctl_led_get_by_access(access);
+	if (!led)
 		return;
-	if (access == SNDRV_CTL_ELEM_ACCESS_SPK_LED) {
-		led_trigger_type = LED_AUDIO_MUTE;
-	} else if (access == SNDRV_CTL_ELEM_ACCESS_MIC_LED) {
-		led_trigger_type = LED_AUDIO_MICMUTE;
-	} else {
-		return;
-	}
 	route = -1;
 	found = false;
 	mutex_lock(&snd_ctl_led_mutex);
@@ -115,7 +136,7 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
 		mutex_unlock(&snd_ctl_led_mutex);
 		return;
 	}
-	list_for_each_entry(lctl, controls, list) {
+	list_for_each_entry(lctl, &led->controls, list) {
 		if (lctl->kctl == kctl && lctl->index_offset == ioff)
 			found = true;
 		UPDATE_ROUTE(route, snd_ctl_led_get(lctl));
@@ -127,23 +148,29 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
 			lctl->access = access;
 			lctl->kctl = kctl;
 			lctl->index_offset = ioff;
-			list_add(&lctl->list, controls);
+			list_add(&lctl->list, &led->controls);
 			UPDATE_ROUTE(route, snd_ctl_led_get(lctl));
 		}
 	}
 	mutex_unlock(&snd_ctl_led_mutex);
+	switch (led->mode) {
+	case MODE_OFF:		route = 1; break;
+	case MODE_ON:		route = 0; break;
+	case MODE_FOLLOW_ROUTE:	if (route >= 0) route ^= 1; break;
+	case MODE_FOLLOW_MUTE:	/* noop */ break;
+	}
 	if (route >= 0)
-		ledtrig_audio_set(led_trigger_type, route ? LED_OFF : LED_ON);
+		ledtrig_audio_set(led->trigger_type, route ? LED_OFF : LED_ON);
 }
 
-static struct snd_ctl_led *snd_ctl_led_find(struct snd_kcontrol *kctl, unsigned int ioff)
+static struct snd_ctl_led_ctl *snd_ctl_led_find(struct snd_kcontrol *kctl, unsigned int ioff)
 {
 	struct list_head *controls;
-	struct snd_ctl_led *lctl;
+	struct snd_ctl_led_ctl *lctl;
 	unsigned int group;
 
 	for (group = 0; group < MAX_LED; group++) {
-		controls = &snd_ctl_led_controls[group];
+		controls = &snd_ctl_leds[group].controls;
 		list_for_each_entry(lctl, controls, list)
 			if (lctl->kctl == kctl && lctl->index_offset == ioff)
 				return lctl;
@@ -154,7 +181,7 @@ static struct snd_ctl_led *snd_ctl_led_find(struct snd_kcontrol *kctl, unsigned
 static unsigned int snd_ctl_led_remove(struct snd_kcontrol *kctl, unsigned int ioff,
 				       unsigned int access)
 {
-	struct snd_ctl_led *lctl;
+	struct snd_ctl_led_ctl *lctl;
 	unsigned int ret = 0;
 
 	mutex_lock(&snd_ctl_led_mutex);
@@ -206,13 +233,13 @@ static void snd_ctl_led_refresh(void)
 static void snd_ctl_led_clean(struct snd_card *card)
 {
 	unsigned int group;
-	struct list_head *controls;
-	struct snd_ctl_led *lctl;
+	struct snd_ctl_led *led;
+	struct snd_ctl_led_ctl *lctl;
 
 	for (group = 0; group < MAX_LED; group++) {
-		controls = &snd_ctl_led_controls[group];
+		led = &snd_ctl_leds[group];
 repeat:
-		list_for_each_entry(lctl, controls, list)
+		list_for_each_entry(lctl, &led->controls, list)
 			if (!card || lctl->card == card) {
 				list_del(&lctl->list);
 				kfree(lctl);
@@ -248,6 +275,82 @@ static void snd_ctl_led_disconnect(struct snd_card *card)
 	snd_ctl_led_refresh();
 }
 
+/*
+ * sysfs
+ */
+
+static ssize_t show_mode(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	struct snd_ctl_led *led = container_of(dev, struct snd_ctl_led, dev);
+	const char *str;
+
+	switch (led->mode) {
+	case MODE_FOLLOW_MUTE:	str = "follow-mute"; break;
+	case MODE_FOLLOW_ROUTE:	str = "follow-route"; break;
+	case MODE_ON:		str = "on"; break;
+	case MODE_OFF:		str = "off"; break;
+	}
+	return sprintf(buf, "%s\n", str);
+}
+
+static ssize_t store_mode(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t count)
+{
+	struct snd_ctl_led *led = container_of(dev, struct snd_ctl_led, dev);
+	char _buf[16];
+	size_t l = min(count, sizeof(_buf) - 1) + 1;
+	enum snd_ctl_led_mode mode;
+
+	memcpy(_buf, buf, l);
+	_buf[l] = '\0';
+	if (strstr(_buf, "mute"))
+		mode = MODE_FOLLOW_MUTE;
+	else if (strstr(_buf, "route"))
+		mode = MODE_FOLLOW_ROUTE;
+	else if (strncmp(_buf, "off", 3) == 0 || strncmp(_buf, "0", 1) == 0)
+		mode = MODE_OFF;
+	else if (strncmp(_buf, "on", 2) == 0 || strncmp(_buf, "1", 1) == 0)
+		mode = MODE_ON;
+	else
+		return count;
+
+	mutex_lock(&snd_ctl_led_mutex);
+	led->mode = mode;
+	mutex_unlock(&snd_ctl_led_mutex);
+
+	snd_ctl_led_set_state(NULL, group_to_access(led->group), NULL, 0);
+	return count;
+}
+
+static ssize_t show_brightness(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct snd_ctl_led *led = container_of(dev, struct snd_ctl_led, dev);
+
+	return sprintf(buf, "%u\n", ledtrig_audio_get(led->trigger_type));
+}
+
+static DEVICE_ATTR(mode, 0644, show_mode, store_mode);
+static DEVICE_ATTR(brightness, 0444, show_brightness, NULL);
+
+static struct attribute *snd_ctl_led_dev_attrs[] = {
+	&dev_attr_mode.attr,
+	&dev_attr_brightness.attr,
+	NULL,
+};
+
+static const struct attribute_group snd_ctl_led_dev_attr_group = {
+	.attrs = snd_ctl_led_dev_attrs,
+};
+
+static const struct attribute_group *snd_ctl_led_dev_attr_groups[] = {
+	&snd_ctl_led_dev_attr_group,
+	NULL,
+};
+
+static struct device snd_ctl_led_dev;
+
 /*
  * Control layer registration
  */
@@ -260,16 +363,47 @@ static struct snd_ctl_layer_ops snd_ctl_led_lops = {
 
 static int __init snd_ctl_led_init(void)
 {
+	struct snd_ctl_led *led;
 	unsigned int group;
 
-	for (group = 0; group < MAX_LED; group++)
-		INIT_LIST_HEAD(&snd_ctl_led_controls[group]);
+	device_initialize(&snd_ctl_led_dev);
+	snd_ctl_led_dev.class = sound_class;
+	dev_set_name(&snd_ctl_led_dev, "ctl-led");
+	if (device_add(&snd_ctl_led_dev)) {
+		put_device(&snd_ctl_led_dev);
+		return -ENOMEM;
+	}
+	for (group = 0; group < MAX_LED; group++) {
+		led = &snd_ctl_leds[group];
+		INIT_LIST_HEAD(&led->controls);
+		device_initialize(&led->dev);
+		led->dev.parent = &snd_ctl_led_dev;
+		led->dev.groups = snd_ctl_led_dev_attr_groups;
+		dev_set_name(&led->dev, led->name);
+		if (device_add(&led->dev)) {
+			put_device(&led->dev);
+			for (; group > 0; group--) {
+				led = &snd_ctl_leds[group];
+				device_del(&led->dev);
+			}
+			device_del(&snd_ctl_led_dev);
+			return -ENOMEM;
+		}
+	}
 	snd_ctl_register_layer(&snd_ctl_led_lops);
 	return 0;
 }
 
 static void __exit snd_ctl_led_exit(void)
 {
+	struct snd_ctl_led *led;
+	unsigned int group;
+
+	for (group = 0; group < MAX_LED; group++) {
+		led = &snd_ctl_leds[group];
+		device_del(&led->dev);
+	}
+	device_del(&snd_ctl_led_dev);
 	snd_ctl_disconnect_layer(&snd_ctl_led_lops);
 	snd_ctl_led_clean(NULL);
 }
-- 
2.29.2