Re: [PATCH v2 1/3] platform/x86: uniwill-laptop: Split uniwill_kbd_led_init()
From: Werner Sembach
Date: Thu Jul 16 2026 - 12:29:27 EST
Am 16.07.26 um 01:56 schrieb Armin Wolf:
Am 15.07.26 um 17:12 schrieb Werner Sembach:
Am 11.07.26 um 00:35 schrieb Armin Wolf:
The function uniwill_kbd_led_init() is quite large and doing multiple
things at once:
- general hardware initialisation
- single color keyboard backlight registration
- RGB keyboard backlight registration
Move the last two things into separate functions to increase the
maintainability of uniwill_kbd_led_init().
Suggested-by: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>
Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
---
drivers/platform/x86/uniwill/uniwill-acpi.c | 123 +++++++++++---------
1 file changed, 70 insertions(+), 53 deletions(-)
diff --git a/drivers/platform/x86/uniwill/uniwill-acpi.c b/drivers/platform/x86/uniwill/uniwill-acpi.c
index 310445583832..d27f316800f6 100644
--- a/drivers/platform/x86/uniwill/uniwill-acpi.c
+++ b/drivers/platform/x86/uniwill/uniwill-acpi.c
@@ -1733,7 +1733,24 @@ static enum led_brightness uniwill_kbd_led_mc_brightness_get(struct led_classdev
return uniwill_kbd_led_read_brightness(data);
}
-static int uniwill_kbd_led_init(struct uniwill_data *data)
+static int uniwill_white_kbd_led_init(struct uniwill_data *data)
+{
+ struct led_init_data init_data = {
+ .default_label = "white:" LED_FUNCTION_KBD_BACKLIGHT,
+ .devicename = DRIVER_NAME,
+ .devname_mandatory = true,
+ };
+
+ data->kbd_led_cdev.max_brightness = data->kbd_led_max_brightness;
+ data->kbd_led_cdev.color = LED_COLOR_ID_WHITE;
+ data->kbd_led_cdev.flags = LED_BRIGHT_HW_CHANGED | LED_REJECT_NAME_CONFLICT;
+ data->kbd_led_cdev.brightness_set_blocking = uniwill_kbd_led_brightness_set;
+ data->kbd_led_cdev.brightness_get = uniwill_kbd_led_brightness_get;
+
+ return devm_led_classdev_register_ext(data->dev, &data->kbd_led_cdev, &init_data);
+}
+
+static int uniwill_rgb_kbd_led_init(struct uniwill_data *data)
{
unsigned int color_indices[KBD_LED_CHANNELS] = {
LED_COLOR_ID_RED,
@@ -1741,6 +1758,7 @@ static int uniwill_kbd_led_init(struct uniwill_data *data)
LED_COLOR_ID_BLUE,
};
struct led_init_data init_data = {
+ .default_label = "multicolor:" LED_FUNCTION_KBD_BACKLIGHT,
.devicename = DRIVER_NAME,
.devname_mandatory = true,
};
@@ -1749,57 +1767,6 @@ static int uniwill_kbd_led_init(struct uniwill_data *data)
unsigned int regval;
int ret;
- if (!uniwill_device_supports(data, UNIWILL_FEATURE_KEYBOARD_BACKLIGHT))
- return 0;
-
- ret = regmap_read(data->regmap, EC_ADDR_SUPPORT_2, ®val);
- if (ret < 0)
- return ret;
-
- if (!(regval & CHINA_MODE)) {
- ret = regmap_set_bits(data->regmap, EC_ADDR_BIOS_OEM_2, ENABLE_CHINA_MODE);
- if (ret < 0)
- return ret;
- }
-
- ret = regmap_read(data->regmap, EC_ADDR_KBD_STATUS, ®val);
- if (ret < 0)
- return ret;
-
- regval |= KBD_APPLY;
- regval &= ~KBD_POWER_OFF;
- ret = regmap_write(data->regmap, EC_ADDR_KBD_STATUS, regval);
- if (ret < 0)
- return ret;
-
- switch (data->project_id) {
- case PROJECT_ID_PF:
- case PROJECT_ID_PF4MU_PF4MN_PF5MU:
- case PROJECT_ID_PH4TRX1:
- case PROJECT_ID_PH4TUX1:
- case PROJECT_ID_PH4TQX1:
- case PROJECT_ID_PH6TRX1:
- case PROJECT_ID_PH6TQXX:
- case PROJECT_ID_PHXAXXX:
- case PROJECT_ID_PHXPXXX:
- data->single_color_kbd = true;
- break;
- default:
- data->single_color_kbd = regval & KBD_WHITE_ONLY;
- break;
- }
-
- if (data->single_color_kbd) {
- init_data.default_label = "white:" LED_FUNCTION_KBD_BACKLIGHT;
- data->kbd_led_cdev.max_brightness = data->kbd_led_max_brightness;
- data->kbd_led_cdev.color = LED_COLOR_ID_WHITE;
- data->kbd_led_cdev.flags = LED_BRIGHT_HW_CHANGED | LED_REJECT_NAME_CONFLICT;
- data->kbd_led_cdev.brightness_set_blocking = uniwill_kbd_led_brightness_set;
- data->kbd_led_cdev.brightness_get = uniwill_kbd_led_brightness_get;
-
- return devm_led_classdev_register_ext(data->dev, &data->kbd_led_cdev, &init_data);
- }
-
for (int i = 0; i < KBD_LED_CHANNELS; i++) {
data->kbd_led_mc_subled_info[i].color_index = color_indices[i];
@@ -1851,7 +1818,6 @@ static int uniwill_kbd_led_init(struct uniwill_data *data)
if (ret < 0)
return ret;
- init_data.default_label = "multicolor:" LED_FUNCTION_KBD_BACKLIGHT;
data->kbd_led_mc_cdev.led_cdev.max_brightness = data->kbd_led_max_brightness;
data->kbd_led_mc_cdev.led_cdev.color = LED_COLOR_ID_MULTI;
data->kbd_led_mc_cdev.led_cdev.flags = LED_BRIGHT_HW_CHANGED | LED_REJECT_NAME_CONFLICT;
@@ -1864,6 +1830,57 @@ static int uniwill_kbd_led_init(struct uniwill_data *data)
&init_data);
}
+static int uniwill_kbd_led_init(struct uniwill_data *data)
+{
+ unsigned int regval;
+ int ret;
+
+ if (!uniwill_device_supports(data, UNIWILL_FEATURE_KEYBOARD_BACKLIGHT))
+ return 0;
+
+ ret = regmap_read(data->regmap, EC_ADDR_SUPPORT_2, ®val);
+ if (ret < 0)
+ return ret;
+
+ if (!(regval & CHINA_MODE)) {
+ ret = regmap_set_bits(data->regmap, EC_ADDR_BIOS_OEM_2, ENABLE_CHINA_MODE);
+ if (ret < 0)
+ return ret;
+ }
I forgot again what this china mode bit is doing?
But overall:
Reviewed-by: Werner Sembach <wse@xxxxxxxxxxxxxxxxxxx>
The China mode bit _apparently_ is necessary for proper keyboard backlight control on some models,
the original control center set it unconditionally if the associated support bit is set.
ah ok, i searched our code: we actually set the the bit you only check: https://gitlab.com/tuxedocomputers/development/packages/tuxedo-drivers/-/blob/0b2f8c6ab7ef709343ae74b3de51dcae45357641/src/uniwill_leds.h#L296
but if the other way it what you reversed from the windows driver that's probably the proper solution
Thanks,
Armin Wolf
+
+ ret = regmap_read(data->regmap, EC_ADDR_KBD_STATUS, ®val);
+ if (ret < 0)
+ return ret;
+
+ regval |= KBD_APPLY;
+ regval &= ~KBD_POWER_OFF;
+ ret = regmap_write(data->regmap, EC_ADDR_KBD_STATUS, regval);
+ if (ret < 0)
+ return ret;
+
+ switch (data->project_id) {
+ case PROJECT_ID_PF:
+ case PROJECT_ID_PF4MU_PF4MN_PF5MU:
+ case PROJECT_ID_PH4TRX1:
+ case PROJECT_ID_PH4TUX1:
+ case PROJECT_ID_PH4TQX1:
+ case PROJECT_ID_PH6TRX1:
+ case PROJECT_ID_PH6TQXX:
+ case PROJECT_ID_PHXAXXX:
+ case PROJECT_ID_PHXPXXX:
+ data->single_color_kbd = true;
+ break;
+ default:
+ data->single_color_kbd = regval & KBD_WHITE_ONLY;
+ break;
+ }
+
+ if (data->single_color_kbd)
+ return uniwill_white_kbd_led_init(data);
+
+ return uniwill_rgb_kbd_led_init(data);
+}
+
static unsigned int uniwill_sanitize_battery_threshold(unsigned int value)
{
/* 0 means "charging threshold not active" */