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@xxxxxxxxx>
> ---

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@xxxxxxxxxxxxx>");
>  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;
>  }
>  
>