Message-ID: <4cefa82993c9a3560c0e0fd8969ac8e5e6d8ecc8.camel@ljones.dev>
Date:   Fri, 12 Aug 2022 10:13:21 +1200
From:   Luke Jones <luke@...nes.dev>
To:     Hans de Goede <hdegoede@...hat.com>, Pavel Machek <pavel@....cz>
Cc:     andy.shevchenko@...il.com, pobrn@...tonmail.com,
        platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/6] asus-wmi: Implement TUF laptop keyboard RGB
 control

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.

Powered by blists - more mailing lists