Message-ID: <5f206a6e-3daf-ca19-47bc-2dc40ca723cb@linux.intel.com>
Date: Mon, 12 Aug 2024 16:17:00 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Carlos Ferreira <carlosmiguelferreira.2003@...il.com>
cc: Hans de Goede <hdegoede@...hat.com>, mustafa.eskieksi@...il.com, 
    platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 1/2] HP: wmi: added support for 4 zone keyboard rgb

On Fri, 19 Jul 2024, Carlos Ferreira wrote:

> This driver adds supports for 4 zone keyboard rgb on omen laptops
> using the multicolor led api.
> 
> Tested on the HP Omen 15-en1001np.
> 
> Signed-off-by: Carlos Ferreira <carlosmiguelferreira.2003@...il.com>
> ---
>  drivers/platform/x86/hp/Kconfig  |   1 +
>  drivers/platform/x86/hp/hp-wmi.c | 282 ++++++++++++++++++++++++++++++-
>  2 files changed, 274 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp/Kconfig b/drivers/platform/x86/hp/Kconfig
> index 7fef4f12e498..6ce6d862ad05 100644
> --- a/drivers/platform/x86/hp/Kconfig
> +++ b/drivers/platform/x86/hp/Kconfig
> @@ -40,6 +40,7 @@ config HP_WMI
>  	depends on ACPI_WMI
>  	depends on INPUT
>  	depends on RFKILL || RFKILL = n
> +	select LEDS_CLASS_MULTICOLOR

Instead of select, use:

	depends on LEDS_CLASS_MULTICOLOR

It solves the build issue LKP found (dependencies of a selected CONFIG 
entry are not propagated which makes select tricky to use).

>  	select INPUT_SPARSEKMAP
>  	select ACPI_PLATFORM_PROFILE
>  	select HWMON
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> index 5fa553023842..b349f8325b93 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -14,7 +14,11 @@
>  #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/mutex.h>
> +#include <linux/mutex_types.h>
>  #include <linux/init.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
> @@ -24,6 +28,8 @@
>  #include <linux/platform_profile.h>
>  #include <linux/hwmon.h>
>  #include <linux/acpi.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
>  #include <linux/rfkill.h>
>  #include <linux/string.h>
>  #include <linux/dmi.h>
> @@ -44,6 +50,13 @@ MODULE_ALIAS("wmi:5FB7F034-2C63-45E9-BE91-3D44E2C707E4");
>  
>  #define zero_if_sup(tmp) (zero_insize_support?0:sizeof(tmp)) // use when zero insize is required
>  
> +#define FOURZONE_LIGHTING_SUPPORTED_BIT	0x01
> +#define FOURZONE_LIGHTING_ON		228
> +#define FOURZONE_LIGHTING_OFF		100
> +
> +#define FOURZONE_COLOR	GENMASK(7, 0)
> +#define KBD_ZONE_COUNT	4
> +
>  /* 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
> @@ -143,18 +156,36 @@ enum hp_wmi_commandtype {
>  };
>  
>  enum hp_wmi_gm_commandtype {
> -	HPWMI_FAN_SPEED_GET_QUERY = 0x11,
> -	HPWMI_SET_PERFORMANCE_MODE = 0x1A,
> -	HPWMI_FAN_SPEED_MAX_GET_QUERY = 0x26,
> -	HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
> -	HPWMI_GET_SYSTEM_DESIGN_DATA = 0x28,
> +	HPWMI_FAN_SPEED_GET_QUERY	= 0x11,
> +	HPWMI_SET_PERFORMANCE_MODE	= 0x1A,
> +	HPWMI_FAN_SPEED_MAX_GET_QUERY	= 0x26,
> +	HPWMI_FAN_SPEED_MAX_SET_QUERY	= 0x27,
> +	HPWMI_GET_SYSTEM_DESIGN_DATA	= 0x28,
> +	HPWMI_GET_KEYBOARD_TYPE		= 0x2B,
> +};
> +
> +enum hp_wmi_fourzone_commandtype {
> +	HPWMI_SUPPORTS_LIGHTNING	= 0x01,
> +	HPWMI_FOURZONE_COLOR_GET	= 0x02,
> +	HPWMI_FOURZONE_COLOR_SET	= 0x03,
> +	HPWMI_LED_BRIGHTNESS_GET	= 0x04,
> +	HPWMI_LED_BRIGHTNESS_SET	= 0x05,
> +};
> +
> +enum hp_wmi_keyboardtype {
> +	HPWMI_KEYBOARD_INVALID        = 0x00,
> +	HPWMI_KEYBOARD_NORMAL         = 0x01,
> +	HPWMI_KEYBOARD_WITH_NUMPAD    = 0x02,
> +	HPWMI_KEYBOARD_WITHOUT_NUMPAD = 0x03,
> +	HPWMI_KEYBOARD_RGB	      = 0x04,

Align with plain tabs, not with spaces after tabs.

>  };
>  
>  enum hp_wmi_command {
> -	HPWMI_READ	= 0x01,
> -	HPWMI_WRITE	= 0x02,
> -	HPWMI_ODM	= 0x03,
> -	HPWMI_GM	= 0x20008,
> +	HPWMI_READ     = 0x01,
> +	HPWMI_WRITE    = 0x02,
> +	HPWMI_ODM      = 0x03,
> +	HPWMI_GM       = 0x20008,

Why did you change these from tabs to spaces? I shouldn't need to touch 
these lines at all.

> +	HPWMI_FOURZONE = 0x20009,

Align with tabs

>  };
>  
>  enum hp_wmi_hardware_mask {
> @@ -821,6 +852,86 @@ static struct attribute *hp_wmi_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(hp_wmi);
>  
> +static const char * const fourzone_zone_names[KBD_ZONE_COUNT] = {
> +	"hp:rgb:kbd_zoned_backlight-right",
> +	"hp:rgb:kbd_zoned_backlight-middle",
> +	"hp:rgb:kbd_zoned_backlight-left",
> +	"hp:rgb:kbd_zoned_backlight-wasd"

Add comma to all non-terminating array entries.

> +};
> +
> +struct hp_fourzone_led {
> +	struct led_classdev_mc mc_led;
> +	struct mc_subled subleds[3];
> +	/*
> +	 * This stores the last set brightness level to restore it on off->on toggle
> +	 * by the FN-key combo.
> +	 */
> +	enum led_brightness brightness;

Noting that there's a parallel effort on (removing) led_brightness and 
turning it into an integer range (if I understood things correctly):

https://lore.kernel.org/all/CAM_RzfbuYYf7P2YK7H0BpUJut8hFvxa-Sm6hP1BKJe-jVFa62w@mail.gmail.com/

> +};
> +static struct hp_fourzone_led fourzone_leds[KBD_ZONE_COUNT];
> +static struct mutex fourzone_mutex;
> +
> +static enum led_brightness fourzone_get_hw_brightness(void)
> +{
> +	u8 buff[4];
> +
> +	hp_wmi_perform_query(HPWMI_LED_BRIGHTNESS_GET, HPWMI_FOURZONE, &buff,
> +			     sizeof(buff), sizeof(buff));

Error handling?

> +
> +	return buff[0] == FOURZONE_LIGHTING_ON ? LED_ON : LED_OFF;
> +}
> +
> +static int fourzone_set_colors(void)

This returns int but neither caller cares to check it?

> +{
> +	int ret, i, j;
> +	u8 buff[128];

Where does this 128 come from? Perhaps it should be named with a define?

> +
> +	ret = hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE, &buff,
> +				   sizeof(buff), sizeof(buff));
> +	if (ret != 0)
> +		return -EINVAL;

This is not a problem with input values (AFAICT) so it shouldn't return 
-EINVAL or something else. Perhaps -EIO?

> +
> +	for (i = 0; i < KBD_ZONE_COUNT; i++)
> +		for (j = 0; j < 3; j++)
> +			buff[25 + i * 3 + j] = fourzone_leds[i].subleds[j].brightness;

25 looks something that should be named with a define.

Also, there's lots of literal 3 in the code which would be nice to name 
properly.

> +
> +	return hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_SET, HPWMI_FOURZONE, &buff,
> +				    sizeof(buff), sizeof(buff));
> +}
> +
> +static void fourzone_set_state(void)
> +{
> +	enum led_brightness hw_brightness;
> +	int i;
> +
> +	mutex_lock(&fourzone_mutex);
> +
> +	hw_brightness = fourzone_get_hw_brightness();
> +
> +	if (hw_brightness)
> +		/* restore old brightness values */
> +		for (i = 0; i < KBD_ZONE_COUNT; i++) {
> +			fourzone_leds[i].mc_led.led_cdev.brightness = fourzone_leds[i].brightness;
> +			led_mc_calc_color_components(&fourzone_leds[i].mc_led,
> +						     fourzone_leds[i].brightness);
> +		}
> +	else
> +		for (i = 0; i < KBD_ZONE_COUNT; i++) {
> +			fourzone_leds[i].brightness = fourzone_leds[i].mc_led.led_cdev.brightness;
> +			fourzone_leds[i].mc_led.led_cdev.brightness = LED_OFF;
> +			led_mc_calc_color_components(&fourzone_leds[i].mc_led, LED_OFF);
> +		}

Please add the braces around these multiline if & else blocks as per the 
coding style.

> +
> +	fourzone_set_colors();
> +
> +	/* notify userspace about the change */
> +	for (i = 0; i < KBD_ZONE_COUNT; i++)
> +		led_classdev_notify_brightness_hw_changed(&fourzone_leds[i].mc_led.led_cdev,
> +							  hw_brightness);
> +
> +	mutex_unlock(&fourzone_mutex);
> +}
> +
>  static void hp_wmi_notify(u32 value, void *context)
>  {
>  	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> @@ -932,6 +1043,7 @@ static void hp_wmi_notify(u32 value, void *context)
>  	case HPWMI_PROXIMITY_SENSOR:
>  		break;
>  	case HPWMI_BACKLIT_KB_BRIGHTNESS:
> +		fourzone_set_state();
>  		break;
>  	case HPWMI_PEAKSHIFT_PERIOD:
>  		break;
> @@ -1505,6 +1617,155 @@ static int thermal_profile_setup(void)
>  	return 0;
>  }
>  
> +static void fourzone_set_brightness(struct led_classdev *led_cdev, enum led_brightness brightness)
> +{
> +	u8 buff[4] = { };
> +	int i, zone = 0;
> +	bool on = false;
> +
> +	for (i = 0; i < KBD_ZONE_COUNT; i++)
> +		if (!strcmp(led_cdev->name, fourzone_zone_names[i]))
> +			zone = i;
> +
> +	mutex_lock(&fourzone_mutex);
> +
> +	/* always update main and per color brightness values even when the backlight is off */
> +	fourzone_leds[zone].mc_led.led_cdev.brightness = brightness;
> +	led_mc_calc_color_components(&fourzone_leds[zone].mc_led, brightness);
> +	fourzone_set_colors();
> +
> +	for (i = 0; i < KBD_ZONE_COUNT; i++)
> +		if (fourzone_leds[i].mc_led.led_cdev.brightness)
> +			on = true;
> +
> +	/*
> +	 * This makes sure that when turning the kbd off with sw and back on with hw, there is a
> +	 * zone with brightness != 0 to go back to
> +	 */
> +	if (on)
> +		fourzone_leds[zone].brightness = brightness;
> +
> +	/* change the keyboard mode to off if all brightness values are set to 0 */
> +	buff[0] = on ? FOURZONE_LIGHTING_ON : FOURZONE_LIGHTING_OFF;
> +	hp_wmi_perform_query(HPWMI_LED_BRIGHTNESS_SET, HPWMI_FOURZONE, &buff, sizeof(buff), 0);
> +
> +	mutex_unlock(&fourzone_mutex);
> +}
> +
> +static int fourzone_get_hw_colors(u32 *colors)
> +{
> +	u8 buff[128];
> +	int ret, i;
> +
> +	ret = hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE, &buff,
> +				   sizeof(buff), sizeof(buff));
> +	if (ret != 0)
> +		return -EINVAL;
> +
> +	for (i = 0; i < KBD_ZONE_COUNT; i++) {
> +		colors[i * 3]     = FIELD_GET(FOURZONE_COLOR, buff[25 + i * 3]);
> +		colors[i * 3 + 1] = FIELD_GET(FOURZONE_COLOR, buff[25 + i * 3 + 1]);
> +		colors[i * 3 + 2] = FIELD_GET(FOURZONE_COLOR, buff[25 + i * 3 + 2]);

Use the named define for 25.

Why not use inner loop here like was used in the other place?

> +	}
> +
> +	return 0;
> +}
> +
> +static int __init fourzone_leds_init(struct platform_device *device)
> +{
> +	enum led_brightness hw_brightness;
> +	u32 colors[KBD_ZONE_COUNT * 3];
> +	int ret, i, j;
> +
> +	ret = fourzone_get_hw_colors(colors);
> +	if (ret < 0)
> +		return ret;
> +
> +	hw_brightness = fourzone_get_hw_brightness();
> +
> +	for (i = 0; i < KBD_ZONE_COUNT; i++) {
> +		for (j = 0; j < 3; j++)
> +			fourzone_leds[i].subleds[j] = (struct mc_subled) {
> +				.color_index = j + 1,
> +				.brightness = hw_brightness ? colors[i * 3 + j] : 0,
> +				.intensity = colors[i * 3 + j],
> +			};
> +
> +		fourzone_leds[i].mc_led = (struct led_classdev_mc) {
> +			.led_cdev = {
> +				.name = fourzone_zone_names[i],
> +				.brightness = hw_brightness ? 255 : 0,
> +				.max_brightness = 255,
> +				.brightness_set = fourzone_set_brightness,
> +				.color = LED_COLOR_ID_RGB,
> +				.flags = LED_BRIGHT_HW_CHANGED | LED_RETAIN_AT_SHUTDOWN,
> +			},
> +			.num_colors = 3,
> +			.subled_info = fourzone_leds[i].subleds,
> +		};
> +
> +		fourzone_leds[i].brightness = 255;
> +
> +		ret = devm_led_classdev_multicolor_register(&device->dev, &fourzone_leds[i].mc_led);
> +		if (ret)
> +			return -ENODEV;
> +	}
> +
> +	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));
> +	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));
> +	if (ret != 0)
> +		return false;
> +
> +	return buff[0] & FOURZONE_LIGHTING_SUPPORTED_BIT;
> +}
> +
> +static void fourzone_mutex_destroy(void *data)
> +{
> +	mutex_destroy((struct mutex *)data);
> +}
> +
> +static int fourzone_setup(struct platform_device *device)
> +{
> +	if (!fourzone_supports_lighting())
> +		return -ENODEV;
> +
> +	if (fourzone_get_keyboard_type() != HPWMI_KEYBOARD_WITHOUT_NUMPAD)
> +		return -ENODEV;
> +
> +	mutex_init(&fourzone_mutex);
> +	if (devm_add_action_or_reset(&hp_wmi_platform_dev->dev, fourzone_mutex_destroy,
> +				     &fourzone_mutex))

devm_mutex_init() should alleviate the need to define custom action for 
its destruction.

> +		return -ENODEV;
> +
> +	/* register leds */
> +	if (fourzone_leds_init(device) < 0)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
>  static int hp_wmi_hwmon_init(void);
>  
>  static int __init hp_wmi_bios_setup(struct platform_device *device)
> @@ -1534,6 +1795,9 @@ static int __init hp_wmi_bios_setup(struct platform_device *device)
>  
>  	thermal_profile_setup();
>  
> +	/* setup 4 zone rgb, no problem on error */
> +	fourzone_setup(device);
> +
>  	return 0;
>  }
>  
> 

-- 
 i.

Powered by blists - more mailing lists