| Message ID | 20220805081909.10962-2-luke@ljones.dev (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | asus-wmi: Add support for RGB keyboards | expand |
On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> wrote: > > Adds support for TUF laptop RGB control via the multicolor LED API. > > As this is the base essentials for adjusting the RGB, it sets the these are ...or... essential > default mode of the keyboard to static. This overwrites the booted > state of the keyboard. ... > #include <linux/leds.h> > +#include <linux/led-class-multicolor.h> Not sure about the ordering ('-' vs. 's') in locale C. ... > +static int tuf_rgb_brightness_set(struct led_classdev *cdev, > + enum led_brightness brightness) > +{ > + u8 r, g, b; > + int err; > + u32 ret; > + > + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev); No need to put blank lines in the definition block. Also it would be better to move the longest line to be first. > + led_mc_calc_color_components(mc_cdev, brightness); > + r = mc_cdev->subled_info[0].brightness; > + g = mc_cdev->subled_info[1].brightness; > + b = mc_cdev->subled_info[2].brightness; > + > + /* Writing out requires some defaults. This will overwrite boot mode */ > + err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, ASUS_WMI_DEVID_TUF_RGB_MODE, > + 1 | 0 | (r << 16) | (g << 24), (b) | 0, &ret); What the point in those ' | 0' additions? > + if (err) { > + pr_err("Unable to set TUF RGB data?\n"); Why not dev_err() ? > + return err; > + } > + return 0; return err; > +} ... > + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE)) { > + struct led_classdev_mc *mc_cdev; > + struct mc_subled *mc_led_info; > + u8 brightness = 127; > + mc_cdev = &asus->keyboard_rgb_mode.dev; Join this with the definition above. It's fine if it's a bit longer than 80 characters. > + /* > + * asus::kbd_backlight still controls a base 3-level backlight and when > + * it is on 0, the RGB is not visible at all. RGB should be treated as > + * an additional step. > + */ > + mc_cdev->led_cdev.name = "asus::multicolour::kbd_backlight"; > + mc_cdev->led_cdev.flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN; > + mc_cdev->led_cdev.brightness_set_blocking = tuf_rgb_brightness_set; > + mc_cdev->led_cdev.brightness_get = tuf_rgb_brightness_get; > + > + /* Let the multicolour LED own the info */ > + mc_led_info = devm_kmalloc_array( > + &asus->platform_device->dev, With a temporary variable you may make this one line shorter and nicer looking struct device *dev = &asus->platform_device->dev; > + 3, > + sizeof(*mc_led_info), > + GFP_KERNEL | __GFP_ZERO); > + > + if (!mc_led_info) > + return -ENOMEM; > + > + mc_led_info[0].color_index = LED_COLOR_ID_RED; > + mc_led_info[1].color_index = LED_COLOR_ID_GREEN; > + mc_led_info[2].color_index = LED_COLOR_ID_BLUE; > + > + /* > + * It's not possible to get last set data from device so set defaults > + * to make it safe for a user to change either RGB or modes. We don't > + * write these defaults to the device because they will overwrite a > + * users last saved boot setting (in NVRAM). > + */ > + mc_cdev->led_cdev.brightness = brightness; > + mc_cdev->led_cdev.max_brightness = brightness; > + mc_led_info[0].intensity = brightness; > + mc_led_info[0].brightness = mc_cdev->led_cdev.brightness; > + mc_led_info[1].brightness = mc_cdev->led_cdev.brightness; > + mc_led_info[2].brightness = mc_cdev->led_cdev.brightness; > + led_mc_calc_color_components(mc_cdev, brightness); > + > + mc_cdev->subled_info = mc_led_info; > + mc_cdev->num_colors = 3; > + > + rv = led_classdev_multicolor_register(&asus->platform_device->dev, mc_cdev); This also becomes shorter. > + }
Hi Andy, thanks for the feedback: On Sat, Aug 6 2022 at 11:44:33 +0200, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> wrote: >> >> Adds support for TUF laptop RGB control via the multicolor LED API. >> >> As this is the base essentials for adjusting the RGB, it sets the > > these are > ...or... > essential > >> default mode of the keyboard to static. This overwrites the booted >> state of the keyboard. > > ... > >> #include <linux/leds.h> >> +#include <linux/led-class-multicolor.h> > > Not sure about the ordering ('-' vs. 's') in locale C. > I used hid-playstation.c as a reference and followed that ordering. > ... > >> +static int tuf_rgb_brightness_set(struct led_classdev *cdev, >> + enum led_brightness brightness) >> +{ >> + u8 r, g, b; >> + int err; >> + u32 ret; > >> + >> + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev); > > No need to put blank lines in the definition block. Also it would be > better to move the longest line to be first. Okay cool. Done. > >> + led_mc_calc_color_components(mc_cdev, brightness); >> + r = mc_cdev->subled_info[0].brightness; >> + g = mc_cdev->subled_info[1].brightness; >> + b = mc_cdev->subled_info[2].brightness; >> + >> + /* Writing out requires some defaults. This will overwrite >> boot mode */ >> + err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, >> ASUS_WMI_DEVID_TUF_RGB_MODE, >> + 1 | 0 | (r << 16) | (g << 24), (b) | 0, >> &ret); > > What the point in those ' | 0' additions? They were place-holders in testing that I forgot to change in the second patch which adds mode configuration :( Should be "save | (mode << 8) | (r << 16) | (g << 24), (b) | (speed << 8), &ret);", two bytes. > >> + if (err) { >> + pr_err("Unable to set TUF RGB data?\n"); > > Why not dev_err() ? I didn't know about it? Is there an example or doc on its use? > >> + return err; >> + } >> + return 0; > > return err; Something like this then? if (err) { pr_err("Unable to set TUF RGB data?\n"); } return err; If so, done. > >> +} > > ... > >> + if (asus_wmi_dev_is_present(asus, >> ASUS_WMI_DEVID_TUF_RGB_MODE)) { >> + struct led_classdev_mc *mc_cdev; >> + struct mc_subled *mc_led_info; >> + u8 brightness = 127; > >> + mc_cdev = &asus->keyboard_rgb_mode.dev; > > Join this with the definition above. It's fine if it's a bit longer > than 80 characters. Done. > >> + /* >> + * asus::kbd_backlight still controls a base >> 3-level backlight and when >> + * it is on 0, the RGB is not visible at all. RGB >> should be treated as >> + * an additional step. >> + */ >> + mc_cdev->led_cdev.name = >> "asus::multicolour::kbd_backlight"; >> + mc_cdev->led_cdev.flags = LED_CORE_SUSPENDRESUME | >> LED_RETAIN_AT_SHUTDOWN; >> + mc_cdev->led_cdev.brightness_set_blocking = >> tuf_rgb_brightness_set; >> + mc_cdev->led_cdev.brightness_get = >> tuf_rgb_brightness_get; >> + >> + /* Let the multicolour LED own the info */ >> + mc_led_info = devm_kmalloc_array( >> + &asus->platform_device->dev, > > With a temporary variable you may make this one line shorter and > nicer looking > > struct device *dev = &asus->platform_device->dev; > Done. >> + 3, >> + sizeof(*mc_led_info), >> + GFP_KERNEL | __GFP_ZERO); >> + >> + if (!mc_led_info) >> + return -ENOMEM; >> + >> + mc_led_info[0].color_index = LED_COLOR_ID_RED; >> + mc_led_info[1].color_index = LED_COLOR_ID_GREEN; >> + mc_led_info[2].color_index = LED_COLOR_ID_BLUE; >> + >> + /* >> + * It's not possible to get last set data from >> device so set defaults >> + * to make it safe for a user to change either RGB >> or modes. We don't >> + * write these defaults to the device because they >> will overwrite a >> + * users last saved boot setting (in NVRAM). >> + */ >> + mc_cdev->led_cdev.brightness = brightness; >> + mc_cdev->led_cdev.max_brightness = brightness; >> + mc_led_info[0].intensity = brightness; >> + mc_led_info[0].brightness = >> mc_cdev->led_cdev.brightness; >> + mc_led_info[1].brightness = >> mc_cdev->led_cdev.brightness; >> + mc_led_info[2].brightness = >> mc_cdev->led_cdev.brightness; >> + led_mc_calc_color_components(mc_cdev, brightness); >> + >> + mc_cdev->subled_info = mc_led_info; >> + mc_cdev->num_colors = 3; >> + >> + rv = >> led_classdev_multicolor_register(&asus->platform_device->dev, >> mc_cdev); > > This also becomes shorter. Done. > >> + } > > -- > With Best Regards, > Andy Shevchenko
On Sat, Aug 6, 2022 at 12:16 PM Luke Jones <luke@ljones.dev> wrote: > On Sat, Aug 6 2022 at 11:44:33 +0200, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> wrote: ... > >> #include <linux/leds.h> > >> +#include <linux/led-class-multicolor.h> > > > > Not sure about the ordering ('-' vs. 's') in locale C. > > I used hid-playstation.c as a reference and followed that ordering. Try something like this: LC_ALL=c sort for these two lines and see if the ordering is the same. ... > >> + if (err) { > >> + pr_err("Unable to set TUF RGB data?\n"); > > > > Why not dev_err() ? > > I didn't know about it? Is there an example or doc on its use? Thousands of examples in the kernel source tree. The point is if you have a device (instance) available, use it for messaging. > >> + return err; > >> + } > >> + return 0; > > > > return err; > > Something like this then? > > if (err) { > pr_err("Unable to set TUF RGB data?\n"); > } > return err; > > If so, done. No parentheses. Have you run checkpatch.pl? Something like if (err) dev_err(...); return err; > >> +}
On Sat, Aug 6, 2022 at 12:27 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sat, Aug 6, 2022 at 12:16 PM Luke Jones <luke@ljones.dev> wrote: > > On Sat, Aug 6 2022 at 11:44:33 +0200, Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Fri, Aug 5, 2022 at 10:20 AM Luke D. Jones <luke@ljones.dev> wrote: ... > > >> #include <linux/leds.h> > > >> +#include <linux/led-class-multicolor.h> > > > > > > Not sure about the ordering ('-' vs. 's') in locale C. > > > > I used hid-playstation.c as a reference and followed that ordering. > > Try something like this: > > LC_ALL=c sort Should be, of course, LC_ALL=C sort > for these two lines and see if the ordering is the same.
Hi 2022. augusztus 5., péntek 10:19 keltezéssel, Luke D. Jones <luke@ljones.dev> írta: > Adds support for TUF laptop RGB control via the multicolor LED API. > > As this is the base essentials for adjusting the RGB, it sets the > default mode of the keyboard to static. This overwrites the booted > state of the keyboard. > > Signed-off-by: Luke D. Jones <luke@ljones.dev> > --- > drivers/platform/x86/asus-wmi.c | 89 ++++++++++++++++++++++ > include/linux/platform_data/x86/asus-wmi.h | 3 + > 2 files changed, 92 insertions(+) > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > index 0e7fbed8a50d..33384e3321bb 100644 > --- a/drivers/platform/x86/asus-wmi.c > +++ b/drivers/platform/x86/asus-wmi.c > @@ -25,6 +25,7 @@ > #include <linux/input/sparse-keymap.h> > #include <linux/kernel.h> > #include <linux/leds.h> > +#include <linux/led-class-multicolor.h> > #include <linux/module.h> > #include <linux/pci.h> > #include <linux/pci_hotplug.h> > @@ -190,6 +191,11 @@ struct fan_curve_data { > u8 percents[FAN_CURVE_POINTS]; > }; > > +struct keyboard_rgb_led { > + struct led_classdev_mc dev; > + struct mc_subled subled_info[3]; /* r g b */ > +}; > + > struct asus_wmi { > int dsts_id; > int spec; > @@ -234,6 +240,9 @@ struct asus_wmi { > bool dgpu_disable_available; > bool dgpu_disable; > > + bool keyboard_rgb_mode_available; I think this variable could be introduced in the next patch, it is not used in this one. > + struct keyboard_rgb_led keyboard_rgb_mode; > + > bool throttle_thermal_policy_available; > u8 throttle_thermal_policy_mode; > > @@ -1028,6 +1037,35 @@ static enum led_brightness lightbar_led_get(struct led_classdev *led_cdev) > return result & ASUS_WMI_DSTS_LIGHTBAR_MASK; > } > > +static int tuf_rgb_brightness_set(struct led_classdev *cdev, > + enum led_brightness brightness) > +{ > + u8 r, g, b; > + int err; > + u32 ret; > + > + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev); > + > + led_mc_calc_color_components(mc_cdev, brightness); > + r = mc_cdev->subled_info[0].brightness; > + g = mc_cdev->subled_info[1].brightness; > + b = mc_cdev->subled_info[2].brightness; > + > + /* Writing out requires some defaults. This will overwrite boot mode */ > + err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, ASUS_WMI_DEVID_TUF_RGB_MODE, > + 1 | 0 | (r << 16) | (g << 24), (b) | 0, &ret); > + if (err) { > + pr_err("Unable to set TUF RGB data?\n"); > + return err; > + } > + return 0; > +} > + > +static enum led_brightness tuf_rgb_brightness_get(struct led_classdev *cdev) > +{ > + return cdev->brightness; > +} If you can't query the brightness from the hardware, I think you can leave `led_classdev::brightness_get` to be `NULL`. This callback is only used from `led_update_brightness()` as far as I can see. > + > static void asus_wmi_led_exit(struct asus_wmi *asus) > { > led_classdev_unregister(&asus->kbd_led); > @@ -1105,6 +1143,57 @@ static int asus_wmi_led_init(struct asus_wmi *asus) > &asus->lightbar_led); > } > > + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE)) { > + struct led_classdev_mc *mc_cdev; > + struct mc_subled *mc_led_info; > + u8 brightness = 127; > + > + mc_cdev = &asus->keyboard_rgb_mode.dev; > + > + /* > + * asus::kbd_backlight still controls a base 3-level backlight and when > + * it is on 0, the RGB is not visible at all. RGB should be treated as > + * an additional step. > + */ > + mc_cdev->led_cdev.name = "asus::multicolour::kbd_backlight"; > + mc_cdev->led_cdev.flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN; > + mc_cdev->led_cdev.brightness_set_blocking = tuf_rgb_brightness_set; > + mc_cdev->led_cdev.brightness_get = tuf_rgb_brightness_get; > + > + /* Let the multicolour LED own the info */ > + mc_led_info = devm_kmalloc_array( > + &asus->platform_device->dev, > + 3, > + sizeof(*mc_led_info), > + GFP_KERNEL | __GFP_ZERO); > + I am a bit confused as to why dynamic allocation is needed here. Haven't you already "allocated" the storage in `keyboard_rgb_led::subled_info`? > + if (!mc_led_info) > + return -ENOMEM; > + > + mc_led_info[0].color_index = LED_COLOR_ID_RED; > + mc_led_info[1].color_index = LED_COLOR_ID_GREEN; > + mc_led_info[2].color_index = LED_COLOR_ID_BLUE; > + > + /* > + * It's not possible to get last set data from device so set defaults > + * to make it safe for a user to change either RGB or modes. We don't > + * write these defaults to the device because they will overwrite a > + * users last saved boot setting (in NVRAM). > + */ > + mc_cdev->led_cdev.brightness = brightness; > + mc_cdev->led_cdev.max_brightness = brightness; > + mc_led_info[0].intensity = brightness; > + mc_led_info[0].brightness = mc_cdev->led_cdev.brightness; > + mc_led_info[1].brightness = mc_cdev->led_cdev.brightness; > + mc_led_info[2].brightness = mc_cdev->led_cdev.brightness; > + led_mc_calc_color_components(mc_cdev, brightness); > + > + mc_cdev->subled_info = mc_led_info; > + mc_cdev->num_colors = 3; `led_mc_calc_color_components()` uses `led_classdev_mc::num_colors`, so I think it needs to be set before calling it. But that function sets the subled brightness based on the intensity, so it will overwrite the brightness values that have just been set. > + > + rv = led_classdev_multicolor_register(&asus->platform_device->dev, mc_cdev); > + } > + > error: > if (rv) > asus_wmi_led_exit(asus); > diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h > index a571b47ff362..d63c9945a17d 100644 > --- a/include/linux/platform_data/x86/asus-wmi.h > +++ b/include/linux/platform_data/x86/asus-wmi.h > @@ -98,6 +98,9 @@ > /* dgpu on/off */ > #define ASUS_WMI_DEVID_DGPU 0x00090020 > > +/* TUF laptop RGB control */ > +#define ASUS_WMI_DEVID_TUF_RGB_MODE 0x00100056 > + > /* DSTS masks */ > #define ASUS_WMI_DSTS_STATUS_BIT 0x00000001 > #define ASUS_WMI_DSTS_UNKNOWN_BIT 0x00000002 > -- > 2.37.1 > > Regards, Barnabás Pőcze
Hi Barnabás, Thank you for taking the time to review On Sat, Aug 6 2022 at 17:30:04 +0000, Barnabás Pőcze <pobrn@protonmail.com> wrote: > Hi > > > 2022. augusztus 5., péntek 10:19 keltezéssel, Luke D. Jones > <luke@ljones.dev> írta: > >> Adds support for TUF laptop RGB control via the multicolor LED API. >> >> As this is the base essentials for adjusting the RGB, it sets the >> default mode of the keyboard to static. This overwrites the booted >> state of the keyboard. >> >> Signed-off-by: Luke D. Jones <luke@ljones.dev> >> --- >> drivers/platform/x86/asus-wmi.c | 89 >> ++++++++++++++++++++++ >> include/linux/platform_data/x86/asus-wmi.h | 3 + >> 2 files changed, 92 insertions(+) >> >> diff --git a/drivers/platform/x86/asus-wmi.c >> b/drivers/platform/x86/asus-wmi.c >> index 0e7fbed8a50d..33384e3321bb 100644 >> --- a/drivers/platform/x86/asus-wmi.c >> +++ b/drivers/platform/x86/asus-wmi.c >> @@ -25,6 +25,7 @@ >> #include <linux/input/sparse-keymap.h> >> #include <linux/kernel.h> >> #include <linux/leds.h> >> +#include <linux/led-class-multicolor.h> >> #include <linux/module.h> >> #include <linux/pci.h> >> #include <linux/pci_hotplug.h> >> @@ -190,6 +191,11 @@ struct fan_curve_data { >> u8 percents[FAN_CURVE_POINTS]; >> }; >> >> +struct keyboard_rgb_led { >> + struct led_classdev_mc dev; >> + struct mc_subled subled_info[3]; /* r g b */ >> +}; >> + >> struct asus_wmi { >> int dsts_id; >> int spec; >> @@ -234,6 +240,9 @@ struct asus_wmi { >> bool dgpu_disable_available; >> bool dgpu_disable; >> >> + bool keyboard_rgb_mode_available; > > I think this variable could be introduced in the next patch, it is > not used in this one. > Thank you for spotting this. I must have missed it when I split the patches. > >> + struct keyboard_rgb_led keyboard_rgb_mode; >> + >> bool throttle_thermal_policy_available; >> u8 throttle_thermal_policy_mode; >> >> @@ -1028,6 +1037,35 @@ static enum led_brightness >> lightbar_led_get(struct led_classdev *led_cdev) >> return result & ASUS_WMI_DSTS_LIGHTBAR_MASK; >> } >> >> +static int tuf_rgb_brightness_set(struct led_classdev *cdev, >> + enum led_brightness brightness) >> +{ >> + u8 r, g, b; >> + int err; >> + u32 ret; >> + >> + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev); >> + >> + led_mc_calc_color_components(mc_cdev, brightness); >> + r = mc_cdev->subled_info[0].brightness; >> + g = mc_cdev->subled_info[1].brightness; >> + b = mc_cdev->subled_info[2].brightness; >> + >> + /* Writing out requires some defaults. This will overwrite boot >> mode */ >> + err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, >> ASUS_WMI_DEVID_TUF_RGB_MODE, >> + 1 | 0 | (r << 16) | (g << 24), (b) | 0, &ret); >> + if (err) { >> + pr_err("Unable to set TUF RGB data?\n"); >> + return err; >> + } >> + return 0; >> +} >> + >> +static enum led_brightness tuf_rgb_brightness_get(struct >> led_classdev *cdev) >> +{ >> + return cdev->brightness; >> +} > > If you can't query the brightness from the hardware, I think you can > leave > `led_classdev::brightness_get` to be `NULL`. This callback is only > used from > `led_update_brightness()` as far as I can see. > I did wonder about this. Thanks, I've nulled it. > >> + >> static void asus_wmi_led_exit(struct asus_wmi *asus) >> { >> led_classdev_unregister(&asus->kbd_led); >> @@ -1105,6 +1143,57 @@ static int asus_wmi_led_init(struct asus_wmi >> *asus) >> &asus->lightbar_led); >> } >> >> + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE)) { >> + struct led_classdev_mc *mc_cdev; >> + struct mc_subled *mc_led_info; >> + u8 brightness = 127; >> + >> + mc_cdev = &asus->keyboard_rgb_mode.dev; >> + >> + /* >> + * asus::kbd_backlight still controls a base 3-level backlight >> and when >> + * it is on 0, the RGB is not visible at all. RGB should be >> treated as >> + * an additional step. >> + */ >> + mc_cdev->led_cdev.name = "asus::multicolour::kbd_backlight"; >> + mc_cdev->led_cdev.flags = LED_CORE_SUSPENDRESUME | >> LED_RETAIN_AT_SHUTDOWN; >> + mc_cdev->led_cdev.brightness_set_blocking = >> tuf_rgb_brightness_set; >> + mc_cdev->led_cdev.brightness_get = tuf_rgb_brightness_get; >> + >> + /* Let the multicolour LED own the info */ >> + mc_led_info = devm_kmalloc_array( >> + &asus->platform_device->dev, >> + 3, >> + sizeof(*mc_led_info), >> + GFP_KERNEL | __GFP_ZERO); >> + > > I am a bit confused as to why dynamic allocation is needed here. > Haven't you > already "allocated" the storage in `keyboard_rgb_led::subled_info`? Honestly, I write rust 90% of the time which is quite clear on these things, so here I wasn't very sure about what to do - I read the hid-playstation.c and it had something similar so I used it and completely forgot about the struct allocation. I've updated to `struct mc_subled *mc_led_info = asus->keyboard_rgb_mode.subled_info;` and removed the dynamic alloc. > > >> + if (!mc_led_info) >> + return -ENOMEM; >> + >> + mc_led_info[0].color_index = LED_COLOR_ID_RED; >> + mc_led_info[1].color_index = LED_COLOR_ID_GREEN; >> + mc_led_info[2].color_index = LED_COLOR_ID_BLUE; >> + >> + /* >> + * It's not possible to get last set data from device so set >> defaults >> + * to make it safe for a user to change either RGB or modes. We >> don't >> + * write these defaults to the device because they will >> overwrite a >> + * users last saved boot setting (in NVRAM). >> + */ >> + mc_cdev->led_cdev.brightness = brightness; >> + mc_cdev->led_cdev.max_brightness = brightness; >> + mc_led_info[0].intensity = brightness; >> + mc_led_info[0].brightness = mc_cdev->led_cdev.brightness; >> + mc_led_info[1].brightness = mc_cdev->led_cdev.brightness; >> + mc_led_info[2].brightness = mc_cdev->led_cdev.brightness; >> + led_mc_calc_color_components(mc_cdev, brightness); >> + >> + mc_cdev->subled_info = mc_led_info; >> + mc_cdev->num_colors = 3; > > `led_mc_calc_color_components()` uses `led_classdev_mc::num_colors`, > so I think > it needs to be set before calling it. But that function sets the > subled brightness > based on the intensity, so it will overwrite the brightness values > that have just > been set. Ah thanks, I don't think I understood that before. I've removed mc_led_info[i].brightness setting now. > > >> + >> + rv = >> led_classdev_multicolor_register(&asus->platform_device->dev, >> mc_cdev); >> + } >> + >> error: >> if (rv) >> asus_wmi_led_exit(asus); >> diff --git a/include/linux/platform_data/x86/asus-wmi.h >> b/include/linux/platform_data/x86/asus-wmi.h >> index a571b47ff362..d63c9945a17d 100644 >> --- a/include/linux/platform_data/x86/asus-wmi.h >> +++ b/include/linux/platform_data/x86/asus-wmi.h >> @@ -98,6 +98,9 @@ >> /* dgpu on/off */ >> #define ASUS_WMI_DEVID_DGPU 0x00090020 >> >> +/* TUF laptop RGB control */ >> +#define ASUS_WMI_DEVID_TUF_RGB_MODE 0x00100056 >> + >> /* DSTS masks */ >> #define ASUS_WMI_DSTS_STATUS_BIT 0x00000001 >> #define ASUS_WMI_DSTS_UNKNOWN_BIT 0x00000002 >> -- >> 2.37.1 >> >> > > > Regards, > Barnabás Pőcze
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index 0e7fbed8a50d..33384e3321bb 100644 --- a/drivers/platform/x86/asus-wmi.c +++ b/drivers/platform/x86/asus-wmi.c @@ -25,6 +25,7 @@ #include <linux/input/sparse-keymap.h> #include <linux/kernel.h> #include <linux/leds.h> +#include <linux/led-class-multicolor.h> #include <linux/module.h> #include <linux/pci.h> #include <linux/pci_hotplug.h> @@ -190,6 +191,11 @@ struct fan_curve_data { u8 percents[FAN_CURVE_POINTS]; }; +struct keyboard_rgb_led { + struct led_classdev_mc dev; + struct mc_subled subled_info[3]; /* r g b */ +}; + struct asus_wmi { int dsts_id; int spec; @@ -234,6 +240,9 @@ struct asus_wmi { bool dgpu_disable_available; bool dgpu_disable; + bool keyboard_rgb_mode_available; + struct keyboard_rgb_led keyboard_rgb_mode; + bool throttle_thermal_policy_available; u8 throttle_thermal_policy_mode; @@ -1028,6 +1037,35 @@ static enum led_brightness lightbar_led_get(struct led_classdev *led_cdev) return result & ASUS_WMI_DSTS_LIGHTBAR_MASK; } +static int tuf_rgb_brightness_set(struct led_classdev *cdev, + enum led_brightness brightness) +{ + u8 r, g, b; + int err; + u32 ret; + + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev); + + led_mc_calc_color_components(mc_cdev, brightness); + r = mc_cdev->subled_info[0].brightness; + g = mc_cdev->subled_info[1].brightness; + b = mc_cdev->subled_info[2].brightness; + + /* Writing out requires some defaults. This will overwrite boot mode */ + err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, ASUS_WMI_DEVID_TUF_RGB_MODE, + 1 | 0 | (r << 16) | (g << 24), (b) | 0, &ret); + if (err) { + pr_err("Unable to set TUF RGB data?\n"); + return err; + } + return 0; +} + +static enum led_brightness tuf_rgb_brightness_get(struct led_classdev *cdev) +{ + return cdev->brightness; +} + static void asus_wmi_led_exit(struct asus_wmi *asus) { led_classdev_unregister(&asus->kbd_led); @@ -1105,6 +1143,57 @@ static int asus_wmi_led_init(struct asus_wmi *asus) &asus->lightbar_led); } + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE)) { + struct led_classdev_mc *mc_cdev; + struct mc_subled *mc_led_info; + u8 brightness = 127; + + mc_cdev = &asus->keyboard_rgb_mode.dev; + + /* + * asus::kbd_backlight still controls a base 3-level backlight and when + * it is on 0, the RGB is not visible at all. RGB should be treated as + * an additional step. + */ + mc_cdev->led_cdev.name = "asus::multicolour::kbd_backlight"; + mc_cdev->led_cdev.flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN; + mc_cdev->led_cdev.brightness_set_blocking = tuf_rgb_brightness_set; + mc_cdev->led_cdev.brightness_get = tuf_rgb_brightness_get; + + /* Let the multicolour LED own the info */ + mc_led_info = devm_kmalloc_array( + &asus->platform_device->dev, + 3, + sizeof(*mc_led_info), + GFP_KERNEL | __GFP_ZERO); + + if (!mc_led_info) + return -ENOMEM; + + mc_led_info[0].color_index = LED_COLOR_ID_RED; + mc_led_info[1].color_index = LED_COLOR_ID_GREEN; + mc_led_info[2].color_index = LED_COLOR_ID_BLUE; + + /* + * It's not possible to get last set data from device so set defaults + * to make it safe for a user to change either RGB or modes. We don't + * write these defaults to the device because they will overwrite a + * users last saved boot setting (in NVRAM). + */ + mc_cdev->led_cdev.brightness = brightness; + mc_cdev->led_cdev.max_brightness = brightness; + mc_led_info[0].intensity = brightness; + mc_led_info[0].brightness = mc_cdev->led_cdev.brightness; + mc_led_info[1].brightness = mc_cdev->led_cdev.brightness; + mc_led_info[2].brightness = mc_cdev->led_cdev.brightness; + led_mc_calc_color_components(mc_cdev, brightness); + + mc_cdev->subled_info = mc_led_info; + mc_cdev->num_colors = 3; + + rv = led_classdev_multicolor_register(&asus->platform_device->dev, mc_cdev); + } + error: if (rv) asus_wmi_led_exit(asus); diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h index a571b47ff362..d63c9945a17d 100644 --- a/include/linux/platform_data/x86/asus-wmi.h +++ b/include/linux/platform_data/x86/asus-wmi.h @@ -98,6 +98,9 @@ /* dgpu on/off */ #define ASUS_WMI_DEVID_DGPU 0x00090020 +/* TUF laptop RGB control */ +#define ASUS_WMI_DEVID_TUF_RGB_MODE 0x00100056 + /* DSTS masks */ #define ASUS_WMI_DSTS_STATUS_BIT 0x00000001 #define ASUS_WMI_DSTS_UNKNOWN_BIT 0x00000002
Adds support for TUF laptop RGB control via the multicolor LED API. As this is the base essentials for adjusting the RGB, it sets the default mode of the keyboard to static. This overwrites the booted state of the keyboard. Signed-off-by: Luke D. Jones <luke@ljones.dev> --- drivers/platform/x86/asus-wmi.c | 89 ++++++++++++++++++++++ include/linux/platform_data/x86/asus-wmi.h | 3 + 2 files changed, 92 insertions(+)