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 <[email protected]>
---
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 <[email protected]>");
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;
+
+ 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);
+
+ 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;
+
+ 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,
+};
+
+static void hp_wmi_fourzone_kbd_bl_notify(void)
+{
+ int brightness;
+
+ 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;
+
+ 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
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 <[email protected]>
> ---
> 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 <[email protected]>");
> 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
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 <[email protected]>
> > ---
> > 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 <[email protected]>");
> > 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.