Re: [PATCH] platform/x86: hp-wmi: add support for hp fourzone lighting

[Enver Balalic]

Hi

On Sun, Oct 24, 2021 at 04:50:48PM +0000, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2021. október 24., vasárnap 17:58 keltezéssel, Enver Balalic írta:
> 
> > This patch adds support for HP Fourzone lighting.
> > It exposes 2 things:
> >  - General keyboard backlight On/Off control
> >  - 4 RGB keyboard backlight zones as multicolor LED's
> >
> > This patch has been tested on a 2020 HP Omen 15 (AMD) 15-en0023dx.
> >
> > Signed-off-by: Enver Balalic <balalic.enver@xxxxxxxxx>
> > ---
> > There are a couple of things I'm unsure about with this patch:
> >  - I exposed the 4 RGB keyboard backlight zones with names
> >   "platform::kbd_backlight-1" up to 4, and the one general toggle
> >   is "platform::kbd_backlight" is this the proper naming
> >   scheme ?
> >
> >  - If the general keyboard backlight toggle is set to off, then
> >   no matter what values are written into any of the zones, the
> >   lights will be off, should we override the general backlight
> >   toggle to On if you write a value into one of the zones ?
> >
> >  - If the general keyboard backlight toggle is set to On,
> >   i set the zones brightness to 255, since it doesn't make sense
> >   for that to be 0 while the backlight is on. I didn't find
> >   an example of a multicolor led device having a brightness_get
> >   function registered, so I don't know if this is the proper way
> >   or if I should register a brightness_get function for each
> >   of the zones and somehow figure out the brightness based on
> >   the rgb value that the firmware reports ?
> >
> >  - The windows omen command center detects if you write zeros
> >   to all 4 of the zones (black) but you set the general keyboard
> >   backlight to On (as that might confuse someone into thinking
> >   that their backlight is not working when in fact it's just
> >   set to black) and overrides the black values to default values
> >   where each of the zones is set to a different color. Should we
> >   do the same in the kernel or leave that to some userspace tool ?
> >
> >
> >  drivers/platform/x86/Kconfig  |   3 +
> >  drivers/platform/x86/hp-wmi.c | 342 ++++++++++++++++++++++++++++++++++
> >  2 files changed, 345 insertions(+)
> >
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 1ebce7b775f2..07411fcd0d4b 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -427,6 +427,9 @@ config HP_WMI
> >  	select INPUT_SPARSEKMAP
> >  	select ACPI_PLATFORM_PROFILE
> >  	select HWMON
> > +	select LEDS_CLASS
> > +	select NEW_LEDS
> > +	select LEDS_CLASS_MULTICOLOR
> >  	help
> >  	 Say Y here if you want to support WMI-based hotkeys on HP laptops and
> >  	 to read data from WMI such as docking or ambient light sensor state.
> > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> > index 9c4c9f5095ae..87b4724d6b9f 100644
> > --- a/drivers/platform/x86/hp-wmi.c
> > +++ b/drivers/platform/x86/hp-wmi.c
> > @@ -27,6 +27,8 @@
> >  #include <linux/rfkill.h>
> >  #include <linux/string.h>
> >  #include <linux/dmi.h>
> > +#include <linux/leds.h>
> > +#include <linux/led-class-multicolor.h>
> >
> >  MODULE_AUTHOR("Matthew Garrett <mjg59@xxxxxxxxxxxxx>");
> >  MODULE_DESCRIPTION("HP laptop WMI hotkeys driver");
> > @@ -43,6 +45,14 @@ MODULE_PARM_DESC(enable_tablet_mode_sw, "Enable SW_TABLET_MODE reporting (-1=aut
> >  #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4"
> >  #define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
> >
> > +#define HP_FOURZONE_N_ZONES 4
> > +#define HP_FOURZONE_LED_NUM_CHANNELS 3
> > +#define HP_FOURZONE_OFFSET_COLORS 25
> > +#define HP_FOURZONE_BUF_LEN 37
> > +#define HP_FOURZONE_OFFSET_BRIGHTNESS 0
> > +#define HP_FOURZONE_KBD_BL_ON 128
> > +#define HP_FOURZONE_KBD_BL_OFF 0
> > +
> >  /* DMI board names of devices that should use the omen specific path for
> >   * thermal profiles.
> >   * This was obtained by taking a look in the windows omen command center
> > @@ -61,6 +71,19 @@ static const char * const omen_thermal_profile_boards[] = {
> >  	"8917", "8918", "8949", "894A", "89EB"
> >  };
> >
> > +/* DMI Board names of devices that have fourzone support.
> > + * A device supports fourzone if the "Feature" array in the json file
> > + * in windows omen command center contains "FourZone".
> > + */
> > +static const char * const fourzone_boards[] = {
> > +	"8466", "8467", "8468", "8469", "846A", "846B", "84DA", "84DB", "84DC",
> > +	"8574", "8575", "860A", "87B5", "8600", "8601", "8602", "8605", "8606",
> > +	"8607", "8746", "8747", "8749", "874A", "8603", "8604", "8748", "878A",
> > +	"878B", "878C", "88C8", "88CB", "8786", "8787", "8788", "88D1", "88D2",
> > +	"88F4", "88FD", "88F5", "88F6", "88F7", "88FE", "88FF", "8900", "8901",
> > +	"8902", "8912"
> > +};
> > +
> >  enum hp_wmi_radio {
> >  	HPWMI_WIFI	= 0x0,
> >  	HPWMI_BLUETOOTH	= 0x1,
> > @@ -117,11 +140,19 @@ enum hp_wmi_gm_commandtype {
> >  	HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
> >  };
> >
> > +enum hp_wmi_lm_commandtype {
> > +	HPWMI_FOURZONE_ZONES_GET	= 0x02,
> > +	HPWMI_FOURZONE_ZONES_SET	= 0x03,
> > +	HPWMI_FOURZONE_KB_BACKLIGHT_GET	= 0x04,
> > +	HPWMI_FOURZONE_KB_BACKLIGHT_SET	= 0x05,
> > +};
> > +
> >  enum hp_wmi_command {
> >  	HPWMI_READ	= 0x01,
> >  	HPWMI_WRITE	= 0x02,
> >  	HPWMI_ODM	= 0x03,
> >  	HPWMI_GM	= 0x20008,
> > +	HPWMI_LM	= 0x20009,
> >  };
> >
> >  enum hp_wmi_hardware_mask {
> > @@ -200,6 +231,32 @@ static const struct key_entry hp_wmi_keymap[] = {
> >  	{ KE_END, 0 }
> >  };
> >
> > +struct hp_wmi_fourzone_data {
> > +	struct {
> > +		unsigned int r;
> > +		unsigned int g;
> > +		unsigned int b;
> > +	} zone[HP_FOURZONE_N_ZONES];
> > +};
> > +
> > +struct hp_fourzone_zone {
> > +	bool initialized;
> > +	struct led_classdev_mc mc_cdev;
> > +	struct mc_subled subled_info[HP_FOURZONE_LED_NUM_CHANNELS];
> > +};
> > +
> > +struct hp_wmi_leds {
> > +	struct {
> > +		bool initialized;
> > +		struct led_classdev led;
> > +		unsigned int last_brightness;
> > +	} fourzone_kbd_bl;
> > +
> > +	struct {
> > +		struct hp_fourzone_zone zones[HP_FOURZONE_N_ZONES];
> > +	} fourzone;
> > +};
> > +
> >  static struct input_dev *hp_wmi_input_dev;
> >  static struct platform_device *hp_wmi_platform_dev;
> >  static struct platform_profile_handler platform_profile_handler;
> > @@ -208,6 +265,7 @@ static bool platform_profile_support;
> >  static struct rfkill *wifi_rfkill;
> >  static struct rfkill *bluetooth_rfkill;
> >  static struct rfkill *wwan_rfkill;
> > +static struct hp_wmi_leds *hp_leds;
> >
> >  struct rfkill2_device {
> >  	u8 id;
> > @@ -373,6 +431,18 @@ static int omen_thermal_profile_set(int mode)
> >  	return mode;
> >  }
> >
> > +static bool is_fourzone_supported(void)
> > +{
> > +	const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
> > +
> > +	if (!board_name)
> > +		return false;
> > +
> > +	return match_string(fourzone_boards,
> > +			    ARRAY_SIZE(fourzone_boards),
> > +			    board_name) >= 0;
> > +}
> > +
> >  static bool is_omen_thermal_profile(void)
> >  {
> >  	const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
> > @@ -653,6 +723,8 @@ static struct attribute *hp_wmi_attrs[] = {
> >  };
> >  ATTRIBUTE_GROUPS(hp_wmi);
> >
> > +static void hp_wmi_fourzone_kbd_bl_notify(void);
> > +
> >  static void hp_wmi_notify(u32 value, void *context)
> >  {
> >  	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> > @@ -754,6 +826,7 @@ static void hp_wmi_notify(u32 value, void *context)
> >  	case HPWMI_PROXIMITY_SENSOR:
> >  		break;
> >  	case HPWMI_BACKLIT_KB_BRIGHTNESS:
> > +		hp_wmi_fourzone_kbd_bl_notify();
> >  		break;
> >  	case HPWMI_PEAKSHIFT_PERIOD:
> >  		break;
> > @@ -1114,6 +1187,269 @@ static int platform_profile_set(struct platform_profile_handler *pprof,
> >  	return 0;
> >  }
> >
> > +static int hp_wmi_fourzone_get_zones_data(struct hp_wmi_fourzone_data *data)
> > +{
> > +	int ret, i, zone;
> > +	unsigned char buf[HP_FOURZONE_BUF_LEN];
> > +
> > +	ret = hp_wmi_perform_query(HPWMI_FOURZONE_ZONES_GET, HPWMI_LM,
> > +				   &buf, sizeof(buf),
> > +				   sizeof(buf));
> > +
> > +	if (ret < 0)
> > +		return ret;
> 
> Only a return value of 0 means success. See the comment above `hp_wmi_perform_query()`.
> 
> 
> > +
> > +	for (i = 0; i < HP_FOURZONE_N_ZONES; i++) {
> > +		zone = HP_FOURZONE_OFFSET_COLORS + (i * 3);
> > +
> > +		data->zone[i].r = buf[zone];
> > +		data->zone[i].g = buf[zone + 1];
> > +		data->zone[i].b = buf[zone + 2];
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int hp_wmi_fourzone_set_zones_data(struct hp_wmi_fourzone_data *data)
> > +{
> > +	int i, zone;
> > +	unsigned char buf[HP_FOURZONE_BUF_LEN];
> > +
> > +	for (i = 0; i < HP_FOURZONE_N_ZONES; i++) {
> > +		zone = HP_FOURZONE_OFFSET_COLORS + (i * 3);
> 
> Isn't it a problem that the first 25 bytes potentially contain random values?
> I think it would be better to explicitly clear them:
> 
>   unsigned char buf[HP_FOURZONE_BUF_LEN] = {0};
> 
> Maybe even in `hp_wmi_fourzone_get_zones_data()` as well.
The firmware only ever reads the offset 25 and up data from what I can see,
but yes there may be other laptops where random values actually break something.
I'll init both to zeros.
> 
> 
> > +
> > +		buf[zone] = data->zone[i].r;
> > +		buf[zone + 1] = data->zone[i].g;
> > +		buf[zone + 2] = data->zone[i].b;
> > +	}
> > +
> > +	return hp_wmi_perform_query(HPWMI_FOURZONE_ZONES_SET, HPWMI_LM,
> > +				       &buf, sizeof(buf),
> > +				       sizeof(buf));
> > +}
> > +
> > +static int hp_wmi_fourzone_set_zone_rgb(int zone, unsigned int r,
> > +					unsigned int g, unsigned int b)
> > +{
> > +	int err;
> > +	struct hp_wmi_fourzone_data data;
> > +
> > +	err = hp_wmi_fourzone_get_zones_data(&data);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	data.zone[zone].r = r;
> > +	data.zone[zone].g = g;
> > +	data.zone[zone].b = b;
> > +
> > +	return hp_wmi_fourzone_set_zones_data(&data);
> > +}
> > +
> > +static int hp_wmi_fourzone_cdev_set_zone(int zone, struct led_classdev *cdev,
> > +					 enum led_brightness brightness)
> > +{
> > +	struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
> > +
> > +	led_mc_calc_color_components(mc_cdev, brightness);
> > +
> > +	return hp_wmi_fourzone_set_zone_rgb(zone,
> > +		mc_cdev->subled_info[0].brightness,
> > +		mc_cdev->subled_info[1].brightness,
> > +		mc_cdev->subled_info[2].brightness);
> > +}
> > +
> > +static int hp_wmi_cdev_fourzone_led_set_brightness_zone0(struct led_classdev *cdev,
> > +							 enum led_brightness brightness)
> > +{
> > +	return hp_wmi_fourzone_cdev_set_zone(0, cdev, brightness);
> > +}
> > +
> > +static int hp_wmi_cdev_fourzone_led_set_brightness_zone1(struct led_classdev *cdev,
> > +							 enum led_brightness brightness)
> > +{
> > +	return hp_wmi_fourzone_cdev_set_zone(1, cdev, brightness);
> > +}
> > +
> > +static int hp_wmi_cdev_fourzone_led_set_brightness_zone2(struct led_classdev *cdev,
> > +							 enum led_brightness brightness)
> > +{
> > +	return hp_wmi_fourzone_cdev_set_zone(2, cdev, brightness);
> > +}
> > +
> > +static int hp_wmi_cdev_fourzone_led_set_brightness_zone3(struct led_classdev *cdev,
> > +							 enum led_brightness brightness)
> > +{
> > +	return hp_wmi_fourzone_cdev_set_zone(3, cdev, brightness);
> > +}
> > +
> > +static int hp_wmi_fourzone_kbd_bl_get(void)
> > +{
> > +	int val = 0, ret;
> > +
> > +	ret = hp_wmi_perform_query(HPWMI_FOURZONE_KB_BACKLIGHT_GET, HPWMI_LM,
> > +				   &val, sizeof(val), sizeof(val));
> > +
> > +	if (ret)
> > +		return ret < 0 ? ret : -EINVAL;
> > +
> > +	return val == HP_FOURZONE_KBD_BL_ON ? 1 : 0;
> > +}
> > +
> > +static int hp_wmi_fourzone_kbd_bl_set(unsigned int brightness)
> > +{
> > +	char kbd_bl_data[1] = { brightness ? HP_FOURZONE_KBD_BL_ON : HP_FOURZONE_KBD_BL_OFF };
> > +
> > +	int ret = hp_wmi_perform_query(HPWMI_FOURZONE_KB_BACKLIGHT_SET, HPWMI_LM,
> > +				       &kbd_bl_data, sizeof(kbd_bl_data),
> > +				       sizeof(kbd_bl_data));
> > +
> > +	if (ret != 0)
> > +		return -EINVAL;
> 
> The comment above `hp_wmi_perform_query()` says
> 
>    * returns zero on success
>    *         an HP WMI query specific error code (which is positive)
>    *         -EINVAL if the query was not successful at all
>    *         -EINVAL if the output buffer size exceeds buffersize
> 
> So I think you should return `ret` as is if it's negative, and return e.g. -EIO
> if it's positive. I am not sure `-EINVAL` is the best error code here.
> 
> 
> > +
> > +	hp_leds->fourzone_kbd_bl.last_brightness = brightness;
> > +
> > +	return brightness;
> > +}
> > +
> > +static enum led_brightness hp_wmi_cdev_fourzone_kbd_bl_get(struct led_classdev *led_cdev)
> > +{
> > +	return hp_wmi_fourzone_kbd_bl_get();
> > +}
> > +
> > +static int hp_wmi_cdev_fourzone_kbd_bl_set(struct led_classdev *led_cdev,
> > +					   enum led_brightness brightness)
> > +{
> > +	return hp_wmi_fourzone_kbd_bl_set(brightness);
> > +}
> > +
> > +static int (*fourzone_cdev_zone_set[HP_FOURZONE_N_ZONES])(struct led_classdev*,
> > +						  enum led_brightness) = {
> > +	&hp_wmi_cdev_fourzone_led_set_brightness_zone0,
> > +	&hp_wmi_cdev_fourzone_led_set_brightness_zone1,
> > +	&hp_wmi_cdev_fourzone_led_set_brightness_zone2,
> > +	&hp_wmi_cdev_fourzone_led_set_brightness_zone3,
> > +};
> 
> I think you can use `mc_led = lcdev_to_mccdev(...)` to get the multicolor LED,
> then use `zone = container_of(mc_led, struct hp_fourzone_zone, mc_cdev)`
> to get the `hp_fourzone_zone` struct, then `zone - hp_leds->fourzone.zones`
> to get the index. There is no need for these four functions as far as I can see.

Wasn't aware of the container_of macro, this works very well, thanks.
> 
> 
> > +
> > +static void hp_wmi_fourzone_kbd_bl_notify(void)
> > +{
> > +	int brightness;
> > +
> 
> If the keyboard backlight is not supported, then `hp_leds` remains NULL, right?
> Can this function be called in that case?
> 
> 
> > +	if (!hp_leds->fourzone_kbd_bl.initialized)
> > +		return;
> > +
> > +	brightness = hp_wmi_fourzone_kbd_bl_get();
> > +	if (brightness < 0)
> > +		return;
> > +
> > +	if (brightness == hp_leds->fourzone_kbd_bl.last_brightness)
> > +		return;
> > +
> > +	hp_leds->fourzone_kbd_bl.last_brightness = brightness;
> > +
> > +	led_classdev_notify_brightness_hw_changed(&hp_leds->fourzone_kbd_bl.led, brightness);
> > +}
> > +
> > +static void fourzone_leds_exit(struct platform_device *device)
> > +{
> > +	int i;
> > +	struct hp_fourzone_zone *zone;
> > +
> > +	if (hp_leds->fourzone_kbd_bl.initialized) {
> > +		hp_leds->fourzone_kbd_bl.initialized = false;
> > +		led_classdev_unregister(&hp_leds->fourzone_kbd_bl.led);
> > +	}
> > +
> > +	for (i = 0; i < HP_FOURZONE_N_ZONES; i++) {
> > +		zone = &hp_leds->fourzone.zones[i];
> > +		if (zone->initialized)
> > +			devm_led_classdev_multicolor_unregister(&device->dev,
> > +								&zone->mc_cdev);
> > +	}
> > +}
> > +
> > +static int fourzone_leds_setup(struct platform_device *device)
> > +{
> > +	int err, brightness, i, zones_brightness;
> > +	struct led_classdev *cdev;
> > +	struct hp_wmi_fourzone_data data;
> > +	struct hp_fourzone_zone *zone;
> > +	char name[64];
> > +
> > +	hp_leds = devm_kzalloc(&device->dev, sizeof(*hp_leds), GFP_KERNEL);
> > +	if (!hp_leds)
> > +		return -ENOMEM;
> 
> Have you considered not dynamically allocating this struct?
I did it this way because I saw the other static structs are also dynamically allocated
but yes this doesn't really need to be. I'll rework it.
> 
> 
> > +
> > +	brightness = hp_wmi_fourzone_kbd_bl_get();
> > +	if (brightness < 0)
> > +		return brightness;
> > +
> > +	hp_leds->fourzone_kbd_bl.last_brightness = brightness;
> > +
> > +	hp_leds->fourzone_kbd_bl.led.name = "platform::" LED_FUNCTION_KBD_BACKLIGHT;
> > +	hp_leds->fourzone_kbd_bl.led.brightness = brightness;
> > +	hp_leds->fourzone_kbd_bl.led.max_brightness = 1;
> > +	hp_leds->fourzone_kbd_bl.led.brightness_get = hp_wmi_cdev_fourzone_kbd_bl_get;
> > +	hp_leds->fourzone_kbd_bl.led.brightness_set_blocking = hp_wmi_cdev_fourzone_kbd_bl_set;
> > +	hp_leds->fourzone_kbd_bl.led.flags = LED_BRIGHT_HW_CHANGED | LED_RETAIN_AT_SHUTDOWN;
> > +
> > +	err = led_classdev_register(&device->dev, &hp_leds->fourzone_kbd_bl.led);
> > +	if (err)
> > +		return err;
> > +
> > +	hp_leds->fourzone_kbd_bl.initialized = true;
> > +
> > +	err = hp_wmi_fourzone_get_zones_data(&data);
> > +	if (err)
> > +		goto fail_fourzone;
> > +
> > +	zones_brightness = brightness == 1 ? 255 : 0;
> > +
> > +	for (i = 0; i < HP_FOURZONE_N_ZONES; i++) {
> > +		zone = &hp_leds->fourzone.zones[i];
> > +
> > +		zone->subled_info[0].color_index = LED_COLOR_ID_RED;
> > +		zone->subled_info[0].channel = 0;
> > +		zone->subled_info[0].intensity = data.zone[i].r;
> > +
> > +		zone->subled_info[1].color_index = LED_COLOR_ID_GREEN;
> > +		zone->subled_info[1].channel = 1;
> > +		zone->subled_info[1].intensity = data.zone[i].g;
> > +
> > +		zone->subled_info[2].color_index = LED_COLOR_ID_BLUE;
> > +		zone->subled_info[2].channel = 2;
> > +		zone->subled_info[2].intensity = data.zone[i].b;
> > +
> > +		zone->mc_cdev.subled_info = zone->subled_info;
> > +		zone->mc_cdev.num_colors = HP_FOURZONE_LED_NUM_CHANNELS;
> > +
> > +		cdev = &zone->mc_cdev.led_cdev;
> > +		cdev->flags = LED_RETAIN_AT_SHUTDOWN;
> > +		cdev->brightness = zones_brightness;
> > +		cdev->max_brightness = 255;
> > +		zone->mc_cdev.led_cdev.brightness_set_blocking = fourzone_cdev_zone_set[i];
> > +		snprintf(name, sizeof(name), "platform::%s-%i", LED_FUNCTION_KBD_BACKLIGHT, i + 1);
> > +		cdev->name = name;
> > +
> > +		err = devm_led_classdev_multicolor_register_ext(&device->dev,
> > +								&zone->mc_cdev,
> > +								NULL);
> > +
> > +		if (err) {
> > +			dev_err(&device->dev, "Cannot register fourzone zone %i LED: %i\n", i, err);
> > +			goto fail_fourzone;
> > +		}
> > +
> > +		zone->initialized = true;
> > +	}
> > +
> > +	return 0;
> > +
> > +fail_fourzone:
> > +	fourzone_leds_exit(device);
> > +
> > +	return err;
> > +}
> > +
> >  static int thermal_profile_setup(void)
> >  {
> >  	int err, tp;
> > @@ -1186,6 +1522,9 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
> >
> >  	thermal_profile_setup();
> >
> > +	if (is_fourzone_supported())
> > +		fourzone_leds_setup(device);
> > +
> >  	return 0;
> >  }
> >
> > @@ -1211,6 +1550,9 @@ static int __exit hp_wmi_bios_remove(struct platform_device *device)
> >  		rfkill_destroy(wwan_rfkill);
> >  	}
> >
> > +	if (hp_leds)
> > +		fourzone_leds_exit(device);
> > +
> >  	if (platform_profile_support)
> >  		platform_profile_remove();
> >
> > --
> > 2.33.1
> >
> 
> 
> Regards,
> Barnabás Pőcze

Ack on the rest.
Thank you very much for the review.

I'll wait a bit before sending out a V2 to see if someone from the LED subsystem 
will answer the questions I had.

Regards,
Enver.