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 >
