Hi Armin,
Am 27.09.24 um 19:15 schrieb Armin Wolf:
[...]
If so, please mark your patches as "RFC" if they are not considered as
a potentially "final" release.
Otherwise they might get accepted with the debug printing still inside.
Talking about noob mistakes ... I'm sorry, will do this with the next patch.
+
+ mutex_lock(&driver_data->wmi_access_mutex);
Does the underlying ACPI method really require external locking? If
not, then please remove this mutex.
Taken from the out of tree driver written by Christoffer, I will ask
him about this.
+ acpi_status status = wmidev_evaluate_method(wdev, 0,
wmi_method_id, &acpi_buffer_in,
+ &acpi_buffer_out);
+ mutex_unlock(&driver_data->wmi_access_mutex);
+ if (ACPI_FAILURE(status)) {
+ pr_err("Failed to evaluate WMI method.\n");
+ return -EIO;
+ }
+ if (!acpi_buffer_out.pointer) {
+ pr_err("Unexpected empty out buffer.\n");
+ return -ENODATA;
+ }
I believe that printing error messages should be done by the callers
of this method.
+
+ *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)
Please use size_t instead of acpi_size.
+{
+ int ret;
+ union acpi_object *acpi_object_out = NULL;
union acpi_object *obj;
int ret;
ack ack ack
+
+ 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) {
The Windows ACPI-WMI mappers accepts oversized buffers and ignores
any additional data,
so please change this code to also accept oversized buffers.
Only for input or also for output?
Only forbuffers coming from the ACPI firmware.
ack
+ pr_err("Unexpected out buffer length.\n");
+ kfree(acpi_object_out);
+ return -EIO;
+ }
+
+ 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);
+}
Those two functions seem useless to me, please use
wmi_method_buffer_out() directly by passing
a pointer to the underlying struct as data and the output of
sizeof() as length.
They are thought of bringing some type safety into the mix so that
for any method id the input/output size is correct.
I do not think that this brings any real benefits when it comes to
type safety. Using predefined structs and sizeof()
already takes care that the buffer size is correct, and choosing the
correct method id already needs to be done by
the driver itself.
ack
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)
+
+
+union tuxedo_nb04_wmi_8_b_in_80_b_out_input {
+ uint8_t raw[8];
+ struct __packed {
+ uint8_t device_type;
+ uint8_t reserved_0[7];
+ } get_device_status_input;
+};
+
+union tuxedo_nb04_wmi_8_b_in_80_b_out_output {
+ uint8_t raw[80];
+ struct __packed {
+ uint16_t return_status;
+ 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];
+ uint8_t lighting_setting_count;
+ 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;
+};
+
+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
