[PATCH 0/2] asus-wmi: Keyboard rgb led multicolor support

[PATCH 0/2] asus-wmi: Keyboard rgb led multicolor support

[Abhijeet V]

Hi

This patch series adds support for rgb keyboards on certain ASUS laptops
by utilizing the multicolor led device class. Much of the wmi
interacting code was obtained from the faustus kernel module:
https://github.com/hackbnw/faustus

The driver is hardcoded to use only the static mode provided by the
platform. There are other modes that are supported like strobe, and
rainbow and also variable speeds. However, I'm not sure how to integrate
that into the led classdev. Support for them could be added in a
separate patch later.

This is my first contribution (other than reporting a regression to
asus-wmi recently). I've tried my best to stick to the kernel's code
standards. I apologize if I've missed any obviously glaring issues.

Thanks
Abhijeet

[PATCH 1/2] asus-wmi: Use led multicolor class for keyboard backlight

[Abhijeet V]

Use the led multicolor class for keyboard backlight so that support for
rgb keyboard leds can be added for supported Asus laptops.

Also refactored the keyboard led functions. The function names are now
indicative of what the function does.

Signed-off-by: Abhijeet V <abhijeetviswa@gmail.com>
---
 drivers/platform/x86/asus-wmi.c | 109 ++++++++++++++++++--------------
 1 file changed, 63 insertions(+), 46 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 2104a2621e50..117fbcb303d3 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>
@@ -88,6 +89,8 @@ module_param(fnlock_default, bool, 0444);
 #define ASUS_FAN_BOOST_MODE_SILENT_MASK		0x02
 #define ASUS_FAN_BOOST_MODES_MASK		0x03
 
+#define ASUS_KBD_SUBLED_COUNT			3
+
 #define ASUS_THROTTLE_THERMAL_POLICY_DEFAULT	0
 #define ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST	1
 #define ASUS_THROTTLE_THERMAL_POLICY_SILENT	2
@@ -204,8 +207,6 @@ struct asus_wmi {
 	int wlan_led_wk;
 	struct led_classdev tpd_led;
 	int tpd_led_wk;
-	struct led_classdev kbd_led;
-	int kbd_led_wk;
 	struct led_classdev lightbar_led;
 	int lightbar_led_wk;
 	struct workqueue_struct *led_workqueue;
@@ -213,6 +214,10 @@ struct asus_wmi {
 	struct work_struct wlan_led_work;
 	struct work_struct lightbar_led_work;
 
+	struct led_classdev_mc kbd_led_mc;
+	int kbd_led_wk;
+	struct mc_subled subled_info[ASUS_KBD_SUBLED_COUNT];
+
 	struct asus_rfkill wlan;
 	struct asus_rfkill bluetooth;
 	struct asus_rfkill wimax;
@@ -870,15 +875,7 @@ static enum led_brightness tpd_led_get(struct led_classdev *led_cdev)
 	return read_tpd_led_state(asus);
 }
 
-static void kbd_led_update(struct asus_wmi *asus)
-{
-	int ctrl_param = 0;
-
-	ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
-	asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
-}
-
-static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
+static int kbd_led_brightness_wmi_read(struct asus_wmi *asus, int *level, int *env)
 {
 	int retval;
 
@@ -905,50 +902,77 @@ static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
 	return 0;
 }
 
-static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
+static void kbd_led_brightness_wmi_write(struct asus_wmi *asus, int value)
 {
-	struct asus_wmi *asus;
 	int max_level;
+	int ctrl_param = 0;
 
-	asus = container_of(led_cdev, struct asus_wmi, kbd_led);
-	max_level = asus->kbd_led.max_brightness;
-
+	max_level = asus->kbd_led_mc.led_cdev.max_brightness;
 	asus->kbd_led_wk = clamp_val(value, 0, max_level);
-	kbd_led_update(asus);
+
+	ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
+	asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
 }
 
-static void kbd_led_set(struct led_classdev *led_cdev,
-			enum led_brightness value)
+static void kbd_led_brightness_set(struct led_classdev *led_cdev,
+		enum led_brightness value)
 {
+	struct asus_wmi *asus;
+	struct led_classdev_mc *led_cdev_mc;
+
 	/* Prevent disabling keyboard backlight on module unregister */
 	if (led_cdev->flags & LED_UNREGISTERING)
 		return;
 
-	do_kbd_led_set(led_cdev, value);
+	led_cdev_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
+	asus = container_of(led_cdev_mc, struct asus_wmi, kbd_led_mc);
+
+	kbd_led_brightness_wmi_write(asus, value);
 }
 
-static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
+static void kbd_led_set_brightness_by_hw(struct asus_wmi *asus,
+		enum led_brightness value)
 {
-	struct led_classdev *led_cdev = &asus->kbd_led;
+	struct led_classdev *led_cdev = &asus->kbd_led_mc.led_cdev;
 
-	do_kbd_led_set(led_cdev, value);
+	kbd_led_brightness_wmi_write(asus, value);
 	led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
 }
 
-static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
+static enum led_brightness kbd_led_brightness_get(struct led_classdev *led_cdev)
 {
 	struct asus_wmi *asus;
+	struct led_classdev_mc *led_cdev_mc;
 	int retval, value;
 
-	asus = container_of(led_cdev, struct asus_wmi, kbd_led);
+	led_cdev_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
+	asus = container_of(led_cdev_mc, struct asus_wmi, kbd_led_mc);
 
-	retval = kbd_led_read(asus, &value, NULL);
+	retval = kbd_led_brightness_wmi_read(asus, &value, NULL);
 	if (retval < 0)
 		return retval;
 
 	return value;
 }
 
+int kbd_led_classdev_init(struct asus_wmi *asus, int brightness)
+{
+	int rv;
+
+	asus->kbd_led_wk = brightness;
+	asus->kbd_led_mc.led_cdev.name = "asus::kbd_backlight";
+	asus->kbd_led_mc.led_cdev.flags = LED_BRIGHT_HW_CHANGED;
+	asus->kbd_led_mc.led_cdev.brightness_set = kbd_led_brightness_set;
+	asus->kbd_led_mc.led_cdev.brightness_get = kbd_led_brightness_get;
+	asus->kbd_led_mc.led_cdev.max_brightness = 3;
+
+	asus->kbd_led_mc.num_colors = ASUS_KBD_SUBLED_COUNT;
+
+	rv = led_classdev_multicolor_register(&asus->platform_device->dev,
+					&asus->kbd_led_mc);
+	return rv;
+}
+
 static int wlan_led_unknown_state(struct asus_wmi *asus)
 {
 	u32 result;
@@ -1026,7 +1050,7 @@ static enum led_brightness lightbar_led_get(struct led_classdev *led_cdev)
 
 static void asus_wmi_led_exit(struct asus_wmi *asus)
 {
-	led_classdev_unregister(&asus->kbd_led);
+	led_classdev_multicolor_unregister(&asus->kbd_led_mc);
 	led_classdev_unregister(&asus->tpd_led);
 	led_classdev_unregister(&asus->wlan_led);
 	led_classdev_unregister(&asus->lightbar_led);
@@ -1057,16 +1081,8 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
 			goto error;
 	}
 
-	if (!kbd_led_read(asus, &led_val, NULL)) {
-		asus->kbd_led_wk = led_val;
-		asus->kbd_led.name = "asus::kbd_backlight";
-		asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
-		asus->kbd_led.brightness_set = kbd_led_set;
-		asus->kbd_led.brightness_get = kbd_led_get;
-		asus->kbd_led.max_brightness = 3;
-
-		rv = led_classdev_register(&asus->platform_device->dev,
-					   &asus->kbd_led);
+	if (!kbd_led_brightness_wmi_read(asus, &led_val, NULL)) {
+		rv = kbd_led_classdev_init(asus, led_val);
 		if (rv)
 			goto error;
 	}
@@ -3057,18 +3073,19 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
 	}
 
 	if (code == NOTIFY_KBD_BRTUP) {
-		kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
+		kbd_led_set_brightness_by_hw(asus, asus->kbd_led_wk + 1);
 		return;
 	}
 	if (code == NOTIFY_KBD_BRTDWN) {
-		kbd_led_set_by_kbd(asus, asus->kbd_led_wk - 1);
+		kbd_led_set_brightness_by_hw(asus, asus->kbd_led_wk - 1);
 		return;
 	}
 	if (code == NOTIFY_KBD_BRTTOGGLE) {
-		if (asus->kbd_led_wk == asus->kbd_led.max_brightness)
-			kbd_led_set_by_kbd(asus, 0);
+		if (asus->kbd_led_wk == asus->kbd_led_mc.led_cdev.max_brightness)
+			kbd_led_set_brightness_by_hw(asus, 0);
 		else
-			kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
+			kbd_led_set_brightness_by_hw(asus,
+					asus->kbd_led_wk + 1);
 		return;
 	}
 
@@ -3720,8 +3737,8 @@ static int asus_hotk_resume(struct device *device)
 {
 	struct asus_wmi *asus = dev_get_drvdata(device);
 
-	if (!IS_ERR_OR_NULL(asus->kbd_led.dev))
-		kbd_led_update(asus);
+	if (!IS_ERR_OR_NULL(asus->kbd_led_mc.led_cdev.dev))
+		kbd_led_brightness_wmi_write(asus, asus->kbd_led_wk);
 
 	if (asus_wmi_has_fnlock_key(asus))
 		asus_wmi_fnlock_update(asus);
@@ -3762,8 +3779,8 @@ static int asus_hotk_restore(struct device *device)
 		bl = !asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_UWB);
 		rfkill_set_sw_state(asus->uwb.rfkill, bl);
 	}
-	if (!IS_ERR_OR_NULL(asus->kbd_led.dev))
-		kbd_led_update(asus);
+	if (!IS_ERR_OR_NULL(asus->kbd_led_mc.led_cdev.dev))
+		kbd_led_brightness_wmi_write(asus, asus->kbd_led_wk);
 
 	if (asus_wmi_has_fnlock_key(asus))
 		asus_wmi_fnlock_update(asus);
-- 
2.35.1

[PATCH 2/2] asus-wmi: Add support for keyboard rgb backlights

[Abhijeet V]

Uses the led multicolor classdev to change the rgb values.
The WMI function expects other settings in addition to the rgb values.
This patch assumes some defaults to get the base rgb functionality
working.

Signed-off-by: Abhijeet V <abhijeetviswa@gmail.com>
---
 drivers/platform/x86/asus-wmi.c            | 137 +++++++++++++++++++++
 include/linux/platform_data/x86/asus-wmi.h |   2 +
 2 files changed, 139 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 117fbcb303d3..f8e92021399c 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -193,6 +193,12 @@ struct fan_curve_data {
 	u8 percents[FAN_CURVE_POINTS];
 };
 
+struct asus_kbd_rgb {
+	u8 red;
+	u8 green;
+	u8 blue;
+};
+
 struct asus_wmi {
 	int dsts_id;
 	int spec;
@@ -217,6 +223,8 @@ struct asus_wmi {
 	struct led_classdev_mc kbd_led_mc;
 	int kbd_led_wk;
 	struct mc_subled subled_info[ASUS_KBD_SUBLED_COUNT];
+	struct asus_kbd_rgb kbd_rgb;
+	bool kbd_rgb_available;
 
 	struct asus_rfkill wlan;
 	struct asus_rfkill bluetooth;
@@ -914,6 +922,114 @@ static void kbd_led_brightness_wmi_write(struct asus_wmi *asus, int value)
 	asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
 }
 
+static int kbd_led_rgb_wmi_write(struct asus_wmi *asus)
+{
+	int err;
+	u32 retval;
+	u8 red;
+	u8 green;
+	u8 blue;
+	u8 speed_byte;
+	u8 mode_byte;
+	u8 speed;
+	u8 mode;
+	u8 flags;
+	u8 persistent;
+
+	speed = 0; // Sane default
+	switch (speed) {
+	case 0:
+	default:
+		speed_byte = 0xe1; // slow
+		speed = 0;
+		break;
+	case 1:
+		speed_byte = 0xeb; // medium
+		break;
+	case 2:
+		speed_byte = 0xf5; // fast
+		break;
+	}
+
+	mode = 0; // Sane default
+	switch (mode) {
+	case 0:
+	default:
+		mode_byte = 0x00; // static color
+		mode = 0;
+		break;
+	case 1:
+		mode_byte = 0x01; // breathing
+		break;
+	case 2:
+		mode_byte = 0x02; // color cycle
+		break;
+	case 3:
+		mode_byte = 0x0a; // strobing
+		break;
+	}
+
+	red = clamp_val(asus->kbd_led_mc.subled_info[0].intensity, 0, 255);
+	green = clamp_val(asus->kbd_led_mc.subled_info[1].intensity, 0, 255);
+	blue = clamp_val(asus->kbd_led_mc.subled_info[2].intensity, 0, 255);
+
+	/*
+	 * 00 - Reset on boot
+	 * 01 - Persist across boot
+	 */
+	persistent = 1; // Sane defaults
+
+	err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS,
+		ASUS_WMI_DEVID_KBD_RGB,
+		(persistent ? 0xb4 : 0xb3) |
+		(mode_byte << 8) |
+		(red << 16) |
+		(green << 24),
+		(blue) |
+		(speed_byte << 8), &retval);
+	if (err) {
+		pr_warn("RGB keyboard device 1, write error: %d\n", err);
+		return err;
+	}
+
+	if (retval != 1) {
+		pr_warn("RGB keyboard device 1, write error (retval): %x\n",
+				retval);
+		return -EIO;
+	}
+
+	/*
+	 * Enable: 02 - on boot (until module load) | 08 - awake | 20 - sleep
+	 * (2a or ff to enable everything)
+	 *
+	 * Logically 80 would be shutdown, but no visible effects of this option
+	 * were observed so far
+	 */
+	flags = 0xff;
+
+	err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS,
+		ASUS_WMI_DEVID_KBD_RGB2,
+		(0xbd) |
+		(flags << 16) |
+		(persistent ? 0x0100 : 0x0000), 0, &retval);
+	if (err) {
+		pr_warn("RGB keyboard device 2, write error: %d\n", err);
+		return err;
+	}
+
+	if (retval != 1) {
+		pr_warn("RGB keyboard device 2, write error (retval): %x\n",
+				retval);
+		return -EIO;
+	}
+
+	asus->kbd_rgb.red = red;
+	asus->kbd_rgb.green = green;
+	asus->kbd_rgb.blue = blue;
+
+	return 0;
+}
+
 static void kbd_led_brightness_set(struct led_classdev *led_cdev,
 		enum led_brightness value)
 {
@@ -928,6 +1044,18 @@ static void kbd_led_brightness_set(struct led_classdev *led_cdev,
 	asus = container_of(led_cdev_mc, struct asus_wmi, kbd_led_mc);
 
 	kbd_led_brightness_wmi_write(asus, value);
+
+	/* Check and set if rgb available */
+	if (!asus->kbd_rgb_available)
+		return;
+
+	if (asus->kbd_rgb.red == asus->subled_info[LED_COLOR_ID_RED].intensity &&
+			asus->kbd_rgb.green == asus->subled_info[LED_COLOR_ID_GREEN].intensity &&
+			asus->kbd_rgb.blue == asus->subled_info[LED_COLOR_ID_BLUE].intensity) {
+		return;
+	}
+
+	kbd_led_rgb_wmi_write(asus);
 }
 
 static void kbd_led_set_brightness_by_hw(struct asus_wmi *asus,
@@ -959,6 +1087,7 @@ int kbd_led_classdev_init(struct asus_wmi *asus, int brightness)
 {
 	int rv;
 
+	asus->kbd_rgb_available = true;
 	asus->kbd_led_wk = brightness;
 	asus->kbd_led_mc.led_cdev.name = "asus::kbd_backlight";
 	asus->kbd_led_mc.led_cdev.flags = LED_BRIGHT_HW_CHANGED;
@@ -966,6 +1095,14 @@ int kbd_led_classdev_init(struct asus_wmi *asus, int brightness)
 	asus->kbd_led_mc.led_cdev.brightness_get = kbd_led_brightness_get;
 	asus->kbd_led_mc.led_cdev.max_brightness = 3;
 
+	asus->subled_info[0].color_index = LED_COLOR_ID_RED;
+	asus->subled_info[0].channel = 0;
+	asus->subled_info[1].color_index = LED_COLOR_ID_GREEN;
+	asus->subled_info[1].channel = 1;
+	asus->subled_info[2].color_index = LED_COLOR_ID_BLUE;
+	asus->subled_info[2].channel = 2;
+	asus->kbd_led_mc.subled_info = asus->subled_info;
+
 	asus->kbd_led_mc.num_colors = ASUS_KBD_SUBLED_COUNT;
 
 	rv = led_classdev_multicolor_register(&asus->platform_device->dev,
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index a571b47ff362..a20ca3787e9f 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -59,6 +59,8 @@
 #define ASUS_WMI_DEVID_LIGHTBAR		0x00050025
 #define ASUS_WMI_DEVID_FAN_BOOST_MODE	0x00110018
 #define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075
+#define ASUS_WMI_DEVID_KBD_RGB		0x00100056
+#define ASUS_WMI_DEVID_KBD_RGB2		0x00100057
 
 /* Misc */
 #define ASUS_WMI_DEVID_PANEL_OD		0x00050019
-- 
2.35.1

Re: [PATCH 1/2] asus-wmi: Use led multicolor class for keyboard backlight

[kernel test robot]

Hi Abhijeet,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.17-rc3 next-20220211]
[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]

url:    https://github.com/0day-ci/linux/commits/Abhijeet-V/asus-wmi-Keyboard-rgb-led-multicolor-support/20220212-040427
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f1baf68e1383f6ed93eb9cff2866d46562607a43
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20220213/202202130255.uwrbJQfB-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project f6685f774697c85d6a352dcea013f46a99f9fe31)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/860e9e7427b00b9dbd0c35851cecda00be1968f2
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Abhijeet-V/asus-wmi-Keyboard-rgb-led-multicolor-support/20220212-040427
        git checkout 860e9e7427b00b9dbd0c35851cecda00be1968f2
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/platform/x86/

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

All warnings (new ones prefixed by >>):

>> drivers/platform/x86/asus-wmi.c:958:5: warning: no previous prototype for function 'kbd_led_classdev_init' [-Wmissing-prototypes]
   int kbd_led_classdev_init(struct asus_wmi *asus, int brightness)
       ^
   drivers/platform/x86/asus-wmi.c:958:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int kbd_led_classdev_init(struct asus_wmi *asus, int brightness)
   ^
   static 
   1 warning generated.


vim +/kbd_led_classdev_init +958 drivers/platform/x86/asus-wmi.c

   957	
 > 958	int kbd_led_classdev_init(struct asus_wmi *asus, int brightness)
   959	{
   960		int rv;
   961	
   962		asus->kbd_led_wk = brightness;
   963		asus->kbd_led_mc.led_cdev.name = "asus::kbd_backlight";
   964		asus->kbd_led_mc.led_cdev.flags = LED_BRIGHT_HW_CHANGED;
   965		asus->kbd_led_mc.led_cdev.brightness_set = kbd_led_brightness_set;
   966		asus->kbd_led_mc.led_cdev.brightness_get = kbd_led_brightness_get;
   967		asus->kbd_led_mc.led_cdev.max_brightness = 3;
   968	
   969		asus->kbd_led_mc.num_colors = ASUS_KBD_SUBLED_COUNT;
   970	
   971		rv = led_classdev_multicolor_register(&asus->platform_device->dev,
   972						&asus->kbd_led_mc);
   973		return rv;
   974	}
   975	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 1/2] asus-wmi: Use led multicolor class for keyboard backlight

[Abhijeet Viswa]

Hi,

Not sure if this email is monitored. But since you asked me to, here ya go.

On 13/02/22 00:38, kernel test robot wrote:
> Hi Abhijeet,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on linus/master]
> [also build test WARNING on v5.17-rc3 next-20220211]
> [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 is intended to be the linux-next/master tree (commit 6d9bd4ad4ca08b1114e814c2c42383b8b13be631).

> url:    https://github.com/0day-ci/linux/commits/Abhijeet-V/asus-wmi-Keyboard-rgb-led-multicolor-support/20220212-040427
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f1baf68e1383f6ed93eb9cff2866d46562607a43
> config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20220213/202202130255.uwrbJQfB-lkp@intel.com/config)
> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project f6685f774697c85d6a352dcea013f46a99f9fe31)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://github.com/0day-ci/linux/commit/860e9e7427b00b9dbd0c35851cecda00be1968f2
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Abhijeet-V/asus-wmi-Keyboard-rgb-led-multicolor-support/20220212-040427
>         git checkout 860e9e7427b00b9dbd0c35851cecda00be1968f2
>         # save the config file to linux build tree
>         mkdir build_dir
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/platform/x86/
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> 
>>> drivers/platform/x86/asus-wmi.c:958:5: warning: no previous prototype for function 'kbd_led_classdev_init' [-Wmissing-prototypes]
>    int kbd_led_classdev_init(struct asus_wmi *asus, int brightness)
>        ^
>    drivers/platform/x86/asus-wmi.c:958:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
>    int kbd_led_classdev_init(struct asus_wmi *asus, int brightness)
>    ^
>    static 
>    1 warning generated.
> 
> 
> vim +/kbd_led_classdev_init +958 drivers/platform/x86/asus-wmi.c
> 
>    957	
>  > 958	int kbd_led_classdev_init(struct asus_wmi *asus, int brightness)
>    959	{
>    960		int rv;
>    961	
>    962		asus->kbd_led_wk = brightness;
>    963		asus->kbd_led_mc.led_cdev.name = "asus::kbd_backlight";
>    964		asus->kbd_led_mc.led_cdev.flags = LED_BRIGHT_HW_CHANGED;
>    965		asus->kbd_led_mc.led_cdev.brightness_set = kbd_led_brightness_set;
>    966		asus->kbd_led_mc.led_cdev.brightness_get = kbd_led_brightness_get;
>    967		asus->kbd_led_mc.led_cdev.max_brightness = 3;
>    968	
>    969		asus->kbd_led_mc.num_colors = ASUS_KBD_SUBLED_COUNT;
>    970	
>    971		rv = led_classdev_multicolor_register(&asus->platform_device->dev,
>    972						&asus->kbd_led_mc);
>    973		return rv;
>    974	}
>    975	

Bot or not, thanks for the report.
I'll fix these in a v2 after others get a chance to review v1.

Thanks,
Abhijeet

> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 1/2] asus-wmi: Use led multicolor class for keyboard backlight

[Pavel Machek]

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

Hi!

> Use the led multicolor class for keyboard backlight so that support for
> rgb keyboard leds can be added for supported Asus laptops.
> 
> Also refactored the keyboard led functions. The function names are now
> indicative of what the function does.
> 
> Signed-off-by: Abhijeet V <abhijeetviswa@gmail.com>

Please Cc LED mailing list/maintainers with LED patches.

Best regards,
								Pavel
								
> +++ 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>
> @@ -88,6 +89,8 @@ module_param(fnlock_default, bool, 0444);
>  #define ASUS_FAN_BOOST_MODE_SILENT_MASK		0x02
>  #define ASUS_FAN_BOOST_MODES_MASK		0x03
>  
> +#define ASUS_KBD_SUBLED_COUNT			3
> +
>  #define ASUS_THROTTLE_THERMAL_POLICY_DEFAULT	0
>  #define ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST	1
>  #define ASUS_THROTTLE_THERMAL_POLICY_SILENT	2
> @@ -204,8 +207,6 @@ struct asus_wmi {
>  	int wlan_led_wk;
>  	struct led_classdev tpd_led;
>  	int tpd_led_wk;
> -	struct led_classdev kbd_led;
> -	int kbd_led_wk;
>  	struct led_classdev lightbar_led;
>  	int lightbar_led_wk;
>  	struct workqueue_struct *led_workqueue;
> @@ -213,6 +214,10 @@ struct asus_wmi {
>  	struct work_struct wlan_led_work;
>  	struct work_struct lightbar_led_work;
>  
> +	struct led_classdev_mc kbd_led_mc;
> +	int kbd_led_wk;
> +	struct mc_subled subled_info[ASUS_KBD_SUBLED_COUNT];
> +
>  	struct asus_rfkill wlan;
>  	struct asus_rfkill bluetooth;
>  	struct asus_rfkill wimax;
> @@ -870,15 +875,7 @@ static enum led_brightness tpd_led_get(struct led_classdev *led_cdev)
>  	return read_tpd_led_state(asus);
>  }
>  
> -static void kbd_led_update(struct asus_wmi *asus)
> -{
> -	int ctrl_param = 0;
> -
> -	ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
> -	asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
> -}
> -
> -static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
> +static int kbd_led_brightness_wmi_read(struct asus_wmi *asus, int *level, int *env)
>  {
>  	int retval;
>  
> @@ -905,50 +902,77 @@ static int kbd_led_read(struct asus_wmi *asus, int *level, int *env)
>  	return 0;
>  }
>  
> -static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
> +static void kbd_led_brightness_wmi_write(struct asus_wmi *asus, int value)
>  {
> -	struct asus_wmi *asus;
>  	int max_level;
> +	int ctrl_param = 0;
>  
> -	asus = container_of(led_cdev, struct asus_wmi, kbd_led);
> -	max_level = asus->kbd_led.max_brightness;
> -
> +	max_level = asus->kbd_led_mc.led_cdev.max_brightness;
>  	asus->kbd_led_wk = clamp_val(value, 0, max_level);
> -	kbd_led_update(asus);
> +
> +	ctrl_param = 0x80 | (asus->kbd_led_wk & 0x7F);
> +	asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
>  }
>  
> -static void kbd_led_set(struct led_classdev *led_cdev,
> -			enum led_brightness value)
> +static void kbd_led_brightness_set(struct led_classdev *led_cdev,
> +		enum led_brightness value)
>  {
> +	struct asus_wmi *asus;
> +	struct led_classdev_mc *led_cdev_mc;
> +
>  	/* Prevent disabling keyboard backlight on module unregister */
>  	if (led_cdev->flags & LED_UNREGISTERING)
>  		return;
>  
> -	do_kbd_led_set(led_cdev, value);
> +	led_cdev_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
> +	asus = container_of(led_cdev_mc, struct asus_wmi, kbd_led_mc);
> +
> +	kbd_led_brightness_wmi_write(asus, value);
>  }
>  
> -static void kbd_led_set_by_kbd(struct asus_wmi *asus, enum led_brightness value)
> +static void kbd_led_set_brightness_by_hw(struct asus_wmi *asus,
> +		enum led_brightness value)
>  {
> -	struct led_classdev *led_cdev = &asus->kbd_led;
> +	struct led_classdev *led_cdev = &asus->kbd_led_mc.led_cdev;
>  
> -	do_kbd_led_set(led_cdev, value);
> +	kbd_led_brightness_wmi_write(asus, value);
>  	led_classdev_notify_brightness_hw_changed(led_cdev, asus->kbd_led_wk);
>  }
>  
> -static enum led_brightness kbd_led_get(struct led_classdev *led_cdev)
> +static enum led_brightness kbd_led_brightness_get(struct led_classdev *led_cdev)
>  {
>  	struct asus_wmi *asus;
> +	struct led_classdev_mc *led_cdev_mc;
>  	int retval, value;
>  
> -	asus = container_of(led_cdev, struct asus_wmi, kbd_led);
> +	led_cdev_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
> +	asus = container_of(led_cdev_mc, struct asus_wmi, kbd_led_mc);
>  
> -	retval = kbd_led_read(asus, &value, NULL);
> +	retval = kbd_led_brightness_wmi_read(asus, &value, NULL);
>  	if (retval < 0)
>  		return retval;
>  
>  	return value;
>  }
>  
> +int kbd_led_classdev_init(struct asus_wmi *asus, int brightness)
> +{
> +	int rv;
> +
> +	asus->kbd_led_wk = brightness;
> +	asus->kbd_led_mc.led_cdev.name = "asus::kbd_backlight";
> +	asus->kbd_led_mc.led_cdev.flags = LED_BRIGHT_HW_CHANGED;
> +	asus->kbd_led_mc.led_cdev.brightness_set = kbd_led_brightness_set;
> +	asus->kbd_led_mc.led_cdev.brightness_get = kbd_led_brightness_get;
> +	asus->kbd_led_mc.led_cdev.max_brightness = 3;
> +
> +	asus->kbd_led_mc.num_colors = ASUS_KBD_SUBLED_COUNT;
> +
> +	rv = led_classdev_multicolor_register(&asus->platform_device->dev,
> +					&asus->kbd_led_mc);
> +	return rv;
> +}
> +
>  static int wlan_led_unknown_state(struct asus_wmi *asus)
>  {
>  	u32 result;
> @@ -1026,7 +1050,7 @@ static enum led_brightness lightbar_led_get(struct led_classdev *led_cdev)
>  
>  static void asus_wmi_led_exit(struct asus_wmi *asus)
>  {
> -	led_classdev_unregister(&asus->kbd_led);
> +	led_classdev_multicolor_unregister(&asus->kbd_led_mc);
>  	led_classdev_unregister(&asus->tpd_led);
>  	led_classdev_unregister(&asus->wlan_led);
>  	led_classdev_unregister(&asus->lightbar_led);
> @@ -1057,16 +1081,8 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
>  			goto error;
>  	}
>  
> -	if (!kbd_led_read(asus, &led_val, NULL)) {
> -		asus->kbd_led_wk = led_val;
> -		asus->kbd_led.name = "asus::kbd_backlight";
> -		asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
> -		asus->kbd_led.brightness_set = kbd_led_set;
> -		asus->kbd_led.brightness_get = kbd_led_get;
> -		asus->kbd_led.max_brightness = 3;
> -
> -		rv = led_classdev_register(&asus->platform_device->dev,
> -					   &asus->kbd_led);
> +	if (!kbd_led_brightness_wmi_read(asus, &led_val, NULL)) {
> +		rv = kbd_led_classdev_init(asus, led_val);
>  		if (rv)
>  			goto error;
>  	}
> @@ -3057,18 +3073,19 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
>  	}
>  
>  	if (code == NOTIFY_KBD_BRTUP) {
> -		kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
> +		kbd_led_set_brightness_by_hw(asus, asus->kbd_led_wk + 1);
>  		return;
>  	}
>  	if (code == NOTIFY_KBD_BRTDWN) {
> -		kbd_led_set_by_kbd(asus, asus->kbd_led_wk - 1);
> +		kbd_led_set_brightness_by_hw(asus, asus->kbd_led_wk - 1);
>  		return;
>  	}
>  	if (code == NOTIFY_KBD_BRTTOGGLE) {
> -		if (asus->kbd_led_wk == asus->kbd_led.max_brightness)
> -			kbd_led_set_by_kbd(asus, 0);
> +		if (asus->kbd_led_wk == asus->kbd_led_mc.led_cdev.max_brightness)
> +			kbd_led_set_brightness_by_hw(asus, 0);
>  		else
> -			kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
> +			kbd_led_set_brightness_by_hw(asus,
> +					asus->kbd_led_wk + 1);
>  		return;
>  	}
>  
> @@ -3720,8 +3737,8 @@ static int asus_hotk_resume(struct device *device)
>  {
>  	struct asus_wmi *asus = dev_get_drvdata(device);
>  
> -	if (!IS_ERR_OR_NULL(asus->kbd_led.dev))
> -		kbd_led_update(asus);
> +	if (!IS_ERR_OR_NULL(asus->kbd_led_mc.led_cdev.dev))
> +		kbd_led_brightness_wmi_write(asus, asus->kbd_led_wk);
>  
>  	if (asus_wmi_has_fnlock_key(asus))
>  		asus_wmi_fnlock_update(asus);
> @@ -3762,8 +3779,8 @@ static int asus_hotk_restore(struct device *device)
>  		bl = !asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_UWB);
>  		rfkill_set_sw_state(asus->uwb.rfkill, bl);
>  	}
> -	if (!IS_ERR_OR_NULL(asus->kbd_led.dev))
> -		kbd_led_update(asus);
> +	if (!IS_ERR_OR_NULL(asus->kbd_led_mc.led_cdev.dev))
> +		kbd_led_brightness_wmi_write(asus, asus->kbd_led_wk);
>  
>  	if (asus_wmi_has_fnlock_key(asus))
>  		asus_wmi_fnlock_update(asus);

-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/2] asus-wmi: Add support for keyboard rgb backlights

[Hans de Goede]

Hi Abhijeet,

On 2/11/22 21:01, Abhijeet V wrote:
> Uses the led multicolor classdev to change the rgb values.
> The WMI function expects other settings in addition to the rgb values.
> This patch assumes some defaults to get the base rgb functionality
> working.
> 
> Signed-off-by: Abhijeet V <abhijeetviswa@gmail.com>

Thank you for your patches. Other then the buildbot issue patch 1/2
looks good to me. I do have some questions / remarks about this one:

> ---
>  drivers/platform/x86/asus-wmi.c            | 137 +++++++++++++++++++++
>  include/linux/platform_data/x86/asus-wmi.h |   2 +
>  2 files changed, 139 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 117fbcb303d3..f8e92021399c 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -193,6 +193,12 @@ struct fan_curve_data {
>  	u8 percents[FAN_CURVE_POINTS];
>  };
>  
> +struct asus_kbd_rgb {
> +	u8 red;
> +	u8 green;
> +	u8 blue;
> +};
> +
>  struct asus_wmi {
>  	int dsts_id;
>  	int spec;
> @@ -217,6 +223,8 @@ struct asus_wmi {
>  	struct led_classdev_mc kbd_led_mc;
>  	int kbd_led_wk;
>  	struct mc_subled subled_info[ASUS_KBD_SUBLED_COUNT];
> +	struct asus_kbd_rgb kbd_rgb;
> +	bool kbd_rgb_available;
>  
>  	struct asus_rfkill wlan;
>  	struct asus_rfkill bluetooth;
> @@ -914,6 +922,114 @@ static void kbd_led_brightness_wmi_write(struct asus_wmi *asus, int value)
>  	asus_wmi_set_devstate(ASUS_WMI_DEVID_KBD_BACKLIGHT, ctrl_param, NULL);
>  }
>  
> +static int kbd_led_rgb_wmi_write(struct asus_wmi *asus)
> +{
> +	int err;
> +	u32 retval;
> +	u8 red;
> +	u8 green;
> +	u8 blue;
> +	u8 speed_byte;
> +	u8 mode_byte;
> +	u8 speed;
> +	u8 mode;
> +	u8 flags;
> +	u8 persistent;
> +
> +	speed = 0; // Sane default
> +	switch (speed) {
> +	case 0:
> +	default:
> +		speed_byte = 0xe1; // slow
> +		speed = 0;
> +		break;
> +	case 1:
> +		speed_byte = 0xeb; // medium
> +		break;
> +	case 2:
> +		speed_byte = 0xf5; // fast
> +		break;
> +	}
> +
> +	mode = 0; // Sane default
> +	switch (mode) {
> +	case 0:
> +	default:
> +		mode_byte = 0x00; // static color
> +		mode = 0;
> +		break;
> +	case 1:
> +		mode_byte = 0x01; // breathing
> +		break;
> +	case 2:
> +		mode_byte = 0x02; // color cycle
> +		break;
> +	case 3:
> +		mode_byte = 0x0a; // strobing
> +		break;
> +	}
> +
> +	red = clamp_val(asus->kbd_led_mc.subled_info[0].intensity, 0, 255);
> +	green = clamp_val(asus->kbd_led_mc.subled_info[1].intensity, 0, 255);
> +	blue = clamp_val(asus->kbd_led_mc.subled_info[2].intensity, 0, 255);
> +
> +	/*
> +	 * 00 - Reset on boot
> +	 * 01 - Persist across boot
> +	 */
> +	persistent = 1; // Sane defaults
> +
> +	err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS,
> +		ASUS_WMI_DEVID_KBD_RGB,
> +		(persistent ? 0xb4 : 0xb3) |
> +		(mode_byte << 8) |
> +		(red << 16) |
> +		(green << 24),
> +		(blue) |
> +		(speed_byte << 8), &retval);
> +	if (err) {
> +		pr_warn("RGB keyboard device 1, write error: %d\n", err);
> +		return err;
> +	}
> +
> +	if (retval != 1) {
> +		pr_warn("RGB keyboard device 1, write error (retval): %x\n",
> +				retval);
> +		return -EIO;
> +	}
> +
> +	/*
> +	 * Enable: 02 - on boot (until module load) | 08 - awake | 20 - sleep
> +	 * (2a or ff to enable everything)
> +	 *
> +	 * Logically 80 would be shutdown, but no visible effects of this option
> +	 * were observed so far
> +	 */
> +	flags = 0xff;
> +
> +	err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS,
> +		ASUS_WMI_DEVID_KBD_RGB2,
> +		(0xbd) |
> +		(flags << 16) |
> +		(persistent ? 0x0100 : 0x0000), 0, &retval);
> +	if (err) {
> +		pr_warn("RGB keyboard device 2, write error: %d\n", err);
> +		return err;
> +	}
> +
> +	if (retval != 1) {
> +		pr_warn("RGB keyboard device 2, write error (retval): %x\n",
> +				retval);
> +		return -EIO;
> +	}
> +
> +	asus->kbd_rgb.red = red;
> +	asus->kbd_rgb.green = green;
> +	asus->kbd_rgb.blue = blue;
> +
> +	return 0;
> +}
> +
>  static void kbd_led_brightness_set(struct led_classdev *led_cdev,
>  		enum led_brightness value)
>  {
> @@ -928,6 +1044,18 @@ static void kbd_led_brightness_set(struct led_classdev *led_cdev,
>  	asus = container_of(led_cdev_mc, struct asus_wmi, kbd_led_mc);
>  
>  	kbd_led_brightness_wmi_write(asus, value);
> +
> +	/* Check and set if rgb available */
> +	if (!asus->kbd_rgb_available)
> +		return;
> +
> +	if (asus->kbd_rgb.red == asus->subled_info[LED_COLOR_ID_RED].intensity &&
> +			asus->kbd_rgb.green == asus->subled_info[LED_COLOR_ID_GREEN].intensity &&
> +			asus->kbd_rgb.blue == asus->subled_info[LED_COLOR_ID_BLUE].intensity) {
> +		return;
> +	}
> +
> +	kbd_led_rgb_wmi_write(asus);
>  }

I notice you are still doing kbd_led_brightness_wmi_write(asus, value); in the 
rgb case is that necessary ? I would expect the rgb settings from kbd_led_rgb_wmi_write()
to also encode/overwrite the brightness setting ?

>  
>  static void kbd_led_set_brightness_by_hw(struct asus_wmi *asus,
> @@ -959,6 +1087,7 @@ int kbd_led_classdev_init(struct asus_wmi *asus, int brightness)
>  {
>  	int rv;
>  
> +	asus->kbd_rgb_available = true;

Sorry but this is not acceptable, you need to detect that this is actually
supported and not change the behavior of the code on laptops without rgb
backlights.

Perhaps Corentin can help with figuring out how to detect this?

>  	asus->kbd_led_wk = brightness;
>  	asus->kbd_led_mc.led_cdev.name = "asus::kbd_backlight";
>  	asus->kbd_led_mc.led_cdev.flags = LED_BRIGHT_HW_CHANGED;
> @@ -966,6 +1095,14 @@ int kbd_led_classdev_init(struct asus_wmi *asus, int brightness)
>  	asus->kbd_led_mc.led_cdev.brightness_get = kbd_led_brightness_get;
>  	asus->kbd_led_mc.led_cdev.max_brightness = 3;
>  
> +	asus->subled_info[0].color_index = LED_COLOR_ID_RED;
> +	asus->subled_info[0].channel = 0;
> +	asus->subled_info[1].color_index = LED_COLOR_ID_GREEN;
> +	asus->subled_info[1].channel = 1;
> +	asus->subled_info[2].color_index = LED_COLOR_ID_BLUE;
> +	asus->subled_info[2].channel = 2;
> +	asus->kbd_led_mc.subled_info = asus->subled_info;
> +

So with just patch 1/2 kbd_led_mc.subled_info is left as NULL, is this allowed?

>  	asus->kbd_led_mc.num_colors = ASUS_KBD_SUBLED_COUNT;
>  
>  	rv = led_classdev_multicolor_register(&asus->platform_device->dev,

Maybe change this to:

	if (asus->kbd_rgb_available)
		rv = led_classdev_multicolor_register(&asus->platform_device->dev,
						      &asus->kbd_led_mc);
	else
		rv led_classdev_register(&asus->platform_device->dev,
					 &asus->kbd_led_mc.led_cdev);

?

(and the same for unregister).

Regards,

Hans


	
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index a571b47ff362..a20ca3787e9f 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -59,6 +59,8 @@
>  #define ASUS_WMI_DEVID_LIGHTBAR		0x00050025
>  #define ASUS_WMI_DEVID_FAN_BOOST_MODE	0x00110018
>  #define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075
> +#define ASUS_WMI_DEVID_KBD_RGB		0x00100056
> +#define ASUS_WMI_DEVID_KBD_RGB2		0x00100057
>  
>  /* Misc */
>  #define ASUS_WMI_DEVID_PANEL_OD		0x00050019
>