[PATCH 0/5] ALSA: control - add generic LED trigger code

[Jaroslav Kysela]

Dne 15. 02. 21 v 13:02 Hans de Goede napsal(a):
> Hi,
> 
> On 2/12/21 9:31 PM, Hans de Goede wrote:
>> On 2/11/21 12:13 PM, Jaroslav Kysela wrote:
> 
> <snip>
> 
>>> The sound driver implementation is really easy:
>>>
>>> 1) call snd_ctl_led_request() when control LED layer should be
>>>    automatically activated
>>>    / it calls module_request("snd-ctl-led") on demand /
>>> 2) mark all related kcontrols with
>>> 	SNDRV_CTL_ELEM_ACCESS_SPK_LED or
>>> 	SNDRV_CTL_ELEM_ACCESS_MIC_LED
>>
>> So I've been running some tests with this,combined with writing
>> UCM profiles for hw volume control, for some Intel Bay- and
>> CherryTrail devices using Intel's Low Power Engine (LPE) for audio,
>> which uses the ASoC framework.
>>
>> My work / experiments for this are progressing a bit slower then I
>> would like, but that is not the fault of this patch-set, but rather
>> an issue with hw-volume control mapping, see below for details.
>>
>> Leaving the ASoC implementation details aside, this patch-set
>> works quite nicely to get the speaker mute-LED to work.
> 
> I've spend some more time this weekend playing with this and I've also
> added mic MUTE LED support for the ASoC rt5672 codec driver now using
> this.
> 
> I will post a RFC patch series with the ASoC rt5672 codec driver LED
> support soon, as adding an extra use-case for this will hopefully help
> with reviewing this.
> 
> FWIW there were some challenges, but those were not related to the
> driver API this patch set adds. The driver API works well for ASoC
> codec drivers.
> 
> Regards,
> 
> Hans
> 
> 
> p.s.
> 
> One open issue is the lockdep issue which I reported in my
> previous email.

Thank you for tests, Hans. I'm working on the lockdep issue - I'll send v2 of the LED patchset soon.

The actual diff (I'd like to do more tests):

diff --git a/sound/core/control.c b/sound/core/control.c
index 4647b3cd41e8..c9f062fada0a 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -2047,6 +2047,7 @@ void snd_ctl_register_layer(struct snd_ctl_layer_ops *lops)
 	down_write(&snd_ctl_layer_rwsem);
 	lops->next = snd_ctl_layer;
 	snd_ctl_layer = lops;
+	up_write(&snd_ctl_layer_rwsem);
 	for (card_number = 0; card_number < SNDRV_CARDS; card_number++) {
 		card = snd_card_ref(card_number);
 		if (card) {
@@ -2054,7 +2055,6 @@ void snd_ctl_register_layer(struct snd_ctl_layer_ops *lops)
 			snd_card_unref(card);
 		}
 	}
-	up_write(&snd_ctl_layer_rwsem);
 }
 EXPORT_SYMBOL_GPL(snd_ctl_register_layer);
 
diff --git a/sound/core/control_led.c b/sound/core/control_led.c
index 638808e397fe..093dce721024 100644
--- a/sound/core/control_led.c
+++ b/sound/core/control_led.c
@@ -75,7 +75,7 @@ static inline unsigned int group_to_access(unsigned int group)
 	return (group + 1) << SNDRV_CTL_ELEM_ACCESS_LED_SHIFT;
 }
 
-struct snd_ctl_led *snd_ctl_led_get_by_access(unsigned int access)
+static struct snd_ctl_led *snd_ctl_led_get_by_access(unsigned int access)
 {
 	unsigned int group = access_to_group(access);
 	if (group >= MAX_LED)
@@ -116,15 +116,20 @@ static int snd_ctl_led_get(struct snd_ctl_led_ctl *lctl)
 	return 0;
 }
 
-static int snd_ctl_led_get_lock(struct snd_ctl_led_ctl *lctl)
+static int snd_ctl_led_get_lock(struct snd_card *ncard, struct snd_ctl_led_ctl *lctl)
 {
 	struct snd_card *card = lctl->card;
 	int route;
 
-	down_read(&card->controls_rwsem);
-	route = snd_ctl_led_get(lctl);
-	up_read(&card->controls_rwsem);
-	return route;
+	/* the rwsem is already taken for the notification card */
+	if (ncard != card) {
+		down_read(&card->controls_rwsem);
+		route = snd_ctl_led_get(lctl);
+		up_read(&card->controls_rwsem);
+		return route;
+	} else {
+		return snd_ctl_led_get(lctl);
+	}
 }
 
 static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
@@ -149,7 +154,7 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
 	list_for_each_entry(lctl, &led->controls, list) {
 		if (lctl->kctl == kctl && lctl->index_offset == ioff)
 			found = true;
-		UPDATE_ROUTE(route, snd_ctl_led_get_lock(lctl));
+		UPDATE_ROUTE(route, snd_ctl_led_get_lock(card, lctl));
 	}
 	if (!found && kctl && card) {
 		lctl = kzalloc(sizeof(*lctl), GFP_KERNEL);
@@ -158,7 +163,7 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
 			lctl->kctl = kctl;
 			lctl->index_offset = ioff;
 			list_add(&lctl->list, &led->controls);
-			UPDATE_ROUTE(route, snd_ctl_led_get_lock(lctl));
+			UPDATE_ROUTE(route, snd_ctl_led_get_lock(card, lctl));
 		}
 	}
 	mutex_unlock(&snd_ctl_led_mutex);
@@ -246,10 +251,11 @@ static void snd_ctl_led_register(struct snd_card *card)
 	mutex_lock(&snd_ctl_led_mutex);
 	snd_ctl_led_card_valid[card->number] = true;
 	mutex_unlock(&snd_ctl_led_mutex);
-	/* the register callback is already called with held rwsem for controls */
+	down_read(&card->controls_rwsem);
 	list_for_each_entry(kctl, &card->controls, list)
 		for (ioff = 0; ioff < kctl->count; ioff++)
 			snd_ctl_led_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, kctl, ioff);
+	up_read(&card->controls_rwsem);
 	snd_ctl_led_refresh();
 }
 
@@ -262,17 +268,6 @@ static void snd_ctl_led_disconnect(struct snd_card *card)
 	snd_ctl_led_refresh();
 }
 
-/**
- * snd_ctl_led_hello - kernel module reference helper
- *
- * Call this helper in the module init function when the control LED
- * code should be activated for the given driver.
- */
-void snd_ctl_led_hello(void)
-{
-}
-EXPORT_SYMBOL(snd_ctl_led_hello);
-
 /*
  * sysfs
  */


					Jaroslav

-- 
Jaroslav Kysela <perex at perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.