[PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Takashi Iwai]

Hi,

this is patch series I've hacked after useful conversation with
Pavel.  Basically this adds a new LED trigger audio-mute and
audio-micmute, and convert the HD-audio driver and the platform
drivers to use the LED trigger instead of the ugly direct dynamic
symbol binding.

The latest version of patches are found in topic/leds-trigger branch
in my sound git tree.
  git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git

As these are cross-tree patches, the branch above is based cleanly on
v4.20-rc3, so that it can be merged well to multiple trees.

Once after getting the ACK's, I'll add tags and fixate for merges.

This patch series don't include huawei-wmi stuff; so Huawei patches
need rework.  I already have some piece of changes for huawei-wmi, so
please ping me if needed.

I checked briefly on my Dell laptop, and a Thinkpad model.
Wider tests are appreciated, of course.


thanks,

Takashi

===

Takashi Iwai (6):
  leds: trigger: Introduce audio mute LED trigger
  platform/x86: dell-laptop: Add micmute LED trigger support
  platform/x86: thinkpad_acpi: Add audio mute LED classdev support
  ALSA: hda - Support led audio trigger
  platform/x86: dell-laptop: Drop superfluous exported function
  platform/x86: thinkpad_acpi: Drop superfluous exported function

 drivers/leds/trigger/Kconfig         |  7 +++
 drivers/leds/trigger/Makefile        |  1 +
 drivers/leds/trigger/ledtrig-audio.c | 45 +++++++++++++++++++
 drivers/platform/x86/Kconfig         |  4 ++
 drivers/platform/x86/dell-laptop.c   | 27 +++++++++---
 drivers/platform/x86/thinkpad_acpi.c | 66 +++++++++++++++++++++-------
 include/linux/dell-led.h             |  7 ---
 include/linux/leds.h                 | 20 +++++++++
 include/linux/thinkpad_acpi.h        | 16 -------
 sound/pci/hda/dell_wmi_helper.c      | 48 --------------------
 sound/pci/hda/hda_generic.c          | 31 +++++++++++++
 sound/pci/hda/hda_generic.h          |  2 +
 sound/pci/hda/patch_realtek.c        | 17 +++----
 sound/pci/hda/thinkpad_helper.c      | 43 +++---------------
 14 files changed, 194 insertions(+), 140 deletions(-)
 create mode 100644 drivers/leds/trigger/ledtrig-audio.c
 delete mode 100644 include/linux/dell-led.h
 delete mode 100644 include/linux/thinkpad_acpi.h
 delete mode 100644 sound/pci/hda/dell_wmi_helper.c

-- 
2.19.1

[PATCH 2/6] platform/x86: dell-laptop: Add micmute LED trigger support

[Takashi Iwai]

This patch adds the LED trigger support for audio mic-mute control.
As of this patch, the LED device isn't tied with the audio driver, and
can be changed via user-space at "dell::micmute" sysfs entry.  The
binding with HD-audio is still done via the existing exported
dell_micmute_led_set().  It will be replaced with the LED trigger
binding in later patches.

Also this selects CONFIG_LEDS_TRIGGERS and CONFIG_LEDS_TRIGGERS_AUDIO
unconditionally.  Strictly speaking, these aren't 100% mandatory, but
leaving these manual selections would lead to a functional regression
easily once after converting from the dynamic symbol binding to the
LEDs trigger in a later patch.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/platform/x86/Kconfig       |  2 ++
 drivers/platform/x86/dell-laptop.c | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 54f6a40c75c6..9b9cc3cc33e9 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -177,6 +177,8 @@ config DELL_LAPTOP
 	select POWER_SUPPLY
 	select LEDS_CLASS
 	select NEW_LEDS
+	select LEDS_TRIGGERS
+	select LEDS_TRIGGER_AUDIO
 	---help---
 	This driver adds support for rfkill and backlight control to Dell
 	laptops (except for some models covered by the Compal driver).
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 06978c14c83b..6a647e2d09ee 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -2131,6 +2131,23 @@ int dell_micmute_led_set(int state)
 }
 EXPORT_SYMBOL_GPL(dell_micmute_led_set);
 
+static int micmute_led_set(struct led_classdev *led_cdev,
+			   enum led_brightness brightness)
+{
+	int state = brightness != LED_OFF;
+	int err;
+
+	err = dell_micmute_led_set(state);
+	return err < 0 ? err : 0;
+}
+
+static struct led_classdev micmute_led_cdev = {
+	.name = "dell::micmute",
+	.max_brightness = 1,
+	.brightness_set_blocking = micmute_led_set,
+	.default_trigger = "audio-micmute",
+};
+
 static int __init dell_init(void)
 {
 	struct calling_interface_token *token;
@@ -2175,6 +2192,11 @@ static int __init dell_init(void)
 
 	dell_laptop_register_notifier(&dell_laptop_notifier);
 
+	micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
+	ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
+	if (ret < 0)
+		goto fail_led;
+
 	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
 		return 0;
 
@@ -2220,6 +2242,8 @@ static int __init dell_init(void)
 fail_get_brightness:
 	backlight_device_unregister(dell_backlight_device);
 fail_backlight:
+	led_classdev_unregister(&micmute_led_cdev);
+fail_led:
 	dell_cleanup_rfkill();
 fail_rfkill:
 	platform_device_del(platform_device);
@@ -2239,6 +2263,7 @@ static void __exit dell_exit(void)
 		touchpad_led_exit();
 	kbd_led_exit();
 	backlight_device_unregister(dell_backlight_device);
+	led_classdev_unregister(&micmute_led_cdev);
 	dell_cleanup_rfkill();
 	if (platform_device) {
 		platform_device_unregister(platform_device);
-- 
2.19.1

[PATCH 3/6] platform/x86: thinkpad_acpi: Add audio mute LED classdev support

[Takashi Iwai]

In the upcoming change, the binding of audio mute / mic-mute LED
controls will be switched with LED trigger.  This patch is the last
piece of preparation: adding the audio mute / mic-mute LED class
devices to thinkpad_acpi driver.

Two devices, tpacpi::mute and tpacpi::micmute, will be added for
controlling the mute LED and mic-mute LED, respectively.

Also this selects CONFIG_LEDS_TRIGGERS and CONFIG_LEDS_TRIGGERS_AUDIO
unconditionally.  Strictly speaking, these aren't 100% mandatory, but
leaving these manual selections would lead to a functional regression
easily once after converting from the dynamic symbol binding to the
LEDs trigger in a later patch.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/platform/x86/Kconfig         |  2 +
 drivers/platform/x86/thinkpad_acpi.c | 56 +++++++++++++++++++++++++---
 2 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 9b9cc3cc33e9..87f70e8f4dd0 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -495,6 +495,8 @@ config THINKPAD_ACPI
 	select NVRAM
 	select NEW_LEDS
 	select LEDS_CLASS
+	select LEDS_TRIGGERS
+	select LEDS_TRIGGER_AUDIO
 	---help---
 	  This is a driver for the IBM and Lenovo ThinkPad laptops. It adds
 	  support for Fn-Fx key combinations, Bluetooth control, video
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index fde08a997557..3c09afae1248 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9203,17 +9203,57 @@ int tpacpi_led_set(int whichled, bool on)
 }
 EXPORT_SYMBOL_GPL(tpacpi_led_set);
 
+static int tpacpi_led_mute_set(struct led_classdev *led_cdev,
+			       enum led_brightness brightness)
+{
+	return tpacpi_led_set(TPACPI_LED_MUTE, brightness != LED_OFF);
+}
+
+static int tpacpi_led_micmute_set(struct led_classdev *led_cdev,
+				  enum led_brightness brightness)
+{
+	return tpacpi_led_set(TPACPI_LED_MICMUTE, brightness != LED_OFF);
+}
+
+static struct led_classdev mute_led_cdev[] = {
+	[TPACPI_LED_MUTE] = {
+		.name		= "tpacpi::mute",
+		.max_brightness = 1,
+		.brightness_set_blocking = tpacpi_led_mute_set,
+		.default_trigger = "audio-mute",
+	},
+	[TPACPI_LED_MICMUTE] = {
+		.name		= "tpacpi::micmute",
+		.max_brightness = 1,
+		.brightness_set_blocking = tpacpi_led_micmute_set,
+		.default_trigger = "audio-micmute",
+	},
+};
+
 static int mute_led_init(struct ibm_init_struct *iibm)
 {
+	static enum led_audio types[] = {
+		[TPACPI_LED_MUTE] = LED_AUDIO_MUTE,
+		[TPACPI_LED_MICMUTE] = LED_AUDIO_MICMUTE,
+	};
 	acpi_handle temp;
-	int i;
+	int i, err;
 
 	for (i = 0; i < TPACPI_LED_MAX; i++) {
 		struct tp_led_table *t = &led_tables[i];
-		if (ACPI_SUCCESS(acpi_get_handle(hkey_handle, t->name, &temp)))
-			mute_led_on_off(t, false);
-		else
+		if (!ACPI_SUCCESS(acpi_get_handle(hkey_handle, t->name, &temp))) {
 			t->state = -ENODEV;
+			continue;
+		}
+
+		mute_led_cdev[i].brightness = ledtrig_audio_get(types[i]);
+		err = led_classdev_register(&tpacpi_pdev->dev, &mute_led_cdev[i]);
+		if (err < 0) {
+			while (--i >= 0)
+				if (led_tables[i].state >= 0)
+					led_classdev_unregister(&mute_led_cdev[i]);
+			return err;
+		}
 	}
 	return 0;
 }
@@ -9222,8 +9262,12 @@ static void mute_led_exit(void)
 {
 	int i;
 
-	for (i = 0; i < TPACPI_LED_MAX; i++)
-		tpacpi_led_set(i, false);
+	for (i = 0; i < TPACPI_LED_MAX; i++) {
+		if (led_tables[i].state >= 0) {
+			led_classdev_unregister(&mute_led_cdev[i]);
+			tpacpi_led_set(i, false);
+		}
+	}
 }
 
 static void mute_led_resume(void)
-- 
2.19.1

Re: [PATCH 3/6] platform/x86: thinkpad_acpi: Add audio mute LED classdev support

[Andy Shevchenko]

On Mon, Nov 26, 2018 at 7:13 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> In the upcoming change, the binding of audio mute / mic-mute LED
> controls will be switched with LED trigger.  This patch is the last
> piece of preparation: adding the audio mute / mic-mute LED class
> devices to thinkpad_acpi driver.
>
> Two devices, tpacpi::mute and tpacpi::micmute, will be added for
> controlling the mute LED and mic-mute LED, respectively.
>
> Also this selects CONFIG_LEDS_TRIGGERS and CONFIG_LEDS_TRIGGERS_AUDIO
> unconditionally.  Strictly speaking, these aren't 100% mandatory, but
> leaving these manual selections would lead to a functional regression
> easily once after converting from the dynamic symbol binding to the
> LEDs trigger in a later patch.

> +               if (!ACPI_SUCCESS(acpi_get_handle(hkey_handle, t->name, &temp))) {

ACPI_FAILURE()

>                         t->state = -ENODEV;
> +                       continue;
> +               }

> +               err = led_classdev_register(&tpacpi_pdev->dev, &mute_led_cdev[i]);
> +               if (err < 0) {

> +                       while (--i >= 0)

Needs { } due to two liner below.

Might be converted to simple

while (i--) {
 ...
}

btw.

> +                               if (led_tables[i].state >= 0)
> +                                       led_classdev_unregister(&mute_led_cdev[i]);
> +                       return err;
> +               }

--
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/6] platform/x86: thinkpad_acpi: Add audio mute LED classdev support

[Takashi Iwai]

On Tue, 27 Nov 2018 00:04:40 +0100,
Andy Shevchenko wrote:
> 
> On Mon, Nov 26, 2018 at 7:13 PM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > In the upcoming change, the binding of audio mute / mic-mute LED
> > controls will be switched with LED trigger.  This patch is the last
> > piece of preparation: adding the audio mute / mic-mute LED class
> > devices to thinkpad_acpi driver.
> >
> > Two devices, tpacpi::mute and tpacpi::micmute, will be added for
> > controlling the mute LED and mic-mute LED, respectively.
> >
> > Also this selects CONFIG_LEDS_TRIGGERS and CONFIG_LEDS_TRIGGERS_AUDIO
> > unconditionally.  Strictly speaking, these aren't 100% mandatory, but
> > leaving these manual selections would lead to a functional regression
> > easily once after converting from the dynamic symbol binding to the
> > LEDs trigger in a later patch.
> 
> > +               if (!ACPI_SUCCESS(acpi_get_handle(hkey_handle, t->name, &temp))) {
> 
> ACPI_FAILURE()
> 
> >                         t->state = -ENODEV;
> > +                       continue;
> > +               }
> 
> > +               err = led_classdev_register(&tpacpi_pdev->dev, &mute_led_cdev[i]);
> > +               if (err < 0) {
> 
> > +                       while (--i >= 0)
> 
> Needs { } due to two liner below.
> 
> Might be converted to simple
> 
> while (i--) {
>  ...
> }
> 
> btw.

Both addressed now.  Thanks.


Takashi

[PATCH 1/6] leds: trigger: Introduce audio mute LED trigger

[Takashi Iwai]

This patch adds a new LED trigger for coupling the audio mixer change
with the LED on laptops or other devices.  Currently there are two
trigger types, "audio-mute" and "audio-micmute".

The audio driver triggers the LED brightness change via
ledtrig_audio_set() call with the proper type (either mute or
mic-mute).  OTOH, the consumers may call ledtrig_audio_get() for the
initial brightness value that may have been set by the audio driver
beforehand.

This new stuff will be used by HD-audio codec driver and some platform
drivers (thinkpad_acpi and dell-laptop, also upcoming huawei-wmi).

Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
---
 drivers/leds/trigger/Kconfig         |  7 +++++
 drivers/leds/trigger/Makefile        |  1 +
 drivers/leds/trigger/ledtrig-audio.c | 45 ++++++++++++++++++++++++++++
 include/linux/leds.h                 | 20 +++++++++++++
 4 files changed, 73 insertions(+)
 create mode 100644 drivers/leds/trigger/ledtrig-audio.c

diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index b76fc3cdc8f8..23cc85e2e0e5 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -136,4 +136,11 @@ config LEDS_TRIGGER_PATTERN
 	  which is a series of tuples, of brightness and duration (ms).
 	  If unsure, say N
 
+config LEDS_TRIGGER_AUDIO
+	tristate "Audio Mute LED Trigger"
+	help
+	  This allows LEDs to be controlled by audio drivers for following
+	  the audio mute and mic-mute changes.
+	  If unsure, say N
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 9bcb64ee8123..733a83e2a718 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
 obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
 obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
 obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
+obj-$(CONFIG_LEDS_TRIGGER_AUDIO)	+= ledtrig-audio.o
diff --git a/drivers/leds/trigger/ledtrig-audio.c b/drivers/leds/trigger/ledtrig-audio.c
new file mode 100644
index 000000000000..cf2b6837f570
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-audio.c
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Audio Mute LED trigger
+//
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+
+static struct led_trigger *ledtrig_audio[NUM_AUDIO_LEDS];
+static enum led_brightness audio_state[NUM_AUDIO_LEDS];
+
+enum led_brightness ledtrig_audio_get(enum led_audio type)
+{
+	return audio_state[type];
+}
+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);
+}
+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]);
+	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]);
+}
+module_exit(ledtrig_audio_exit);
+
+MODULE_DESCRIPTION("LED trigger for audio mute control");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 7393a316d9fa..580cbaef789a 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -487,4 +487,24 @@ struct led_pattern {
 	int brightness;
 };
 
+enum led_audio {
+	LED_AUDIO_MUTE,		/* master mute LED */
+	LED_AUDIO_MICMUTE,	/* mic mute LED */
+	NUM_AUDIO_LEDS
+};
+
+#if IS_ENABLED(CONFIG_LEDS_TRIGGER_AUDIO)
+enum led_brightness ledtrig_audio_get(enum led_audio type);
+void ledtrig_audio_set(enum led_audio type, enum led_brightness state);
+#else
+static inline enum led_brightness ledtrig_audio_get(enum led_audio type)
+{
+	return LED_OFF;
+}
+static inline void ledtrig_audio_set(enum led_audio type,
+				     enum led_brightness state)
+{
+}
+#endif
+
 #endif		/* __LINUX_LEDS_H_INCLUDED */
-- 
2.19.1

Re: [PATCH 1/6] leds: trigger: Introduce audio mute LED trigger

[Jacek Anaszewski]

Hi Takashi,

Thank you for the patch set.

On 11/26/2018 06:11 PM, Takashi Iwai wrote:
> This patch adds a new LED trigger for coupling the audio mixer change
> with the LED on laptops or other devices.  Currently there are two
> trigger types, "audio-mute" and "audio-micmute".
> 
> The audio driver triggers the LED brightness change via
> ledtrig_audio_set() call with the proper type (either mute or
> mic-mute).  OTOH, the consumers may call ledtrig_audio_get() for the
> initial brightness value that may have been set by the audio driver
> beforehand.
> 
> This new stuff will be used by HD-audio codec driver and some platform
> drivers (thinkpad_acpi and dell-laptop, also upcoming huawei-wmi).
> 
> Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
> ---
>  drivers/leds/trigger/Kconfig         |  7 +++++
>  drivers/leds/trigger/Makefile        |  1 +
>  drivers/leds/trigger/ledtrig-audio.c | 45 ++++++++++++++++++++++++++++
>  include/linux/leds.h                 | 20 +++++++++++++
>  4 files changed, 73 insertions(+)
>  create mode 100644 drivers/leds/trigger/ledtrig-audio.c
> 
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index b76fc3cdc8f8..23cc85e2e0e5 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -136,4 +136,11 @@ config LEDS_TRIGGER_PATTERN
>  	  which is a series of tuples, of brightness and duration (ms).
>  	  If unsure, say N
>  
> +config LEDS_TRIGGER_AUDIO
> +	tristate "Audio Mute LED Trigger"
> +	help
> +	  This allows LEDs to be controlled by audio drivers for following
> +	  the audio mute and mic-mute changes.
> +	  If unsure, say N
> +
>  endif # LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index 9bcb64ee8123..733a83e2a718 100644
> --- a/drivers/leds/trigger/Makefile
> +++ b/drivers/leds/trigger/Makefile
> @@ -14,3 +14,4 @@ obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
>  obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
>  obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
>  obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
> +obj-$(CONFIG_LEDS_TRIGGER_AUDIO)	+= ledtrig-audio.o
> diff --git a/drivers/leds/trigger/ledtrig-audio.c b/drivers/leds/trigger/ledtrig-audio.c
> new file mode 100644
> index 000000000000..cf2b6837f570
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-audio.c
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Audio Mute LED trigger
> +//
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/leds.h>
> +
> +static struct led_trigger *ledtrig_audio[NUM_AUDIO_LEDS];
> +static enum led_brightness audio_state[NUM_AUDIO_LEDS];
> +
> +enum led_brightness ledtrig_audio_get(enum led_audio type)
> +{
> +	return audio_state[type];
> +}
> +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);
> +}
> +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]);
> +	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]);
> +}
> +module_exit(ledtrig_audio_exit);
> +
> +MODULE_DESCRIPTION("LED trigger for audio mute control");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 7393a316d9fa..580cbaef789a 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -487,4 +487,24 @@ struct led_pattern {
>  	int brightness;
>  };
>  
> +enum led_audio {
> +	LED_AUDIO_MUTE,		/* master mute LED */
> +	LED_AUDIO_MICMUTE,	/* mic mute LED */
> +	NUM_AUDIO_LEDS
> +};
> +
> +#if IS_ENABLED(CONFIG_LEDS_TRIGGER_AUDIO)
> +enum led_brightness ledtrig_audio_get(enum led_audio type);
> +void ledtrig_audio_set(enum led_audio type, enum led_brightness state);
> +#else
> +static inline enum led_brightness ledtrig_audio_get(enum led_audio type)
> +{
> +	return LED_OFF;
> +}
> +static inline void ledtrig_audio_set(enum led_audio type,
> +				     enum led_brightness state)
> +{
> +}
> +#endif
> +
>  #endif		/* __LINUX_LEDS_H_INCLUDED */
> 

For this patch and FWIW for the whole patch set:

Acked-by: Jacek Anaszewski <jacek.anaszewski-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 1/6] leds: trigger: Introduce audio mute LED trigger

[Takashi Iwai]

On Mon, 26 Nov 2018 21:59:24 +0100,
Jacek Anaszewski wrote:
> 
> For this patch and FWIW for the whole patch set:
> 
> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

Thanks, now added to the series.

The latest patches are found in topic/leds-trigger branch of my sound
git tree.

I'm going to resubmit v2 series tomorrow or later, then merge the
branch to for-next branch.


Takashi

Re: [PATCH 1/6] leds: trigger: Introduce audio mute LED trigger

[Andy Shevchenko]

On Mon, Nov 26, 2018 at 7:14 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> This patch adds a new LED trigger for coupling the audio mixer change
> with the LED on laptops or other devices.  Currently there are two
> trigger types, "audio-mute" and "audio-micmute".
>
> The audio driver triggers the LED brightness change via
> ledtrig_audio_set() call with the proper type (either mute or
> mic-mute).  OTOH, the consumers may call ledtrig_audio_get() for the
> initial brightness value that may have been set by the audio driver
> beforehand.
>
> This new stuff will be used by HD-audio codec driver and some platform
> drivers (thinkpad_acpi and dell-laptop, also upcoming huawei-wmi).

> +#include <linux/module.h>

> +#include <linux/init.h>

Only on of above is needed, I think you meant module.h.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/6] leds: trigger: Introduce audio mute LED trigger

[Takashi Iwai]

On Mon, 26 Nov 2018 23:58:36 +0100,
Andy Shevchenko wrote:
> 
> On Mon, Nov 26, 2018 at 7:14 PM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > This patch adds a new LED trigger for coupling the audio mixer change
> > with the LED on laptops or other devices.  Currently there are two
> > trigger types, "audio-mute" and "audio-micmute".
> >
> > The audio driver triggers the LED brightness change via
> > ledtrig_audio_set() call with the proper type (either mute or
> > mic-mute).  OTOH, the consumers may call ledtrig_audio_get() for the
> > initial brightness value that may have been set by the audio driver
> > beforehand.
> >
> > This new stuff will be used by HD-audio codec driver and some platform
> > drivers (thinkpad_acpi and dell-laptop, also upcoming huawei-wmi).
> 
> > +#include <linux/module.h>
> 
> > +#include <linux/init.h>
> 
> Only on of above is needed, I think you meant module.h.

Right, dropped init.h inclusion.  Also sorted now.


thanks,

Takashi

[PATCH 4/6] ALSA: hda - Support led audio trigger

[Takashi Iwai]

Now all relevant platform drivers are providing the LED audio trigger,
we can switch the mute LED control with the LED trigger, finally.

For the mic-mute LED trigger, a common fixup function,
snd_hda_gen_fixup_micmute_led(), is provided to be called for the
corresponding quirk entries.  This sets up the capture sync hook with
ledtrig_audio_set() call appropriately.

For the mute LED trigger, which is done currently only for
thinkpad_acpi, the call is replaced with ledtrig_audio_set() as well.

Overall, the beauty of the new implementation is that he whole ugly
binding with request_symbol() are dropped, and also it provides more
flexibility to users.

One potential behavior change by this patch is that the mute LED enum
may be created on machines that actually have no LED device.  In the
former code, we did test-call and abort binding if the test failed.
But with the LED-trigger binding, this test isn't possible, and the
actual check is done in the LED class device side.  So it's the
downside of simpleness.

Also, note that the HD-audio codec driver doesn't select CONFIG_LEDS
and co by itself.  It's supposed to be selected by the platform
drivers instead.

Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
---
 sound/pci/hda/dell_wmi_helper.c | 48 ---------------------------------
 sound/pci/hda/hda_generic.c     | 31 +++++++++++++++++++++
 sound/pci/hda/hda_generic.h     |  2 ++
 sound/pci/hda/patch_realtek.c   | 17 +++++-------
 sound/pci/hda/thinkpad_helper.c | 43 +++++------------------------
 5 files changed, 46 insertions(+), 95 deletions(-)
 delete mode 100644 sound/pci/hda/dell_wmi_helper.c

diff --git a/sound/pci/hda/dell_wmi_helper.c b/sound/pci/hda/dell_wmi_helper.c
deleted file mode 100644
index bbd6c87a4ed6..000000000000
--- a/sound/pci/hda/dell_wmi_helper.c
+++ /dev/null
@@ -1,48 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/* Helper functions for Dell Mic Mute LED control;
- * to be included from codec driver
- */
-
-#if IS_ENABLED(CONFIG_DELL_LAPTOP)
-#include <linux/dell-led.h>
-
-static int (*dell_micmute_led_set_func)(int);
-
-static void dell_micmute_update(struct hda_codec *codec)
-{
-	struct hda_gen_spec *spec = codec->spec;
-
-	dell_micmute_led_set_func(spec->micmute_led.led_value);
-}
-
-static void alc_fixup_dell_wmi(struct hda_codec *codec,
-			       const struct hda_fixup *fix, int action)
-{
-	bool removefunc = false;
-
-	if (action == HDA_FIXUP_ACT_PROBE) {
-		if (!dell_micmute_led_set_func)
-			dell_micmute_led_set_func = symbol_request(dell_micmute_led_set);
-		if (!dell_micmute_led_set_func) {
-			codec_warn(codec, "Failed to find dell wmi symbol dell_micmute_led_set\n");
-			return;
-		}
-
-		removefunc = (dell_micmute_led_set_func(false) < 0) ||
-			(snd_hda_gen_add_micmute_led(codec,
-						     dell_micmute_update) < 0);
-	}
-
-	if (dell_micmute_led_set_func && (action == HDA_FIXUP_ACT_FREE || removefunc)) {
-		symbol_put(dell_micmute_led_set);
-		dell_micmute_led_set_func = NULL;
-	}
-}
-
-#else /* CONFIG_DELL_LAPTOP */
-static void alc_fixup_dell_wmi(struct hda_codec *codec,
-			       const struct hda_fixup *fix, int action)
-{
-}
-
-#endif /* CONFIG_DELL_LAPTOP */
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index 276150f29cda..4095cd7c56c6 100644
--- a/sound/pci/hda/hda_generic.c
+++ b/sound/pci/hda/hda_generic.c
@@ -29,6 +29,7 @@
 #include <linux/string.h>
 #include <linux/bitops.h>
 #include <linux/module.h>
+#include <linux/leds.h>
 #include <sound/core.h>
 #include <sound/jack.h>
 #include <sound/tlv.h>
@@ -4035,6 +4036,36 @@ int snd_hda_gen_add_micmute_led(struct hda_codec *codec,
 }
 EXPORT_SYMBOL_GPL(snd_hda_gen_add_micmute_led);
 
+#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
+static void call_ledtrig_micmute(struct hda_codec *codec)
+{
+	struct hda_gen_spec *spec = codec->spec;
+
+	ledtrig_audio_set(LED_AUDIO_MICMUTE,
+			  spec->micmute_led.led_value ? LED_ON : LED_OFF);
+}
+#endif
+
+/**
+ * snd_hda_gen_fixup_micmute_led - A fixup for mic-mute LED trigger
+ *
+ * Pass this function to the quirk entry if another driver supports the
+ * audio mic-mute LED trigger.  Then this will bind the mixer capture switch
+ * change with the LED.
+ *
+ * Note that this fixup has to be called after other fixup that sets
+ * cap_sync_hook.  Otherwise the chaining wouldn't work.
+ */
+void snd_hda_gen_fixup_micmute_led(struct hda_codec *codec,
+				   const struct hda_fixup *fix, int action)
+{
+#if IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
+	if (action == HDA_FIXUP_ACT_PROBE)
+		snd_hda_gen_add_micmute_led(codec, call_ledtrig_micmute);
+#endif
+}
+EXPORT_SYMBOL_GPL(snd_hda_gen_fixup_micmute_led);
+
 /*
  * parse digital I/Os and set up NIDs in BIOS auto-parse mode
  */
diff --git a/sound/pci/hda/hda_generic.h b/sound/pci/hda/hda_generic.h
index 10123664fa61..78d77042b05a 100644
--- a/sound/pci/hda/hda_generic.h
+++ b/sound/pci/hda/hda_generic.h
@@ -357,5 +357,7 @@ int snd_hda_gen_fix_pin_power(struct hda_codec *codec, hda_nid_t pin);
 
 int snd_hda_gen_add_micmute_led(struct hda_codec *codec,
 				void (*hook)(struct hda_codec *));
+void snd_hda_gen_fixup_micmute_led(struct hda_codec *codec,
+				   const struct hda_fixup *fix, int action);
 
 #endif /* __SOUND_HDA_GENERIC_H */
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index fa61674a5605..993d34c141c2 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -5368,9 +5368,6 @@ static void alc_fixup_thinkpad_acpi(struct hda_codec *codec,
 	hda_fixup_thinkpad_acpi(codec, fix, action);
 }
 
-/* for dell wmi mic mute led */
-#include "dell_wmi_helper.c"
-
 /* for alc295_fixup_hp_top_speakers */
 #include "hp_x360_helper.c"
 
@@ -5448,7 +5445,7 @@ enum {
 	ALC292_FIXUP_TPT440_DOCK,
 	ALC292_FIXUP_TPT440,
 	ALC283_FIXUP_HEADSET_MIC,
-	ALC255_FIXUP_DELL_WMI_MIC_MUTE_LED,
+	ALC255_FIXUP_MIC_MUTE_LED,
 	ALC282_FIXUP_ASPIRE_V5_PINS,
 	ALC280_FIXUP_HP_GPIO4,
 	ALC286_FIXUP_HP_GPIO_LED,
@@ -5740,7 +5737,7 @@ static const struct hda_fixup alc269_fixups[] = {
 		.type = HDA_FIXUP_FUNC,
 		.v.func = alc_fixup_headset_mode,
 		.chained = true,
-		.chain_id = ALC255_FIXUP_DELL_WMI_MIC_MUTE_LED
+		.chain_id = ALC255_FIXUP_MIC_MUTE_LED
 	},
 	[ALC269_FIXUP_HEADSET_MODE_NO_HP_MIC] = {
 		.type = HDA_FIXUP_FUNC,
@@ -5966,7 +5963,7 @@ static const struct hda_fixup alc269_fixups[] = {
 		.type = HDA_FIXUP_FUNC,
 		.v.func = alc_fixup_headset_mode_alc255,
 		.chained = true,
-		.chain_id = ALC255_FIXUP_DELL_WMI_MIC_MUTE_LED
+		.chain_id = ALC255_FIXUP_MIC_MUTE_LED
 	},
 	[ALC255_FIXUP_HEADSET_MODE_NO_HP_MIC] = {
 		.type = HDA_FIXUP_FUNC,
@@ -6001,9 +5998,9 @@ static const struct hda_fixup alc269_fixups[] = {
 			{ },
 		},
 	},
-	[ALC255_FIXUP_DELL_WMI_MIC_MUTE_LED] = {
+	[ALC255_FIXUP_MIC_MUTE_LED] = {
 		.type = HDA_FIXUP_FUNC,
-		.v.func = alc_fixup_dell_wmi,
+		.v.func = snd_hda_gen_fixup_micmute_led,
 	},
 	[ALC282_FIXUP_ASPIRE_V5_PINS] = {
 		.type = HDA_FIXUP_PINS,
@@ -6062,7 +6059,7 @@ static const struct hda_fixup alc269_fixups[] = {
 		.type = HDA_FIXUP_FUNC,
 		.v.func = alc_fixup_headset_mode_dell_alc288,
 		.chained = true,
-		.chain_id = ALC255_FIXUP_DELL_WMI_MIC_MUTE_LED
+		.chain_id = ALC255_FIXUP_MIC_MUTE_LED
 	},
 	[ALC288_FIXUP_DELL1_MIC_NO_PRESENCE] = {
 		.type = HDA_FIXUP_PINS,
@@ -6719,7 +6716,7 @@ static const struct hda_model_fixup alc269_fixup_models[] = {
 	{.id = ALC255_FIXUP_DELL2_MIC_NO_PRESENCE, .name = "alc255-dell2"},
 	{.id = ALC293_FIXUP_DELL1_MIC_NO_PRESENCE, .name = "alc293-dell1"},
 	{.id = ALC283_FIXUP_HEADSET_MIC, .name = "alc283-headset"},
-	{.id = ALC255_FIXUP_DELL_WMI_MIC_MUTE_LED, .name = "alc255-dell-mute"},
+	{.id = ALC255_FIXUP_MIC_MUTE_LED, .name = "alc255-dell-mute"},
 	{.id = ALC282_FIXUP_ASPIRE_V5_PINS, .name = "aspire-v5"},
 	{.id = ALC280_FIXUP_HP_GPIO4, .name = "hp-gpio4"},
 	{.id = ALC286_FIXUP_HP_GPIO_LED, .name = "hp-gpio-led"},
diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
index 568575b72f2f..4089feb8c68e 100644
--- a/sound/pci/hda/thinkpad_helper.c
+++ b/sound/pci/hda/thinkpad_helper.c
@@ -3,12 +3,11 @@
  * to be included from codec driver
  */
 
-#if IS_ENABLED(CONFIG_THINKPAD_ACPI)
+#if IS_ENABLED(CONFIG_THINKPAD_ACPI) && IS_REACHABLE(CONFIG_LEDS_TRIGGER_AUDIO)
 
 #include <linux/acpi.h>
-#include <linux/thinkpad_acpi.h>
+#include <linux/leds.h>
 
-static int (*led_set_func)(int, bool);
 static void (*old_vmaster_hook)(void *, int);
 
 static bool is_thinkpad(struct hda_codec *codec)
@@ -23,50 +22,20 @@ static void update_tpacpi_mute_led(void *private_data, int enabled)
 	if (old_vmaster_hook)
 		old_vmaster_hook(private_data, enabled);
 
-	if (led_set_func)
-		led_set_func(TPACPI_LED_MUTE, !enabled);
-}
-
-static void update_tpacpi_micmute(struct hda_codec *codec)
-{
-	struct hda_gen_spec *spec = codec->spec;
-
-	led_set_func(TPACPI_LED_MICMUTE, spec->micmute_led.led_value);
+	ledtrig_audio_set(LED_AUDIO_MUTE, enabled ? LED_OFF : LED_ON);
 }
 
 static void hda_fixup_thinkpad_acpi(struct hda_codec *codec,
 				    const struct hda_fixup *fix, int action)
 {
 	struct hda_gen_spec *spec = codec->spec;
-	bool removefunc = false;
 
 	if (action == HDA_FIXUP_ACT_PROBE) {
 		if (!is_thinkpad(codec))
 			return;
-		if (!led_set_func)
-			led_set_func = symbol_request(tpacpi_led_set);
-		if (!led_set_func) {
-			codec_warn(codec,
-				   "Failed to find thinkpad-acpi symbol tpacpi_led_set\n");
-			return;
-		}
-
-		removefunc = true;
-		if (led_set_func(TPACPI_LED_MUTE, false) >= 0) {
-			old_vmaster_hook = spec->vmaster_mute.hook;
-			spec->vmaster_mute.hook = update_tpacpi_mute_led;
-			removefunc = false;
-		}
-		if (led_set_func(TPACPI_LED_MICMUTE, false) >= 0 &&
-		    !snd_hda_gen_add_micmute_led(codec,
-						 update_tpacpi_micmute))
-			removefunc = false;
-	}
-
-	if (led_set_func && (action == HDA_FIXUP_ACT_FREE || removefunc)) {
-		symbol_put(tpacpi_led_set);
-		led_set_func = NULL;
-		old_vmaster_hook = NULL;
+		old_vmaster_hook = spec->vmaster_mute.hook;
+		spec->vmaster_mute.hook = update_tpacpi_mute_led;
+		snd_hda_gen_fixup_micmute_led(codec, fix, action);
 	}
 }
 
-- 
2.19.1

[PATCH 6/6] platform/x86: thinkpad_acpi: Drop superfluous exported function

[Takashi Iwai]

Since we've switched to the LED trigger for binding with HD-audio,
we can drop the exported function as well as the whole
linux/thinkpad_acpi.h.

The own TPACPI_LED_MUTE and TPACPI_LED_MICMUTE definitions are
replaced with the identical ones for LEDS, i.e. LED_AUDIO_MUTE and
LED_AUDIO_MICMUTE, respectively.  They are no longer needed as
referred only locally.

Signed-off-by: Takashi Iwai <tiwai-l3A5Bk7waGM@public.gmane.org>
---
 drivers/platform/x86/thinkpad_acpi.c | 30 ++++++++++------------------
 include/linux/thinkpad_acpi.h        | 16 ---------------
 2 files changed, 11 insertions(+), 35 deletions(-)
 delete mode 100644 include/linux/thinkpad_acpi.h

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 3c09afae1248..0113873dfd13 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -81,7 +81,6 @@
 #include <linux/acpi.h>
 #include <linux/pci_ids.h>
 #include <linux/power_supply.h>
-#include <linux/thinkpad_acpi.h>
 #include <sound/core.h>
 #include <sound/control.h>
 #include <sound/initval.h>
@@ -9150,6 +9149,7 @@ static struct ibm_struct fan_driver_data = {
  * Mute LED subdriver
  */
 
+#define TPACPI_LED_MAX		2
 
 struct tp_led_table {
 	acpi_string name;
@@ -9158,13 +9158,13 @@ struct tp_led_table {
 	int state;
 };
 
-static struct tp_led_table led_tables[] = {
-	[TPACPI_LED_MUTE] = {
+static struct tp_led_table led_tables[TPACPI_LED_MAX] = {
+	[LED_AUDIO_MUTE] = {
 		.name = "SSMS",
 		.on_value = 1,
 		.off_value = 0,
 	},
-	[TPACPI_LED_MICMUTE] = {
+	[LED_AUDIO_MICMUTE] = {
 		.name = "MMTS",
 		.on_value = 2,
 		.off_value = 0,
@@ -9189,40 +9189,36 @@ static int mute_led_on_off(struct tp_led_table *t, bool state)
 	return state;
 }
 
-int tpacpi_led_set(int whichled, bool on)
+static int tpacpi_led_set(int whichled, bool on)
 {
 	struct tp_led_table *t;
 
-	if (whichled < 0 || whichled >= TPACPI_LED_MAX)
-		return -EINVAL;
-
 	t = &led_tables[whichled];
 	if (t->state < 0 || t->state == on)
 		return t->state;
 	return mute_led_on_off(t, on);
 }
-EXPORT_SYMBOL_GPL(tpacpi_led_set);
 
 static int tpacpi_led_mute_set(struct led_classdev *led_cdev,
 			       enum led_brightness brightness)
 {
-	return tpacpi_led_set(TPACPI_LED_MUTE, brightness != LED_OFF);
+	return tpacpi_led_set(LED_AUDIO_MUTE, brightness != LED_OFF);
 }
 
 static int tpacpi_led_micmute_set(struct led_classdev *led_cdev,
 				  enum led_brightness brightness)
 {
-	return tpacpi_led_set(TPACPI_LED_MICMUTE, brightness != LED_OFF);
+	return tpacpi_led_set(LED_AUDIO_MICMUTE, brightness != LED_OFF);
 }
 
-static struct led_classdev mute_led_cdev[] = {
-	[TPACPI_LED_MUTE] = {
+static struct led_classdev mute_led_cdev[TPACPI_LED_MAX] = {
+	[LED_AUDIO_MUTE] = {
 		.name		= "tpacpi::mute",
 		.max_brightness = 1,
 		.brightness_set_blocking = tpacpi_led_mute_set,
 		.default_trigger = "audio-mute",
 	},
-	[TPACPI_LED_MICMUTE] = {
+	[LED_AUDIO_MICMUTE] = {
 		.name		= "tpacpi::micmute",
 		.max_brightness = 1,
 		.brightness_set_blocking = tpacpi_led_micmute_set,
@@ -9232,10 +9228,6 @@ static struct led_classdev mute_led_cdev[] = {
 
 static int mute_led_init(struct ibm_init_struct *iibm)
 {
-	static enum led_audio types[] = {
-		[TPACPI_LED_MUTE] = LED_AUDIO_MUTE,
-		[TPACPI_LED_MICMUTE] = LED_AUDIO_MICMUTE,
-	};
 	acpi_handle temp;
 	int i, err;
 
@@ -9246,7 +9238,7 @@ static int mute_led_init(struct ibm_init_struct *iibm)
 			continue;
 		}
 
-		mute_led_cdev[i].brightness = ledtrig_audio_get(types[i]);
+		mute_led_cdev[i].brightness = ledtrig_audio_get(i);
 		err = led_classdev_register(&tpacpi_pdev->dev, &mute_led_cdev[i]);
 		if (err < 0) {
 			while (--i >= 0)
diff --git a/include/linux/thinkpad_acpi.h b/include/linux/thinkpad_acpi.h
deleted file mode 100644
index 9fb317970c01..000000000000
--- a/include/linux/thinkpad_acpi.h
+++ /dev/null
@@ -1,16 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __THINKPAD_ACPI_H__
-#define __THINKPAD_ACPI_H__
-
-/* These two functions return 0 if success, or negative error code
-   (e g -ENODEV if no led present) */
-
-enum {
-	TPACPI_LED_MUTE,
-	TPACPI_LED_MICMUTE,
-	TPACPI_LED_MAX,
-};
-
-int tpacpi_led_set(int whichled, bool on);
-
-#endif
-- 
2.19.1

[PATCH 5/6] platform/x86: dell-laptop: Drop superfluous exported function

[Takashi Iwai]

Since we've switched to the LED trigger for binding with HD-audio,
we can drop the exported function as well as the whole
linux/dell-led.h.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/platform/x86/dell-laptop.c | 22 +++++-----------------
 include/linux/dell-led.h           |  7 -------
 2 files changed, 5 insertions(+), 24 deletions(-)
 delete mode 100644 include/linux/dell-led.h

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 6a647e2d09ee..40236895b263 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -29,7 +29,6 @@
 #include <linux/mm.h>
 #include <linux/i8042.h>
 #include <linux/debugfs.h>
-#include <linux/dell-led.h>
 #include <linux/seq_file.h>
 #include <acpi/video.h>
 #include "dell-rbtn.h"
@@ -2109,17 +2108,17 @@ static struct notifier_block dell_laptop_notifier = {
 	.notifier_call = dell_laptop_notifier_call,
 };
 
-int dell_micmute_led_set(int state)
+static int micmute_led_set(struct led_classdev *led_cdev,
+			   enum led_brightness brightness)
 {
 	struct calling_interface_buffer buffer;
 	struct calling_interface_token *token;
+	int state = brightness != LED_OFF;
 
 	if (state == 0)
 		token = dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE);
-	else if (state == 1)
-		token = dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE);
 	else
-		return -EINVAL;
+		token = dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE);
 
 	if (!token)
 		return -ENODEV;
@@ -2127,18 +2126,7 @@ int dell_micmute_led_set(int state)
 	dell_fill_request(&buffer, token->location, token->value, 0, 0);
 	dell_send_request(&buffer, CLASS_TOKEN_WRITE, SELECT_TOKEN_STD);
 
-	return state;
-}
-EXPORT_SYMBOL_GPL(dell_micmute_led_set);
-
-static int micmute_led_set(struct led_classdev *led_cdev,
-			   enum led_brightness brightness)
-{
-	int state = brightness != LED_OFF;
-	int err;
-
-	err = dell_micmute_led_set(state);
-	return err < 0 ? err : 0;
+	return 0;
 }
 
 static struct led_classdev micmute_led_cdev = {
diff --git a/include/linux/dell-led.h b/include/linux/dell-led.h
deleted file mode 100644
index 92521471517f..000000000000
--- a/include/linux/dell-led.h
+++ /dev/null
@@ -1,7 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __DELL_LED_H__
-#define __DELL_LED_H__
-
-int dell_micmute_led_set(int on);
-
-#endif
-- 
2.19.1

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Andy Shevchenko]

On Mon, Nov 26, 2018 at 7:14 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> Hi,
>
> this is patch series I've hacked after useful conversation with
> Pavel.  Basically this adds a new LED trigger audio-mute and
> audio-micmute, and convert the HD-audio driver and the platform
> drivers to use the LED trigger instead of the ugly direct dynamic
> symbol binding.
>
> The latest version of patches are found in topic/leds-trigger branch
> in my sound git tree.
>   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
>
> As these are cross-tree patches, the branch above is based cleanly on
> v4.20-rc3, so that it can be merged well to multiple trees.
>
> Once after getting the ACK's, I'll add tags and fixate for merges.
>
> This patch series don't include huawei-wmi stuff; so Huawei patches
> need rework.  I already have some piece of changes for huawei-wmi, so
> please ping me if needed.
>
> I checked briefly on my Dell laptop, and a Thinkpad model.
> Wider tests are appreciated, of course.
>

Few minor comments, otherwise I like this series.
Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
for PDx86 bits.

>
> thanks,
>
> Takashi
>
> ===
>
> Takashi Iwai (6):
>   leds: trigger: Introduce audio mute LED trigger
>   platform/x86: dell-laptop: Add micmute LED trigger support
>   platform/x86: thinkpad_acpi: Add audio mute LED classdev support
>   ALSA: hda - Support led audio trigger
>   platform/x86: dell-laptop: Drop superfluous exported function
>   platform/x86: thinkpad_acpi: Drop superfluous exported function
>
>  drivers/leds/trigger/Kconfig         |  7 +++
>  drivers/leds/trigger/Makefile        |  1 +
>  drivers/leds/trigger/ledtrig-audio.c | 45 +++++++++++++++++++
>  drivers/platform/x86/Kconfig         |  4 ++
>  drivers/platform/x86/dell-laptop.c   | 27 +++++++++---
>  drivers/platform/x86/thinkpad_acpi.c | 66 +++++++++++++++++++++-------
>  include/linux/dell-led.h             |  7 ---
>  include/linux/leds.h                 | 20 +++++++++
>  include/linux/thinkpad_acpi.h        | 16 -------
>  sound/pci/hda/dell_wmi_helper.c      | 48 --------------------
>  sound/pci/hda/hda_generic.c          | 31 +++++++++++++
>  sound/pci/hda/hda_generic.h          |  2 +
>  sound/pci/hda/patch_realtek.c        | 17 +++----
>  sound/pci/hda/thinkpad_helper.c      | 43 +++---------------
>  14 files changed, 194 insertions(+), 140 deletions(-)
>  create mode 100644 drivers/leds/trigger/ledtrig-audio.c
>  delete mode 100644 include/linux/dell-led.h
>  delete mode 100644 include/linux/thinkpad_acpi.h
>  delete mode 100644 sound/pci/hda/dell_wmi_helper.c
>
> --
> 2.19.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Takashi Iwai]

On Tue, 27 Nov 2018 00:09:17 +0100,
Andy Shevchenko wrote:
> 
> On Mon, Nov 26, 2018 at 7:14 PM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > Hi,
> >
> > this is patch series I've hacked after useful conversation with
> > Pavel.  Basically this adds a new LED trigger audio-mute and
> > audio-micmute, and convert the HD-audio driver and the platform
> > drivers to use the LED trigger instead of the ugly direct dynamic
> > symbol binding.
> >
> > The latest version of patches are found in topic/leds-trigger branch
> > in my sound git tree.
> >   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> >
> > As these are cross-tree patches, the branch above is based cleanly on
> > v4.20-rc3, so that it can be merged well to multiple trees.
> >
> > Once after getting the ACK's, I'll add tags and fixate for merges.
> >
> > This patch series don't include huawei-wmi stuff; so Huawei patches
> > need rework.  I already have some piece of changes for huawei-wmi, so
> > please ping me if needed.
> >
> > I checked briefly on my Dell laptop, and a Thinkpad model.
> > Wider tests are appreciated, of course.
> >
> 
> Few minor comments, otherwise I like this series.
> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> for PDx86 bits.

Thanks, added your ack to platform patches.

The latest patches are found in topic/leds-trigger branch of my sound
git tree.

I'm going to resubmit v2 series tomorrow or later, then merge the
branch to for-next branch.


Takashi

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1531 bytes --]

Hi!

> this is patch series I've hacked after useful conversation with
> Pavel.  Basically this adds a new LED trigger audio-mute and
> audio-micmute, and convert the HD-audio driver and the platform
> drivers to use the LED trigger instead of the ugly direct dynamic
> symbol binding.

Thanks a lot for doing this!

> The latest version of patches are found in topic/leds-trigger branch
> in my sound git tree.
>   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> 
> As these are cross-tree patches, the branch above is based cleanly on
> v4.20-rc3, so that it can be merged well to multiple trees.
> 
> Once after getting the ACK's, I'll add tags and fixate for merges.
> 
> This patch series don't include huawei-wmi stuff; so Huawei patches
> need rework.  I already have some piece of changes for huawei-wmi, so
> please ping me if needed.
> 
> I checked briefly on my Dell laptop, and a Thinkpad model.
> Wider tests are appreciated, of course.

Looks good... except one detail: you have "tpacpi::micmute" and
"dell::micmute". I know it follows "tradition", but we are trying to
fix that at the moment. Laptop micmute button is a laptop micmute
button, and userspace should not need to know what prefix to use
depending on vendor.

I'd suggest using "sys::micmute".

With that:

Acked-by: Pavel Machek <pavel@ucw.cz>

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Takashi Iwai]

On Tue, 27 Nov 2018 09:44:18 +0100,
Pavel Machek wrote:
> 
> Hi!
> 
> > this is patch series I've hacked after useful conversation with
> > Pavel.  Basically this adds a new LED trigger audio-mute and
> > audio-micmute, and convert the HD-audio driver and the platform
> > drivers to use the LED trigger instead of the ugly direct dynamic
> > symbol binding.
> 
> Thanks a lot for doing this!
> 
> > The latest version of patches are found in topic/leds-trigger branch
> > in my sound git tree.
> >   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> > 
> > As these are cross-tree patches, the branch above is based cleanly on
> > v4.20-rc3, so that it can be merged well to multiple trees.
> > 
> > Once after getting the ACK's, I'll add tags and fixate for merges.
> > 
> > This patch series don't include huawei-wmi stuff; so Huawei patches
> > need rework.  I already have some piece of changes for huawei-wmi, so
> > please ping me if needed.
> > 
> > I checked briefly on my Dell laptop, and a Thinkpad model.
> > Wider tests are appreciated, of course.
> 
> Looks good... except one detail: you have "tpacpi::micmute" and
> "dell::micmute". I know it follows "tradition", but we are trying to
> fix that at the moment. Laptop micmute button is a laptop micmute
> button, and userspace should not need to know what prefix to use
> depending on vendor.
> 
> I'd suggest using "sys::micmute".

OK, replaced them, and also add explanation in the patch logs.

> With that:
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>

Added now to the series in topic/leds-trigger branch.

I'm going to resubmit v2 series tomorrow or later, then merge the
branch to for-next branch.


Thanks!

Takashi

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Pali Rohár]

On Tuesday 27 November 2018 09:44:18 Pavel Machek wrote:
> Looks good... except one detail: you have "tpacpi::micmute" and
> "dell::micmute". I know it follows "tradition", but we are trying to
> fix that at the moment. Laptop micmute button is a laptop micmute
> button, and userspace should not need to know what prefix to use
> depending on vendor.
> 
> I'd suggest using "sys::micmute".

I can imagine that in future some devices like keyboards would have also
mute led. We already have keyboards with mute key, so it is something
not unrealistic. What should be name convention for these mute leds?

Is not "sys::" prefix too generic?

-- 
Pali Rohár
pali.rohar@gmail.com
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Takashi Iwai]

On Wed, 28 Nov 2018 12:18:06 +0100,
Pali Rohár wrote:
> 
> On Tuesday 27 November 2018 09:44:18 Pavel Machek wrote:
> > Looks good... except one detail: you have "tpacpi::micmute" and
> > "dell::micmute". I know it follows "tradition", but we are trying to
> > fix that at the moment. Laptop micmute button is a laptop micmute
> > button, and userspace should not need to know what prefix to use
> > depending on vendor.
> > 
> > I'd suggest using "sys::micmute".
> 
> I can imagine that in future some devices like keyboards would have also
> mute led. We already have keyboards with mute key, so it is something
> not unrealistic. What should be name convention for these mute leds?
> 
> Is not "sys::" prefix too generic?

Good point.  I thought of "laptop::" but it's not always laptop.
"builtin::"?  Doesn't sound great, either.

A nice godfather is required here...


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1465 bytes --]

On Wed 2018-11-28 12:38:19, Takashi Iwai wrote:
> On Wed, 28 Nov 2018 12:18:06 +0100,
> Pali Rohár wrote:
> > 
> > On Tuesday 27 November 2018 09:44:18 Pavel Machek wrote:
> > > Looks good... except one detail: you have "tpacpi::micmute" and
> > > "dell::micmute". I know it follows "tradition", but we are trying to
> > > fix that at the moment. Laptop micmute button is a laptop micmute
> > > button, and userspace should not need to know what prefix to use
> > > depending on vendor.
> > > 
> > > I'd suggest using "sys::micmute".
> > 
> > I can imagine that in future some devices like keyboards would have also
> > mute led. We already have keyboards with mute key, so it is something
> > not unrealistic. What should be name convention for these mute leds?
> > 
> > Is not "sys::" prefix too generic?
> 
> Good point.  I thought of "laptop::" but it's not always laptop.
> "builtin::"?  Doesn't sound great, either.
> 
> A nice godfather is required here...

Just use sys:: :-).

laptop:: would work for me, too. (It is always laptop in the cases we
are handling now, right?)

When we get a keyboard with mute led, we'll have to decide if it
should be input6::mute -- because it is on keyboard, or if it is
sys::mute -- because the key is expected to mute whole system.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Takashi Iwai]

On Wed, 28 Nov 2018 13:25:05 +0100,
Pavel Machek wrote:
> 
> On Wed 2018-11-28 12:38:19, Takashi Iwai wrote:
> > On Wed, 28 Nov 2018 12:18:06 +0100,
> > Pali Rohár wrote:
> > > 
> > > On Tuesday 27 November 2018 09:44:18 Pavel Machek wrote:
> > > > Looks good... except one detail: you have "tpacpi::micmute" and
> > > > "dell::micmute". I know it follows "tradition", but we are trying to
> > > > fix that at the moment. Laptop micmute button is a laptop micmute
> > > > button, and userspace should not need to know what prefix to use
> > > > depending on vendor.
> > > > 
> > > > I'd suggest using "sys::micmute".
> > > 
> > > I can imagine that in future some devices like keyboards would have also
> > > mute led. We already have keyboards with mute key, so it is something
> > > not unrealistic. What should be name convention for these mute leds?
> > > 
> > > Is not "sys::" prefix too generic?
> > 
> > Good point.  I thought of "laptop::" but it's not always laptop.
> > "builtin::"?  Doesn't sound great, either.
> > 
> > A nice godfather is required here...
> 
> Just use sys:: :-).
> 
> laptop:: would work for me, too. (It is always laptop in the cases we
> are handling now, right?)

In theory, such a thing can be on all-in-one desktops, too.

> When we get a keyboard with mute led, we'll have to decide if it
> should be input6::mute -- because it is on keyboard, or if it is
> sys::mute -- because the key is expected to mute whole system.

OK, I'll wait for more comments in today and update accordingly.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Andy Shevchenko]

On Wed, Nov 28, 2018 at 2:59 PM Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 28 Nov 2018 13:25:05 +0100,
> > On Wed 2018-11-28 12:38:19, Takashi Iwai wrote:

> > Just use sys:: :-).
> >
> > laptop:: would work for me, too. (It is always laptop in the cases we
> > are handling now, right?)
>
> In theory, such a thing can be on all-in-one desktops, too.
>
> > When we get a keyboard with mute led, we'll have to decide if it
> > should be input6::mute -- because it is on keyboard, or if it is
> > sys::mute -- because the key is expected to mute whole system.
>
> OK, I'll wait for more comments in today and update accordingly.

sys sounds plausible to me. In any case we may revisit in the future
if something happens to it. OTOH it would be part of ABI, right?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Jacek Anaszewski]

On 11/28/2018 01:25 PM, Pavel Machek wrote:
> On Wed 2018-11-28 12:38:19, Takashi Iwai wrote:
>> On Wed, 28 Nov 2018 12:18:06 +0100,
>> Pali Rohár wrote:
>>>
>>> On Tuesday 27 November 2018 09:44:18 Pavel Machek wrote:
>>>> Looks good... except one detail: you have "tpacpi::micmute" and
>>>> "dell::micmute". I know it follows "tradition", but we are trying to
>>>> fix that at the moment. Laptop micmute button is a laptop micmute
>>>> button, and userspace should not need to know what prefix to use
>>>> depending on vendor.
>>>>
>>>> I'd suggest using "sys::micmute".
>>>
>>> I can imagine that in future some devices like keyboards would have also
>>> mute led. We already have keyboards with mute key, so it is something
>>> not unrealistic. What should be name convention for these mute leds?
>>>
>>> Is not "sys::" prefix too generic?
>>
>> Good point.  I thought of "laptop::" but it's not always laptop.
>> "builtin::"?  Doesn't sound great, either.
>>
>> A nice godfather is required here...
> 
> Just use sys:: :-).
> 
> laptop:: would work for me, too. (It is always laptop in the cases we
> are handling now, right?)
> 
> When we get a keyboard with mute led, we'll have to decide if it
> should be input6::mute -- because it is on keyboard, or if it is
> sys::mute -- because the key is expected to mute whole system.

drivers/input/input-leds.c seems to already support mute LED.
It will be exposed as inputN::mute.

Documentation/leds/leds-class.txt defines LED naming pattern
to <devicename:color:function> and "sys" does not look as
something resembling device name.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1823 bytes --]

Hi!

> >>>> Looks good... except one detail: you have "tpacpi::micmute" and
> >>>> "dell::micmute". I know it follows "tradition", but we are trying to
> >>>> fix that at the moment. Laptop micmute button is a laptop micmute
> >>>> button, and userspace should not need to know what prefix to use
> >>>> depending on vendor.
> >>>>
> >>>> I'd suggest using "sys::micmute".
> >>>
> >>> I can imagine that in future some devices like keyboards would have also
> >>> mute led. We already have keyboards with mute key, so it is something
> >>> not unrealistic. What should be name convention for these mute leds?
> >>>
> >>> Is not "sys::" prefix too generic?
> >>
> >> Good point.  I thought of "laptop::" but it's not always laptop.
> >> "builtin::"?  Doesn't sound great, either.
> >>
> >> A nice godfather is required here...
> > 
> > Just use sys:: :-).
> > 
> > laptop:: would work for me, too. (It is always laptop in the cases we
> > are handling now, right?)
> > 
> > When we get a keyboard with mute led, we'll have to decide if it
> > should be input6::mute -- because it is on keyboard, or if it is
> > sys::mute -- because the key is expected to mute whole system.
> 
> drivers/input/input-leds.c seems to already support mute LED.
> It will be exposed as inputN::mute.
> 
> Documentation/leds/leds-class.txt defines LED naming pattern
> to <devicename:color:function> and "sys" does not look as
> something resembling device name.

So what is your suggestion?

I don't care much as long as it is same in tpacpi and dell
case. (Neither are device names, btw :-).

Actually "::mute" would make sense, too.
									
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Pali Rohár]

[-- Attachment #1.1: Type: text/plain, Size: 2821 bytes --]

On Wednesday 28 November 2018 21:34:10 Pavel Machek wrote:
> Hi!
> 
> > >>>> Looks good... except one detail: you have "tpacpi::micmute" and
> > >>>> "dell::micmute". I know it follows "tradition", but we are trying to
> > >>>> fix that at the moment. Laptop micmute button is a laptop micmute
> > >>>> button, and userspace should not need to know what prefix to use
> > >>>> depending on vendor.
> > >>>>
> > >>>> I'd suggest using "sys::micmute".
> > >>>
> > >>> I can imagine that in future some devices like keyboards would have also
> > >>> mute led. We already have keyboards with mute key, so it is something
> > >>> not unrealistic. What should be name convention for these mute leds?
> > >>>
> > >>> Is not "sys::" prefix too generic?
> > >>
> > >> Good point.  I thought of "laptop::" but it's not always laptop.
> > >> "builtin::"?  Doesn't sound great, either.
> > >>
> > >> A nice godfather is required here...
> > > 
> > > Just use sys:: :-).
> > > 
> > > laptop:: would work for me, too. (It is always laptop in the cases we
> > > are handling now, right?)
> > > 
> > > When we get a keyboard with mute led, we'll have to decide if it
> > > should be input6::mute -- because it is on keyboard, or if it is
> > > sys::mute -- because the key is expected to mute whole system.
> > 
> > drivers/input/input-leds.c seems to already support mute LED.
> > It will be exposed as inputN::mute.
> > 
> > Documentation/leds/leds-class.txt defines LED naming pattern
> > to <devicename:color:function> and "sys" does not look as
> > something resembling device name.
> 
> So what is your suggestion?

I guess we should follow documentation. Or update documentation if it
does not make sense to follow it.

> I don't care much as long as it is same in tpacpi and dell
> case. (Neither are device names, btw :-).
> 
> Actually "::mute" would make sense, too.

"::mute" is not a good idea due to name uniqueness.

In case you would have two drivers which both provides "mute" led, then
they need to have different name. Reason also why generic name "sys" is
not a good idea.

input subsystem seems to solved this problem by appending number after
"input" word.

I think that driver name or subsystem name would be usable together with
number.

Userspace application would be probably interested to distinguish
between "mute led which is part of laptop" and "mute led which is
available on external USB keyboard".

If external USB keyboard is identified as "input7" device, then
"input7::mute" is a good name for mute key. But "sys::mute" does not say
anything to which device or hardware it belongs nor does not solve
problem that which device/driver/subsystem should have privilege to take
this "sys" name.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Jacek Anaszewski]

On 11/28/2018 09:46 PM, Pali Rohár wrote:
> On Wednesday 28 November 2018 21:34:10 Pavel Machek wrote:
>> Hi!
>>
>>>>>>> Looks good... except one detail: you have "tpacpi::micmute" and
>>>>>>> "dell::micmute". I know it follows "tradition", but we are trying to
>>>>>>> fix that at the moment. Laptop micmute button is a laptop micmute
>>>>>>> button, and userspace should not need to know what prefix to use
>>>>>>> depending on vendor.
>>>>>>>
>>>>>>> I'd suggest using "sys::micmute".
>>>>>>
>>>>>> I can imagine that in future some devices like keyboards would have also
>>>>>> mute led. We already have keyboards with mute key, so it is something
>>>>>> not unrealistic. What should be name convention for these mute leds?
>>>>>>
>>>>>> Is not "sys::" prefix too generic?
>>>>>
>>>>> Good point.  I thought of "laptop::" but it's not always laptop.
>>>>> "builtin::"?  Doesn't sound great, either.
>>>>>
>>>>> A nice godfather is required here...
>>>>
>>>> Just use sys:: :-).
>>>>
>>>> laptop:: would work for me, too. (It is always laptop in the cases we
>>>> are handling now, right?)
>>>>
>>>> When we get a keyboard with mute led, we'll have to decide if it
>>>> should be input6::mute -- because it is on keyboard, or if it is
>>>> sys::mute -- because the key is expected to mute whole system.
>>>
>>> drivers/input/input-leds.c seems to already support mute LED.
>>> It will be exposed as inputN::mute.
>>>
>>> Documentation/leds/leds-class.txt defines LED naming pattern
>>> to <devicename:color:function> and "sys" does not look as
>>> something resembling device name.
>>
>> So what is your suggestion?
> 
> I guess we should follow documentation. Or update documentation if it
> does not make sense to follow it.
> 
>> I don't care much as long as it is same in tpacpi and dell
>> case. (Neither are device names, btw :-).
>>
>> Actually "::mute" would make sense, too.
> 
> "::mute" is not a good idea due to name uniqueness.

LED core adds a numerical suffix to the original name
if it is already taken. Of course it is a last resort just
to avoid name clash.

> In case you would have two drivers which both provides "mute" led, then
> they need to have different name. Reason also why generic name "sys" is
> not a good idea.
> 
> input subsystem seems to solved this problem by appending number after
> "input" word.
> 
> I think that driver name or subsystem name would be usable together with
> number.
> 
> Userspace application would be probably interested to distinguish
> between "mute led which is part of laptop" and "mute led which is
> available on external USB keyboard".
> 
> If external USB keyboard is identified as "input7" device, then
> "input7::mute" is a good name for mute key. But "sys::mute" does not say
> anything to which device or hardware it belongs nor does not solve
> problem that which device/driver/subsystem should have privilege to take
> this "sys" name.

How about just "platform" for the LEDs being part of the device
on which the system is running?

-- 
Best regards,
Jacek Anaszewski
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 900 bytes --]

Hi!

> > If external USB keyboard is identified as "input7" device, then
> > "input7::mute" is a good name for mute key. But "sys::mute" does not say
> > anything to which device or hardware it belongs nor does not solve
> > problem that which device/driver/subsystem should have privilege to take
> > this "sys" name.
> 
> How about just "platform" for the LEDs being part of the device
> on which the system is running?

"platform" works for me.

Are we in agreement that this name will be used for all similar LEDs,
as long as they are on the "main box" of the device, no matter if they
are connected using acpi, gpio, i2c, ...?

Should we start Documentation file explaining common names we want
people to use?

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Jacek Anaszewski]

On 11/28/2018 10:22 PM, Pavel Machek wrote:
> Hi!
> 
>>> If external USB keyboard is identified as "input7" device, then
>>> "input7::mute" is a good name for mute key. But "sys::mute" does not say
>>> anything to which device or hardware it belongs nor does not solve
>>> problem that which device/driver/subsystem should have privilege to take
>>> this "sys" name.
>>
>> How about just "platform" for the LEDs being part of the device
>> on which the system is running?
> 
> "platform" works for me.
> 
> Are we in agreement that this name will be used for all similar LEDs,
> as long as they are on the "main box" of the device, no matter if they
> are connected using acpi, gpio, i2c, ...?

One doubt: say we have hdd activity LED on the "main box" - it
would be named "platform::disk".

Now, we add external USB disk. Previously we were considering
the naming for disk LEDs in the form e.g. "sdb::disk".

If so, there would be discrepancy between internal and external disk
LED names. Similarly in case of eth/adsl/wlan/camera LEDs.

Regarding sound devices - how would we name micmute LED for USB
microphone? I suppose that it is possible to discover some unique
identifier in runtime - this is a question to ALSA people.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Takashi Iwai]

On Thu, 29 Nov 2018 22:23:31 +0100,
Jacek Anaszewski wrote:
> 
> On 11/28/2018 10:22 PM, Pavel Machek wrote:
> > Hi!
> > 
> >>> If external USB keyboard is identified as "input7" device, then
> >>> "input7::mute" is a good name for mute key. But "sys::mute" does not say
> >>> anything to which device or hardware it belongs nor does not solve
> >>> problem that which device/driver/subsystem should have privilege to take
> >>> this "sys" name.
> >>
> >> How about just "platform" for the LEDs being part of the device
> >> on which the system is running?
> > 
> > "platform" works for me.
> > 
> > Are we in agreement that this name will be used for all similar LEDs,
> > as long as they are on the "main box" of the device, no matter if they
> > are connected using acpi, gpio, i2c, ...?
> 
> One doubt: say we have hdd activity LED on the "main box" - it
> would be named "platform::disk".
> 
> Now, we add external USB disk. Previously we were considering
> the naming for disk LEDs in the form e.g. "sdb::disk".
> 
> If so, there would be discrepancy between internal and external disk
> LED names. Similarly in case of eth/adsl/wlan/camera LEDs.
> 
> Regarding sound devices - how would we name micmute LED for USB
> microphone? I suppose that it is possible to discover some unique
> identifier in runtime - this is a question to ALSA people.

There are little USB-audio devices supporting the LED control, so we
haven't had any chance for LED class for USB-audio.

At most, we may create LED class devices for some HD-audio (usually
driven via exotic verbs or GPIO over HD-audio), but I wouldn't go in
that direction; it'll need too make things more complex than the
direct hook as of now.


thanks,

Takashi

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Andy Shevchenko]

On Thu, Nov 29, 2018 at 10:23:31PM +0100, Jacek Anaszewski wrote:

> Regarding sound devices - how would we name micmute LED for USB
> microphone? I suppose that it is possible to discover some unique
> identifier in runtime - this is a question to ALSA people.

USB path (e.g. 2-1.1.2.4.3) + '::' + name ?

-- 
With Best Regards,
Andy Shevchenko

Well-known LED names was Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 3337 bytes --]

Hi!

> >>> If external USB keyboard is identified as "input7" device, then
> >>> "input7::mute" is a good name for mute key. But "sys::mute" does not say
> >>> anything to which device or hardware it belongs nor does not solve
> >>> problem that which device/driver/subsystem should have privilege to take
> >>> this "sys" name.
> >>
> >> How about just "platform" for the LEDs being part of the device
> >> on which the system is running?
> > 
> > "platform" works for me.
> > 
> > Are we in agreement that this name will be used for all similar LEDs,
> > as long as they are on the "main box" of the device, no matter if they
> > are connected using acpi, gpio, i2c, ...?
> 
> One doubt: say we have hdd activity LED on the "main box" - it
> would be named "platform::disk".
> 
> Now, we add external USB disk. Previously we were considering
> the naming for disk LEDs in the form e.g. "sdb::disk".
> 
> If so, there would be discrepancy between internal and external disk
> LED names. Similarly in case of eth/adsl/wlan/camera LEDs.

Well, it really depends if the "plaform::disk" is for all the disks,
or if it is for sda. In the second case, it might be better to name it
sda::disk...

Anyway... I believe we should start documenting good and bad LEDs, so
that patch authors know what is there, and can try to be consistent,
and so that userland knows what names to probe for.

What about following? Any other LEDs worth mentioning?

commit c56708addf9c312cefd760bca218a0545258b217
Author: Pavel <pavel@ucw.cz>
Date:   Sat Dec 1 15:32:13 2018 +0100

leds: Add list of well-known LED names
    
Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/Documentation/leds/well-known-leds.txt b/Documentation/leds/well-known-leds.txt
new file mode 100644
index 0000000..9a262db
--- /dev/null
+++ b/Documentation/leds/well-known-leds.txt
@@ -0,0 +1,42 @@
+-*- org -*-
+
+It is somehow important to provide consistent interface to the
+userland. LED devices have one problem there, and that is naming of
+directories in /sys/class/leds. It would be nice if userland would
+just know right "name" for given LED function, but situation got more
+complex.
+
+Anyway, if backwards compatibility is not an issue, new code should
+use one of the "good" names from this list, and you should extend the
+list where applicable.
+
+Bad names are listed, too, in case you are writing application that
+wants to use particular feature, you should probe for good name first
+but then try the bad ones, too".
+
+* Keyboards
+
+Good: "input*:*:capslock"
+Good: "input*:*:scrolllock"
+Good: "input*:*:numlock"
+
+Set of common keyboard LEDs, going back to PC AT or so.
+
+Bad: "tpacpi::thinklight" (IBM/Lenovo Thinkpads)
+Bad: "lp5523:kb{1,2,3,4,5,6,7}" (Nokia N900)
+
+Keyboard frontlight/backlight.
+
+* Sound subsystem
+
+Good: "platform:*:mute"
+Good: "platform:*:micmute"
+
+LEDs on notebook body, indicating that sound input / output is muted.
+
+* System notification
+
+Good: "status-led:{red,green,blue}" (Motorola Droid 4)
+Bad: "lp5523:{r,g,b}" (Nokia N900)
+
+Phones usually have multi-color status LED.



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Well-known LED names was Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Jacek Anaszewski]

Hi Pavel,

On 12/1/18 3:41 PM, Pavel Machek wrote:
> Hi!
> 
>>>>> If external USB keyboard is identified as "input7" device, then
>>>>> "input7::mute" is a good name for mute key. But "sys::mute" does not say
>>>>> anything to which device or hardware it belongs nor does not solve
>>>>> problem that which device/driver/subsystem should have privilege to take
>>>>> this "sys" name.
>>>>
>>>> How about just "platform" for the LEDs being part of the device
>>>> on which the system is running?
>>>
>>> "platform" works for me.
>>>
>>> Are we in agreement that this name will be used for all similar LEDs,
>>> as long as they are on the "main box" of the device, no matter if they
>>> are connected using acpi, gpio, i2c, ...?
>>
>> One doubt: say we have hdd activity LED on the "main box" - it
>> would be named "platform::disk".
>>
>> Now, we add external USB disk. Previously we were considering
>> the naming for disk LEDs in the form e.g. "sdb::disk".
>>
>> If so, there would be discrepancy between internal and external disk
>> LED names. Similarly in case of eth/adsl/wlan/camera LEDs.
> 
> Well, it really depends if the "plaform::disk" is for all the disks,
> or if it is for sda. In the second case, it might be better to name it
> sda::disk...
> 
> Anyway... I believe we should start documenting good and bad LEDs, so
> that patch authors know what is there, and can try to be consistent,
> and so that userland knows what names to probe for.

The idea looks reasonable. In addition to that, I'd change one
thing in the LED naming pattern - replace "devicename " with
"affiliation". AFAICS that would fit best for both "platform"
and e.g. "input*".

Also in the DT we would need related "affiliation" property.
In most cases it would be "platform" probably.
Possible is also "camera" - but we would have to state it clear
how to proceed in case of video devices - shouldn't v4l2
drivers create LEDs similarly to how network drivers do that.
Then it would be possible to provide videoN name.

> What about following? Any other LEDs worth mentioning?
disk: sdX
network devices: ethN, adslN

These "affiliations" would have to be provided by the
network drivers creating LED class devices.

> commit c56708addf9c312cefd760bca218a0545258b217
> Author: Pavel <pavel@ucw.cz>
> Date:   Sat Dec 1 15:32:13 2018 +0100
> 
> leds: Add list of well-known LED names
>      
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/Documentation/leds/well-known-leds.txt b/Documentation/leds/well-known-leds.txt
> new file mode 100644
> index 0000000..9a262db
> --- /dev/null
> +++ b/Documentation/leds/well-known-leds.txt
> @@ -0,0 +1,42 @@
> +-*- org -*-
> +
> +It is somehow important to provide consistent interface to the
> +userland. LED devices have one problem there, and that is naming of
> +directories in /sys/class/leds. It would be nice if userland would
> +just know right "name" for given LED function, but situation got more
> +complex.
> +
> +Anyway, if backwards compatibility is not an issue, new code should
> +use one of the "good" names from this list, and you should extend the
> +list where applicable.
> +
> +Bad names are listed, too, in case you are writing application that
> +wants to use particular feature, you should probe for good name first
> +but then try the bad ones, too".
> +
> +* Keyboards
> +
> +Good: "input*:*:capslock"
> +Good: "input*:*:scrolllock"
> +Good: "input*:*:numlock"
> +
> +Set of common keyboard LEDs, going back to PC AT or so.
> +
> +Bad: "tpacpi::thinklight" (IBM/Lenovo Thinkpads)
> +Bad: "lp5523:kb{1,2,3,4,5,6,7}" (Nokia N900)
> +
> +Keyboard frontlight/backlight.

Isn't example missing here?

> +
> +* Sound subsystem
> +
> +Good: "platform:*:mute"
> +Good: "platform:*:micmute"
> +
> +LEDs on notebook body, indicating that sound input / output is muted.
> +
> +* System notification
> +
> +Good: "status-led:{red,green,blue}" (Motorola Droid 4)

Two problems here:
- "status-led" is in place of "devicename" instead of "function"
- "-led" is obvious and non-generic - we will have "status"
   in the "functions.h"

> +Bad: "lp5523:{r,g,b}" (Nokia N900)
> +
> +Phones usually have multi-color status LED.
> 
> 
>

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 2346 bytes --]

Hi!

> > > >> A nice godfather is required here...
> > > > 
> > > > Just use sys:: :-).
> > > > 
> > > > laptop:: would work for me, too. (It is always laptop in the cases we
> > > > are handling now, right?)
> > > > 
> > > > When we get a keyboard with mute led, we'll have to decide if it
> > > > should be input6::mute -- because it is on keyboard, or if it is
> > > > sys::mute -- because the key is expected to mute whole system.
> > > 
> > > drivers/input/input-leds.c seems to already support mute LED.
> > > It will be exposed as inputN::mute.
> > > 
> > > Documentation/leds/leds-class.txt defines LED naming pattern
> > > to <devicename:color:function> and "sys" does not look as
> > > something resembling device name.
> > 
> > So what is your suggestion?
> 
> I guess we should follow documentation. Or update documentation if it
> does not make sense to follow it.

That's actually in progress. Let me and Jacek deal with it. We don't
need great "how to name a LED" discussion here (google: bike shed paiting).

> > I don't care much as long as it is same in tpacpi and dell
> > case. (Neither are device names, btw :-).
> > 
> > Actually "::mute" would make sense, too.
> 
> "::mute" is not a good idea due to name uniqueness.
> 
> In case you would have two drivers which both provides "mute" led, then
> they need to have different name. Reason also why generic name "sys" is
> not a good idea.

> I think that driver name or subsystem name would be usable together with
> number.
> 
> Userspace application would be probably interested to distinguish
> between "mute led which is part of laptop" and "mute led which is
> available on external USB keyboard".
> 
> If external USB keyboard is identified as "input7" device, then
> "input7::mute" is a good name for mute key. But "sys::mute" does not say
> anything to which device or hardware it belongs nor does not solve
> problem that which device/driver/subsystem should have privilege to take
> this "sys" name.

Well, "sys" says this is system-wide mute LED. There are not expected
to be two of those, and there never will be, with the drivers
currently proposed.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [ibm-acpi-devel] [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Henrique de Moraes Holschuh]

On Mon, 26 Nov 2018, Takashi Iwai wrote:
> this is patch series I've hacked after useful conversation with
> Pavel.  Basically this adds a new LED trigger audio-mute and
> audio-micmute, and convert the HD-audio driver and the platform
> drivers to use the LED trigger instead of the ugly direct dynamic
> symbol binding.

For the thinkpad-acpi bits, provided that Andy's comments are addressed:
Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

> I checked briefly on my Dell laptop, and a Thinkpad model.

Thanks :-)

-- 
  Henrique Holschuh

Re: [ibm-acpi-devel] [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Takashi Iwai]

On Tue, 27 Nov 2018 11:26:32 +0100,
Henrique de Moraes Holschuh wrote:
> 
> On Mon, 26 Nov 2018, Takashi Iwai wrote:
> > this is patch series I've hacked after useful conversation with
> > Pavel.  Basically this adds a new LED trigger audio-mute and
> > audio-micmute, and convert the HD-audio driver and the platform
> > drivers to use the LED trigger instead of the ugly direct dynamic
> > symbol binding.
> 
> For the thinkpad-acpi bits, provided that Andy's comments are addressed:
> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

Thanks, added now to topic/leds-trigger branch.

I'm going to resubmit v2 series tomorrow or later, then merge the
branch to for-next branch.


Takashi

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Pali Rohár]

On Monday 26 November 2018 18:11:20 Takashi Iwai wrote:
> Hi,
> 
> this is patch series I've hacked after useful conversation with
> Pavel.  Basically this adds a new LED trigger audio-mute and
> audio-micmute, and convert the HD-audio driver and the platform
> drivers to use the LED trigger instead of the ugly direct dynamic
> symbol binding.
> 
> The latest version of patches are found in topic/leds-trigger branch
> in my sound git tree.
>   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> 
> As these are cross-tree patches, the branch above is based cleanly on
> v4.20-rc3, so that it can be merged well to multiple trees.
> 
> Once after getting the ACK's, I'll add tags and fixate for merges.
> 
> This patch series don't include huawei-wmi stuff; so Huawei patches
> need rework.  I already have some piece of changes for huawei-wmi, so
> please ping me if needed.
> 
> I checked briefly on my Dell laptop, and a Thinkpad model.
> Wider tests are appreciated, of course.

Hi! Thanks for patch series. It is really improvement in current mute
led situation. I quickly looked at patches and they look good. So you
can add my Acked-By: Pali Rohár <pali.rohar@gmail.com>.

-- 
Pali Rohár
pali.rohar@gmail.com
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 0/6] Introduce audio-mute LED trigger (and conversions to it)

[Takashi Iwai]

On Wed, 28 Nov 2018 12:14:34 +0100,
Pali Rohár wrote:
> 
> On Monday 26 November 2018 18:11:20 Takashi Iwai wrote:
> > Hi,
> > 
> > this is patch series I've hacked after useful conversation with
> > Pavel.  Basically this adds a new LED trigger audio-mute and
> > audio-micmute, and convert the HD-audio driver and the platform
> > drivers to use the LED trigger instead of the ugly direct dynamic
> > symbol binding.
> > 
> > The latest version of patches are found in topic/leds-trigger branch
> > in my sound git tree.
> >   git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> > 
> > As these are cross-tree patches, the branch above is based cleanly on
> > v4.20-rc3, so that it can be merged well to multiple trees.
> > 
> > Once after getting the ACK's, I'll add tags and fixate for merges.
> > 
> > This patch series don't include huawei-wmi stuff; so Huawei patches
> > need rework.  I already have some piece of changes for huawei-wmi, so
> > please ping me if needed.
> > 
> > I checked briefly on my Dell laptop, and a Thinkpad model.
> > Wider tests are appreciated, of course.
> 
> Hi! Thanks for patch series. It is really improvement in current mute
> led situation. I quickly looked at patches and they look good. So you
> can add my Acked-By: Pali Rohár <pali.rohar@gmail.com>.

Added your ack now.  Thanks!


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel