* Re: [PATCH v4 1/2] HP: wmi: added support for 4 zone keyboard rgb
2024-07-19 9:59 ` [PATCH v4 1/2] " Carlos Ferreira
@ 2024-07-25 3:40 ` kernel test robot
2024-08-12 13:17 ` Ilpo Järvinen
2024-08-12 13:44 ` Hans de Goede
2 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2024-07-25 3:40 UTC (permalink / raw)
To: Carlos Ferreira, hdegoede, ilpo.jarvinen
Cc: Paul Gazzillo, Necip Fazil Yildiran, oe-kbuild-all,
mustafa.eskieksi, platform-driver-x86, linux-kernel,
Carlos Ferreira
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)
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v4 1/2] HP: wmi: added support for 4 zone keyboard rgb
2024-07-19 9:59 ` [PATCH v4 1/2] " Carlos Ferreira
2024-07-25 3:40 ` kernel test robot
@ 2024-08-12 13:17 ` Ilpo Järvinen
2024-08-12 13:44 ` Hans de Goede
2 siblings, 0 replies; 9+ messages in thread
From: Ilpo Järvinen @ 2024-08-12 13:17 UTC (permalink / raw)
To: Carlos Ferreira
Cc: Hans de Goede, mustafa.eskieksi, platform-driver-x86, LKML
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;
> }
>
>
--
i.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v4 1/2] HP: wmi: added support for 4 zone keyboard rgb
2024-07-19 9:59 ` [PATCH v4 1/2] " Carlos Ferreira
2024-07-25 3:40 ` kernel test robot
2024-08-12 13:17 ` Ilpo Järvinen
@ 2024-08-12 13:44 ` Hans de Goede
2024-08-12 15:57 ` Ilpo Järvinen
2 siblings, 1 reply; 9+ messages in thread
From: Hans de Goede @ 2024-08-12 13:44 UTC (permalink / raw)
To: Carlos Ferreira, ilpo.jarvinen
Cc: mustafa.eskieksi, platform-driver-x86, linux-kernel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v4 1/2] HP: wmi: added support for 4 zone keyboard rgb
2024-08-12 13:44 ` Hans de Goede
@ 2024-08-12 15:57 ` Ilpo Järvinen
0 siblings, 0 replies; 9+ messages in thread
From: Ilpo Järvinen @ 2024-08-12 15:57 UTC (permalink / raw)
To: Carlos Ferreira
Cc: mustafa.eskieksi, Hans de Goede, platform-driver-x86, LKML
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;
> > +}
--
i.
^ permalink raw reply [flat|nested] 9+ messages in thread