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.
