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

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

[Carlos Ferreira]

Added support for 4 zone keyboard rgb on omen laptops.

Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@gmail.com>
---
 drivers/platform/x86/hp/hp-wmi.c | 308 +++++++++++++++++++++++++++----
 1 file changed, 273 insertions(+), 35 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
index e53660422..f420f41c7 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -103,7 +103,7 @@ enum hp_wmi_event_ids {
 /*
  * struct bios_args buffer is dynamically allocated.  New WMI command types
  * were introduced that exceeds 128-byte data size.  Changes to handle
- * the data size allocation scheme were kept in hp_wmi_perform_qurey function.
+ * the data size allocation scheme were kept in hp_wmi_perform_query function.
  */
 struct bios_args {
 	u32 signature;
@@ -114,15 +114,15 @@ struct bios_args {
 };
 
 enum hp_wmi_commandtype {
-	HPWMI_DISPLAY_QUERY		= 0x01,
-	HPWMI_HDDTEMP_QUERY		= 0x02,
-	HPWMI_ALS_QUERY			= 0x03,
+	HPWMI_DISPLAY_QUERY			= 0x01,
+	HPWMI_HDDTEMP_QUERY			= 0x02,
+	HPWMI_ALS_QUERY				= 0x03,
 	HPWMI_HARDWARE_QUERY		= 0x04,
 	HPWMI_WIRELESS_QUERY		= 0x05,
-	HPWMI_BATTERY_QUERY		= 0x07,
-	HPWMI_BIOS_QUERY		= 0x09,
-	HPWMI_FEATURE_QUERY		= 0x0b,
-	HPWMI_HOTKEY_QUERY		= 0x0c,
+	HPWMI_BATTERY_QUERY			= 0x07,
+	HPWMI_BIOS_QUERY			= 0x09,
+	HPWMI_FEATURE_QUERY			= 0x0b,
+	HPWMI_HOTKEY_QUERY			= 0x0c,
 	HPWMI_FEATURE2_QUERY		= 0x0d,
 	HPWMI_WIRELESS2_QUERY		= 0x1b,
 	HPWMI_POSTCODEERROR_QUERY	= 0x2a,
@@ -131,18 +131,36 @@ enum hp_wmi_commandtype {
 };
 
 enum hp_wmi_gm_commandtype {
-	HPWMI_FAN_SPEED_GET_QUERY = 0x11,
-	HPWMI_SET_PERFORMANCE_MODE = 0x1A,
+	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_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_FOURZONE_MODE_GET  = 0x04,
+	HPWMI_FOURZONE_MODE_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 {
@@ -156,18 +174,18 @@ struct bios_return {
 };
 
 enum hp_return_value {
-	HPWMI_RET_WRONG_SIGNATURE	= 0x02,
-	HPWMI_RET_UNKNOWN_COMMAND	= 0x03,
-	HPWMI_RET_UNKNOWN_CMDTYPE	= 0x04,
-	HPWMI_RET_INVALID_PARAMETERS	= 0x05,
+	HPWMI_RET_WRONG_SIGNATURE	 = 0x02,
+	HPWMI_RET_UNKNOWN_COMMAND	 = 0x03,
+	HPWMI_RET_UNKNOWN_CMDTYPE	 = 0x04,
+	HPWMI_RET_INVALID_PARAMETERS = 0x05,
 };
 
 enum hp_wireless2_bits {
-	HPWMI_POWER_STATE	= 0x01,
-	HPWMI_POWER_SOFT	= 0x02,
-	HPWMI_POWER_BIOS	= 0x04,
-	HPWMI_POWER_HARD	= 0x08,
-	HPWMI_POWER_FW_OR_HW	= HPWMI_POWER_BIOS | HPWMI_POWER_HARD,
+	HPWMI_POWER_STATE	 = 0x01,
+	HPWMI_POWER_SOFT	 = 0x02,
+	HPWMI_POWER_BIOS	 = 0x04,
+	HPWMI_POWER_HARD	 = 0x08,
+	HPWMI_POWER_FW_OR_HW = HPWMI_POWER_BIOS | HPWMI_POWER_HARD,
 };
 
 enum hp_thermal_profile_omen_v0 {
@@ -177,22 +195,22 @@ enum hp_thermal_profile_omen_v0 {
 };
 
 enum hp_thermal_profile_omen_v1 {
-	HP_OMEN_V1_THERMAL_PROFILE_DEFAULT	= 0x30,
-	HP_OMEN_V1_THERMAL_PROFILE_PERFORMANCE	= 0x31,
-	HP_OMEN_V1_THERMAL_PROFILE_COOL		= 0x50,
+	HP_OMEN_V1_THERMAL_PROFILE_DEFAULT	   = 0x30,
+	HP_OMEN_V1_THERMAL_PROFILE_PERFORMANCE = 0x31,
+	HP_OMEN_V1_THERMAL_PROFILE_COOL		   = 0x50,
 };
 
 enum hp_thermal_profile_victus {
-	HP_VICTUS_THERMAL_PROFILE_DEFAULT		= 0x00,
-	HP_VICTUS_THERMAL_PROFILE_PERFORMANCE		= 0x01,
-	HP_VICTUS_THERMAL_PROFILE_QUIET			= 0x03,
+	HP_VICTUS_THERMAL_PROFILE_DEFAULT	  = 0x00,
+	HP_VICTUS_THERMAL_PROFILE_PERFORMANCE = 0x01,
+	HP_VICTUS_THERMAL_PROFILE_QUIET		  = 0x03,
 };
 
 enum hp_thermal_profile {
-	HP_THERMAL_PROFILE_PERFORMANCE	= 0x00,
-	HP_THERMAL_PROFILE_DEFAULT		= 0x01,
-	HP_THERMAL_PROFILE_COOL			= 0x02,
-	HP_THERMAL_PROFILE_QUIET		= 0x03,
+	HP_THERMAL_PROFILE_PERFORMANCE = 0x00,
+	HP_THERMAL_PROFILE_DEFAULT	   = 0x01,
+	HP_THERMAL_PROFILE_COOL		   = 0x02,
+	HP_THERMAL_PROFILE_QUIET	   = 0x03,
 };
 
 #define IS_HWBLOCKED(x) ((x & HPWMI_POWER_FW_OR_HW) != HPWMI_POWER_FW_OR_HW)
@@ -754,6 +772,67 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static int fourzone_get_colors(u32 *colors);
+static int fourzone_set_colors(u32 *colors);
+
+static ssize_t colors_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	u32 colors[4];
+
+	/* read the colors from wmi and write them to the buf */
+	if (fourzone_get_colors(colors) == 0)
+		return sprintf(buf, "%06x %06x %06x %06x\n",
+					colors[0], colors[1], colors[2], colors[3]);
+
+	return -EINVAL;
+}
+
+static ssize_t colors_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	u32 colors[4];
+
+	if (sscanf(buf, "%6x %6x %6x %6x", &colors[0], &colors[1], &colors[2], &colors[3]) != 4)
+		return -EINVAL;
+
+	/* set the colors */
+	int ret = fourzone_set_colors(colors);
+
+	return ret == 0 ? count : ret;
+}
+
+static int fourzone_get_mode(void);
+static int fourzone_set_mode(u32 mode);
+
+static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	/* read the mode from wmi and write it to the buf */
+	u32 ret = fourzone_get_mode();
+
+	if (ret >= 0)
+		return sprintf(buf, "%d\n", ret);
+
+	return ret;
+}
+
+static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	u32 mode;
+
+	int ret = kstrtou32(buf, 10, &mode);
+
+	if (ret)
+		return ret;
+
+	/* set the mode */
+	ret = fourzone_set_mode(mode);
+
+	return ret == 0 ? count : ret;
+}
+
 static int camera_shutter_input_setup(void)
 {
 	int err;
@@ -781,6 +860,22 @@ static int camera_shutter_input_setup(void)
 	return err;
 }
 
+static enum hp_wmi_keyboardtype fourzone_get_keyboard_type(void);
+
+static ssize_t type_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	enum hp_wmi_keyboardtype type = fourzone_get_keyboard_type();
+
+	if (type != HPWMI_KEYBOARD_INVALID)
+		return sprintf(buf, "%d\n", type - 1);
+
+	return -EINVAL;
+}
+
+/*
+ * Generic device attributes.
+ */
 static DEVICE_ATTR_RO(display);
 static DEVICE_ATTR_RO(hddtemp);
 static DEVICE_ATTR_RW(als);
@@ -797,7 +892,35 @@ static struct attribute *hp_wmi_attrs[] = {
 	&dev_attr_postcode.attr,
 	NULL,
 };
-ATTRIBUTE_GROUPS(hp_wmi);
+
+static struct attribute_group hp_wmi_group = {
+	.attrs = hp_wmi_attrs,
+};
+
+/*
+ * Omen fourzone specific device attributes.
+ */
+static DEVICE_ATTR_RW(colors);
+static DEVICE_ATTR_RW(mode);
+static DEVICE_ATTR_RO(type);
+
+static struct attribute *hp_wmi_fourzone_attrs[] = {
+	&dev_attr_colors.attr,
+	&dev_attr_mode.attr,
+	&dev_attr_type.attr,
+	NULL,
+};
+
+static struct attribute_group hp_wmi_fourzone_group = {
+	.attrs = hp_wmi_fourzone_attrs,
+	.name = "kbd-backlight",
+};
+
+static const struct attribute_group *hp_wmi_groups[] = {
+	&hp_wmi_group,
+	NULL,
+	NULL,
+};
 
 static void hp_wmi_notify(u32 value, void *context)
 {
@@ -1446,6 +1569,118 @@ static int thermal_profile_setup(void)
 	return 0;
 }
 
+static bool fourzone_supports_lighting(void)
+{
+	u8 buff[128];
+
+	int ret = hp_wmi_perform_query(HPWMI_SUPPORTS_LIGHTNING, HPWMI_FOURZONE,
+								&buff, sizeof(buff), sizeof(buff));
+	/* the first bit in the response is set to 1 if the system supports lighting */
+	if (ret == 0)
+		return (buff[0] & 0x01) == 1;
+
+	return false;
+}
+
+static enum hp_wmi_keyboardtype fourzone_get_keyboard_type(void)
+{
+	u8 buff[128];
+
+	int ret = hp_wmi_perform_query(HPWMI_GET_KEYBOARD_TYPE, HPWMI_GM,
+								&buff, sizeof(buff), sizeof(buff));
+	/* the first byte in the response represents the keyborad type */
+	if (ret == 0)
+		return (enum hp_wmi_keyboardtype)(buff[0] + 1);
+
+	return HPWMI_KEYBOARD_INVALID;
+}
+
+static int fourzone_get_colors(u32 *colors)
+{
+	u8 buff[128];
+
+	if (hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
+							&buff, sizeof(buff), sizeof(buff)) == 0) {
+		for (int i = 0; i < 4; i++) {
+			colors[3 - i] = ((buff[25 + i * 3]     & 0xFF) << 16)  // r
+						  | ((buff[25 + i * 3 + 1] & 0xFF) <<  8)  // g
+						  | ((buff[25 + i * 3 + 2] & 0xFF) <<  0); // b
+		}
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int fourzone_set_colors(u32 *colors)
+{
+	u8 buff[128];
+
+	if (hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
+							&buff, sizeof(buff), sizeof(buff)) == 0) {
+		for (int i = 0; i < 4; i++) {
+			buff[25 + i * 3]	 = (colors[3 - i] >> 16) & 0xFF; // r
+			buff[25 + i * 3 + 1] = (colors[3 - i] >>  8) & 0xFF; // g
+			buff[25 + i * 3 + 2] = (colors[3 - i] >>  0) & 0xFF; // b
+		}
+
+		return hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_SET, HPWMI_FOURZONE,
+						&buff, sizeof(buff), sizeof(buff));
+	}
+
+	return -EINVAL;
+}
+
+/*
+ * Returns a negative number on error or 0/1 for the mode.
+ */
+static int fourzone_get_mode(void)
+{
+	u8 buff[4];
+
+	int ret = hp_wmi_perform_query(HPWMI_FOURZONE_MODE_GET, HPWMI_FOURZONE,
+								&buff, sizeof(buff), sizeof(buff));
+
+	if (ret == 0)
+		return buff[0] == 228 ? 1 : 0;
+
+	return ret;
+}
+
+/*
+ * This device supports only two different modes:
+ * 1 -> lights on
+ * 0 -> lights off
+ */
+static int fourzone_set_mode(u32 mode)
+{
+	u8 buff[4] = {mode ? 228 : 100, 0, 0, 0};
+
+	return hp_wmi_perform_query(HPWMI_FOURZONE_MODE_SET, HPWMI_FOURZONE,
+								&buff, sizeof(buff), 0);
+}
+
+static int fourzone_setup(struct platform_device *device)
+{
+	/* check if the system supports lighting */
+	bool supports_lighting = fourzone_supports_lighting();
+
+	if (!supports_lighting)
+		return -ENODEV;
+
+	/* check if we have a supported keyboard type */
+	enum hp_wmi_keyboardtype kbd_type = fourzone_get_keyboard_type();
+
+	if (kbd_type != HPWMI_KEYBOARD_WITHOUT_NUMPAD)
+		return -ENODEV;
+
+	/* register the new groups */
+	hp_wmi_groups[1] = &hp_wmi_fourzone_group;
+
+	return 0;
+}
+
 static int hp_wmi_hwmon_init(void);
 
 static int __init hp_wmi_bios_setup(struct platform_device *device)
@@ -1475,6 +1710,9 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
 
 	thermal_profile_setup();
 
+	/* setup 4 zone rgb, no problem if it fails */
+	fourzone_setup(device);
+
 	return 0;
 }
 
-- 
2.44.0

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

[Ilpo Järvinen]

On Sun, 24 Mar 2024, Carlos Ferreira wrote:

> Added support for 4 zone keyboard rgb on omen laptops.
> 
> Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@gmail.com>
> ---
>  drivers/platform/x86/hp/hp-wmi.c | 308 +++++++++++++++++++++++++++----
>  1 file changed, 273 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> index e53660422..f420f41c7 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -103,7 +103,7 @@ enum hp_wmi_event_ids {
>  /*
>   * struct bios_args buffer is dynamically allocated.  New WMI command types
>   * were introduced that exceeds 128-byte data size.  Changes to handle
> - * the data size allocation scheme were kept in hp_wmi_perform_qurey function.
> + * the data size allocation scheme were kept in hp_wmi_perform_query function.

Good change, but unrelated to "added support for" patch. Please make a 
separate change out of it.

>   */
>  struct bios_args {
>  	u32 signature;
> @@ -114,15 +114,15 @@ struct bios_args {
>  };
>  
>  enum hp_wmi_commandtype {
> -	HPWMI_DISPLAY_QUERY		= 0x01,
> -	HPWMI_HDDTEMP_QUERY		= 0x02,
> -	HPWMI_ALS_QUERY			= 0x03,
> +	HPWMI_DISPLAY_QUERY			= 0x01,
> +	HPWMI_HDDTEMP_QUERY			= 0x02,
> +	HPWMI_ALS_QUERY				= 0x03,

Likewise, these changes of whitespace are unrelated so if you want to 
make them, please make a separate patch for them (please go through your 
whole patch this in mind).

>  	HPWMI_HARDWARE_QUERY		= 0x04,
>  	HPWMI_WIRELESS_QUERY		= 0x05,
> -	HPWMI_BATTERY_QUERY		= 0x07,
> -	HPWMI_BIOS_QUERY		= 0x09,
> -	HPWMI_FEATURE_QUERY		= 0x0b,
> -	HPWMI_HOTKEY_QUERY		= 0x0c,
> +	HPWMI_BATTERY_QUERY			= 0x07,
> +	HPWMI_BIOS_QUERY			= 0x09,
> +	HPWMI_FEATURE_QUERY			= 0x0b,
> +	HPWMI_HOTKEY_QUERY			= 0x0c,
>  	HPWMI_FEATURE2_QUERY		= 0x0d,
>  	HPWMI_WIRELESS2_QUERY		= 0x1b,
>  	HPWMI_POSTCODEERROR_QUERY	= 0x2a,
> @@ -131,18 +131,36 @@ enum hp_wmi_commandtype {
>  };
>  
>  enum hp_wmi_gm_commandtype {
> -	HPWMI_FAN_SPEED_GET_QUERY = 0x11,
> -	HPWMI_SET_PERFORMANCE_MODE = 0x1A,
> +	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_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_FOURZONE_MODE_GET  = 0x04,
> +	HPWMI_FOURZONE_MODE_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 {
> @@ -156,18 +174,18 @@ struct bios_return {
>  };
>  
>  enum hp_return_value {
> -	HPWMI_RET_WRONG_SIGNATURE	= 0x02,
> -	HPWMI_RET_UNKNOWN_COMMAND	= 0x03,
> -	HPWMI_RET_UNKNOWN_CMDTYPE	= 0x04,
> -	HPWMI_RET_INVALID_PARAMETERS	= 0x05,
> +	HPWMI_RET_WRONG_SIGNATURE	 = 0x02,
> +	HPWMI_RET_UNKNOWN_COMMAND	 = 0x03,
> +	HPWMI_RET_UNKNOWN_CMDTYPE	 = 0x04,
> +	HPWMI_RET_INVALID_PARAMETERS = 0x05,
>  };
>  
>  enum hp_wireless2_bits {
> -	HPWMI_POWER_STATE	= 0x01,
> -	HPWMI_POWER_SOFT	= 0x02,
> -	HPWMI_POWER_BIOS	= 0x04,
> -	HPWMI_POWER_HARD	= 0x08,
> -	HPWMI_POWER_FW_OR_HW	= HPWMI_POWER_BIOS | HPWMI_POWER_HARD,
> +	HPWMI_POWER_STATE	 = 0x01,
> +	HPWMI_POWER_SOFT	 = 0x02,
> +	HPWMI_POWER_BIOS	 = 0x04,
> +	HPWMI_POWER_HARD	 = 0x08,
> +	HPWMI_POWER_FW_OR_HW = HPWMI_POWER_BIOS | HPWMI_POWER_HARD,
>  };
>  
>  enum hp_thermal_profile_omen_v0 {
> @@ -177,22 +195,22 @@ enum hp_thermal_profile_omen_v0 {
>  };
>  
>  enum hp_thermal_profile_omen_v1 {
> -	HP_OMEN_V1_THERMAL_PROFILE_DEFAULT	= 0x30,
> -	HP_OMEN_V1_THERMAL_PROFILE_PERFORMANCE	= 0x31,
> -	HP_OMEN_V1_THERMAL_PROFILE_COOL		= 0x50,
> +	HP_OMEN_V1_THERMAL_PROFILE_DEFAULT	   = 0x30,
> +	HP_OMEN_V1_THERMAL_PROFILE_PERFORMANCE = 0x31,
> +	HP_OMEN_V1_THERMAL_PROFILE_COOL		   = 0x50,
>  };
>  
>  enum hp_thermal_profile_victus {
> -	HP_VICTUS_THERMAL_PROFILE_DEFAULT		= 0x00,
> -	HP_VICTUS_THERMAL_PROFILE_PERFORMANCE		= 0x01,
> -	HP_VICTUS_THERMAL_PROFILE_QUIET			= 0x03,
> +	HP_VICTUS_THERMAL_PROFILE_DEFAULT	  = 0x00,
> +	HP_VICTUS_THERMAL_PROFILE_PERFORMANCE = 0x01,
> +	HP_VICTUS_THERMAL_PROFILE_QUIET		  = 0x03,
>  };
>  
>  enum hp_thermal_profile {
> -	HP_THERMAL_PROFILE_PERFORMANCE	= 0x00,
> -	HP_THERMAL_PROFILE_DEFAULT		= 0x01,
> -	HP_THERMAL_PROFILE_COOL			= 0x02,
> -	HP_THERMAL_PROFILE_QUIET		= 0x03,
> +	HP_THERMAL_PROFILE_PERFORMANCE = 0x00,
> +	HP_THERMAL_PROFILE_DEFAULT	   = 0x01,
> +	HP_THERMAL_PROFILE_COOL		   = 0x02,
> +	HP_THERMAL_PROFILE_QUIET	   = 0x03,
>  };
>  
>  #define IS_HWBLOCKED(x) ((x & HPWMI_POWER_FW_OR_HW) != HPWMI_POWER_FW_OR_HW)
> @@ -754,6 +772,67 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
>  	return count;
>  }
>  
> +static int fourzone_get_colors(u32 *colors);
> +static int fourzone_set_colors(u32 *colors);

Please rearrange code so tha you don't need forward declarations for 
function.

> +static ssize_t colors_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	u32 colors[4];
> +
> +	/* read the colors from wmi and write them to the buf */
> +	if (fourzone_get_colors(colors) == 0)
> +		return sprintf(buf, "%06x %06x %06x %06x\n",
> +					colors[0], colors[1], colors[2], colors[3]);

sysfs_emit()

The second line Looks misaligned. 

> +
> +	return -EINVAL;

IMO, it would be better to reverse the condition and return -EINVAL if
fourzone_get_colors(colors) != 0.

> +}
> +
> +static ssize_t colors_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	u32 colors[4];
> +
> +	if (sscanf(buf, "%6x %6x %6x %6x", &colors[0], &colors[1], &colors[2], &colors[3]) != 4)

Please add ret variable and do:

	ret = sscanf(...);
	if (ret != 4)

> +		return -EINVAL;
> +
> +	/* set the colors */

Unnecessary comment.

> +	int ret = fourzone_set_colors(colors);
> +
> +	return ret == 0 ? count : ret;
> +}
> +
> +static int fourzone_get_mode(void);
> +static int fourzone_set_mode(u32 mode);
> +
> +static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	/* read the mode from wmi and write it to the buf */

Unnecessary comment (and not placed well anyway since it cover what the 
entire function does).

> +	u32 ret = fourzone_get_mode();
> +
> +	if (ret >= 0)

Reverse logic:

	if (ret < 0)
		return ret;

> +		return sprintf(buf, "%d\n", ret);

sysfs_emit()

> +
> +	return ret;
> +}
> +
> +static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	u32 mode;
> +
> +	int ret = kstrtou32(buf, 10, &mode);
> +
> +	if (ret)

Misgrouped lines, please do:

	u32 mode;
	int ret;

	ret = kstrtou32(buf, 10, &mode);
	if (ret)

> +		return ret;
> +
> +	/* set the mode */

Unnecessary comment, your function name already tell the very same bit of 
information.

> +	ret = fourzone_set_mode(mode);
> +
> +	return ret == 0 ? count : ret;
> +}
> +
>  static int camera_shutter_input_setup(void)
>  {
>  	int err;
> @@ -781,6 +860,22 @@ static int camera_shutter_input_setup(void)
>  	return err;
>  }
>  
> +static enum hp_wmi_keyboardtype fourzone_get_keyboard_type(void);
> +
> +static ssize_t type_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	enum hp_wmi_keyboardtype type = fourzone_get_keyboard_type();
> +
> +	if (type != HPWMI_KEYBOARD_INVALID)

Reverse logic.

> +		return sprintf(buf, "%d\n", type - 1);

sysfs_emit()

> +
> +	return -EINVAL;
> +}
> +
> +/*
> + * Generic device attributes.
> + */
>  static DEVICE_ATTR_RO(display);
>  static DEVICE_ATTR_RO(hddtemp);
>  static DEVICE_ATTR_RW(als);
> @@ -797,7 +892,35 @@ static struct attribute *hp_wmi_attrs[] = {
>  	&dev_attr_postcode.attr,
>  	NULL,
>  };
> -ATTRIBUTE_GROUPS(hp_wmi);
> +
> +static struct attribute_group hp_wmi_group = {
> +	.attrs = hp_wmi_attrs,
> +};
> +
> +/*
> + * Omen fourzone specific device attributes.
> + */
> +static DEVICE_ATTR_RW(colors);
> +static DEVICE_ATTR_RW(mode);
> +static DEVICE_ATTR_RO(type);
> +
> +static struct attribute *hp_wmi_fourzone_attrs[] = {
> +	&dev_attr_colors.attr,
> +	&dev_attr_mode.attr,
> +	&dev_attr_type.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group hp_wmi_fourzone_group = {
> +	.attrs = hp_wmi_fourzone_attrs,
> +	.name = "kbd-backlight",
> +};
> +
> +static const struct attribute_group *hp_wmi_groups[] = {
> +	&hp_wmi_group,
> +	NULL,
> +	NULL,
> +};
>  
>  static void hp_wmi_notify(u32 value, void *context)
>  {
> @@ -1446,6 +1569,118 @@ static int thermal_profile_setup(void)
>  	return 0;
>  }
>  
> +static bool fourzone_supports_lighting(void)
> +{
> +	u8 buff[128];
> +
> +	int ret = hp_wmi_perform_query(HPWMI_SUPPORTS_LIGHTNING, HPWMI_FOURZONE,
> +								&buff, sizeof(buff), sizeof(buff));

Misaligned line, please fix all of these in your patch.

> +	/* the first bit in the response is set to 1 if the system supports lighting */
> +	if (ret == 0)
> +		return (buff[0] & 0x01) == 1;

Please name 0x01 with a #define. After doing that, I suspect the comment 
is unnecessary as the code is self-explaining.

Since you're returning bool, == 1 is unnecessary.

> +
> +	return false;
> +}
> +
> +static enum hp_wmi_keyboardtype fourzone_get_keyboard_type(void)
> +{
> +	u8 buff[128];
> +
> +	int ret = hp_wmi_perform_query(HPWMI_GET_KEYBOARD_TYPE, HPWMI_GM,
> +								&buff, sizeof(buff), sizeof(buff));
> +	/* the first byte in the response represents the keyborad type */
> +	if (ret == 0)
> +		return (enum hp_wmi_keyboardtype)(buff[0] + 1);
> +
> +	return HPWMI_KEYBOARD_INVALID;
> +}
> +
> +static int fourzone_get_colors(u32 *colors)
> +{
> +	u8 buff[128];
> +
> +	if (hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
> +							&buff, sizeof(buff), sizeof(buff)) == 0) {

Use ret variable and separate if ().

Again, reverse the logic so error is returned first so the for look can 
be with lower indentation level.

> +		for (int i = 0; i < 4; i++) {
> +			colors[3 - i] = ((buff[25 + i * 3]     & 0xFF) << 16)  // r
> +						  | ((buff[25 + i * 3 + 1] & 0xFF) <<  8)  // g
> +						  | ((buff[25 + i * 3 + 2] & 0xFF) <<  0); // b

Looks like #define XX GENMASK(x, y) + FIELD_PREP(XX, ) for me.

Don't forget to add #include for those macros.

> +		}
> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int fourzone_set_colors(u32 *colors)
> +{
> +	u8 buff[128];
> +
> +	if (hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
> +							&buff, sizeof(buff), sizeof(buff)) == 0) {

Same comments as above.

> +		for (int i = 0; i < 4; i++) {
> +			buff[25 + i * 3]	 = (colors[3 - i] >> 16) & 0xFF; // r
> +			buff[25 + i * 3 + 1] = (colors[3 - i] >>  8) & 0xFF; // g
> +			buff[25 + i * 3 + 2] = (colors[3 - i] >>  0) & 0xFF; // b

Named #defines + FIELD_GET().

> +		}
> +
> +		return hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_SET, HPWMI_FOURZONE,
> +						&buff, sizeof(buff), sizeof(buff));
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +/*
> + * Returns a negative number on error or 0/1 for the mode.
> + */
> +static int fourzone_get_mode(void)
> +{
> +	u8 buff[4];
> +
> +	int ret = hp_wmi_perform_query(HPWMI_FOURZONE_MODE_GET, HPWMI_FOURZONE,
> +								&buff, sizeof(buff), sizeof(buff));
> +
> +	if (ret == 0)

Don't declare ret on the same line as the call but move the declaration 
into declarations block.

Reverse the logic and keep function call and it's error handling together 
without extra lines in between them.

> +		return buff[0] == 228 ? 1 : 0;

Name 228 magic with a #define.

> +
> +	return ret;
> +}
> +
> +/*
> + * This device supports only two different modes:
> + * 1 -> lights on
> + * 0 -> lights off
> + */
> +static int fourzone_set_mode(u32 mode)
> +{
> +	u8 buff[4] = {mode ? 228 : 100, 0, 0, 0};

Magic numbers, name with #defines

> +
> +	return hp_wmi_perform_query(HPWMI_FOURZONE_MODE_SET, HPWMI_FOURZONE,
> +								&buff, sizeof(buff), 0);

Misaligned.

> +}
> +
> +static int fourzone_setup(struct platform_device *device)
> +{
> +	/* check if the system supports lighting */
> +	bool supports_lighting = fourzone_supports_lighting();
> +
> +	if (!supports_lighting)
> +		return -ENODEV;
> +
> +	/* check if we have a supported keyboard type */
> +	enum hp_wmi_keyboardtype kbd_type = fourzone_get_keyboard_type();

Don't declare variable within function like this but in the declaration 
block (unless needed by cleanup.h which is not the case here).

But I don't understand why you need either of these variables, they can 
be part of the if condition without a variable.

> +
> +	if (kbd_type != HPWMI_KEYBOARD_WITHOUT_NUMPAD)
> +		return -ENODEV;
> +
> +	/* register the new groups */
> +	hp_wmi_groups[1] = &hp_wmi_fourzone_group;
> +
> +	return 0;
> +}
> +
>  static int hp_wmi_hwmon_init(void);
>  
>  static int __init hp_wmi_bios_setup(struct platform_device *device)
> @@ -1475,6 +1710,9 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>  
>  	thermal_profile_setup();
>  
> +	/* setup 4 zone rgb, no problem if it fails */
> +	fourzone_setup(device);
> +
>  	return 0;
>  }
>  
> 

-- 
 i.

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

[Carlos Ferreira]

Hi, i have changed some of the code. How does it look now?

Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@gmail.com>
---
 drivers/platform/x86/hp/hp-wmi.c | 251 +++++++++++++++++++++++++++++--
 1 file changed, 241 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
index e53660422..8108ca7e9 100644
--- a/drivers/platform/x86/hp/hp-wmi.c
+++ b/drivers/platform/x86/hp/hp-wmi.c
@@ -27,6 +27,7 @@
 #include <linux/rfkill.h>
 #include <linux/string.h>
 #include <linux/dmi.h>
+#include <linux/bitfield.h>
 
 MODULE_AUTHOR("Matthew Garrett <mjg59@srcf.ucam.org>");
 MODULE_DESCRIPTION("HP laptop WMI hotkeys driver");
@@ -40,6 +41,10 @@ MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4");
 #define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
 #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
+
 /* 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
@@ -131,18 +136,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_FOURZONE_MODE_GET  = 0x04,
+	HPWMI_FOURZONE_MODE_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 {
@@ -652,6 +675,74 @@ static int hp_wmi_rfkill2_refresh(void)
 	return 0;
 }
 
+static int fourzone_get_colors(u32 *colors)
+{
+	int ret;
+	u8 buff[128];
+
+	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[3 - i] = FIELD_PREP(GENMASK(23, 16), buff[25 + i * 3])	   // r
+			      |	FIELD_PREP(GENMASK(15, 8),  buff[25 + i * 3 + 1])  // g
+			      | FIELD_PREP(GENMASK(7, 0),   buff[25 + i * 3 + 2]); // b
+	}
+
+	return 0;
+}
+
+static int fourzone_set_colors(u32 *colors)
+{
+	int ret;
+	u8 buff[128];
+
+	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++) {
+		buff[25 + i * 3]     = FIELD_GET(GENMASK(23, 16), colors[3 - i]); // r
+		buff[25 + i * 3 + 1] = FIELD_GET(GENMASK(15, 8),  colors[3 - i]); // g
+		buff[25 + i * 3 + 2] = FIELD_GET(GENMASK(7, 0),	  colors[3 - i]); // b
+	}
+
+	return hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_SET, HPWMI_FOURZONE,
+				    &buff, sizeof(buff), sizeof(buff));
+}
+
+/*
+ * Returns a negative number on error or 0/1 for the mode.
+ */
+static int fourzone_get_mode(void)
+{
+	int ret;
+	u8 buff[4];
+
+	ret = hp_wmi_perform_query(HPWMI_FOURZONE_MODE_GET, HPWMI_FOURZONE,
+				   &buff, sizeof(buff), sizeof(buff));
+	if (ret != 0)
+		return ret;
+
+	return buff[0] == FOURZONE_LIGHTING_ON ? 1 : 0;
+}
+
+/*
+ * This device supports only two different modes:
+ * 1 -> lights on
+ * 0 -> lights off
+ */
+static int fourzone_set_mode(u32 mode)
+{
+	u8 buff[4] = {mode ? FOURZONE_LIGHTING_ON : FOURZONE_LIGHTING_OFF, 0, 0, 0};
+
+	return hp_wmi_perform_query(HPWMI_FOURZONE_MODE_SET, HPWMI_FOURZONE,
+				    &buff, sizeof(buff), 0);
+}
+
 static ssize_t display_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
@@ -754,6 +845,58 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static ssize_t colors_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	int ret;
+	u32 colors[4];
+
+	ret = fourzone_get_colors(colors);
+	if (ret != 0)
+		return -EINVAL;
+
+	return sysfs_emit(buf, "%06x %06x %06x %06x\n", colors[0], colors[1], colors[2], colors[3]);
+}
+
+static ssize_t colors_store(struct device *dev, struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	int ret;
+	u32 colors[4];
+
+	ret = sscanf(buf, "%6x %6x %6x %6x", &colors[0], &colors[1], &colors[2], &colors[3]);
+	if (ret != 4)
+		return -EINVAL;
+
+	ret = fourzone_set_colors(colors);
+	return ret == 0 ? count : ret;
+}
+
+static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	u32 ret = fourzone_get_mode();
+
+	if (ret < 0)
+		return ret;
+
+	return sysfs_emit(buf, "%d\n", ret);
+}
+
+static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	int ret;
+	u32 mode;
+
+	ret = kstrtou32(buf, 10, &mode);
+	if (ret)
+		return ret;
+
+	ret = fourzone_set_mode(mode);
+	return ret == 0 ? count : ret;
+}
+
 static int camera_shutter_input_setup(void)
 {
 	int err;
@@ -781,6 +924,34 @@ static int camera_shutter_input_setup(void)
 	return err;
 }
 
+static enum hp_wmi_keyboardtype fourzone_get_keyboard_type(void)
+{
+	int ret;
+	u8 buff[128];
+
+	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 ssize_t type_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	enum hp_wmi_keyboardtype type = fourzone_get_keyboard_type();
+
+	if (type == HPWMI_KEYBOARD_INVALID)
+		return -EINVAL;
+
+	return sysfs_emit(buf, "%d\n", type - 1);
+}
+
+/*
+ * Generic device attributes.
+ */
 static DEVICE_ATTR_RO(display);
 static DEVICE_ATTR_RO(hddtemp);
 static DEVICE_ATTR_RW(als);
@@ -797,7 +968,35 @@ static struct attribute *hp_wmi_attrs[] = {
 	&dev_attr_postcode.attr,
 	NULL,
 };
-ATTRIBUTE_GROUPS(hp_wmi);
+
+static struct attribute_group hp_wmi_group = {
+	.attrs = hp_wmi_attrs,
+};
+
+/*
+ * Omen fourzone specific device attributes.
+ */
+static DEVICE_ATTR_RW(colors);
+static DEVICE_ATTR_RW(mode);
+static DEVICE_ATTR_RO(type);
+
+static struct attribute *hp_wmi_fourzone_attrs[] = {
+	&dev_attr_colors.attr,
+	&dev_attr_mode.attr,
+	&dev_attr_type.attr,
+	NULL,
+};
+
+static struct attribute_group hp_wmi_fourzone_group = {
+	.attrs = hp_wmi_fourzone_attrs,
+	.name = "kbd-backlight",
+};
+
+static const struct attribute_group *hp_wmi_groups[] = {
+	&hp_wmi_group,
+	NULL,
+	NULL,
+};
 
 static void hp_wmi_notify(u32 value, void *context)
 {
@@ -1446,6 +1645,35 @@ static int thermal_profile_setup(void)
 	return 0;
 }
 
+static bool fourzone_supports_lighting(void)
+{
+	int ret;
+	u8 buff[128];
+
+	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)
+{
+	/* check if the system supports lighting */
+	if (!fourzone_supports_lighting())
+		return -ENODEV;
+
+	/* check if we have a supported keyboard type */
+	if (fourzone_get_keyboard_type() != HPWMI_KEYBOARD_WITHOUT_NUMPAD)
+		return -ENODEV;
+
+	/* register the new groups */
+	hp_wmi_groups[1] = &hp_wmi_fourzone_group;
+
+	return 0;
+}
+
 static int hp_wmi_hwmon_init(void);
 
 static int __init hp_wmi_bios_setup(struct platform_device *device)
@@ -1475,6 +1703,9 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
 
 	thermal_profile_setup();
 
+	/* setup 4 zone rgb, no problem if it fails */
+	fourzone_setup(device);
+
 	return 0;
 }
 
-- 
2.44.0

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

[Ilpo Järvinen]

On Tue, 26 Mar 2024, Carlos Ferreira wrote:

> Hi, i have changed some of the code. How does it look now?
> 
> Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@gmail.com>
> ---

First of all, you need to make a proper submission with versioning, that 
is:

- Put version into the subject: PATCH v2
- Don't put extra stuff into changelog like the above question, if you 
need to ask something, put your question underneath the first --- line.
- List the changes you made underneath the first --- line (see ML 
archives for examples about formatting)

>  drivers/platform/x86/hp/hp-wmi.c | 251 +++++++++++++++++++++++++++++--
>  1 file changed, 241 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> index e53660422..8108ca7e9 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -27,6 +27,7 @@
>  #include <linux/rfkill.h>
>  #include <linux/string.h>
>  #include <linux/dmi.h>
> +#include <linux/bitfield.h>

Try to put it earlier, these should eventually be in alphabetic order 
(again, ordered by a separate patch, not this one).

>  MODULE_AUTHOR("Matthew Garrett <mjg59@srcf.ucam.org>");
>  MODULE_DESCRIPTION("HP laptop WMI hotkeys driver");
> @@ -40,6 +41,10 @@ MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4");
>  #define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
>  #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
> +
>  /* 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
> @@ -131,18 +136,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_FOURZONE_MODE_GET  = 0x04,
> +	HPWMI_FOURZONE_MODE_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 {
> @@ -652,6 +675,74 @@ static int hp_wmi_rfkill2_refresh(void)
>  	return 0;
>  }
>  
> +static int fourzone_get_colors(u32 *colors)
> +{
> +	int ret;
> +	u8 buff[128];
> +
> +	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[3 - i] = FIELD_PREP(GENMASK(23, 16), buff[25 + i * 3])	   // r
> +			      |	FIELD_PREP(GENMASK(15, 8),  buff[25 + i * 3 + 1])  // g
> +			      | FIELD_PREP(GENMASK(7, 0),   buff[25 + i * 3 + 2]); // b

Those GENMASK() calls should be done in #define and only used here:

#define FOURZONE_COLOR_R		GENMASK(23, 16)
#define FOURZONE_COLOR_G		GENMASK(15, 8)
#define FOURZONE_COLOR_B		GENMASK(7, 0)

You can then drop the comments because the code is self-explanatory.

Add #include <linux/bits.h>.

...But please see my comments about multicolor below.

> +	}
> +
> +	return 0;
> +}
> +
> +static int fourzone_set_colors(u32 *colors)
> +{
> +	int ret;
> +	u8 buff[128];
> +
> +	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++) {
> +		buff[25 + i * 3]     = FIELD_GET(GENMASK(23, 16), colors[3 - i]); // r
> +		buff[25 + i * 3 + 1] = FIELD_GET(GENMASK(15, 8),  colors[3 - i]); // g
> +		buff[25 + i * 3 + 2] = FIELD_GET(GENMASK(7, 0),	  colors[3 - i]); // b
> +	}
> +	return hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_SET, HPWMI_FOURZONE,
> +				    &buff, sizeof(buff), sizeof(buff));
> +}
> +
> +/*
> + * Returns a negative number on error or 0/1 for the mode.
> + */
> +static int fourzone_get_mode(void)
> +{
> +	int ret;
> +	u8 buff[4];

Try to use reverse xmas tree order where possible (go through the other 
functions too).

> +
> +	ret = hp_wmi_perform_query(HPWMI_FOURZONE_MODE_GET, HPWMI_FOURZONE,
> +				   &buff, sizeof(buff), sizeof(buff));
> +	if (ret != 0)
> +		return ret;
> +
> +	return buff[0] == FOURZONE_LIGHTING_ON ? 1 : 0;
> +}
> +
> +/*
> + * This device supports only two different modes:
> + * 1 -> lights on
> + * 0 -> lights off
> + */
> +static int fourzone_set_mode(u32 mode)
> +{
> +	u8 buff[4] = {mode ? FOURZONE_LIGHTING_ON : FOURZONE_LIGHTING_OFF, 0, 0, 0};
> +
> +	return hp_wmi_perform_query(HPWMI_FOURZONE_MODE_SET, HPWMI_FOURZONE,
> +				    &buff, sizeof(buff), 0);
> +}
> +
>  static ssize_t display_show(struct device *dev, struct device_attribute *attr,
>  			    char *buf)
>  {
> @@ -754,6 +845,58 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
>  	return count;
>  }
>  
> +static ssize_t colors_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	int ret;
> +	u32 colors[4];
> +
> +	ret = fourzone_get_colors(colors);
> +	if (ret != 0)
> +		return -EINVAL;
> +
> +	return sysfs_emit(buf, "%06x %06x %06x %06x\n", colors[0], colors[1], colors[2], colors[3]);
> +}
> +
> +static ssize_t colors_store(struct device *dev, struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	int ret;
> +	u32 colors[4];
> +
> +	ret = sscanf(buf, "%6x %6x %6x %6x", &colors[0], &colors[1], &colors[2], &colors[3]);
> +	if (ret != 4)
> +		return -EINVAL;

I now realize this should use leds multicolor API instead.

> +	ret = fourzone_set_colors(colors);
> +	return ret == 0 ? count : ret;
> +}
> +
> +static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	u32 ret = fourzone_get_mode();
> +
> +	if (ret < 0)

Please don't save one line like this, put the declaration on own 
line/declaration block so you can keep function call and it's error 
handling next to each other.

Also, compiler should have warned you here because u32 is not the correct 
type and you're checking for < 0!

> +		return ret;
> +
> +	return sysfs_emit(buf, "%d\n", ret);
> +}
> +
> +static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	int ret;
> +	u32 mode;
> +
> +	ret = kstrtou32(buf, 10, &mode);
> +	if (ret)
> +		return ret;
> +
> +	ret = fourzone_set_mode(mode);
> +	return ret == 0 ? count : ret;
> +}
> +
>  static int camera_shutter_input_setup(void)
>  {
>  	int err;
> @@ -781,6 +924,34 @@ static int camera_shutter_input_setup(void)
>  	return err;
>  }
>  
> +static enum hp_wmi_keyboardtype fourzone_get_keyboard_type(void)
> +{
> +	int ret;
> +	u8 buff[128];
> +
> +	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 ssize_t type_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	enum hp_wmi_keyboardtype type = fourzone_get_keyboard_type();
> +
> +	if (type == HPWMI_KEYBOARD_INVALID)
> +		return -EINVAL;
> +
> +	return sysfs_emit(buf, "%d\n", type - 1);

These are always positive, right? So %u is better.

> +}
> +
> +/*
> + * Generic device attributes.
> + */
>  static DEVICE_ATTR_RO(display);
>  static DEVICE_ATTR_RO(hddtemp);
>  static DEVICE_ATTR_RW(als);
> @@ -797,7 +968,35 @@ static struct attribute *hp_wmi_attrs[] = {
>  	&dev_attr_postcode.attr,
>  	NULL,
>  };
> -ATTRIBUTE_GROUPS(hp_wmi);
> +
> +static struct attribute_group hp_wmi_group = {
> +	.attrs = hp_wmi_attrs,
> +};
> +
> +/*
> + * Omen fourzone specific device attributes.
> + */
> +static DEVICE_ATTR_RW(colors);
> +static DEVICE_ATTR_RW(mode);
> +static DEVICE_ATTR_RO(type);
> +
> +static struct attribute *hp_wmi_fourzone_attrs[] = {
> +	&dev_attr_colors.attr,
> +	&dev_attr_mode.attr,
> +	&dev_attr_type.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group hp_wmi_fourzone_group = {
> +	.attrs = hp_wmi_fourzone_attrs,
> +	.name = "kbd-backlight",
> +};
> +
> +static const struct attribute_group *hp_wmi_groups[] = {
> +	&hp_wmi_group,
> +	NULL,
> +	NULL,
> +};
>  
>  static void hp_wmi_notify(u32 value, void *context)
>  {
> @@ -1446,6 +1645,35 @@ static int thermal_profile_setup(void)
>  	return 0;
>  }
>  
> +static bool fourzone_supports_lighting(void)
> +{
> +	int ret;
> +	u8 buff[128];
> +
> +	ret = hp_wmi_perform_query(HPWMI_SUPPORTS_LIGHTNING, HPWMI_FOURZONE,
> +				   &buff, sizeof(buff), sizeof(buff));
> +	if (ret != 0)

< 0 ?

-- 
 i.

> +		return false;
> +
> +	return buff[0] & FOURZONE_LIGHTING_SUPPORTED_BIT;
> +}
> +
> +static int fourzone_setup(struct platform_device *device)
> +{
> +	/* check if the system supports lighting */
> +	if (!fourzone_supports_lighting())
> +		return -ENODEV;
> +
> +	/* check if we have a supported keyboard type */
> +	if (fourzone_get_keyboard_type() != HPWMI_KEYBOARD_WITHOUT_NUMPAD)
> +		return -ENODEV;
> +
> +	/* register the new groups */
> +	hp_wmi_groups[1] = &hp_wmi_fourzone_group;
> +
> +	return 0;
> +}
> +
>  static int hp_wmi_hwmon_init(void);
>  
>  static int __init hp_wmi_bios_setup(struct platform_device *device)
> @@ -1475,6 +1703,9 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>  
>  	thermal_profile_setup();
>  
> +	/* setup 4 zone rgb, no problem if it fails */
> +	fourzone_setup(device);
> +
>  	return 0;
>  }
>  
> 

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

[Carlos Ferreira]

On 3/27/24 1:05 PM, Ilpo Järvinen wrote:

> On Tue, 26 Mar 2024, Carlos Ferreira wrote:
> 
>> Hi, i have changed some of the code. How does it look now?
>>
>> Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@gmail.com>
>> ---
> 
> First of all, you need to make a proper submission with versioning, that 
> is:
> 
> - Put version into the subject: PATCH v2
> - Don't put extra stuff into changelog like the above question, if you 
> need to ask something, put your question underneath the first --- line.
> - List the changes you made underneath the first --- line (see ML 
> archives for examples about formatting)

Should i submit the v2 patch with the current state or with the changes suggested such as the use of the leds API?

>>  drivers/platform/x86/hp/hp-wmi.c | 251 +++++++++++++++++++++++++++++--
>>  1 file changed, 241 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
>> index e53660422..8108ca7e9 100644
>> --- a/drivers/platform/x86/hp/hp-wmi.c
>> +++ b/drivers/platform/x86/hp/hp-wmi.c
>> @@ -27,6 +27,7 @@
>>  #include <linux/rfkill.h>
>>  #include <linux/string.h>
>>  #include <linux/dmi.h>
>> +#include <linux/bitfield.h>
> 
> Try to put it earlier, these should eventually be in alphabetic order 
> (again, ordered by a separate patch, not this one).

You mean organizing all the imports like this?
#include <linux/acpi.h>
#include <linux/bitfield.h>
#include <linux/dmi.h>
#include <linux/hwmon.h>
#include <linux/init.h>
#include <linux/input.h>
#include <linux/input/sparse-keymap.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/platform_profile.h>
#include <linux/rfkill.h>
#include <linux/slab.h>
#include <linux/string.h>
#include <linux/types.h>

>>  MODULE_AUTHOR("Matthew Garrett <mjg59@srcf.ucam.org>");
>>  MODULE_DESCRIPTION("HP laptop WMI hotkeys driver");
>> @@ -40,6 +41,10 @@ MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4");
>>  #define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
>>  #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
>> +
>>  /* 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
>> @@ -131,18 +136,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_FOURZONE_MODE_GET  = 0x04,
>> +	HPWMI_FOURZONE_MODE_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 {
>> @@ -652,6 +675,74 @@ static int hp_wmi_rfkill2_refresh(void)
>>  	return 0;
>>  }
>>  
>> +static int fourzone_get_colors(u32 *colors)
>> +{
>> +	int ret;
>> +	u8 buff[128];
>> +
>> +	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[3 - i] = FIELD_PREP(GENMASK(23, 16), buff[25 + i * 3])	   // r
>> +			      |	FIELD_PREP(GENMASK(15, 8),  buff[25 + i * 3 + 1])  // g
>> +			      | FIELD_PREP(GENMASK(7, 0),   buff[25 + i * 3 + 2]); // b
> 
> Those GENMASK() calls should be done in #define and only used here:
> 
> #define FOURZONE_COLOR_R		GENMASK(23, 16)
> #define FOURZONE_COLOR_G		GENMASK(15, 8)
> #define FOURZONE_COLOR_B		GENMASK(7, 0)
> 
> You can then drop the comments because the code is self-explanatory.
> 
> Add #include <linux/bits.h>.
> 
> ...But please see my comments about multicolor below.
> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int fourzone_set_colors(u32 *colors)
>> +{
>> +	int ret;
>> +	u8 buff[128];
>> +
>> +	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++) {
>> +		buff[25 + i * 3]     = FIELD_GET(GENMASK(23, 16), colors[3 - i]); // r
>> +		buff[25 + i * 3 + 1] = FIELD_GET(GENMASK(15, 8),  colors[3 - i]); // g
>> +		buff[25 + i * 3 + 2] = FIELD_GET(GENMASK(7, 0),	  colors[3 - i]); // b
>> +	}
>> +	return hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_SET, HPWMI_FOURZONE,
>> +				    &buff, sizeof(buff), sizeof(buff));
>> +}
>> +
>> +/*
>> + * Returns a negative number on error or 0/1 for the mode.
>> + */
>> +static int fourzone_get_mode(void)
>> +{
>> +	int ret;
>> +	u8 buff[4];
> 
> Try to use reverse xmas tree order where possible (go through the other 
> functions too).
> 
>> +
>> +	ret = hp_wmi_perform_query(HPWMI_FOURZONE_MODE_GET, HPWMI_FOURZONE,
>> +				   &buff, sizeof(buff), sizeof(buff));
>> +	if (ret != 0)
>> +		return ret;
>> +
>> +	return buff[0] == FOURZONE_LIGHTING_ON ? 1 : 0;
>> +}
>> +
>> +/*
>> + * This device supports only two different modes:
>> + * 1 -> lights on
>> + * 0 -> lights off
>> + */
>> +static int fourzone_set_mode(u32 mode)
>> +{
>> +	u8 buff[4] = {mode ? FOURZONE_LIGHTING_ON : FOURZONE_LIGHTING_OFF, 0, 0, 0};
>> +
>> +	return hp_wmi_perform_query(HPWMI_FOURZONE_MODE_SET, HPWMI_FOURZONE,
>> +				    &buff, sizeof(buff), 0);
>> +}
>> +
>>  static ssize_t display_show(struct device *dev, struct device_attribute *attr,
>>  			    char *buf)
>>  {
>> @@ -754,6 +845,58 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
>>  	return count;
>>  }
>>  
>> +static ssize_t colors_show(struct device *dev, struct device_attribute *attr,
>> +			    char *buf)
>> +{
>> +	int ret;
>> +	u32 colors[4];
>> +
>> +	ret = fourzone_get_colors(colors);
>> +	if (ret != 0)
>> +		return -EINVAL;
>> +
>> +	return sysfs_emit(buf, "%06x %06x %06x %06x\n", colors[0], colors[1], colors[2], colors[3]);
>> +}
>> +
>> +static ssize_t colors_store(struct device *dev, struct device_attribute *attr,
>> +				const char *buf, size_t count)
>> +{
>> +	int ret;
>> +	u32 colors[4];
>> +
>> +	ret = sscanf(buf, "%6x %6x %6x %6x", &colors[0], &colors[1], &colors[2], &colors[3]);
>> +	if (ret != 4)
>> +		return -EINVAL;
> 
> I now realize this should use leds multicolor API instead.
> 
>> +	ret = fourzone_set_colors(colors);
>> +	return ret == 0 ? count : ret;
>> +}
>> +
>> +static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
>> +			    char *buf)
>> +{
>> +	u32 ret = fourzone_get_mode();
>> +
>> +	if (ret < 0)
> 
> Please don't save one line like this, put the declaration on own 
> line/declaration block so you can keep function call and it's error 
> handling next to each other.
> 
> Also, compiler should have warned you here because u32 is not the correct 
> type and you're checking for < 0!
> 
>> +		return ret;
>> +
>> +	return sysfs_emit(buf, "%d\n", ret);
>> +}
>> +
>> +static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
>> +				const char *buf, size_t count)
>> +{
>> +	int ret;
>> +	u32 mode;
>> +
>> +	ret = kstrtou32(buf, 10, &mode);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = fourzone_set_mode(mode);
>> +	return ret == 0 ? count : ret;
>> +}
>> +
>>  static int camera_shutter_input_setup(void)
>>  {
>>  	int err;
>> @@ -781,6 +924,34 @@ static int camera_shutter_input_setup(void)
>>  	return err;
>>  }
>>  
>> +static enum hp_wmi_keyboardtype fourzone_get_keyboard_type(void)
>> +{
>> +	int ret;
>> +	u8 buff[128];
>> +
>> +	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 ssize_t type_show(struct device *dev, struct device_attribute *attr,
>> +			    char *buf)
>> +{
>> +	enum hp_wmi_keyboardtype type = fourzone_get_keyboard_type();
>> +
>> +	if (type == HPWMI_KEYBOARD_INVALID)
>> +		return -EINVAL;
>> +
>> +	return sysfs_emit(buf, "%d\n", type - 1);
> 
> These are always positive, right? So %u is better.
> 
>> +}
>> +
>> +/*
>> + * Generic device attributes.
>> + */
>>  static DEVICE_ATTR_RO(display);
>>  static DEVICE_ATTR_RO(hddtemp);
>>  static DEVICE_ATTR_RW(als);
>> @@ -797,7 +968,35 @@ static struct attribute *hp_wmi_attrs[] = {
>>  	&dev_attr_postcode.attr,
>>  	NULL,
>>  };
>> -ATTRIBUTE_GROUPS(hp_wmi);
>> +
>> +static struct attribute_group hp_wmi_group = {
>> +	.attrs = hp_wmi_attrs,
>> +};
>> +
>> +/*
>> + * Omen fourzone specific device attributes.
>> + */
>> +static DEVICE_ATTR_RW(colors);
>> +static DEVICE_ATTR_RW(mode);
>> +static DEVICE_ATTR_RO(type);
>> +
>> +static struct attribute *hp_wmi_fourzone_attrs[] = {
>> +	&dev_attr_colors.attr,
>> +	&dev_attr_mode.attr,
>> +	&dev_attr_type.attr,
>> +	NULL,
>> +};
>> +
>> +static struct attribute_group hp_wmi_fourzone_group = {
>> +	.attrs = hp_wmi_fourzone_attrs,
>> +	.name = "kbd-backlight",
>> +};
>> +
>> +static const struct attribute_group *hp_wmi_groups[] = {
>> +	&hp_wmi_group,
>> +	NULL,
>> +	NULL,
>> +};
>>  
>>  static void hp_wmi_notify(u32 value, void *context)
>>  {
>> @@ -1446,6 +1645,35 @@ static int thermal_profile_setup(void)
>>  	return 0;
>>  }
>>  
>> +static bool fourzone_supports_lighting(void)
>> +{
>> +	int ret;
>> +	u8 buff[128];
>> +
>> +	ret = hp_wmi_perform_query(HPWMI_SUPPORTS_LIGHTNING, HPWMI_FOURZONE,
>> +				   &buff, sizeof(buff), sizeof(buff));
>> +	if (ret != 0)
> 
> < 0 ?
> 

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

[Ilpo Järvinen]

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

On Wed, 27 Mar 2024, Carlos Ferreira wrote:

> On 3/27/24 1:05 PM, Ilpo Järvinen wrote:
> 
> > On Tue, 26 Mar 2024, Carlos Ferreira wrote:
> > 
> >> Hi, i have changed some of the code. How does it look now?
> >>
> >> Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@gmail.com>
> >> ---
> > 
> > First of all, you need to make a proper submission with versioning, that 
> > is:
> > 
> > - Put version into the subject: PATCH v2
> > - Don't put extra stuff into changelog like the above question, if you 
> > need to ask something, put your question underneath the first --- line.
> > - List the changes you made underneath the first --- line (see ML 
> > archives for examples about formatting)
> 
> Should i submit the v2 patch with the current state or with the changes 
> suggested such as the use of the leds API? 

Please send v3 once you've made the changes, there's no need to send 
patches which we know to become obsolete.

> >>  drivers/platform/x86/hp/hp-wmi.c | 251 +++++++++++++++++++++++++++++--
> >>  1 file changed, 241 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> >> index e53660422..8108ca7e9 100644
> >> --- a/drivers/platform/x86/hp/hp-wmi.c
> >> +++ b/drivers/platform/x86/hp/hp-wmi.c
> >> @@ -27,6 +27,7 @@
> >>  #include <linux/rfkill.h>
> >>  #include <linux/string.h>
> >>  #include <linux/dmi.h>
> >> +#include <linux/bitfield.h>
> > 
> > Try to put it earlier, these should eventually be in alphabetic order 
> > (again, ordered by a separate patch, not this one).
> 
> You mean organizing all the imports like this?
> #include <linux/acpi.h>
> #include <linux/bitfield.h>
> #include <linux/dmi.h>
> #include <linux/hwmon.h>
> #include <linux/init.h>
> #include <linux/input.h>
> #include <linux/input/sparse-keymap.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> #include <linux/platform_profile.h>
> #include <linux/rfkill.h>
> #include <linux/slab.h>
> #include <linux/string.h>
> #include <linux/types.h>

Yes.

If adding to a non-sorted list, the include should be placed before any 
entries that are alphabetically after it but providing another patch 
to sort them is of course even better than that.

-- 
 i.

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

[Hans de Goede]

Hi Carlos,

On 3/24/24 7:05 PM, Carlos Ferreira wrote:
> Added support for 4 zone keyboard rgb on omen laptops.
> 
> Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@gmail.com>

Thank you for your patch and sorry for being slow with replying to this.

There actually already was a previous attemp to add support for
the 4 zone keyboard to hp-wmi by Rishit Bansal:

https://lore.kernel.org/platform-driver-x86/20230131235027.36304-1-rishitbansal0@gmail.com/

As discussed there we really want to define a new standardized
userspace API for the backlight functionality of these zoned
RGB keyboards. Using driver specific sysfs attributes for this
is undesirable, since that will never get wide support in userspace.

OTOH if we define and document a new standard userspace API for this
then hopefully standard userspace stacks like KDE and GNOME will
eventually get support for this and then for the next zoned rgb
keyboard things will just work using the new standard API once
kernel support is merged.

I realize that using a single LED class device with kbd_backlight
in the name to tap into the existing userspace support to at least
control the overall backlight brightness is useful and tempting but

IMHO this really is a case where we need a new userspace API and then
emulating just a single brightness control for compatilbility with
existing userspace UI code can be done in powerdevil (KDE) or
upower (GNOME and others) in combination with offereing a more
complete DBUS API to also allow controlling the zones separately.

Recently another (laptop) driver for Casper Excalibur laptops has
been posted and this also include support for a 4 zone rgb keyboard:
https://lore.kernel.org/platform-driver-x86/20240324181201.87882-2-mustafa.eskieksi@gmail.com/

This driver actually already implements the userspace API proposed in
the discussion surrounding the earlier "[PATCH V3] platform/x86: hp-wmi:
Support omen backlight control wmi-acpi methods" patch.

This driver creates 4 LED class devices using the multi-color LED API
for RGB. One LED class device per zone. These are named:

casper:rgb:kbd_zoned_backlight-right
casper:rgb:kbd_zoned_backlight-middle
casper:rgb:kbd_zoned_backlight-left
casper:rgb:kbd_zoned_backlight-corners

Where as for the HP laptop case I believe the 4 multi-color LED
class devices should have the following names since the zones
are different:

hp:rgb:kbd_zoned_backlight-main
hp:rgb:kbd_zoned_backlight-wasd
hp:rgb:kbd_zoned_backlight-cursor
hp:rgb:kbd_zoned_backlight-numpad

As I mentioned in my review of the Casper Excalibur laptop driver
as part of adding support for these zoned rgb keyboards to various
laptop vendor specific drivers we should also document how these
devices are presented to userspace:

A separate patch needs to be written to add documentation about
the use of these names for zoned RGB backlit kbds in a new paragraph /
subsection of the "LED Device Naming" section of:

Documentation/leds/leds-class.rst 

And this should document at least the 2 currently known
zone sets:

:rgb:kbd_zoned_backlight-right
:rgb:kbd_zoned_backlight-middle
:rgb:kbd_zoned_backlight-left
:rgb:kbd_zoned_backlight-corners

:rgb:kbd_zoned_backlight-main
:rgb:kbd_zoned_backlight-wasd
:rgb:kbd_zoned_backlight-cursor
:rgb:kbd_zoned_backlight-numpad

with a comment that in the future different zone names are possible
if keyboards with a different zoning scheme show up.

Perhaps you can work together with Mustafa on writing a patch for:
Documentation/leds/leds-class.rst ?

for this and then hopefully Pavel can review + ack this patch
and then we can move forward with both the hp and the casper
laptop zoned rgb keyboard support.

Regards,

Hans




> ---
>  drivers/platform/x86/hp/hp-wmi.c | 308 +++++++++++++++++++++++++++----
>  1 file changed, 273 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> index e53660422..f420f41c7 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -103,7 +103,7 @@ enum hp_wmi_event_ids {
>  /*
>   * struct bios_args buffer is dynamically allocated.  New WMI command types
>   * were introduced that exceeds 128-byte data size.  Changes to handle
> - * the data size allocation scheme were kept in hp_wmi_perform_qurey function.
> + * the data size allocation scheme were kept in hp_wmi_perform_query function.
>   */
>  struct bios_args {
>  	u32 signature;
> @@ -114,15 +114,15 @@ struct bios_args {
>  };
>  
>  enum hp_wmi_commandtype {
> -	HPWMI_DISPLAY_QUERY		= 0x01,
> -	HPWMI_HDDTEMP_QUERY		= 0x02,
> -	HPWMI_ALS_QUERY			= 0x03,
> +	HPWMI_DISPLAY_QUERY			= 0x01,
> +	HPWMI_HDDTEMP_QUERY			= 0x02,
> +	HPWMI_ALS_QUERY				= 0x03,
>  	HPWMI_HARDWARE_QUERY		= 0x04,
>  	HPWMI_WIRELESS_QUERY		= 0x05,
> -	HPWMI_BATTERY_QUERY		= 0x07,
> -	HPWMI_BIOS_QUERY		= 0x09,
> -	HPWMI_FEATURE_QUERY		= 0x0b,
> -	HPWMI_HOTKEY_QUERY		= 0x0c,
> +	HPWMI_BATTERY_QUERY			= 0x07,
> +	HPWMI_BIOS_QUERY			= 0x09,
> +	HPWMI_FEATURE_QUERY			= 0x0b,
> +	HPWMI_HOTKEY_QUERY			= 0x0c,
>  	HPWMI_FEATURE2_QUERY		= 0x0d,
>  	HPWMI_WIRELESS2_QUERY		= 0x1b,
>  	HPWMI_POSTCODEERROR_QUERY	= 0x2a,
> @@ -131,18 +131,36 @@ enum hp_wmi_commandtype {
>  };
>  
>  enum hp_wmi_gm_commandtype {
> -	HPWMI_FAN_SPEED_GET_QUERY = 0x11,
> -	HPWMI_SET_PERFORMANCE_MODE = 0x1A,
> +	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_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_FOURZONE_MODE_GET  = 0x04,
> +	HPWMI_FOURZONE_MODE_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 {
> @@ -156,18 +174,18 @@ struct bios_return {
>  };
>  
>  enum hp_return_value {
> -	HPWMI_RET_WRONG_SIGNATURE	= 0x02,
> -	HPWMI_RET_UNKNOWN_COMMAND	= 0x03,
> -	HPWMI_RET_UNKNOWN_CMDTYPE	= 0x04,
> -	HPWMI_RET_INVALID_PARAMETERS	= 0x05,
> +	HPWMI_RET_WRONG_SIGNATURE	 = 0x02,
> +	HPWMI_RET_UNKNOWN_COMMAND	 = 0x03,
> +	HPWMI_RET_UNKNOWN_CMDTYPE	 = 0x04,
> +	HPWMI_RET_INVALID_PARAMETERS = 0x05,
>  };
>  
>  enum hp_wireless2_bits {
> -	HPWMI_POWER_STATE	= 0x01,
> -	HPWMI_POWER_SOFT	= 0x02,
> -	HPWMI_POWER_BIOS	= 0x04,
> -	HPWMI_POWER_HARD	= 0x08,
> -	HPWMI_POWER_FW_OR_HW	= HPWMI_POWER_BIOS | HPWMI_POWER_HARD,
> +	HPWMI_POWER_STATE	 = 0x01,
> +	HPWMI_POWER_SOFT	 = 0x02,
> +	HPWMI_POWER_BIOS	 = 0x04,
> +	HPWMI_POWER_HARD	 = 0x08,
> +	HPWMI_POWER_FW_OR_HW = HPWMI_POWER_BIOS | HPWMI_POWER_HARD,
>  };
>  
>  enum hp_thermal_profile_omen_v0 {
> @@ -177,22 +195,22 @@ enum hp_thermal_profile_omen_v0 {
>  };
>  
>  enum hp_thermal_profile_omen_v1 {
> -	HP_OMEN_V1_THERMAL_PROFILE_DEFAULT	= 0x30,
> -	HP_OMEN_V1_THERMAL_PROFILE_PERFORMANCE	= 0x31,
> -	HP_OMEN_V1_THERMAL_PROFILE_COOL		= 0x50,
> +	HP_OMEN_V1_THERMAL_PROFILE_DEFAULT	   = 0x30,
> +	HP_OMEN_V1_THERMAL_PROFILE_PERFORMANCE = 0x31,
> +	HP_OMEN_V1_THERMAL_PROFILE_COOL		   = 0x50,
>  };
>  
>  enum hp_thermal_profile_victus {
> -	HP_VICTUS_THERMAL_PROFILE_DEFAULT		= 0x00,
> -	HP_VICTUS_THERMAL_PROFILE_PERFORMANCE		= 0x01,
> -	HP_VICTUS_THERMAL_PROFILE_QUIET			= 0x03,
> +	HP_VICTUS_THERMAL_PROFILE_DEFAULT	  = 0x00,
> +	HP_VICTUS_THERMAL_PROFILE_PERFORMANCE = 0x01,
> +	HP_VICTUS_THERMAL_PROFILE_QUIET		  = 0x03,
>  };
>  
>  enum hp_thermal_profile {
> -	HP_THERMAL_PROFILE_PERFORMANCE	= 0x00,
> -	HP_THERMAL_PROFILE_DEFAULT		= 0x01,
> -	HP_THERMAL_PROFILE_COOL			= 0x02,
> -	HP_THERMAL_PROFILE_QUIET		= 0x03,
> +	HP_THERMAL_PROFILE_PERFORMANCE = 0x00,
> +	HP_THERMAL_PROFILE_DEFAULT	   = 0x01,
> +	HP_THERMAL_PROFILE_COOL		   = 0x02,
> +	HP_THERMAL_PROFILE_QUIET	   = 0x03,
>  };
>  
>  #define IS_HWBLOCKED(x) ((x & HPWMI_POWER_FW_OR_HW) != HPWMI_POWER_FW_OR_HW)
> @@ -754,6 +772,67 @@ static ssize_t postcode_store(struct device *dev, struct device_attribute *attr,
>  	return count;
>  }
>  
> +static int fourzone_get_colors(u32 *colors);
> +static int fourzone_set_colors(u32 *colors);
> +
> +static ssize_t colors_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	u32 colors[4];
> +
> +	/* read the colors from wmi and write them to the buf */
> +	if (fourzone_get_colors(colors) == 0)
> +		return sprintf(buf, "%06x %06x %06x %06x\n",
> +					colors[0], colors[1], colors[2], colors[3]);
> +
> +	return -EINVAL;
> +}
> +
> +static ssize_t colors_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	u32 colors[4];
> +
> +	if (sscanf(buf, "%6x %6x %6x %6x", &colors[0], &colors[1], &colors[2], &colors[3]) != 4)
> +		return -EINVAL;
> +
> +	/* set the colors */
> +	int ret = fourzone_set_colors(colors);
> +
> +	return ret == 0 ? count : ret;
> +}
> +
> +static int fourzone_get_mode(void);
> +static int fourzone_set_mode(u32 mode);
> +
> +static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	/* read the mode from wmi and write it to the buf */
> +	u32 ret = fourzone_get_mode();
> +
> +	if (ret >= 0)
> +		return sprintf(buf, "%d\n", ret);
> +
> +	return ret;
> +}
> +
> +static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	u32 mode;
> +
> +	int ret = kstrtou32(buf, 10, &mode);
> +
> +	if (ret)
> +		return ret;
> +
> +	/* set the mode */
> +	ret = fourzone_set_mode(mode);
> +
> +	return ret == 0 ? count : ret;
> +}
> +
>  static int camera_shutter_input_setup(void)
>  {
>  	int err;
> @@ -781,6 +860,22 @@ static int camera_shutter_input_setup(void)
>  	return err;
>  }
>  
> +static enum hp_wmi_keyboardtype fourzone_get_keyboard_type(void);
> +
> +static ssize_t type_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	enum hp_wmi_keyboardtype type = fourzone_get_keyboard_type();
> +
> +	if (type != HPWMI_KEYBOARD_INVALID)
> +		return sprintf(buf, "%d\n", type - 1);
> +
> +	return -EINVAL;
> +}
> +
> +/*
> + * Generic device attributes.
> + */
>  static DEVICE_ATTR_RO(display);
>  static DEVICE_ATTR_RO(hddtemp);
>  static DEVICE_ATTR_RW(als);
> @@ -797,7 +892,35 @@ static struct attribute *hp_wmi_attrs[] = {
>  	&dev_attr_postcode.attr,
>  	NULL,
>  };
> -ATTRIBUTE_GROUPS(hp_wmi);
> +
> +static struct attribute_group hp_wmi_group = {
> +	.attrs = hp_wmi_attrs,
> +};
> +
> +/*
> + * Omen fourzone specific device attributes.
> + */
> +static DEVICE_ATTR_RW(colors);
> +static DEVICE_ATTR_RW(mode);
> +static DEVICE_ATTR_RO(type);
> +
> +static struct attribute *hp_wmi_fourzone_attrs[] = {
> +	&dev_attr_colors.attr,
> +	&dev_attr_mode.attr,
> +	&dev_attr_type.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group hp_wmi_fourzone_group = {
> +	.attrs = hp_wmi_fourzone_attrs,
> +	.name = "kbd-backlight",
> +};
> +
> +static const struct attribute_group *hp_wmi_groups[] = {
> +	&hp_wmi_group,
> +	NULL,
> +	NULL,
> +};
>  
>  static void hp_wmi_notify(u32 value, void *context)
>  {
> @@ -1446,6 +1569,118 @@ static int thermal_profile_setup(void)
>  	return 0;
>  }
>  
> +static bool fourzone_supports_lighting(void)
> +{
> +	u8 buff[128];
> +
> +	int ret = hp_wmi_perform_query(HPWMI_SUPPORTS_LIGHTNING, HPWMI_FOURZONE,
> +								&buff, sizeof(buff), sizeof(buff));
> +	/* the first bit in the response is set to 1 if the system supports lighting */
> +	if (ret == 0)
> +		return (buff[0] & 0x01) == 1;
> +
> +	return false;
> +}
> +
> +static enum hp_wmi_keyboardtype fourzone_get_keyboard_type(void)
> +{
> +	u8 buff[128];
> +
> +	int ret = hp_wmi_perform_query(HPWMI_GET_KEYBOARD_TYPE, HPWMI_GM,
> +								&buff, sizeof(buff), sizeof(buff));
> +	/* the first byte in the response represents the keyborad type */
> +	if (ret == 0)
> +		return (enum hp_wmi_keyboardtype)(buff[0] + 1);
> +
> +	return HPWMI_KEYBOARD_INVALID;
> +}
> +
> +static int fourzone_get_colors(u32 *colors)
> +{
> +	u8 buff[128];
> +
> +	if (hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
> +							&buff, sizeof(buff), sizeof(buff)) == 0) {
> +		for (int i = 0; i < 4; i++) {
> +			colors[3 - i] = ((buff[25 + i * 3]     & 0xFF) << 16)  // r
> +						  | ((buff[25 + i * 3 + 1] & 0xFF) <<  8)  // g
> +						  | ((buff[25 + i * 3 + 2] & 0xFF) <<  0); // b
> +		}
> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int fourzone_set_colors(u32 *colors)
> +{
> +	u8 buff[128];
> +
> +	if (hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
> +							&buff, sizeof(buff), sizeof(buff)) == 0) {
> +		for (int i = 0; i < 4; i++) {
> +			buff[25 + i * 3]	 = (colors[3 - i] >> 16) & 0xFF; // r
> +			buff[25 + i * 3 + 1] = (colors[3 - i] >>  8) & 0xFF; // g
> +			buff[25 + i * 3 + 2] = (colors[3 - i] >>  0) & 0xFF; // b
> +		}
> +
> +		return hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_SET, HPWMI_FOURZONE,
> +						&buff, sizeof(buff), sizeof(buff));
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +/*
> + * Returns a negative number on error or 0/1 for the mode.
> + */
> +static int fourzone_get_mode(void)
> +{
> +	u8 buff[4];
> +
> +	int ret = hp_wmi_perform_query(HPWMI_FOURZONE_MODE_GET, HPWMI_FOURZONE,
> +								&buff, sizeof(buff), sizeof(buff));
> +
> +	if (ret == 0)
> +		return buff[0] == 228 ? 1 : 0;
> +
> +	return ret;
> +}
> +
> +/*
> + * This device supports only two different modes:
> + * 1 -> lights on
> + * 0 -> lights off
> + */
> +static int fourzone_set_mode(u32 mode)
> +{
> +	u8 buff[4] = {mode ? 228 : 100, 0, 0, 0};
> +
> +	return hp_wmi_perform_query(HPWMI_FOURZONE_MODE_SET, HPWMI_FOURZONE,
> +								&buff, sizeof(buff), 0);
> +}
> +
> +static int fourzone_setup(struct platform_device *device)
> +{
> +	/* check if the system supports lighting */
> +	bool supports_lighting = fourzone_supports_lighting();
> +
> +	if (!supports_lighting)
> +		return -ENODEV;
> +
> +	/* check if we have a supported keyboard type */
> +	enum hp_wmi_keyboardtype kbd_type = fourzone_get_keyboard_type();
> +
> +	if (kbd_type != HPWMI_KEYBOARD_WITHOUT_NUMPAD)
> +		return -ENODEV;
> +
> +	/* register the new groups */
> +	hp_wmi_groups[1] = &hp_wmi_fourzone_group;
> +
> +	return 0;
> +}
> +
>  static int hp_wmi_hwmon_init(void);
>  
>  static int __init hp_wmi_bios_setup(struct platform_device *device)
> @@ -1475,6 +1710,9 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>  
>  	thermal_profile_setup();
>  
> +	/* setup 4 zone rgb, no problem if it fails */
> +	fourzone_setup(device);
> +
>  	return 0;
>  }
>  

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

[Carlos Ferreira]

Hi, sorry for the (big) delay

> Hi Carlos,
> 
> On 3/24/24 7:05 PM, Carlos Ferreira wrote:
> > Added support for 4 zone keyboard rgb on omen laptops.
> > 
> > Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@gmail.com>
> 
> Thank you for your patch and sorry for being slow with replying to this.
> 
> There actually already was a previous attemp to add support for
> the 4 zone keyboard to hp-wmi by Rishit Bansal:
> 
> https://lore.kernel.org/platform-driver-x86/20230131235027.36304-1-rishitbansal0@gmail.com/
> 
> As discussed there we really want to define a new standardized
> userspace API for the backlight functionality of these zoned
> RGB keyboards. Using driver specific sysfs attributes for this
> is undesirable, since that will never get wide support in userspace.
> 
> OTOH if we define and document a new standard userspace API for this
> then hopefully standard userspace stacks like KDE and GNOME will
> eventually get support for this and then for the next zoned rgb
> keyboard things will just work using the new standard API once
> kernel support is merged.
> 
> I realize that using a single LED class device with kbd_backlight
> in the name to tap into the existing userspace support to at least
> control the overall backlight brightness is useful and tempting but
> 
> IMHO this really is a case where we need a new userspace API and then
> emulating just a single brightness control for compatilbility with
> existing userspace UI code can be done in powerdevil (KDE) or
> upower (GNOME and others) in combination with offereing a more
> complete DBUS API to also allow controlling the zones separately.
> 

That makes sense. I should post my first implementation using the
multicolor led api soon.

> Recently another (laptop) driver for Casper Excalibur laptops has
> been posted and this also include support for a 4 zone rgb keyboard:
> https://lore.kernel.org/platform-driver-x86/20240324181201.87882-2-mustafa.eskieksi@gmail.com/
> 
> This driver actually already implements the userspace API proposed in
> the discussion surrounding the earlier "[PATCH V3] platform/x86: hp-wmi:
> Support omen backlight control wmi-acpi methods" patch.
> 
> This driver creates 4 LED class devices using the multi-color LED API
> for RGB. One LED class device per zone. These are named:
> 
> casper:rgb:kbd_zoned_backlight-right
> casper:rgb:kbd_zoned_backlight-middle
> casper:rgb:kbd_zoned_backlight-left
> casper:rgb:kbd_zoned_backlight-corners
> 
> Where as for the HP laptop case I believe the 4 multi-color LED
> class devices should have the following names since the zones
> are different:
> 
> hp:rgb:kbd_zoned_backlight-main
> hp:rgb:kbd_zoned_backlight-wasd
> hp:rgb:kbd_zoned_backlight-cursor
> hp:rgb:kbd_zoned_backlight-numpad
> 

For this driver i think it should be something more like this:

hp:rgb:kbd_zoned_backlight-right
hp:rgb:kbd_zoned_backlight-middle
hp:rgb:kbd_zoned_backlight-left
hp:rgb:kbd_zoned_backlight-wasd

> As I mentioned in my review of the Casper Excalibur laptop driver
> as part of adding support for these zoned rgb keyboards to various
> laptop vendor specific drivers we should also document how these
> devices are presented to userspace:
> 
> A separate patch needs to be written to add documentation about
> the use of these names for zoned RGB backlit kbds in a new paragraph /
> subsection of the "LED Device Naming" section of:
> 
> Documentation/leds/leds-class.rst 
> 
> And this should document at least the 2 currently known
> zone sets:
> 
> :rgb:kbd_zoned_backlight-right
> :rgb:kbd_zoned_backlight-middle
> :rgb:kbd_zoned_backlight-left
> :rgb:kbd_zoned_backlight-corners
> 
> :rgb:kbd_zoned_backlight-main
> :rgb:kbd_zoned_backlight-wasd
> :rgb:kbd_zoned_backlight-cursor
> :rgb:kbd_zoned_backlight-numpad
> 
> with a comment that in the future different zone names are possible
> if keyboards with a different zoning scheme show up.
> 
> Perhaps you can work together with Mustafa on writing a patch for:
> Documentation/leds/leds-class.rst ?
> 

I am open to it if it was not done yet. But could you please
be a bit more specific about what exactly
needs to be documented about my patch?
Zone names, brightness control, userspace interaction?

> for this and then hopefully Pavel can review + ack this patch
> and then we can move forward with both the hp and the casper
> laptop zoned rgb keyboard support.
> 
> Regards,
> 
> Hans

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

[Hans de Goede]

Hi Carlos,

On 7/7/24 7:20 PM, Carlos Ferreira wrote:
> Hi, sorry for the (big) delay

No worries. I'm happy that you have managed to make some time
now to look further into this.

>> Hi Carlos,
>>
>> On 3/24/24 7:05 PM, Carlos Ferreira wrote:
>>> Added support for 4 zone keyboard rgb on omen laptops.
>>>
>>> Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@gmail.com>
>>
>> Thank you for your patch and sorry for being slow with replying to this.
>>
>> There actually already was a previous attemp to add support for
>> the 4 zone keyboard to hp-wmi by Rishit Bansal:
>>
>> https://lore.kernel.org/platform-driver-x86/20230131235027.36304-1-rishitbansal0@gmail.com/
>>
>> As discussed there we really want to define a new standardized
>> userspace API for the backlight functionality of these zoned
>> RGB keyboards. Using driver specific sysfs attributes for this
>> is undesirable, since that will never get wide support in userspace.
>>
>> OTOH if we define and document a new standard userspace API for this
>> then hopefully standard userspace stacks like KDE and GNOME will
>> eventually get support for this and then for the next zoned rgb
>> keyboard things will just work using the new standard API once
>> kernel support is merged.
>>
>> I realize that using a single LED class device with kbd_backlight
>> in the name to tap into the existing userspace support to at least
>> control the overall backlight brightness is useful and tempting but
>>
>> IMHO this really is a case where we need a new userspace API and then
>> emulating just a single brightness control for compatilbility with
>> existing userspace UI code can be done in powerdevil (KDE) or
>> upower (GNOME and others) in combination with offereing a more
>> complete DBUS API to also allow controlling the zones separately.
>>
> 
> That makes sense. I should post my first implementation using the
> multicolor led api soon.

I see that you already have done that, thank you.

>> Recently another (laptop) driver for Casper Excalibur laptops has
>> been posted and this also include support for a 4 zone rgb keyboard:
>> https://lore.kernel.org/platform-driver-x86/20240324181201.87882-2-mustafa.eskieksi@gmail.com/
>>
>> This driver actually already implements the userspace API proposed in
>> the discussion surrounding the earlier "[PATCH V3] platform/x86: hp-wmi:
>> Support omen backlight control wmi-acpi methods" patch.
>>
>> This driver creates 4 LED class devices using the multi-color LED API
>> for RGB. One LED class device per zone. These are named:
>>
>> casper:rgb:kbd_zoned_backlight-right
>> casper:rgb:kbd_zoned_backlight-middle
>> casper:rgb:kbd_zoned_backlight-left
>> casper:rgb:kbd_zoned_backlight-corners
>>
>> Where as for the HP laptop case I believe the 4 multi-color LED
>> class devices should have the following names since the zones
>> are different:
>>
>> hp:rgb:kbd_zoned_backlight-main
>> hp:rgb:kbd_zoned_backlight-wasd
>> hp:rgb:kbd_zoned_backlight-cursor
>> hp:rgb:kbd_zoned_backlight-numpad
>>
> 
> For this driver i think it should be something more like this:
> 
> hp:rgb:kbd_zoned_backlight-right
> hp:rgb:kbd_zoned_backlight-middle
> hp:rgb:kbd_zoned_backlight-left
> hp:rgb:kbd_zoned_backlight-wasd

Ok that sounds good.

>> As I mentioned in my review of the Casper Excalibur laptop driver
>> as part of adding support for these zoned rgb keyboards to various
>> laptop vendor specific drivers we should also document how these
>> devices are presented to userspace:
>>
>> A separate patch needs to be written to add documentation about
>> the use of these names for zoned RGB backlit kbds in a new paragraph /
>> subsection of the "LED Device Naming" section of:
>>
>> Documentation/leds/leds-class.rst 
>>
>> And this should document at least the 2 currently known
>> zone sets:
>>
>> :rgb:kbd_zoned_backlight-right
>> :rgb:kbd_zoned_backlight-middle
>> :rgb:kbd_zoned_backlight-left
>> :rgb:kbd_zoned_backlight-corners
>>
>> :rgb:kbd_zoned_backlight-main
>> :rgb:kbd_zoned_backlight-wasd
>> :rgb:kbd_zoned_backlight-cursor
>> :rgb:kbd_zoned_backlight-numpad
>>
>> with a comment that in the future different zone names are possible
>> if keyboards with a different zoning scheme show up.
>>
>> Perhaps you can work together with Mustafa on writing a patch for:
>> Documentation/leds/leds-class.rst ?
>>
> 
> I am open to it if it was not done yet. But could you please
> be a bit more specific about what exactly
> needs to be documented about my patch?
> Zone names, brightness control, userspace interaction?

Brightness control is part of the standard sysfs LED class API:

Documentation/ABI/testing/sysfs-class-led
Documentation/ABI/testing/sysfs-class-led-multicolor

So that is already documented and those files also specify
userspace interaction. 

So what needs to be documented is the existing practice
of using foo::kbd_backlight as sysfs LED class-device name for
keyboards with a single brightness / color setting. As well as
documenting the new API + naming for zoned RGB keyboards.

So basically add a new "Keyboard backlight control"
section to:

Documentation/leds/leds-class.rst

Under the "LED Device Naming" section with as contents e.g.:

"""
For backlit keyboards with a single brightness / color settings a
single (multicolor) LED device should be used to allow userspace
to change the backlight brightness (and if possible the color). 
This LED device must have a name ending in ':kbd_backlight'.

For RGB backlit keyboards with multiple control zones one multicolor
LED device should be used per zone. These LED devices' name
must follow the following form:

	"<devicename>:rgb:kbd_zoned_backlight-<zone_name>"

and <devicename> must be the same for zones of a single keyboard.

Where possible <zone_name> should be a value already used by other
zoned keyboards with a similar or identical zone layout, e.g.:

<devicename>:rgb:kbd_zoned_backlight-right
<devicename>:rgb:kbd_zoned_backlight-middle
<devicename>:rgb:kbd_zoned_backlight-left
<devicename>:rgb:kbd_zoned_backlight-corners
<devicename>:rgb:kbd_zoned_backlight-wasd

or:

<devicename>:rgb:kbd_zoned_backlight-main
<devicename>:rgb:kbd_zoned_backlight-cursor
<devicename>:rgb:kbd_zoned_backlight-numpad
<devicename>:rgb:kbd_zoned_backlight-corners
<devicename>:rgb:kbd_zoned_backlight-wasd

Note this is intended for keyboards with a limited number of zones,
keyboards with per key addressable backlighting must not use LED
class devices since the sysfs API is not suitable for rapidly change
multiple LEDs in one "commit" as is necessary to do animations /
special effects on such keyboards.
"""

Feel free to use the above example verbatim, although if you think
you can improve on this, then that would be great :)

Regards,

Hans