| Message ID | 20220809025054.1626339-2-luke@ljones.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | asus-wmi: Add support for RGB keyboards | expand |
On Tue, Aug 9, 2022 at 4:51 AM Luke D. Jones <luke@ljones.dev> wrote: > > Adds support for TUF laptop RGB control via the multicolor LED API. > > As this is the bas for adjusting only the RGB values, it sets the > default mode of the keyboard to static since there is no way to read > any existing settings from the device. These defaults overwrite the > booted state of the keyboard when the module is loaded. ... > + err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, ASUS_WMI_DEVID_TUF_RGB_MODE, > + rgb->save | (rgb->mode << 8) | (r << 16) | (g << 24), > + (b) | (rgb->speed << 8), Too many parentheses. > + &ret); > + if (err) > + dev_err(dev, "Unable to set TUF RGB data?\n"); > + > + return err; How ret is being used?
Hi Andy, On Tue, Aug 9 2022 at 10:46:33 +0200, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Aug 9, 2022 at 4:51 AM Luke D. Jones <luke@ljones.dev> wrote: >> >> Adds support for TUF laptop RGB control via the multicolor LED API. >> >> As this is the bas for adjusting only the RGB values, it sets the >> default mode of the keyboard to static since there is no way to read >> any existing settings from the device. These defaults overwrite the >> booted state of the keyboard when the module is loaded. > > ... > >> + err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, >> ASUS_WMI_DEVID_TUF_RGB_MODE, >> + rgb->save | (rgb->mode << 8) | (r >> << 16) | (g << 24), >> + (b) | (rgb->speed << 8), > > Too many parentheses. Uh, yeah. I was unable to find concrete info on this. I at one point experienced an issue where the order of operations *without* parentheses ended up as "x | y << (8 | z) << 16". But now I'm not even sure if I remember that correctly. I see the order here https://www.cs.uic.edu/~i109/Notes/COperatorPrecedenceTable.pdf I'll do as said and test it to be certain. > >> + &ret); >> + if (err) >> + dev_err(dev, "Unable to set TUF RGB data?\n"); >> + >> + return err; > > How ret is being used? Damn.. fixed now. > > -- > With Best Regards, > Andy Shevchenko
On Tue, Aug 9, 2022 at 10:56 AM Luke Jones <luke@ljones.dev> wrote: > On Tue, Aug 9 2022 at 10:46:33 +0200, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Tue, Aug 9, 2022 at 4:51 AM Luke D. Jones <luke@ljones.dev> wrote: ... > >> + err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, > >> ASUS_WMI_DEVID_TUF_RGB_MODE, > >> + rgb->save | (rgb->mode << 8) | (r > >> << 16) | (g << 24), > >> + (b) | (rgb->speed << 8), > > > > Too many parentheses. > > Uh, yeah. I was unable to find concrete info on this. I at one point > experienced an issue where the order of operations *without* > parentheses ended up as "x | y << (8 | z) << 16". But now I'm not even > sure if I remember that correctly. I see the order here > https://www.cs.uic.edu/~i109/Notes/COperatorPrecedenceTable.pdf > > I'll do as said and test it to be certain. I'm talking about the '(b)' part. The rest is okay to me.
On Tue, 2022-08-09 at 11:29 +0200, Andy Shevchenko wrote: > On Tue, Aug 9, 2022 at 10:56 AM Luke Jones <luke@ljones.dev> wrote: > > On Tue, Aug 9 2022 at 10:46:33 +0200, Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Tue, Aug 9, 2022 at 4:51 AM Luke D. Jones <luke@ljones.dev> > > > wrote: > > ... > > > > > + err = > > > > asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, > > > > ASUS_WMI_DEVID_TUF_RGB_MODE, > > > > + rgb->save | (rgb->mode << 8) | > > > > (r > > > > << 16) | (g << 24), > > > > + (b) | (rgb->speed << 8), > > > > > > Too many parentheses. > > > > Uh, yeah. I was unable to find concrete info on this. I at one > > point > > experienced an issue where the order of operations *without* > > parentheses ended up as "x | y << (8 | z) << 16". But now I'm not > > even > > sure if I remember that correctly. I see the order here > > https://www.cs.uic.edu/~i109/Notes/COperatorPrecedenceTable.pdf > > > > I'll do as said and test it to be certain. > > I'm talking about the '(b)' part. The rest is okay to me. > Understood, thanks.
Hi! > Adds support for TUF laptop RGB control via the multicolor LED API. > > As this is the bas for adjusting only the RGB values, it sets the > default mode of the keyboard to static since there is no way to read > any existing settings from the device. These defaults overwrite the > booted state of the keyboard when the module is loaded. > + if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE)) { > + struct mc_subled *mc_led_info = asus->keyboard_rgb_mode.subled_info; > + struct led_classdev_mc *mc_cdev = &asus->keyboard_rgb_mode.dev; > + struct device *dev = &asus->platform_device->dev; > + u8 led_brightness = 255; > + > + /* > + * 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. > + */ Ouch. Lets not do that? If rgb interface is available, hide the 3 level one, or something. > + mc_cdev->led_cdev.name = "asus::multicolour::kbd_backlight"; Make this "rgb:kbd_backlight" or "inputX:rgb:kbd_backligh" and document it in Documentation/leds/well-known-leds.txt. Thanks and best regards, Pavel
Hi Pavel, Andy, Hans, > > > > > > > > + /* > > > > > > > > + * 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. > > > > > > > > + */ > > > > > > > > Ouch. Lets not do that? If rgb interface is available, hide the > > > > 3 > > > > level one, or something. > > > > I really don't think this is safe or sensible. There are some laptops that default the 3-stage method to off, and this means that the LEDs will not show regardless of multicolor brightness. > > > > > > > > + mc_cdev->led_cdev.name = > > > > > > > > > > > > > > "asus::multicolour::kbd_backlight"; > > > > > > > > Make this "rgb:kbd_backlight" or "inputX:rgb:kbd_backligh" and > > > > document it in Documentation/leds/well-known-leds.txt. Will do. -- 4 hours later -- I've spent a lot of time working on this now. I don't think multicolor LED is suitable for use with the way these keyboards work. The biggest issue is related to the brightness setting. 1. If the ASUS_WMI_DEVID_KBD_BACKLIGHT method defaults to 0 on boot then RGB is not visible I worked around this by setting it to "3" by default in module if ASUS_WMI_DEVID_TUF_RGB_MODE is found. And added a check in the button events to adjust multicolor brightness (+/- 17). This works but now I can't do led notify (led_classdev_notify_brightness_hw_changed). 2. Pattern trigger can't be used for these keyboard modes as the modes are done entirely in hardware via a single switch in the complete command packet. I don't see any way forward with this, and looking at the complexity I don't have time either. 3. Nodes everywhere.. To fully control control these keyboards there are two WMI methods, one for mode/rgb, one for power-state. Splitting each of these parameters out to individual nodes with sensible naming and expectations gives: - keyboard_rgb_apply, new WO node to actually write out data - keyboard_rgb_save, first parameter of packet, retain-on-boot - keyboard_rgb_mode, the factory built-in modes, - keyboard_rgb_speed, speed of certain modes And then for power-state: - keyboard_state_apply, new WO node to actually write out data - keyboard_state_save, first parameter of packet, retain-on-boot - keyboard_state_boot, play boot animation (on boot) - keyboard_state_awake, LEDs visible while awake - keyboard_state_sleep, play suspend animation (while suspended) - keyboard_state_keyboard, unknown effect Quite frankly I would rather use the method I had in the first patch I submitted where mode and state had two nodes each, - keyboard_rgb_mode, WO = "n n n n n n" - keyboard_rgb_mode_index, output = "save/apply, mode, r, g, b, speed" - keyboard_rgb_state, WO = "n n n n n" - keyboard_rgb_state_index, output = "save/apply, boot, awake, sleep, keyboard" A big benefit of this structure is that not being able to read settings back from the keyboard (not possible!) becomes a non-issue because users have to write a full input, not partial, and it will apply right away. Multicolor class could still be used, but from everything I've tried now it really isn't suitable when the proper method for brightness is a separate WMI method (0-3), and when that is 0 it means LEDs are fully off - there's potential for mistakes/issues. Losing led-notif is an issue for users for sure. In short, from dog-fooding the current state inlcuding the trial of using multicolor brightness (and hiding the proper WMI method) I can only conclude that multicolor is not suitable for how these keyboards work. Hans, Andy, can I please revert back to the node + _index pairs taking an array input. Everything will be cleaner and simpler. Cheers, Luke.
Hi, On 8/10/22 06:44, Luke Jones wrote: > Hi Pavel, Andy, Hans, > >>>>>>>>> + /* >>>>>>>>> + * 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. >>>>>>>>> + */ >>>>> >>>>> Ouch. Lets not do that? If rgb interface is available, hide the >>>>> 3 >>>>> level one, or something. >>>>> > > I really don't think this is safe or sensible. There are some laptops > that default the 3-stage method to off, and this means that the LEDs > will not show regardless of multicolor brightness. > > > >>>>>>>>> + mc_cdev->led_cdev.name = > > > > > > >>>>>>>>> "asus::multicolour::kbd_backlight"; >>>>> >>>>> Make this "rgb:kbd_backlight" or "inputX:rgb:kbd_backligh" and >>>>> document it in Documentation/leds/well-known-leds.txt. > > Will do. > > -- 4 hours later -- > > I've spent a lot of time working on this now. I don't think multicolor > LED is suitable for use with the way these keyboards work. > > The biggest issue is related to the brightness setting. > 1. If the ASUS_WMI_DEVID_KBD_BACKLIGHT method defaults to 0 on boot > then RGB is not visible Note to others following this thread I asked Luke to clarify this a bit in an unrelated 1:1 conversation we were having: On 8/10/22 23:45, Luke Jones wrote: > On 8/10/22, Hans de Goede wrote: >> I plan to go through all the asus-wmi stuff you've posted tomorrow, >> so I'll reply to this then. One thing which is not entirely >> clear to me is that: >> >> 1. If I understand you correctly the laptops >> with the RGB keyboards have both the old mono-color >> "asus::kbd_backlight" >> as well as a new RGB interface and these somehow interact with each >> other, do I understand that correctly? > > Yes, and that is the problem. The "mono" switch takes precedence. > >> 2. If yes, then can you explain the interaction in a bit more detail, >> I see you say someting along the lines of the RGB controls only >> working when the old mono-color "asus::kbd_backlight" brightness >> is set to 3 (which is its max brightness) ? > > Adjusting this changes the overall keyboard brightness. So if this is > at 1, and all RGB is at 255, then when you switch 2, 3, the overall > brightness increases. > >> 3. So what happens e.g. if writing 2 to the old mono-color >> "asus::kbd_backlight" brightness after setting some RGB values ? > > If the brightness was 3, then the overall brightness decreases. > If it was at 1, then it increases. I see, so the old (still present) mono-color "asus::kbd_backlight" brightness works as a master brightness control and the rgb values in the ASUS_WMI_DEVID_TUF_RGB_MODE WMI set commands are really just to set the color. And I guess that the Fn + whatever kbd brightness hotkey also still modifies the old mono-color "asus::kbd_backlight"? Which means that the "asus::kbd_backlight" device is also the device on which the led_classdev_notify_brightness_hw_changed is done as you mention below. (continued below. > I worked around this by setting it to "3" by default in module if > ASUS_WMI_DEVID_TUF_RGB_MODE is found. And added a check in the button > events to adjust multicolor brightness (+/- 17). This works but now I > can't do led notify (led_classdev_notify_brightness_hw_changed). > > 2. Pattern trigger can't be used for these keyboard modes as the modes > are done entirely in hardware via a single switch in the complete > command packet. > > I don't see any way forward with this, and looking at the complexity I > don't have time either. > > 3. Nodes everywhere.. > > To fully control control these keyboards there are two WMI methods, one > for mode/rgb, one for power-state. Splitting each of these parameters > out to individual nodes with sensible naming and expectations gives: <snip> > Quite frankly I would rather use the method I had in the first patch I > submitted where mode and state had two nodes each, > - keyboard_rgb_mode, WO = "n n n n n n" > - keyboard_rgb_mode_index, output = "save/apply, mode, r, g, b, speed" > - keyboard_rgb_state, WO = "n n n n n" > - keyboard_rgb_state_index, output = "save/apply, boot, awake, sleep, > keyboard" > > A big benefit of this structure is that not being able to read settings > back from the keyboard (not possible!) becomes a non-issue because > users have to write a full input, not partial, and it will apply right > away. Right to me this not being able to read back the values shows that the firmware API here really is not suitable for doing a more fancy "nice" / standard sysfs API on top. Since we cannot read back any of the r, g, b, mode or speed values we would need to pick defaults and then setting any of them would override the actual values the hw is using for the others, which is really not a good thing to do. So that only leaves something akin to keyboard_rgb_mode[_index] + keyboard_rgb_state[_index] which sets all values at once, mirroring the limited WMI API as a good option here, I agree with you on this. Sorry Pavel, I know you don't like custom sysfs attributes being added to LED class devices, but I have to agree with Luke that there really is not a good way to deal with this here and we did try! Only request I have for the next version wrt the decision to circle all the way back to having: > - keyboard_rgb_mode, WO = "n n n n n n" > - keyboard_rgb_mode_index, output = "save/apply, mode, r, g, b, speed" > - keyboard_rgb_state, WO = "n n n n n" > - keyboard_rgb_state_index, output = "save/apply, boot, awake, sleep, Is please put these new attributes under the: /sys/class/leds/asus::kbd_backlight Using the led_class_device.groups member as discussed before, now that we have decided to drop the multicolor LED stuff that should work :) Although maybe Pavel prefers to have the new sysfs attributes under /sys/bus/platform/devices/asus-nb-wmi/ instead since they are non standard. Pavel, to me having these under /sys/class/leds/asus::kbd_backlight seems more logical. But since there are non-standard and since there already is a bunch of asus-wmi sysfs API under /sys/bus/platform/devices/asus-nb-wmi/ putting them there if you prefer that is fine with me too. So what do you prefer ? > Hans, Andy, can I please revert back to the node + _index pairs taking > an array input. Everything will be cleaner and simpler. Ack, see above. Thank you for at least trying to use the multi-color LED API. Regards, Hans
On 8/11/22 17:01, Hans de Goede wrote: > Hi, > > On 8/10/22 06:44, Luke Jones wrote: >> Hi Pavel, Andy, Hans, >> >>>>>>>>>> + /* >>>>>>>>>> + * 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. >>>>>>>>>> + */ >>>>>> >>>>>> Ouch. Lets not do that? If rgb interface is available, hide the >>>>>> 3 >>>>>> level one, or something. >>>>>> >> >> I really don't think this is safe or sensible. There are some laptops >> that default the 3-stage method to off, and this means that the LEDs >> will not show regardless of multicolor brightness. >> >> >> >>>>>>>>>> + mc_cdev->led_cdev.name = > > > > > > >>>>>>>>>> "asus::multicolour::kbd_backlight"; >>>>>> >>>>>> Make this "rgb:kbd_backlight" or "inputX:rgb:kbd_backligh" and >>>>>> document it in Documentation/leds/well-known-leds.txt. >> >> Will do. >> >> -- 4 hours later -- >> >> I've spent a lot of time working on this now. I don't think multicolor >> LED is suitable for use with the way these keyboards work. >> >> The biggest issue is related to the brightness setting. >> 1. If the ASUS_WMI_DEVID_KBD_BACKLIGHT method defaults to 0 on boot >> then RGB is not visible > > Note to others following this thread I asked Luke to clarify this > a bit in an unrelated 1:1 conversation we were having: > > On 8/10/22 23:45, Luke Jones wrote: >> On 8/10/22, Hans de Goede wrote: >>> I plan to go through all the asus-wmi stuff you've posted tomorrow, >>> so I'll reply to this then. One thing which is not entirely >>> clear to me is that: >>> >>> 1. If I understand you correctly the laptops >>> with the RGB keyboards have both the old mono-color >>> "asus::kbd_backlight" >>> as well as a new RGB interface and these somehow interact with each >>> other, do I understand that correctly? >> >> Yes, and that is the problem. The "mono" switch takes precedence. >> >>> 2. If yes, then can you explain the interaction in a bit more detail, >>> I see you say someting along the lines of the RGB controls only >>> working when the old mono-color "asus::kbd_backlight" brightness >>> is set to 3 (which is its max brightness) ? >> >> Adjusting this changes the overall keyboard brightness. So if this is >> at 1, and all RGB is at 255, then when you switch 2, 3, the overall >> brightness increases. >> >>> 3. So what happens e.g. if writing 2 to the old mono-color >>> "asus::kbd_backlight" brightness after setting some RGB values ? >> >> If the brightness was 3, then the overall brightness decreases. >> If it was at 1, then it increases. > > I see, so the old (still present) mono-color "asus::kbd_backlight" > brightness works as a master brightness control and the rgb values > in the ASUS_WMI_DEVID_TUF_RGB_MODE WMI set commands are really > just to set the color. > > And I guess that the Fn + whatever kbd brightness hotkey also still > modifies the old mono-color "asus::kbd_backlight"? Which means that > the "asus::kbd_backlight" device is also the device on which the > led_classdev_notify_brightness_hw_changed is done as you mention > below. > > (continued below. > >> I worked around this by setting it to "3" by default in module if >> ASUS_WMI_DEVID_TUF_RGB_MODE is found. And added a check in the button >> events to adjust multicolor brightness (+/- 17). This works but now I >> can't do led notify (led_classdev_notify_brightness_hw_changed). >> >> 2. Pattern trigger can't be used for these keyboard modes as the modes >> are done entirely in hardware via a single switch in the complete >> command packet. >> >> I don't see any way forward with this, and looking at the complexity I >> don't have time either. >> >> 3. Nodes everywhere.. >> >> To fully control control these keyboards there are two WMI methods, one >> for mode/rgb, one for power-state. Splitting each of these parameters >> out to individual nodes with sensible naming and expectations gives: > > <snip> > >> Quite frankly I would rather use the method I had in the first patch I >> submitted where mode and state had two nodes each, >> - keyboard_rgb_mode, WO = "n n n n n n" >> - keyboard_rgb_mode_index, output = "save/apply, mode, r, g, b, speed" >> - keyboard_rgb_state, WO = "n n n n n" >> - keyboard_rgb_state_index, output = "save/apply, boot, awake, sleep, >> keyboard" >> >> A big benefit of this structure is that not being able to read settings >> back from the keyboard (not possible!) becomes a non-issue because >> users have to write a full input, not partial, and it will apply right >> away. > > Right to me this not being able to read back the values shows that > the firmware API here really is not suitable for doing a more > fancy "nice" / standard sysfs API on top. > > Since we cannot read back any of the r, g, b, mode or speed values > we would need to pick defaults and then setting any of them would > override the actual values the hw is using for the others, which > is really not a good thing to do. > > So that only leaves something akin to keyboard_rgb_mode[_index] + > keyboard_rgb_state[_index] which sets all values at once, mirroring > the limited WMI API as a good option here, I agree with you on this. > > Sorry Pavel, I know you don't like custom sysfs attributes > being added to LED class devices, but I have to agree with Luke > that there really is not a good way to deal with this here and > we did try! > > Only request I have for the next version wrt the decision to > circle all the way back to having: > >> - keyboard_rgb_mode, WO = "n n n n n n" >> - keyboard_rgb_mode_index, output = "save/apply, mode, r, g, b, speed" >> - keyboard_rgb_state, WO = "n n n n n" >> - keyboard_rgb_state_index, output = "save/apply, boot, awake, sleep, > > Is please put these new attributes under the: > /sys/class/leds/asus::kbd_backlight > > Using the led_class_device.groups member as discussed before, now > that we have decided to drop the multicolor LED stuff that should > work :) > > Although maybe Pavel prefers to have the new sysfs attributes > under /sys/bus/platform/devices/asus-nb-wmi/ instead since they > are non standard. > > Pavel, to me having these under /sys/class/leds/asus::kbd_backlight > seems more logical. p.s. Besides it being more logical to group these together with the main brightness control for the kbd_backlight, I believe this way the files will also be easier to discover for users (users not using the asusctl utility that is). > But since there are non-standard and since > there already is a bunch of asus-wmi sysfs API under > /sys/bus/platform/devices/asus-nb-wmi/ putting them there if you > prefer that is fine with me too. So what do you prefer ? > >> Hans, Andy, can I please revert back to the node + _index pairs taking >> an array input. Everything will be cleaner and simpler. > > Ack, see above. Thank you for at least trying to use the multi-color > LED API. > > Regards, > > Hans >
On Thu, 2022-08-11 at 17:05 +0200, Hans de Goede wrote: > > > On 8/11/22 17:01, Hans de Goede wrote: > > Hi, > > > > On 8/10/22 06:44, Luke Jones wrote: > > > Hi Pavel, Andy, Hans, > > > > > > > > > > > > > > + /* > > > > > > > > > > > + * 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. > > > > > > > > > > > + */ > > > > > > > > > > > > > > Ouch. Lets not do that? If rgb interface is available, > > > > > > > hide the > > > > > > > 3 > > > > > > > level one, or something. > > > > > > > > > > > > > I really don't think this is safe or sensible. There are some > > > laptops > > > that default the 3-stage method to off, and this means that the > > > LEDs > > > will not show regardless of multicolor brightness. > > > > > > > > > > > > > > > > > > > > + mc_cdev->led_cdev.name = > > > > > > > > > > > > > > > > > > > > > > > > > > > > "asus::multicolour::kbd_backlight"; > > > > > > > > > > > > > > Make this "rgb:kbd_backlight" or > > > > > > > "inputX:rgb:kbd_backligh" and > > > > > > > document it in Documentation/leds/well-known-leds.txt. > > > > > > Will do. > > > > > > -- 4 hours later -- > > > > > > I've spent a lot of time working on this now. I don't think > > > multicolor > > > LED is suitable for use with the way these keyboards work. > > > > > > The biggest issue is related to the brightness setting. > > > 1. If the ASUS_WMI_DEVID_KBD_BACKLIGHT method defaults to 0 on > > > boot > > > then RGB is not visible > > > > Note to others following this thread I asked Luke to clarify this > > a bit in an unrelated 1:1 conversation we were having: > > > > On 8/10/22 23:45, Luke Jones wrote: > > > On 8/10/22, Hans de Goede wrote: > > > > I plan to go through all the asus-wmi stuff you've posted > > > > tomorrow, > > > > so I'll reply to this then. One thing which is not entirely > > > > clear to me is that: > > > > > > > > 1. If I understand you correctly the laptops > > > > with the RGB keyboards have both the old mono-color > > > > "asus::kbd_backlight" > > > > as well as a new RGB interface and these somehow interact with > > > > each > > > > other, do I understand that correctly? > > > > > > Yes, and that is the problem. The "mono" switch takes precedence. > > > > > > > 2. If yes, then can you explain the interaction in a bit more > > > > detail, > > > > I see you say someting along the lines of the RGB controls only > > > > working when the old mono-color "asus::kbd_backlight" > > > > brightness > > > > is set to 3 (which is its max brightness) ? > > > > > > Adjusting this changes the overall keyboard brightness. So if > > > this is > > > at 1, and all RGB is at 255, then when you switch 2, 3, the > > > overall > > > brightness increases. > > > > > > > 3. So what happens e.g. if writing 2 to the old mono-color > > > > "asus::kbd_backlight" brightness after setting some RGB values > > > > ? > > > > > > If the brightness was 3, then the overall brightness decreases. > > > If it was at 1, then it increases. > > > > I see, so the old (still present) mono-color "asus::kbd_backlight" > > brightness works as a master brightness control and the rgb values > > in the ASUS_WMI_DEVID_TUF_RGB_MODE WMI set commands are really > > just to set the color. > > > > And I guess that the Fn + whatever kbd brightness hotkey also still > > modifies the old mono-color "asus::kbd_backlight"? Which means that > > the "asus::kbd_backlight" device is also the device on which the > > led_classdev_notify_brightness_hw_changed is done as you mention > > below. > > > > (continued below. > > > > > I worked around this by setting it to "3" by default in module if > > > ASUS_WMI_DEVID_TUF_RGB_MODE is found. And added a check in the > > > button > > > events to adjust multicolor brightness (+/- 17). This works but > > > now I > > > can't do led notify (led_classdev_notify_brightness_hw_changed). > > > > > > 2. Pattern trigger can't be used for these keyboard modes as the > > > modes > > > are done entirely in hardware via a single switch in the complete > > > command packet. > > > > > > I don't see any way forward with this, and looking at the > > > complexity I > > > don't have time either. > > > > > > 3. Nodes everywhere.. > > > > > > To fully control control these keyboards there are two WMI > > > methods, one > > > for mode/rgb, one for power-state. Splitting each of these > > > parameters > > > out to individual nodes with sensible naming and expectations > > > gives: > > > > <snip> > > > > > Quite frankly I would rather use the method I had in the first > > > patch I > > > submitted where mode and state had two nodes each, > > > - keyboard_rgb_mode, WO = "n n n n n n" > > > - keyboard_rgb_mode_index, output = "save/apply, mode, r, g, b, > > > speed" > > > - keyboard_rgb_state, WO = "n n n n n" > > > - keyboard_rgb_state_index, output = "save/apply, boot, awake, > > > sleep, > > > keyboard" > > > > > > A big benefit of this structure is that not being able to read > > > settings > > > back from the keyboard (not possible!) becomes a non-issue > > > because > > > users have to write a full input, not partial, and it will apply > > > right > > > away. > > > > Right to me this not being able to read back the values shows that > > the firmware API here really is not suitable for doing a more > > fancy "nice" / standard sysfs API on top. > > > > Since we cannot read back any of the r, g, b, mode or speed values > > we would need to pick defaults and then setting any of them would > > override the actual values the hw is using for the others, which > > is really not a good thing to do. > > > > So that only leaves something akin to keyboard_rgb_mode[_index] + > > keyboard_rgb_state[_index] which sets all values at once, mirroring > > the limited WMI API as a good option here, I agree with you on > > this. > > > > Sorry Pavel, I know you don't like custom sysfs attributes > > being added to LED class devices, but I have to agree with Luke > > that there really is not a good way to deal with this here and > > we did try! > > > > Only request I have for the next version wrt the decision to > > circle all the way back to having: > > > > > - keyboard_rgb_mode, WO = "n n n n n n" > > > - keyboard_rgb_mode_index, output = "save/apply, mode, r, g, b, > > > speed" > > > - keyboard_rgb_state, WO = "n n n n n" > > > - keyboard_rgb_state_index, output = "save/apply, boot, awake, > > > sleep, > > > > Is please put these new attributes under the: > > /sys/class/leds/asus::kbd_backlight > > > > Using the led_class_device.groups member as discussed before, now > > that we have decided to drop the multicolor LED stuff that should > > work :) > > > > Although maybe Pavel prefers to have the new sysfs attributes > > under /sys/bus/platform/devices/asus-nb-wmi/ instead since they > > are non standard. > > > > Pavel, to me having these under /sys/class/leds/asus::kbd_backlight > > seems more logical. > > p.s. > > Besides it being more logical to group these together with the > main brightness control for the kbd_backlight, I believe this > way the files will also be easier to discover for users > (users not using the asusctl utility that is). I agree with this. From what I've seen with folks trying to debug RGB keyboard issues they aren't familiar with they tend to reach straight for looking in /sys/class/leds/asus::kbd_backlight. Doing it this way mean that the attributes will show up in udev under this LED class also, making it a more logical way to discover an added feature. > > But since there are non-standard and since > > there already is a bunch of asus-wmi sysfs API under > > /sys/bus/platform/devices/asus-nb-wmi/ putting them there if you > > prefer that is fine with me too. So what do you prefer ? > > > > > Hans, Andy, can I please revert back to the node + _index pairs > > > taking > > > an array input. Everything will be cleaner and simpler. > > > > Ack, see above. Thank you for at least trying to use the multi- > > color > > LED API. > > > > Regards, > > > > Hans > > > Kind regards, Luke.
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index 0e7fbed8a50d..233e73f4313d 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> @@ -117,6 +118,9 @@ static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL }; static int throttle_thermal_policy_write(struct asus_wmi *); +static int tuf_rgb_brightness_set(struct led_classdev *cdev, + enum led_brightness brightness); + static bool ashs_present(void) { int i = 0; @@ -190,6 +194,14 @@ struct fan_curve_data { u8 percents[FAN_CURVE_POINTS]; }; +struct keyboard_rgb_led { + struct mc_subled subled_info[3]; /* r g b */ + struct led_classdev_mc dev; + u8 save; + u8 mode; + u8 speed; +}; + struct asus_wmi { int dsts_id; int spec; @@ -234,6 +246,8 @@ struct asus_wmi { bool dgpu_disable_available; bool dgpu_disable; + struct keyboard_rgb_led keyboard_rgb_mode; + bool throttle_thermal_policy_available; u8 throttle_thermal_policy_mode; @@ -1028,12 +1042,40 @@ 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) +{ + struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev); + struct keyboard_rgb_led *rgb = container_of(mc_cdev, struct keyboard_rgb_led, dev); + struct asus_wmi *asus = container_of(rgb, struct asus_wmi, keyboard_rgb_mode); + struct device *dev = &asus->platform_device->dev; + u8 r, g, b; + int err; + u32 ret; + + 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, + rgb->save | (rgb->mode << 8) | (r << 16) | (g << 24), + (b) | (rgb->speed << 8), + &ret); + if (err) + dev_err(dev, "Unable to set TUF RGB data?\n"); + + return err; +} + static void asus_wmi_led_exit(struct asus_wmi *asus) { led_classdev_unregister(&asus->kbd_led); led_classdev_unregister(&asus->tpd_led); led_classdev_unregister(&asus->wlan_led); led_classdev_unregister(&asus->lightbar_led); + led_classdev_multicolor_unregister(&asus->keyboard_rgb_mode.dev); if (asus->led_workqueue) destroy_workqueue(asus->led_workqueue); @@ -1105,6 +1147,44 @@ 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 mc_subled *mc_led_info = asus->keyboard_rgb_mode.subled_info; + struct led_classdev_mc *mc_cdev = &asus->keyboard_rgb_mode.dev; + struct device *dev = &asus->platform_device->dev; + u8 led_brightness = 255; + + /* + * 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 = NULL; + + mc_cdev->subled_info = mc_led_info; + mc_led_info[0].intensity = 127; + 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; + mc_cdev->num_colors = 3; + + /* + * 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. + */ + asus->keyboard_rgb_mode.save = 1; + asus->keyboard_rgb_mode.mode = 0; + asus->keyboard_rgb_mode.speed = 0xeb; + + mc_cdev->led_cdev.brightness = led_brightness; + mc_cdev->led_cdev.max_brightness = led_brightness; + led_mc_calc_color_components(mc_cdev, led_brightness); + + rv = led_classdev_multicolor_register(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 bas for adjusting only the RGB values, it sets the default mode of the keyboard to static since there is no way to read any existing settings from the device. These defaults overwrite the booted state of the keyboard when the module is loaded. Signed-off-by: Luke D. Jones <luke@ljones.dev> --- drivers/platform/x86/asus-wmi.c | 80 ++++++++++++++++++++++ include/linux/platform_data/x86/asus-wmi.h | 3 + 2 files changed, 83 insertions(+)