On Tue, 1 Oct 2024, Werner Sembach wrote:ok
The TUXEDO Sirius 16 Gen1 and TUXEDO Sirius 16 Gen2 devices have a per-keyPlease limit comment length to 80 chars and since you need multiple lines
controllable RGB keyboard backlight. The firmware API for it is implemented
via WMI.
To make the backlight userspace configurable this driver emulates a
LampArray HID device and translates the input from hidraw to the
corresponding WMI calls. This is a new approach as the leds subsystem lacks
a suitable UAPI for per-key keyboard backlights, and like this no new UAPI
needs to be established.
v2: Integrated Armins feedback and fixed kernel test robot warnings.
v3: Fixed borked subject line of v2.
v4: Remove unrequired WMI mutex.
Move device checking from probe to init.
Fix device checking working exactly reverse as it should.
Fix null pointer dereference because, hdev->driver_data != hdev->dev.driver_data.
Co-developed-by: Christoffer Sandberg <cs@xxxxxxxxx>
Signed-off-by: Christoffer Sandberg <cs@xxxxxxxxx>
Signed-off-by: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>
Link: https://lore.kernel.org/all/1fb08a74-62c7-4d0c-ba5d-648e23082dcb@xxxxxxxxxxxxxxxxxxx/
---
MAINTAINERS | 6 +
drivers/platform/x86/Kconfig | 2 +
drivers/platform/x86/Makefile | 3 +
drivers/platform/x86/tuxedo/Kbuild | 9 +
drivers/platform/x86/tuxedo/Kconfig | 14 +
.../x86/tuxedo/tuxedo_nb04_wmi_ab_init.c | 99 +++
.../x86/tuxedo/tuxedo_nb04_wmi_ab_init.h | 19 +
.../tuxedo_nb04_wmi_ab_virtual_lamp_array.c | 735 ++++++++++++++++++
.../tuxedo_nb04_wmi_ab_virtual_lamp_array.h | 18 +
.../x86/tuxedo/tuxedo_nb04_wmi_util.c | 82 ++
.../x86/tuxedo/tuxedo_nb04_wmi_util.h | 112 +++
11 files changed, 1099 insertions(+)
create mode 100644 drivers/platform/x86/tuxedo/Kbuild
create mode 100644 drivers/platform/x86/tuxedo/Kconfig
create mode 100644 drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_ab_init.c
create mode 100644 drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_ab_init.h
create mode 100644 drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_ab_virtual_lamp_array.c
create mode 100644 drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_ab_virtual_lamp_array.h
create mode 100644 drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_util.c
create mode 100644 drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_util.h
diff --git a/MAINTAINERS b/MAINTAINERS
index cc40a9d9b8cd1..3385ad51af194 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23358,6 +23358,12 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux.git turbostat
F: tools/power/x86/turbostat/
F: tools/testing/selftests/turbostat/
+TUXEDO DRIVERS
+M: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>
+L: platform-driver-x86@xxxxxxxxxxxxxxx
+S: Supported
+F: drivers/platform/x86/tuxedo/
+
TW5864 VIDEO4LINUX DRIVER
M: Bluecherry Maintainers <maintainers@xxxxxxxxxxxxxxxxx>
M: Andrey Utkin <andrey.utkin@xxxxxxxxxxxxxxxxxxx>
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index ddfccc226751f..c7cffb222adac 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1196,3 +1196,5 @@ config P2SB
The main purpose of this library is to unhide P2SB device in case
firmware kept it hidden on some platforms in order to access devices
behind it.
+
+source "drivers/platform/x86/tuxedo/Kconfig"
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index e1b1429470674..1562dcd7ad9a5 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -153,3 +153,6 @@ obj-$(CONFIG_WINMATE_FM07_KEYS) += winmate-fm07-keys.o
# SEL
obj-$(CONFIG_SEL3350_PLATFORM) += sel3350-platform.o
+
+# TUXEDO
+obj-y += tuxedo/
diff --git a/drivers/platform/x86/tuxedo/Kbuild b/drivers/platform/x86/tuxedo/Kbuild
new file mode 100644
index 0000000000000..5a3506ab98131
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/Kbuild
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# TUXEDO X86 Platform Specific Drivers
+#
+
+tuxedo_nb04_wmi_ab-y := tuxedo_nb04_wmi_ab_init.o
+tuxedo_nb04_wmi_ab-y += tuxedo_nb04_wmi_util.o
+tuxedo_nb04_wmi_ab-y += tuxedo_nb04_wmi_ab_virtual_lamp_array.o
+obj-$(CONFIG_TUXEDO_NB04_WMI_AB) += tuxedo_nb04_wmi_ab.o
diff --git a/drivers/platform/x86/tuxedo/Kconfig b/drivers/platform/x86/tuxedo/Kconfig
new file mode 100644
index 0000000000000..b1f7c6ceeaae4
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/Kconfig
@@ -0,0 +1,14 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# TUXEDO X86 Platform Specific Drivers
+#
+
+menuconfig TUXEDO_NB04_WMI_AB
+ tristate "TUXEDO NB04 WMI AB Platform Driver"
+ default m
+ help
+ This driver implements the WMI AB device found on TUXEDO Notebooks
+ with board vendor NB04. For the time being only the keyboard backlight
+ control is implemented.
+
+ When compiled as a module it will be called tuxedo_nb04_wmi_ab.
diff --git a/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_ab_init.c b/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_ab_init.c
new file mode 100644
index 0000000000000..bd5bca3b15bc7
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_ab_init.c
@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This driver implements the WMI AB device found on TUXEDO Notebooks with board
+ * vendor NB04.
+ *
+ * Copyright (C) 2024 Werner Sembach wse@xxxxxxxxxxxxxxxxxxx
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/wmi.h>
+#include <linux/dmi.h>
+
+#include "tuxedo_nb04_wmi_ab_virtual_lamp_array.h"
+
+#include "tuxedo_nb04_wmi_ab_init.h"
+
+static int probe(struct wmi_device *wdev, const void __always_unused *context)
+{
+ struct tuxedo_nb04_wmi_driver_data_t *driver_data;
+ int ret;
+
+ driver_data = devm_kzalloc(&wdev->dev, sizeof(*driver_data), GFP_KERNEL);
+ if (!driver_data)
+ return -ENOMEM;
+
+ dev_set_drvdata(&wdev->dev, driver_data);
+
+ ret = tuxedo_nb04_virtual_lamp_array_add_device(wdev,
+ &driver_data->virtual_lamp_array_hdev);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static void remove(struct wmi_device *wdev)
+{
+ struct tuxedo_nb04_wmi_driver_data_t *driver_data = dev_get_drvdata(&wdev->dev);
+
+ hid_destroy_device(driver_data->virtual_lamp_array_hdev);
+}
+
+static const struct wmi_device_id tuxedo_nb04_wmi_ab_device_ids[] = {
+ { .guid_string = "80C9BAA6-AC48-4538-9234-9F81A55E7C85" },
+ { }
+};
+MODULE_DEVICE_TABLE(wmi, tuxedo_nb04_wmi_ab_device_ids);
+
+static struct wmi_driver tuxedo_nb04_wmi_ab_driver = {
+ .driver = {
+ .name = "tuxedo_nb04_wmi_ab",
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ },
+ .id_table = tuxedo_nb04_wmi_ab_device_ids,
+ .probe = probe,
+ .remove = remove,
+ .no_singleton = true,
+};
+
+// We don't know if the WMI API is stable and how unique the GUID is for this ODM. To be on the safe
+// side we therefore only run this driver on tested devices defined by this list.
here anyway, use the usual /* */ multiline comment formatting.
+static const struct dmi_system_id tested_devices_dmi_table[] __initconst = {Please add all the includes here for the things you use below (in
+ {
+ // TUXEDO Sirius 16 Gen1
+ .matches = {
+ DMI_EXACT_MATCH(DMI_SYS_VENDOR, "TUXEDO"),
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "APX958"),
+ },
+ },
+ {
+ // TUXEDO Sirius 16 Gen2
+ .matches = {
+ DMI_EXACT_MATCH(DMI_SYS_VENDOR, "TUXEDO"),
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "AHP958"),
+ },
+ },
+ { }
+};
+
+static int __init tuxedo_nb04_wmi_ab_init(void)
+{
+ if (!dmi_check_system(tested_devices_dmi_table))
+ return -ENODEV;
+
+ return wmi_driver_register(&tuxedo_nb04_wmi_ab_driver);
+}
+module_init(tuxedo_nb04_wmi_ab_init);
+
+static void __exit tuxedo_nb04_wmi_ab_exit(void)
+{
+ return wmi_driver_unregister(&tuxedo_nb04_wmi_ab_driver);
+}
+module_exit(tuxedo_nb04_wmi_ab_exit);
+
+MODULE_DESCRIPTION("Virtual HID LampArray interface for TUXEDO NB04 devices");
+MODULE_AUTHOR("Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_ab_init.h b/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_ab_init.h
new file mode 100644
index 0000000000000..3306e3d28bcab
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_ab_init.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * This driver implements the WMI AB device found on TUXEDO Notebooks with board
+ * vendor NB04.
+ *
+ * Copyright (C) 2024 Werner Sembach wse@xxxxxxxxxxxxxxxxxxx
+ */
+
+#ifndef TUXEDO_NB04_WMI_AB_INIT_H
+#define TUXEDO_NB04_WMI_AB_INIT_H
+
+#include <linux/mutex.h>
+#include <linux/hid.h>
+
+struct tuxedo_nb04_wmi_driver_data_t {
+ struct hid_device *virtual_lamp_array_hdev;
+};
+
+#endif
diff --git a/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_ab_virtual_lamp_array.c b/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_ab_virtual_lamp_array.c
new file mode 100644
index 0000000000000..906eb59c5cfd2
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_ab_virtual_lamp_array.c
@@ -0,0 +1,735 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This code gives the built in RGB lighting of the TUXEDO NB04 devices a
+ * standardised interface, namely HID LampArray.
+ *
+ * Copyright (C) 2024 Werner Sembach wse@xxxxxxxxxxxxxxxxxxx
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
alphabetical order).
+#include "tuxedo_nb04_wmi_util.h"u8 for kernel internal things.
+
+#include "tuxedo_nb04_wmi_ab_virtual_lamp_array.h"
+
+enum report_ids {
+ LAMP_ARRAY_ATTRIBUTES_REPORT_ID = 0x01,
+ LAMP_ATTRIBUTES_REQUEST_REPORT_ID = 0x02,
+ LAMP_ATTRIBUTES_RESPONSE_REPORT_ID = 0x03,
+ LAMP_MULTI_UPDATE_REPORT_ID = 0x04,
+ LAMP_RANGE_UPDATE_REPORT_ID = 0x05,
+ LAMP_ARRAY_CONTROL_REPORT_ID = 0x06,
+};
+
+static const uint8_t sirius_16_ansii_kbl_mapping[] = {
+ 0x29, 0x3a, 0x3b, 0x3c, 0x3d, 0x3e, 0x3f, 0x40, 0x41, 0x42,Why are these aligned in the odd way?
+ 0x43, 0x44, 0x45, 0xf1, 0x46, 0x4c, 0x4a, 0x4d, 0x4b, 0x4e,
+ 0x35, 0x1e, 0x1f, 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26,
+ 0x27, 0x2d, 0x2e, 0x2a, 0x53, 0x55, 0x54, 0x56,
+ 0x2b, 0x14, 0x1a, 0x08, 0x15, 0x17, 0x1c, 0x18, 0x0c, 0x12,
+ 0x13, 0x2f, 0x30, 0x31, 0x5f, 0x60, 0x61,
+ 0x39, 0x04, 0x16, 0x07, 0x09, 0x0a, 0x0b, 0x0d, 0x0e, 0x0f,
+ 0x33, 0x34, 0x28, 0x5c, 0x5d, 0x5e, 0x57,
+ 0xe1, 0x1d, 0x1b, 0x06, 0x19, 0x05, 0x11, 0x10, 0x36, 0x37,
+ 0x38, 0xe5, 0x52, 0x59, 0x5a, 0x5b,
+ 0xe0, 0xfe, 0xe3, 0xe2, 0x2c, 0xe6, 0x65, 0xe4, 0x50, 0x51,
+ 0x4f, 0x62, 0x63, 0x58
ok
+};u32
+
+static const uint32_t sirius_16_ansii_kbl_mapping_pos_x[] = {
ok
+ 25000, 41700, 58400, 75100, 91800, 108500, 125200, 141900, 158600, 175300,Reverse xmas-tree order.
+ 192000, 208700, 225400, 242100, 258800, 275500, 294500, 311200, 327900, 344600,
+ 24500, 42500, 61000, 79500, 98000, 116500, 135000, 153500, 172000, 190500,
+ 209000, 227500, 246000, 269500, 294500, 311200, 327900, 344600,
+ 31000, 51500, 70000, 88500, 107000, 125500, 144000, 162500, 181000, 199500,
+ 218000, 236500, 255000, 273500, 294500, 311200, 327900,
+ 33000, 57000, 75500, 94000, 112500, 131000, 149500, 168000, 186500, 205000,
+ 223500, 242000, 267500, 294500, 311200, 327900, 344600,
+ 37000, 66000, 84500, 103000, 121500, 140000, 158500, 177000, 195500, 214000,
+ 232500, 251500, 273500, 294500, 311200, 327900,
+ 28000, 47500, 66000, 84500, 140000, 195500, 214000, 234000, 255000, 273500,
+ 292000, 311200, 327900, 344600
+};
+
+static const uint32_t sirius_16_ansii_kbl_mapping_pos_y[] = {
+ 53000, 53000, 53000, 53000, 53000, 53000, 53000, 53000, 53000, 53000,
+ 53000, 53000, 53000, 53000, 53000, 53000, 53000, 53000, 53000, 53000,
+ 67500, 67500, 67500, 67500, 67500, 67500, 67500, 67500, 67500, 67500,
+ 67500, 67500, 67500, 67500, 67500, 67500, 67500, 67500,
+ 85500, 85500, 85500, 85500, 85500, 85500, 85500, 85500, 85500, 85500,
+ 85500, 85500, 85500, 85500, 85500, 85500, 85500,
+ 103500, 103500, 103500, 103500, 103500, 103500, 103500, 103500, 103500, 103500,
+ 103500, 103500, 103500, 103500, 103500, 103500, 94500,
+ 121500, 121500, 121500, 121500, 121500, 121500, 121500, 121500, 121500, 121500,
+ 121500, 121500, 129000, 121500, 121500, 121500,
+ 139500, 139500, 139500, 139500, 139500, 139500, 139500, 139500, 147000, 147000,
+ 147000, 139500, 139500, 130500
+};
+
+static const uint32_t sirius_16_ansii_kbl_mapping_pos_z[] = {
+ 5000, 5000, 5000, 5000, 5000, 5000, 5000, 5000, 5000, 5000,
+ 5000, 5000, 5000, 5000, 5000, 5000, 5000, 5000, 5000, 5000,
+ 5250, 5250, 5250, 5250, 5250, 5250, 5250, 5250, 5250, 5250,
+ 5250, 5250, 5250, 5250, 5250, 5250, 5250, 5250,
+ 5500, 5500, 5500, 5500, 5500, 5500, 5500, 5500, 5500, 5500,
+ 5500, 5500, 5500, 5500, 5500, 5500, 5500,
+ 5750, 5750, 5750, 5750, 5750, 5750, 5750, 5750, 5750, 5750,
+ 5750, 5750, 5750, 5750, 5750, 5750, 5625,
+ 6000, 6000, 6000, 6000, 6000, 6000, 6000, 6000, 6000, 6000,
+ 6000, 6000, 6125, 6000, 6000, 6000,
+ 6250, 6250, 6250, 6250, 6250, 6250, 6250, 6250, 6375, 6375,
+ 6375, 6250, 6250, 6125
+};
+
+static const uint8_t sirius_16_iso_kbl_mapping[] = {
+ 0x29, 0x3a, 0x3b, 0x3c, 0x3d, 0x3e, 0x3f, 0x40, 0x41, 0x42,
+ 0x43, 0x44, 0x45, 0xf1, 0x46, 0x4c, 0x4a, 0x4d, 0x4b, 0x4e,
+ 0x35, 0x1e, 0x1f, 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26,
+ 0x27, 0x2d, 0x2e, 0x2a, 0x53, 0x55, 0x54, 0x56,
+ 0x2b, 0x14, 0x1a, 0x08, 0x15, 0x17, 0x1c, 0x18, 0x0c, 0x12,
+ 0x13, 0x2f, 0x30, 0x5f, 0x60, 0x61,
+ 0x39, 0x04, 0x16, 0x07, 0x09, 0x0a, 0x0b, 0x0d, 0x0e, 0x0f,
+ 0x33, 0x34, 0x32, 0x28, 0x5c, 0x5d, 0x5e, 0x57,
+ 0xe1, 0x64, 0x1d, 0x1b, 0x06, 0x19, 0x05, 0x11, 0x10, 0x36,
+ 0x37, 0x38, 0xe5, 0x52, 0x59, 0x5a, 0x5b,
+ 0xe0, 0xfe, 0xe3, 0xe2, 0x2c, 0xe6, 0x65, 0xe4, 0x50, 0x51,
+ 0x4f, 0x62, 0x63, 0x58
+};
+
+static const uint32_t sirius_16_iso_kbl_mapping_pos_x[] = {
+ 25000, 41700, 58400, 75100, 91800, 108500, 125200, 141900, 158600, 175300,
+ 192000, 208700, 225400, 242100, 258800, 275500, 294500, 311200, 327900, 344600,
+ 24500, 42500, 61000, 79500, 98000, 116500, 135000, 153500, 172000, 190500,
+ 209000, 227500, 246000, 269500, 294500, 311200, 327900, 344600,
+ 31000, 51500, 70000, 88500, 107000, 125500, 144000, 162500, 181000, 199500,
+ 218000, 234500, 251000, 294500, 311200, 327900,
+ 33000, 57000, 75500, 94000, 112500, 131000, 149500, 168000, 186500, 205000,
+ 223500, 240000, 256500, 271500, 294500, 311200, 327900, 344600,
+ 28000, 47500, 66000, 84500, 103000, 121500, 140000, 158500, 177000, 195500,
+ 214000, 232500, 251500, 273500, 294500, 311200, 327900,
+ 28000, 47500, 66000, 84500, 140000, 195500, 214000, 234000, 255000, 273500,
+ 292000, 311200, 327900, 344600
+};
+
+static const uint32_t sirius_16_iso_kbl_mapping_pos_y[] = {
+ 53000, 53000, 53000, 53000, 53000, 53000, 53000, 53000, 53000, 53000,
+ 53000, 53000, 53000, 53000, 53000, 53000, 53000, 53000, 53000, 53000,
+ 67500, 67500, 67500, 67500, 67500, 67500, 67500, 67500, 67500, 67500,
+ 67500, 67500, 67500, 67500, 67500, 67500, 67500, 67500,
+ 85500, 85500, 85500, 85500, 85500, 85500, 85500, 85500, 85500, 85500,
+ 85500, 85500, 85500, 85500, 85500, 85500,
+ 103500, 103500, 103500, 103500, 103500, 103500, 103500, 103500, 103500, 103500,
+ 103500, 103500, 103500, 94500, 103500, 103500, 103500, 94500,
+ 121500, 121500, 121500, 121500, 121500, 121500, 121500, 121500, 121500, 121500,
+ 121500, 121500, 121500, 129000, 121500, 121500, 121500,
+ 139500, 139500, 139500, 139500, 139500, 139500, 139500, 139500, 147000, 147000,
+ 147000, 139500, 139500, 130500
+};
+
+static const uint32_t sirius_16_iso_kbl_mapping_pos_z[] = {
+ 5000, 5000, 5000, 5000, 5000, 5000, 5000, 5000, 5000, 5000,
+ 5000, 5000, 5000, 5000, 5000, 5000, 5000, 5000, 5000, 5000,
+ 5250, 5250, 5250, 5250, 5250, 5250, 5250, 5250, 5250, 5250,
+ 5250, 5250, 5250, 5250, 5250, 5250, 5250, 5250,
+ 5500, 5500, 5500, 5500, 5500, 5500, 5500, 5500, 5500, 5500,
+ 5500, 5500, 5500, 5500, 5500, 5500,
+ 5750, 5750, 5750, 5750, 5750, 5750, 5750, 5750, 5750, 5750,
+ 5750, 5750, 5750, 5750, 5750, 5750, 5750, 5625,
+ 6000, 6000, 6000, 6000, 6000, 6000, 6000, 6000, 6000, 6000,
+ 6000, 6000, 6000, 6125, 6000, 6000, 6000,
+ 6250, 6250, 6250, 6250, 6250, 6250, 6250, 6250, 6375, 6375,
+ 6375, 6250, 6250, 6125
+};
+
+struct driver_data_t {
+ uint8_t keyboard_type;
+ uint8_t lamp_count;
+ uint8_t next_lamp_id;
+ union tuxedo_nb04_wmi_496_b_in_80_b_out_input next_kbl_set_multiple_keys_input;
+};
+
+
+static int ll_start(struct hid_device *hdev)
+{
+ struct driver_data_t *driver_data;
+ struct wmi_device *wdev = to_wmi_device(hdev->dev.parent);
+ int ret;
+ union tuxedo_nb04_wmi_8_b_in_80_b_out_input input;
+ union tuxedo_nb04_wmi_8_b_in_80_b_out_output output;
ok
+Use u8 for things internal to kernel.
+ driver_data = devm_kzalloc(&hdev->dev, sizeof(*driver_data), GFP_KERNEL);
+ if (!driver_data)
+ return -ENOMEM;
+
+ input.get_device_status_input.device_type = WMI_AB_GET_DEVICE_STATUS_DEVICE_ID_KEYBOARD;
+ ret = tuxedo_nb04_wmi_8_b_in_80_b_out(wdev, WMI_AB_GET_DEVICE_STATUS, &input, &output);
+ if (ret)
+ return ret;
+
+ driver_data->keyboard_type = output.get_device_status_output.keyboard_physical_layout;
+ driver_data->lamp_count = sizeof(sirius_16_ansii_kbl_mapping);
+ driver_data->next_lamp_id = 0;
+
+ hdev->driver_data = driver_data;
+
+ return ret;
+}
+
+
+static void ll_stop(struct hid_device __always_unused *hdev)
+{
+}
+
+
+static int ll_open(struct hid_device __always_unused *hdev)
+{
+ return 0;
+}
+
+
+static void ll_close(struct hid_device __always_unused *hdev)
+{
+}
+
+
+static uint8_t report_descriptor[327] = {
ok
+ 0x05, 0x59, // Usage Page (Lighting and Illumination)Use uXX types.
+ 0x09, 0x01, // Usage (Lamp Array)
+ 0xa1, 0x01, // Collection (Application)
+ 0x85, LAMP_ARRAY_ATTRIBUTES_REPORT_ID, // Report ID (1)
+ 0x09, 0x02, // Usage (Lamp Array Attributes Report)
+ 0xa1, 0x02, // Collection (Logical)
+ 0x09, 0x03, // Usage (Lamp Count)
+ 0x15, 0x00, // Logical Minimum (0)
+ 0x27, 0xff, 0xff, 0x00, 0x00, // Logical Maximum (65535)
+ 0x75, 0x10, // Report Size (16)
+ 0x95, 0x01, // Report Count (1)
+ 0xb1, 0x03, // Feature (Cnst,Var,Abs)
+ 0x09, 0x04, // Usage (Bounding Box Width In Micrometers)
+ 0x09, 0x05, // Usage (Bounding Box Height In Micrometers)
+ 0x09, 0x06, // Usage (Bounding Box Depth In Micrometers)
+ 0x09, 0x07, // Usage (Lamp Array Kind)
+ 0x09, 0x08, // Usage (Min Update Interval In Microseconds)
+ 0x15, 0x00, // Logical Minimum (0)
+ 0x27, 0xff, 0xff, 0xff, 0x7f, // Logical Maximum (2147483647)
+ 0x75, 0x20, // Report Size (32)
+ 0x95, 0x05, // Report Count (5)
+ 0xb1, 0x03, // Feature (Cnst,Var,Abs)
+ 0xc0, // End Collection
+ 0x85, LAMP_ATTRIBUTES_REQUEST_REPORT_ID, // Report ID (2)
+ 0x09, 0x20, // Usage (Lamp Attributes Request Report)
+ 0xa1, 0x02, // Collection (Logical)
+ 0x09, 0x21, // Usage (Lamp Id)
+ 0x15, 0x00, // Logical Minimum (0)
+ 0x27, 0xff, 0xff, 0x00, 0x00, // Logical Maximum (65535)
+ 0x75, 0x10, // Report Size (16)
+ 0x95, 0x01, // Report Count (1)
+ 0xb1, 0x02, // Feature (Data,Var,Abs)
+ 0xc0, // End Collection
+ 0x85, LAMP_ATTRIBUTES_RESPONSE_REPORT_ID, // Report ID (3)
+ 0x09, 0x22, // Usage (Lamp Attributes Response Report)
+ 0xa1, 0x02, // Collection (Logical)
+ 0x09, 0x21, // Usage (Lamp Id)
+ 0x15, 0x00, // Logical Minimum (0)
+ 0x27, 0xff, 0xff, 0x00, 0x00, // Logical Maximum (65535)
+ 0x75, 0x10, // Report Size (16)
+ 0x95, 0x01, // Report Count (1)
+ 0xb1, 0x02, // Feature (Data,Var,Abs)
+ 0x09, 0x23, // Usage (Position X In Micrometers)
+ 0x09, 0x24, // Usage (Position Y In Micrometers)
+ 0x09, 0x25, // Usage (Position Z In Micrometers)
+ 0x09, 0x27, // Usage (Update Latency In Microseconds)
+ 0x09, 0x26, // Usage (Lamp Purposes)
+ 0x15, 0x00, // Logical Minimum (0)
+ 0x27, 0xff, 0xff, 0xff, 0x7f, // Logical Maximum (2147483647)
+ 0x75, 0x20, // Report Size (32)
+ 0x95, 0x05, // Report Count (5)
+ 0xb1, 0x02, // Feature (Data,Var,Abs)
+ 0x09, 0x28, // Usage (Red Level Count)
+ 0x09, 0x29, // Usage (Green Level Count)
+ 0x09, 0x2a, // Usage (Blue Level Count)
+ 0x09, 0x2b, // Usage (Intensity Level Count)
+ 0x09, 0x2c, // Usage (Is Programmable)
+ 0x09, 0x2d, // Usage (Input Binding)
+ 0x15, 0x00, // Logical Minimum (0)
+ 0x26, 0xff, 0x00, // Logical Maximum (255)
+ 0x75, 0x08, // Report Size (8)
+ 0x95, 0x06, // Report Count (6)
+ 0xb1, 0x02, // Feature (Data,Var,Abs)
+ 0xc0, // End Collection
+ 0x85, LAMP_MULTI_UPDATE_REPORT_ID, // Report ID (4)
+ 0x09, 0x50, // Usage (Lamp Multi Update Report)
+ 0xa1, 0x02, // Collection (Logical)
+ 0x09, 0x03, // Usage (Lamp Count)
+ 0x09, 0x55, // Usage (Lamp Update Flags)
+ 0x15, 0x00, // Logical Minimum (0)
+ 0x25, 0x08, // Logical Maximum (8)
+ 0x75, 0x08, // Report Size (8)
+ 0x95, 0x02, // Report Count (2)
+ 0xb1, 0x02, // Feature (Data,Var,Abs)
+ 0x09, 0x21, // Usage (Lamp Id)
+ 0x15, 0x00, // Logical Minimum (0)
+ 0x27, 0xff, 0xff, 0x00, 0x00, // Logical Maximum (65535)
+ 0x75, 0x10, // Report Size (16)
+ 0x95, 0x08, // Report Count (8)
+ 0xb1, 0x02, // Feature (Data,Var,Abs)
+ 0x09, 0x51, // Usage (Red Update Channel)
+ 0x09, 0x52, // Usage (Green Update Channel)
+ 0x09, 0x53, // Usage (Blue Update Channel)
+ 0x09, 0x54, // Usage (Intensity Update Channel)
+ 0x09, 0x51, // Usage (Red Update Channel)
+ 0x09, 0x52, // Usage (Green Update Channel)
+ 0x09, 0x53, // Usage (Blue Update Channel)
+ 0x09, 0x54, // Usage (Intensity Update Channel)
+ 0x09, 0x51, // Usage (Red Update Channel)
+ 0x09, 0x52, // Usage (Green Update Channel)
+ 0x09, 0x53, // Usage (Blue Update Channel)
+ 0x09, 0x54, // Usage (Intensity Update Channel)
+ 0x09, 0x51, // Usage (Red Update Channel)
+ 0x09, 0x52, // Usage (Green Update Channel)
+ 0x09, 0x53, // Usage (Blue Update Channel)
+ 0x09, 0x54, // Usage (Intensity Update Channel)
+ 0x09, 0x51, // Usage (Red Update Channel)
+ 0x09, 0x52, // Usage (Green Update Channel)
+ 0x09, 0x53, // Usage (Blue Update Channel)
+ 0x09, 0x54, // Usage (Intensity Update Channel)
+ 0x09, 0x51, // Usage (Red Update Channel)
+ 0x09, 0x52, // Usage (Green Update Channel)
+ 0x09, 0x53, // Usage (Blue Update Channel)
+ 0x09, 0x54, // Usage (Intensity Update Channel)
+ 0x09, 0x51, // Usage (Red Update Channel)
+ 0x09, 0x52, // Usage (Green Update Channel)
+ 0x09, 0x53, // Usage (Blue Update Channel)
+ 0x09, 0x54, // Usage (Intensity Update Channel)
+ 0x09, 0x51, // Usage (Red Update Channel)
+ 0x09, 0x52, // Usage (Green Update Channel)
+ 0x09, 0x53, // Usage (Blue Update Channel)
+ 0x09, 0x54, // Usage (Intensity Update Channel)
+ 0x15, 0x00, // Logical Minimum (0)
+ 0x26, 0xff, 0x00, // Logical Maximum (255)
+ 0x75, 0x08, // Report Size (8)
+ 0x95, 0x20, // Report Count (32)
+ 0xb1, 0x02, // Feature (Data,Var,Abs)
+ 0xc0, // End Collection
+ 0x85, LAMP_RANGE_UPDATE_REPORT_ID, // Report ID (5)
+ 0x09, 0x60, // Usage (Lamp Range Update Report)
+ 0xa1, 0x02, // Collection (Logical)
+ 0x09, 0x55, // Usage (Lamp Update Flags)
+ 0x15, 0x00, // Logical Minimum (0)
+ 0x25, 0x08, // Logical Maximum (8)
+ 0x75, 0x08, // Report Size (8)
+ 0x95, 0x01, // Report Count (1)
+ 0xb1, 0x02, // Feature (Data,Var,Abs)
+ 0x09, 0x61, // Usage (Lamp Id Start)
+ 0x09, 0x62, // Usage (Lamp Id End)
+ 0x15, 0x00, // Logical Minimum (0)
+ 0x27, 0xff, 0xff, 0x00, 0x00, // Logical Maximum (65535)
+ 0x75, 0x10, // Report Size (16)
+ 0x95, 0x02, // Report Count (2)
+ 0xb1, 0x02, // Feature (Data,Var,Abs)
+ 0x09, 0x51, // Usage (Red Update Channel)
+ 0x09, 0x52, // Usage (Green Update Channel)
+ 0x09, 0x53, // Usage (Blue Update Channel)
+ 0x09, 0x54, // Usage (Intensity Update Channel)
+ 0x15, 0x00, // Logical Minimum (0)
+ 0x26, 0xff, 0x00, // Logical Maximum (255)
+ 0x75, 0x08, // Report Size (8)
+ 0x95, 0x04, // Report Count (4)
+ 0xb1, 0x02, // Feature (Data,Var,Abs)
+ 0xc0, // End Collection
+ 0x85, LAMP_ARRAY_CONTROL_REPORT_ID, // Report ID (6)
+ 0x09, 0x70, // Usage (Lamp Array Control Report)
+ 0xa1, 0x02, // Collection (Logical)
+ 0x09, 0x71, // Usage (Autonomous Mode)
+ 0x15, 0x00, // Logical Minimum (0)
+ 0x25, 0x01, // Logical Maximum (1)
+ 0x75, 0x08, // Report Size (8)
+ 0x95, 0x01, // Report Count (1)
+ 0xb1, 0x02, // Feature (Data,Var,Abs)
+ 0xc0, // End Collection
+ 0xc0 // End Collection
+};
+
+static int ll_parse(struct hid_device *hdev)
+{
+ return hid_parse_report(hdev, report_descriptor, sizeof(report_descriptor));
+}
+
+
+struct __packed lamp_array_attributes_report_t {
+ const uint8_t report_id;
+ uint16_t lamp_count;
+ uint32_t bounding_box_width_in_micrometers;
+ uint32_t bounding_box_height_in_micrometers;
+ uint32_t bounding_box_depth_in_micrometers;
+ uint32_t lamp_array_kind;
+ uint32_t min_update_interval_in_microseconds;
+};Limit length to 80 chars.
+
+static int handle_lamp_array_attributes_report(struct hid_device *hdev,
+ struct lamp_array_attributes_report_t *rep)
+{
+ struct driver_data_t *driver_data = hdev->driver_data;
+
+ rep->lamp_count = driver_data->lamp_count;
+ rep->bounding_box_width_in_micrometers = 368000;
+ rep->bounding_box_height_in_micrometers = 266000;
+ rep->bounding_box_depth_in_micrometers = 30000;
+ // LampArrayKindKeyboard, see "26.2.1 LampArrayKind Values" of "HID Usage Tables v1.5"
ok
+ rep->lamp_array_kind = 1;sizeof(*rep) ?
+ // Some guessed value for interval microseconds
+ rep->min_update_interval_in_microseconds = 500;
+
+ return sizeof(struct lamp_array_attributes_report_t);
ok
+}sizeof(*rep) ?
+
+
+struct __packed lamp_attributes_request_report_t {
+ const uint8_t report_id;
+ uint16_t lamp_id;
+};
+
+static int handle_lamp_attributes_request_report(struct hid_device *hdev,
+ struct lamp_attributes_request_report_t *rep)
+{
+ struct driver_data_t *driver_data = hdev->driver_data;
+
+ if (rep->lamp_id < driver_data->lamp_count)
+ driver_data->next_lamp_id = rep->lamp_id;
+ else
+ driver_data->next_lamp_id = 0;
+
+ return sizeof(struct lamp_attributes_request_report_t);
ok
+}uXX types.
+
+
+struct __packed lamp_attributes_response_report_t {
+ const uint8_t report_id;
+ uint16_t lamp_id;
+ uint32_t position_x_in_micrometers;
+ uint32_t position_y_in_micrometers;
+ uint32_t position_z_in_micrometers;
+ uint32_t update_latency_in_microseconds;
+ uint32_t lamp_purpose;
+ uint8_t red_level_count;
+ uint8_t green_level_count;
+ uint8_t blue_level_count;
+ uint8_t intensity_level_count;
+ uint8_t is_programmable;
+ uint8_t input_binding;
ok
+};uXX types.
+
+static int handle_lamp_attributes_response_report(struct hid_device *hdev,
+ struct lamp_attributes_response_report_t *rep)
+{
+ struct driver_data_t *driver_data = hdev->driver_data;
+ uint16_t lamp_id = driver_data->next_lamp_id;
+ const uint8_t *kbl_mapping;
+ const uint32_t *kbl_mapping_pos_x, *kbl_mapping_pos_y, *kbl_mapping_pos_z;
woopsy, will fix
+Extra space.
+ rep->lamp_id = lamp_id;
+ // Some guessed value for latency microseconds
+ rep->update_latency_in_microseconds = 100;
+ // LampPurposeControl, see "26.3.1 LampPurposes Flags" of "HID Usage Tables v1.5"
see above
Limit to 80 chars.
ok/see above
+ rep->lamp_purpose = 1;Use multiline /**/ comment formatting and limit to 80 chars.
+ rep->red_level_count = 0xff;
+ rep->green_level_count = 0xff;
+ rep->blue_level_count = 0xff;
+ rep->intensity_level_count = 0xff;
+ rep->is_programmable = 1;
+
+ if (driver_data->keyboard_type == WMI_AB_GET_DEVICE_STATUS_KEYBOARD_LAYOUT_ANSII) {
+ kbl_mapping = &sirius_16_ansii_kbl_mapping[0];
+ kbl_mapping_pos_x = &sirius_16_ansii_kbl_mapping_pos_x[0];
+ kbl_mapping_pos_y = &sirius_16_ansii_kbl_mapping_pos_y[0];
+ kbl_mapping_pos_z = &sirius_16_ansii_kbl_mapping_pos_z[0];
+ } else if (driver_data->keyboard_type == WMI_AB_GET_DEVICE_STATUS_KEYBOARD_LAYOUT_ISO) {
+ kbl_mapping = &sirius_16_iso_kbl_mapping[0];
+ kbl_mapping_pos_x = &sirius_16_iso_kbl_mapping_pos_x[0];
+ kbl_mapping_pos_y = &sirius_16_iso_kbl_mapping_pos_y[0];
+ kbl_mapping_pos_z = &sirius_16_iso_kbl_mapping_pos_z[0];
+ } else
+ return -EINVAL;
+
+ if (kbl_mapping[lamp_id] <= 0xe8)
+ rep->input_binding = kbl_mapping[lamp_id];
+ else
+ // Everything bigger is reserved/undefined, see "10 Keyboard/Keypad Page (0x07)" of
+ // "HID Usage Tables v1.5" and should return 0, see "26.8.3 Lamp Attributes" of the
+ // same document.
ok
+ rep->input_binding = 0;sizeof(*rep) ?
+ rep->position_x_in_micrometers = kbl_mapping_pos_x[lamp_id];
+ rep->position_y_in_micrometers = kbl_mapping_pos_y[lamp_id];
+ rep->position_z_in_micrometers = kbl_mapping_pos_z[lamp_id];
+
+ driver_data->next_lamp_id = (driver_data->next_lamp_id + 1) % driver_data->lamp_count;
+
+ return sizeof(struct lamp_attributes_response_report_t);
ok
+}uXX types.
+
+
+#define LAMP_UPDATE_FLAGS_LAMP_UPDATE_COMPLETE BIT(0)
+
+struct __packed lamp_multi_update_report_t {
+ const uint8_t report_id;
+ uint8_t lamp_count;
+ uint8_t lamp_update_flags;
+ uint16_t lamp_id[8];
+ struct {
+ uint8_t red;
+ uint8_t green;
+ uint8_t blue;
+ uint8_t intensity;
+ } update_channels[8];
ok
+};Reverse xmas-tree order (to the extent viable).
+
+static int handle_lamp_multi_update_report(struct hid_device *hdev,
+ struct lamp_multi_update_report_t *rep)
+{
+ struct driver_data_t *driver_data = hdev->driver_data;
+ struct wmi_device *wdev = to_wmi_device(hdev->dev.parent);
+ int ret;
+ uint8_t lamp_count = 0, key_id, key_id_j;
+ union tuxedo_nb04_wmi_496_b_in_80_b_out_input *next =
+ &driver_data->next_kbl_set_multiple_keys_input;
+ union tuxedo_nb04_wmi_496_b_in_80_b_out_output output;
ok
+sizeof(*rep)?
+ // Catching missformated lamp_multi_update_report and fail silently according to
+ // "HID Usage Tables v1.5"
+ for (int i = 0; i < rep->lamp_count; ++i) {
+ if (driver_data->keyboard_type == WMI_AB_GET_DEVICE_STATUS_KEYBOARD_LAYOUT_ANSII)
+ lamp_count = sizeof(sirius_16_ansii_kbl_mapping);
+ else if (driver_data->keyboard_type == WMI_AB_GET_DEVICE_STATUS_KEYBOARD_LAYOUT_ISO)
+ lamp_count = sizeof(sirius_16_ansii_kbl_mapping);
+
+ if (rep->lamp_id[i] > lamp_count) {
+ pr_debug("Out of bounds lamp_id in lamp_multi_update_report. Skippng whole report!\n");
+ return sizeof(struct lamp_multi_update_report_t);
Not sure if this helps with readability, at least for me i think that would take more time to parse the reversed condition.
+ }Reverse condition and use continue to lower the indentation level of the
+
+ for (int j = i + 1; j < rep->lamp_count; ++j) {
+ if (rep->lamp_id[i] == rep->lamp_id[j]) {
+ pr_debug("Duplicate lamp_id in lamp_multi_update_report. Skippng whole report!\n");
+ return sizeof(struct lamp_multi_update_report_t);
+ }
+ }
+ }
+
+ for (int i = 0; i < rep->lamp_count; ++i) {
+ if (driver_data->keyboard_type == WMI_AB_GET_DEVICE_STATUS_KEYBOARD_LAYOUT_ANSII)
+ key_id = sirius_16_ansii_kbl_mapping[rep->lamp_id[i]];
+ else if (driver_data->keyboard_type == WMI_AB_GET_DEVICE_STATUS_KEYBOARD_LAYOUT_ISO)
+ key_id = sirius_16_iso_kbl_mapping[rep->lamp_id[i]];
+
+ for (int j = 0; j < WMI_AB_KBL_SET_MULTIPLE_KEYS_LIGHTING_SETTINGS_COUNT_MAX; ++j) {
+ key_id_j = next->kbl_set_multiple_keys_input.lighting_settings[j].key_id;
+ if (key_id_j == 0x00 || key_id_j == key_id) {
main loop body.
+ if (key_id_j == 0x00)No, reverse the logic in the condition and return -EINVAL explicitly.
+ next->kbl_set_multiple_keys_input.lighting_setting_count =
+ j + 1;
+ next->kbl_set_multiple_keys_input.lighting_settings[j].key_id =
+ key_id;
+ // While this driver respects
+ // intensity_update_channel according to "HID
+ // Usage Tables v1.5" also on RGB leds, the
+ // Microsoft MacroPad reference implementation
+ // (https://github.com/microsoft/RP2040MacropadHidSample
+ // 1d6c3ad) does not and ignores it. If it turns
+ // out that Windows writes intensity = 0 for RGB
+ // leds instead of intensity = 255, this driver
+ // should also irgnore the
+ // intensity_update_channel.
+ next->kbl_set_multiple_keys_input.lighting_settings[j].red =
+ rep->update_channels[i].red
+ * rep->update_channels[i].intensity / 0xff;
+ next->kbl_set_multiple_keys_input.lighting_settings[j].green =
+ rep->update_channels[i].green
+ * rep->update_channels[i].intensity / 0xff;
+ next->kbl_set_multiple_keys_input.lighting_settings[j].blue =
+ rep->update_channels[i].blue
+ * rep->update_channels[i].intensity / 0xff;
+
+ break;
+ }
+ }
+ }
+
+ if (rep->lamp_update_flags & LAMP_UPDATE_FLAGS_LAMP_UPDATE_COMPLETE) {
+ ret = tuxedo_nb04_wmi_496_b_in_80_b_out(wdev, WMI_AB_KBL_SET_MULTIPLE_KEYS, next,
+ &output);
+ memset(next, 0, sizeof(union tuxedo_nb04_wmi_496_b_in_80_b_out_input));
+ if (ret)
+ return ret;
+ }
+
+ return sizeof(struct lamp_multi_update_report_t);
+}
+
+
+struct __packed lamp_range_update_report_t {
+ const uint8_t report_id;
+ uint8_t lamp_update_flags;
+ uint16_t lamp_id_start;
+ uint16_t lamp_id_end;
+ uint8_t red_update_channel;
+ uint8_t green_update_channel;
+ uint8_t blue_update_channel;
+ uint8_t intensity_update_channel;
+};
+
+static int handle_lamp_range_update_report(struct hid_device *hdev,
+ struct lamp_range_update_report_t *report)
+{
+ struct driver_data_t *driver_data = hdev->driver_data;
+ int ret;
+ uint8_t lamp_count;
+ struct lamp_multi_update_report_t lamp_multi_update_report = {
+ .report_id = LAMP_MULTI_UPDATE_REPORT_ID
+ };
+
+ // Catching missformated lamp_range_update_report and fail silently according to
+ // "HID Usage Tables v1.5"
+ if (report->lamp_id_start > report->lamp_id_end) {
+ pr_debug("lamp_id_start > lamp_id_end in lamp_range_update_report. Skippng whole report!\n");
+ return sizeof(struct lamp_range_update_report_t);
+ }
+
+ if (driver_data->keyboard_type == WMI_AB_GET_DEVICE_STATUS_KEYBOARD_LAYOUT_ANSII)
+ lamp_count = sizeof(sirius_16_ansii_kbl_mapping);
+ else if (driver_data->keyboard_type == WMI_AB_GET_DEVICE_STATUS_KEYBOARD_LAYOUT_ISO)
+ lamp_count = sizeof(sirius_16_ansii_kbl_mapping);
+
+ if (report->lamp_id_end > lamp_count - 1) {
+ pr_debug("Out of bounds lamp_id_* in lamp_range_update_report. Skippng whole report!\n");
+ return sizeof(struct lamp_range_update_report_t);
+ }
+
+ // Break handle_lamp_range_update_report call down to multiple
+ // handle_lamp_multi_update_report calls to easily ensure that mixing
+ // handle_lamp_range_update_report and handle_lamp_multi_update_report
+ // does not break things.
+ for (int i = report->lamp_id_start; i < report->lamp_id_end + 1; i = i + 8) {
+ lamp_multi_update_report.lamp_count = MIN(report->lamp_id_end + 1 - i, 8);
+ if (i + lamp_multi_update_report.lamp_count == report->lamp_id_end + 1)
+ lamp_multi_update_report.lamp_update_flags |=
+ LAMP_UPDATE_FLAGS_LAMP_UPDATE_COMPLETE;
+
+ for (int j = 0; j < lamp_multi_update_report.lamp_count; ++j) {
+ lamp_multi_update_report.lamp_id[j] = i + j;
+ lamp_multi_update_report.update_channels[j].red =
+ report->red_update_channel;
+ lamp_multi_update_report.update_channels[j].green =
+ report->green_update_channel;
+ lamp_multi_update_report.update_channels[j].blue =
+ report->blue_update_channel;
+ lamp_multi_update_report.update_channels[j].intensity =
+ report->intensity_update_channel;
+ }
+
+ ret = handle_lamp_multi_update_report(hdev, &lamp_multi_update_report);
+ if (ret < 0)
+ return ret;
+ else if (ret != sizeof(struct lamp_multi_update_report_t))
+ return -EIO;
+ }
+
+ return sizeof(struct lamp_range_update_report_t);
+}
+
+
+struct __packed lamp_array_control_report_t {
+ const uint8_t report_id;
+ uint8_t autonomous_mode;
+};
+
+static int handle_lamp_array_control_report(struct hid_device __always_unused *hdev,
+ struct lamp_array_control_report_t __always_unused *rep)
+{
+ // The keyboard firmware doesn't have any built in effects or controls
+ // so this is a NOOP.
+ // According to the HID Documentation (HID Usage Tables v1.5) this
+ // function is optional and can be removed from the HID Report
+ // Descriptor, but it should first be confirmed that userspace respects
+ // this possibility too. The Microsoft MacroPad reference implementation
+ // (https://github.com/microsoft/RP2040MacropadHidSample 1d6c3ad)
+ // already deviates from the spec at another point, see
+ // handle_lamp_*_update_report.
+
+ return sizeof(struct lamp_array_control_report_t);
+}
+
+
+static int ll_raw_request(struct hid_device *hdev, unsigned char reportnum, __u8 *buf, size_t len,
+ unsigned char rtype, int reqtype)
+{
+ int ret;
+
+ ret = -EINVAL;
+ if (rtype == HID_FEATURE_REPORT) {
It lets you lower the indentation level of the normal path.
Wouldn't that mean more intendation? One for the switch case and another one for the now seperate len check?
+ if (reqtype == HID_REQ_GET_REPORT) {This is very messy, and should IMHO use switch&case and directly return
+ if (reportnum == LAMP_ARRAY_ATTRIBUTES_REPORT_ID
+ && len == sizeof(struct lamp_array_attributes_report_t))
+ ret = handle_lamp_array_attributes_report(
+ hdev, (struct lamp_array_attributes_report_t *)buf);
+ else if (reportnum == LAMP_ATTRIBUTES_RESPONSE_REPORT_ID
+ && len == sizeof(struct lamp_attributes_response_report_t))
+ ret = handle_lamp_attributes_response_report(
+ hdev, (struct lamp_attributes_response_report_t *)buf);
+ } else if (reqtype == HID_REQ_SET_REPORT) {
+ if (reportnum == LAMP_ATTRIBUTES_REQUEST_REPORT_ID
+ && len == sizeof(struct lamp_attributes_request_report_t))
+ ret = handle_lamp_attributes_request_report(
+ hdev, (struct lamp_attributes_request_report_t *)buf);
+ else if (reportnum == LAMP_MULTI_UPDATE_REPORT_ID
+ && len == sizeof(struct lamp_multi_update_report_t))
+ ret = handle_lamp_multi_update_report(
+ hdev, (struct lamp_multi_update_report_t *)buf);
+ else if (reportnum == LAMP_RANGE_UPDATE_REPORT_ID
+ && len == sizeof(struct lamp_range_update_report_t))
+ ret = handle_lamp_range_update_report(
+ hdev, (struct lamp_range_update_report_t *)buf);
+ else if (reportnum == LAMP_ARRAY_CONTROL_REPORT_ID
+ && len == sizeof(struct lamp_array_control_report_t))
+ ret = handle_lamp_array_control_report(
+ hdev, (struct lamp_array_control_report_t *)buf);
+ }
-EINVAL on every len check inside the case blocks.
ok
+ }The usual custom is to put <> around email addresses.
+
+ return ret;
+}
+
+static const struct hid_ll_driver ll_driver = {
+ .start = &ll_start,
+ .stop = &ll_stop,
+ .open = &ll_open,
+ .close = &ll_close,
+ .parse = &ll_parse,
+ .raw_request = &ll_raw_request,
+};
+
+int tuxedo_nb04_virtual_lamp_array_add_device(struct wmi_device *wdev, struct hid_device **hdev_out)
+{
+ struct hid_device *hdev;
+ int ret;
+
+ pr_debug("Adding TUXEDO NB04 Virtual LampArray device.\n");
+
+ hdev = hid_allocate_device();
+ if (IS_ERR(hdev))
+ return PTR_ERR(hdev);
+ *hdev_out = hdev;
+
+ strscpy(hdev->name, "TUXEDO NB04 RGB Lighting", sizeof(hdev->name));
+
+ hdev->ll_driver = &ll_driver;
+ hdev->bus = BUS_VIRTUAL;
+ hdev->vendor = 0x21ba;
+ hdev->product = 0x0400;
+ hdev->dev.parent = &wdev->dev;
+
+ ret = hid_add_device(hdev);
+ if (ret)
+ hid_destroy_device(hdev);
+ return ret;
+}
+EXPORT_SYMBOL(tuxedo_nb04_virtual_lamp_array_add_device);
diff --git a/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_ab_virtual_lamp_array.h b/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_ab_virtual_lamp_array.h
new file mode 100644
index 0000000000000..fdc2a01d95c24
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_ab_virtual_lamp_array.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * This code gives the built in RGB lighting of the TUXEDO NB04 devices a
+ * standardised interface, namely HID LampArray.
+ *
+ * Copyright (C) 2024 Werner Sembach wse@xxxxxxxxxxxxxxxxxxx
+ */
+
+#ifndef TUXEDO_NB04_WMI_AB_VIRTUAL_LAMP_ARRAY_H
+#define TUXEDO_NB04_WMI_AB_VIRTUAL_LAMP_ARRAY_H
+
+#include <linux/wmi.h>
+#include <linux/hid.h>
+
+int tuxedo_nb04_virtual_lamp_array_add_device(struct wmi_device *wdev,
+ struct hid_device **hdev_out);
+
+#endif
diff --git a/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_util.c b/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_util.c
new file mode 100644
index 0000000000000..a61b59d225f9f
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_util.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This code gives functions to avoid code duplication while interacting with
+ * the TUXEDO NB04 wmi interfaces.
+ *
+ * Copyright (C) 2024 Werner Sembach wse@xxxxxxxxxxxxxxxxxxx
also linux/wmi.h? or it it enough that that is in the directly associated .h file?
+ */Add all includes for the stuff you use below.
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
ok
+#include "tuxedo_nb04_wmi_ab_init.h"You should use dev_err() instead of pr_err().
+
+#include "tuxedo_nb04_wmi_util.h"
+
+static int __wmi_method_acpi_object_out(struct wmi_device *wdev, uint32_t wmi_method_id,
+ uint8_t *in, acpi_size in_len, union acpi_object **out)
+{
+ struct acpi_buffer acpi_buffer_in = { in_len, in };
+ struct acpi_buffer acpi_buffer_out = { ACPI_ALLOCATE_BUFFER, NULL };
+
+ pr_debug("Evaluate WMI method: %u in:\n", wmi_method_id);
+ print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, in, in_len);
+
+ acpi_status status = wmidev_evaluate_method(wdev, 0, wmi_method_id, &acpi_buffer_in,
+ &acpi_buffer_out);
+ if (ACPI_FAILURE(status)) {
+ pr_err("Failed to evaluate WMI method.\n");
ok
+ return -EIO;Reverse xmas tree order.
+ }
+ if (!acpi_buffer_out.pointer) {
+ pr_err("Unexpected empty out buffer.\n");
+ return -ENODATA;
+ }
+
+ *out = acpi_buffer_out.pointer;
+
+ return 0;
+}
+
+static int __wmi_method_buffer_out(struct wmi_device *wdev, uint32_t wmi_method_id, uint8_t *in,
+ acpi_size in_len, uint8_t *out, acpi_size out_len)
+{
+ int ret;
+ union acpi_object *acpi_object_out = NULL;
ok
+Duplicated error paths should use goto to handle rollback.
+ ret = __wmi_method_acpi_object_out(wdev, wmi_method_id, in, in_len, &acpi_object_out);
+ if (ret)
+ return ret;
+
+ if (acpi_object_out->type != ACPI_TYPE_BUFFER) {
+ pr_err("Unexpected out buffer type. Expected: %u Got: %u\n", ACPI_TYPE_BUFFER,
+ acpi_object_out->type);
+ kfree(acpi_object_out);
+ return -EIO;
+ }
+ if (acpi_object_out->buffer.length < out_len) {
+ pr_err("Unexpected out buffer length.\n");
+ kfree(acpi_object_out);
+ return -EIO;
These names match directly internal documentation, if it's no major problem i would like to keep the names for future reference.
+ }All these are quite long, I'd consider ways to make them shorter such as:
+
+ memcpy(out, acpi_object_out->buffer.pointer, out_len);
+ kfree(acpi_object_out);
+
+ return ret;
+}
+
+int tuxedo_nb04_wmi_8_b_in_80_b_out(struct wmi_device *wdev,
+ enum tuxedo_nb04_wmi_8_b_in_80_b_out_methods method,
+ union tuxedo_nb04_wmi_8_b_in_80_b_out_input *input,
+ union tuxedo_nb04_wmi_8_b_in_80_b_out_output *output)
+{
+ return __wmi_method_buffer_out(wdev, method, input->raw, 8, output->raw, 80);
+}
+
+int tuxedo_nb04_wmi_496_b_in_80_b_out(struct wmi_device *wdev,
+ enum tuxedo_nb04_wmi_496_b_in_80_b_out_methods method,
+ union tuxedo_nb04_wmi_496_b_in_80_b_out_input *input,
+ union tuxedo_nb04_wmi_496_b_in_80_b_out_output *output)
+{
+ return __wmi_method_buffer_out(wdev, method, input->raw, 496, output->raw, 80);
+}
diff --git a/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_util.h b/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_util.h
new file mode 100644
index 0000000000000..2765cbe9fcfef
--- /dev/null
+++ b/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_util.h
@@ -0,0 +1,112 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * This code gives functions to avoid code duplication while interacting with
+ * the TUXEDO NB04 wmi interfaces.
+ *
+ * Copyright (C) 2024 Werner Sembach wse@xxxxxxxxxxxxxxxxxxx
+ */
+
+#ifndef TUXEDO_NB04_WMI_UTIL_H
+#define TUXEDO_NB04_WMI_UTIL_H
+
+#include <linux/wmi.h>
+
+#define WMI_AB_GET_DEVICE_STATUS_DEVICE_ID_TOUCHPAD 1
+#define WMI_AB_GET_DEVICE_STATUS_DEVICE_ID_KEYBOARD 2
+#define WMI_AB_GET_DEVICE_STATUS_DEVICE_ID_APP_PAGES 3
+
+#define WMI_AB_GET_DEVICE_STATUS_KBL_TYPE_NONE 0
+#define WMI_AB_GET_DEVICE_STATUS_KBL_TYPE_PER_KEY 1
+#define WMI_AB_GET_DEVICE_STATUS_KBL_TYPE_FOUR_ZONE 2
+#define WMI_AB_GET_DEVICE_STATUS_KBL_TYPE_WHITE_ONLY 3
+
+#define WMI_AB_GET_DEVICE_STATUS_KEYBOARD_LAYOUT_ANSII 0
+#define WMI_AB_GET_DEVICE_STATUS_KEYBOARD_LAYOUT_ISO 1
+
+#define WMI_AB_GET_DEVICE_STATUS_COLOR_ID_RED 1
+#define WMI_AB_GET_DEVICE_STATUS_COLOR_ID_GREEN 2
+#define WMI_AB_GET_DEVICE_STATUS_COLOR_ID_YELLOW 3
+#define WMI_AB_GET_DEVICE_STATUS_COLOR_ID_BLUE 4
+#define WMI_AB_GET_DEVICE_STATUS_COLOR_ID_PURPLE 5
+#define WMI_AB_GET_DEVICE_STATUS_COLOR_ID_INDIGO 6
+#define WMI_AB_GET_DEVICE_STATUS_COLOR_ID_WHITE 7
+
+#define WMI_AB_GET_DEVICE_STATUS_APP_PAGES_DASHBOARD BIT(0)
+#define WMI_AB_GET_DEVICE_STATUS_APP_PAGES_SYSTEMINFOS BIT(1)
+#define WMI_AB_GET_DEVICE_STATUS_APP_PAGES_KBL BIT(2)
+#define WMI_AB_GET_DEVICE_STATUS_APP_PAGES_HOTKEYS BIT(3)
DEVICE -> DEV
COLOR_ID -> COLOR
STATUS -> STS ?
KEYBOARD_LAYOUT -> KEY_LAYOUT or KEYBOARD -> KEYB (in general)
ok
+Extra newline.
+
ok
+union tuxedo_nb04_wmi_8_b_in_80_b_out_input {uXX for types.
+ uint8_t raw[8];
if it's not harmfull i would like to keep it so I don't forget it when the reserved byts might be used sometime int the future
+ struct __packed {Unnecessary packed.
to match the name scheme of the other structs
+ uint8_t device_type;Why isn't this just reserved[7] ?
+ uint8_t reserved_0[7];
see above
+ } get_device_status_input;This too looks naturally aligned so packet is unnecessary.
+};
+
+union tuxedo_nb04_wmi_8_b_in_80_b_out_output {
+ uint8_t raw[80];
+ struct __packed {
see above
+ uint16_t return_status;reserved[15] ?
+ uint8_t device_enabled;
+ uint8_t kbl_type;
+ uint8_t kbl_side_bar_supported;
+ uint8_t keyboard_physical_layout;
+ uint8_t app_pages;
+ uint8_t per_key_kbl_default_color;
+ uint8_t four_zone_kbl_default_color_1;
+ uint8_t four_zone_kbl_default_color_2;
+ uint8_t four_zone_kbl_default_color_3;
+ uint8_t four_zone_kbl_default_color_4;
+ uint8_t light_bar_kbl_default_color;
+ uint8_t reserved_0[1];
+ uint16_t dedicated_gpu_id;
+ uint8_t reserved_1[64];
+ } get_device_status_output;
+};
+
+enum tuxedo_nb04_wmi_8_b_in_80_b_out_methods {
+ WMI_AB_GET_DEVICE_STATUS = 2,
+};
+
+
+#define WMI_AB_KBL_SET_MULTIPLE_KEYS_LIGHTING_SETTINGS_COUNT_MAX 120
+
+union tuxedo_nb04_wmi_496_b_in_80_b_out_input {
+ uint8_t raw[496];
+ struct __packed {
+ uint8_t reserved_0[15];
see above
+ uint8_t lighting_setting_count;reserved[79] ?
+ struct {
+ uint8_t key_id;
+ uint8_t red;
+ uint8_t green;
+ uint8_t blue;
+ } lighting_settings[WMI_AB_KBL_SET_MULTIPLE_KEYS_LIGHTING_SETTINGS_COUNT_MAX];
+ } kbl_set_multiple_keys_input;
+};
+
+union tuxedo_nb04_wmi_496_b_in_80_b_out_output {
+ uint8_t raw[80];
+ struct __packed {
+ uint8_t return_value;
+ uint8_t reserved_0[79];
+ } kbl_set_multiple_keys_output;There are number of similar cases beyond those I've marked. Please go
+};
+
+enum tuxedo_nb04_wmi_496_b_in_80_b_out_methods {
+ WMI_AB_KBL_SET_MULTIPLE_KEYS = 6,
+};
+
+
+int tuxedo_nb04_wmi_8_b_in_80_b_out(struct wmi_device *wdev,
+ enum tuxedo_nb04_wmi_8_b_in_80_b_out_methods method,
+ union tuxedo_nb04_wmi_8_b_in_80_b_out_input *input,
+ union tuxedo_nb04_wmi_8_b_in_80_b_out_output *output);
+int tuxedo_nb04_wmi_496_b_in_80_b_out(struct wmi_device *wdev,
+ enum tuxedo_nb04_wmi_496_b_in_80_b_out_methods method,
+ union tuxedo_nb04_wmi_496_b_in_80_b_out_input *input,
+ union tuxedo_nb04_wmi_496_b_in_80_b_out_output *output);
+
+#endif
through all the cases, not just the ones I marked for you.
My general feel is that this driver is quite heavy to read which is likely
because of what feels excessively long naming used. I'm not convinved
being _that verbose_ adds enough value.
While i currently only implement 2 functions of that wmi api there are more and this interface is written with the intend to be easily expandable to the other functions in the future.
E.g., right above you have tuxedo_nb04_wmi_8_b_in_80_b_out_methods which
mostly contains what looks prefix and some sizes. What is the value of
having those sizes in the name? It would be much more useful to name
things by functionality rather than sizes.