Re: [PATCH v3 1/1] HP: wmi: added support for 4 zone keyboard rgb

[Ilpo Järvinen]

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.