Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i5ec1447f:Fastmail
Message-ID: <fcc7b7eb29abc1ac9053bce02fd9f705e5f06b0b.camel@ljones.dev>
Subject: Re: [PATCH v3 1/6] asus-wmi: Implement TUF laptop keyboard RGB
 control
From:   Luke Jones <luke@ljones.dev>
To:     Pavel Machek <pavel@ucw.cz>
Cc:     hdegoede@redhat.com, andy.shevchenko@gmail.com,
        pobrn@protonmail.com, platform-driver-x86@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Wed, 10 Aug 2022 16:44:20 +1200
In-Reply-To: <20220809105031.GA4971@duo.ucw.cz>
References: <20220809025054.1626339-1-luke@ljones.dev>
         <20220809025054.1626339-2-luke@ljones.dev>
         <20220809105031.GA4971@duo.ucw.cz>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (by Flathub.org) 
MIME-Version: 1.0
Precedence: bulk

Hi Pavel, Andy, Hans,

> > > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/*
> > > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * asus::kbd_backlight still controls a
> > > > > > > > base > > > > > > 3-level backlight and when
> > > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * it is on 0, the RGB is not visible
> > > > > > > > at all. > > > > RGB > > should be treated as
> > > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * an additional step.
> > > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 */
> > > >=20
> > > > Ouch. Lets not do that? If rgb interface is available, hide the
> > > > 3
> > > > level one, or something.
> > > >=20

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.


> > > > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mc_cdev->led_cdev.name =3D=C2=A0=C2=A0 > >=
 > > > >
> > > > > > > > "asus::multicolour::kbd_backlight";
> > > >=20
> > > > 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

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:

- keyboard_rgb_apply, new WO node to actually write out data
- keyboard_rgb_save, first parameter of packet, retain-on-boot
- keyboard_rgb_mode, the factory built-in modes,
- keyboard_rgb_speed, speed of certain modes

And then for power-state:

- keyboard_state_apply, new WO node to actually write out data
- keyboard_state_save, first parameter of packet, retain-on-boot
- keyboard_state_boot, play boot animation (on boot)
- keyboard_state_awake, LEDs visible while awake
- keyboard_state_sleep, play suspend animation (while suspended)
- keyboard_state_keyboard, unknown effect

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 =3D "n n n n n n"
- keyboard_rgb_mode_index, output =3D "save/apply, mode, r, g, b, speed"
- keyboard_rgb_state, WO =3D "n n n n n"
- keyboard_rgb_state_index, output =3D "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.

Multicolor class could still be used, but from everything I've tried
now it really isn't suitable when the proper method for brightness is a
separate WMI method (0-3), and when that is 0 it means LEDs are fully
off - there's potential for mistakes/issues. Losing led-notif is an
issue for users for sure.

In short, from dog-fooding the current state inlcuding the trial of
using multicolor brightness (and hiding the proper WMI method) I can
only conclude that multicolor is not suitable for how these keyboards
work.

Hans, Andy, can I please revert back to the node + _index pairs taking
an array input. Everything will be cleaner and simpler.

Cheers,
Luke.