Date:   Sun, 24 Oct 2021 16:50:48 +0000
From:   Barnabás Pőcze <pobrn@...tonmail.com>
To:     Enver Balalic <balalic.enver@...il.com>
Cc:     hdegoede@...hat.com, mgross@...ux.intel.com, pavel@....cz,
        platform-driver-x86@...r.kernel.org, linux-leds@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/x86: hp-wmi: add support for hp fourzone lighting

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@...il.com>
> ---
> 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@...f.ucam.org>");
>  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.


> +
> +		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.


> +
> +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?


> +
> +	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

Powered by blists - more mailing lists