| Message ID | 20240719100011.16656-2-carlosmiguelferreira.2003@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | HP: wmi: added support for 4 zone keyboard rgb | expand |
Hi Carlos,
kernel test robot noticed the following build warnings:
[auto build test WARNING on lee-leds/for-leds-next]
[also build test WARNING on v6.10]
[cannot apply to linus/master next-20240724]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Carlos-Ferreira/HP-wmi-added-support-for-4-zone-keyboard-rgb/20240719-180603
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git for-leds-next
patch link: https://lore.kernel.org/r/20240719100011.16656-2-carlosmiguelferreira.2003%40gmail.com
patch subject: [PATCH v4 1/2] HP: wmi: added support for 4 zone keyboard rgb
config: i386-kismet-CONFIG_LEDS_CLASS_MULTICOLOR-CONFIG_HP_WMI-0-0 (https://download.01.org/0day-ci/archive/20240725/202407251136.aymIqEw3-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20240725/202407251136.aymIqEw3-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407251136.aymIqEw3-lkp@intel.com/
kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for LEDS_CLASS_MULTICOLOR when selected by HP_WMI
WARNING: unmet direct dependencies detected for LEDS_CLASS_MULTICOLOR
Depends on [n]: NEW_LEDS [=n] && LEDS_CLASS [=n]
Selected by [y]:
- HP_WMI [=y] && X86_PLATFORM_DEVICES [=y] && X86_PLATFORM_DRIVERS_HP [=y] && ACPI_WMI [=y] && INPUT [=y] && (RFKILL [=y] || RFKILL [=y]=n)
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@gmail.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; > } > >
Hi, Thank you for the new version, much better, almost there I would say. On 7/19/24 11:59 AM, 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@gmail.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 As pointed out by the kernel test robot, LEDS_COLOR_MULTICOLOR is a symbol which for which you should use "depends on" not select. Note that in general when adding new dependencies it is a good idea to grep for them in the kernel tree and see what other consumers of the dependency are doing. Generally speaking either all existing consumers will all use "depends on" (which is the case here), or they will all use select and you should follow the example of the existing consumers to avoid things like circular dependency issues. Note sometimes exiting use unfortunately is inconsistent. > 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, > }; > > 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, > + HPWMI_FOURZONE = 0x20009, > }; > > 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" > +}; > + > +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; As Ilpo also just mentioned please make this a regular "int" since the LED subsystem is working towards replacing "enum led_brightness" with plain int, see: 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)); > + > + return buff[0] == FOURZONE_LIGHTING_ON ? LED_ON : LED_OFF; > +} Please make the return type a u8 and just return buff[0]. and then (continued below after the mutex remark) ... > + > +static int fourzone_set_colors(void) > +{ > + int ret, i, j; > + u8 buff[128]; > + > + 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++) > + for (j = 0; j < 3; j++) > + buff[25 + i * 3 + j] = fourzone_leds[i].subleds[j].brightness; > + > + 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); Please add "#include <linux/cleanup.h>" to the includes and use guard(mutex)(&fourzone_mutex); here instead. This will auto-unlock on leaving the function, so you can then drop the mutex_unlock() below and if any error exit (early return) paths get added later those cannot forget to unlock the mutex since this is done automatically. > + > + hw_brightness = fourzone_get_hw_brightness(); > + Make the type of hw_brightness a u8 and replace this line: > + if (hw_brightness) With: if (hw_brightness == FOURZONE_LIGHTING_ON) this avoids the need to translate the hw specific values into some other range first. > + /* 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); > + } > + > + 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); Replace this with: guard(mutex)(&fourzone_mutex); as discussed above. > + > + /* 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]); > + } > + > + 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, I think it would be cleaner to drop setting subled brightness here and instead call led_mc_calc_color_components() below ... : > + .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; > + }; > + With this all setup, you can now call: led_mc_calc_color_components(&fourzone_leds[i].mc_led, fourzone_leds[i].mc_led.led_cdev.brightness); here, this makes how the subled brightness is set here (on init) identical with how it is done on set_brightness calls which is more consistent. > + fourzone_leds[i].brightness = 255; > + > + ret = devm_led_classdev_multicolor_register(&device->dev, &fourzone_leds[i].mc_led); > + if (ret) > + return -ENODEV; > + } > + > + return 0; > +} <snip> Regards, Hans p.s. Please also address all the comments from Ilpo's review for v5.
On Mon, 12 Aug 2024, Hans de Goede wrote: > Hi, > > Thank you for the new version, much better, almost there I would say. > > On 7/19/24 11:59 AM, 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@gmail.com> > > +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, > > I think it would be cleaner to drop setting subled brightness here and instead > call led_mc_calc_color_components() below ... : > > > + .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; > > + }; > > + > > With this all setup, you can now call: > > led_mc_calc_color_components(&fourzone_leds[i].mc_led, fourzone_leds[i].mc_led.led_cdev.brightness); One additional thing, having a temporary variable for fourzone_leds[i] would be advicable to reduce the line lengths / complexity of the expressions. > here, this makes how the subled brightness is set here (on init) identical with > how it is done on set_brightness calls which is more consistent. > > > + fourzone_leds[i].brightness = 255; > > + > > + ret = devm_led_classdev_multicolor_register(&device->dev, &fourzone_leds[i].mc_led); > > + if (ret) > > + return -ENODEV; > > + } > > + > > + return 0; > > +}
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 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, }; 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, + HPWMI_FOURZONE = 0x20009, }; 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" +}; + +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; +}; +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)); + + return buff[0] == FOURZONE_LIGHTING_ON ? LED_ON : LED_OFF; +} + +static int fourzone_set_colors(void) +{ + int ret, i, j; + u8 buff[128]; + + 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++) + for (j = 0; j < 3; j++) + buff[25 + i * 3 + j] = fourzone_leds[i].subleds[j].brightness; + + 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); + } + + 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]); + } + + 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)) + 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; }
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@gmail.com> --- drivers/platform/x86/hp/Kconfig | 1 + drivers/platform/x86/hp/hp-wmi.c | 282 ++++++++++++++++++++++++++++++- 2 files changed, 274 insertions(+), 9 deletions(-)