Re: [PATCH 1/5] asus-wmi: Add basic support for TUF laptop keyboard RGB

[Luke Jones <luke@ljones.dev>]

Hi Barnabás,

Thank you for taking the time to review

On Sat, Aug 6 2022 at 17:30:04 +0000, Barnabás Pőcze 
<pobrn@protonmail.com> wrote:
> Hi
> 
> 
> 2022. augusztus 5., péntek 10:19 keltezéssel, Luke D. Jones 
> <luke@ljones.dev> írta:
> 
>>  Adds support for TUF laptop RGB control via the multicolor LED API.
>> 
>>  As this is the base essentials for adjusting the RGB, it sets the
>>  default mode of the keyboard to static. This overwrites the booted
>>  state of the keyboard.
>> 
>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>  ---
>>   drivers/platform/x86/asus-wmi.c            | 89 
>> ++++++++++++++++++++++
>>   include/linux/platform_data/x86/asus-wmi.h |  3 +
>>   2 files changed, 92 insertions(+)
>> 
>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>> b/drivers/platform/x86/asus-wmi.c
>>  index 0e7fbed8a50d..33384e3321bb 100644
>>  --- a/drivers/platform/x86/asus-wmi.c
>>  +++ b/drivers/platform/x86/asus-wmi.c
>>  @@ -25,6 +25,7 @@
>>   #include <linux/input/sparse-keymap.h>
>>   #include <linux/kernel.h>
>>   #include <linux/leds.h>
>>  +#include <linux/led-class-multicolor.h>
>>   #include <linux/module.h>
>>   #include <linux/pci.h>
>>   #include <linux/pci_hotplug.h>
>>  @@ -190,6 +191,11 @@ struct fan_curve_data {
>>   	u8 percents[FAN_CURVE_POINTS];
>>   };
>> 
>>  +struct keyboard_rgb_led {
>>  +	struct led_classdev_mc dev;
>>  +	struct mc_subled subled_info[3]; /* r g b */
>>  +};
>>  +
>>   struct asus_wmi {
>>   	int dsts_id;
>>   	int spec;
>>  @@ -234,6 +240,9 @@ struct asus_wmi {
>>   	bool dgpu_disable_available;
>>   	bool dgpu_disable;
>> 
>>  +	bool keyboard_rgb_mode_available;
> 
> I think this variable could be introduced in the next patch, it is 
> not used in this one.
> 

Thank you for spotting this. I must have missed it when I split the 
patches.

> 
>>  +	struct keyboard_rgb_led keyboard_rgb_mode;
>>  +
>>   	bool throttle_thermal_policy_available;
>>   	u8 throttle_thermal_policy_mode;
>> 
>>  @@ -1028,6 +1037,35 @@ static enum led_brightness 
>> lightbar_led_get(struct led_classdev *led_cdev)
>>   	return result & ASUS_WMI_DSTS_LIGHTBAR_MASK;
>>   }
>> 
>>  +static int tuf_rgb_brightness_set(struct led_classdev *cdev,
>>  +	enum led_brightness brightness)
>>  +{
>>  +	u8 r, g, b;
>>  +	int err;
>>  +	u32 ret;
>>  +
>>  +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
>>  +
>>  +	led_mc_calc_color_components(mc_cdev, brightness);
>>  +	r = mc_cdev->subled_info[0].brightness;
>>  +	g = mc_cdev->subled_info[1].brightness;
>>  +	b = mc_cdev->subled_info[2].brightness;
>>  +
>>  +	/* Writing out requires some defaults. This will overwrite boot 
>> mode */
>>  +	err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, 
>> ASUS_WMI_DEVID_TUF_RGB_MODE,
>>  +			1 | 0 | (r << 16) | (g << 24), (b) | 0, &ret);
>>  +	if (err) {
>>  +		pr_err("Unable to set TUF RGB data?\n");
>>  +		return err;
>>  +	}
>>  +	return 0;
>>  +}
>>  +
>>  +static enum led_brightness tuf_rgb_brightness_get(struct 
>> led_classdev *cdev)
>>  +{
>>  +	return cdev->brightness;
>>  +}
> 
> If you can't query the brightness from the hardware, I think you can 
> leave
> `led_classdev::brightness_get` to be `NULL`. This callback is only 
> used from
> `led_update_brightness()` as far as I can see.
> 

I did wonder about this. Thanks, I've nulled it.

> 
>>  +
>>   static void asus_wmi_led_exit(struct asus_wmi *asus)
>>   {
>>   	led_classdev_unregister(&asus->kbd_led);
>>  @@ -1105,6 +1143,57 @@ static int asus_wmi_led_init(struct asus_wmi 
>> *asus)
>>   					   &asus->lightbar_led);
>>   	}
>> 
>>  +	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE)) {
>>  +		struct led_classdev_mc *mc_cdev;
>>  +		struct mc_subled *mc_led_info;
>>  +		u8 brightness = 127;
>>  +
>>  +		mc_cdev = &asus->keyboard_rgb_mode.dev;
>>  +
>>  +		/*
>>  +		 * asus::kbd_backlight still controls a base 3-level backlight 
>> and when
>>  +		 * it is on 0, the RGB is not visible at all. RGB should be 
>> treated as
>>  +		 * an additional step.
>>  +		 */
>>  +		mc_cdev->led_cdev.name = "asus::multicolour::kbd_backlight";
>>  +		mc_cdev->led_cdev.flags = LED_CORE_SUSPENDRESUME | 
>> LED_RETAIN_AT_SHUTDOWN;
>>  +		mc_cdev->led_cdev.brightness_set_blocking = 
>> tuf_rgb_brightness_set;
>>  +		mc_cdev->led_cdev.brightness_get = tuf_rgb_brightness_get;
>>  +
>>  +		/* Let the multicolour LED own the info */
>>  +		mc_led_info = devm_kmalloc_array(
>>  +			&asus->platform_device->dev,
>>  +			3,
>>  +			sizeof(*mc_led_info),
>>  +			GFP_KERNEL | __GFP_ZERO);
>>  +
> 
> I am a bit confused as to why dynamic allocation is needed here. 
> Haven't you
> already "allocated" the storage in `keyboard_rgb_led::subled_info`?

Honestly, I write rust 90% of the time which is quite clear on these 
things, so here I wasn't very sure about what to do - I read the 
hid-playstation.c and it had something similar so I used it and 
completely forgot about the struct allocation.

I've updated to `struct mc_subled *mc_led_info = 
asus->keyboard_rgb_mode.subled_info;` and removed the dynamic alloc.

> 
> 
>>  +		if (!mc_led_info)
>>  +			return -ENOMEM;
>>  +
>>  +		mc_led_info[0].color_index = LED_COLOR_ID_RED;
>>  +		mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
>>  +		mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
>>  +
>>  +		/*
>>  +		 * It's not possible to get last set data from device so set 
>> defaults
>>  +		 * to make it safe for a user to change either RGB or modes. We 
>> don't
>>  +		 * write these defaults to the device because they will 
>> overwrite a
>>  +		 * users last saved boot setting (in NVRAM).
>>  +		 */
>>  +		mc_cdev->led_cdev.brightness = brightness;
>>  +		mc_cdev->led_cdev.max_brightness = brightness;
>>  +		mc_led_info[0].intensity = brightness;
>>  +		mc_led_info[0].brightness = mc_cdev->led_cdev.brightness;
>>  +		mc_led_info[1].brightness = mc_cdev->led_cdev.brightness;
>>  +		mc_led_info[2].brightness = mc_cdev->led_cdev.brightness;
>>  +		led_mc_calc_color_components(mc_cdev, brightness);
>>  +
>>  +		mc_cdev->subled_info = mc_led_info;
>>  +		mc_cdev->num_colors = 3;
> 
> `led_mc_calc_color_components()` uses `led_classdev_mc::num_colors`, 
> so I think
> it needs to be set before calling it. But that function sets the 
> subled brightness
> based on the intensity, so it will overwrite the brightness values 
> that have just
> been set.

Ah thanks, I don't think I understood that before. I've removed 
mc_led_info[i].brightness setting now.

> 
> 
>>  +
>>  +		rv = 
>> led_classdev_multicolor_register(&asus->platform_device->dev, 
>> mc_cdev);
>>  +	}
>>  +
>>   error:
>>   	if (rv)
>>   		asus_wmi_led_exit(asus);
>>  diff --git a/include/linux/platform_data/x86/asus-wmi.h 
>> b/include/linux/platform_data/x86/asus-wmi.h
>>  index a571b47ff362..d63c9945a17d 100644
>>  --- a/include/linux/platform_data/x86/asus-wmi.h
>>  +++ b/include/linux/platform_data/x86/asus-wmi.h
>>  @@ -98,6 +98,9 @@
>>   /* dgpu on/off */
>>   #define ASUS_WMI_DEVID_DGPU		0x00090020
>> 
>>  +/* TUF laptop RGB control */
>>  +#define ASUS_WMI_DEVID_TUF_RGB_MODE	0x00100056
>>  +
>>   /* DSTS masks */
>>   #define ASUS_WMI_DSTS_STATUS_BIT	0x00000001
>>   #define ASUS_WMI_DSTS_UNKNOWN_BIT	0x00000002
>>  --
>>  2.37.1
>> 
>> 
> 
> 
> Regards,
> Barnabás Pőcze