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

[Hans de Goede <hdegoede@redhat.com>]

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)