[PATCH v2] leds: trigger: audio: Add an activate callback to ensure the initial brightness is set

[PATCH v2] leds: trigger: audio: Add an activate callback to ensure the initial brightness is set

[Hans de Goede]

Some 2-in-1s with a detachable (USB) keyboard(dock) have mute-LEDs in
the speaker- and/or mic-mute keys on the keyboard.

Examples of this are the Lenovo Thinkpad10 tablet (with its USB kbd-dock)
and the HP x2 10 series.

The detachable nature of these keyboards means that the keyboard and
thus the mute LEDs may show up after the user (or userspace restoring
old mixer settings) has muted the speaker and/or mic.

Current LED-class devices with a default_trigger of "audio-mute" or
"audio-micmute" initialize the brightness member of led_classdev with
ledtrig_audio_get() before registering the LED.

This makes the software state after attaching the keyboard match the
actual audio mute state, e.g. cat /sys/class/leds/foo/brightness will
show the right value.

But before this commit nothing was actually calling the led_classdev's
brightness_set[_blocking] callback so the value returned by
ledtrig_audio_get() was never actually being sent to the hw, leading
to the mute LEDs staying in their default power-on state, after
attaching the keyboard, even if ledtrig_audio_get() returned a different
state.

This could be fixed by having the individual LED drivers call
brightness_set[_blocking] themselves after registering the LED,
but this really is something which should be done by a led-trigger
activate callback.

Add an activate callback for this, fixing the issue of the
mute LEDs being out of sync after (re)attaching the keyboard.

Cc: Takashi Iwai <tiwai@suse.de>
Fixes: faa2541f5b1a ("leds: trigger: Introduce audio mute LED trigger")
Reviewed-by: Marek Behún <kabel@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Fix a couple of grammar errors in the commit-message
- Add Marek's Reviewed-by (thank you)
---
 drivers/leds/trigger/ledtrig-audio.c | 37 ++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-audio.c b/drivers/leds/trigger/ledtrig-audio.c
index f76621e88482..c6b437e6369b 100644
--- a/drivers/leds/trigger/ledtrig-audio.c
+++ b/drivers/leds/trigger/ledtrig-audio.c
@@ -6,10 +6,33 @@
 #include <linux/kernel.h>
 #include <linux/leds.h>
 #include <linux/module.h>
+#include "../leds.h"
 
-static struct led_trigger *ledtrig_audio[NUM_AUDIO_LEDS];
 static enum led_brightness audio_state[NUM_AUDIO_LEDS];
 
+static int ledtrig_audio_mute_activate(struct led_classdev *led_cdev)
+{
+	led_set_brightness_nosleep(led_cdev, audio_state[LED_AUDIO_MUTE]);
+	return 0;
+}
+
+static int ledtrig_audio_micmute_activate(struct led_classdev *led_cdev)
+{
+	led_set_brightness_nosleep(led_cdev, audio_state[LED_AUDIO_MICMUTE]);
+	return 0;
+}
+
+static struct led_trigger ledtrig_audio[NUM_AUDIO_LEDS] = {
+	[LED_AUDIO_MUTE] = {
+		.name     = "audio-mute",
+		.activate = ledtrig_audio_mute_activate,
+	},
+	[LED_AUDIO_MICMUTE] = {
+		.name     = "audio-micmute",
+		.activate = ledtrig_audio_micmute_activate,
+	},
+};
+
 enum led_brightness ledtrig_audio_get(enum led_audio type)
 {
 	return audio_state[type];
@@ -19,24 +42,22 @@ EXPORT_SYMBOL_GPL(ledtrig_audio_get);
 void ledtrig_audio_set(enum led_audio type, enum led_brightness state)
 {
 	audio_state[type] = state;
-	led_trigger_event(ledtrig_audio[type], state);
+	led_trigger_event(&ledtrig_audio[type], state);
 }
 EXPORT_SYMBOL_GPL(ledtrig_audio_set);
 
 static int __init ledtrig_audio_init(void)
 {
-	led_trigger_register_simple("audio-mute",
-				    &ledtrig_audio[LED_AUDIO_MUTE]);
-	led_trigger_register_simple("audio-micmute",
-				    &ledtrig_audio[LED_AUDIO_MICMUTE]);
+	led_trigger_register(&ledtrig_audio[LED_AUDIO_MUTE]);
+	led_trigger_register(&ledtrig_audio[LED_AUDIO_MICMUTE]);
 	return 0;
 }
 module_init(ledtrig_audio_init);
 
 static void __exit ledtrig_audio_exit(void)
 {
-	led_trigger_unregister_simple(ledtrig_audio[LED_AUDIO_MUTE]);
-	led_trigger_unregister_simple(ledtrig_audio[LED_AUDIO_MICMUTE]);
+	led_trigger_unregister(&ledtrig_audio[LED_AUDIO_MUTE]);
+	led_trigger_unregister(&ledtrig_audio[LED_AUDIO_MICMUTE]);
 }
 module_exit(ledtrig_audio_exit);
 
-- 
2.30.1

Re: [PATCH v2] leds: trigger: audio: Add an activate callback to ensure the initial brightness is set

[Pavel Machek]

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

Hi!

> Some 2-in-1s with a detachable (USB) keyboard(dock) have mute-LEDs in
> the speaker- and/or mic-mute keys on the keyboard.
> 
> Examples of this are the Lenovo Thinkpad10 tablet (with its USB kbd-dock)
> and the HP x2 10 series.
> 
> The detachable nature of these keyboards means that the keyboard and
> thus the mute LEDs may show up after the user (or userspace restoring
> old mixer settings) has muted the speaker and/or mic.
> 
> Current LED-class devices with a default_trigger of "audio-mute" or
> "audio-micmute" initialize the brightness member of led_classdev with
> ledtrig_audio_get() before registering the LED.
> 
> This makes the software state after attaching the keyboard match the
> actual audio mute state, e.g. cat /sys/class/leds/foo/brightness will
> show the right value.

Makes sense.

> +++ b/drivers/leds/trigger/ledtrig-audio.c
> @@ -6,10 +6,33 @@
>  #include <linux/kernel.h>
>  #include <linux/leds.h>
>  #include <linux/module.h>
> +#include "../leds.h"
>  
> -static struct led_trigger *ledtrig_audio[NUM_AUDIO_LEDS];
>  static enum led_brightness audio_state[NUM_AUDIO_LEDS];
>  
> +static int ledtrig_audio_mute_activate(struct led_classdev *led_cdev)
> +{
> +	led_set_brightness_nosleep(led_cdev, audio_state[LED_AUDIO_MUTE]);
> +	return 0;
> +}

Is mute_activate called from atomic context?

Best regards,
								Pavel

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2] leds: trigger: audio: Add an activate callback to ensure the initial brightness is set

[Hans de Goede]

Hi,

On 2/23/21 10:12 AM, Pavel Machek wrote:
> Hi!
> 
>> Some 2-in-1s with a detachable (USB) keyboard(dock) have mute-LEDs in
>> the speaker- and/or mic-mute keys on the keyboard.
>>
>> Examples of this are the Lenovo Thinkpad10 tablet (with its USB kbd-dock)
>> and the HP x2 10 series.
>>
>> The detachable nature of these keyboards means that the keyboard and
>> thus the mute LEDs may show up after the user (or userspace restoring
>> old mixer settings) has muted the speaker and/or mic.
>>
>> Current LED-class devices with a default_trigger of "audio-mute" or
>> "audio-micmute" initialize the brightness member of led_classdev with
>> ledtrig_audio_get() before registering the LED.
>>
>> This makes the software state after attaching the keyboard match the
>> actual audio mute state, e.g. cat /sys/class/leds/foo/brightness will
>> show the right value.
> 
> Makes sense.
> 
>> +++ b/drivers/leds/trigger/ledtrig-audio.c
>> @@ -6,10 +6,33 @@
>>  #include <linux/kernel.h>
>>  #include <linux/leds.h>
>>  #include <linux/module.h>
>> +#include "../leds.h"
>>  
>> -static struct led_trigger *ledtrig_audio[NUM_AUDIO_LEDS];
>>  static enum led_brightness audio_state[NUM_AUDIO_LEDS];
>>  
>> +static int ledtrig_audio_mute_activate(struct led_classdev *led_cdev)
>> +{
>> +	led_set_brightness_nosleep(led_cdev, audio_state[LED_AUDIO_MUTE]);
>> +	return 0;
>> +}
> 
> Is mute_activate called from atomic context?

All the other ledtrig-foo.c activate callbacks use led_set_brightness_nosleep(),
so yes I would assume so (I did not check, I assumed the others have good
reasons to do this).

Regards,

Hans

Re: [PATCH v2] leds: trigger: audio: Add an activate callback to ensure the initial brightness is set

[Takashi Iwai]

On Sun, 21 Feb 2021 12:52:08 +0100,
Hans de Goede wrote:
> 
> Some 2-in-1s with a detachable (USB) keyboard(dock) have mute-LEDs in
> the speaker- and/or mic-mute keys on the keyboard.
> 
> Examples of this are the Lenovo Thinkpad10 tablet (with its USB kbd-dock)
> and the HP x2 10 series.
> 
> The detachable nature of these keyboards means that the keyboard and
> thus the mute LEDs may show up after the user (or userspace restoring
> old mixer settings) has muted the speaker and/or mic.
> 
> Current LED-class devices with a default_trigger of "audio-mute" or
> "audio-micmute" initialize the brightness member of led_classdev with
> ledtrig_audio_get() before registering the LED.
> 
> This makes the software state after attaching the keyboard match the
> actual audio mute state, e.g. cat /sys/class/leds/foo/brightness will
> show the right value.
> 
> But before this commit nothing was actually calling the led_classdev's
> brightness_set[_blocking] callback so the value returned by
> ledtrig_audio_get() was never actually being sent to the hw, leading
> to the mute LEDs staying in their default power-on state, after
> attaching the keyboard, even if ledtrig_audio_get() returned a different
> state.
> 
> This could be fixed by having the individual LED drivers call
> brightness_set[_blocking] themselves after registering the LED,
> but this really is something which should be done by a led-trigger
> activate callback.
> 
> Add an activate callback for this, fixing the issue of the
> mute LEDs being out of sync after (re)attaching the keyboard.
> 
> Cc: Takashi Iwai <tiwai@suse.de>
> Fixes: faa2541f5b1a ("leds: trigger: Introduce audio mute LED trigger")
> Reviewed-by: Marek Behún <kabel@kernel.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> - Fix a couple of grammar errors in the commit-message
> - Add Marek's Reviewed-by (thank you)

Looks good to me, thanks!
Reviewed-by: Takashi Iwai <tiwai@suse.de>


Takashi

> ---
>  drivers/leds/trigger/ledtrig-audio.c | 37 ++++++++++++++++++++++------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-audio.c b/drivers/leds/trigger/ledtrig-audio.c
> index f76621e88482..c6b437e6369b 100644
> --- a/drivers/leds/trigger/ledtrig-audio.c
> +++ b/drivers/leds/trigger/ledtrig-audio.c
> @@ -6,10 +6,33 @@
>  #include <linux/kernel.h>
>  #include <linux/leds.h>
>  #include <linux/module.h>
> +#include "../leds.h"
>  
> -static struct led_trigger *ledtrig_audio[NUM_AUDIO_LEDS];
>  static enum led_brightness audio_state[NUM_AUDIO_LEDS];
>  
> +static int ledtrig_audio_mute_activate(struct led_classdev *led_cdev)
> +{
> +	led_set_brightness_nosleep(led_cdev, audio_state[LED_AUDIO_MUTE]);
> +	return 0;
> +}
> +
> +static int ledtrig_audio_micmute_activate(struct led_classdev *led_cdev)
> +{
> +	led_set_brightness_nosleep(led_cdev, audio_state[LED_AUDIO_MICMUTE]);
> +	return 0;
> +}
> +
> +static struct led_trigger ledtrig_audio[NUM_AUDIO_LEDS] = {
> +	[LED_AUDIO_MUTE] = {
> +		.name     = "audio-mute",
> +		.activate = ledtrig_audio_mute_activate,
> +	},
> +	[LED_AUDIO_MICMUTE] = {
> +		.name     = "audio-micmute",
> +		.activate = ledtrig_audio_micmute_activate,
> +	},
> +};
> +
>  enum led_brightness ledtrig_audio_get(enum led_audio type)
>  {
>  	return audio_state[type];
> @@ -19,24 +42,22 @@ EXPORT_SYMBOL_GPL(ledtrig_audio_get);
>  void ledtrig_audio_set(enum led_audio type, enum led_brightness state)
>  {
>  	audio_state[type] = state;
> -	led_trigger_event(ledtrig_audio[type], state);
> +	led_trigger_event(&ledtrig_audio[type], state);
>  }
>  EXPORT_SYMBOL_GPL(ledtrig_audio_set);
>  
>  static int __init ledtrig_audio_init(void)
>  {
> -	led_trigger_register_simple("audio-mute",
> -				    &ledtrig_audio[LED_AUDIO_MUTE]);
> -	led_trigger_register_simple("audio-micmute",
> -				    &ledtrig_audio[LED_AUDIO_MICMUTE]);
> +	led_trigger_register(&ledtrig_audio[LED_AUDIO_MUTE]);
> +	led_trigger_register(&ledtrig_audio[LED_AUDIO_MICMUTE]);
>  	return 0;
>  }
>  module_init(ledtrig_audio_init);
>  
>  static void __exit ledtrig_audio_exit(void)
>  {
> -	led_trigger_unregister_simple(ledtrig_audio[LED_AUDIO_MUTE]);
> -	led_trigger_unregister_simple(ledtrig_audio[LED_AUDIO_MICMUTE]);
> +	led_trigger_unregister(&ledtrig_audio[LED_AUDIO_MUTE]);
> +	led_trigger_unregister(&ledtrig_audio[LED_AUDIO_MICMUTE]);
>  }
>  module_exit(ledtrig_audio_exit);
>  
> -- 
> 2.30.1
>