[PATCH 0/5] asus-wmi: Add support for RGB keyboards

[PATCH 0/5] asus-wmi: Add support for RGB keyboards

[Luke D. Jones]

This is a patch series to add RGB support for ASUS laptops.
The laptops with this RGB tend to be the TUF series of gamer laptops.

The first step is initial bringup of support using the multicolor LED API.

These types of keyboards implement a slightly more complex interface than
just RGB control however - they also have modes with can be static LED,
blinking, rainbow, color cycles, and more. They also have some custom
animations that can play depending on device state, such as suspended
playing a fancy colour cycle, or playing a "wave" animation.

Two of the patches add support for these features.

The last patch adds documentation in:
Documentation/ABI/testing/sysfs-platform-asus-wmi

Some notes:

- this patch series obsoletes the previous RGB patches by myself

- it is not possible to add attribute groups to multicolor LED as
  they get overwritten by `led_multicolor_groups` in
  `led_classdev_multicolor_register_ext`.

- the methods for RGB control do not provide a way to fetch exisiting
  state, so these methods are WO.

- There is an existing `asus::kbd_backlight`, this provides a 4-step
  brightness to the RGB (off,low,med,high) individually to multicolor.
  I was unsure of the effect of adding a similar path so have used the
  `asus::multicolour::kbd_backlight` name to be clear about purpose.
  If the `asus::kbd_backlight` is off, then no RGB is shown at all.\

I'm hopeful that this patch series addresses all previous feedback related
to the obsoleted patches.

Luke D. Jones (5):
  asus-wmi: Add basic support for TUF laptop keyboard RGB
  asus-wmi: Add support for TUF laptop keyboard RGB mode control
  asus-wmi: Add support for TUF laptop keyboard states
  asus-wmi: Document many of the undocumented API
  asus-wmi: Convert all attr _show to use sysfs_emit

 .../ABI/testing/sysfs-platform-asus-wmi       |  50 ++++
 drivers/platform/x86/asus-wmi.c               | 263 +++++++++++++++++-
 include/linux/platform_data/x86/asus-wmi.h    |   5 +
 3 files changed, 311 insertions(+), 7 deletions(-)

--
2.37.1

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

[Luke D. Jones]

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;
+	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;
+}
+
 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);
+
+		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;
+
+		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

[PATCH 2/5] asus-wmi: Add support for TUF laptop keyboard RGB mode control

[Luke D. Jones]

Adds support for TUF laptop RGB mode control.

Two paths are added:
- /sys/devices/platform/asus-nb-wmi/kernel_rgb_mode
- /sys/devices/platform/asus-nb-wmi/kernel_rgb_mode_index

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c | 86 +++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 33384e3321bb..9e6b83d8dd75 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -118,6 +118,9 @@ static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
 
 static int throttle_thermal_policy_write(struct asus_wmi *);
 
+static int tuf_rgb_brightness_set(struct led_classdev *cdev,
+							enum led_brightness brightness);
+
 static bool ashs_present(void)
 {
 	int i = 0;
@@ -194,6 +197,9 @@ struct fan_curve_data {
 struct keyboard_rgb_led {
 	struct led_classdev_mc dev;
 	struct mc_subled subled_info[3]; /* r g b */
+	u8 save;
+	u8 mode;
+	u8 speed;
 };
 
 struct asus_wmi {
@@ -743,6 +749,72 @@ static ssize_t egpu_enable_store(struct device *dev,
 
 static DEVICE_ATTR_RW(egpu_enable);
 
+/* TUF Laptop Keyboard RGB Modes **********************************************/
+static int keyboard_rgb_mode_check_present(struct asus_wmi *asus)
+{
+	u32 result;
+	int err;
+
+	asus->keyboard_rgb_mode_available = false;
+
+	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_TUF_RGB_MODE, &result);
+	if (err) {
+		if (err == -ENODEV)
+			return 0;
+		return err;
+	}
+
+	if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
+		asus->keyboard_rgb_mode_available = true;
+	}
+
+	return 0;
+}
+
+static ssize_t keyboard_rgb_mode_store(struct device *device,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	u8 save, mode, speed;
+	int err;
+
+	struct asus_wmi *asus = dev_get_drvdata(device);
+	struct led_classdev *cdev = &asus->keyboard_rgb_mode.dev.led_cdev;
+
+	if (sscanf(buf, "%hhd %hhd %hhd", &save, &mode, &speed) != 3)
+		return -EINVAL;
+
+	asus->keyboard_rgb_mode.save = save > 0 ? 1 : 0;
+
+	/* These are the known usable modes across all TUF/ROG */
+	asus->keyboard_rgb_mode.mode = mode < 12 && mode != 9 ? mode : 0x0a;
+
+	if (speed == 0)
+		asus->keyboard_rgb_mode.speed = 0xe1;
+	else if (speed == 1)
+		asus->keyboard_rgb_mode.speed = 0xeb;
+	else if (speed == 2)
+		asus->keyboard_rgb_mode.speed = 0xf5;
+	else
+		asus->keyboard_rgb_mode.speed = 0xeb;
+
+	err = tuf_rgb_brightness_set(cdev, cdev->brightness);
+	if (err)
+		return err;
+	return 0;
+}
+
+static DEVICE_ATTR_WO(keyboard_rgb_mode);
+
+static ssize_t keyboard_rgb_mode_index_show(struct device *device,
+						 struct device_attribute *attr,
+						 char *buf)
+{
+	return sysfs_emit(buf, "%s\n", "save mode speed\n");
+}
+
+static DEVICE_ATTR_RO(keyboard_rgb_mode_index);
+
 /* Battery ********************************************************************/
 
 /* The battery maximum charging percentage */
@@ -1180,6 +1252,9 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
 		 * write these defaults to the device because they will overwrite a
 		 * users last saved boot setting (in NVRAM).
 		 */
+		asus->keyboard_rgb_mode.save = 1;
+		asus->keyboard_rgb_mode.mode = 0;
+		asus->keyboard_rgb_mode.speed = 1;
 		mc_cdev->led_cdev.brightness = brightness;
 		mc_cdev->led_cdev.max_brightness = brightness;
 		mc_led_info[0].intensity = brightness;
@@ -3347,6 +3422,8 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_touchpad.attr,
 	&dev_attr_egpu_enable.attr,
 	&dev_attr_dgpu_disable.attr,
+	&dev_attr_keyboard_rgb_mode.attr,
+	&dev_attr_keyboard_rgb_mode_index.attr,
 	&dev_attr_lid_resume.attr,
 	&dev_attr_als_enable.attr,
 	&dev_attr_fan_boost_mode.attr,
@@ -3377,6 +3454,10 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 		ok = asus->egpu_enable_available;
 	else if (attr == &dev_attr_dgpu_disable.attr)
 		ok = asus->dgpu_disable_available;
+	else if (attr == &dev_attr_keyboard_rgb_mode.attr)
+		ok = asus->keyboard_rgb_mode_available;
+	else if (attr == &dev_attr_keyboard_rgb_mode_index.attr)
+		ok = asus->keyboard_rgb_mode_available;
 	else if (attr == &dev_attr_fan_boost_mode.attr)
 		ok = asus->fan_boost_mode_available;
 	else if (attr == &dev_attr_throttle_thermal_policy.attr)
@@ -3646,6 +3727,10 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_dgpu_disable;
 
+	err = keyboard_rgb_mode_check_present(asus);
+	if (err)
+		goto fail_keyboard_rgb_mode;
+
 	err = fan_boost_mode_check_present(asus);
 	if (err)
 		goto fail_fan_boost_mode;
@@ -3760,6 +3845,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 fail_fan_boost_mode:
 fail_egpu_enable:
 fail_dgpu_disable:
+fail_keyboard_rgb_mode:
 fail_platform:
 fail_panel_od:
 	kfree(asus);
-- 
2.37.1

[PATCH 3/5] asus-wmi: Add support for TUF laptop keyboard states

[Luke D. Jones]

Adds support for the TUF series laptop power states.

Adds two paths:
- /sys/devices/platform/asus-nb-wmi/keyboard_rgb_state
- /sys/devices/platform/asus-nb-wmi/keyboard_rgb_state_index

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c            | 74 ++++++++++++++++++++++
 include/linux/platform_data/x86/asus-wmi.h |  2 +
 2 files changed, 76 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 9e6b83d8dd75..ad758845edc0 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -246,6 +246,7 @@ struct asus_wmi {
 	bool dgpu_disable_available;
 	bool dgpu_disable;
 
+	bool keyboard_rgb_state_available;
 	bool keyboard_rgb_mode_available;
 	struct keyboard_rgb_led keyboard_rgb_mode;
 
@@ -815,6 +816,68 @@ static ssize_t keyboard_rgb_mode_index_show(struct device *device,
 
 static DEVICE_ATTR_RO(keyboard_rgb_mode_index);
 
+/* TUF Laptop Keyboard RGB States *********************************************/
+static int keyboard_rgb_state_check_present(struct asus_wmi *asus)
+{
+	u32 result;
+	int err;
+
+	asus->keyboard_rgb_state_available = false;
+
+	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_TUF_RGB_STATE, &result);
+	if (err) {
+		if (err == -ENODEV)
+			return 0;
+		return err;
+	}
+
+	if (result & ASUS_WMI_DSTS_PRESENCE_BIT)
+		asus->keyboard_rgb_state_available = true;
+
+	return 0;
+}
+
+static ssize_t keyboard_rgb_state_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	u8 flags, save, boot, awake, sleep, keyboard;
+	int err;
+	u32 ret;
+
+	flags = 0;
+	if (sscanf(buf, "%hhd %hhd %hhd %hhd %hhd", &save, &boot, &awake, &sleep, &keyboard) != 5)
+		return -EINVAL;
+
+	save = save == 0 ? 0x0100 : 0x0000;
+	if (boot)
+		flags |= 0x02;
+	if (awake)
+		flags |= 0x08;
+	if (sleep)
+		flags |= 0x20;
+	if (keyboard)
+		flags |= 0x80;
+
+	err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS,
+			ASUS_WMI_DEVID_TUF_RGB_STATE, 0xBD | save | (flags << 16), 0, &ret);
+	if (err)
+		return err;
+
+	return count;
+}
+
+static DEVICE_ATTR_WO(keyboard_rgb_state);
+
+static ssize_t keyboard_rgb_state_index_show(struct device *device,
+						 struct device_attribute *attr,
+						 char *buf)
+{
+	return sysfs_emit(buf, "%s\n", "save boot awake sleep keyboard\n");
+}
+
+static DEVICE_ATTR_RO(keyboard_rgb_state_index);
+
 /* Battery ********************************************************************/
 
 /* The battery maximum charging percentage */
@@ -3424,6 +3487,8 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_dgpu_disable.attr,
 	&dev_attr_keyboard_rgb_mode.attr,
 	&dev_attr_keyboard_rgb_mode_index.attr,
+	&dev_attr_keyboard_rgb_state.attr,
+	&dev_attr_keyboard_rgb_state_index.attr,
 	&dev_attr_lid_resume.attr,
 	&dev_attr_als_enable.attr,
 	&dev_attr_fan_boost_mode.attr,
@@ -3458,6 +3523,10 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 		ok = asus->keyboard_rgb_mode_available;
 	else if (attr == &dev_attr_keyboard_rgb_mode_index.attr)
 		ok = asus->keyboard_rgb_mode_available;
+	else if (attr == &dev_attr_keyboard_rgb_state.attr)
+		ok = asus->keyboard_rgb_state_available;
+	else if (attr == &dev_attr_keyboard_rgb_state_index.attr)
+		ok = asus->keyboard_rgb_state_available;
 	else if (attr == &dev_attr_fan_boost_mode.attr)
 		ok = asus->fan_boost_mode_available;
 	else if (attr == &dev_attr_throttle_thermal_policy.attr)
@@ -3731,6 +3800,10 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_keyboard_rgb_mode;
 
+	err = keyboard_rgb_state_check_present(asus);
+	if (err)
+		goto fail_keyboard_rgb_state;
+
 	err = fan_boost_mode_check_present(asus);
 	if (err)
 		goto fail_fan_boost_mode;
@@ -3846,6 +3919,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 fail_egpu_enable:
 fail_dgpu_disable:
 fail_keyboard_rgb_mode:
+fail_keyboard_rgb_state:
 fail_platform:
 fail_panel_od:
 	kfree(asus);
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index d63c9945a17d..b5c966798ef8 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -100,6 +100,8 @@
 
 /* TUF laptop RGB control */
 #define ASUS_WMI_DEVID_TUF_RGB_MODE	0x00100056
+/* TUF laptop RGB state control */
+#define ASUS_WMI_DEVID_TUF_RGB_STATE	0x00100057
 
 /* DSTS masks */
 #define ASUS_WMI_DSTS_STATUS_BIT	0x00000001
-- 
2.37.1

[PATCH 4/5] asus-wmi: Document many of the undocumented API

[Luke D. Jones]

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 .../ABI/testing/sysfs-platform-asus-wmi       | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 04885738cf15..afcaba6c4bfd 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -57,3 +57,53 @@ Description:
 			* 0 - default,
 			* 1 - overboost,
 			* 2 - silent
+
+What:		/sys/devices/platform/<platform>/dgpu_disable
+Date:		Dec 2022
+KernelVersion:	5.17
+Contact:	"Luke Jones" <luke@ljones.dev>
+Description:
+		Disable discrete GPU:
+			* 0 - Enable dGPU,
+			* 1 - Disable dGPU,
+
+What:		/sys/devices/platform/<platform>/egpu_enable
+Date:		Dec 2022
+KernelVersion:	5.17
+Contact:	"Luke Jones" <luke@ljones.dev>
+Description:
+		Enable the external GPU paired with ROG X-Flow laptops.
+		Toggling this setting will also trigger ACPI to disable the dGPU:
+			* 0 - Disable,
+			* 1 - Enable,
+
+What:		/sys/devices/platform/<platform>/keyboard_rgb_mode
+Date:		Dec 2022
+KernelVersion:	5.20
+Contact:	"Luke Jones" <luke@ljones.dev>
+Description:
+		Set some RGB keyboard modes and features (write-only).
+
+		The accepted input is "save mode speed", where "n n n" options
+		are:
+			* save - 0 or 1, if 0 then settings are not retained on boot
+			* mode - 0 to 12, each is an RGB such as static, rainbow, pulse.
+					Not all keyboards accept every mode.
+			* speed - 0, 1, 2, equal to low, medium, high.
+					Only applies to certain modes.
+
+What:		/sys/devices/platform/<platform>/panel_od
+Date:		Dec 2022
+KernelVersion:	5.20
+Contact:	"Luke Jones" <luke@ljones.dev>
+Description:
+		Set some RGB keyboard power states (write-only).
+
+		The accepted input is "boot awake sleep keyboard", where "n n n n n"
+		options	are:
+			* save - 0 or 1, if 0 then settings are not retained on boot
+			* boot - 0 or 1, controls if a boot animation is shown
+			* awake - 0 or 1, controls if the keyboard LED are on during awake
+			* sleep - 0 or 1, controls if a suspended animation is shown.
+						This is only active if the AC is connected.
+			* keyboard - 0 or 1, unknown what effect this really has
-- 
2.37.1

[PATCH 5/5] asus-wmi: Convert all attr _show to use sysfs_emit

[Luke D. Jones]

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index ad758845edc0..1e1b5226e0b3 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -914,7 +914,7 @@ static ssize_t charge_control_end_threshold_show(struct device *device,
 						 struct device_attribute *attr,
 						 char *buf)
 {
-	return sprintf(buf, "%d\n", charge_end_threshold);
+	return sysfs_emit(buf, "%d\n", charge_end_threshold);
 }
 
 static DEVICE_ATTR_RW(charge_control_end_threshold);
@@ -2021,7 +2021,7 @@ static ssize_t pwm1_show(struct device *dev,
 		value = -1;
 	}
 
-	return sprintf(buf, "%d\n", value);
+	return sysfs_emit(buf, "%d\n", value);
 }
 
 static ssize_t pwm1_store(struct device *dev,
@@ -2081,7 +2081,7 @@ static ssize_t fan1_input_show(struct device *dev,
 		return -ENXIO;
 	}
 
-	return sprintf(buf, "%d\n", value < 0 ? -1 : value*100);
+	return sysfs_emit(buf, "%d\n", value < 0 ? -1 : value*100);
 }
 
 static ssize_t pwm1_enable_show(struct device *dev,
@@ -2099,7 +2099,7 @@ static ssize_t pwm1_enable_show(struct device *dev,
 	 * in practice on X532FL at least (the bit is always 0) and there's
 	 * also nothing in the DSDT to indicate that this behaviour exists.
 	 */
-	return sprintf(buf, "%d\n", asus->fan_pwm_mode);
+	return sysfs_emit(buf, "%d\n", asus->fan_pwm_mode);
 }
 
 static ssize_t pwm1_enable_store(struct device *dev,
@@ -2167,7 +2167,7 @@ static ssize_t fan1_label_show(struct device *dev,
 					  struct device_attribute *attr,
 					  char *buf)
 {
-	return sprintf(buf, "%s\n", ASUS_FAN_DESC);
+	return sysfs_emit(buf, "%s\n", ASUS_FAN_DESC);
 }
 
 static ssize_t asus_hwmon_temp1(struct device *dev,
@@ -2360,7 +2360,7 @@ static ssize_t fan_boost_mode_show(struct device *dev,
 {
 	struct asus_wmi *asus = dev_get_drvdata(dev);
 
-	return scnprintf(buf, PAGE_SIZE, "%d\n", asus->fan_boost_mode);
+	return sysfs_emit(buf, "%d\n", asus->fan_boost_mode);
 }
 
 static ssize_t fan_boost_mode_store(struct device *dev,
@@ -2913,7 +2913,7 @@ static ssize_t throttle_thermal_policy_show(struct device *dev,
 	struct asus_wmi *asus = dev_get_drvdata(dev);
 	u8 mode = asus->throttle_thermal_policy_mode;
 
-	return scnprintf(buf, PAGE_SIZE, "%d\n", mode);
+	return sysfs_emit(buf, "%d\n", mode);
 }
 
 static ssize_t throttle_thermal_policy_store(struct device *dev,
-- 
2.37.1

Re: [PATCH 0/5] asus-wmi: Add support for RGB keyboards

[Pavel Machek]

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

Hi!

> This is a patch series to add RGB support for ASUS laptops.
> The laptops with this RGB tend to be the TUF series of gamer laptops.
> 
> The first step is initial bringup of support using the multicolor LED API.
> 
> These types of keyboards implement a slightly more complex interface than
> just RGB control however - they also have modes with can be static LED,
> blinking, rainbow, color cycles, and more. They also have some custom
> animations that can play depending on device state, such as suspended
> playing a fancy colour cycle, or playing a "wave" animation.
> 
> Two of the patches add support for these features.

Please Cc: LED maintainers with LED patches.

Best regards,
								Pavel

-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 3/5] asus-wmi: Add support for TUF laptop keyboard states

[Pavel Machek]

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

Hi!
> 
> Adds two paths:
> - /sys/devices/platform/asus-nb-wmi/keyboard_rgb_state
> - /sys/devices/platform/asus-nb-wmi/keyboard_rgb_state_index

Patches 2-3 -- we already have pattern trigger. This should use it...

Best regards,
									Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 3/5] asus-wmi: Add support for TUF laptop keyboard states

[Luke Jones]

Hi Pavel,

On Fri, Aug 5 2022 at 14:08:59 +0200, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>> 
>>  Adds two paths:
>>  - /sys/devices/platform/asus-nb-wmi/keyboard_rgb_state
>>  - /sys/devices/platform/asus-nb-wmi/keyboard_rgb_state_index
> 
> Patches 2-3 -- we already have pattern trigger. This should use it...

Can you please provide more information? I'd be happy to use it, but 
because I do this in my free time I don't have the best knowledge of 
helpful stuff that already exists - so far I've been learning about 
stuff like this when someone like yourself brings it up, otherwise I 
try to follow existing code patterns in the source file.

Many thanks,
Luke.

> 
> Best regards,
> 									Pavel
> --
> People of Russia, stop Putin before his war on Ukraine escalates.

Re: [PATCH 0/5] asus-wmi: Add support for RGB keyboards

[Luke Jones]

I'll be sure to do that when I next adjust things. Thanks!

On Fri, Aug 5 2022 at 14:05:39 +0200, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
> 
>>  This is a patch series to add RGB support for ASUS laptops.
>>  The laptops with this RGB tend to be the TUF series of gamer 
>> laptops.
>> 
>>  The first step is initial bringup of support using the multicolor 
>> LED API.
>> 
>>  These types of keyboards implement a slightly more complex 
>> interface than
>>  just RGB control however - they also have modes with can be static 
>> LED,
>>  blinking, rainbow, color cycles, and more. They also have some 
>> custom
>>  animations that can play depending on device state, such as 
>> suspended
>>  playing a fancy colour cycle, or playing a "wave" animation.
>> 
>>  Two of the patches add support for these features.
> 
> Please Cc: LED maintainers with LED patches.
> 
> Best regards,
> 								Pavel
> 
> --
> People of Russia, stop Putin before his war on Ukraine escalates.

Re: [PATCH 4/5] asus-wmi: Document many of the undocumented API

[kernel test robot]

Hi "Luke,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.19]
[cannot apply to linus/master next-20220805]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Luke-D-Jones/asus-wmi-Add-support-for-RGB-keyboards/20220805-162136
base:    3d7cb6b04c3f3115719235cc6866b10326de34cd
reproduce: make htmldocs

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> Documentation/ABI/testing/sysfs-platform-asus-wmi:71: WARNING: Unexpected indentation.

vim +71 Documentation/ABI/testing/sysfs-platform-asus-wmi

  > 71	Date:		Dec 2022
    72	KernelVersion:	5.17
    73	Contact:	"Luke Jones" <luke@ljones.dev>
    74	Description:
    75			Enable the external GPU paired with ROG X-Flow laptops.
    76			Toggling this setting will also trigger ACPI to disable the dGPU:
    77				* 0 - Disable,
    78				* 1 - Enable,
    79	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 0/5] asus-wmi: Add support for RGB keyboards

[Andy Shevchenko]

On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> wrote:
>
> This is a patch series to add RGB support for ASUS laptops.
> The laptops with this RGB tend to be the TUF series of gamer laptops.
>
> The first step is initial bringup of support using the multicolor LED API.
>
> These types of keyboards implement a slightly more complex interface than
> just RGB control however - they also have modes with can be static LED,
> blinking, rainbow, color cycles, and more. They also have some custom
> animations that can play depending on device state, such as suspended
> playing a fancy colour cycle, or playing a "wave" animation.
>
> Two of the patches add support for these features.
>
> The last patch adds documentation in:
> Documentation/ABI/testing/sysfs-platform-asus-wmi
>
> Some notes:
>
> - this patch series obsoletes the previous RGB patches by myself
>
> - it is not possible to add attribute groups to multicolor LED as
>   they get overwritten by `led_multicolor_groups` in
>   `led_classdev_multicolor_register_ext`.
>
> - the methods for RGB control do not provide a way to fetch exisiting
>   state, so these methods are WO.
>
> - There is an existing `asus::kbd_backlight`, this provides a 4-step
>   brightness to the RGB (off,low,med,high) individually to multicolor.
>   I was unsure of the effect of adding a similar path so have used the
>   `asus::multicolour::kbd_backlight` name to be clear about purpose.
>   If the `asus::kbd_backlight` is off, then no RGB is shown at all.\
>
> I'm hopeful that this patch series addresses all previous feedback related
> to the obsoleted patches.

There are so many patches and versioning of all of this is completely
broken. You really have to clean up the mess and realize what version
of this is. To me it looks like this series is v5 or so of the
previously sent patch(es). Also you missed the changelog between
versions so we can see what you have done from vX to vX+1 for the
whole range (1 ... X+1).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/5] asus-wmi: Add support for RGB keyboards

[Luke Jones]

Hi,

On Sat, Aug 6 2022 at 11:10:37 +0200, Andy Shevchenko 
<andy.shevchenko@gmail.com> wrote:
> On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> wrote:
>> 
>>  This is a patch series to add RGB support for ASUS laptops.
>>  The laptops with this RGB tend to be the TUF series of gamer 
>> laptops.
>> 
>>  The first step is initial bringup of support using the multicolor 
>> LED API.
>> 
>>  These types of keyboards implement a slightly more complex 
>> interface than
>>  just RGB control however - they also have modes with can be static 
>> LED,
>>  blinking, rainbow, color cycles, and more. They also have some 
>> custom
>>  animations that can play depending on device state, such as 
>> suspended
>>  playing a fancy colour cycle, or playing a "wave" animation.
>> 
>>  Two of the patches add support for these features.
>> 
>>  The last patch adds documentation in:
>>  Documentation/ABI/testing/sysfs-platform-asus-wmi
>> 
>>  Some notes:
>> 
>>  - this patch series obsoletes the previous RGB patches by myself
>> 
>>  - it is not possible to add attribute groups to multicolor LED as
>>    they get overwritten by `led_multicolor_groups` in
>>    `led_classdev_multicolor_register_ext`.
>> 
>>  - the methods for RGB control do not provide a way to fetch 
>> exisiting
>>    state, so these methods are WO.
>> 
>>  - There is an existing `asus::kbd_backlight`, this provides a 4-step
>>    brightness to the RGB (off,low,med,high) individually to 
>> multicolor.
>>    I was unsure of the effect of adding a similar path so have used 
>> the
>>    `asus::multicolour::kbd_backlight` name to be clear about purpose.
>>    If the `asus::kbd_backlight` is off, then no RGB is shown at all.\
>> 
>>  I'm hopeful that this patch series addresses all previous feedback 
>> related
>>  to the obsoleted patches.
> 
> There are so many patches

This is what Hans requested that I do after the previous submissions,

>  and versioning of all of this is completely
> broken.

I was unsure how to handle this as the previous patches were 
individual, I thought perhaps this patch series is a good place to 
restart since the work done is a bit different.

I will try to better track what I do in future.

> You really have to clean up the mess and realize what version
> of this is. To me it looks like this series is v5 or so of the
> previously sent patch(es). Also you missed the changelog between
> versions so we can see what you have done from vX to vX+1 for the
> whole range (1 ... X+1).

As described before I thought this would hopefully be a good point at 
which to reset due to the changes requested by Hans which meant that 
the underlying structure is different.

I do have another version already prepped, so I will do my best to 
address the previous submissions and your concerns in the cover letter 
along with a changelog.

> 
> --
> With Best Regards,
> Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> wrote:
>
> 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

these are
...or...
essential

> default mode of the keyboard to static. This overwrites the booted
> state of the keyboard.

...

>  #include <linux/leds.h>
> +#include <linux/led-class-multicolor.h>

Not sure about the ordering ('-' vs. 's') in locale C.

...

> +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);

No need to put blank lines in the definition block. Also it would be
better to move the longest line to be first.

> +       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);

What the point in those ' | 0'  additions?

> +       if (err) {
> +               pr_err("Unable to set TUF RGB data?\n");

Why not dev_err() ?

> +               return err;
> +       }
> +       return 0;

return err;

> +}

...

> +       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;

Join this with the definition above. It's fine if it's a bit longer
than 80 characters.

> +               /*
> +                * 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,

With a temporary variable you may make this one line shorter and nicer looking

  struct device *dev = &asus->platform_device->dev;

> +                       3,
> +                       sizeof(*mc_led_info),
> +                       GFP_KERNEL | __GFP_ZERO);
> +
> +               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;
> +
> +               rv = led_classdev_multicolor_register(&asus->platform_device->dev, mc_cdev);

This also becomes shorter.

> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/5] asus-wmi: Add support for RGB keyboards

[Andy Shevchenko]

On Sat, Aug 6, 2022 at 11:33 AM Luke Jones <luke@ljones.dev> wrote:
> On Sat, Aug 6 2022 at 11:10:37 +0200, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> wrote:

> > There are so many patches
>
> This is what Hans requested that I do after the previous submissions,

No, what I'm referring to is that it was so many versions of the same
patch(es) that are floating around and it's too messy to understand
which version is which and what to consider.

> >  and versioning of all of this is completely
> > broken.
>
> I was unsure how to handle this as the previous patches were
> individual, I thought perhaps this patch series is a good place to
> restart since the work done is a bit different.
>
> I will try to better track what I do in future.

Thanks!

> > You really have to clean up the mess and realize what version
> > of this is. To me it looks like this series is v5 or so of the
> > previously sent patch(es). Also you missed the changelog between
> > versions so we can see what you have done from vX to vX+1 for the
> > whole range (1 ... X+1).
>
> As described before I thought this would hopefully be a good point at
> which to reset due to the changes requested by Hans which meant that
> the underlying structure is different.
>
> I do have another version already prepped, so I will do my best to
> address the previous submissions and your concerns in the cover letter
> along with a changelog.

Thanks. For the less confusion continue this versioning than (v2 will
be the next one for the series).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] asus-wmi: Add support for TUF laptop keyboard RGB mode control

[Andy Shevchenko]

On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> wrote:
>
> Adds support for TUF laptop RGB mode control.
>
> Two paths are added:
> - /sys/devices/platform/asus-nb-wmi/kernel_rgb_mode
> - /sys/devices/platform/asus-nb-wmi/kernel_rgb_mode_index

...

> +static int keyboard_rgb_mode_check_present(struct asus_wmi *asus)
> +{
> +       u32 result;
> +       int err;
> +
> +       asus->keyboard_rgb_mode_available = false;
> +
> +       err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_TUF_RGB_MODE, &result);
> +       if (err) {
> +               if (err == -ENODEV)
> +                       return 0;
> +               return err;
> +       }

> +       if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
> +               asus->keyboard_rgb_mode_available = true;
> +       }

{} are not needed (except if they will be utilized in the next patches
in the series).

> +       return 0;
> +}

...

> +       if (sscanf(buf, "%hhd %hhd %hhd", &save, &mode, &speed) != 3)
> +               return -EINVAL;

Usually we have three separate nodes for that, but they are kinda
hidden in one driver, so I don't care much.

...

> +       asus->keyboard_rgb_mode.save = save > 0 ? 1 : 0;

So, it's actually boolean.

You may write it as

    ...save = !!save;

> +       /* These are the known usable modes across all TUF/ROG */
> +       asus->keyboard_rgb_mode.mode = mode < 12 && mode != 9 ? mode : 0x0a;
> +
> +       if (speed == 0)
> +               asus->keyboard_rgb_mode.speed = 0xe1;
> +       else if (speed == 1)
> +               asus->keyboard_rgb_mode.speed = 0xeb;
> +       else if (speed == 2)
> +               asus->keyboard_rgb_mode.speed = 0xf5;

> +       else
> +               asus->keyboard_rgb_mode.speed = 0xeb;

So the 1 is default then, why not use switch-case to show this explicitly?

switch (speed) {
  case 0:
    ...
  break;
  case 1:
  default:
    ...
  break;
  case 2:
    ...
  break;
}

Yes, it's longer, but I think it's cleaner.

> +       err = tuf_rgb_brightness_set(cdev, cdev->brightness);
> +       if (err)
> +               return err;
> +       return 0;

return tuf_rgb_brightness_set(...);

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/5] asus-wmi: Document many of the undocumented API

[Andy Shevchenko]

On Fri, Aug 5, 2022 at 10:21 AM Luke D. Jones <luke@ljones.dev> wrote:
>

Missed commit message.

> Signed-off-by: Luke D. Jones <luke@ljones.dev>

...

> +Date:          Dec 2022

I would be more optimistic here...

> +KernelVersion: 5.17

...and definitely this is the wrong version. I would suggest 6.0.

Both comments are applicable for other similar cases.

In case you are documenting new and old APIs, split this to two
patches with different dates and kernel versions. And commit messages
should be different.
-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/5] asus-wmi: Add support for RGB keyboards

[Andy Shevchenko]

On Sat, Aug 6, 2022 at 11:33 AM Luke Jones <luke@ljones.dev> wrote:
> On Sat, Aug 6 2022 at 11:10:37 +0200, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:

...

> I do have another version already prepped

Hold on and try to address many more review comments. It seems the
series needs much more work, otherwise it will be spam in the mailing
list and demotivating reviewers to continue.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/5] asus-wmi: Convert all attr _show to use sysfs_emit

[Andy Shevchenko]

On Fri, Aug 5, 2022 at 10:21 AM Luke D. Jones <luke@ljones.dev> wrote:

Commit message is missing. It's no go.

I recommend reading [1] to make your contribution to the projects
(even close source, if any) better.

> Signed-off-by: Luke D. Jones <luke@ljones.dev>

[1]: https://cbea.ms/git-commit/

-- 
With Best Regards,
Andy Shevchenko

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

[Luke Jones]

Hi Andy, thanks for the feedback:

On Sat, Aug 6 2022 at 11:44:33 +0200, Andy Shevchenko 
<andy.shevchenko@gmail.com> wrote:
> On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> wrote:
>> 
>>  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
> 
> these are
> ...or...
> essential
> 
>>  default mode of the keyboard to static. This overwrites the booted
>>  state of the keyboard.
> 
> ...
> 
>>   #include <linux/leds.h>
>>  +#include <linux/led-class-multicolor.h>
> 
> Not sure about the ordering ('-' vs. 's') in locale C.
> 

I used hid-playstation.c as a reference and followed that ordering.

> ...
> 
>>  +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);
> 
> No need to put blank lines in the definition block. Also it would be
> better to move the longest line to be first.

Okay cool. Done.

> 
>>  +       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);
> 
> What the point in those ' | 0'  additions?

They were place-holders in testing that I forgot to change in the 
second patch which adds mode configuration :(

Should be "save | (mode << 8) | (r << 16) | (g << 24), (b) | (speed << 
8), &ret);", two bytes.

> 
>>  +       if (err) {
>>  +               pr_err("Unable to set TUF RGB data?\n");
> 
> Why not dev_err() ?

I didn't know about it? Is there an example or doc on its use?

> 
>>  +               return err;
>>  +       }
>>  +       return 0;
> 
> return err;

Something like this then?

if (err) {
	pr_err("Unable to set TUF RGB data?\n");
}
return err;

If so, done.

> 
>>  +}
> 
> ...
> 
>>  +       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;
> 
> Join this with the definition above. It's fine if it's a bit longer
> than 80 characters.

Done.

> 
>>  +               /*
>>  +                * 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,
> 
> With a temporary variable you may make this one line shorter and 
> nicer looking
> 
>   struct device *dev = &asus->platform_device->dev;
> 

Done.

>>  +                       3,
>>  +                       sizeof(*mc_led_info),
>>  +                       GFP_KERNEL | __GFP_ZERO);
>>  +
>>  +               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;
>>  +
>>  +               rv = 
>> led_classdev_multicolor_register(&asus->platform_device->dev, 
>> mc_cdev);
> 
> This also becomes shorter.

Done.

> 
>>  +       }
> 
> --
> With Best Regards,
> Andy Shevchenko

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

[Andy Shevchenko]

On Sat, Aug 6, 2022 at 12:16 PM Luke Jones <luke@ljones.dev> wrote:
> On Sat, Aug 6 2022 at 11:44:33 +0200, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> wrote:

...

> >>   #include <linux/leds.h>
> >>  +#include <linux/led-class-multicolor.h>
> >
> > Not sure about the ordering ('-' vs. 's') in locale C.
>
> I used hid-playstation.c as a reference and followed that ordering.

Try something like this:

  LC_ALL=c sort

for these two lines and see if the ordering is the same.

...

> >>  +       if (err) {
> >>  +               pr_err("Unable to set TUF RGB data?\n");
> >
> > Why not dev_err() ?
>
> I didn't know about it? Is there an example or doc on its use?

Thousands of examples in the kernel source tree. The point is if you
have a device (instance) available, use it for messaging.

> >>  +               return err;
> >>  +       }
> >>  +       return 0;
> >
> > return err;
>
> Something like this then?
>
> if (err) {
>         pr_err("Unable to set TUF RGB data?\n");
> }
> return err;
>
> If so, done.

No parentheses. Have you run checkpatch.pl?

Something like

  if (err)
    dev_err(...);

  return err;

> >>  +}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sat, Aug 6, 2022 at 12:27 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sat, Aug 6, 2022 at 12:16 PM Luke Jones <luke@ljones.dev> wrote:
> > On Sat, Aug 6 2022 at 11:44:33 +0200, Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> wrote:

...

> > >>   #include <linux/leds.h>
> > >>  +#include <linux/led-class-multicolor.h>
> > >
> > > Not sure about the ordering ('-' vs. 's') in locale C.
> >
> > I used hid-playstation.c as a reference and followed that ordering.
>
> Try something like this:
>
>   LC_ALL=c sort

Should be, of course,

  LC_ALL=C sort

> for these two lines and see if the ordering is the same.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] asus-wmi: Add support for TUF laptop keyboard RGB mode control

[Luke Jones]

Hi Andy,

On Sat, Aug 6 2022 at 11:56:58 +0200, Andy Shevchenko 
<andy.shevchenko@gmail.com> wrote:
> On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> wrote:
>> 
>>  Adds support for TUF laptop RGB mode control.
>> 
>>  Two paths are added:
>>  - /sys/devices/platform/asus-nb-wmi/kernel_rgb_mode
>>  - /sys/devices/platform/asus-nb-wmi/kernel_rgb_mode_index
> 
> ...
> 
>>  +static int keyboard_rgb_mode_check_present(struct asus_wmi *asus)
>>  +{
>>  +       u32 result;
>>  +       int err;
>>  +
>>  +       asus->keyboard_rgb_mode_available = false;
>>  +
>>  +       err = asus_wmi_get_devstate(asus, 
>> ASUS_WMI_DEVID_TUF_RGB_MODE, &result);
>>  +       if (err) {
>>  +               if (err == -ENODEV)
>>  +                       return 0;
>>  +               return err;
>>  +       }
> 
>>  +       if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
>>  +               asus->keyboard_rgb_mode_available = true;
>>  +       }
> 
> {} are not needed (except if they will be utilized in the next patches
> in the series).

I've usually been pretty good at catching these. I must not have run 
the patch check script on this one.

Fixed.

> 
>>  +       return 0;
>>  +}
> 
> ...
> 
>>  +       if (sscanf(buf, "%hhd %hhd %hhd", &save, &mode, &speed) != 
>> 3)
>>  +               return -EINVAL;
> 
> Usually we have three separate nodes for that, but they are kinda
> hidden in one driver, so I don't care much.

I don't really understand what you mean sorry.

> 
> ...
> 
>>  +       asus->keyboard_rgb_mode.save = save > 0 ? 1 : 0;
> 
> So, it's actually boolean.
> 
> You may write it as
> 
>     ...save = !!save;

Err okay. Done.

> 
>>  +       /* These are the known usable modes across all TUF/ROG */
>>  +       asus->keyboard_rgb_mode.mode = mode < 12 && mode != 9 ? 
>> mode : 0x0a;
>>  +
>>  +       if (speed == 0)
>>  +               asus->keyboard_rgb_mode.speed = 0xe1;
>>  +       else if (speed == 1)
>>  +               asus->keyboard_rgb_mode.speed = 0xeb;
>>  +       else if (speed == 2)
>>  +               asus->keyboard_rgb_mode.speed = 0xf5;
> 
>>  +       else
>>  +               asus->keyboard_rgb_mode.speed = 0xeb;
> 
> So the 1 is default then, why not use switch-case to show this 
> explicitly?
> 
> switch (speed) {
>   case 0:
>     ...
>   break;
>   case 1:
>   default:
>     ...
>   break;
>   case 2:
>     ...
>   break;
> }
> 
> Yes, it's longer, but I think it's cleaner.

Agreed. Done.

> 
>>  +       err = tuf_rgb_brightness_set(cdev, cdev->brightness);
>>  +       if (err)
>>  +               return err;
>>  +       return 0;
> 
> return tuf_rgb_brightness_set(...);

This causes a hang (waiting for return somewhere?) if I don't return 
count. Especially true if the return is 0.

> 
>>  +}
> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 0/5] asus-wmi: Add support for RGB keyboards

[Luke Jones]

I do agree. It's what I meant by prepped. I think I've addressed 
everything you've raised, so I'll leave it to bake a few days with my 
testing then submit new version on Tuesday (Hans please wait for v2).

Many thanks for your time taken to review this. I'm not an expert at C 
or the kernel by any stretch so reviews are critical for me. Also thank 
you for the link on git messages - I'll go through each patch and 
ensure they're better.

Kind regards,
Luke.

On Sat, Aug 6 2022 at 12:02:19 +0200, Andy Shevchenko 
<andy.shevchenko@gmail.com> wrote:
> On Sat, Aug 6, 2022 at 11:33 AM Luke Jones <luke@ljones.dev> wrote:
>>  On Sat, Aug 6 2022 at 11:10:37 +0200, Andy Shevchenko
>>  <andy.shevchenko@gmail.com> wrote:
> 
> ...
> 
>>  I do have another version already prepped
> 
> Hold on and try to address many more review comments. It seems the
> series needs much more work, otherwise it will be spam in the mailing
> list and demotivating reviewers to continue.
> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 2/5] asus-wmi: Add support for TUF laptop keyboard RGB mode control

[Andy Shevchenko]

On Sat, Aug 6, 2022 at 12:34 PM Luke Jones <luke@ljones.dev> wrote:
> On Sat, Aug 6 2022 at 11:56:58 +0200, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> wrote:

...

> >>  +       if (sscanf(buf, "%hhd %hhd %hhd", &save, &mode, &speed) !=
> >> 3)
> >>  +               return -EINVAL;
> >
> > Usually we have three separate nodes for that, but they are kinda
> > hidden in one driver, so I don't care much.
>
> I don't really understand what you mean sorry.

Each value is in a separate sysfs "file" (we call it "sysfs node"),
but it seems Pavel proposed a better solution so LED framework has
something to offer you.

...

> >>  +       /* These are the known usable modes across all TUF/ROG */
> >>  +       asus->keyboard_rgb_mode.mode = mode < 12 && mode != 9 ?
> >> mode : 0x0a;

This also can be improved

  if (mode >= 12 || mode == 9)
    ...mode = 10;
  else
    ...mode = mode;

Or, if it's important, switch all above to be hexadecimal constants.

...

> >>  +       err = tuf_rgb_brightness_set(cdev, cdev->brightness);
> >>  +       if (err)
> >>  +               return err;
> >>  +       return 0;
> >
> > return tuf_rgb_brightness_set(...);
>
> This causes a hang (waiting for return somewhere?) if I don't return
> count. Especially true if the return is 0.

I didn't get this, because what I suggested is an equivalent to the
above 4 lines.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/5] asus-wmi: Add support for RGB keyboards

[Andy Shevchenko]

On Sat, Aug 6, 2022 at 12:49 PM Luke Jones <luke@ljones.dev> wrote:

Please, do not top-post! It's not appreciated in the OSS community(ies).

> I do agree. It's what I meant by prepped. I think I've addressed
> everything you've raised, so I'll leave it to bake a few days with my
> testing then submit new version on Tuesday (Hans please wait for v2).

We are now at the merge window, it means you have enough time to take
review comments and address them carefully, no need to rush. Your
series at the best can be in v6.0, which is 2 months ahead, which
means we have somewhat 6-7 weeks for you to clean up and make the
series better.

> Many thanks for your time taken to review this. I'm not an expert at C
> or the kernel by any stretch so reviews are critical for me. Also thank
> you for the link on git messages - I'll go through each patch and
> ensure they're better.

You are welcome!

> On Sat, Aug 6 2022 at 12:02:19 +0200, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sat, Aug 6, 2022 at 11:33 AM Luke Jones <luke@ljones.dev> wrote:
> >>  On Sat, Aug 6 2022 at 11:10:37 +0200, Andy Shevchenko
> >>  <andy.shevchenko@gmail.com> wrote:

...

> >>  I do have another version already prepped
> >
> > Hold on and try to address many more review comments. It seems the
> > series needs much more work, otherwise it will be spam in the mailing
> > list and demotivating reviewers to continue.

-- 
With Best Regards,
Andy Shevchenko

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

[Barnabás Pőcze]

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.


> +	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.


> +
>  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`?


> +		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.


> +
> +		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

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

[Luke Jones]

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

Re: [PATCH 3/5] asus-wmi: Add support for TUF laptop keyboard states

[Luke Jones]

Hi Pavel,

I'm sorry but can you direct me to a source file or other that shows 
use of "pattern trigger". I don't know what this means or what to look 
for. From your response it seems I should certainly be using it.

I've finished with all the feedback I've received so far, and this is 
the last piece.

Kind regards,
Luke.

On Fri, Aug 5 2022 at 14:08:59 +0200, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>> 
>>  Adds two paths:
>>  - /sys/devices/platform/asus-nb-wmi/keyboard_rgb_state
>>  - /sys/devices/platform/asus-nb-wmi/keyboard_rgb_state_index
> 
> Patches 2-3 -- we already have pattern trigger. This should use it...
> 
> Best regards,
> 									Pavel
> --
> People of Russia, stop Putin before his war on Ukraine escalates.

Re: [PATCH 3/5] asus-wmi: Add support for TUF laptop keyboard states

[Pavel Machek]

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

Hi!

> I'm sorry but can you direct me to a source file or other that shows use of
> "pattern trigger". I don't know what this means or what to look for. From
> your response it seems I should certainly be using it.

Trigger is at drivers/leds/trigger/ledtrig-pattern.c , you'd want to
do something similar to drivers/leds/rgb/leds-qcom-lpg.c .

> I've finished with all the feedback I've received so far, and this is the
> last piece.

Actually... you may want to submit version without patterns at
first. It will be easier to review.

Best regards,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 3/5] asus-wmi: Add support for TUF laptop keyboard states

[Luke Jones]

On Sun, 2022-08-07 at 14:41 +0200, Pavel Machek wrote:
> Hi!
> 
> > I'm sorry but can you direct me to a source file or other that
> > shows use of
> > "pattern trigger". I don't know what this means or what to look
> > for. From
> > your response it seems I should certainly be using it.
> 
> Trigger is at drivers/leds/trigger/ledtrig-pattern.c , you'd want to
> do something similar to drivers/leds/rgb/leds-qcom-lpg.c .

I think we lost something in communication. This looks like it is all
done in software.

On these laptops, TUF and ROG, the LED effects are all done by the
keyboard EC. For the TUF series you can reference this
https://gitlab.com/asus-linux/reverse-engineering/-/blob/master/TUF-i2c_laptops/led-rgb


It's for this reason I first went with a single node using "n n n n n
n" input.

Kind regards,
Luke.