Re: [RFC PATCH v4 1/1] platform/x86/tuxedo: Add virtual LampArray for TUXEDO NB04 devices

Werner Sembach
2024-10-04T14:30:31Z

Hi Ilpo,

Am 03.10.24 um 12:54 schrieb Ilpo Järvinen:
> On Wed, 2 Oct 2024, Werner Sembach wrote:
>> Am 02.10.24 um 11:52 schrieb Ilpo Järvinen:
>>> On Tue, 1 Oct 2024, Werner Sembach wrote:
>>>
>>>> The TUXEDO Sirius 16 Gen1 and TUXEDO Sirius 16 Gen2 devices have a per-key
>>>> 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@tuxedo.de>
>>>> Signed-off-by: Christoffer Sandberg <cs@tuxedo.de>
>>>> Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
>>>> Link:
>>>> https://lore.kernel.org/all/1fb08a74-62c7-4d0c-ba5d-648e23082dcb@tuxedocomputers.com/
>>>> ---
> I came across few new things that I didn't notice previously while
> writing this reply. I tried to cut the reply size down so hopefully they
> don't get lost as easily.

Thanks for the additional feedback.

>
>>>> + 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, 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
>>> Why are these aligned in the odd way?
>> to see where the numpad begin to have some kind of orientation
>>
>> 2 rows here are one physical row with the gap in front of the numpad
> Okay, thanks.
>
>>>> +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"
>>> Limit length to 80 chars.
>> is this just for comments or also code?
>>
>> because checkpatch explicitly allows 100 chars
> For comments. For code you can use longer line lengths where they make
> sense (doesn't mean that all lines should be tempting that limit which
> is usually an indication that code should be structured differently).
Ack
>
>>>> + }
>>>> +
>>>> + for (int j = i + 1; j < rep->lamp_count; ++j) {
> The convention these days is to use unsigned int for loop variables that
> are never supposed to be negative.
Ack
>
>>>> + if (rep->lamp_id[i] == rep->lamp_id[j]) {
>>>> + pr_debug("Duplicate lamp_id in
>>>> lamp_multi_update_report. Skippng whole report!\n");
> Skipping
>
>>>> + 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) {
>>> Reverse condition and use continue to lower the indentation level of the
>>> main loop body.
>> Not sure if this helps with readability, at least for me i think that would
>> take more time to parse the reversed condition.
> ?
>
> if (key_id_j != 0x00 && key_id_j != key_id)
> continue;
>
> The problem is not the condition itself but the indentation level in what
> follows. Lowering that is going to be overall beneficial.
>
>>>> + if (key_id_j == 0x00)
>>>> +
>>>> next->kbl_set_multiple_keys_input.lighting_setting_count =
>>>> + j + 1;
> But now that I think of it more, you could also do:
> if (key_id_j == 0x00)
> next->... = j+1;
> else if (key_id_j != key_id)
> continue;
Will look into it.
>
>>>> +
>>>> next->kbl_set_multiple_keys_input.lighting_settings[j].key_id =
>>>> + key_id;
> You could also create local variable for
> next->kbl_set_multiple_keys_input.lighting_settings[j]
> to make this loop body more readable.
>
> Similar local vars might help elsewhere in your code too if you need to do
> repeated accesses deep into the same structure.
Ack
>
>>>> + // 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
> ignore
Ack
>
>>>> + // 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 = {
> No idea why chose to you make the local variable this long as you seem to
> be fine using just "report" for naming the function parameter.
Because i didn't thought to much about this variable name.
>
> How about taking e.g., the first chars from the words of the time, i.e.,
> lmur or some similar convention for local names? The type is there close
> by for the code reader if one needs to know what the chars mean.
tbh I realy don't like acronym variable names, will see if i find a better short
name
>
>>>> + .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);
> Please use min() or min_t() instead of MIN().
ack
>
>>>> + 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;
> Shorter local var name would help here to stay on a single line. If that's
> not enough, local var for lmur->update_channels[j] would help further.
will look into it
>
>>>> + }
>>>> +
>>>> + 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) {
>>> No, reverse the logic in the condition and return -EINVAL explicitly.
>>> It lets you lower the indentation level of the normal path.
>> I don't quite get what you mean, i have to check 3 levels:
>>
>> Feature Report?
>>
>> Get or Set Report?
>>
>> Report ID?
>>
>> I don't see how i can do this without code duplication or 3 levels of
>> indendation?
> if (rtype != HID_FEATURE_REPORT)
> return -EINVAL;
Sorry was a slowpoke on this one
>
> This already brings you down one indentation level and simplifies things
> as you don't need to do ret = -EINVAL + return at the end.
>
>>>> + if (reqtype == HID_REQ_GET_REPORT) {
>>>> + 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);
>>>> + }
>>> This is very messy, and should IMHO use switch&case and directly return
>>> -EINVAL on every len check inside the case blocks.
>> Wouldn't that mean more intendation? One for the switch case and another one
>> for the now seperate len check?
> No it doesn't add indentation level compared with yours and this is also
> way easier to read:
>
> switch (reqtype) {
> case HID_REQ_GET_REPORT:
> switch (reportnum) {
> case LAMP_ATTRIBUTES_REQUEST_REPORT_ID:
> if (len != sizeof(struct lamp_array_attributes_report_t))
> return -EINVAL;
>
> return handle_lamp_array_attributes_report(hdev, (struct lamp_array_attributes_report_t *)buf);
> case ...:
> if (len != ...)
> return -EINVAL;
>
> return ...;
> default:
> return -EINVAL;
> }
>
> case HID_REQ_SET_REPORT:
> switch (reportnum) {
> case:
> ...
> default:
> return -EINVAL;
> }
> default:
> return -EINVAL;
> }
> }
>
> Compiler might be stupid enough to require a few additional breaks that
> will never be reachable + return to the end of function so make those
> tweaks as needed.
Will look into it
>
> I'm sorry if I copy-pasted something from a wrong place in above but I
> believe you get the point. The error handling is now clearly visible
> instead of being build inside a complex if condition and returns
> immediately to clearly show it's really doing error handling.
>
>>>> +++ 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@tuxedocomputers.com
>>>> + */
>>>> +
>>>> +#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@tuxedocomputers.com
>>> The usual custom is to put <> around email addresses.
>> ok
>>>> + */
>>>> +
>>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>> Add all includes for the stuff you use below.
>> also linux/wmi.h? or it it enough that that is in the directly associated .h
>> file?
> I'd prefer to see tham all here even if some "xx.h" does need them itself.
> Only if some <yy.h> includes <zz.h> and that dependency is something that
> is pretty much cast into stone, then including just yy.h is enough.
>
> Sadly the include-what-you-use clang tool is not there yet for the kernel
> so it's all manual process currently depending on reviewers paying
> attention to what headers are missing and which are extra.
ack
>
>>>> +#include "tuxedo_nb04_wmi_ab_init.h"
>>>> +
>>>> +#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");
>>> You should use dev_err() instead of pr_err().
>> ok
>>>> + return -EIO;
>>>> + }
>>>> + 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;
>>> Reverse xmas tree order.
>> ok
>>>> +
>>>> + 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;
>>> Duplicated error paths should use goto to handle rollback.
>> ok
>>>> + }
>>>> +
>>>> + 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@tuxedocomputers.com
>>>> + */
>>>> +
>>>> +#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)
>>> All these are quite long, I'd consider ways to make them shorter such as:
>>>
>>> DEVICE -> DEV
>>> COLOR_ID -> COLOR
>>> STATUS -> STS ?
>>> KEYBOARD_LAYOUT -> KEY_LAYOUT or KEYBOARD -> KEYB (in general)
>> These names match directly internal documentation, if it's no major problem i
>> would like to keep the names for future reference.
> I kind of assumed it might be the case, won't force you to do the
> shortening but please realize it will make the code look more messy
> because of the long line lengths, very long name are harder to read.
>
>>>> + struct __packed {
>>> Unnecessary packed.
>> 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
> Packed has code generation impact so it is harmful at times.
>
>>>> + uint8_t device_type;
>>>> + uint8_t reserved_0[7];
>>> Why isn't this just reserved[7] ?
>> to match the name scheme of the other structs
> Does it have to match? These are supposed to be dummy names that just
> tell don't use/touch this field?
yeah you might be right and that was just ocd talking or something xD
>
>>>> + } get_device_status_input;
>>>> +};
>>>> +
>>>> +union tuxedo_nb04_wmi_8_b_in_80_b_out_output {
>>>> + uint8_t raw[80];
>>>> + struct __packed {
>>> This too looks naturally aligned so packet is unnecessary.
>> see above
>>>> + 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];
>>> reserved[15] ?
>> see above
>>>> + 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];
>>> reserved[79] ?
>> see above
>>>> + } 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
>>> There are number of similar cases beyond those I've marked. Please go
>>> 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.
>> When it comes down to the wmi structs I named stuff after the internal
>> documentation for easy future reference. Everywhere else I shortened the names
>> for everything that is static, but i can do a second pass.
>>> 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.
>>>
>> 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.
>>
>> That why i choose the rather generic names of just the input and output length
>> because there is no semantic connection between the wmi methods in
>> tuxedo_nb04_wmi_8_b_in_80_b_out and tuxedo_nb04_wmi_496_b_in_80_b_out
>> respectively that would make for a good name.
> So the only valuable characters are prefix + 8/496/80 the rest doesn't
> really tell much despite all its characters :-). Details like which of the
> numbers is in/out and that the numbers are in bytes could IMO be left to
> struct's comment without loss of much information value.
>
tuxedo_nb04_wmi_8_80 kinda looks strange to me, what about
tuxedo_nb04_wmi_8_in_80_out? but that's on 4 chars shorter.