Re: [PATCH v3 1/6] asus-wmi: Implement TUF laptop keyboard RGB control

[Hans de Goede]

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
>