[PATCH v3 0/3] Huawei laptops

[PATCH v3 0/3] Huawei laptops

[Ayman Bagabas]

Changes from v2:
* Support for Huawei MBX
* Style and formating issues

[PATCH v3 1/3] 
The first patch adds support for missing hotkeys on some models.

[PATCH v3 2/3]
This one enables the front speakers on the Huawei Matebook X Pro (MBXP). This
solves bug 200501 https://bugzilla.kernel.org/show_bug.cgi?id=200501
It simply uses the pins configurations generated by hdajackretast using the
settings posted on the bug page https://imgur.com/a/N1xsCVZ

[PATCH v3 3/3]
This enables the micmute LED on Huawei laptops using ACPI calls.

Ayman Bagabas (3):
  x86: add support for Huawei WMI hotkeys.
  ALSA: hda: fix front speakers on Huawei MBXP.
  ALSA: hda: add support for Huawei WMI micmute LED

 drivers/platform/x86/Kconfig                 |  11 +
 drivers/platform/x86/Makefile                |   1 +
 drivers/platform/x86/huawei_wmi.c            | 236 +++++++++++++++++++
 include/linux/platform_data/x86/huawei_wmi.h |   9 +
 sound/pci/hda/huawei_wmi_helper.c            |  47 ++++
 sound/pci/hda/patch_realtek.c                |  29 +++
 6 files changed, 333 insertions(+)
 create mode 100644 drivers/platform/x86/huawei_wmi.c
 create mode 100644 include/linux/platform_data/x86/huawei_wmi.h
 create mode 100644 sound/pci/hda/huawei_wmi_helper.c

-- 
2.17.2

[PATCH v3 1/3] x86: add support for Huawei WMI hotkeys.

[Ayman Bagabas]

This driver adds support for missing hotkeys on some Huawei laptops.
Currently, only Huawei Matebook X and Matebook X Pro is supported.

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
---
 drivers/platform/x86/Kconfig      |  11 ++
 drivers/platform/x86/Makefile     |   1 +
 drivers/platform/x86/huawei_wmi.c | 235 ++++++++++++++++++++++++++++++
 3 files changed, 247 insertions(+)
 create mode 100644 drivers/platform/x86/huawei_wmi.c

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 54f6a40c75c6..b45c1294df7e 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1288,6 +1288,17 @@ config INTEL_ATOMISP2_PM
 	  To compile this driver as a module, choose M here: the module
 	  will be called intel_atomisp2_pm.
 
+config HUAWEI_LAPTOP
+	tristate "Huawei WMI hotkeys driver"
+	depends on ACPI
+	depends on ACPI_WMI
+	depends on INPUT
+	select INPUT_SPARSEKMAP
+	help
+	  This driver provides support for Huawei WMI hotkeys.
+	  It enables the missing keys and adds support to the micmute
+	  LED found on some of these laptops.
+
 endif # X86_PLATFORM_DEVICES
 
 config PMC_ATOM
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 39ae94135406..ee93655d8bc1 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_ACERHDF)		+= acerhdf.o
 obj-$(CONFIG_HP_ACCEL)		+= hp_accel.o
 obj-$(CONFIG_HP_WIRELESS)	+= hp-wireless.o
 obj-$(CONFIG_HP_WMI)		+= hp-wmi.o
+obj-$(CONFIG_HUAWEI_LAPTOP)		+= huawei_wmi.o
 obj-$(CONFIG_AMILO_RFKILL)	+= amilo-rfkill.o
 obj-$(CONFIG_GPD_POCKET_FAN)	+= gpd-pocket-fan.o
 obj-$(CONFIG_TC1100_WMI)	+= tc1100-wmi.o
diff --git a/drivers/platform/x86/huawei_wmi.c b/drivers/platform/x86/huawei_wmi.c
new file mode 100644
index 000000000000..658c44ab2126
--- /dev/null
+++ b/drivers/platform/x86/huawei_wmi.c
@@ -0,0 +1,235 @@
+/*
+ *  Huawei WMI hotkeys
+ *
+ *  Copyright (C) 2018	      Ayman Bagabas <ayman.bagabas@gmail.com>
+ *
+ *  This program is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/init.h>
+#include <linux/input.h>
+#include <linux/input/sparse-keymap.h>
+#include <linux/module.h>
+
+MODULE_AUTHOR("Ayman Bagabas <ayman.bagabas@gmail.com>");
+MODULE_DESCRIPTION("Huawei WMI hotkeys");
+MODULE_LICENSE("GPL");
+
+/*
+ * Huawei WMI Events GUIDs
+ */
+#define MBX_EVENT_GUID "59142400-C6A3-40fa-BADB-8A2652834100"
+#define MBXP_EVENT_GUID "ABBC0F5C-8EA1-11D1-A000-C90629100000"
+
+MODULE_ALIAS("wmi:"MBX_EVENT_GUID);
+MODULE_ALIAS("wmi:"MBXP_EVENT_GUID);
+
+static const struct key_entry huawei_wmi_keymap[] __initconst = {
+		{ KE_KEY,    0x281, { KEY_BRIGHTNESSDOWN } },
+		{ KE_KEY,    0x282, { KEY_BRIGHTNESSUP } },
+		{ KE_KEY,    0x284, { KEY_MUTE } },
+		{ KE_KEY,    0x285, { KEY_VOLUMEDOWN } },
+		{ KE_KEY,    0x286, { KEY_VOLUMEUP } },
+		{ KE_KEY,	 0x287, { KEY_MICMUTE } },
+		{ KE_KEY,	 0x289, { KEY_WLAN } },
+		// Huawei |M| button
+		{ KE_KEY,	 0x28a, { KEY_PROG1 } },
+		// Keyboard light
+		{ KE_IGNORE, 0x293, { KEY_KBDILLUMTOGGLE } },
+		{ KE_IGNORE, 0x294, { KEY_KBDILLUMUP } },
+		{ KE_IGNORE, 0x295, { KEY_KBDILLUMUP } },
+		{ KE_END,	 0 }
+};
+
+static char *event_guid;
+static struct input_dev *inputdev;
+
+int huawei_wmi_micmute_led_set(bool on)
+{
+	acpi_handle handle = ACPI_HANDLE(&inputdev->dev);
+	char *method;
+	union acpi_object args[3];
+	args[0].type = args[1].type = args[2].type = ACPI_TYPE_INTEGER;
+	args[1].integer.value = 0x04;
+	struct acpi_object_list arg_list = {
+		.pointer = args,
+		.count = ARRAY_SIZE(args),
+	};
+
+	if (acpi_has_method(handle, method = "\\_SB.PCI0.LPCB.EC0.SPIN")) {
+		args[0].integer.value = 0;
+		args[2].integer.value = on ? 1 : 0;
+	} else if (acpi_has_method(handle, method = "\\_SB.PCI0.LPCB.EC0.WPIN")) {
+		args[0].integer.value = 1;
+		args[2].integer.value = on ? 0 : 1;
+	} else {
+		pr_err("Unable to find ACPI method\n");
+		return -1;
+	}
+
+	acpi_evaluate_object(handle, method, &arg_list, NULL);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(huawei_wmi_micmute_led_set);
+
+static void huawei_wmi_process_key(struct input_dev *inputdev, int code)
+{
+	const struct key_entry *key;
+
+	/*
+	 * MBX uses code 0x80 to indicate a hotkey event.
+	 * The actual key is fetched from the method WQ00
+	 */
+	if (code == 0x80) {
+		acpi_status status;
+		unsigned long long result;
+		const char *method = "\\WMI0.WQ00";
+		union acpi_object args[] = {
+				{ .type = ACPI_TYPE_INTEGER  },
+		};
+		args[0].integer.value = 0;
+		struct acpi_object_list arg_list = {
+			.pointer = args,
+			.count = ARRAY_SIZE(args),
+		};
+
+		status = acpi_evaluate_integer(ACPI_HANDLE(&inputdev->dev), (char *)method, &arg_list, &result);
+		if (ACPI_FAILURE(status)) {
+			dev_err(&inputdev->dev, "Unable to evaluate ACPI method %s\n", method);
+			return;
+		}
+
+		code = result;
+	}
+
+	key = sparse_keymap_entry_from_scancode(inputdev, code);
+	if (!key) {
+		dev_info(&inputdev->dev, "Unknown key pressed, code: 0x%04x\n", code);
+		return;
+	}
+
+	/*
+	 * The MBXP handles backlight natively using ACPI,
+	 * but not the MBX. If MBXP is being used, skip reporting event.
+	 */
+	if ((key->sw.code == KEY_BRIGHTNESSUP
+			|| key->sw.code == KEY_BRIGHTNESSDOWN)
+			&& strcmp(event_guid, MBXP_EVENT_GUID) == 0)
+		return;
+
+	sparse_keymap_report_entry(inputdev, key, 1, true);
+}
+
+static void huawei_wmi_notify(u32 value, void *context)
+{
+	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	acpi_status status;
+	struct input_dev *inputdev = (struct input_dev*)context;
+
+	status = wmi_get_event_data(value, &response);
+	if (ACPI_FAILURE(status)) {
+		dev_err(&inputdev->dev, "Bad event status 0x%x\n", status);
+		return;
+	}
+
+	obj = (union acpi_object *)response.pointer;
+	if (!obj)
+		return;
+
+	if (obj->type == ACPI_TYPE_INTEGER)
+		huawei_wmi_process_key(inputdev, obj->integer.value);
+	else
+		dev_info(&inputdev->dev, "Unknown response received %d\n", obj->type);
+
+	kfree(response.pointer);
+}
+
+static int huawei_wmi_input_init(void)
+{
+	acpi_status status;
+	int err;
+
+	inputdev = input_allocate_device();
+	if (!inputdev)
+		return -ENOMEM;
+
+	inputdev->name = "Huawei WMI hotkeys";
+	inputdev->phys = "wmi/input0";
+	inputdev->id.bustype = BUS_HOST;
+
+	err = sparse_keymap_setup(inputdev,
+			huawei_wmi_keymap, NULL);
+	if (err)
+		goto err_free_dev;
+
+	status = wmi_install_notify_handler(event_guid,
+			huawei_wmi_notify,
+			inputdev);
+	if (ACPI_FAILURE(status)) {
+		err = -EIO;
+		goto err_free_dev;
+	}
+
+	err = input_register_device(inputdev);
+	if (err)
+		goto err_remove_notifier;
+
+	return 0;
+
+err_remove_notifier:
+	wmi_remove_notify_handler(event_guid);
+err_free_dev:
+	input_free_device(inputdev);
+	return err;
+}
+
+static void huawei_wmi_input_exit(void)
+{
+	wmi_remove_notify_handler(event_guid);
+	input_unregister_device(inputdev);
+}
+
+static int __init huawei_wmi_init(void)
+{
+	int err;
+
+	if (wmi_has_guid(MBX_EVENT_GUID)) {
+		event_guid = MBX_EVENT_GUID;
+	} else if (wmi_has_guid(MBXP_EVENT_GUID)) {
+		event_guid = MBXP_EVENT_GUID;
+	} else {
+		pr_warn("No known WMI GUID found\n");
+		return -ENODEV;
+	}
+
+	err = huawei_wmi_input_init();
+	if (err) {
+		pr_err("Failed to setup input device\n");
+		return err;
+	}
+
+	return 0;
+}
+
+static void __exit huawei_wmi_exit(void)
+{
+	huawei_wmi_input_exit();
+}
+
+module_init(huawei_wmi_init);
+module_exit(huawei_wmi_exit);
-- 
2.17.2

[PATCH v3 2/3] ALSA: hda: fix front speakers on Huawei MBXP.

[Ayman Bagabas]

This patch solves bug 200501 'Only 2 of 4 speakers playing sound.'
https://bugzilla.kernel.org/show_bug.cgi?id=200501
It enables the front speakers on Huawei Matebook X Pro laptops.
These laptops come with Dolby Atmos sound system and these pins
configuration enables the front speakers.

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
---
 sound/pci/hda/patch_realtek.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index fa61674a5605..ed6c9c79ce46 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -5493,6 +5493,7 @@ enum {
 	ALC298_FIXUP_TPT470_DOCK,
 	ALC255_FIXUP_DUMMY_LINEOUT_VERB,
 	ALC255_FIXUP_DELL_HEADSET_MIC,
+	ALC256_FIXUP_HUAWEI_MBXP_PINS,
 	ALC295_FIXUP_HP_X360,
 	ALC221_FIXUP_HP_HEADSET_MIC,
 };
@@ -5764,6 +5765,22 @@ static const struct hda_fixup alc269_fixups[] = {
 		.chained = true,
 		.chain_id = ALC269_FIXUP_HEADSET_MIC
 	},
+	[ALC256_FIXUP_HUAWEI_MBXP_PINS] = {
+		.type = HDA_FIXUP_PINS,
+		.v.pins = (const struct hda_pintbl[]) {
+			{0x12, 0x90a60130},
+			{0x13, 0x40000000},
+			{0x14, 0x90170110},
+			{0x18, 0x411111f0},
+			{0x19, 0x04a11040},
+			{0x1a, 0x411111f0},
+			{0x1b, 0x90170112},
+			{0x1d, 0x40759a05},
+			{0x1e, 0x411111f0},
+			{0x21, 0x04211020},
+			{ }
+		}
+	},
 	[ALC269_FIXUP_ASUS_X101_FUNC] = {
 		.type = HDA_FIXUP_FUNC,
 		.v.func = alc269_fixup_x101_headset_mic,
@@ -6592,6 +6609,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
 	SND_PCI_QUIRK(0x17aa, 0x5109, "Thinkpad", ALC269_FIXUP_LIMIT_INT_MIC_BOOST),
 	SND_PCI_QUIRK(0x17aa, 0x511e, "Thinkpad", ALC298_FIXUP_TPT470_DOCK),
 	SND_PCI_QUIRK(0x17aa, 0x511f, "Thinkpad", ALC298_FIXUP_TPT470_DOCK),
+	SND_PCI_QUIRK(0x19e5, 0x3204, "Huawei MBXP", ALC256_FIXUP_HUAWEI_MBXP_PINS),
 	SND_PCI_QUIRK(0x17aa, 0x3bf8, "Quanta FL1", ALC269_FIXUP_PCM_44K),
 	SND_PCI_QUIRK(0x17aa, 0x9e54, "LENOVO NB", ALC269_FIXUP_LENOVO_EAPD),
 	SND_PCI_QUIRK(0x1b7d, 0xa831, "Ordissimo EVE2 ", ALC269VB_FIXUP_ORDISSIMO_EVE2), /* Also known as Malata PC-B1303 */
-- 
2.17.2

[PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Ayman Bagabas]

Some of Huawei laptops come with a LED in the micmute key. This patch
enables and disable this LED accordingly.

Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
---
 drivers/platform/x86/huawei_wmi.c            |  1 +
 include/linux/platform_data/x86/huawei_wmi.h |  9 ++++
 sound/pci/hda/huawei_wmi_helper.c            | 47 ++++++++++++++++++++
 sound/pci/hda/patch_realtek.c                | 13 +++++-
 4 files changed, 69 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/platform_data/x86/huawei_wmi.h
 create mode 100644 sound/pci/hda/huawei_wmi_helper.c

diff --git a/drivers/platform/x86/huawei_wmi.c b/drivers/platform/x86/huawei_wmi.c
index 658c44ab2126..f06aa967c311 100644
--- a/drivers/platform/x86/huawei_wmi.c
+++ b/drivers/platform/x86/huawei_wmi.c
@@ -23,6 +23,7 @@
 #include <linux/input.h>
 #include <linux/input/sparse-keymap.h>
 #include <linux/module.h>
+#include <linux/platform_data/x86/huawei_wmi.h>
 
 MODULE_AUTHOR("Ayman Bagabas <ayman.bagabas@gmail.com>");
 MODULE_DESCRIPTION("Huawei WMI hotkeys");
diff --git a/include/linux/platform_data/x86/huawei_wmi.h b/include/linux/platform_data/x86/huawei_wmi.h
new file mode 100644
index 000000000000..dd251780ee5c
--- /dev/null
+++ b/include/linux/platform_data/x86/huawei_wmi.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#if IS_ENABLED(CONFIG_HUAWEI_LAPTOP)
+#ifndef __HUAWEI_WMI_H__
+#define __HUAWEI_WMI_H__
+
+int huawei_wmi_micmute_led_set(bool on);
+
+#endif
+#endif
diff --git a/sound/pci/hda/huawei_wmi_helper.c b/sound/pci/hda/huawei_wmi_helper.c
new file mode 100644
index 000000000000..7c91f914ffba
--- /dev/null
+++ b/sound/pci/hda/huawei_wmi_helper.c
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Helper functions for Huawei WMI micmute LED;
+ * to be included from codec driver
+ */
+
+#if IS_ENABLED(CONFIG_HUAWEI_LAPTOP)
+#include <linux/platform_data/x86/huawei_wmi.h>
+
+static int (*huawei_wmi_micmute_led_set_func)(bool);
+
+static void update_huawei_wmi_micmute_led(struct hda_codec *codec)
+{
+	struct hda_gen_spec *spec = codec->spec;
+	huawei_wmi_micmute_led_set_func(spec->micmute_led.led_value);
+}
+
+static void alc_fixup_huawei_wmi(struct hda_codec *codec,
+				   const struct hda_fixup *fix, int action)
+{
+	bool removefunc = false;
+
+	if (action == HDA_FIXUP_ACT_PROBE) {
+		if (!huawei_wmi_micmute_led_set_func)
+			huawei_wmi_micmute_led_set_func = symbol_request(huawei_wmi_micmute_led_set);
+		if (!huawei_wmi_micmute_led_set_func) {
+			codec_warn(codec, "Failed to find huawei_wmi symbol huawei_wmi_micmute_led_set\n");
+			return;
+		}
+
+		removefunc = (huawei_wmi_micmute_led_set_func(false) < 0)
+			|| (snd_hda_gen_add_micmute_led(codec, update_huawei_wmi_micmute_led) < 0);
+	}
+
+	if (huawei_wmi_micmute_led_set_func && (action == HDA_FIXUP_ACT_FREE || removefunc)) {
+		symbol_put(huawei_wmi_micmute_led_set);
+		huawei_wmi_micmute_led_set_func = NULL;
+	}
+}
+
+#else
+
+static void alc_fixup_huawei_wmi(struct hda_codec *codec,
+				const struct hda_fixup *fix, int action)
+{
+}
+
+#endif
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index ed6c9c79ce46..a0fdcfb0b635 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -5374,6 +5374,9 @@ static void alc_fixup_thinkpad_acpi(struct hda_codec *codec,
 /* for alc295_fixup_hp_top_speakers */
 #include "hp_x360_helper.c"
 
+/* for alc_fixup_huawei_micmute_led */
+#include "huawei_wmi_helper.c"
+
 enum {
 	ALC269_FIXUP_SONY_VAIO,
 	ALC275_FIXUP_SONY_VAIO_GPIO2,
@@ -5494,6 +5497,7 @@ enum {
 	ALC255_FIXUP_DUMMY_LINEOUT_VERB,
 	ALC255_FIXUP_DELL_HEADSET_MIC,
 	ALC256_FIXUP_HUAWEI_MBXP_PINS,
+	ALC256_FIXUP_HUAWEI_WMI_MICMUTE_LED,
 	ALC295_FIXUP_HP_X360,
 	ALC221_FIXUP_HP_HEADSET_MIC,
 };
@@ -5765,6 +5769,10 @@ static const struct hda_fixup alc269_fixups[] = {
 		.chained = true,
 		.chain_id = ALC269_FIXUP_HEADSET_MIC
 	},
+	[ALC256_FIXUP_HUAWEI_WMI_MICMUTE_LED] = {
+		.type = HDA_FIXUP_FUNC,
+		.v.func = alc_fixup_huawei_wmi
+	},
 	[ALC256_FIXUP_HUAWEI_MBXP_PINS] = {
 		.type = HDA_FIXUP_PINS,
 		.v.pins = (const struct hda_pintbl[]) {
@@ -5779,7 +5787,9 @@ static const struct hda_fixup alc269_fixups[] = {
 			{0x1e, 0x411111f0},
 			{0x21, 0x04211020},
 			{ }
-		}
+		},
+		.chained = true,
+		.chain_id = ALC256_FIXUP_HUAWEI_WMI_MICMUTE_LED
 	},
 	[ALC269_FIXUP_ASUS_X101_FUNC] = {
 		.type = HDA_FIXUP_FUNC,
@@ -6609,6 +6619,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
 	SND_PCI_QUIRK(0x17aa, 0x5109, "Thinkpad", ALC269_FIXUP_LIMIT_INT_MIC_BOOST),
 	SND_PCI_QUIRK(0x17aa, 0x511e, "Thinkpad", ALC298_FIXUP_TPT470_DOCK),
 	SND_PCI_QUIRK(0x17aa, 0x511f, "Thinkpad", ALC298_FIXUP_TPT470_DOCK),
+	SND_PCI_QUIRK(0x19e5, 0x3200, "Huawei MBX", ALC256_FIXUP_HUAWEI_WMI_MICMUTE_LED),
 	SND_PCI_QUIRK(0x19e5, 0x3204, "Huawei MBXP", ALC256_FIXUP_HUAWEI_MBXP_PINS),
 	SND_PCI_QUIRK(0x17aa, 0x3bf8, "Quanta FL1", ALC269_FIXUP_PCM_44K),
 	SND_PCI_QUIRK(0x17aa, 0x9e54, "LENOVO NB", ALC269_FIXUP_LENOVO_EAPD),
-- 
2.17.2

Re: [PATCH v3 1/3] x86: add support for Huawei WMI hotkeys.

[Andy Shevchenko]

On Thu, Nov 8, 2018 at 7:17 PM Ayman Bagabas <ayman.bagabas@gmail.com> wrote:
>
> This driver adds support for missing hotkeys on some Huawei laptops.
> Currently, only Huawei Matebook X and Matebook X Pro is supported.
>

Thanks for an update, my comments below.


> +config HUAWEI_LAPTOP
> +       tristate "Huawei WMI hotkeys driver"

> +       depends on ACPI

Do you need an ACPI dependency be explicit here?

> +       depends on ACPI_WMI
> +       depends on INPUT
> +       select INPUT_SPARSEKMAP
> +       help
> +         This driver provides support for Huawei WMI hotkeys.
> +         It enables the missing keys and adds support to the micmute
> +         LED found on some of these laptops.


> +/*
> + *  Huawei WMI hotkeys
> + *
> + *  Copyright (C) 2018       Ayman Bagabas <ayman.bagabas@gmail.com>

> + *
> + *  This program is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program.  If not, see <https://www.gnu.org/licenses/>.
> + *

Please, replace this boilerplate text with appropriate SPDX identifier.

> + */

> +#include <linux/init.h>
> +#include <linux/module.h>

One of them should be chosen.

> +static char *event_guid;
> +static struct input_dev *inputdev;

> +int huawei_wmi_micmute_led_set(bool on)
> +{
> +       acpi_handle handle = ACPI_HANDLE(&inputdev->dev);
> +       char *method;
> +       union acpi_object args[3];

> +       args[0].type = args[1].type = args[2].type = ACPI_TYPE_INTEGER;
> +       args[1].integer.value = 0x04;

Please, don't mix definitions and code.

> +       struct acpi_object_list arg_list = {
> +               .pointer = args,
> +               .count = ARRAY_SIZE(args),
> +       };
> +
> +       if (acpi_has_method(handle, method = "\\_SB.PCI0.LPCB.EC0.SPIN")) {
> +               args[0].integer.value = 0;
> +               args[2].integer.value = on ? 1 : 0;
> +       } else if (acpi_has_method(handle, method = "\\_SB.PCI0.LPCB.EC0.WPIN")) {
> +               args[0].integer.value = 1;
> +               args[2].integer.value = on ? 0 : 1;
> +       } else {
> +               pr_err("Unable to find ACPI method\n");

dev_err() here.

> +               return -1;

Return appropriate error code.

> +       }
> +
> +       acpi_evaluate_object(handle, method, &arg_list, NULL);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(huawei_wmi_micmute_led_set);
> +
> +static void huawei_wmi_process_key(struct input_dev *inputdev, int code)
> +{

> +               acpi_status status;
> +               unsigned long long result;
> +               const char *method = "\\WMI0.WQ00";
> +               union acpi_object args[] = {
> +                               { .type = ACPI_TYPE_INTEGER  },
> +               };

> +               args[0].integer.value = 0;

Don't mix definitions and code.

> +               struct acpi_object_list arg_list = {
> +                       .pointer = args,
> +                       .count = ARRAY_SIZE(args),
> +               };

> +       if ((key->sw.code == KEY_BRIGHTNESSUP
> +                       || key->sw.code == KEY_BRIGHTNESSDOWN)

I believe this can fit one line.

> +                       && strcmp(event_guid, MBXP_EVENT_GUID) == 0)
> +               return;
> +
> +       sparse_keymap_report_entry(inputdev, key, 1, true);
> +}

> +static int __init huawei_wmi_init(void)
> +{
> +       int err;
> +
> +       if (wmi_has_guid(MBX_EVENT_GUID)) {
> +               event_guid = MBX_EVENT_GUID;
> +       } else if (wmi_has_guid(MBXP_EVENT_GUID)) {
> +               event_guid = MBXP_EVENT_GUID;
> +       } else {

> +               pr_warn("No known WMI GUID found\n");

Simple "Compatible WMI GUID not found\n".

> +               return -ENODEV;
> +       }
> +

> +       err = huawei_wmi_input_init();
> +       if (err) {

> +               pr_err("Failed to setup input device\n");

Noise.

> +               return err;
> +       }
> +
> +       return 0;

...just do

return huawei_wmi_input_init();

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/3] Huawei laptops

[Andy Shevchenko]

On Thu, Nov 8, 2018 at 7:17 PM Ayman Bagabas <ayman.bagabas@gmail.com> wrote:

Is it supposed to go via PDx86 or ALSA tree?

> Changes from v2:
> * Support for Huawei MBX
> * Style and formating issues
>
> [PATCH v3 1/3]
> The first patch adds support for missing hotkeys on some models.
>
> [PATCH v3 2/3]
> This one enables the front speakers on the Huawei Matebook X Pro (MBXP). This
> solves bug 200501 https://bugzilla.kernel.org/show_bug.cgi?id=200501
> It simply uses the pins configurations generated by hdajackretast using the
> settings posted on the bug page https://imgur.com/a/N1xsCVZ
>
> [PATCH v3 3/3]
> This enables the micmute LED on Huawei laptops using ACPI calls.
>
> Ayman Bagabas (3):
>   x86: add support for Huawei WMI hotkeys.
>   ALSA: hda: fix front speakers on Huawei MBXP.
>   ALSA: hda: add support for Huawei WMI micmute LED
>
>  drivers/platform/x86/Kconfig                 |  11 +
>  drivers/platform/x86/Makefile                |   1 +
>  drivers/platform/x86/huawei_wmi.c            | 236 +++++++++++++++++++
>  include/linux/platform_data/x86/huawei_wmi.h |   9 +
>  sound/pci/hda/huawei_wmi_helper.c            |  47 ++++
>  sound/pci/hda/patch_realtek.c                |  29 +++
>  6 files changed, 333 insertions(+)
>  create mode 100644 drivers/platform/x86/huawei_wmi.c
>  create mode 100644 include/linux/platform_data/x86/huawei_wmi.h
>  create mode 100644 sound/pci/hda/huawei_wmi_helper.c
>
> --
> 2.17.2
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/3] Huawei laptops

[ayman.bagabas]

On Thu, 2018-11-08 at 21:59 +0200, Andy Shevchenko wrote:
> On Thu, Nov 8, 2018 at 7:17 PM Ayman Bagabas <ayman.bagabas@gmail.com
> > wrote:
> 
> Is it supposed to go via PDx86 or ALSA tree?

There isn't much of work done on behave of ALSA. So PDx86?

> 
> > Changes from v2:
> > * Support for Huawei MBX
> > * Style and formating issues
> > 
> > [PATCH v3 1/3]
> > The first patch adds support for missing hotkeys on some models.
> > 
> > [PATCH v3 2/3]
> > This one enables the front speakers on the Huawei Matebook X Pro
> > (MBXP). This
> > solves bug 200501 
> > https://bugzilla.kernel.org/show_bug.cgi?id=200501
> > It simply uses the pins configurations generated by hdajackretast
> > using the
> > settings posted on the bug page https://imgur.com/a/N1xsCVZ
> > 
> > [PATCH v3 3/3]
> > This enables the micmute LED on Huawei laptops using ACPI calls.
> > 
> > Ayman Bagabas (3):
> >   x86: add support for Huawei WMI hotkeys.
> >   ALSA: hda: fix front speakers on Huawei MBXP.
> >   ALSA: hda: add support for Huawei WMI micmute LED
> > 
> >  drivers/platform/x86/Kconfig                 |  11 +
> >  drivers/platform/x86/Makefile                |   1 +
> >  drivers/platform/x86/huawei_wmi.c            | 236
> > +++++++++++++++++++
> >  include/linux/platform_data/x86/huawei_wmi.h |   9 +
> >  sound/pci/hda/huawei_wmi_helper.c            |  47 ++++
> >  sound/pci/hda/patch_realtek.c                |  29 +++
> >  6 files changed, 333 insertions(+)
> >  create mode 100644 drivers/platform/x86/huawei_wmi.c
> >  create mode 100644 include/linux/platform_data/x86/huawei_wmi.h
> >  create mode 100644 sound/pci/hda/huawei_wmi_helper.c
> > 
> > --
> > 2.17.2
> > 
> 
> 

Re: [PATCH v3 1/3] x86: add support for Huawei WMI hotkeys.

[ayman.bagabas]

On Thu, 2018-11-08 at 21:58 +0200, Andy Shevchenko wrote:
> On Thu, Nov 8, 2018 at 7:17 PM Ayman Bagabas <ayman.bagabas@gmail.com
> > wrote:
> > This driver adds support for missing hotkeys on some Huawei
> > laptops.
> > Currently, only Huawei Matebook X and Matebook X Pro is supported.
> > 
> 
> Thanks for an update, my comments below.
> 
> 
> > +config HUAWEI_LAPTOP
> > +       tristate "Huawei WMI hotkeys driver"
> > +       depends on ACPI
> 
> Do you need an ACPI dependency be explicit here?

Probably don't need it.

> 
> > +       depends on ACPI_WMI
> > +       depends on INPUT
> > +       select INPUT_SPARSEKMAP
> > +       help
> > +         This driver provides support for Huawei WMI hotkeys.
> > +         It enables the missing keys and adds support to the
> > micmute
> > +         LED found on some of these laptops.
> > +/*
> > + *  Huawei WMI hotkeys
> > + *
> > + *  Copyright (C) 2018       Ayman Bagabas <
> > ayman.bagabas@gmail.com>
> > + *
> > + *  This program is free software: you can redistribute it and/or
> > modify
> > + *  it under the terms of the GNU General Public License as
> > published by
> > + *  the Free Software Foundation, either version 2 of the License,
> > or
> > + *  (at your option) any later version.
> > + *
> > + *  This program is distributed in the hope that it will be
> > useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU General Public
> > License
> > + *  along with this program.  If not, see <
> > https://www.gnu.org/licenses/>;.
> > + *
> 
> Please, replace this boilerplate text with appropriate SPDX
> identifier.

Soryy about that. This alright?

// SPDX-License-Identifier: GPL-2.0
/*
 * Huawei WMI hotkeys
 *
 * Copyright (C) 2018		Ayman Bagabas <ayman.bagabas@gmail.com>
 */

Re: [PATCH v3 0/3] Huawei laptops

[Takashi Iwai]

On Thu, 08 Nov 2018 20:59:45 +0100,
Andy Shevchenko wrote:
> 
> On Thu, Nov 8, 2018 at 7:17 PM Ayman Bagabas <ayman.bagabas@gmail.com> wrote:
> 
> Is it supposed to go via PDx86 or ALSA tree?

I don't mind either way.  The addition in platform is more
significant, so I suppose you can take it more easily.


thanks,

Takashi

Re: [PATCH v3 2/3] ALSA: hda: fix front speakers on Huawei MBXP.

[Takashi Iwai]

On Thu, 08 Nov 2018 18:16:54 +0100,
Ayman Bagabas wrote:
> 
> @@ -6592,6 +6609,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
>  	SND_PCI_QUIRK(0x17aa, 0x5109, "Thinkpad", ALC269_FIXUP_LIMIT_INT_MIC_BOOST),
>  	SND_PCI_QUIRK(0x17aa, 0x511e, "Thinkpad", ALC298_FIXUP_TPT470_DOCK),
>  	SND_PCI_QUIRK(0x17aa, 0x511f, "Thinkpad", ALC298_FIXUP_TPT470_DOCK),
> +	SND_PCI_QUIRK(0x19e5, 0x3204, "Huawei MBXP", ALC256_FIXUP_HUAWEI_MBXP_PINS),
>  	SND_PCI_QUIRK(0x17aa, 0x3bf8, "Quanta FL1", ALC269_FIXUP_PCM_44K),
>  	SND_PCI_QUIRK(0x17aa, 0x9e54, "LENOVO NB", ALC269_FIXUP_LENOVO_EAPD),
>  	SND_PCI_QUIRK(0x1b7d, 0xa831, "Ordissimo EVE2 ", ALC269VB_FIXUP_ORDISSIMO_EVE2), /* Also known as Malata PC-B1303 */

The list is sorted, so please please insert the entry at the right
place.


thanks,

Takashi

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Takashi Iwai]

On Thu, 08 Nov 2018 18:16:55 +0100,
Ayman Bagabas wrote:
> 
> diff --git a/drivers/platform/x86/huawei_wmi.c b/drivers/platform/x86/huawei_wmi.c
> index 658c44ab2126..f06aa967c311 100644
> --- a/drivers/platform/x86/huawei_wmi.c
> +++ b/drivers/platform/x86/huawei_wmi.c
> @@ -23,6 +23,7 @@
>  #include <linux/input.h>
>  #include <linux/input/sparse-keymap.h>
>  #include <linux/module.h>
> +#include <linux/platform_data/x86/huawei_wmi.h>
>  
>  MODULE_AUTHOR("Ayman Bagabas <ayman.bagabas@gmail.com>");
>  MODULE_DESCRIPTION("Huawei WMI hotkeys");
> diff --git a/include/linux/platform_data/x86/huawei_wmi.h b/include/linux/platform_data/x86/huawei_wmi.h
> new file mode 100644
> index 000000000000..dd251780ee5c
> --- /dev/null
> +++ b/include/linux/platform_data/x86/huawei_wmi.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#if IS_ENABLED(CONFIG_HUAWEI_LAPTOP)
> +#ifndef __HUAWEI_WMI_H__
> +#define __HUAWEI_WMI_H__
> +
> +int huawei_wmi_micmute_led_set(bool on);
> +
> +#endif
> +#endif

These changes should belong to the WMI patch.

> @@ -5765,6 +5769,10 @@ static const struct hda_fixup alc269_fixups[] = {
>  		.chained = true,
>  		.chain_id = ALC269_FIXUP_HEADSET_MIC
>  	},
> +	[ALC256_FIXUP_HUAWEI_WMI_MICMUTE_LED] = {
> +		.type = HDA_FIXUP_FUNC,
> +		.v.func = alc_fixup_huawei_wmi
> +	},
>  	[ALC256_FIXUP_HUAWEI_MBXP_PINS] = {
>  		.type = HDA_FIXUP_PINS,
>  		.v.pins = (const struct hda_pintbl[]) {
> @@ -5779,7 +5787,9 @@ static const struct hda_fixup alc269_fixups[] = {
>  			{0x1e, 0x411111f0},
>  			{0x21, 0x04211020},
>  			{ }
> -		}
> +		},
> +		.chained = true,
> +		.chain_id = ALC256_FIXUP_HUAWEI_WMI_MICMUTE_LED
>  	},
>  	[ALC269_FIXUP_ASUS_X101_FUNC] = {
>  		.type = HDA_FIXUP_FUNC,

This means that ALC256_FIXUP_HUAWEI_MBXP_PINS performs both the pin
config fixup and the mic-mute LED enablement.

> @@ -6609,6 +6619,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = {
>  	SND_PCI_QUIRK(0x17aa, 0x5109, "Thinkpad", ALC269_FIXUP_LIMIT_INT_MIC_BOOST),
>  	SND_PCI_QUIRK(0x17aa, 0x511e, "Thinkpad", ALC298_FIXUP_TPT470_DOCK),
>  	SND_PCI_QUIRK(0x17aa, 0x511f, "Thinkpad", ALC298_FIXUP_TPT470_DOCK),
> +	SND_PCI_QUIRK(0x19e5, 0x3200, "Huawei MBX", ALC256_FIXUP_HUAWEI_WMI_MICMUTE_LED),
>  	SND_PCI_QUIRK(0x19e5, 0x3204, "Huawei MBXP", ALC256_FIXUP_HUAWEI_MBXP_PINS),
>  	SND_PCI_QUIRK(0x17aa, 0x3bf8, "Quanta FL1", ALC269_FIXUP_PCM_44K),
>  	SND_PCI_QUIRK(0x17aa, 0x9e54, "LENOVO NB", ALC269_FIXUP_LENOVO_EAPD),

... and yet you add a new entry for performing only mic-mute LED.
I guess the chaining is done wrongly above?


thanks,

Takashi

Re: [PATCH v3 1/3] x86: add support for Huawei WMI hotkeys.

[Andy Shevchenko]

Yes, it does.
On Fri, Nov 9, 2018 at 5:52 AM <ayman.bagabas@gmail.com> wrote:
>
> On Thu, 2018-11-08 at 21:58 +0200, Andy Shevchenko wrote:
> > On Thu, Nov 8, 2018 at 7:17 PM Ayman Bagabas <ayman.bagabas@gmail.com
> > > wrote:
> > > This driver adds support for missing hotkeys on some Huawei
> > > laptops.
> > > Currently, only Huawei Matebook X and Matebook X Pro is supported.
> > >
> >
> > Thanks for an update, my comments below.
> >
> >
> > > +config HUAWEI_LAPTOP
> > > +       tristate "Huawei WMI hotkeys driver"
> > > +       depends on ACPI
> >
> > Do you need an ACPI dependency be explicit here?
>
> Probably don't need it.
>
> >
> > > +       depends on ACPI_WMI
> > > +       depends on INPUT
> > > +       select INPUT_SPARSEKMAP
> > > +       help
> > > +         This driver provides support for Huawei WMI hotkeys.
> > > +         It enables the missing keys and adds support to the
> > > micmute
> > > +         LED found on some of these laptops.
> > > +/*
> > > + *  Huawei WMI hotkeys
> > > + *
> > > + *  Copyright (C) 2018       Ayman Bagabas <
> > > ayman.bagabas@gmail.com>
> > > + *
> > > + *  This program is free software: you can redistribute it and/or
> > > modify
> > > + *  it under the terms of the GNU General Public License as
> > > published by
> > > + *  the Free Software Foundation, either version 2 of the License,
> > > or
> > > + *  (at your option) any later version.
> > > + *
> > > + *  This program is distributed in the hope that it will be
> > > useful,
> > > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + *  GNU General Public License for more details.
> > > + *
> > > + *  You should have received a copy of the GNU General Public
> > > License
> > > + *  along with this program.  If not, see <
> > > https://www.gnu.org/licenses/>;.
> > > + *
> >
> > Please, replace this boilerplate text with appropriate SPDX
> > identifier.
>
> Soryy about that. This alright?
>
> // SPDX-License-Identifier: GPL-2.0
> /*
>  * Huawei WMI hotkeys
>  *
>  * Copyright (C) 2018           Ayman Bagabas <ayman.bagabas@gmail.com>
>  */
>
>
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Takashi Iwai]

On Fri, 09 Nov 2018 14:20:47 +0100,
Ayman Bagabas wrote:
> 
> [1  <text/plain; UTF-8 (7bit)>]
> 
> On Fri, Nov 9, 2018, 4:01 AM Takashi Iwai <tiwai@suse.de wrote:
> 
>     On Thu, 08 Nov 2018 18:16:55 +0100,
>     Ayman Bagabas wrote:
>     >
>     > diff --git a/drivers/platform/x86/huawei_wmi.c b/drivers/platform/x86/
>     huawei_wmi.c
>     > index 658c44ab2126..f06aa967c311 100644
>     > --- a/drivers/platform/x86/huawei_wmi.c
>     > +++ b/drivers/platform/x86/huawei_wmi.c
>     > @@ -23,6 +23,7 @@
>     >  #include <linux/input.h>
>     >  #include <linux/input/sparse-keymap.h>
> 
>     >  #include <linux/module.h>
>     > +#include <linux/platform_data/x86/huawei_wmi.h>
>     > 
>     >  MODULE_AUTHOR("Ayman Bagabas <ayman.bagabas@gmail.com>");
>     >  MODULE_DESCRIPTION("Huawei WMI hotkeys");
>     > diff --git a/include/linux/platform_data/x86/huawei_wmi.h b/include/
>     linux/platform_data/x86/huawei_wmi.h
>     > new file mode 100644
>     > index 000000000000..dd251780ee5c
>     > --- /dev/null
>     > +++ b/include/linux/platform_data/x86/huawei_wmi.h
>     > @@ -0,0 +1,9 @@
>     > +/* SPDX-License-Identifier: GPL-2.0 */
>     > +#if IS_ENABLED(CONFIG_HUAWEI_LAPTOP)
>     > +#ifndef __HUAWEI_WMI_H__
>     > +#define __HUAWEI_WMI_H__
>     > +
>     > +int huawei_wmi_micmute_led_set(bool on);
>     > +
>     > +#endif
>     > +#endif
>    
>     These changes should belong to the WMI patch.
>    
>     > @@ -5765,6 +5769,10 @@ static const struct hda_fixup alc269_fixups[] = {
>     >               .chained = true,
>     >               .chain_id = ALC269_FIXUP_HEADSET_MIC
>     >       },
>     > +     [ALC256_FIXUP_HUAWEI_WMI_MICMUTE_LED] = {
>     > +             .type = HDA_FIXUP_FUNC,
>     > +             .v.func = alc_fixup_huawei_wmi
>     > +     },
>     >       [ALC256_FIXUP_HUAWEI_MBXP_PINS] = {
>     >               .type = HDA_FIXUP_PINS,
>     >               .v.pins = (const struct hda_pintbl[]) {
>     > @@ -5779,7 +5787,9 @@ static const struct hda_fixup alc269_fixups[] = {
>     >                       {0x1e, 0x411111f0},
>     >                       {0x21, 0x04211020},
>     >                       { }
>     > -             }
>     > +             },
>     > +             .chained = true,
>     > +             .chain_id = ALC256_FIXUP_HUAWEI_WMI_MICMUTE_LED
>     >       },
>     >       [ALC269_FIXUP_ASUS_X101_FUNC] = {
>     >               .type = HDA_FIXUP_FUNC,
>    
>     This means that ALC256_FIXUP_HUAWEI_MBXP_PINS performs both the pin
>     config fixup and the mic-mute LED enablement.
>    
>     > @@ -6609,6 +6619,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl
>     [] = {
>     >       SND_PCI_QUIRK(0x17aa, 0x5109, "Thinkpad",
>     ALC269_FIXUP_LIMIT_INT_MIC_BOOST),
>     >       SND_PCI_QUIRK(0x17aa, 0x511e, "Thinkpad",
>     ALC298_FIXUP_TPT470_DOCK),
>     >       SND_PCI_QUIRK(0x17aa, 0x511f, "Thinkpad",
>     ALC298_FIXUP_TPT470_DOCK),
>     > +     SND_PCI_QUIRK(0x19e5, 0x3200, "Huawei MBX",
>     ALC256_FIXUP_HUAWEI_WMI_MICMUTE_LED),
>     >       SND_PCI_QUIRK(0x19e5, 0x3204, "Huawei MBXP",
>     ALC256_FIXUP_HUAWEI_MBXP_PINS),
>     >       SND_PCI_QUIRK(0x17aa, 0x3bf8, "Quanta FL1", ALC269_FIXUP_PCM_44K),
>     >       SND_PCI_QUIRK(0x17aa, 0x9e54, "LENOVO NB",
>     ALC269_FIXUP_LENOVO_EAPD),
>    
>     ... and yet you add a new entry for performing only mic-mute LED.
>     I guess the chaining is done wrongly above?
> 
> They are suppose to be two different devices. MBXP should apply both, but the
> MBX should only perform the LED.

Then please write it the changelog.  Otherwise readers have no idea
whether it's an intentional change or an oversight.


thanks,

Takashi

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Pavel Machek]

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

Hi!

> Some of Huawei laptops come with a LED in the micmute key. This patch
> enables and disable this LED accordingly.
> 
> Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>

NAK.

We already have a LED subsystem. 

> +#if IS_ENABLED(CONFIG_HUAWEI_LAPTOP)
> +#include <linux/platform_data/x86/huawei_wmi.h>
> +
> +static int (*huawei_wmi_micmute_led_set_func)(bool);
> +

So we should not be doing this.

Thinkpad ACPI module exports its LEDs there, for example.

Thanks,

									Pavel

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

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

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Takashi Iwai]

On Tue, 20 Nov 2018 00:57:13 +0100,
Pavel Machek wrote:
> 
> > +#if IS_ENABLED(CONFIG_HUAWEI_LAPTOP)
> > +#include <linux/platform_data/x86/huawei_wmi.h>
> > +
> > +static int (*huawei_wmi_micmute_led_set_func)(bool);
> > +
> 
> So we should not be doing this.
> 
> Thinkpad ACPI module exports its LEDs there, for example.

Both thinkpad_acpi and dell_laptop provide the interface to HD-audio
in the very same way like this.


thanks,

Takashi

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Pavel Machek]

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

On Tue 2018-11-20 08:07:09, Takashi Iwai wrote:
> On Tue, 20 Nov 2018 00:57:13 +0100,
> Pavel Machek wrote:
> > 
> > > +#if IS_ENABLED(CONFIG_HUAWEI_LAPTOP)
> > > +#include <linux/platform_data/x86/huawei_wmi.h>
> > > +
> > > +static int (*huawei_wmi_micmute_led_set_func)(bool);
> > > +
> > 
> > So we should not be doing this.
> > 
> > Thinkpad ACPI module exports its LEDs there, for example.
> 
> Both thinkpad_acpi and dell_laptop provide the interface to HD-audio
> in the very same way like this.

Not good :-(. Please don't add new ones, general purpose LEDs should
really use LED subsystem.

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

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

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Takashi Iwai]

On Tue, 20 Nov 2018 10:10:39 +0100,
Pavel Machek wrote:
> 
> On Tue 2018-11-20 08:07:09, Takashi Iwai wrote:
> > On Tue, 20 Nov 2018 00:57:13 +0100,
> > Pavel Machek wrote:
> > > 
> > > > +#if IS_ENABLED(CONFIG_HUAWEI_LAPTOP)
> > > > +#include <linux/platform_data/x86/huawei_wmi.h>
> > > > +
> > > > +static int (*huawei_wmi_micmute_led_set_func)(bool);
> > > > +
> > > 
> > > So we should not be doing this.
> > > 
> > > Thinkpad ACPI module exports its LEDs there, for example.
> > 
> > Both thinkpad_acpi and dell_laptop provide the interface to HD-audio
> > in the very same way like this.
> 
> Not good :-(. Please don't add new ones, general purpose LEDs should
> really use LED subsystem.

What's the problem with this approach?


Takashi

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Pavel Machek]

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

On Tue 2018-11-20 10:23:25, Takashi Iwai wrote:
> On Tue, 20 Nov 2018 10:10:39 +0100,
> Pavel Machek wrote:
> > 
> > On Tue 2018-11-20 08:07:09, Takashi Iwai wrote:
> > > On Tue, 20 Nov 2018 00:57:13 +0100,
> > > Pavel Machek wrote:
> > > > 
> > > > > +#if IS_ENABLED(CONFIG_HUAWEI_LAPTOP)
> > > > > +#include <linux/platform_data/x86/huawei_wmi.h>
> > > > > +
> > > > > +static int (*huawei_wmi_micmute_led_set_func)(bool);
> > > > > +
> > > > 
> > > > So we should not be doing this.
> > > > 
> > > > Thinkpad ACPI module exports its LEDs there, for example.
> > > 
> > > Both thinkpad_acpi and dell_laptop provide the interface to HD-audio
> > > in the very same way like this.
> > 
> > Not good :-(. Please don't add new ones, general purpose LEDs should
> > really use LED subsystem.
> 
> What's the problem with this approach?

You have general-purpose LED, yet you are treating it as "something
special". That means ugly code (quoted above) and lack of flexibility.

For example, if my notebook lacks HDD LED, I can use scrollock LED for
that instead. Or, in reverse way, maybe "mic mute" LED is not useful
for me, and I'd like to use it for notifications instead.

(If the LED was driven by hardware, and always reflected microphone
status, that would be different. But that's not the case AFAICT).


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

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

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Takashi Iwai]

On Tue, 20 Nov 2018 10:36:10 +0100,
Pavel Machek wrote:
> 
> On Tue 2018-11-20 10:23:25, Takashi Iwai wrote:
> > On Tue, 20 Nov 2018 10:10:39 +0100,
> > Pavel Machek wrote:
> > > 
> > > On Tue 2018-11-20 08:07:09, Takashi Iwai wrote:
> > > > On Tue, 20 Nov 2018 00:57:13 +0100,
> > > > Pavel Machek wrote:
> > > > > 
> > > > > > +#if IS_ENABLED(CONFIG_HUAWEI_LAPTOP)
> > > > > > +#include <linux/platform_data/x86/huawei_wmi.h>
> > > > > > +
> > > > > > +static int (*huawei_wmi_micmute_led_set_func)(bool);
> > > > > > +
> > > > > 
> > > > > So we should not be doing this.
> > > > > 
> > > > > Thinkpad ACPI module exports its LEDs there, for example.
> > > > 
> > > > Both thinkpad_acpi and dell_laptop provide the interface to HD-audio
> > > > in the very same way like this.
> > > 
> > > Not good :-(. Please don't add new ones, general purpose LEDs should
> > > really use LED subsystem.
> > 
> > What's the problem with this approach?
> 
> You have general-purpose LED, yet you are treating it as "something
> special". That means ugly code (quoted above) and lack of flexibility.
> 
> For example, if my notebook lacks HDD LED, I can use scrollock LED for
> that instead. Or, in reverse way, maybe "mic mute" LED is not useful
> for me, and I'd like to use it for notifications instead.

I'm not against adding the LEDs device implementation for any exotic
usage.

But for the audio mute LED features, you'll need really lots of other
works if it were implemented via leds device.  That's the hardest
part, and a few lines of hooks solves it easily in the kernel side.
That's all about it.

If you are ready for submitting the real solutions in user-space side
(patching PulseAudio and whatever all existing sound daemons, and
creating yet another daemon for non-PA systems (another footprint,
lovely), and so on), we can happily delete such in-kernel hooks :)


thanks,

Takashi

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Pavel Machek]

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

Hi!

> > You have general-purpose LED, yet you are treating it as "something
> > special". That means ugly code (quoted above) and lack of flexibility.
> > 
> > For example, if my notebook lacks HDD LED, I can use scrollock LED for
> > that instead. Or, in reverse way, maybe "mic mute" LED is not useful
> > for me, and I'd like to use it for notifications instead.
> 
> I'm not against adding the LEDs device implementation for any exotic
> usage.
> 
> But for the audio mute LED features, you'll need really lots of other
> works if it were implemented via leds device.  That's the hardest
> part, and a few lines of hooks solves it easily in the kernel side.
> That's all about it.
> 
> If you are ready for submitting the real solutions in user-space side
> (patching PulseAudio and whatever all existing sound daemons, and
> creating yet another daemon for non-PA systems (another footprint,
> lovely), and so on), we can happily delete such in-kernel hooks :)

I'm not saying we should move it to the userspace.

I'm saying this should be "normal" led. drivers/leds/led-huawei-acpi.c,
or something. Perhaps this acpi stuff is so similar you don't really
need .c code.

And then there should be a "mic muted" trigger. Similar to
drivers/leds/trigger/ledtrig-disk.c.

That should give us the flexibility, and should not be really much
different from current implementation...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Takashi Iwai]

On Tue, 20 Nov 2018 12:51:59 +0100,
Pavel Machek wrote:
> 
> Hi!
> 
> > > You have general-purpose LED, yet you are treating it as "something
> > > special". That means ugly code (quoted above) and lack of flexibility.
> > > 
> > > For example, if my notebook lacks HDD LED, I can use scrollock LED for
> > > that instead. Or, in reverse way, maybe "mic mute" LED is not useful
> > > for me, and I'd like to use it for notifications instead.
> > 
> > I'm not against adding the LEDs device implementation for any exotic
> > usage.
> > 
> > But for the audio mute LED features, you'll need really lots of other
> > works if it were implemented via leds device.  That's the hardest
> > part, and a few lines of hooks solves it easily in the kernel side.
> > That's all about it.
> > 
> > If you are ready for submitting the real solutions in user-space side
> > (patching PulseAudio and whatever all existing sound daemons, and
> > creating yet another daemon for non-PA systems (another footprint,
> > lovely), and so on), we can happily delete such in-kernel hooks :)
> 
> I'm not saying we should move it to the userspace.
> 
> I'm saying this should be "normal" led. drivers/leds/led-huawei-acpi.c,
> or something. Perhaps this acpi stuff is so similar you don't really
> need .c code.
> 
> And then there should be a "mic muted" trigger. Similar to
> drivers/leds/trigger/ledtrig-disk.c.

And who will trigger this, e.g. when the mixer is muted?


Takashi

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Andy Shevchenko]

On Tue, Nov 20, 2018 at 2:19 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Tue, 20 Nov 2018 12:51:59 +0100,
> Pavel Machek wrote:
> >
> > Hi!
> >
> > > > You have general-purpose LED, yet you are treating it as "something
> > > > special". That means ugly code (quoted above) and lack of flexibility.
> > > >
> > > > For example, if my notebook lacks HDD LED, I can use scrollock LED for
> > > > that instead. Or, in reverse way, maybe "mic mute" LED is not useful
> > > > for me, and I'd like to use it for notifications instead.
> > >
> > > I'm not against adding the LEDs device implementation for any exotic
> > > usage.
> > >
> > > But for the audio mute LED features, you'll need really lots of other
> > > works if it were implemented via leds device.  That's the hardest
> > > part, and a few lines of hooks solves it easily in the kernel side.
> > > That's all about it.
> > >
> > > If you are ready for submitting the real solutions in user-space side
> > > (patching PulseAudio and whatever all existing sound daemons, and
> > > creating yet another daemon for non-PA systems (another footprint,
> > > lovely), and so on), we can happily delete such in-kernel hooks :)
> >
> > I'm not saying we should move it to the userspace.
> >
> > I'm saying this should be "normal" led. drivers/leds/led-huawei-acpi.c,
> > or something. Perhaps this acpi stuff is so similar you don't really
> > need .c code.
> >
> > And then there should be a "mic muted" trigger. Similar to
> > drivers/leds/trigger/ledtrig-disk.c.
>
> And who will trigger this, e.g. when the mixer is muted?

Is this settled? I'm encouraged to promote this series to our for-next branch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Pavel Machek]

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

On Tue 2018-11-20 13:19:26, Takashi Iwai wrote:
> On Tue, 20 Nov 2018 12:51:59 +0100,
> Pavel Machek wrote:
> > 
> > Hi!
> > 
> > > > You have general-purpose LED, yet you are treating it as "something
> > > > special". That means ugly code (quoted above) and lack of flexibility.
> > > > 
> > > > For example, if my notebook lacks HDD LED, I can use scrollock LED for
> > > > that instead. Or, in reverse way, maybe "mic mute" LED is not useful
> > > > for me, and I'd like to use it for notifications instead.
> > > 
> > > I'm not against adding the LEDs device implementation for any exotic
> > > usage.
> > > 
> > > But for the audio mute LED features, you'll need really lots of other
> > > works if it were implemented via leds device.  That's the hardest
> > > part, and a few lines of hooks solves it easily in the kernel side.
> > > That's all about it.
> > > 
> > > If you are ready for submitting the real solutions in user-space side
> > > (patching PulseAudio and whatever all existing sound daemons, and
> > > creating yet another daemon for non-PA systems (another footprint,
> > > lovely), and so on), we can happily delete such in-kernel hooks :)
> > 
> > I'm not saying we should move it to the userspace.
> > 
> > I'm saying this should be "normal" led. drivers/leds/led-huawei-acpi.c,
> > or something. Perhaps this acpi stuff is so similar you don't really
> > need .c code.
> > 
> > And then there should be a "mic muted" trigger. Similar to
> > drivers/leds/trigger/ledtrig-disk.c.
> 
> And who will trigger this, e.g. when the mixer is muted?

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

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

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Takashi Iwai]

On Thu, 22 Nov 2018 14:12:16 +0100,
Pavel Machek wrote:
> 
> On Tue 2018-11-20 13:19:26, Takashi Iwai wrote:
> > On Tue, 20 Nov 2018 12:51:59 +0100,
> > Pavel Machek wrote:
> > > 
> > > Hi!
> > > 
> > > > > You have general-purpose LED, yet you are treating it as "something
> > > > > special". That means ugly code (quoted above) and lack of flexibility.
> > > > > 
> > > > > For example, if my notebook lacks HDD LED, I can use scrollock LED for
> > > > > that instead. Or, in reverse way, maybe "mic mute" LED is not useful
> > > > > for me, and I'd like to use it for notifications instead.
> > > > 
> > > > I'm not against adding the LEDs device implementation for any exotic
> > > > usage.
> > > > 
> > > > But for the audio mute LED features, you'll need really lots of other
> > > > works if it were implemented via leds device.  That's the hardest
> > > > part, and a few lines of hooks solves it easily in the kernel side.
> > > > That's all about it.
> > > > 
> > > > If you are ready for submitting the real solutions in user-space side
> > > > (patching PulseAudio and whatever all existing sound daemons, and
> > > > creating yet another daemon for non-PA systems (another footprint,
> > > > lovely), and so on), we can happily delete such in-kernel hooks :)
> > > 
> > > I'm not saying we should move it to the userspace.
> > > 
> > > I'm saying this should be "normal" led. drivers/leds/led-huawei-acpi.c,
> > > or something. Perhaps this acpi stuff is so similar you don't really
> > > need .c code.
> > > 
> > > And then there should be a "mic muted" trigger. Similar to
> > > drivers/leds/trigger/ledtrig-disk.c.
> > 
> > And who will trigger this, e.g. when the mixer is muted?
> 
> Same code that does it now.

But it needs the binding, and currently it's done by querying the
exported symbol.  So we do need still that API.


Takashi

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Pavel Machek]

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

On Thu 2018-11-22 13:36:43, Andy Shevchenko wrote:
> On Tue, Nov 20, 2018 at 2:19 PM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > On Tue, 20 Nov 2018 12:51:59 +0100,
> > Pavel Machek wrote:
> > >
> > > Hi!
> > >
> > > > > You have general-purpose LED, yet you are treating it as "something
> > > > > special". That means ugly code (quoted above) and lack of flexibility.
> > > > >
> > > > > For example, if my notebook lacks HDD LED, I can use scrollock LED for
> > > > > that instead. Or, in reverse way, maybe "mic mute" LED is not useful
> > > > > for me, and I'd like to use it for notifications instead.
> > > >
> > > > I'm not against adding the LEDs device implementation for any exotic
> > > > usage.
> > > >
> > > > But for the audio mute LED features, you'll need really lots of other
> > > > works if it were implemented via leds device.  That's the hardest
> > > > part, and a few lines of hooks solves it easily in the kernel side.
> > > > That's all about it.
> > > >
> > > > If you are ready for submitting the real solutions in user-space side
> > > > (patching PulseAudio and whatever all existing sound daemons, and
> > > > creating yet another daemon for non-PA systems (another footprint,
> > > > lovely), and so on), we can happily delete such in-kernel hooks :)
> > >
> > > I'm not saying we should move it to the userspace.
> > >
> > > I'm saying this should be "normal" led. drivers/leds/led-huawei-acpi.c,
> > > or something. Perhaps this acpi stuff is so similar you don't really
> > > need .c code.
> > >
> > > And then there should be a "mic muted" trigger. Similar to
> > > drivers/leds/trigger/ledtrig-disk.c.
> >
> > And who will trigger this, e.g. when the mixer is muted?
> 
> Is this settled? I'm encouraged to promote this series to our for-next branch.

I'd prefer this to be normal LED and "mic muted" to become normal
trigger.

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

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

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Takashi Iwai]

On Thu, 22 Nov 2018 14:18:02 +0100,
Pavel Machek wrote:
> 
> On Thu 2018-11-22 13:36:43, Andy Shevchenko wrote:
> > On Tue, Nov 20, 2018 at 2:19 PM Takashi Iwai <tiwai@suse.de> wrote:
> > >
> > > On Tue, 20 Nov 2018 12:51:59 +0100,
> > > Pavel Machek wrote:
> > > >
> > > > Hi!
> > > >
> > > > > > You have general-purpose LED, yet you are treating it as "something
> > > > > > special". That means ugly code (quoted above) and lack of flexibility.
> > > > > >
> > > > > > For example, if my notebook lacks HDD LED, I can use scrollock LED for
> > > > > > that instead. Or, in reverse way, maybe "mic mute" LED is not useful
> > > > > > for me, and I'd like to use it for notifications instead.
> > > > >
> > > > > I'm not against adding the LEDs device implementation for any exotic
> > > > > usage.
> > > > >
> > > > > But for the audio mute LED features, you'll need really lots of other
> > > > > works if it were implemented via leds device.  That's the hardest
> > > > > part, and a few lines of hooks solves it easily in the kernel side.
> > > > > That's all about it.
> > > > >
> > > > > If you are ready for submitting the real solutions in user-space side
> > > > > (patching PulseAudio and whatever all existing sound daemons, and
> > > > > creating yet another daemon for non-PA systems (another footprint,
> > > > > lovely), and so on), we can happily delete such in-kernel hooks :)
> > > >
> > > > I'm not saying we should move it to the userspace.
> > > >
> > > > I'm saying this should be "normal" led. drivers/leds/led-huawei-acpi.c,
> > > > or something. Perhaps this acpi stuff is so similar you don't really
> > > > need .c code.
> > > >
> > > > And then there should be a "mic muted" trigger. Similar to
> > > > drivers/leds/trigger/ledtrig-disk.c.
> > >
> > > And who will trigger this, e.g. when the mixer is muted?
> > 
> > Is this settled? I'm encouraged to promote this series to our for-next branch.
> 
> I'd prefer this to be normal LED and "mic muted" to become normal
> trigger.

But how would you solve the existing problem?

As already mentioned, you'll need to hook the LED trigger and the
actual mixer value change.  This is the biggest missing piece, and
it's the very reason we have the exported symbol from the platform
driver side.

So, if you prefer in that way, please implement that for the existing
driver (thinkpad_acpi and dell-laptop) at first.  I'll be really happy
to get rid of the present ugly solution!  But it's been there just
because it's not so trivial at all.  FWIW, this must be all done
inside the kernel; otherwise you'll hit a regression.

If the path via leds class can't be achieved quickly, I'd prefer
taking the current approach at first.  Once after it's done for those
above, we can apply the same for huawei-wmi later, too.


thanks,

Takashi

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Pavel Machek]

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

HI!

> > > > > > > You have general-purpose LED, yet you are treating it as "something
> > > > > > > special". That means ugly code (quoted above) and lack of flexibility.
> > > > > > >
> > > > > > > For example, if my notebook lacks HDD LED, I can use scrollock LED for
> > > > > > > that instead. Or, in reverse way, maybe "mic mute" LED is not useful
> > > > > > > for me, and I'd like to use it for notifications instead.
> > > > > >
> > > > > > I'm not against adding the LEDs device implementation for any exotic
> > > > > > usage.
> > > > > >
> > > > > > But for the audio mute LED features, you'll need really lots of other
> > > > > > works if it were implemented via leds device.  That's the hardest
> > > > > > part, and a few lines of hooks solves it easily in the kernel side.
> > > > > > That's all about it.
> > > > > >
> > > > > > If you are ready for submitting the real solutions in user-space side
> > > > > > (patching PulseAudio and whatever all existing sound daemons, and
> > > > > > creating yet another daemon for non-PA systems (another footprint,
> > > > > > lovely), and so on), we can happily delete such in-kernel hooks :)
> > > > >
> > > > > I'm not saying we should move it to the userspace.
> > > > >
> > > > > I'm saying this should be "normal" led. drivers/leds/led-huawei-acpi.c,
> > > > > or something. Perhaps this acpi stuff is so similar you don't really
> > > > > need .c code.
> > > > >
> > > > > And then there should be a "mic muted" trigger. Similar to
> > > > > drivers/leds/trigger/ledtrig-disk.c.
> > > >
> > > > And who will trigger this, e.g. when the mixer is muted?
> > > 
> > > Is this settled? I'm encouraged to promote this series to our for-next branch.
> > 
> > I'd prefer this to be normal LED and "mic muted" to become normal
> > trigger.
> 
> But how would you solve the existing problem?
> 
> As already mentioned, you'll need to hook the LED trigger and the
> actual mixer value change.  This is the biggest missing piece, and
> it's the very reason we have the exported symbol from the platform
> driver side.
> 
> So, if you prefer in that way, please implement that for the existing
> driver (thinkpad_acpi and dell-laptop) at first.  I'll be really happy
> to get rid of the present ugly solution!  But it's been there just
> because it's not so trivial at all.  FWIW, this must be all done
> inside the kernel; otherwise you'll hit a regression.

I am really have enough work to do at the moment, but let me take a look.

> If the path via leds class can't be achieved quickly, I'd prefer
> taking the current approach at first.  Once after it's done for those
> above, we can apply the same for huawei-wmi later, too.

I'd prefer not adding more mess. Don't expect full solution from me,
but ... I should have something better than what is in the tree rather
soon.

Best regards,
									Pavel

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

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

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Pavel Machek]

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

Hi!

> > > > > > > You have general-purpose LED, yet you are treating it as "something
> > > > > > > special". That means ugly code (quoted above) and lack of flexibility.
> > > > > > >

> > I'd prefer this to be normal LED and "mic muted" to become normal
> > trigger.
> 
> But how would you solve the existing problem?
> 
> As already mentioned, you'll need to hook the LED trigger and the
> actual mixer value change.  This is the biggest missing piece, and
> it's the very reason we have the exported symbol from the platform
> driver side.
> 
> So, if you prefer in that way, please implement that for the existing
> driver (thinkpad_acpi and dell-laptop) at first.  I'll be really happy
> to get rid of the present ugly solution!  But it's been there just
> because it's not so trivial at all.  FWIW, this must be all done
> inside the kernel; otherwise you'll hit a regression.

Ok, what about something like this?

Tested, and it did not work. I guess I hooked it up at the wrong place
in LED subsystem... or maybe thinkpad x60 is wrong machine to test on.

Anyway, it looks less ugly than current code in alsa. We should not
really be using mixer settings to turn LED on and off.

Plus, it works in similar way triggers and LEDs "usually" do, and has
all the flexibility.

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

commit e7d6d170f2f45ea7a42c9ebb31869f440382e3ad

    leds: ledtrig-sound: provide indication of muted microphone
    
    Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 9bcb64e..c5ea19e 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-y					+= ledtrig-sound.o
diff --git a/drivers/leds/trigger/ledtrig-sound.c b/drivers/leds/trigger/ledtrig-sound.c
new file mode 100644
index 0000000..608df35
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-sound.c
@@ -0,0 +1,23 @@
+/* GPLv2+.
+ * Copyright 2018 Pavel Machek <pavel@ucw.cz>
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+
+DEFINE_LED_TRIGGER(ledtrig_micmute);
+
+void ledtrig_mic_muted(bool muted)
+{
+	led_trigger_event(ledtrig_micmute, muted ? LED_FULL : LED_OFF);
+}
+EXPORT_SYMBOL(ledtrig_mic_muted);
+
+static int __init ledtrig_micmute_init(void)
+{
+	led_trigger_register_simple("mic-muted", &ledtrig_micmute);
+
+	return 0;
+}
+device_initcall(ledtrig_micmute_init);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index e4a9a00..c30e1cb 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -494,6 +494,14 @@ static inline void ledtrig_cpu(enum cpu_led_event evt)
 }
 #endif
 
+#ifdef CONFIG_LEDS_TRIGGERS
+extern void ledtrig_mic_muted(bool m);
+#else
+static inline void ledtrig_mic_muted(bool m) {}
+#endif
+
+
+
 #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
 extern void led_classdev_notify_brightness_hw_changed(
 	struct led_classdev *led_cdev, enum led_brightness brightness);
diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
index 276150f..3ec1f61 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>
@@ -3929,6 +3930,7 @@ static void call_micmute_led_update(struct hda_codec *codec)
 		val = !spec->micmute_led.capture;
 		break;
 	}
+	ledtrig_mic_muted(!spec->micmute_led.capture);
 
 	if (val == spec->micmute_led.led_value)
 		return;
diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
index 568575b..f1b2265 100644
--- a/sound/pci/hda/thinkpad_helper.c
+++ b/sound/pci/hda/thinkpad_helper.c
@@ -23,6 +23,8 @@ static void update_tpacpi_mute_led(void *private_data, int enabled)
 	if (old_vmaster_hook)
 		old_vmaster_hook(private_data, enabled);
 
+	printk("mute led... %d\n", !enabled);
+
 	if (led_set_func)
 		led_set_func(TPACPI_LED_MUTE, !enabled);
 }


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

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

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Takashi Iwai]

On Sat, 24 Nov 2018 00:33:09 +0100,
Pavel Machek wrote:
> 
> Hi!
> 
> > > > > > > > You have general-purpose LED, yet you are treating it as "something
> > > > > > > > special". That means ugly code (quoted above) and lack of flexibility.
> > > > > > > >
> 
> > > I'd prefer this to be normal LED and "mic muted" to become normal
> > > trigger.
> > 
> > But how would you solve the existing problem?
> > 
> > As already mentioned, you'll need to hook the LED trigger and the
> > actual mixer value change.  This is the biggest missing piece, and
> > it's the very reason we have the exported symbol from the platform
> > driver side.
> > 
> > So, if you prefer in that way, please implement that for the existing
> > driver (thinkpad_acpi and dell-laptop) at first.  I'll be really happy
> > to get rid of the present ugly solution!  But it's been there just
> > because it's not so trivial at all.  FWIW, this must be all done
> > inside the kernel; otherwise you'll hit a regression.
> 
> Ok, what about something like this?
> 
> Tested, and it did not work. I guess I hooked it up at the wrong place
> in LED subsystem... or maybe thinkpad x60 is wrong machine to test on.
> 
> Anyway, it looks less ugly than current code in alsa. We should not
> really be using mixer settings to turn LED on and off.
> 
> Plus, it works in similar way triggers and LEDs "usually" do, and has
> all the flexibility.

Yes, thanks, that's something similar as what I had in mind, too.
I guess it's just a matter of thinkpad_acpi side implementation that
differs from your expectation.

However, one remaining problem is that the state will be inconsistent
depending on the driver module order, if we get rid of the dynamic
symbol binding.  Then both modules become completely individual and
thinkpad_acpi can be loaded at anytime later than HD-audio codec.
If a mixer state is already changed before loading thinkpad_acpi, this
event is lost and the state may be different in both sides.

So, we'd need to record the state in the mute-trigger side, and add a
function to return the current state.  Then thinkpad_acpi will query
the state at the initialization time.

Also, we'll need also the normal mute LED in addition to the mic mute
LED, so there need to be two triggers.

In anyway, moving to this direction requires the leds class
implementation for dell-laptop.c as well as the new huawei stuff.  So,
I'd really like to have the "already good working" code before
actually hacking this, so that we can see and track the functional
regression more obviously.

I can start hacking this in the next week.  At least I have a Dell
laptop and I can check the part of the changes locally.

Since the changes will be fairly cross-tree, it's better to have an
immutable branch to start with.  I'd branch off at 4.20-rc3 (or rc4),
apply the current patches, so that it can be merged to all relevant
trees cleanly.

Andy, could you give your sign-off for the platform part of Huawei
bits?


thanks,

Takashi

> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> commit e7d6d170f2f45ea7a42c9ebb31869f440382e3ad
> 
>     leds: ledtrig-sound: provide indication of muted microphone
>     
>     Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index 9bcb64e..c5ea19e 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-y					+= ledtrig-sound.o
> diff --git a/drivers/leds/trigger/ledtrig-sound.c b/drivers/leds/trigger/ledtrig-sound.c
> new file mode 100644
> index 0000000..608df35
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-sound.c
> @@ -0,0 +1,23 @@
> +/* GPLv2+.
> + * Copyright 2018 Pavel Machek <pavel@ucw.cz>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/leds.h>
> +
> +DEFINE_LED_TRIGGER(ledtrig_micmute);
> +
> +void ledtrig_mic_muted(bool muted)
> +{
> +	led_trigger_event(ledtrig_micmute, muted ? LED_FULL : LED_OFF);
> +}
> +EXPORT_SYMBOL(ledtrig_mic_muted);
> +
> +static int __init ledtrig_micmute_init(void)
> +{
> +	led_trigger_register_simple("mic-muted", &ledtrig_micmute);
> +
> +	return 0;
> +}
> +device_initcall(ledtrig_micmute_init);
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index e4a9a00..c30e1cb 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -494,6 +494,14 @@ static inline void ledtrig_cpu(enum cpu_led_event evt)
>  }
>  #endif
>  
> +#ifdef CONFIG_LEDS_TRIGGERS
> +extern void ledtrig_mic_muted(bool m);
> +#else
> +static inline void ledtrig_mic_muted(bool m) {}
> +#endif
> +
> +
> +
>  #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
>  extern void led_classdev_notify_brightness_hw_changed(
>  	struct led_classdev *led_cdev, enum led_brightness brightness);
> diff --git a/sound/pci/hda/hda_generic.c b/sound/pci/hda/hda_generic.c
> index 276150f..3ec1f61 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>
> @@ -3929,6 +3930,7 @@ static void call_micmute_led_update(struct hda_codec *codec)
>  		val = !spec->micmute_led.capture;
>  		break;
>  	}
> +	ledtrig_mic_muted(!spec->micmute_led.capture);
>  
>  	if (val == spec->micmute_led.led_value)
>  		return;
> diff --git a/sound/pci/hda/thinkpad_helper.c b/sound/pci/hda/thinkpad_helper.c
> index 568575b..f1b2265 100644
> --- a/sound/pci/hda/thinkpad_helper.c
> +++ b/sound/pci/hda/thinkpad_helper.c
> @@ -23,6 +23,8 @@ static void update_tpacpi_mute_led(void *private_data, int enabled)
>  	if (old_vmaster_hook)
>  		old_vmaster_hook(private_data, enabled);
>  
> +	printk("mute led... %d\n", !enabled);
> +
>  	if (led_set_func)
>  		led_set_func(TPACPI_LED_MUTE, !enabled);
>  }
> 
> 
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> [2 Digital signature <application/pgp-signature (7bit)>]
> 

Re: [PATCH v3 3/3] ALSA: hda: add support for Huawei WMI micmute LED

[Pavel Machek]

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

Hi!

> > > > > > > > > You have general-purpose LED, yet you are treating it as "something
> > > > > > > > > special". That means ugly code (quoted above) and lack of flexibility.
> > > > > > > > >
> > 
> > > > I'd prefer this to be normal LED and "mic muted" to become normal
> > > > trigger.
> > > 
> > > But how would you solve the existing problem?
> > > 
> > > As already mentioned, you'll need to hook the LED trigger and the
> > > actual mixer value change.  This is the biggest missing piece, and
> > > it's the very reason we have the exported symbol from the platform
> > > driver side.
> > > 
> > > So, if you prefer in that way, please implement that for the existing
> > > driver (thinkpad_acpi and dell-laptop) at first.  I'll be really happy
> > > to get rid of the present ugly solution!  But it's been there just
> > > because it's not so trivial at all.  FWIW, this must be all done
> > > inside the kernel; otherwise you'll hit a regression.
> > 
> > Ok, what about something like this?
> > 
> > Tested, and it did not work. I guess I hooked it up at the wrong place
> > in LED subsystem... or maybe thinkpad x60 is wrong machine to test on.

I meant to say "wrong place in ALSA subsystem".

> > Anyway, it looks less ugly than current code in alsa. We should not
> > really be using mixer settings to turn LED on and off.
> > 
> > Plus, it works in similar way triggers and LEDs "usually" do, and has
> > all the flexibility.
> 
> Yes, thanks, that's something similar as what I had in mind, too.
> I guess it's just a matter of thinkpad_acpi side implementation that
> differs from your expectation.
> 
> However, one remaining problem is that the state will be inconsistent
> depending on the driver module order, if we get rid of the dynamic
> symbol binding.  Then both modules become completely individual and
> thinkpad_acpi can be loaded at anytime later than HD-audio codec.
> If a mixer state is already changed before loading thinkpad_acpi, this
> event is lost and the state may be different in both sides.



> So, we'd need to record the state in the mute-trigger side, and add a
> function to return the current state.  Then thinkpad_acpi will query
> the state at the initialization time.

We want to record state at mute-trigger side, yes. Should not be
really different from other triggers.

> Also, we'll need also the normal mute LED in addition to the mic mute
> LED, so there need to be two triggers.

Yes.

> In anyway, moving to this direction requires the leds class
> implementation for dell-laptop.c as well as the new huawei stuff.  So,
> I'd really like to have the "already good working" code before
> actually hacking this, so that we can see and track the functional
> regression more obviously.

As well as thinkpad; note I did not do that part, because I don't have
that LED on my test machine.

Anyway, normally we get the architecture right and only then we merge
the drivers.

All Huawei hackers need to do is to convert their driver into normal
LED driver, and test it with some other trigger (like heartbeat). Then
we can be fairly sure it will work when we get the micmute done.

> I can start hacking this in the next week.  At least I have a Dell
> laptop and I can check the part of the changes locally.
> 
> Since the changes will be fairly cross-tree, it's better to have an
> immutable branch to start with.  I'd branch off at 4.20-rc3 (or rc4),
> apply the current patches, so that it can be merged to all relevant
> trees cleanly.

As you will not change any core LED code, I don't think any heavy
synchronization with LED tree should be neccessary. We have LED
triggers living outside drivers/leds/triggers, too, such as
drivers/usb/common/led.c .

(Actually, that's the beauty of the LED subsystem. You have standard
trigger. You have standard LED -- which says it prefers that trigger if
available. They are really independend, so you can test and merge them
separately).

Let me know if you need any more help.

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

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