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

[Hans de Goede]

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