[PATCH v2] platform/x86: system76: Handle new KBLED ACPI methods

[PATCH v2] platform/x86: system76: Handle new KBLED ACPI methods

[Tim Crawford]

System76 EC since system76/ec@9ac513128ad9 detects if the keyboard is
white or RGB backlit via `RGBKB-DET#` at run-time instead of being set
at compile-time. As part of this, the brightness of white-only backlit
keyboards was also changed to behave more like the RGB-backlit
keyboards: a value between 0 and 255 instead of a firmware-defined
level.

The EC ACPI methods in coreboot have been updated for this new
functionality only, removing the old behavior.

This should preserve behavior as we roll out new firmware with these
changes included and users update to it.

Ref: https://github.com/system76/ec/pull/357
Ref: https://review.coreboot.org/c/coreboot/+/76152
Signed-off-by: Tim Crawford <tcrawford@system76.com>
---
 drivers/platform/x86/system76_acpi.c | 74 ++++++++++++++++++++++------
 1 file changed, 59 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
index fc4708fa6ebe..3da753b3d00d 100644
--- a/drivers/platform/x86/system76_acpi.c
+++ b/drivers/platform/x86/system76_acpi.c
@@ -2,7 +2,7 @@
 /*
  * System76 ACPI Driver
  *
- * Copyright (C) 2019 System76
+ * Copyright (C) 2023 System76
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -24,6 +24,12 @@
 
 #include <acpi/battery.h>
 
+enum kbled_type {
+	KBLED_NONE,
+	KBLED_WHITE,
+	KBLED_RGB,
+};
+
 struct system76_data {
 	struct acpi_device *acpi_dev;
 	struct led_classdev ap_led;
@@ -36,6 +42,7 @@ struct system76_data {
 	union acpi_object *ntmp;
 	struct input_dev *input;
 	bool has_open_ec;
+	enum kbled_type kbled_type;
 };
 
 static const struct acpi_device_id device_ids[] = {
@@ -327,7 +334,11 @@ static int kb_led_set(struct led_classdev *led, enum led_brightness value)
 
 	data = container_of(led, struct system76_data, kb_led);
 	data->kb_brightness = value;
-	return system76_set(data, "SKBL", (int)data->kb_brightness);
+	if (acpi_has_method(acpi_device_handle(data->acpi_dev), "GKBK")) {
+		return system76_set(data, "SKBB", (int)data->kb_brightness);
+	} else {
+		return system76_set(data, "SKBL", (int)data->kb_brightness);
+	}
 }
 
 // Get the last set keyboard LED color
@@ -399,7 +410,12 @@ static void kb_led_hotkey_hardware(struct system76_data *data)
 {
 	int value;
 
-	value = system76_get(data, "GKBL");
+	if (acpi_has_method(acpi_device_handle(data->acpi_dev), "GKBK")) {
+		value = system76_get(data, "GKBB");
+	} else {
+		value = system76_get(data, "GKBL");
+	}
+
 	if (value < 0)
 		return;
 	data->kb_brightness = value;
@@ -459,8 +475,9 @@ static void kb_led_hotkey_color(struct system76_data *data)
 {
 	int i;
 
-	if (data->kb_color < 0)
+	if (data->kbled_type != KBLED_RGB)
 		return;
+
 	if (data->kb_brightness > 0) {
 		for (i = 0; i < ARRAY_SIZE(kb_colors); i++) {
 			if (kb_colors[i] == data->kb_color)
@@ -687,19 +704,46 @@ static int system76_add(struct acpi_device *acpi_dev)
 	data->kb_led.flags = LED_BRIGHT_HW_CHANGED | LED_CORE_SUSPENDRESUME;
 	data->kb_led.brightness_get = kb_led_get;
 	data->kb_led.brightness_set_blocking = kb_led_set;
-	if (acpi_has_method(acpi_device_handle(data->acpi_dev), "SKBC")) {
-		data->kb_led.max_brightness = 255;
-		data->kb_led.groups = system76_kb_led_color_groups;
-		data->kb_toggle_brightness = 72;
-		data->kb_color = 0xffffff;
-		system76_set(data, "SKBC", data->kb_color);
+	if (acpi_has_method(acpi_device_handle(data->acpi_dev), "GKBK")) {
+		// Use the new ACPI methods
+		data->kbled_type = system76_get(data, "GKBK");
+
+		switch (data->kbled_type) {
+		case KBLED_NONE:
+			// Nothing to do: Device will not be registered.
+			break;
+		case KBLED_WHITE:
+			data->kb_led.max_brightness = 255;
+			data->kb_toggle_brightness = 72;
+			break;
+		case KBLED_RGB:
+			data->kb_led.max_brightness = 255;
+			data->kb_led.groups = system76_kb_led_color_groups;
+			data->kb_toggle_brightness = 72;
+			data->kb_color = 0xffffff;
+			system76_set(data, "SKBC", data->kb_color);
+			break;
+		}
 	} else {
-		data->kb_led.max_brightness = 5;
-		data->kb_color = -1;
+		// Use the old ACPI methods
+		if (acpi_has_method(acpi_device_handle(data->acpi_dev), "SKBC")) {
+			data->kbled_type = KBLED_RGB;
+			data->kb_led.max_brightness = 255;
+			data->kb_led.groups = system76_kb_led_color_groups;
+			data->kb_toggle_brightness = 72;
+			data->kb_color = 0xffffff;
+			system76_set(data, "SKBC", data->kb_color);
+		} else {
+			data->kbled_type = KBLED_WHITE;
+			data->kb_led.max_brightness = 5;
+		}
+	}
+
+	if (data->kbled_type != KBLED_NONE) {
+		err = devm_led_classdev_register(&acpi_dev->dev, &data->kb_led);
+		if (err)
+			return err;
 	}
-	err = devm_led_classdev_register(&acpi_dev->dev, &data->kb_led);
-	if (err)
-		return err;
 
 	data->input = devm_input_allocate_device(&acpi_dev->dev);
 	if (!data->input)
-- 
2.41.0

Re: [PATCH v2] platform/x86: system76: Handle new KBLED ACPI methods

[Hans de Goede]

Hi,

On 7/19/23 20:13, Tim Crawford wrote:
> System76 EC since system76/ec@9ac513128ad9 detects if the keyboard is
> white or RGB backlit via `RGBKB-DET#` at run-time instead of being set
> at compile-time. As part of this, the brightness of white-only backlit
> keyboards was also changed to behave more like the RGB-backlit
> keyboards: a value between 0 and 255 instead of a firmware-defined
> level.
> 
> The EC ACPI methods in coreboot have been updated for this new
> functionality only, removing the old behavior.
> 
> This should preserve behavior as we roll out new firmware with these
> changes included and users update to it.
> 
> Ref: https://github.com/system76/ec/pull/357
> Ref: https://review.coreboot.org/c/coreboot/+/76152
> Signed-off-by: Tim Crawford <tcrawford@system76.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/platform/x86/system76_acpi.c | 74 ++++++++++++++++++++++------
>  1 file changed, 59 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
> index fc4708fa6ebe..3da753b3d00d 100644
> --- a/drivers/platform/x86/system76_acpi.c
> +++ b/drivers/platform/x86/system76_acpi.c
> @@ -2,7 +2,7 @@
>  /*
>   * System76 ACPI Driver
>   *
> - * Copyright (C) 2019 System76
> + * Copyright (C) 2023 System76
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
> @@ -24,6 +24,12 @@
>  
>  #include <acpi/battery.h>
>  
> +enum kbled_type {
> +	KBLED_NONE,
> +	KBLED_WHITE,
> +	KBLED_RGB,
> +};
> +
>  struct system76_data {
>  	struct acpi_device *acpi_dev;
>  	struct led_classdev ap_led;
> @@ -36,6 +42,7 @@ struct system76_data {
>  	union acpi_object *ntmp;
>  	struct input_dev *input;
>  	bool has_open_ec;
> +	enum kbled_type kbled_type;
>  };
>  
>  static const struct acpi_device_id device_ids[] = {
> @@ -327,7 +334,11 @@ static int kb_led_set(struct led_classdev *led, enum led_brightness value)
>  
>  	data = container_of(led, struct system76_data, kb_led);
>  	data->kb_brightness = value;
> -	return system76_set(data, "SKBL", (int)data->kb_brightness);
> +	if (acpi_has_method(acpi_device_handle(data->acpi_dev), "GKBK")) {
> +		return system76_set(data, "SKBB", (int)data->kb_brightness);
> +	} else {
> +		return system76_set(data, "SKBL", (int)data->kb_brightness);
> +	}
>  }
>  
>  // Get the last set keyboard LED color
> @@ -399,7 +410,12 @@ static void kb_led_hotkey_hardware(struct system76_data *data)
>  {
>  	int value;
>  
> -	value = system76_get(data, "GKBL");
> +	if (acpi_has_method(acpi_device_handle(data->acpi_dev), "GKBK")) {
> +		value = system76_get(data, "GKBB");
> +	} else {
> +		value = system76_get(data, "GKBL");
> +	}
> +
>  	if (value < 0)
>  		return;
>  	data->kb_brightness = value;
> @@ -459,8 +475,9 @@ static void kb_led_hotkey_color(struct system76_data *data)
>  {
>  	int i;
>  
> -	if (data->kb_color < 0)
> +	if (data->kbled_type != KBLED_RGB)
>  		return;
> +
>  	if (data->kb_brightness > 0) {
>  		for (i = 0; i < ARRAY_SIZE(kb_colors); i++) {
>  			if (kb_colors[i] == data->kb_color)
> @@ -687,19 +704,46 @@ static int system76_add(struct acpi_device *acpi_dev)
>  	data->kb_led.flags = LED_BRIGHT_HW_CHANGED | LED_CORE_SUSPENDRESUME;
>  	data->kb_led.brightness_get = kb_led_get;
>  	data->kb_led.brightness_set_blocking = kb_led_set;
> -	if (acpi_has_method(acpi_device_handle(data->acpi_dev), "SKBC")) {
> -		data->kb_led.max_brightness = 255;
> -		data->kb_led.groups = system76_kb_led_color_groups;
> -		data->kb_toggle_brightness = 72;
> -		data->kb_color = 0xffffff;
> -		system76_set(data, "SKBC", data->kb_color);
> +	if (acpi_has_method(acpi_device_handle(data->acpi_dev), "GKBK")) {
> +		// Use the new ACPI methods
> +		data->kbled_type = system76_get(data, "GKBK");
> +
> +		switch (data->kbled_type) {
> +		case KBLED_NONE:
> +			// Nothing to do: Device will not be registered.
> +			break;
> +		case KBLED_WHITE:
> +			data->kb_led.max_brightness = 255;
> +			data->kb_toggle_brightness = 72;
> +			break;
> +		case KBLED_RGB:
> +			data->kb_led.max_brightness = 255;
> +			data->kb_led.groups = system76_kb_led_color_groups;
> +			data->kb_toggle_brightness = 72;
> +			data->kb_color = 0xffffff;
> +			system76_set(data, "SKBC", data->kb_color);
> +			break;
> +		}
>  	} else {
> -		data->kb_led.max_brightness = 5;
> -		data->kb_color = -1;
> +		// Use the old ACPI methods
> +		if (acpi_has_method(acpi_device_handle(data->acpi_dev), "SKBC")) {
> +			data->kbled_type = KBLED_RGB;
> +			data->kb_led.max_brightness = 255;
> +			data->kb_led.groups = system76_kb_led_color_groups;
> +			data->kb_toggle_brightness = 72;
> +			data->kb_color = 0xffffff;
> +			system76_set(data, "SKBC", data->kb_color);
> +		} else {
> +			data->kbled_type = KBLED_WHITE;
> +			data->kb_led.max_brightness = 5;
> +		}
> +	}
> +
> +	if (data->kbled_type != KBLED_NONE) {
> +		err = devm_led_classdev_register(&acpi_dev->dev, &data->kb_led);
> +		if (err)
> +			return err;
>  	}
> -	err = devm_led_classdev_register(&acpi_dev->dev, &data->kb_led);
> -	if (err)
> -		return err;
>  
>  	data->input = devm_input_allocate_device(&acpi_dev->dev);
>  	if (!data->input)

Re: [PATCH v2] platform/x86: system76: Handle new KBLED ACPI methods

[Hans de Goede]

Hi,

On 7/19/23 20:13, Tim Crawford wrote:
> System76 EC since system76/ec@9ac513128ad9 detects if the keyboard is
> white or RGB backlit via `RGBKB-DET#` at run-time instead of being set
> at compile-time. As part of this, the brightness of white-only backlit
> keyboards was also changed to behave more like the RGB-backlit
> keyboards: a value between 0 and 255 instead of a firmware-defined
> level.
> 
> The EC ACPI methods in coreboot have been updated for this new
> functionality only, removing the old behavior.
> 
> This should preserve behavior as we roll out new firmware with these
> changes included and users update to it.
> 
> Ref: https://github.com/system76/ec/pull/357
> Ref: https://review.coreboot.org/c/coreboot/+/76152
> Signed-off-by: Tim Crawford <tcrawford@system76.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans




> ---
>  drivers/platform/x86/system76_acpi.c | 74 ++++++++++++++++++++++------
>  1 file changed, 59 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
> index fc4708fa6ebe..3da753b3d00d 100644
> --- a/drivers/platform/x86/system76_acpi.c
> +++ b/drivers/platform/x86/system76_acpi.c
> @@ -2,7 +2,7 @@
>  /*
>   * System76 ACPI Driver
>   *
> - * Copyright (C) 2019 System76
> + * Copyright (C) 2023 System76
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
> @@ -24,6 +24,12 @@
>  
>  #include <acpi/battery.h>
>  
> +enum kbled_type {
> +	KBLED_NONE,
> +	KBLED_WHITE,
> +	KBLED_RGB,
> +};
> +
>  struct system76_data {
>  	struct acpi_device *acpi_dev;
>  	struct led_classdev ap_led;
> @@ -36,6 +42,7 @@ struct system76_data {
>  	union acpi_object *ntmp;
>  	struct input_dev *input;
>  	bool has_open_ec;
> +	enum kbled_type kbled_type;
>  };
>  
>  static const struct acpi_device_id device_ids[] = {
> @@ -327,7 +334,11 @@ static int kb_led_set(struct led_classdev *led, enum led_brightness value)
>  
>  	data = container_of(led, struct system76_data, kb_led);
>  	data->kb_brightness = value;
> -	return system76_set(data, "SKBL", (int)data->kb_brightness);
> +	if (acpi_has_method(acpi_device_handle(data->acpi_dev), "GKBK")) {
> +		return system76_set(data, "SKBB", (int)data->kb_brightness);
> +	} else {
> +		return system76_set(data, "SKBL", (int)data->kb_brightness);
> +	}
>  }
>  
>  // Get the last set keyboard LED color
> @@ -399,7 +410,12 @@ static void kb_led_hotkey_hardware(struct system76_data *data)
>  {
>  	int value;
>  
> -	value = system76_get(data, "GKBL");
> +	if (acpi_has_method(acpi_device_handle(data->acpi_dev), "GKBK")) {
> +		value = system76_get(data, "GKBB");
> +	} else {
> +		value = system76_get(data, "GKBL");
> +	}
> +
>  	if (value < 0)
>  		return;
>  	data->kb_brightness = value;
> @@ -459,8 +475,9 @@ static void kb_led_hotkey_color(struct system76_data *data)
>  {
>  	int i;
>  
> -	if (data->kb_color < 0)
> +	if (data->kbled_type != KBLED_RGB)
>  		return;
> +
>  	if (data->kb_brightness > 0) {
>  		for (i = 0; i < ARRAY_SIZE(kb_colors); i++) {
>  			if (kb_colors[i] == data->kb_color)
> @@ -687,19 +704,46 @@ static int system76_add(struct acpi_device *acpi_dev)
>  	data->kb_led.flags = LED_BRIGHT_HW_CHANGED | LED_CORE_SUSPENDRESUME;
>  	data->kb_led.brightness_get = kb_led_get;
>  	data->kb_led.brightness_set_blocking = kb_led_set;
> -	if (acpi_has_method(acpi_device_handle(data->acpi_dev), "SKBC")) {
> -		data->kb_led.max_brightness = 255;
> -		data->kb_led.groups = system76_kb_led_color_groups;
> -		data->kb_toggle_brightness = 72;
> -		data->kb_color = 0xffffff;
> -		system76_set(data, "SKBC", data->kb_color);
> +	if (acpi_has_method(acpi_device_handle(data->acpi_dev), "GKBK")) {
> +		// Use the new ACPI methods
> +		data->kbled_type = system76_get(data, "GKBK");
> +
> +		switch (data->kbled_type) {
> +		case KBLED_NONE:
> +			// Nothing to do: Device will not be registered.
> +			break;
> +		case KBLED_WHITE:
> +			data->kb_led.max_brightness = 255;
> +			data->kb_toggle_brightness = 72;
> +			break;
> +		case KBLED_RGB:
> +			data->kb_led.max_brightness = 255;
> +			data->kb_led.groups = system76_kb_led_color_groups;
> +			data->kb_toggle_brightness = 72;
> +			data->kb_color = 0xffffff;
> +			system76_set(data, "SKBC", data->kb_color);
> +			break;
> +		}
>  	} else {
> -		data->kb_led.max_brightness = 5;
> -		data->kb_color = -1;
> +		// Use the old ACPI methods
> +		if (acpi_has_method(acpi_device_handle(data->acpi_dev), "SKBC")) {
> +			data->kbled_type = KBLED_RGB;
> +			data->kb_led.max_brightness = 255;
> +			data->kb_led.groups = system76_kb_led_color_groups;
> +			data->kb_toggle_brightness = 72;
> +			data->kb_color = 0xffffff;
> +			system76_set(data, "SKBC", data->kb_color);
> +		} else {
> +			data->kbled_type = KBLED_WHITE;
> +			data->kb_led.max_brightness = 5;
> +		}
> +	}
> +
> +	if (data->kbled_type != KBLED_NONE) {
> +		err = devm_led_classdev_register(&acpi_dev->dev, &data->kb_led);
> +		if (err)
> +			return err;
>  	}
> -	err = devm_led_classdev_register(&acpi_dev->dev, &data->kb_led);
> -	if (err)
> -		return err;
>  
>  	data->input = devm_input_allocate_device(&acpi_dev->dev);
>  	if (!data->input)