On Sun, 7 Jul 2024, Carlos Ferreira wrote:
> This driver adds supports for 4 zone keyboard rgb on omen laptops
> and maps the wmi backlight toggle event to KEY_KBDILLUMTOGGLE.
> For the backlight, it uses the multicolor led api.
>
> Tested on the HP Omen 15-en1001np.
>
> Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@xxxxxxxxx>
> ---
> Changes in v3:
> - Moved to the multicolor led api
> - Mapped the wmi backlight toggle event to KEY_KBDILLUMTOGGLE
> - Some other minor changes
> Changes in v2:
> - Rearranged code to remove forward declarations
> - Changed from sprintf() to sysfs_emit()
> - Fixed some identation and coding style problems
> - Switched from manual bit manipulation to GENMASK(x, y) + FIELD_PREP(XX, )
> - #define'ed magic constants
> ---
> drivers/platform/x86/hp/hp-wmi.c | 248 +++++++++++++++++++++++++++++--
> 1 file changed, 239 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> index 5fa553023842..5eae47961f76 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -14,6 +14,8 @@
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> #include <linux/kernel.h>
> +#include <linux/led-class-multicolor.h>
> +#include <linux/leds.h>
> #include <linux/module.h>
> #include <linux/init.h>
> #include <linux/slab.h>
> @@ -24,6 +26,7 @@
> #include <linux/platform_profile.h>
> #include <linux/hwmon.h>
> #include <linux/acpi.h>
> +#include <linux/bits.h>
You need to add #include <linux/bitfield.h> as LKP found out. The
MODULE_DESCRIPTION() ones you can ignore as LKP unfortunately spams some
kernel-wide problems to all patches even if they're entirely unrelated
to the patch in question.
> + for (size_t i = 0; i < 4; i++)
> + fourzone_leds.leds[i].led_cdev.brightness = br;
> + for (int i = 0; i < 4; i++) {
> + colors[i] = FIELD_PREP(FOURZONE_COLOR_R, buff[25 + i * 3])
> + | FIELD_PREP(FOURZONE_COLOR_G, buff[25 + i * 3 + 1])
> + | FIELD_PREP(FOURZONE_COLOR_B, buff[25 + i * 3 + 2]);
The loop variables are not particularly consistent... I'd prefer them to
be declared in the normal function variable declaration block.
> +
> + for (size_t i = 0; i < 4; i++)
> + if (strcmp(led_cdev->name, fourzone_zone_names[i]) == 0)
!strcmp()
> + ret = fourzone_get_colors(colors);
> + if (ret < 0)
> + return ret;
> +
> + memcpy(fourzone_leds.color_cache, colors, sizeof(colors));
> +
> + brightness = get_fourzone_brightness(NULL);
> +
> + for (size_t i = 0; i < 4; i++) {
> + fourzone_leds.subleds[i] = (struct mc_subled) {
> + .color_index = LED_COLOR_ID_RGB,
> + .brightness = 1,
> + .intensity = colors[i]
Please add comma to the last line because if this is ever extended, this
line doesn't need to be touched.
> + };
> +
> + fourzone_leds.leds[i] = (struct led_classdev_mc) {
> + .led_cdev = {
> + .name = fourzone_zone_names[i],
> + .brightness = brightness,
> + .max_brightness = 1,
> + .brightness_set = set_fourzone_brightness,
> + .brightness_get = get_fourzone_brightness,
> + .color = LED_COLOR_ID_RGB,
> + .flags = LED_BRIGHT_HW_CHANGED
Ditto.
> + },
> + .num_colors = 1,
> + .subled_info = &fourzone_leds.subleds[i]
Ditto.
> + return 0;
> +}
> +
> +static enum hp_wmi_keyboardtype fourzone_get_keyboard_type(void)
> +{
> + u8 buff[128];
> + int ret;
> +
> + ret = hp_wmi_perform_query(HPWMI_GET_KEYBOARD_TYPE, HPWMI_GM,
> + &buff, sizeof(buff), sizeof(buff));
Align the continuation of parameters to the opening (.
> + 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 bool fourzone_supports_lighting(void)
> +{
> + u8 buff[128];
> + int ret;
> +
> + ret = hp_wmi_perform_query(HPWMI_SUPPORTS_LIGHTNING, HPWMI_FOURZONE,
> + &buff, sizeof(buff), sizeof(buff));
Ditto.
I probably missed a few similar cases as mentioned above so please go
through your patch and correct them all.
I'm left unsure if a mutex would be necessary to protect the brightness
updates. Probably.
Hans might have more to say about the approach.
--
i.
