> > 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>
>
> This means that you now also need to update Kconfig to depend on
> LEDS_CLASS_MULTICOLOR, so add the following line to the existing
> Kconfig entry for the HP WMI driver:
>
> depends on LEDS_CLASS_MULTICOLOR
>
> Adding this should fix the following errors reported by
> the kernel test robot:
>
> ERROR: modpost: "devm_led_classdev_multicolor_unregister" [drivers/platform/x86/hp/hp-wmi.ko] undefined!
> ERROR: modpost: "devm_led_classdev_multicolor_register_ext" [drivers/platform/x86/hp/hp-wmi.ko] undefined!
Yes, i completely forgot that. Thank you.
> > +static const char * const fourzone_zone_names[4] = {
> > + "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_leds {
> > + struct led_classdev_mc leds[4];
> > + struct mc_subled subleds[4];
>
> The idea with the multi-color API and subleds is that
> a RGB LED really are 3 seperate LEDs (R + G + B) in one.
> This is alsohow they are actually phyiscally made.
> So for 4 zones you need 12 subleds.
>
> I think it would be best to to have a single struct per zone:
>
> struct hp_fourzone_led {
> struct led_classdev_mc mc_led;
> struct mc_subled subleds[3];
> u32 cache; /* Not sure if you still want this */
> };
>
> And then declare an array of 4 of these:
>
> static struct hp_fourzone_led hp_fourzone_leds[4];
That makes sense, i will do it like that.
> > + u32 color_cache[4];
> > +};
> > +static struct hp_fourzone_leds fourzone_leds;
> > +
> > +static enum led_brightness get_fourzone_brightness(struct led_classdev *led_cdev)
> > +{
> > + 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 void fourzone_update_brightness(void)
> > +{
> > + unsigned int br;
> > +
> > + /* synchronize the brightness level on all zones */
> > + br = get_fourzone_brightness(NULL);
> > + for (size_t i = 0; i < 4; i++)
> > + fourzone_leds.leds[i].led_cdev.brightness = br;
> > +}
> > +
> > static void hp_wmi_notify(u32 value, void *context)
> > {
> > struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
> > @@ -932,6 +996,14 @@ static void hp_wmi_notify(u32 value, void *context)
> > case HPWMI_PROXIMITY_SENSOR:
> > break;
> > case HPWMI_BACKLIT_KB_BRIGHTNESS:
> > + if (fourzone_lightning_support) {
> > + input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, true);
> > + input_sync(hp_wmi_input_dev);
> > + input_report_key(hp_wmi_input_dev, KEY_KBDILLUMTOGGLE, false);
> > + input_sync(hp_wmi_input_dev);
> > +
> > + fourzone_update_brightness();
>
> Does you calling fourzone_update_brightness() here mean that the embedded
> controller (EC) if the laptop toggles the kbd backlight on/off itself when
> you press the Fn + key combo for this on the kbd ? In that case you
It switches between the on/off modes.
More about this below.
> should not be sending a KEY_KBDILLUMTOGGLE key press event. That event
> is telling userspace that it should toggle the brightness, which you
> should only do if this is not done by the EC itself.
Is there a way i could do this that i still get this nice indication on the
screen that the key was pressed?
For me it would make sense to tell the user that the brightness key was pressed.
> If the EC does indeed toggle the brightness on/off (or even cycles it
> between various brightness levels) then the right thing to do is to
> call led_classdev_notify_brightness_hw_changed() on mc_led.led_cdev for
> each of the 4 zones when receiving this event.
For this to work properly, it would depend on how we manage the brightness.
More about this below.
> > + }
> > break;
> > case HPWMI_PEAKSHIFT_PERIOD:
> > break;
> > @@ -1505,6 +1577,154 @@ static int thermal_profile_setup(void)
> > return 0;
> > }
> >
> > +static int fourzone_set_colors(u32 color, size_t zone)
> > +{
> > + u8 buff[128];
> > + int ret;
> > +
> > + ret = hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
> > + &buff, sizeof(buff), sizeof(buff));
> > + if (ret != 0)
> > + return -EINVAL;
>
> You are doing a read + modify + write of the kbd settings here.
>
> This is fine, but to avoid racing against another r/m/w cycle
> done at the same time if userspace writes 2 zones at the same
> time you need to take a mutex here.
I agree with you and Ilpo, this should have a mutex.
> > +
> > + buff[25 + zone * 3] = FIELD_GET(FOURZONE_COLOR_R, color);
> > + buff[25 + zone * 3 + 1] = FIELD_GET(FOURZONE_COLOR_G, color);
> > + buff[25 + zone * 3 + 2] = FIELD_GET(FOURZONE_COLOR_B, color);
>
> As mentioned above this is wrong. You should have a separate mc_subled
> struct for each color for each zone (so 3 mc_subled-s per zone,
> one for each of R, G and B; and the you take subled.brightness field
> from the 3 subleds for the 3 different values.
>
> > +
> > + return hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_SET, HPWMI_FOURZONE,
> > + &buff, sizeof(buff), sizeof(buff));
> > +}
> > +
> > +static int fourzone_get_colors(u32 *colors)
> > +{
> > + u8 buff[128];
> > + int ret;
> > +
> > + ret = hp_wmi_perform_query(HPWMI_FOURZONE_COLOR_GET, HPWMI_FOURZONE,
> > + &buff, sizeof(buff), sizeof(buff));
> > + if (ret != 0)
> > + return -EINVAL;
> > +
> > + 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]);
> > + }
>
> same here.
>
> > +
> > + return 0;
> > +}
> > +
> > +static void set_fourzone_brightness(struct led_classdev *led_cdev, enum led_brightness brightness)
> > +{
> > + size_t zone;
> > +
> > + for (size_t i = 0; i < 4; i++)
> > + if (strcmp(led_cdev->name, fourzone_zone_names[i]) == 0)
> > + zone = i;
> > +
>
> So the way how the multicolor LED class devices work is that they have 2
> brightness controls:
>
> /sys/class/leds/hp:rgb:kbd_zoned_backlight-right/brightness
> /sys/class/leds/hp:rgb:kbd_zoned_backlight-right/multi_intensity
>
> Where brightness is a single integer value for overall brightness
> control and multi_intensity is a per channel brightness control, also see
> Documentation/leds/leds-class-multicolor.rst
>
> Now most hw does not have a main/overall brightness control only
> per channel controls (like this hp code) so there is a helper which
> you pass the overall brightness value and which then calculates the
> per channel brightnesses for you.
This keyboard uses the Fn + key combo more like a mode switcher where it puts
the kbd backligh in on/off mode.
What i was doing was taking the overall brightness and using it as a mode controller
where it would take only 2 value 1/0 for on/off respectively and propagate the values
to the other zones to avoid some edge case problems.
If i start using the overall brightness control to control the brightness of each
channel (like it should probably be) then i loose the ability to control the modes
and we would need to find a way to control them.
But i would definitely agree that having mode control and brightness control at the same
time would be better.
Please note that brightness/color control is completely independent from mode control in
this keyboard. I can set any brightness/color to it even if the backligh is off (it will not
turn the backligh on) and when turned on, the new color will be there.
Perhaps we can create a sysfs entry to control the mode and use the main/overall brightness
control the way it was intended?
Now that i think about it, what i made was kinda of a mess :/
> The LED core code caches the last multi_intensity values for you
> and there is a helper you can call from the (this) brightness_set()
> callback:
>
> led_mc_calc_color_components(&fourzone_leds[i].mc_led, brightness);
>
> which will then update the fourzone_leds[i].subleds[0 - 2].brightness
> values with the desired per channel brightness values and then you can
> use those 3 brightness values in fourzone_set_colors() to send to
> the keyboard.
Again, this would depend on how we make brightness and mode control.
> > + if (fourzone_leds.leds[zone].subled_info->intensity == fourzone_leds.color_cache[zone]) {
> > + u8 buff[4] = {
> > + brightness == LED_ON ? FOURZONE_LIGHTING_ON : FOURZONE_LIGHTING_OFF,
> > + 0, 0, 0
> > + };
> > +
> > + hp_wmi_perform_query(HPWMI_LED_BRIGHTNESS_SET, HPWMI_FOURZONE, &buff,
> > + sizeof(buff), 0);
> > +
> > + fourzone_update_brightness();
> > + } else {
> > + fourzone_set_colors(fourzone_leds.leds[zone].subled_info->intensity, zone);
> > + fourzone_leds.color_cache[zone] = fourzone_leds.leds[zone].subled_info->intensity;
> > + }
>
> And this weird cahce thing can be removed then too, just always send the newly calculated
> 3 brightness values to the kbd.
It is weird indeed but it was used to know if i should change the color or the keyboard mode.
So when the color didn't change between calls, i would take the overall brightness and
change the mode from it. Obviously, this means loosing brightness control but the way
i see it, mode control is more important.
> > @@ -1561,6 +1785,12 @@ static void __exit hp_wmi_bios_remove(struct platform_device *device)
> >
> > if (platform_profile_support)
> > platform_profile_remove();
> > +
> > + if (fourzone_lightning_support)
> > + for (size_t i = 0; i < 4; i++) {
> > + devm_led_classdev_multicolor_unregister(&device->dev,
> > + &fourzone_leds.leds[i]);
> > + }
>
> The whole idea behind devm_register_foo() functions is that they get automatically
> removed when the driver is unbound from the device. So this code and
> the fourzone_lightning_support flag are not necessary.
That is good to know. I will remove it.
