All of lore.kernel.org
 help / color / mirror / Atom feed
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
Subject: Re: [PATCH v3 1/6] asus-wmi: Implement TUF laptop keyboard RGB control
Date: Wed, 10 Aug 2022 16:44:20 +1200	[thread overview]
Message-ID: <fcc7b7eb29abc1ac9053bce02fd9f705e5f06b0b.camel@ljones.dev> (raw)
In-Reply-To: <20220809105031.GA4971@duo.ucw.cz>

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

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 = "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.

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.


  reply	other threads:[~2022-08-10  4:44 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-09  2:50 [PATCH v3 0/6] asus-wmi: Add support for RGB keyboards Luke D. Jones
2022-08-09  2:50 ` [PATCH v3 1/6] asus-wmi: Implement TUF laptop keyboard RGB control Luke D. Jones
2022-08-09  8:46   ` Andy Shevchenko
2022-08-09  8:55     ` Luke Jones
2022-08-09  9:29       ` Andy Shevchenko
2022-08-09  9:31         ` Luke Jones
2022-08-09 10:50   ` Pavel Machek
2022-08-10  4:44     ` Luke Jones [this message]
2022-08-11 15:01       ` Hans de Goede
2022-08-11 15:05         ` Hans de Goede
2022-08-11 22:13           ` Luke Jones
2022-08-09  2:50 ` [PATCH v3 2/6] asus-wmi: Implement TUF laptop keyboard LED modes Luke D. Jones
2022-08-09  3:16   ` Luke Jones
2022-08-09  9:22   ` Andy Shevchenko
2022-08-09  9:30     ` Luke Jones
2022-08-09  2:50 ` [PATCH v3 3/6] asus-wmi: Implement TUF laptop keyboard power states Luke D. Jones
2022-08-09  8:52   ` Andy Shevchenko
2022-08-09  8:58     ` Luke Jones
2022-08-09  2:50 ` [PATCH v3 4/6] asus-wmi: Document previously added attributes Luke D. Jones
2022-08-09  9:25   ` Andy Shevchenko
2022-08-11 15:08     ` Hans de Goede
2022-08-11 22:08       ` Luke Jones
2022-08-09  2:50 ` [PATCH v3 5/6] asus-wmi: Convert all attr-show to use sysfs_emit Luke D. Jones
2022-08-09  9:27   ` Andy Shevchenko
2022-08-11 18:52     ` Hans de Goede
2022-08-09  2:50 ` [PATCH v3 6/6] asus-wmi: Support the hardware GPU MUX on some laptops Luke D. Jones
2022-08-09  7:19   ` Luke Jones
2022-08-11 13:53   ` Hans de Goede
2022-08-11 22:01     ` Luke Jones
2022-08-12  7:59       ` Hans de Goede
2022-08-12  8:31         ` Thomas Weißschuh
2022-08-12  8:44           ` Hans de Goede
2022-08-09  8:55 ` [PATCH v3 0/6] asus-wmi: Add support for RGB keyboards Andy Shevchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fcc7b7eb29abc1ac9053bce02fd9f705e5f06b0b.camel@ljones.dev \
    --to=luke@ljones.dev \
    --cc=andy.shevchenko@gmail.com \
    --cc=hdegoede@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pavel@ucw.cz \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=pobrn@protonmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.