[PATCH v3 0/1] HP: wmi: added support for 4 zone keyboard rgb

[PATCH v3 0/1] HP: wmi: added support for 4 zone keyboard rgb

[Carlos Ferreira]

Hi,
As suggested, this driver now uses the multicolor led api.

Please note that for brightness, it takes the brightness files in
each zone and uses them as one as there is no per zone brightness
control, so all the files control the brightness of the entire keyboard.
Also, when the backlight toggle key is pressed it updates the brightness
on all the zones to avoid some edge case problems i ran into
where setting the same color more than once would set the brightness
to the previous value.

Please let me know if this is a good approach.

Carlos Ferreira (1):
  HP: wmi: added support for 4 zone keyboard rgb

 drivers/platform/x86/hp/hp-wmi.c | 248 +++++++++++++++++++++++++++++--
 1 file changed, 239 insertions(+), 9 deletions(-)

-- 
2.45.2

[PATCH v3 1/1] HP: wmi: added support for 4 zone keyboard rgb

[Carlos Ferreira]

This driver adds supports for 4 zone keyboard rgb on omen laptops
and maps the wmi backlight toggle event to KEY_KBDILLUMTOGGLE.
For the backlight, it uses the multicolor led api.

Tested on the HP Omen 15-en1001np.

Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@gmail.com>
---
Changes in v3:
 - Moved to the multicolor led api
 - Mapped the wmi backlight toggle event to KEY_KBDILLUMTOGGLE
 - Some other minor changes
Changes in v2:
 - Rearranged code to remove forward declarations
 - Changed from sprintf() to sysfs_emit()
 - Fixed some identation and coding style problems
 - Switched from manual bit manipulation to GENMASK(x, y) + FIELD_PREP(XX, )
 - #define'ed magic constants
---
 drivers/platform/x86/hp/hp-wmi.c | 248 +++++++++++++++++++++++++++++--
 1 file changed, 239 insertions(+), 9 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
index 5fa553023842..5eae47961f76 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -14,6 +14,8 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/kernel.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/leds.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/slab.h>
@@ -24,6 +26,7 @@
 #include <linux/platform_profile.h>
 #include <linux/hwmon.h>
 #include <linux/acpi.h>
+#include <linux/bits.h>
 #include <linux/rfkill.h>
 #include <linux/string.h>
 #include <linux/dmi.h>
@@ -44,6 +47,14 @@ MODULE_ALIAS("wmi:5FB7F034-2C63-45E9-BE91-3D44E2C707E4");
 
 #define zero_if_sup(tmp) (zero_insize_support?0:sizeof(tmp)) // use when zero insize is required
 
+#define FOURZONE_LIGHTING_SUPPORTED_BIT	0x01
+#define FOURZONE_LIGHTING_ON		228
+#define FOURZONE_LIGHTING_OFF		100
+
+#define FOURZONE_COLOR_R		GENMASK(23, 16)
+#define FOURZONE_COLOR_G		GENMASK(15, 8)
+#define FOURZONE_COLOR_B		GENMASK(7, 0)
+
 /* DMI board names of devices that should use the omen specific path for
  * thermal profiles.
  * This was obtained by taking a look in the windows omen command center
@@ -143,18 +154,36 @@ enum hp_wmi_commandtype {
 };
 
 enum hp_wmi_gm_commandtype {
-	HPWMI_FAN_SPEED_GET_QUERY = 0x11,
-	HPWMI_SET_PERFORMANCE_MODE = 0x1A,
-	HPWMI_FAN_SPEED_MAX_GET_QUERY = 0x26,
-	HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
-	HPWMI_GET_SYSTEM_DESIGN_DATA = 0x28,
+	HPWMI_FAN_SPEED_GET_QUERY	= 0x11,
+	HPWMI_SET_PERFORMANCE_MODE	= 0x1A,
+	HPWMI_FAN_SPEED_MAX_GET_QUERY	= 0x26,
+	HPWMI_FAN_SPEED_MAX_SET_QUERY	= 0x27,
+	HPWMI_GET_SYSTEM_DESIGN_DATA	= 0x28,
+	HPWMI_GET_KEYBOARD_TYPE		= 0x2B,
+};
+
+enum hp_wmi_fourzone_commandtype {
+	HPWMI_SUPPORTS_LIGHTNING	= 0x01,
+	HPWMI_FOURZONE_COLOR_GET	= 0x02,
+	HPWMI_FOURZONE_COLOR_SET	= 0x03,
+	HPWMI_LED_BRIGHTNESS_GET	= 0x04,
+	HPWMI_LED_BRIGHTNESS_SET	= 0x05,
+};
+
+enum hp_wmi_keyboardtype {
+	HPWMI_KEYBOARD_INVALID        = 0x00,
+	HPWMI_KEYBOARD_NORMAL         = 0x01,
+	HPWMI_KEYBOARD_WITH_NUMPAD    = 0x02,
+	HPWMI_KEYBOARD_WITHOUT_NUMPAD = 0x03,
+	HPWMI_KEYBOARD_RGB	      = 0x04,
 };
 
 enum hp_wmi_command {
-	HPWMI_READ	= 0x01,
-	HPWMI_WRITE	= 0x02,
-	HPWMI_ODM	= 0x03,
-	HPWMI_GM	= 0x20008,
+	HPWMI_READ     = 0x01,
+	HPWMI_WRITE    = 0x02,
+	HPWMI_ODM      = 0x03,
+	HPWMI_GM       = 0x20008,
+	HPWMI_FOURZONE = 0x20009,
 };
 
 enum hp_wmi_hardware_mask {
@@ -265,6 +294,7 @@ static struct platform_device *hp_wmi_platform_dev;
 static struct platform_profile_handler platform_profile_handler;
 static bool platform_profile_support;
 static bool zero_insize_support;
+static bool fourzone_lightning_support;
 
 static struct rfkill *wifi_rfkill;
 static struct rfkill *bluetooth_rfkill;
@@ -821,6 +851,40 @@ static struct attribute *hp_wmi_attrs[] = {
 };
 ATTRIBUTE_GROUPS(hp_wmi);
 
+static const char * const fourzone_zone_names[4] = {
+	"hp:rgb:kbd_zoned_backlight-right",
+	"hp:rgb:kbd_zoned_backlight-middle",
+	"hp:rgb:kbd_zoned_backlight-left",
+	"hp:rgb:kbd_zoned_backlight-wasd"
+};
+
+struct hp_fourzone_leds {
+	struct led_classdev_mc leds[4];
+	struct mc_subled subleds[4];
+	u32 color_cache[4];
+};
+static struct hp_fourzone_leds fourzone_leds;
+
+static enum led_brightness get_fourzone_brightness(struct led_classdev *led_cdev)
+{
+	u8 buff[4];
+
+	hp_wmi_perform_query(HPWMI_LED_BRIGHTNESS_GET, HPWMI_FOURZONE,
+		&buff, sizeof(buff), sizeof(buff));
+
+	return buff[0] == FOURZONE_LIGHTING_ON ? LED_ON : LED_OFF;
+}
+
+static void fourzone_update_brightness(void)
+{
+	unsigned int br;
+
+	/* synchronize the brightness level on all zones */
+	br = get_fourzone_brightness(NULL);
+	for (size_t i = 0; i < 4; i++)
+		fourzone_leds.leds[i].led_cdev.brightness = br;
+}
+
 static void hp_wmi_notify(u32 value, void *context)
 {
 	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -932,6 +996,14 @@ static void hp_wmi_notify(u32 value, void *context)
 	case HPWMI_PROXIMITY_SENSOR:
 		break;
 	case HPWMI_BACKLIT_KB_BRIGHTNESS:
+		if (fourzone_lightning_support) {
+			input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, true);
+			input_sync(hp_wmi_input_dev);
+			input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, false);
+			input_sync(hp_wmi_input_dev);
+
+			fourzone_update_brightness();
+		}
 		break;
 	case HPWMI_PEAKSHIFT_PERIOD:
 		break;
@@ -1505,6 +1577,154 @@ static int thermal_profile_setup(void)
 	return 0;
 }
 
+static int fourzone_set_colors(u32 color, size_t zone)
+{
+	u8 buff[128];
+	int ret;
+
+	ret = hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
+		&buff, sizeof(buff), sizeof(buff));
+	if (ret != 0)
+		return -EINVAL;
+
+	buff[25 + zone * 3]     = FIELD_GET(FOURZONE_COLOR_R, color);
+	buff[25 + zone * 3 + 1] = FIELD_GET(FOURZONE_COLOR_G, color);
+	buff[25 + zone * 3 + 2] = FIELD_GET(FOURZONE_COLOR_B, color);
+
+	return hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_SET, HPWMI_FOURZONE,
+		&buff, sizeof(buff), sizeof(buff));
+}
+
+static int fourzone_get_colors(u32 *colors)
+{
+	u8 buff[128];
+	int ret;
+
+	ret = hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
+		&buff, sizeof(buff), sizeof(buff));
+	if (ret != 0)
+		return -EINVAL;
+
+	for (int i = 0; i < 4; i++) {
+		colors[i] = FIELD_PREP(FOURZONE_COLOR_R, buff[25 + i * 3])
+			  | FIELD_PREP(FOURZONE_COLOR_G, buff[25 + i * 3 + 1])
+			  | FIELD_PREP(FOURZONE_COLOR_B, buff[25 + i * 3 + 2]);
+	}
+
+	return 0;
+}
+
+static void set_fourzone_brightness(struct led_classdev *led_cdev, enum led_brightness brightness)
+{
+	size_t zone;
+
+	for (size_t i = 0; i < 4; i++)
+		if (strcmp(led_cdev->name, fourzone_zone_names[i]) == 0)
+			zone = i;
+
+	if (fourzone_leds.leds[zone].subled_info->intensity == fourzone_leds.color_cache[zone]) {
+		u8 buff[4] = {
+			brightness == LED_ON ? FOURZONE_LIGHTING_ON : FOURZONE_LIGHTING_OFF,
+			0, 0, 0
+		};
+
+		hp_wmi_perform_query(HPWMI_LED_BRIGHTNESS_SET, HPWMI_FOURZONE, &buff,
+			sizeof(buff), 0);
+
+		fourzone_update_brightness();
+	} else {
+		fourzone_set_colors(fourzone_leds.leds[zone].subled_info->intensity, zone);
+		fourzone_leds.color_cache[zone] = fourzone_leds.leds[zone].subled_info->intensity;
+	}
+}
+
+static int __init fourzone_leds_init(struct platform_device *device)
+{
+	enum led_brightness brightness;
+	u32 colors[4];
+	int ret;
+
+	ret = fourzone_get_colors(colors);
+	if (ret < 0)
+		return ret;
+
+	memcpy(fourzone_leds.color_cache, colors, sizeof(colors));
+
+	brightness = get_fourzone_brightness(NULL);
+
+	for (size_t i = 0; i < 4; i++) {
+		fourzone_leds.subleds[i] = (struct mc_subled) {
+			.color_index = LED_COLOR_ID_RGB,
+			.brightness = 1,
+			.intensity = colors[i]
+		};
+
+		fourzone_leds.leds[i] = (struct led_classdev_mc) {
+			.led_cdev = {
+				.name = fourzone_zone_names[i],
+				.brightness = brightness,
+				.max_brightness = 1,
+				.brightness_set = set_fourzone_brightness,
+				.brightness_get = get_fourzone_brightness,
+				.color = LED_COLOR_ID_RGB,
+				.flags = LED_BRIGHT_HW_CHANGED
+			},
+			.num_colors = 1,
+			.subled_info = &fourzone_leds.subleds[i]
+		};
+
+		ret = devm_led_classdev_multicolor_register(&device->dev, &fourzone_leds.leds[i]);
+		if (ret)
+			return -ENODEV;
+	}
+
+	return 0;
+}
+
+static enum hp_wmi_keyboardtype fourzone_get_keyboard_type(void)
+{
+	u8 buff[128];
+	int ret;
+
+	ret = hp_wmi_perform_query(HPWMI_GET_KEYBOARD_TYPE, HPWMI_GM,
+		&buff, sizeof(buff), sizeof(buff));
+	if (ret != 0)
+		return HPWMI_KEYBOARD_INVALID;
+
+	/* the first byte in the response represents the keyboard type */
+	return (enum hp_wmi_keyboardtype)(buff[0] + 1);
+}
+
+static bool fourzone_supports_lighting(void)
+{
+	u8 buff[128];
+	int ret;
+
+	ret = hp_wmi_perform_query(HPWMI_SUPPORTS_LIGHTNING, HPWMI_FOURZONE,
+		&buff, sizeof(buff), sizeof(buff));
+	if (ret != 0)
+		return false;
+
+	return buff[0] & FOURZONE_LIGHTING_SUPPORTED_BIT;
+}
+
+static int fourzone_setup(struct platform_device *device)
+{
+	if (!fourzone_supports_lighting())
+		return -ENODEV;
+
+	if (fourzone_get_keyboard_type() != HPWMI_KEYBOARD_WITHOUT_NUMPAD)
+		return -ENODEV;
+
+	/* register leds */
+	if (fourzone_leds_init(device) < 0)
+		return -ENODEV;
+
+	input_set_capability(hp_wmi_input_dev, KE_KEY, KEY_KBDILLUMTOGGLE);
+
+	return 0;
+}
+
 static int hp_wmi_hwmon_init(void);
 
 static int __init hp_wmi_bios_setup(struct platform_device *device)
@@ -1534,6 +1754,10 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
 
 	thermal_profile_setup();
 
+	/* setup 4 zone rgb */
+	if (!fourzone_setup(device))
+		fourzone_lightning_support = true;
+
 	return 0;
 }
 
@@ -1561,6 +1785,12 @@ static void __exit hp_wmi_bios_remove(struct platform_device *device)
 
 	if (platform_profile_support)
 		platform_profile_remove();
+
+	if (fourzone_lightning_support)
+		for (size_t i = 0; i < 4; i++) {
+			devm_led_classdev_multicolor_unregister(&device->dev,
+				&fourzone_leds.leds[i]);
+		}
 }
 
 static int hp_wmi_resume_handler(struct device *device)
-- 
2.45.2

Re: [PATCH v3 1/1] HP: wmi: added support for 4 zone keyboard rgb

[kernel test robot]

Hi Carlos,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.10-rc6 next-20240703]
[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/Carlos-Ferreira/HP-wmi-added-support-for-4-zone-keyboard-rgb/20240708-015808
base:   linus/master
patch link:    https://lore.kernel.org/r/20240707175613.27529-2-carlosmiguelferreira.2003%40gmail.com
patch subject: [PATCH v3 1/1] HP: wmi: added support for 4 zone keyboard rgb
config: i386-randconfig-001-20240708 (https://download.01.org/0day-ci/archive/20240708/202407080657.XLHI1WRv-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240708/202407080657.XLHI1WRv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407080657.XLHI1WRv-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/platform/x86/hp/hp-wmi.c:1590:28: error: call to undeclared function 'FIELD_GET'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1590 |         buff[25 + zone * 3]     = FIELD_GET(FOURZONE_COLOR_R, color);
         |                                   ^
>> drivers/platform/x86/hp/hp-wmi.c:1609:15: error: call to undeclared function 'FIELD_PREP'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1609 |                 colors[i] = FIELD_PREP(FOURZONE_COLOR_R, buff[25 + i * 3])
         |                             ^
   2 errors generated.


vim +/FIELD_GET +1590 drivers/platform/x86/hp/hp-wmi.c

  1579	
  1580	static int fourzone_set_colors(u32 color, size_t zone)
  1581	{
  1582		u8 buff[128];
  1583		int ret;
  1584	
  1585		ret = hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
  1586			&buff, sizeof(buff), sizeof(buff));
  1587		if (ret != 0)
  1588			return -EINVAL;
  1589	
> 1590		buff[25 + zone * 3]     = FIELD_GET(FOURZONE_COLOR_R, color);
  1591		buff[25 + zone * 3 + 1] = FIELD_GET(FOURZONE_COLOR_G, color);
  1592		buff[25 + zone * 3 + 2] = FIELD_GET(FOURZONE_COLOR_B, color);
  1593	
  1594		return hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_SET, HPWMI_FOURZONE,
  1595			&buff, sizeof(buff), sizeof(buff));
  1596	}
  1597	
  1598	static int fourzone_get_colors(u32 *colors)
  1599	{
  1600		u8 buff[128];
  1601		int ret;
  1602	
  1603		ret = hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
  1604			&buff, sizeof(buff), sizeof(buff));
  1605		if (ret != 0)
  1606			return -EINVAL;
  1607	
  1608		for (int i = 0; i < 4; i++) {
> 1609			colors[i] = FIELD_PREP(FOURZONE_COLOR_R, buff[25 + i * 3])
  1610				  | FIELD_PREP(FOURZONE_COLOR_G, buff[25 + i * 3 + 1])
  1611				  | FIELD_PREP(FOURZONE_COLOR_B, buff[25 + i * 3 + 2]);
  1612		}
  1613	
  1614		return 0;
  1615	}
  1616	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 1/1] HP: wmi: added support for 4 zone keyboard rgb

[kernel test robot]

Hi Carlos,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.10-rc7 next-20240703]
[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/Carlos-Ferreira/HP-wmi-added-support-for-4-zone-keyboard-rgb/20240708-015808
base:   linus/master
patch link:    https://lore.kernel.org/r/20240707175613.27529-2-carlosmiguelferreira.2003%40gmail.com
patch subject: [PATCH v3 1/1] HP: wmi: added support for 4 zone keyboard rgb
config: i386-randconfig-012-20240708 (https://download.01.org/0day-ci/archive/20240708/202407080802.Qbwt3Edr-lkp@intel.com/config)
compiler: gcc-9 (Ubuntu 9.5.0-4ubuntu2) 9.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240708/202407080802.Qbwt3Edr-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407080802.Qbwt3Edr-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in kernel/locking/locktorture.o
WARNING: modpost: missing MODULE_DESCRIPTION() in kernel/locking/test-ww_mutex.o
WARNING: modpost: missing MODULE_DESCRIPTION() in kernel/rcu/rcutorture.o
WARNING: modpost: missing MODULE_DESCRIPTION() in kernel/rcu/rcuscale.o
WARNING: modpost: missing MODULE_DESCRIPTION() in kernel/rcu/refscale.o
WARNING: modpost: missing MODULE_DESCRIPTION() in kernel/torture.o
WARNING: modpost: missing MODULE_DESCRIPTION() in mm/dmapool_test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nfs/nfsv4.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_cp437.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_cp737.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_cp852.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_cp855.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_cp861.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_cp862.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_cp932.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_euc-jp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_ascii.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_iso8859-2.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_iso8859-9.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_koi8-u.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_koi8-ru.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/mac-centeuro.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/mac-croatian.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/mac-roman.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/nls/nls_ucs2_utils.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/binfmt_misc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in fs/autofs/autofs4.o
WARNING: modpost: missing MODULE_DESCRIPTION() in crypto/curve25519-generic.o
WARNING: modpost: missing MODULE_DESCRIPTION() in lib/math/prime_numbers.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/acpi/platform_profile.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/dma/qcom/hdma_mgmt.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/regulator/da9121-regulator.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/tty/ttynull.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/char/agp/ati-agp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/char/agp/amd64-agp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/char/agp/efficeon-agp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/char/agp/sis-agp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/char/ppdev.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/gpu/drm/tiny/cirrus.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/base/regmap/regmap-spmi.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mfd/pcf50633-gpio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mfd/qcom-pm8008.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_aec.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_netx.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/i2c/busses/i2c-pxa.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/rc/rc-core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mmc/core/mmc_core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mmc/core/pwrseq_simple.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mmc/core/pwrseq_emmc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/leds/simple/simatic-ipc-leds-gpio-core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/leds/simple/simatic-ipc-leds-gpio-f7188x.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/firmware/google/coreboot_table.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/firmware/google/framebuffer-coreboot.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/firmware/google/memconsole.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/firmware/google/memconsole-coreboot.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_userspace.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-aspeed.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-gpio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/parport/parport.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mtd/chips/cfi_util.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mtd/chips/cfi_cmdset_0020.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/spmi/hisi-spmi-controller.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwmon/mr75203.o
>> ERROR: modpost: "devm_led_classdev_multicolor_unregister" [drivers/platform/x86/hp/hp-wmi.ko] undefined!
>> ERROR: modpost: "devm_led_classdev_multicolor_register_ext" [drivers/platform/x86/hp/hp-wmi.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 1/1] HP: wmi: added support for 4 zone keyboard rgb

[Ilpo Järvinen]

On Sun, 7 Jul 2024, Carlos Ferreira wrote:

> This driver adds supports for 4 zone keyboard rgb on omen laptops
> and maps the wmi backlight toggle event to KEY_KBDILLUMTOGGLE.
> For the backlight, it uses the multicolor led api.
> 
> Tested on the HP Omen 15-en1001np.
> 
> Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@gmail.com>
> ---
> Changes in v3:
>  - Moved to the multicolor led api
>  - Mapped the wmi backlight toggle event to KEY_KBDILLUMTOGGLE
>  - Some other minor changes
> Changes in v2:
>  - Rearranged code to remove forward declarations
>  - Changed from sprintf() to sysfs_emit()
>  - Fixed some identation and coding style problems
>  - Switched from manual bit manipulation to GENMASK(x, y) + FIELD_PREP(XX, )
>  - #define'ed magic constants
> ---
>  drivers/platform/x86/hp/hp-wmi.c | 248 +++++++++++++++++++++++++++++--
>  1 file changed, 239 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> index 5fa553023842..5eae47961f76 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -14,6 +14,8 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/kernel.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/leds.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/slab.h>
> @@ -24,6 +26,7 @@
>  #include <linux/platform_profile.h>
>  #include <linux/hwmon.h>
>  #include <linux/acpi.h>
> +#include <linux/bits.h>

You need to add #include <linux/bitfield.h> as LKP found out. The 
MODULE_DESCRIPTION() ones you can ignore as LKP unfortunately spams some 
kernel-wide problems to all patches even if they're entirely unrelated
to the patch in question.

> +	for (size_t i = 0; i < 4; i++)
> +		fourzone_leds.leds[i].led_cdev.brightness = br;

> +	for (int i = 0; i < 4; i++) {
> +		colors[i] = FIELD_PREP(FOURZONE_COLOR_R, buff[25 + i * 3])
> +			  | FIELD_PREP(FOURZONE_COLOR_G, buff[25 + i * 3 + 1])
> +			  | FIELD_PREP(FOURZONE_COLOR_B, buff[25 + i * 3 + 2]);

The loop variables are not particularly consistent... I'd prefer them to 
be declared in the normal function variable declaration block.

> +
> +	for (size_t i = 0; i < 4; i++)
> +		if (strcmp(led_cdev->name, fourzone_zone_names[i]) == 0)

!strcmp()

> +	ret = fourzone_get_colors(colors);
> +	if (ret < 0)
> +		return ret;
> +
> +	memcpy(fourzone_leds.color_cache, colors, sizeof(colors));
> +
> +	brightness = get_fourzone_brightness(NULL);
> +
> +	for (size_t i = 0; i < 4; i++) {
> +		fourzone_leds.subleds[i] = (struct mc_subled) {
> +			.color_index = LED_COLOR_ID_RGB,
> +			.brightness = 1,
> +			.intensity = colors[i]

Please add comma to the last line because if this is ever extended, this 
line doesn't need to be touched.

> +		};
> +
> +		fourzone_leds.leds[i] = (struct led_classdev_mc) {
> +			.led_cdev = {
> +				.name = fourzone_zone_names[i],
> +				.brightness = brightness,
> +				.max_brightness = 1,
> +				.brightness_set = set_fourzone_brightness,
> +				.brightness_get = get_fourzone_brightness,
> +				.color = LED_COLOR_ID_RGB,
> +				.flags = LED_BRIGHT_HW_CHANGED

Ditto.

> +			},
> +			.num_colors = 1,
> +			.subled_info = &fourzone_leds.subleds[i]

Ditto.

> +	return 0;
> +}
> +
> +static enum hp_wmi_keyboardtype fourzone_get_keyboard_type(void)
> +{
> +	u8 buff[128];
> +	int ret;
> +
> +	ret = hp_wmi_perform_query(HPWMI_GET_KEYBOARD_TYPE, HPWMI_GM,
> +		&buff, sizeof(buff), sizeof(buff));

Align the continuation of parameters to the opening (.

> +	if (ret != 0)
> +		return HPWMI_KEYBOARD_INVALID;
> +
> +	/* the first byte in the response represents the keyboard type */
> +	return (enum hp_wmi_keyboardtype)(buff[0] + 1);
> +}
> +
> +static bool fourzone_supports_lighting(void)
> +{
> +	u8 buff[128];
> +	int ret;
> +
> +	ret = hp_wmi_perform_query(HPWMI_SUPPORTS_LIGHTNING, HPWMI_FOURZONE,
> +		&buff, sizeof(buff), sizeof(buff));

Ditto.

I probably missed a few similar cases as mentioned above so please go 
through your patch and correct them all.

I'm left unsure if a mutex would be necessary to protect the brightness
updates. Probably.


Hans might have more to say about the approach.


-- 
 i.

Re: [PATCH v3 1/1] HP: wmi: added support for 4 zone keyboard rgb

[Hans de Goede]

Hi Ilpo,

Thank you for reviewing this.

On 7/11/24 10:41 AM, Ilpo Järvinen wrote:
> On Sun, 7 Jul 2024, Carlos Ferreira wrote:
> 
>> This driver adds supports for 4 zone keyboard rgb on omen laptops
>> and maps the wmi backlight toggle event to KEY_KBDILLUMTOGGLE.
>> For the backlight, it uses the multicolor led api.
>>
>> Tested on the HP Omen 15-en1001np.
>>
>> Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@gmail.com>
>> ---
>> Changes in v3:
>>  - Moved to the multicolor led api
>>  - Mapped the wmi backlight toggle event to KEY_KBDILLUMTOGGLE
>>  - Some other minor changes
>> Changes in v2:
>>  - Rearranged code to remove forward declarations
>>  - Changed from sprintf() to sysfs_emit()
>>  - Fixed some identation and coding style problems
>>  - Switched from manual bit manipulation to GENMASK(x, y) + FIELD_PREP(XX, )
>>  - #define'ed magic constants
>> ---
>>  drivers/platform/x86/hp/hp-wmi.c | 248 +++++++++++++++++++++++++++++--
>>  1 file changed, 239 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
>> index 5fa553023842..5eae47961f76 100644
>> --- a/drivers/platform/x86/hp/hp-wmi.c
>> +++ b/drivers/platform/x86/hp/hp-wmi.c
>> @@ -14,6 +14,8 @@
>>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>  
>>  #include <linux/kernel.h>
>> +#include <linux/led-class-multicolor.h>
>> +#include <linux/leds.h>
>>  #include <linux/module.h>
>>  #include <linux/init.h>
>>  #include <linux/slab.h>
>> @@ -24,6 +26,7 @@
>>  #include <linux/platform_profile.h>
>>  #include <linux/hwmon.h>
>>  #include <linux/acpi.h>
>> +#include <linux/bits.h>
> 
> You need to add #include <linux/bitfield.h> as LKP found out. The 
> MODULE_DESCRIPTION() ones you can ignore as LKP unfortunately spams some 
> kernel-wide problems to all patches even if they're entirely unrelated
> to the patch in question.

Actually the LKP / kernel test robot email with the MODULE_DESCRIPTION()
warnings does contain useful, new errors. The problem though is that
the bot prefixes new (not seen on builds without this patch) messages
with '>>' making them show up as quoted (replied-to) text making the
actual new errors harder to spot :|   The 2 new actual errors in the
email with all the MODULE_DESCRIPTION warnings are:

ERROR: modpost: "devm_led_classdev_multicolor_unregister" [drivers/platform/x86/hp/hp-wmi.ko] undefined!
ERROR: modpost: "devm_led_classdev_multicolor_register_ext" [drivers/platform/x86/hp/hp-wmi.ko] undefined!

I agree with all the other review remarks and I'll do my own review
soon.

Regards,

Hans

Re: [PATCH v3 1/1] HP: wmi: added support for 4 zone keyboard rgb

[Hans de Goede]

Hi Carlos,

On 7/7/24 7:54 PM, Carlos Ferreira wrote:
> This driver adds supports for 4 zone keyboard rgb on omen laptops
> and maps the wmi backlight toggle event to KEY_KBDILLUMTOGGLE.
> For the backlight, it uses the multicolor led api.
> 
> Tested on the HP Omen 15-en1001np.
> 
> Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@gmail.com>
> ---
> Changes in v3:
>  - Moved to the multicolor led api
>  - Mapped the wmi backlight toggle event to KEY_KBDILLUMTOGGLE
>  - Some other minor changes
> Changes in v2:
>  - Rearranged code to remove forward declarations
>  - Changed from sprintf() to sysfs_emit()
>  - Fixed some identation and coding style problems
>  - Switched from manual bit manipulation to GENMASK(x, y) + FIELD_PREP(XX, )
>  - #define'ed magic constants
> ---
>  drivers/platform/x86/hp/hp-wmi.c | 248 +++++++++++++++++++++++++++++--
>  1 file changed, 239 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> index 5fa553023842..5eae47961f76 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -14,6 +14,8 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/kernel.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/leds.h>

This means that you now also need to update Kconfig to depend on
LEDS_CLASS_MULTICOLOR, so add the following line to the existing
Kconfig entry for the HP WMI driver:

	depends on LEDS_CLASS_MULTICOLOR

Adding this should fix the following errors reported by
the kernel test robot:

ERROR: modpost: "devm_led_classdev_multicolor_unregister" [drivers/platform/x86/hp/hp-wmi.ko] undefined!
ERROR: modpost: "devm_led_classdev_multicolor_register_ext" [drivers/platform/x86/hp/hp-wmi.ko] undefined!



>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/slab.h>
> @@ -24,6 +26,7 @@
>  #include <linux/platform_profile.h>
>  #include <linux/hwmon.h>
>  #include <linux/acpi.h>
> +#include <linux/bits.h>
>  #include <linux/rfkill.h>
>  #include <linux/string.h>
>  #include <linux/dmi.h>

As Ilpo mentioned you need to add a:

#include <linux/bitfield.h>

here.

> @@ -44,6 +47,14 @@ MODULE_ALIAS("wmi:5FB7F034-2C63-45E9-BE91-3D44E2C707E4");
>  
>  #define zero_if_sup(tmp) (zero_insize_support?0:sizeof(tmp)) // use when zero insize is required
>  
> +#define FOURZONE_LIGHTING_SUPPORTED_BIT	0x01
> +#define FOURZONE_LIGHTING_ON		228
> +#define FOURZONE_LIGHTING_OFF		100
> +
> +#define FOURZONE_COLOR_R		GENMASK(23, 16)
> +#define FOURZONE_COLOR_G		GENMASK(15, 8)
> +#define FOURZONE_COLOR_B		GENMASK(7, 0)

I see here and below that you are encoding all 3 color components in
a single 24bit intensity / brightness value, that is not how you
are supposed to use the multi-color LED API. More about this below.

> +
>  /* DMI board names of devices that should use the omen specific path for
>   * thermal profiles.
>   * This was obtained by taking a look in the windows omen command center
> @@ -143,18 +154,36 @@ enum hp_wmi_commandtype {
>  };
>  
>  enum hp_wmi_gm_commandtype {
> -	HPWMI_FAN_SPEED_GET_QUERY = 0x11,
> -	HPWMI_SET_PERFORMANCE_MODE = 0x1A,
> -	HPWMI_FAN_SPEED_MAX_GET_QUERY = 0x26,
> -	HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
> -	HPWMI_GET_SYSTEM_DESIGN_DATA = 0x28,
> +	HPWMI_FAN_SPEED_GET_QUERY	= 0x11,
> +	HPWMI_SET_PERFORMANCE_MODE	= 0x1A,
> +	HPWMI_FAN_SPEED_MAX_GET_QUERY	= 0x26,
> +	HPWMI_FAN_SPEED_MAX_SET_QUERY	= 0x27,
> +	HPWMI_GET_SYSTEM_DESIGN_DATA	= 0x28,
> +	HPWMI_GET_KEYBOARD_TYPE		= 0x2B,
> +};
> +
> +enum hp_wmi_fourzone_commandtype {
> +	HPWMI_SUPPORTS_LIGHTNING	= 0x01,
> +	HPWMI_FOURZONE_COLOR_GET	= 0x02,
> +	HPWMI_FOURZONE_COLOR_SET	= 0x03,
> +	HPWMI_LED_BRIGHTNESS_GET	= 0x04,
> +	HPWMI_LED_BRIGHTNESS_SET	= 0x05,
> +};
> +
> +enum hp_wmi_keyboardtype {
> +	HPWMI_KEYBOARD_INVALID        = 0x00,
> +	HPWMI_KEYBOARD_NORMAL         = 0x01,
> +	HPWMI_KEYBOARD_WITH_NUMPAD    = 0x02,
> +	HPWMI_KEYBOARD_WITHOUT_NUMPAD = 0x03,
> +	HPWMI_KEYBOARD_RGB	      = 0x04,
>  };
>  
>  enum hp_wmi_command {
> -	HPWMI_READ	= 0x01,
> -	HPWMI_WRITE	= 0x02,
> -	HPWMI_ODM	= 0x03,
> -	HPWMI_GM	= 0x20008,
> +	HPWMI_READ     = 0x01,
> +	HPWMI_WRITE    = 0x02,
> +	HPWMI_ODM      = 0x03,
> +	HPWMI_GM       = 0x20008,
> +	HPWMI_FOURZONE = 0x20009,
>  };
>  
>  enum hp_wmi_hardware_mask {
> @@ -265,6 +294,7 @@ static struct platform_device *hp_wmi_platform_dev;
>  static struct platform_profile_handler platform_profile_handler;
>  static bool platform_profile_support;
>  static bool zero_insize_support;
> +static bool fourzone_lightning_support;
>  
>  static struct rfkill *wifi_rfkill;
>  static struct rfkill *bluetooth_rfkill;
> @@ -821,6 +851,40 @@ static struct attribute *hp_wmi_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(hp_wmi);
>  
> +static const char * const fourzone_zone_names[4] = {
> +	"hp:rgb:kbd_zoned_backlight-right",
> +	"hp:rgb:kbd_zoned_backlight-middle",
> +	"hp:rgb:kbd_zoned_backlight-left",
> +	"hp:rgb:kbd_zoned_backlight-wasd"
> +};
> +
> +struct hp_fourzone_leds {
> +	struct led_classdev_mc leds[4];
> +	struct mc_subled subleds[4];

The idea with the multi-color API and subleds is that
a RGB LED really are 3 seperate LEDs (R + G + B) in one.
This is alsohow they are actually phyiscally made.
So for 4 zones you need 12 subleds.

I think it would be best to to have a single struct per zone:

struct hp_fourzone_led {
	struct led_classdev_mc mc_led;
	struct mc_subled subleds[3];
	u32 cache; /* Not sure if you still want this */
};

And then declare an array of 4 of these:

static struct hp_fourzone_led hp_fourzone_leds[4];



> +	u32 color_cache[4];
> +};
> +static struct hp_fourzone_leds fourzone_leds;
> +
> +static enum led_brightness get_fourzone_brightness(struct led_classdev *led_cdev)
> +{
> +	u8 buff[4];
> +
> +	hp_wmi_perform_query(HPWMI_LED_BRIGHTNESS_GET, HPWMI_FOURZONE,
> +		&buff, sizeof(buff), sizeof(buff));
> +
> +	return buff[0] == FOURZONE_LIGHTING_ON ? LED_ON : LED_OFF;
> +}
> +
> +static void fourzone_update_brightness(void)
> +{
> +	unsigned int br;
> +
> +	/* synchronize the brightness level on all zones */
> +	br = get_fourzone_brightness(NULL);
> +	for (size_t i = 0; i < 4; i++)
> +		fourzone_leds.leds[i].led_cdev.brightness = br;
> +}
> +
>  static void hp_wmi_notify(u32 value, void *context)
>  {
>  	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> @@ -932,6 +996,14 @@ static void hp_wmi_notify(u32 value, void *context)
>  	case HPWMI_PROXIMITY_SENSOR:
>  		break;
>  	case HPWMI_BACKLIT_KB_BRIGHTNESS:
> +		if (fourzone_lightning_support) {
> +			input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, true);
> +			input_sync(hp_wmi_input_dev);
> +			input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, false);
> +			input_sync(hp_wmi_input_dev);
> +
> +			fourzone_update_brightness();

Does you calling fourzone_update_brightness() here mean that the embedded
controller (EC) if the laptop toggles the kbd backlight on/off itself when
you press the Fn + key combo for this on the kbd ? In that case you
should not be sending a KEY_KBDILLUMTOGGLE key press event. That event
is telling userspace that it should toggle the brightness, which you
should only do if this is not done by the EC itself.

If the EC does indeed toggle the brightness on/off (or even cycles it
between various brightness levels) then the right thing to do is to
call led_classdev_notify_brightness_hw_changed() on mc_led.led_cdev for
each of the 4 zones when receiving this event.

> +		}
>  		break;
>  	case HPWMI_PEAKSHIFT_PERIOD:
>  		break;
> @@ -1505,6 +1577,154 @@ static int thermal_profile_setup(void)
>  	return 0;
>  }
>  
> +static int fourzone_set_colors(u32 color, size_t zone)
> +{
> +	u8 buff[128];
> +	int ret;
> +
> +	ret = hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
> +		&buff, sizeof(buff), sizeof(buff));
> +	if (ret != 0)
> +		return -EINVAL;

You are doing a read + modify + write of the kbd settings here.

This is fine, but to avoid racing against another r/m/w cycle
done at the same time if userspace writes 2 zones at the same
time you need to take a mutex here.

> +
> +	buff[25 + zone * 3]     = FIELD_GET(FOURZONE_COLOR_R, color);
> +	buff[25 + zone * 3 + 1] = FIELD_GET(FOURZONE_COLOR_G, color);
> +	buff[25 + zone * 3 + 2] = FIELD_GET(FOURZONE_COLOR_B, color);

As mentioned above this is wrong. You should have a separate mc_subled
struct for each color for each zone (so 3 mc_subled-s per zone,
one for each of R, G and B; and the you take subled.brightness field
from the 3 subleds for the 3 different values.

> +
> +	return hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_SET, HPWMI_FOURZONE,
> +		&buff, sizeof(buff), sizeof(buff));
> +}
> +
> +static int fourzone_get_colors(u32 *colors)
> +{
> +	u8 buff[128];
> +	int ret;
> +
> +	ret = hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
> +		&buff, sizeof(buff), sizeof(buff));
> +	if (ret != 0)
> +		return -EINVAL;
> +
> +	for (int i = 0; i < 4; i++) {
> +		colors[i] = FIELD_PREP(FOURZONE_COLOR_R, buff[25 + i * 3])
> +			  | FIELD_PREP(FOURZONE_COLOR_G, buff[25 + i * 3 + 1])
> +			  | FIELD_PREP(FOURZONE_COLOR_B, buff[25 + i * 3 + 2]);
> +	}

same here.

> +
> +	return 0;
> +}
> +
> +static void set_fourzone_brightness(struct led_classdev *led_cdev, enum led_brightness brightness)
> +{
> +	size_t zone;
> +
> +	for (size_t i = 0; i < 4; i++)
> +		if (strcmp(led_cdev->name, fourzone_zone_names[i]) == 0)
> +			zone = i;
> +

So the way how the multicolor LED class devices work is that they have 2
brightness controls:

/sys/class/leds/hp:rgb:kbd_zoned_backlight-right/brightness 
/sys/class/leds/hp:rgb:kbd_zoned_backlight-right/multi_intensity

Where brightness is a single integer value for overall brightness
control and multi_intensity is a per channel brightness control, also see
Documentation/leds/leds-class-multicolor.rst

Now most hw does not have a main/overall brightness control only
per channel controls (like this hp code) so there is a helper which
you pass the overall brightness value and which then calculates the
per channel brightnesses for you.

The LED core code caches the last multi_intensity values for you
and there is a helper you can call from the (this) brightness_set()
callback:

	led_mc_calc_color_components(&fourzone_leds[i].mc_led, brightness);

which will then update the fourzone_leds[i].subleds[0 - 2].brightness
values with the desired per channel brightness values and then you can
use those 3 brightness values in fourzone_set_colors() to send to
the keyboard.


> +	if (fourzone_leds.leds[zone].subled_info->intensity == fourzone_leds.color_cache[zone]) {
> +		u8 buff[4] = {
> +			brightness == LED_ON ? FOURZONE_LIGHTING_ON : FOURZONE_LIGHTING_OFF,
> +			0, 0, 0
> +		};
> +
> +		hp_wmi_perform_query(HPWMI_LED_BRIGHTNESS_SET, HPWMI_FOURZONE, &buff,
> +			sizeof(buff), 0);
> +
> +		fourzone_update_brightness();
> +	} else {
> +		fourzone_set_colors(fourzone_leds.leds[zone].subled_info->intensity, zone);
> +		fourzone_leds.color_cache[zone] = fourzone_leds.leds[zone].subled_info->intensity;
> +	}

And this weird cahce thing can be removed then too, just always send the newly calculated
3 brightness values to the kbd.


> +}
> +
> +static int __init fourzone_leds_init(struct platform_device *device)
> +{
> +	enum led_brightness brightness;
> +	u32 colors[4];
> +	int ret;
> +
> +	ret = fourzone_get_colors(colors);
> +	if (ret < 0)
> +		return ret;
> +
> +	memcpy(fourzone_leds.color_cache, colors, sizeof(colors));
> +
> +	brightness = get_fourzone_brightness(NULL);
> +
> +	for (size_t i = 0; i < 4; i++) {
> +		fourzone_leds.subleds[i] = (struct mc_subled) {
> +			.color_index = LED_COLOR_ID_RGB,
> +			.brightness = 1,
> +			.intensity = colors[i]
> +		};

So over here you need to have a nested for loop to initialize
all 3 subleds for each zone.


> +
> +		fourzone_leds.leds[i] = (struct led_classdev_mc) {
> +			.led_cdev = {
> +				.name = fourzone_zone_names[i],
> +				.brightness = brightness,
> +				.max_brightness = 1,

This should be 255 now.

> +				.brightness_set = set_fourzone_brightness,
> +			`	.brightness_get = get_fourzone_brightness,
> +				.color = LED_COLOR_ID_RGB,
> +				.flags = LED_BRIGHT_HW_CHANGED
> +			},
> +			.num_colors = 1,

This should be 3.

> +			.subled_info = &fourzone_leds.subleds[i]

And this points to fourzone_leds[i].subleds now.

> +		};
> +
> +		ret = devm_led_classdev_multicolor_register(&device->dev, &fourzone_leds.leds[i]);
> +		if (ret)
> +			return -ENODEV;
> +	}


> +
> +	return 0;
> +}
> +
> +static enum hp_wmi_keyboardtype fourzone_get_keyboard_type(void)
> +{
> +	u8 buff[128];
> +	int ret;
> +
> +	ret = hp_wmi_perform_query(HPWMI_GET_KEYBOARD_TYPE, HPWMI_GM,
> +		&buff, sizeof(buff), sizeof(buff));
> +	if (ret != 0)
> +		return HPWMI_KEYBOARD_INVALID;
> +
> +	/* the first byte in the response represents the keyboard type */
> +	return (enum hp_wmi_keyboardtype)(buff[0] + 1);
> +}
> +
> +static bool fourzone_supports_lighting(void)
> +{
> +	u8 buff[128];
> +	int ret;
> +
> +	ret = hp_wmi_perform_query(HPWMI_SUPPORTS_LIGHTNING, HPWMI_FOURZONE,
> +		&buff, sizeof(buff), sizeof(buff));
> +	if (ret != 0)
> +		return false;
> +
> +	return buff[0] & FOURZONE_LIGHTING_SUPPORTED_BIT;
> +}
> +
> +static int fourzone_setup(struct platform_device *device)
> +{
> +	if (!fourzone_supports_lighting())
> +		return -ENODEV;
> +
> +	if (fourzone_get_keyboard_type() != HPWMI_KEYBOARD_WITHOUT_NUMPAD)
> +		return -ENODEV;
> +
> +	/* register leds */
> +	if (fourzone_leds_init(device) < 0)
> +		return -ENODEV;
> +
> +	input_set_capability(hp_wmi_input_dev, KE_KEY, KEY_KBDILLUMTOGGLE);
> +
> +	return 0;
> +}
> +
>  static int hp_wmi_hwmon_init(void);
>  
>  static int __init hp_wmi_bios_setup(struct platform_device *device)
> @@ -1534,6 +1754,10 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>  
>  	thermal_profile_setup();
>  
> +	/* setup 4 zone rgb */
> +	if (!fourzone_setup(device))
> +		fourzone_lightning_support = true;
> +
>  	return 0;
>  }
>  
> @@ -1561,6 +1785,12 @@ static void __exit hp_wmi_bios_remove(struct platform_device *device)
>  
>  	if (platform_profile_support)
>  		platform_profile_remove();
> +
> +	if (fourzone_lightning_support)
> +		for (size_t i = 0; i < 4; i++) {
> +			devm_led_classdev_multicolor_unregister(&device->dev,
> +				&fourzone_leds.leds[i]);
> +		}

The whole idea behind devm_register_foo() functions is that they get automatically
removed when the driver is unbound from the device. So this code and
the fourzone_lightning_support flag are not necessary.

Regards,

Hans



>  }
>  
>  static int hp_wmi_resume_handler(struct device *device)

Re: [PATCH v3 1/1] HP: wmi: added support for 4 zone keyboard rgb

[Carlos Ferreira]

> >  drivers/platform/x86/hp/hp-wmi.c | 248 +++++++++++++++++++++++++++++--
> >  1 file changed, 239 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> > index 5fa553023842..5eae47961f76 100644
> > --- a/drivers/platform/x86/hp/hp-wmi.c
> > +++ b/drivers/platform/x86/hp/hp-wmi.c
> > @@ -14,6 +14,8 @@
> >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >  
> >  #include <linux/kernel.h>
> > +#include <linux/led-class-multicolor.h>
> > +#include <linux/leds.h>
> 
> This means that you now also need to update Kconfig to depend on
> LEDS_CLASS_MULTICOLOR, so add the following line to the existing
> Kconfig entry for the HP WMI driver:
> 
> 	depends on LEDS_CLASS_MULTICOLOR
> 
> Adding this should fix the following errors reported by
> the kernel test robot:
> 
> ERROR: modpost: "devm_led_classdev_multicolor_unregister" [drivers/platform/x86/hp/hp-wmi.ko] undefined!
> ERROR: modpost: "devm_led_classdev_multicolor_register_ext" [drivers/platform/x86/hp/hp-wmi.ko] undefined!

Yes, i completely forgot that. Thank you.

> > +static const char * const fourzone_zone_names[4] = {
> > +	"hp:rgb:kbd_zoned_backlight-right",
> > +	"hp:rgb:kbd_zoned_backlight-middle",
> > +	"hp:rgb:kbd_zoned_backlight-left",
> > +	"hp:rgb:kbd_zoned_backlight-wasd"
> > +};
> > +
> > +struct hp_fourzone_leds {
> > +	struct led_classdev_mc leds[4];
> > +	struct mc_subled subleds[4];
> 
> The idea with the multi-color API and subleds is that
> a RGB LED really are 3 seperate LEDs (R + G + B) in one.
> This is alsohow they are actually phyiscally made.
> So for 4 zones you need 12 subleds.
> 
> I think it would be best to to have a single struct per zone:
> 
> struct hp_fourzone_led {
> 	struct led_classdev_mc mc_led;
> 	struct mc_subled subleds[3];
> 	u32 cache; /* Not sure if you still want this */
> };
> 
> And then declare an array of 4 of these:
> 
> static struct hp_fourzone_led hp_fourzone_leds[4];

That makes sense, i will do it like that.

> > +	u32 color_cache[4];
> > +};
> > +static struct hp_fourzone_leds fourzone_leds;
> > +
> > +static enum led_brightness get_fourzone_brightness(struct led_classdev *led_cdev)
> > +{
> > +	u8 buff[4];
> > +
> > +	hp_wmi_perform_query(HPWMI_LED_BRIGHTNESS_GET, HPWMI_FOURZONE,
> > +		&buff, sizeof(buff), sizeof(buff));
> > +
> > +	return buff[0] == FOURZONE_LIGHTING_ON ? LED_ON : LED_OFF;
> > +}
> > +
> > +static void fourzone_update_brightness(void)
> > +{
> > +	unsigned int br;
> > +
> > +	/* synchronize the brightness level on all zones */
> > +	br = get_fourzone_brightness(NULL);
> > +	for (size_t i = 0; i < 4; i++)
> > +		fourzone_leds.leds[i].led_cdev.brightness = br;
> > +}
> > +
> >  static void hp_wmi_notify(u32 value, void *context)
> >  {
> >  	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> > @@ -932,6 +996,14 @@ static void hp_wmi_notify(u32 value, void *context)
> >  	case HPWMI_PROXIMITY_SENSOR:
> >  		break;
> >  	case HPWMI_BACKLIT_KB_BRIGHTNESS:
> > +		if (fourzone_lightning_support) {
> > +			input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, true);
> > +			input_sync(hp_wmi_input_dev);
> > +			input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, false);
> > +			input_sync(hp_wmi_input_dev);
> > +
> > +			fourzone_update_brightness();
> 
> Does you calling fourzone_update_brightness() here mean that the embedded
> controller (EC) if the laptop toggles the kbd backlight on/off itself when
> you press the Fn + key combo for this on the kbd ? In that case you

It switches between the on/off modes.
More about this below.

> should not be sending a KEY_KBDILLUMTOGGLE key press event. That event
> is telling userspace that it should toggle the brightness, which you
> should only do if this is not done by the EC itself.

Is there a way i could do this that i still get this nice indication on the
screen that the key was pressed?

For me it would make sense to tell the user that the brightness key was pressed.

> If the EC does indeed toggle the brightness on/off (or even cycles it
> between various brightness levels) then the right thing to do is to
> call led_classdev_notify_brightness_hw_changed() on mc_led.led_cdev for
> each of the 4 zones when receiving this event.

For this to work properly, it would depend on how we manage the brightness.
More about this below.

> > +		}
> >  		break;
> >  	case HPWMI_PEAKSHIFT_PERIOD:
> >  		break;
> > @@ -1505,6 +1577,154 @@ static int thermal_profile_setup(void)
> >  	return 0;
> >  }
> >  
> > +static int fourzone_set_colors(u32 color, size_t zone)
> > +{
> > +	u8 buff[128];
> > +	int ret;
> > +
> > +	ret = hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
> > +		&buff, sizeof(buff), sizeof(buff));
> > +	if (ret != 0)
> > +		return -EINVAL;
> 
> You are doing a read + modify + write of the kbd settings here.
> 
> This is fine, but to avoid racing against another r/m/w cycle
> done at the same time if userspace writes 2 zones at the same
> time you need to take a mutex here.

I agree with you and Ilpo, this should have a mutex.

> > +
> > +	buff[25 + zone * 3]     = FIELD_GET(FOURZONE_COLOR_R, color);
> > +	buff[25 + zone * 3 + 1] = FIELD_GET(FOURZONE_COLOR_G, color);
> > +	buff[25 + zone * 3 + 2] = FIELD_GET(FOURZONE_COLOR_B, color);
> 
> As mentioned above this is wrong. You should have a separate mc_subled
> struct for each color for each zone (so 3 mc_subled-s per zone,
> one for each of R, G and B; and the you take subled.brightness field
> from the 3 subleds for the 3 different values.
> 
> > +
> > +	return hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_SET, HPWMI_FOURZONE,
> > +		&buff, sizeof(buff), sizeof(buff));
> > +}
> > +
> > +static int fourzone_get_colors(u32 *colors)
> > +{
> > +	u8 buff[128];
> > +	int ret;
> > +
> > +	ret = hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
> > +		&buff, sizeof(buff), sizeof(buff));
> > +	if (ret != 0)
> > +		return -EINVAL;
> > +
> > +	for (int i = 0; i < 4; i++) {
> > +		colors[i] = FIELD_PREP(FOURZONE_COLOR_R, buff[25 + i * 3])
> > +			  | FIELD_PREP(FOURZONE_COLOR_G, buff[25 + i * 3 + 1])
> > +			  | FIELD_PREP(FOURZONE_COLOR_B, buff[25 + i * 3 + 2]);
> > +	}
> 
> same here.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void set_fourzone_brightness(struct led_classdev *led_cdev, enum led_brightness brightness)
> > +{
> > +	size_t zone;
> > +
> > +	for (size_t i = 0; i < 4; i++)
> > +		if (strcmp(led_cdev->name, fourzone_zone_names[i]) == 0)
> > +			zone = i;
> > +
> 
> So the way how the multicolor LED class devices work is that they have 2
> brightness controls:
> 
> /sys/class/leds/hp:rgb:kbd_zoned_backlight-right/brightness 
> /sys/class/leds/hp:rgb:kbd_zoned_backlight-right/multi_intensity
> 
> Where brightness is a single integer value for overall brightness
> control and multi_intensity is a per channel brightness control, also see
> Documentation/leds/leds-class-multicolor.rst
> 
> Now most hw does not have a main/overall brightness control only
> per channel controls (like this hp code) so there is a helper which
> you pass the overall brightness value and which then calculates the
> per channel brightnesses for you.

This keyboard uses the Fn + key combo more like a mode switcher where it puts
the kbd backligh in on/off mode.

What i was doing was taking the overall brightness and using it as a mode controller
where it would take only 2 value 1/0 for on/off respectively and propagate the values
to the other zones to avoid some edge case problems.

If i start using the overall brightness control to control the brightness of each
channel (like it should probably be) then i loose the ability to control the modes
and we would need to find a way to control them.

But i would definitely agree that having mode control and brightness control at the same
time would be better.

Please note that brightness/color control is completely independent from mode control in
this keyboard. I can set any brightness/color to it even if the backligh is off (it will not
turn the backligh on) and when turned on, the new color will be there.

Perhaps we can create a sysfs entry to control the mode and use the main/overall brightness
control the way it was intended?

Now that i think about it, what i made was kinda of a mess :/

> The LED core code caches the last multi_intensity values for you
> and there is a helper you can call from the (this) brightness_set()
> callback:
> 
> 	led_mc_calc_color_components(&fourzone_leds[i].mc_led, brightness);
> 
> which will then update the fourzone_leds[i].subleds[0 - 2].brightness
> values with the desired per channel brightness values and then you can
> use those 3 brightness values in fourzone_set_colors() to send to
> the keyboard.

Again, this would depend on how we make brightness and mode control.

> > +	if (fourzone_leds.leds[zone].subled_info->intensity == fourzone_leds.color_cache[zone]) {
> > +		u8 buff[4] = {
> > +			brightness == LED_ON ? FOURZONE_LIGHTING_ON : FOURZONE_LIGHTING_OFF,
> > +			0, 0, 0
> > +		};
> > +
> > +		hp_wmi_perform_query(HPWMI_LED_BRIGHTNESS_SET, HPWMI_FOURZONE, &buff,
> > +			sizeof(buff), 0);
> > +
> > +		fourzone_update_brightness();
> > +	} else {
> > +		fourzone_set_colors(fourzone_leds.leds[zone].subled_info->intensity, zone);
> > +		fourzone_leds.color_cache[zone] = fourzone_leds.leds[zone].subled_info->intensity;
> > +	}
> 
> And this weird cahce thing can be removed then too, just always send the newly calculated
> 3 brightness values to the kbd.

It is weird indeed but it was used to know if i should change the color or the keyboard mode.

So when the color didn't change between calls, i would take the overall brightness and
change the mode from it. Obviously, this means loosing brightness control but the way
i see it, mode control is more important.

> > @@ -1561,6 +1785,12 @@ static void __exit hp_wmi_bios_remove(struct platform_device *device)
> >  
> >  	if (platform_profile_support)
> >  		platform_profile_remove();
> > +
> > +	if (fourzone_lightning_support)
> > +		for (size_t i = 0; i < 4; i++) {
> > +			devm_led_classdev_multicolor_unregister(&device->dev,
> > +				&fourzone_leds.leds[i]);
> > +		}
> 
> The whole idea behind devm_register_foo() functions is that they get automatically
> removed when the driver is unbound from the device. So this code and
> the fourzone_lightning_support flag are not necessary.

That is good to know. I will remove it.

Re: [PATCH v3 1/1] HP: wmi: added support for 4 zone keyboard rgb

[Hans de Goede]

Hi Carlos,

On 7/12/24 6:39 PM, Carlos Ferreira wrote:
>>>  drivers/platform/x86/hp/hp-wmi.c | 248 +++++++++++++++++++++++++++++--
>>>  1 file changed, 239 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
>>> index 5fa553023842..5eae47961f76 100644
>>> --- a/drivers/platform/x86/hp/hp-wmi.c
>>> +++ b/drivers/platform/x86/hp/hp-wmi.c
>>> @@ -14,6 +14,8 @@
>>>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>  
>>>  #include <linux/kernel.h>
>>> +#include <linux/led-class-multicolor.h>
>>> +#include <linux/leds.h>
>>
>> This means that you now also need to update Kconfig to depend on
>> LEDS_CLASS_MULTICOLOR, so add the following line to the existing
>> Kconfig entry for the HP WMI driver:
>>
>> 	depends on LEDS_CLASS_MULTICOLOR
>>
>> Adding this should fix the following errors reported by
>> the kernel test robot:
>>
>> ERROR: modpost: "devm_led_classdev_multicolor_unregister" [drivers/platform/x86/hp/hp-wmi.ko] undefined!
>> ERROR: modpost: "devm_led_classdev_multicolor_register_ext" [drivers/platform/x86/hp/hp-wmi.ko] undefined!
> 
> Yes, i completely forgot that. Thank you.
> 
>>> +static const char * const fourzone_zone_names[4] = {
>>> +	"hp:rgb:kbd_zoned_backlight-right",
>>> +	"hp:rgb:kbd_zoned_backlight-middle",
>>> +	"hp:rgb:kbd_zoned_backlight-left",
>>> +	"hp:rgb:kbd_zoned_backlight-wasd"
>>> +};
>>> +
>>> +struct hp_fourzone_leds {
>>> +	struct led_classdev_mc leds[4];
>>> +	struct mc_subled subleds[4];
>>
>> The idea with the multi-color API and subleds is that
>> a RGB LED really are 3 seperate LEDs (R + G + B) in one.
>> This is alsohow they are actually phyiscally made.
>> So for 4 zones you need 12 subleds.
>>
>> I think it would be best to to have a single struct per zone:
>>
>> struct hp_fourzone_led {
>> 	struct led_classdev_mc mc_led;
>> 	struct mc_subled subleds[3];
>> 	u32 cache; /* Not sure if you still want this */
>> };
>>
>> And then declare an array of 4 of these:
>>
>> static struct hp_fourzone_led hp_fourzone_leds[4];
> 
> That makes sense, i will do it like that.
> 
>>> +	u32 color_cache[4];
>>> +};
>>> +static struct hp_fourzone_leds fourzone_leds;
>>> +
>>> +static enum led_brightness get_fourzone_brightness(struct led_classdev *led_cdev)
>>> +{
>>> +	u8 buff[4];
>>> +
>>> +	hp_wmi_perform_query(HPWMI_LED_BRIGHTNESS_GET, HPWMI_FOURZONE,
>>> +		&buff, sizeof(buff), sizeof(buff));
>>> +
>>> +	return buff[0] == FOURZONE_LIGHTING_ON ? LED_ON : LED_OFF;
>>> +}
>>> +
>>> +static void fourzone_update_brightness(void)
>>> +{
>>> +	unsigned int br;
>>> +
>>> +	/* synchronize the brightness level on all zones */
>>> +	br = get_fourzone_brightness(NULL);
>>> +	for (size_t i = 0; i < 4; i++)
>>> +		fourzone_leds.leds[i].led_cdev.brightness = br;
>>> +}
>>> +
>>>  static void hp_wmi_notify(u32 value, void *context)
>>>  {
>>>  	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
>>> @@ -932,6 +996,14 @@ static void hp_wmi_notify(u32 value, void *context)
>>>  	case HPWMI_PROXIMITY_SENSOR:
>>>  		break;
>>>  	case HPWMI_BACKLIT_KB_BRIGHTNESS:
>>> +		if (fourzone_lightning_support) {
>>> +			input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, true);
>>> +			input_sync(hp_wmi_input_dev);
>>> +			input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, false);
>>> +			input_sync(hp_wmi_input_dev);
>>> +
>>> +			fourzone_update_brightness();
>>
>> Does you calling fourzone_update_brightness() here mean that the embedded
>> controller (EC) if the laptop toggles the kbd backlight on/off itself when
>> you press the Fn + key combo for this on the kbd ? In that case you
> 
> It switches between the on/off modes.
> More about this below.
> 
>> should not be sending a KEY_KBDILLUMTOGGLE key press event. That event
>> is telling userspace that it should toggle the brightness, which you
>> should only do if this is not done by the EC itself.
> 
> Is there a way i could do this that i still get this nice indication on the
> screen that the key was pressed?

Yes, led_classdev_notify_brightness_hw_changed() will also inform userspace
about the brightness being changed by the hw itself and at least upower
and GNOME support this. One problem is that upower does not know that the
new /sys/class/leds/hp:rgb:kbd_zoned_backlight-<zone> devices are for
a backlit keyboard. So for testing what you could do is name one of the
zones in the driver "hp:rgb:kbd_backlight". Then GNOME should show
the nice on screen notification when you toggle the brightness with the key,
if you call led_classdev_notify_brightness_hw_changed() on all 4 led_classdev-s
when this happens.

> For me it would make sense to tell the user that the brightness key was pressed.

Right and that is what led_classdev_notify_brightness_hw_changed() does.

The difference is led_classdev_notify_brightness_hw_changed() is for notifying
of the hw changing the brightness itself, where as KEY_KBDILLUMTOGGLE key press
events ask of userspace to toggle the brightness itself. You are not seeing
userspace toggling the brightness itself with your testing because upower
does not support /sys/class/leds/<foo>:rgb:kbd_zoned_backlight-<zone> devices
yet. But once it does support those then sending KEY_KBDILLUMTOGGLE will not
only show the on screen notification it will also cause GNOME to toggle
the brightness (a second time) basically undoing the hw toggle.

>> If the EC does indeed toggle the brightness on/off (or even cycles it
>> between various brightness levels) then the right thing to do is to
>> call led_classdev_notify_brightness_hw_changed() on mc_led.led_cdev for
>> each of the 4 zones when receiving this event.
> 
> For this to work properly, it would depend on how we manage the brightness.
> More about this below.
> 
>>> +		}
>>>  		break;
>>>  	case HPWMI_PEAKSHIFT_PERIOD:
>>>  		break;
>>> @@ -1505,6 +1577,154 @@ static int thermal_profile_setup(void)
>>>  	return 0;
>>>  }
>>>  
>>> +static int fourzone_set_colors(u32 color, size_t zone)
>>> +{
>>> +	u8 buff[128];
>>> +	int ret;
>>> +
>>> +	ret = hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
>>> +		&buff, sizeof(buff), sizeof(buff));
>>> +	if (ret != 0)
>>> +		return -EINVAL;
>>
>> You are doing a read + modify + write of the kbd settings here.
>>
>> This is fine, but to avoid racing against another r/m/w cycle
>> done at the same time if userspace writes 2 zones at the same
>> time you need to take a mutex here.
> 
> I agree with you and Ilpo, this should have a mutex.
> 
>>> +
>>> +	buff[25 + zone * 3]     = FIELD_GET(FOURZONE_COLOR_R, color);
>>> +	buff[25 + zone * 3 + 1] = FIELD_GET(FOURZONE_COLOR_G, color);
>>> +	buff[25 + zone * 3 + 2] = FIELD_GET(FOURZONE_COLOR_B, color);
>>
>> As mentioned above this is wrong. You should have a separate mc_subled
>> struct for each color for each zone (so 3 mc_subled-s per zone,
>> one for each of R, G and B; and the you take subled.brightness field
>> from the 3 subleds for the 3 different values.
>>
>>> +
>>> +	return hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_SET, HPWMI_FOURZONE,
>>> +		&buff, sizeof(buff), sizeof(buff));
>>> +}
>>> +
>>> +static int fourzone_get_colors(u32 *colors)
>>> +{
>>> +	u8 buff[128];
>>> +	int ret;
>>> +
>>> +	ret = hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
>>> +		&buff, sizeof(buff), sizeof(buff));
>>> +	if (ret != 0)
>>> +		return -EINVAL;
>>> +
>>> +	for (int i = 0; i < 4; i++) {
>>> +		colors[i] = FIELD_PREP(FOURZONE_COLOR_R, buff[25 + i * 3])
>>> +			  | FIELD_PREP(FOURZONE_COLOR_G, buff[25 + i * 3 + 1])
>>> +			  | FIELD_PREP(FOURZONE_COLOR_B, buff[25 + i * 3 + 2]);
>>> +	}
>>
>> same here.
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void set_fourzone_brightness(struct led_classdev *led_cdev, enum led_brightness brightness)
>>> +{
>>> +	size_t zone;
>>> +
>>> +	for (size_t i = 0; i < 4; i++)
>>> +		if (strcmp(led_cdev->name, fourzone_zone_names[i]) == 0)
>>> +			zone = i;
>>> +
>>
>> So the way how the multicolor LED class devices work is that they have 2
>> brightness controls:
>>
>> /sys/class/leds/hp:rgb:kbd_zoned_backlight-right/brightness 
>> /sys/class/leds/hp:rgb:kbd_zoned_backlight-right/multi_intensity
>>
>> Where brightness is a single integer value for overall brightness
>> control and multi_intensity is a per channel brightness control, also see
>> Documentation/leds/leds-class-multicolor.rst
>>
>> Now most hw does not have a main/overall brightness control only
>> per channel controls (like this hp code) so there is a helper which
>> you pass the overall brightness value and which then calculates the
>> per channel brightnesses for you.
> 
> This keyboard uses the Fn + key combo more like a mode switcher where it puts
> the kbd backligh in on/off mode.
> 
> What i was doing was taking the overall brightness and using it as a mode controller
> where it would take only 2 value 1/0 for on/off respectively and propagate the values
> to the other zones to avoid some edge case problems.
> 
> If i start using the overall brightness control to control the brightness of each
> channel (like it should probably be) then i loose the ability to control the modes
> and we would need to find a way to control them.
> 
> But i would definitely agree that having mode control and brightness control at the same
> time would be better.
> 
> Please note that brightness/color control is completely independent from mode control in
> this keyboard. I can set any brightness/color to it even if the backligh is off (it will not
> turn the backlight on) and when turned on, the new color will be there.
> 
> Perhaps we can create a sysfs entry to control the mode and use the main/overall brightness
> control the way it was intended?

So basically we have 1 on/off toggle for the entire keyboard and 12 rgb channels
as controls.

So my suggestion would be to do as I proposed previously which is to use
the standard multi-color LED model / helpers with an overall brightness value
per zone and multi_intensity with 3 values for the R / G / B ratio and then
set the global on/off toggle to off if all 4 main brightness values are
set to off (to zero).

So then the brightness_set() callback function should do something like this:

#define KBD_ZONE_COUNT 4

struct hp_fourzone_led {
 	struct led_classdev_mc mc_led;
 	struct mc_subled subleds[3];
	/*
	 * This stores the last set brightness level to restore it on off->on toggle
	 * by the FN-key combo.
	 */
 	enum led_brightness brightness;
};

static void set_fourzone_brightness(struct led_classdev *led_cdev, enum led_brightness brightness)
{
	u8 buff[4] = { };
	int i, zone = 0;
	bool on = false;

	for (i = 0; i < KBD_ZONE_COUNT; i++)
		if (strcmp(led_cdev->name, fourzone_zone_names[i]) == 0)
			zone = i;

	/* Always update main and per color brightness values even when the backlight is off */
	fourzone_leds[zone].brightness = brightness;
	fourzone_leds[zone].mc_led.led_cdev.brightness = brightness;
	led_mc_calc_color_components(&fourzone_leds[zone].mc_led, brightness);

	/* Update the RGB channels for the current zone */
	/* This should use a new version of fourzone_set_colors() which sends
           the fourzone_leds[zone].subleds[0 - 2].brightness to the kbd */
	fourzone_set_colors(zone);

	for (i = 0; i < KBD_ZONE_COUNT; i++)
		if (fourzone_leds[i].mc_led.led_cdev.brightness)
			on = true;

	buff[0] = on ? FOURZONE_LIGHTING_ON : FOURZONE_LIGHTING_OFF;	
	hp_wmi_perform_query(HPWMI_LED_BRIGHTNESS_SET, HPWMI_FOURZONE, &buff, sizeof(buff), 0);
}

Admittedly this is the easier part. The harder part is what to do on
brightness_get() I would not implement a brightness_get() callback, since
that then will be called every time userspace reads the brightness. Since
there is an event when the brightness is changed underneath us by the hw
we don't need to poll the hw. By leaving out the brightness_get() callback
we get to control when we read back the info from the hw.

So instead of implementing a brightness_get() callback, there should be
a helper to get the state of the global on/off toggle and use this in
probe() to set initial brightness values.

Also the actual brightness level (without the on/off toggle taking into
account) should now become an extra member of struct hp_fourzone_led
replacing the old cache this should always represent the latest brightness
value set by either probe() or userspace calling set_brightness() even if
the toggle is off (I already added this above).

And then on events that the Fn-key combo is used, call the helper to get
the new state.

If the new state is off, set fourzone_leds[0-3].mc_led.led_cdev.brightness
to 0 for all 4 zones *before* calling
led_classdev_notify_brightness_hw_changed(), so that when userspace reads
/sys/class/leds/hp:rgb:kbd_zoned_backlight-<zone>/brightness it will read 0.

If the new state is on, restore the old brightness levels saved in
fourzone_leds[i].brightness to fourzone_leds[i].mc_led.led_cdev.brightness
*before* calling led_classdev_notify_brightness_hw_changed().

This way we correctly reflect the on/off toggle state to userspace, making
/sys/class/leds/hp:rgb:kbd_zoned_backlight-<zone>/brightness read 0 when
it is off, while otherwise making things behave as standard multi-color
LED class devices.

> Now that i think about it, what i made was kinda of a mess :/

No worries, that is what the review process is for :)

Regards,

Hans