[PATCH v3 0/6] platform/x86/tuxedo: Fixes for TUXEDO NB04 driver

[PATCH v3 0/6] platform/x86/tuxedo: Fixes for TUXEDO NB04 driver

[Aaron Erhardt]

This patch series contains several fixes for the TUXEDO NB04 driver.
Additional userspace tooling and a new WIP generic lamp array driver 
being tested brought more attention to this driver, thus uncovering 
some problems and potential for improvements.

v3: Improve commit message to properly explain changes
v2: Send all patches as one thread (sorry for the noise)

Aaron Erhardt (6):
  platform/x86/tuxedo: Don't use device driver data
  platform/x86/tuxedo: Set HID report ID on success
  platform/x86/tuxedo: Use intensity according to HID spec
  platform/x86/tuxedo: Fix keyboard LED map ordering
  platform/x86/tuxedo: Update and extend documentation
  MAINTAINERS: Add Aaron Erhardt as maintainer of TUXEDO DRIVERS

 MAINTAINERS                               |   1 +
 drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 122 ++++++++++++----------
 2 files changed, 70 insertions(+), 53 deletions(-)

-- 
2.43.0

[PATCH v3 1/6] platform/x86/tuxedo: Don't use device driver data

[Aaron Erhardt]

The WMI driver for TUXEDO Sirius devices used to rely on the device
driver data through dev_set_drvdata even though it is only a virtual
low level HID driver. For this purpose, it is better to use the
driver_data of the hid_device struct to avoid interfering with high
level device drivers.

Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
---
 drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
index 32d7756022c2..72205de72256 100644
--- a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
+++ b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
@@ -448,7 +448,7 @@ static int tux_ll_start(struct hid_device *hdev)
 	}
 	driver_data->next_lamp_id = 0;
 
-	dev_set_drvdata(&hdev->dev, driver_data);
+	hdev->driver_data = driver_data;
 
 	return ret;
 }
@@ -485,7 +485,7 @@ struct __packed lamp_array_attributes_report_t {
 static int handle_lamp_array_attributes_report(struct hid_device *hdev,
 					       struct lamp_array_attributes_report_t *rep)
 {
-	struct tux_hdev_driver_data_t *driver_data = dev_get_drvdata(&hdev->dev);
+	struct tux_hdev_driver_data_t *driver_data = hdev->driver_data;
 
 	rep->lamp_count = driver_data->lamp_count;
 	rep->bounding_box_width_in_micrometers = 368000;
@@ -510,7 +510,7 @@ struct __packed lamp_attributes_request_report_t {
 static int handle_lamp_attributes_request_report(struct hid_device *hdev,
 						 struct lamp_attributes_request_report_t *rep)
 {
-	struct tux_hdev_driver_data_t *driver_data = dev_get_drvdata(&hdev->dev);
+	struct tux_hdev_driver_data_t *driver_data = hdev->driver_data;
 
 	if (rep->lamp_id < driver_data->lamp_count)
 		driver_data->next_lamp_id = rep->lamp_id;
@@ -539,7 +539,7 @@ struct __packed lamp_attributes_response_report_t {
 static int handle_lamp_attributes_response_report(struct hid_device *hdev,
 						  struct lamp_attributes_response_report_t *rep)
 {
-	struct tux_hdev_driver_data_t *driver_data = dev_get_drvdata(&hdev->dev);
+	struct tux_hdev_driver_data_t *driver_data = hdev->driver_data;
 	u16 lamp_id = driver_data->next_lamp_id;
 
 	rep->lamp_id = lamp_id;
@@ -598,7 +598,7 @@ struct __packed lamp_multi_update_report_t {
 static int handle_lamp_multi_update_report(struct hid_device *hdev,
 					   struct lamp_multi_update_report_t *rep)
 {
-	struct tux_hdev_driver_data_t *driver_data = dev_get_drvdata(&hdev->dev);
+	struct tux_hdev_driver_data_t *driver_data = hdev->driver_data;
 	union tux_wmi_xx_496in_80out_in_t *next = &driver_data->next_kbl_set_multiple_keys_in;
 	struct tux_kbl_set_multiple_keys_in_rgb_config_t *rgb_configs_j;
 	struct wmi_device *wdev = to_wmi_device(hdev->dev.parent);
@@ -683,7 +683,7 @@ struct __packed lamp_range_update_report_t {
 static int handle_lamp_range_update_report(struct hid_device *hdev,
 					   struct lamp_range_update_report_t *rep)
 {
-	struct tux_hdev_driver_data_t *driver_data = dev_get_drvdata(&hdev->dev);
+	struct tux_hdev_driver_data_t *driver_data = hdev->driver_data;
 	struct lamp_multi_update_report_t lamp_multi_update_report = {
 		.report_id = LAMP_MULTI_UPDATE_REPORT_ID,
 	};
-- 
2.43.0

Re: [PATCH v3 1/6] platform/x86/tuxedo: Don't use device driver data

[Werner Sembach]

Am 26.08.26 um 10:11 schrieb Aaron Erhardt:
> The WMI driver for TUXEDO Sirius devices used to rely on the device
> driver data through dev_set_drvdata even though it is only a virtual
> low level HID driver. For this purpose, it is better to use the
> driver_data of the hid_device struct to avoid interfering with high
> level device drivers.
>
> Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
Reviewed-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>   drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> index 32d7756022c2..72205de72256 100644
> --- a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> +++ b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> @@ -448,7 +448,7 @@ static int tux_ll_start(struct hid_device *hdev)
>   	}
>   	driver_data->next_lamp_id = 0;
>   
> -	dev_set_drvdata(&hdev->dev, driver_data);
> +	hdev->driver_data = driver_data;
>   
>   	return ret;
>   }
> @@ -485,7 +485,7 @@ struct __packed lamp_array_attributes_report_t {
>   static int handle_lamp_array_attributes_report(struct hid_device *hdev,
>   					       struct lamp_array_attributes_report_t *rep)
>   {
> -	struct tux_hdev_driver_data_t *driver_data = dev_get_drvdata(&hdev->dev);
> +	struct tux_hdev_driver_data_t *driver_data = hdev->driver_data;
>   
>   	rep->lamp_count = driver_data->lamp_count;
>   	rep->bounding_box_width_in_micrometers = 368000;
> @@ -510,7 +510,7 @@ struct __packed lamp_attributes_request_report_t {
>   static int handle_lamp_attributes_request_report(struct hid_device *hdev,
>   						 struct lamp_attributes_request_report_t *rep)
>   {
> -	struct tux_hdev_driver_data_t *driver_data = dev_get_drvdata(&hdev->dev);
> +	struct tux_hdev_driver_data_t *driver_data = hdev->driver_data;
>   
>   	if (rep->lamp_id < driver_data->lamp_count)
>   		driver_data->next_lamp_id = rep->lamp_id;
> @@ -539,7 +539,7 @@ struct __packed lamp_attributes_response_report_t {
>   static int handle_lamp_attributes_response_report(struct hid_device *hdev,
>   						  struct lamp_attributes_response_report_t *rep)
>   {
> -	struct tux_hdev_driver_data_t *driver_data = dev_get_drvdata(&hdev->dev);
> +	struct tux_hdev_driver_data_t *driver_data = hdev->driver_data;
>   	u16 lamp_id = driver_data->next_lamp_id;
>   
>   	rep->lamp_id = lamp_id;
> @@ -598,7 +598,7 @@ struct __packed lamp_multi_update_report_t {
>   static int handle_lamp_multi_update_report(struct hid_device *hdev,
>   					   struct lamp_multi_update_report_t *rep)
>   {
> -	struct tux_hdev_driver_data_t *driver_data = dev_get_drvdata(&hdev->dev);
> +	struct tux_hdev_driver_data_t *driver_data = hdev->driver_data;
>   	union tux_wmi_xx_496in_80out_in_t *next = &driver_data->next_kbl_set_multiple_keys_in;
>   	struct tux_kbl_set_multiple_keys_in_rgb_config_t *rgb_configs_j;
>   	struct wmi_device *wdev = to_wmi_device(hdev->dev.parent);
> @@ -683,7 +683,7 @@ struct __packed lamp_range_update_report_t {
>   static int handle_lamp_range_update_report(struct hid_device *hdev,
>   					   struct lamp_range_update_report_t *rep)
>   {
> -	struct tux_hdev_driver_data_t *driver_data = dev_get_drvdata(&hdev->dev);
> +	struct tux_hdev_driver_data_t *driver_data = hdev->driver_data;
>   	struct lamp_multi_update_report_t lamp_multi_update_report = {
>   		.report_id = LAMP_MULTI_UPDATE_REPORT_ID,
>   	};

[PATCH v3 2/6] platform/x86/tuxedo: Set HID report ID on success

[Aaron Erhardt]

While not strictly necessary due to the synchronous handling of HID
requests, it is better to set the ID of the HID report before
returning the buffer. This also better complies with the HID spec and
avoids problems with userspace tools expecting a report ID.

Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
---
 drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 26 +++++++++++++++++------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
index 72205de72256..11babc7c7767 100644
--- a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
+++ b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
@@ -766,6 +766,8 @@ static int handle_lamp_array_control_report(struct hid_device *hdev __always_unu
 static int tux_ll_raw_request(struct hid_device *hdev, u8 reportnum, u8 *buf,
 			      size_t len, unsigned char rtype, int reqtype)
 {
+	int ret = -EINVAL;
+
 	if (rtype != HID_FEATURE_REPORT)
 		return -EINVAL;
 
@@ -775,13 +777,15 @@ static int tux_ll_raw_request(struct hid_device *hdev, u8 reportnum, u8 *buf,
 		case LAMP_ARRAY_ATTRIBUTES_REPORT_ID:
 			if (len != sizeof(struct lamp_array_attributes_report_t))
 				return -EINVAL;
-			return handle_lamp_array_attributes_report(hdev,
+			ret = handle_lamp_array_attributes_report(hdev,
 				(struct lamp_array_attributes_report_t *)buf);
+			break;
 		case LAMP_ATTRIBUTES_RESPONSE_REPORT_ID:
 			if (len != sizeof(struct lamp_attributes_response_report_t))
 				return -EINVAL;
-			return handle_lamp_attributes_response_report(hdev,
+			ret = handle_lamp_attributes_response_report(hdev,
 				(struct lamp_attributes_response_report_t *)buf);
+			break;
 		}
 		break;
 	case HID_REQ_SET_REPORT:
@@ -789,28 +793,36 @@ static int tux_ll_raw_request(struct hid_device *hdev, u8 reportnum, u8 *buf,
 		case LAMP_ATTRIBUTES_REQUEST_REPORT_ID:
 			if (len != sizeof(struct lamp_attributes_request_report_t))
 				return -EINVAL;
-			return handle_lamp_attributes_request_report(hdev,
+			ret = handle_lamp_attributes_request_report(hdev,
 				(struct lamp_attributes_request_report_t *)buf);
+			break;
 		case LAMP_MULTI_UPDATE_REPORT_ID:
 			if (len != sizeof(struct lamp_multi_update_report_t))
 				return -EINVAL;
-			return handle_lamp_multi_update_report(hdev,
+			ret = handle_lamp_multi_update_report(hdev,
 				(struct lamp_multi_update_report_t *)buf);
+			break;
 		case LAMP_RANGE_UPDATE_REPORT_ID:
 			if (len != sizeof(struct lamp_range_update_report_t))
 				return -EINVAL;
-			return handle_lamp_range_update_report(hdev,
+			ret = handle_lamp_range_update_report(hdev,
 				(struct lamp_range_update_report_t *)buf);
+			break;
 		case LAMP_ARRAY_CONTROL_REPORT_ID:
 			if (len != sizeof(struct lamp_array_control_report_t))
 				return -EINVAL;
-			return handle_lamp_array_control_report(hdev,
+			ret = handle_lamp_array_control_report(hdev,
 				(struct lamp_array_control_report_t *)buf);
+			break;
 		}
 		break;
 	}
 
-	return -EINVAL;
+	/* Set report number on success */
+	if (ret > 0)
+		buf[0] = reportnum;
+
+	return ret;
 }
 
 static const struct hid_ll_driver tux_ll_driver = {
-- 
2.43.0

Re: [PATCH v3 2/6] platform/x86/tuxedo: Set HID report ID on success

[Werner Sembach]

Am 26.08.26 um 10:11 schrieb Aaron Erhardt:
> While not strictly necessary due to the synchronous handling of HID
> requests, it is better to set the ID of the HID report before
> returning the buffer. This also better complies with the HID spec and
> avoids problems with userspace tools expecting a report ID.
>
> Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
Reviewed-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>   drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 26 +++++++++++++++++------
>   1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> index 72205de72256..11babc7c7767 100644
> --- a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> +++ b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> @@ -766,6 +766,8 @@ static int handle_lamp_array_control_report(struct hid_device *hdev __always_unu
>   static int tux_ll_raw_request(struct hid_device *hdev, u8 reportnum, u8 *buf,
>   			      size_t len, unsigned char rtype, int reqtype)
>   {
> +	int ret = -EINVAL;
> +
>   	if (rtype != HID_FEATURE_REPORT)
>   		return -EINVAL;
>   
> @@ -775,13 +777,15 @@ static int tux_ll_raw_request(struct hid_device *hdev, u8 reportnum, u8 *buf,
>   		case LAMP_ARRAY_ATTRIBUTES_REPORT_ID:
>   			if (len != sizeof(struct lamp_array_attributes_report_t))
>   				return -EINVAL;
> -			return handle_lamp_array_attributes_report(hdev,
> +			ret = handle_lamp_array_attributes_report(hdev,
>   				(struct lamp_array_attributes_report_t *)buf);
> +			break;
>   		case LAMP_ATTRIBUTES_RESPONSE_REPORT_ID:
>   			if (len != sizeof(struct lamp_attributes_response_report_t))
>   				return -EINVAL;
> -			return handle_lamp_attributes_response_report(hdev,
> +			ret = handle_lamp_attributes_response_report(hdev,
>   				(struct lamp_attributes_response_report_t *)buf);
> +			break;
>   		}
>   		break;
>   	case HID_REQ_SET_REPORT:
> @@ -789,28 +793,36 @@ static int tux_ll_raw_request(struct hid_device *hdev, u8 reportnum, u8 *buf,
>   		case LAMP_ATTRIBUTES_REQUEST_REPORT_ID:
>   			if (len != sizeof(struct lamp_attributes_request_report_t))
>   				return -EINVAL;
> -			return handle_lamp_attributes_request_report(hdev,
> +			ret = handle_lamp_attributes_request_report(hdev,
>   				(struct lamp_attributes_request_report_t *)buf);
> +			break;
>   		case LAMP_MULTI_UPDATE_REPORT_ID:
>   			if (len != sizeof(struct lamp_multi_update_report_t))
>   				return -EINVAL;
> -			return handle_lamp_multi_update_report(hdev,
> +			ret = handle_lamp_multi_update_report(hdev,
>   				(struct lamp_multi_update_report_t *)buf);
> +			break;
>   		case LAMP_RANGE_UPDATE_REPORT_ID:
>   			if (len != sizeof(struct lamp_range_update_report_t))
>   				return -EINVAL;
> -			return handle_lamp_range_update_report(hdev,
> +			ret = handle_lamp_range_update_report(hdev,
>   				(struct lamp_range_update_report_t *)buf);
> +			break;
>   		case LAMP_ARRAY_CONTROL_REPORT_ID:
>   			if (len != sizeof(struct lamp_array_control_report_t))
>   				return -EINVAL;
> -			return handle_lamp_array_control_report(hdev,
> +			ret = handle_lamp_array_control_report(hdev,
>   				(struct lamp_array_control_report_t *)buf);
> +			break;
>   		}
>   		break;
>   	}
>   
> -	return -EINVAL;
> +	/* Set report number on success */
> +	if (ret > 0)
> +		buf[0] = reportnum;
> +
> +	return ret;
>   }
>   
>   static const struct hid_ll_driver tux_ll_driver = {

[PATCH v3 3/6] platform/x86/tuxedo: Use intensity according to HID spec

[Aaron Erhardt]

So far, this driver assumed incorrectly that the HID spec requires an
8-bit intensity value to be honored. The spec actually allows multiple
intensities for RGB lamps, but suggest to rather set brightness
through the individual channels, which is also what the Microsoft
MacroPad reference implementation does.

Accordingly, this commit simplifies intensity handling by offering
only two intensity values for turning LEDs on and off. All other color
values are submitted through the color channels individually, thus
avoiding duplicated handling of brightness.

Additionally, the incorrect comments explaining the deviation from the
MacroPad reference implementation were removed.

Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
---
 drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 25 ++++++++++++-----------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
index 11babc7c7767..8f1ffca0430d 100644
--- a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
+++ b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
@@ -553,7 +553,7 @@ static int handle_lamp_attributes_response_report(struct hid_device *hdev,
 	rep->red_level_count = 0xff;
 	rep->green_level_count = 0xff;
 	rep->blue_level_count = 0xff;
-	rep->intensity_level_count = 0xff;
+	rep->intensity_level_count = 0x1;
 	rep->is_programmable = 1;
 
 	if (driver_data->kbl_map[lamp_id].code <= 0xe8) {
@@ -640,22 +640,23 @@ static int handle_lamp_multi_update_report(struct hid_device *hdev,
 					j + 1;
 			rgb_configs_j->key_id = key_id;
 			/*
-			 * While this driver respects update_channel.intensity
-			 * according to "HID Usage Tables v1.5" also on RGB
-			 * leds, the Microsoft MacroPad reference implementation
+			 * This driver uses update_channel.intensity according to
+			 * "Color Attributes Examples" in "HID Usage Tables v1.7".
+			 * Only two intensity values are allowed for turning LEDs
+			 * on or off, while color and brightness can be controlled
+			 * through the RGB values. This is also identical to 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
-			 * ignore the update_channel.intensity.
+			 * 1d6c3ad).
 			 */
-			intensity_i = rep->update_channels[i].intensity;
+			intensity_i = min(1, rep->update_channels[i].intensity);
 			red_i = rep->update_channels[i].red;
 			green_i = rep->update_channels[i].green;
 			blue_i = rep->update_channels[i].blue;
-			rgb_configs_j->red = red_i * intensity_i / 0xff;
-			rgb_configs_j->green = green_i * intensity_i / 0xff;
-			rgb_configs_j->blue = blue_i * intensity_i / 0xff;
+
+			rgb_configs_j->red = red_i * intensity_i;
+			rgb_configs_j->green = green_i * intensity_i;
+			rgb_configs_j->blue = blue_i * intensity_i;
 
 			break;
 		}
-- 
2.43.0

Re: [PATCH v3 3/6] platform/x86/tuxedo: Use intensity according to HID spec

[Werner Sembach]

Am 26.08.26 um 10:11 schrieb Aaron Erhardt:
> So far, this driver assumed incorrectly that the HID spec requires an
> 8-bit intensity value to be honored. The spec actually allows multiple
> intensities for RGB lamps, but suggest to rather set brightness
> through the individual channels, which is also what the Microsoft
> MacroPad reference implementation does.
>
> Accordingly, this commit simplifies intensity handling by offering
> only two intensity values for turning LEDs on and off. All other color
> values are submitted through the color channels individually, thus
> avoiding duplicated handling of brightness.
>
> Additionally, the incorrect comments explaining the deviation from the
> MacroPad reference implementation were removed.
>
> Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
Reviewed-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>   drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 25 ++++++++++++-----------
>   1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> index 11babc7c7767..8f1ffca0430d 100644
> --- a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> +++ b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> @@ -553,7 +553,7 @@ static int handle_lamp_attributes_response_report(struct hid_device *hdev,
>   	rep->red_level_count = 0xff;
>   	rep->green_level_count = 0xff;
>   	rep->blue_level_count = 0xff;
> -	rep->intensity_level_count = 0xff;
> +	rep->intensity_level_count = 0x1;
>   	rep->is_programmable = 1;
>   
>   	if (driver_data->kbl_map[lamp_id].code <= 0xe8) {
> @@ -640,22 +640,23 @@ static int handle_lamp_multi_update_report(struct hid_device *hdev,
>   					j + 1;
>   			rgb_configs_j->key_id = key_id;
>   			/*
> -			 * While this driver respects update_channel.intensity
> -			 * according to "HID Usage Tables v1.5" also on RGB
> -			 * leds, the Microsoft MacroPad reference implementation
> +			 * This driver uses update_channel.intensity according to
> +			 * "Color Attributes Examples" in "HID Usage Tables v1.7".
> +			 * Only two intensity values are allowed for turning LEDs
> +			 * on or off, while color and brightness can be controlled
> +			 * through the RGB values. This is also identical to 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
> -			 * ignore the update_channel.intensity.
> +			 * 1d6c3ad).
>   			 */
> -			intensity_i = rep->update_channels[i].intensity;
> +			intensity_i = min(1, rep->update_channels[i].intensity);
>   			red_i = rep->update_channels[i].red;
>   			green_i = rep->update_channels[i].green;
>   			blue_i = rep->update_channels[i].blue;
> -			rgb_configs_j->red = red_i * intensity_i / 0xff;
> -			rgb_configs_j->green = green_i * intensity_i / 0xff;
> -			rgb_configs_j->blue = blue_i * intensity_i / 0xff;
> +
> +			rgb_configs_j->red = red_i * intensity_i;
> +			rgb_configs_j->green = green_i * intensity_i;
> +			rgb_configs_j->blue = blue_i * intensity_i;
>   
>   			break;
>   		}

[PATCH v3 4/6] platform/x86/tuxedo: Fix keyboard LED map ordering

[Aaron Erhardt]

The LED mappings in sirius_16_[iso|ansii]_kbl_map did contain a
flipped line that caused LEDs to turn on in the wrong order when
counting up the Lamp ID.

Cc: stable@vger.kernel.org
Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
---
 drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
index 8f1ffca0430d..2b985b030197 100644
--- a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
+++ b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
@@ -232,8 +232,8 @@ static const struct tux_kbl_map_entry_t sirius_16_ansii_kbl_map[] = {
 	{ 0x2e, { 246000,  67500, 5250 } },
 	{ 0x2a, { 269500,  67500, 5250 } },
 	{ 0x53, { 294500,  67500, 5250 } },
-	{ 0x55, { 311200,  67500, 5250 } },
-	{ 0x54, { 327900,  67500, 5250 } },
+	{ 0x54, { 311200,  67500, 5250 } },
+	{ 0x55, { 327900,  67500, 5250 } },
 	{ 0x56, { 344600,  67500, 5250 } },
 	{ 0x2b, {  31000,  85500, 5500 } },
 	{ 0x14, {  51500,  85500, 5500 } },
@@ -337,8 +337,8 @@ static const struct tux_kbl_map_entry_t sirius_16_iso_kbl_map[] = {
 	{ 0x2e, { 246000,  67500, 5250 } },
 	{ 0x2a, { 269500,  67500, 5250 } },
 	{ 0x53, { 294500,  67500, 5250 } },
-	{ 0x55, { 311200,  67500, 5250 } },
-	{ 0x54, { 327900,  67500, 5250 } },
+	{ 0x54, { 311200,  67500, 5250 } },
+	{ 0x55, { 327900,  67500, 5250 } },
 	{ 0x56, { 344600,  67500, 5250 } },
 	{ 0x2b, {  31000,  85500, 5500 } },
 	{ 0x14, {  51500,  85500, 5500 } },
-- 
2.43.0

Re: [PATCH v3 4/6] platform/x86/tuxedo: Fix keyboard LED map ordering

[Werner Sembach]

Am 26.08.26 um 10:11 schrieb Aaron Erhardt:
> The LED mappings in sirius_16_[iso|ansii]_kbl_map did contain a
> flipped line that caused LEDs to turn on in the wrong order when
> counting up the Lamp ID.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
Reviewed-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>   drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> index 8f1ffca0430d..2b985b030197 100644
> --- a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> +++ b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> @@ -232,8 +232,8 @@ static const struct tux_kbl_map_entry_t sirius_16_ansii_kbl_map[] = {
>   	{ 0x2e, { 246000,  67500, 5250 } },
>   	{ 0x2a, { 269500,  67500, 5250 } },
>   	{ 0x53, { 294500,  67500, 5250 } },
> -	{ 0x55, { 311200,  67500, 5250 } },
> -	{ 0x54, { 327900,  67500, 5250 } },
> +	{ 0x54, { 311200,  67500, 5250 } },
> +	{ 0x55, { 327900,  67500, 5250 } },
>   	{ 0x56, { 344600,  67500, 5250 } },
>   	{ 0x2b, {  31000,  85500, 5500 } },
>   	{ 0x14, {  51500,  85500, 5500 } },
> @@ -337,8 +337,8 @@ static const struct tux_kbl_map_entry_t sirius_16_iso_kbl_map[] = {
>   	{ 0x2e, { 246000,  67500, 5250 } },
>   	{ 0x2a, { 269500,  67500, 5250 } },
>   	{ 0x53, { 294500,  67500, 5250 } },
> -	{ 0x55, { 311200,  67500, 5250 } },
> -	{ 0x54, { 327900,  67500, 5250 } },
> +	{ 0x54, { 311200,  67500, 5250 } },
> +	{ 0x55, { 327900,  67500, 5250 } },
>   	{ 0x56, { 344600,  67500, 5250 } },
>   	{ 0x2b, {  31000,  85500, 5500 } },
>   	{ 0x14, {  51500,  85500, 5500 } },

Re: [PATCH v3 4/6] platform/x86/tuxedo: Fix keyboard LED map ordering

[Ilpo Järvinen]

On Wed, 26 Aug 2026, Aaron Erhardt wrote:

> The LED mappings in sirius_16_[iso|ansii]_kbl_map did contain a
> flipped line that caused LEDs to turn on in the wrong order when
> counting up the Lamp ID.
> 
> Cc: stable@vger.kernel.org

And the Fixes tag?

> Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
> ---
>  drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> index 8f1ffca0430d..2b985b030197 100644
> --- a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> +++ b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> @@ -232,8 +232,8 @@ static const struct tux_kbl_map_entry_t sirius_16_ansii_kbl_map[] = {
>  	{ 0x2e, { 246000,  67500, 5250 } },
>  	{ 0x2a, { 269500,  67500, 5250 } },
>  	{ 0x53, { 294500,  67500, 5250 } },
> -	{ 0x55, { 311200,  67500, 5250 } },
> -	{ 0x54, { 327900,  67500, 5250 } },
> +	{ 0x54, { 311200,  67500, 5250 } },
> +	{ 0x55, { 327900,  67500, 5250 } },
>  	{ 0x56, { 344600,  67500, 5250 } },
>  	{ 0x2b, {  31000,  85500, 5500 } },
>  	{ 0x14, {  51500,  85500, 5500 } },
> @@ -337,8 +337,8 @@ static const struct tux_kbl_map_entry_t sirius_16_iso_kbl_map[] = {
>  	{ 0x2e, { 246000,  67500, 5250 } },
>  	{ 0x2a, { 269500,  67500, 5250 } },
>  	{ 0x53, { 294500,  67500, 5250 } },
> -	{ 0x55, { 311200,  67500, 5250 } },
> -	{ 0x54, { 327900,  67500, 5250 } },
> +	{ 0x54, { 311200,  67500, 5250 } },
> +	{ 0x55, { 327900,  67500, 5250 } },
>  	{ 0x56, { 344600,  67500, 5250 } },
>  	{ 0x2b, {  31000,  85500, 5500 } },
>  	{ 0x14, {  51500,  85500, 5500 } },
> 

-- 
 i.

Re: [PATCH v3 4/6] platform/x86/tuxedo: Fix keyboard LED map ordering

[Aaron Erhardt]

Am 15.09.26 um 15:00 schrieb Ilpo Järvinen:
> On Wed, 26 Aug 2026, Aaron Erhardt wrote:
> 
>> The LED mappings in sirius_16_[iso|ansii]_kbl_map did contain a
>> flipped line that caused LEDs to turn on in the wrong order when
>> counting up the Lamp ID.
>>
>> Cc: stable@vger.kernel.org
> 
> And the Fixes tag?

I assume this is mainly for the stable maintainers to be able to track down
when the bug was introduced, right? I thought "Fixes:" would only be used for
regression bugs.

The only commit relevant to this is the one that introduced the driver itself:
cfd84b3f419bf0aec60ecddc92c61b539c339ec9

I correct this in the next series.

> 
>> Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
>> ---
>>  drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
>> index 8f1ffca0430d..2b985b030197 100644
>> --- a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
>> +++ b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
>> @@ -232,8 +232,8 @@ static const struct tux_kbl_map_entry_t sirius_16_ansii_kbl_map[] = {
>>  	{ 0x2e, { 246000,  67500, 5250 } },
>>  	{ 0x2a, { 269500,  67500, 5250 } },
>>  	{ 0x53, { 294500,  67500, 5250 } },
>> -	{ 0x55, { 311200,  67500, 5250 } },
>> -	{ 0x54, { 327900,  67500, 5250 } },
>> +	{ 0x54, { 311200,  67500, 5250 } },
>> +	{ 0x55, { 327900,  67500, 5250 } },
>>  	{ 0x56, { 344600,  67500, 5250 } },
>>  	{ 0x2b, {  31000,  85500, 5500 } },
>>  	{ 0x14, {  51500,  85500, 5500 } },
>> @@ -337,8 +337,8 @@ static const struct tux_kbl_map_entry_t sirius_16_iso_kbl_map[] = {
>>  	{ 0x2e, { 246000,  67500, 5250 } },
>>  	{ 0x2a, { 269500,  67500, 5250 } },
>>  	{ 0x53, { 294500,  67500, 5250 } },
>> -	{ 0x55, { 311200,  67500, 5250 } },
>> -	{ 0x54, { 327900,  67500, 5250 } },
>> +	{ 0x54, { 311200,  67500, 5250 } },
>> +	{ 0x55, { 327900,  67500, 5250 } },
>>  	{ 0x56, { 344600,  67500, 5250 } },
>>  	{ 0x2b, {  31000,  85500, 5500 } },
>>  	{ 0x14, {  51500,  85500, 5500 } },
>>
> 

Re: [PATCH v3 4/6] platform/x86/tuxedo: Fix keyboard LED map ordering

[Ilpo Järvinen]

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

On Tue, 15 Sep 2026, Aaron Erhardt wrote:

> Am 15.09.26 um 15:00 schrieb Ilpo Järvinen:
> > On Wed, 26 Aug 2026, Aaron Erhardt wrote:
> > 
> >> The LED mappings in sirius_16_[iso|ansii]_kbl_map did contain a
> >> flipped line that caused LEDs to turn on in the wrong order when
> >> counting up the Lamp ID.
> >>
> >> Cc: stable@vger.kernel.org
> > 
> > And the Fixes tag?
> 
> I assume this is mainly for the stable maintainers to be able to track down
> when the bug was introduced, right? I thought "Fixes:" would only be used for
> regression bugs.

Fixes tag is not limited to regressions but should be used with any bug 
to link the commits. And yes, it helps stable people among other things.

Only "bugs" which aren't bugs (cannot be triggered) usually shouldn't have 
one, or comment or style corrections, etc. changes that have no functional 
impact.

> The only commit relevant to this is the one that introduced the driver itself:
> cfd84b3f419bf0aec60ecddc92c61b539c339ec9
> 
> I correct this in the next series.

-- 
 i.

> >> Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
> >> ---
> >>  drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> >> index 8f1ffca0430d..2b985b030197 100644
> >> --- a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> >> +++ b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> >> @@ -232,8 +232,8 @@ static const struct tux_kbl_map_entry_t sirius_16_ansii_kbl_map[] = {
> >>  	{ 0x2e, { 246000,  67500, 5250 } },
> >>  	{ 0x2a, { 269500,  67500, 5250 } },
> >>  	{ 0x53, { 294500,  67500, 5250 } },
> >> -	{ 0x55, { 311200,  67500, 5250 } },
> >> -	{ 0x54, { 327900,  67500, 5250 } },
> >> +	{ 0x54, { 311200,  67500, 5250 } },
> >> +	{ 0x55, { 327900,  67500, 5250 } },
> >>  	{ 0x56, { 344600,  67500, 5250 } },
> >>  	{ 0x2b, {  31000,  85500, 5500 } },
> >>  	{ 0x14, {  51500,  85500, 5500 } },
> >> @@ -337,8 +337,8 @@ static const struct tux_kbl_map_entry_t sirius_16_iso_kbl_map[] = {
> >>  	{ 0x2e, { 246000,  67500, 5250 } },
> >>  	{ 0x2a, { 269500,  67500, 5250 } },
> >>  	{ 0x53, { 294500,  67500, 5250 } },
> >> -	{ 0x55, { 311200,  67500, 5250 } },
> >> -	{ 0x54, { 327900,  67500, 5250 } },
> >> +	{ 0x54, { 311200,  67500, 5250 } },
> >> +	{ 0x55, { 327900,  67500, 5250 } },
> >>  	{ 0x56, { 344600,  67500, 5250 } },
> >>  	{ 0x2b, {  31000,  85500, 5500 } },
> >>  	{ 0x14, {  51500,  85500, 5500 } },
> >>
> > 
> 

[PATCH v3 5/6] platform/x86/tuxedo: Update and extend documentation

[Aaron Erhardt]

Remove an incorrect comment about the Microsoft MacroPad reference
implementation allegedly deviating from the spec and add more
information about the module and some other minor improvements.

Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
---
 drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 51 ++++++++++++-----------
 1 file changed, 27 insertions(+), 24 deletions(-)

diff --git a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
index 2b985b030197..3e5ba524fe58 100644
--- a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
+++ b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
@@ -1,9 +1,15 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * This driver implements the WMI AB device found on TUXEDO notebooks with board
- * vendor NB04.
+ * vendor NB04. This enables keyboard backlight control via a virtual HID
+ * LampArray device.
+ *
+ * The device will be available through the regular HID interfaces, such as
+ * hidraw and can be used by any userspace program that implements the HID
+ * LampArray standard.
  *
  * Copyright (C) 2024-2025 Werner Sembach <wse@tuxedocomputers.com>
+ * Copyright (C) 2026 Aaron Erhardt <aer@tuxedocomputers.com>
  */
 
 #include <linux/dmi.h>
@@ -488,12 +494,14 @@ static int handle_lamp_array_attributes_report(struct hid_device *hdev,
 	struct tux_hdev_driver_data_t *driver_data = hdev->driver_data;
 
 	rep->lamp_count = driver_data->lamp_count;
+
+	// Physical dimensions of the Sirius 16 keyboard
 	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"
+	 * "HID Usage Tables v1.7"
 	 */
 	rep->lamp_array_kind = 1;
 	// Some guessed value for interval microseconds
@@ -547,7 +555,7 @@ static int handle_lamp_attributes_response_report(struct hid_device *hdev,
 	rep->update_latency_in_microseconds = 100;
 	/*
 	 * LampPurposeControl, see "26.3.1 LampPurposes Flags" of
-	 * "HID Usage Tables v1.5"
+	 * "HID Usage Tables v1.7"
 	 */
 	rep->lamp_purpose = 1;
 	rep->red_level_count = 0xff;
@@ -560,8 +568,8 @@ static int handle_lamp_attributes_response_report(struct hid_device *hdev,
 		rep->input_binding = driver_data->kbl_map[lamp_id].code;
 	} else {
 		/*
-		 * Everything bigger is reserved/undefined, see
-		 * "10 Keyboard/Keypad Page (0x07)" of "HID Usage Tables v1.5"
+		 * Everything bigger than 0xe8 is reserved/undefined, see
+		 * "10 Keyboard/Keypad Page (0x07)" of "HID Usage Tables v1.7"
 		 * and should return 0, see "26.8.3 Lamp Attributes" of the same
 		 * document.
 		 */
@@ -606,10 +614,7 @@ static int handle_lamp_multi_update_report(struct hid_device *hdev,
 	u8 key_id, key_id_j, intensity_i, red_i, green_i, blue_i;
 	int ret;
 
-	/*
-	 * Catching misformatted lamp_multi_update_report and fail silently
-	 * according to "HID Usage Tables v1.5"
-	 */
+	// Catch bad reports and fail silently according to "HID Usage Tables v1.7"
 	for (unsigned int i = 0; i < rep->lamp_count; ++i) {
 		if (rep->lamp_id[i] > driver_data->lamp_count) {
 			hid_dbg(hdev, "Out of bounds lamp_id in lamp_multi_update_report. Skipping whole report!\n");
@@ -624,6 +629,7 @@ static int handle_lamp_multi_update_report(struct hid_device *hdev,
 		}
 	}
 
+	// Fill kbl_set_multiple_keys_in update buffer
 	for (unsigned int i = 0; i < rep->lamp_count; ++i) {
 		key_id = driver_data->kbl_map[rep->lamp_id[i]].code;
 
@@ -632,6 +638,8 @@ static int handle_lamp_multi_update_report(struct hid_device *hdev,
 		     ++j) {
 			rgb_configs_j = &next->kbl_set_multiple_keys_in.rgb_configs[j];
 			key_id_j = rgb_configs_j->key_id;
+
+			// Search for existing or empty entry
 			if (key_id_j != 0x00 && key_id_j != key_id)
 				continue;
 
@@ -691,10 +699,7 @@ static int handle_lamp_range_update_report(struct hid_device *hdev,
 	struct lamp_rgbi_tuple_t *update_channels_j;
 	int ret;
 
-	/*
-	 * Catching misformatted lamp_range_update_report and fail silently
-	 * according to "HID Usage Tables v1.5"
-	 */
+	// Catch bad reports and fail silently according to "HID Usage Tables v1.7"
 	if (rep->lamp_id_start > rep->lamp_id_end) {
 		hid_dbg(hdev, "lamp_id_start > lamp_id_end in lamp_range_update_report. Skipping whole report!\n");
 		return sizeof(*rep);
@@ -706,8 +711,8 @@ static int handle_lamp_range_update_report(struct hid_device *hdev,
 	}
 
 	/*
-	 * Break handle_lamp_range_update_report call down to multiple
-	 * handle_lamp_multi_update_report calls to easily ensure that mixing
+	 * Break handle_lamp_range_update_report call down into multiple
+	 * handle_lamp_multi_update_report calls to ensure that mixing
 	 * handle_lamp_range_update_report and handle_lamp_multi_update_report
 	 * does not break things.
 	 */
@@ -750,15 +755,12 @@ static int handle_lamp_array_control_report(struct hid_device *hdev __always_unu
 					    struct lamp_array_control_report_t *rep)
 {
 	/*
-	 * The keyboards firmware doesn't have any built in controls and the
-	 * built in effects are not implemented so this is a NOOP.
-	 * According to the HID Documentation (HID Usage Tables v1.5) this
+	 * The keyboard's firmware doesn't have any built-in controls and the
+	 * built-in effects are not implemented so this is a NOOP.
+	 * According to the HID Documentation (HID Usage Tables v1.7) 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.
+	 * this possibility too.
 	 */
 
 	return sizeof(*rep);
@@ -894,8 +896,8 @@ static struct wmi_driver tuxedo_nb04_wmi_tux_driver = {
 };
 
 /*
- * 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
+ * We don't know whether 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.
  */
 static const struct dmi_system_id tested_devices_dmi_table[] __initconst = {
@@ -933,4 +935,5 @@ module_exit(tuxedo_nb04_wmi_tux_exit);
 
 MODULE_DESCRIPTION("Virtual HID LampArray interface for TUXEDO NB04 devices");
 MODULE_AUTHOR("Werner Sembach <wse@tuxedocomputers.com>");
+MODULE_AUTHOR("Aaron Erhardt <aer@tuxedocomputers.com>");
 MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH v3 5/6] platform/x86/tuxedo: Update and extend documentation

[Werner Sembach]

Am 26.08.26 um 10:11 schrieb Aaron Erhardt:
> Remove an incorrect comment about the Microsoft MacroPad reference
> implementation allegedly deviating from the spec and add more
> information about the module and some other minor improvements.
>
> Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
Reviewed-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>   drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 51 ++++++++++++-----------
>   1 file changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> index 2b985b030197..3e5ba524fe58 100644
> --- a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> +++ b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> @@ -1,9 +1,15 @@
>   // SPDX-License-Identifier: GPL-2.0-or-later
>   /*
>    * This driver implements the WMI AB device found on TUXEDO notebooks with board
> - * vendor NB04.
> + * vendor NB04. This enables keyboard backlight control via a virtual HID
> + * LampArray device.
> + *
> + * The device will be available through the regular HID interfaces, such as
> + * hidraw and can be used by any userspace program that implements the HID
> + * LampArray standard.
>    *
>    * Copyright (C) 2024-2025 Werner Sembach <wse@tuxedocomputers.com>
> + * Copyright (C) 2026 Aaron Erhardt <aer@tuxedocomputers.com>
>    */
>   
>   #include <linux/dmi.h>
> @@ -488,12 +494,14 @@ static int handle_lamp_array_attributes_report(struct hid_device *hdev,
>   	struct tux_hdev_driver_data_t *driver_data = hdev->driver_data;
>   
>   	rep->lamp_count = driver_data->lamp_count;
> +
> +	// Physical dimensions of the Sirius 16 keyboard
>   	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"
> +	 * "HID Usage Tables v1.7"
>   	 */
>   	rep->lamp_array_kind = 1;
>   	// Some guessed value for interval microseconds
> @@ -547,7 +555,7 @@ static int handle_lamp_attributes_response_report(struct hid_device *hdev,
>   	rep->update_latency_in_microseconds = 100;
>   	/*
>   	 * LampPurposeControl, see "26.3.1 LampPurposes Flags" of
> -	 * "HID Usage Tables v1.5"
> +	 * "HID Usage Tables v1.7"
>   	 */
>   	rep->lamp_purpose = 1;
>   	rep->red_level_count = 0xff;
> @@ -560,8 +568,8 @@ static int handle_lamp_attributes_response_report(struct hid_device *hdev,
>   		rep->input_binding = driver_data->kbl_map[lamp_id].code;
>   	} else {
>   		/*
> -		 * Everything bigger is reserved/undefined, see
> -		 * "10 Keyboard/Keypad Page (0x07)" of "HID Usage Tables v1.5"
> +		 * Everything bigger than 0xe8 is reserved/undefined, see
> +		 * "10 Keyboard/Keypad Page (0x07)" of "HID Usage Tables v1.7"
>   		 * and should return 0, see "26.8.3 Lamp Attributes" of the same
>   		 * document.
>   		 */
> @@ -606,10 +614,7 @@ static int handle_lamp_multi_update_report(struct hid_device *hdev,
>   	u8 key_id, key_id_j, intensity_i, red_i, green_i, blue_i;
>   	int ret;
>   
> -	/*
> -	 * Catching misformatted lamp_multi_update_report and fail silently
> -	 * according to "HID Usage Tables v1.5"
> -	 */
> +	// Catch bad reports and fail silently according to "HID Usage Tables v1.7"
>   	for (unsigned int i = 0; i < rep->lamp_count; ++i) {
>   		if (rep->lamp_id[i] > driver_data->lamp_count) {
>   			hid_dbg(hdev, "Out of bounds lamp_id in lamp_multi_update_report. Skipping whole report!\n");
> @@ -624,6 +629,7 @@ static int handle_lamp_multi_update_report(struct hid_device *hdev,
>   		}
>   	}
>   
> +	// Fill kbl_set_multiple_keys_in update buffer
>   	for (unsigned int i = 0; i < rep->lamp_count; ++i) {
>   		key_id = driver_data->kbl_map[rep->lamp_id[i]].code;
>   
> @@ -632,6 +638,8 @@ static int handle_lamp_multi_update_report(struct hid_device *hdev,
>   		     ++j) {
>   			rgb_configs_j = &next->kbl_set_multiple_keys_in.rgb_configs[j];
>   			key_id_j = rgb_configs_j->key_id;
> +
> +			// Search for existing or empty entry
>   			if (key_id_j != 0x00 && key_id_j != key_id)
>   				continue;
>   
> @@ -691,10 +699,7 @@ static int handle_lamp_range_update_report(struct hid_device *hdev,
>   	struct lamp_rgbi_tuple_t *update_channels_j;
>   	int ret;
>   
> -	/*
> -	 * Catching misformatted lamp_range_update_report and fail silently
> -	 * according to "HID Usage Tables v1.5"
> -	 */
> +	// Catch bad reports and fail silently according to "HID Usage Tables v1.7"
>   	if (rep->lamp_id_start > rep->lamp_id_end) {
>   		hid_dbg(hdev, "lamp_id_start > lamp_id_end in lamp_range_update_report. Skipping whole report!\n");
>   		return sizeof(*rep);
> @@ -706,8 +711,8 @@ static int handle_lamp_range_update_report(struct hid_device *hdev,
>   	}
>   
>   	/*
> -	 * Break handle_lamp_range_update_report call down to multiple
> -	 * handle_lamp_multi_update_report calls to easily ensure that mixing
> +	 * Break handle_lamp_range_update_report call down into multiple
> +	 * handle_lamp_multi_update_report calls to ensure that mixing
>   	 * handle_lamp_range_update_report and handle_lamp_multi_update_report
>   	 * does not break things.
>   	 */
> @@ -750,15 +755,12 @@ static int handle_lamp_array_control_report(struct hid_device *hdev __always_unu
>   					    struct lamp_array_control_report_t *rep)
>   {
>   	/*
> -	 * The keyboards firmware doesn't have any built in controls and the
> -	 * built in effects are not implemented so this is a NOOP.
> -	 * According to the HID Documentation (HID Usage Tables v1.5) this
> +	 * The keyboard's firmware doesn't have any built-in controls and the
> +	 * built-in effects are not implemented so this is a NOOP.
> +	 * According to the HID Documentation (HID Usage Tables v1.7) 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.
> +	 * this possibility too.
>   	 */
>   
>   	return sizeof(*rep);
> @@ -894,8 +896,8 @@ static struct wmi_driver tuxedo_nb04_wmi_tux_driver = {
>   };
>   
>   /*
> - * 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
> + * We don't know whether 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.
>    */
>   static const struct dmi_system_id tested_devices_dmi_table[] __initconst = {
> @@ -933,4 +935,5 @@ module_exit(tuxedo_nb04_wmi_tux_exit);
>   
>   MODULE_DESCRIPTION("Virtual HID LampArray interface for TUXEDO NB04 devices");
>   MODULE_AUTHOR("Werner Sembach <wse@tuxedocomputers.com>");
> +MODULE_AUTHOR("Aaron Erhardt <aer@tuxedocomputers.com>");
>   MODULE_LICENSE("GPL");

Re: [PATCH v3 5/6] platform/x86/tuxedo: Update and extend documentation

[Ilpo Järvinen]

On Wed, 26 Aug 2026, Aaron Erhardt wrote:

> Remove an incorrect comment about the Microsoft MacroPad reference
> implementation allegedly deviating from the spec and add more
> information about the module and some other minor improvements.

Was it "incorrect" or was the spec clarified in a later version? If the 
latter, that would be worth to mention instead of claiming the original 
comment was "incorrect".

This is a honest question, I don't know the answer but I'm kind trying to 
read in between lines here how we ended up in this situation so my 
impression could be entirely wrong. ...Thus, please don't assume I know 
much about the content of these specs (despite me briefly looking into 
what I could find around this feature was introduced).

-- 
 i.

> Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
> ---
>  drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 51 ++++++++++++-----------
>  1 file changed, 27 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> index 2b985b030197..3e5ba524fe58 100644
> --- a/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> +++ b/drivers/platform/x86/tuxedo/nb04/wmi_ab.c
> @@ -1,9 +1,15 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * This driver implements the WMI AB device found on TUXEDO notebooks with board
> - * vendor NB04.
> + * vendor NB04. This enables keyboard backlight control via a virtual HID
> + * LampArray device.
> + *
> + * The device will be available through the regular HID interfaces, such as
> + * hidraw and can be used by any userspace program that implements the HID
> + * LampArray standard.
>   *
>   * Copyright (C) 2024-2025 Werner Sembach <wse@tuxedocomputers.com>
> + * Copyright (C) 2026 Aaron Erhardt <aer@tuxedocomputers.com>
>   */
>  
>  #include <linux/dmi.h>
> @@ -488,12 +494,14 @@ static int handle_lamp_array_attributes_report(struct hid_device *hdev,
>  	struct tux_hdev_driver_data_t *driver_data = hdev->driver_data;
>  
>  	rep->lamp_count = driver_data->lamp_count;
> +
> +	// Physical dimensions of the Sirius 16 keyboard
>  	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"
> +	 * "HID Usage Tables v1.7"
>  	 */
>  	rep->lamp_array_kind = 1;
>  	// Some guessed value for interval microseconds
> @@ -547,7 +555,7 @@ static int handle_lamp_attributes_response_report(struct hid_device *hdev,
>  	rep->update_latency_in_microseconds = 100;
>  	/*
>  	 * LampPurposeControl, see "26.3.1 LampPurposes Flags" of
> -	 * "HID Usage Tables v1.5"
> +	 * "HID Usage Tables v1.7"
>  	 */
>  	rep->lamp_purpose = 1;
>  	rep->red_level_count = 0xff;
> @@ -560,8 +568,8 @@ static int handle_lamp_attributes_response_report(struct hid_device *hdev,
>  		rep->input_binding = driver_data->kbl_map[lamp_id].code;
>  	} else {
>  		/*
> -		 * Everything bigger is reserved/undefined, see
> -		 * "10 Keyboard/Keypad Page (0x07)" of "HID Usage Tables v1.5"
> +		 * Everything bigger than 0xe8 is reserved/undefined, see
> +		 * "10 Keyboard/Keypad Page (0x07)" of "HID Usage Tables v1.7"
>  		 * and should return 0, see "26.8.3 Lamp Attributes" of the same
>  		 * document.
>  		 */
> @@ -606,10 +614,7 @@ static int handle_lamp_multi_update_report(struct hid_device *hdev,
>  	u8 key_id, key_id_j, intensity_i, red_i, green_i, blue_i;
>  	int ret;
>  
> -	/*
> -	 * Catching misformatted lamp_multi_update_report and fail silently
> -	 * according to "HID Usage Tables v1.5"
> -	 */
> +	// Catch bad reports and fail silently according to "HID Usage Tables v1.7"
>  	for (unsigned int i = 0; i < rep->lamp_count; ++i) {
>  		if (rep->lamp_id[i] > driver_data->lamp_count) {
>  			hid_dbg(hdev, "Out of bounds lamp_id in lamp_multi_update_report. Skipping whole report!\n");
> @@ -624,6 +629,7 @@ static int handle_lamp_multi_update_report(struct hid_device *hdev,
>  		}
>  	}
>  
> +	// Fill kbl_set_multiple_keys_in update buffer
>  	for (unsigned int i = 0; i < rep->lamp_count; ++i) {
>  		key_id = driver_data->kbl_map[rep->lamp_id[i]].code;
>  
> @@ -632,6 +638,8 @@ static int handle_lamp_multi_update_report(struct hid_device *hdev,
>  		     ++j) {
>  			rgb_configs_j = &next->kbl_set_multiple_keys_in.rgb_configs[j];
>  			key_id_j = rgb_configs_j->key_id;
> +
> +			// Search for existing or empty entry
>  			if (key_id_j != 0x00 && key_id_j != key_id)
>  				continue;
>  
> @@ -691,10 +699,7 @@ static int handle_lamp_range_update_report(struct hid_device *hdev,
>  	struct lamp_rgbi_tuple_t *update_channels_j;
>  	int ret;
>  
> -	/*
> -	 * Catching misformatted lamp_range_update_report and fail silently
> -	 * according to "HID Usage Tables v1.5"
> -	 */
> +	// Catch bad reports and fail silently according to "HID Usage Tables v1.7"
>  	if (rep->lamp_id_start > rep->lamp_id_end) {
>  		hid_dbg(hdev, "lamp_id_start > lamp_id_end in lamp_range_update_report. Skipping whole report!\n");
>  		return sizeof(*rep);
> @@ -706,8 +711,8 @@ static int handle_lamp_range_update_report(struct hid_device *hdev,
>  	}
>  
>  	/*
> -	 * Break handle_lamp_range_update_report call down to multiple
> -	 * handle_lamp_multi_update_report calls to easily ensure that mixing
> +	 * Break handle_lamp_range_update_report call down into multiple
> +	 * handle_lamp_multi_update_report calls to ensure that mixing
>  	 * handle_lamp_range_update_report and handle_lamp_multi_update_report
>  	 * does not break things.
>  	 */
> @@ -750,15 +755,12 @@ static int handle_lamp_array_control_report(struct hid_device *hdev __always_unu
>  					    struct lamp_array_control_report_t *rep)
>  {
>  	/*
> -	 * The keyboards firmware doesn't have any built in controls and the
> -	 * built in effects are not implemented so this is a NOOP.
> -	 * According to the HID Documentation (HID Usage Tables v1.5) this
> +	 * The keyboard's firmware doesn't have any built-in controls and the
> +	 * built-in effects are not implemented so this is a NOOP.
> +	 * According to the HID Documentation (HID Usage Tables v1.7) 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.
> +	 * this possibility too.
>  	 */
>  
>  	return sizeof(*rep);
> @@ -894,8 +896,8 @@ static struct wmi_driver tuxedo_nb04_wmi_tux_driver = {
>  };
>  
>  /*
> - * 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
> + * We don't know whether 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.
>   */
>  static const struct dmi_system_id tested_devices_dmi_table[] __initconst = {
> @@ -933,4 +935,5 @@ module_exit(tuxedo_nb04_wmi_tux_exit);
>  
>  MODULE_DESCRIPTION("Virtual HID LampArray interface for TUXEDO NB04 devices");
>  MODULE_AUTHOR("Werner Sembach <wse@tuxedocomputers.com>");
> +MODULE_AUTHOR("Aaron Erhardt <aer@tuxedocomputers.com>");
>  MODULE_LICENSE("GPL");
> 

Re: [PATCH v3 5/6] platform/x86/tuxedo: Update and extend documentation

[Aaron Erhardt]

Am 15.09.26 um 15:08 schrieb Ilpo Järvinen:
> On Wed, 26 Aug 2026, Aaron Erhardt wrote:
> 
>> Remove an incorrect comment about the Microsoft MacroPad reference
>> implementation allegedly deviating from the spec and add more
>> information about the module and some other minor improvements.
> 
> Was it "incorrect" or was the spec clarified in a later version? If the 
> latter, that would be worth to mention instead of claiming the original 
> comment was "incorrect".
> 
> This is a honest question, I don't know the answer but I'm kind trying to 
> read in between lines here how we ended up in this situation so my 
> impression could be entirely wrong. ...Thus, please don't assume I know 
> much about the content of these specs (despite me briefly looking into 
> what I could find around this feature was introduced).
> 

The MacroPad reference implementation never deviated from the spec in this
area, at least not in the way the comment suggests. The comment removed here
references another comment, which is removed alongside the code that was
touched in patch 3/6 and assumed that intensities should allow multiple values
to be assigned (e.g. 256 levels), but that was never required in the spec. Even
version 1.4 (the initial driver had 1.5 as a basis) of the spec already
suggests using only two intensities for on and off while everything else is
done through the RGB channels unless the device has some sort of global
brightness control in its hardware.

I think the reason for this misconception might come from an actual bug in the
reference implementation (which is fixed now:
https://github.com/microsoft/RP2040MacropadHidSample/commit/cfc29120a3910c5772976da29ecd57392dfd44d6)
and the natural assumption, that intensity should, similar to the RGB channels,
have 8 bit. Therefore, the driver initially implemented brightness exactly that
way with 256 levels, scaling the RGB intensities with integer arithmetic. But 
since the hardware doesn't scale the brightness and the spec doesn't require
this, there is no good reason to do this.

Re: [PATCH v3 5/6] platform/x86/tuxedo: Update and extend documentation

[Ilpo Järvinen]

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

On Tue, 15 Sep 2026, Aaron Erhardt wrote:

> Am 15.09.26 um 15:08 schrieb Ilpo Järvinen:
> > On Wed, 26 Aug 2026, Aaron Erhardt wrote:
> > 
> >> Remove an incorrect comment about the Microsoft MacroPad reference
> >> implementation allegedly deviating from the spec and add more
> >> information about the module and some other minor improvements.
> > 
> > Was it "incorrect" or was the spec clarified in a later version? If the 
> > latter, that would be worth to mention instead of claiming the original 
> > comment was "incorrect".
> > 
> > This is a honest question, I don't know the answer but I'm kind trying to 
> > read in between lines here how we ended up in this situation so my 
> > impression could be entirely wrong. ...Thus, please don't assume I know 
> > much about the content of these specs (despite me briefly looking into 
> > what I could find around this feature was introduced).
> > 
> 
> The MacroPad reference implementation never deviated from the spec in this
> area, at least not in the way the comment suggests. The comment removed here
> references another comment, which is removed alongside the code that was
> touched in patch 3/6 and assumed that intensities should allow multiple values
> to be assigned (e.g. 256 levels), but that was never required in the spec. Even
> version 1.4 (the initial driver had 1.5 as a basis) of the spec already
> suggests using only two intensities for on and off while everything else is
> done through the RGB channels unless the device has some sort of global
> brightness control in its hardware.
>
> I think the reason for this misconception might come from an actual bug in the
> reference implementation (which is fixed now:
> https://github.com/microsoft/RP2040MacropadHidSample/commit/cfc29120a3910c5772976da29ecd57392dfd44d6)
> and the natural assumption, that intensity should, similar to the RGB channels,
> have 8 bit. Therefore, the driver initially implemented brightness exactly that
> way with 256 levels, scaling the RGB intensities with integer arithmetic. But 
> since the hardware doesn't scale the brightness and the spec doesn't require
> this, there is no good reason to do this.

Okay, thanks for the explanation. No further action required then for 
this patch.

-- 
 i.

[PATCH v3 6/6] MAINTAINERS: Add Aaron Erhardt as maintainer of TUXEDO DRIVERS

[Aaron Erhardt]

I'm actively working on this driver as part of my responsibilities now.

Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a674e36529f7..792eb26722b3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -27551,6 +27551,7 @@ F:	tools/testing/selftests/turbostat/
 
 TUXEDO DRIVERS
 M:	Werner Sembach <wse@tuxedocomputers.com>
+M:	Aaron Erhardt <aer@tuxedocomputers.com>
 L:	platform-driver-x86@vger.kernel.org
 S:	Supported
 F:	drivers/platform/x86/tuxedo/
-- 
2.43.0

Re: [PATCH v3 6/6] MAINTAINERS: Add Aaron Erhardt as maintainer of TUXEDO DRIVERS

[Werner Sembach]

Am 26.08.26 um 10:11 schrieb Aaron Erhardt:
> I'm actively working on this driver as part of my responsibilities now.
>
> Signed-off-by: Aaron Erhardt <aer@tuxedocomputers.com>
Reviewed-by: Werner Sembach <wse@tuxedocomputers.com>
> ---
>   MAINTAINERS | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a674e36529f7..792eb26722b3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -27551,6 +27551,7 @@ F:	tools/testing/selftests/turbostat/
>   
>   TUXEDO DRIVERS
>   M:	Werner Sembach <wse@tuxedocomputers.com>
> +M:	Aaron Erhardt <aer@tuxedocomputers.com>
>   L:	platform-driver-x86@vger.kernel.org
>   S:	Supported
>   F:	drivers/platform/x86/tuxedo/

Re: [PATCH v3 0/6] platform/x86/tuxedo: Fixes for TUXEDO NB04 driver

[Aaron Erhardt]

A friendly bump :)

I'd like to get this series merged because it is necessary for enabling
LampArray functionality on some TUXEDO devices in combination with the recent
patch sent here:

https://lore.kernel.org/linux-input/20260903073602.3815258-1-aer@tuxedocomputers.com/T/#m1b8fdad3bb17ddf6f9139b373ef41c626e52e44f

Am 26.08.26 um 10:11 schrieb Aaron Erhardt:
> This patch series contains several fixes for the TUXEDO NB04 driver.
> Additional userspace tooling and a new WIP generic lamp array driver 
> being tested brought more attention to this driver, thus uncovering 
> some problems and potential for improvements.
> 
> v3: Improve commit message to properly explain changes
> v2: Send all patches as one thread (sorry for the noise)
> 
> Aaron Erhardt (6):
>   platform/x86/tuxedo: Don't use device driver data
>   platform/x86/tuxedo: Set HID report ID on success
>   platform/x86/tuxedo: Use intensity according to HID spec
>   platform/x86/tuxedo: Fix keyboard LED map ordering
>   platform/x86/tuxedo: Update and extend documentation
>   MAINTAINERS: Add Aaron Erhardt as maintainer of TUXEDO DRIVERS
> 
>  MAINTAINERS                               |   1 +
>  drivers/platform/x86/tuxedo/nb04/wmi_ab.c | 122 ++++++++++++----------
>  2 files changed, 70 insertions(+), 53 deletions(-)
>