| Message ID | 20220808030420.8633-4-luke@ljones.dev (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | asus-wmi: Add support for RGB keyboards | expand |
On Mon, Aug 8, 2022 at 5:09 AM Luke D. Jones <luke@ljones.dev> wrote: > > Adds support for setting various power states of TUF keyboards. > These states are combinations of: > - boot, set if a boot animation is shown on keyboard > - awake, set if the keyboard LEDs are visible while laptop is on > - sleep, set if an animation is displayed while the laptop is suspended > - keyboard (unknown effect) > > Adds two sysfs attributes to asus-nb-wmi: > - keyboard_rgb_state > - keyboard_rgb_state_index ... > + flags = 0; This can be done before 'if (boot)' > + if (sscanf(buf, "%hhd %hhd %hhd %hhd %hhd", &save, &boot, &awake, &sleep, &keyboard) != 5) > + return -EINVAL; Same Q here: wouldn't it be better to put each of the parameters to a separate sysfs node? Or look at the LED ABI (that what Pavel mentioned for multi-color patterns) and see if there are already some established ways of how to represent necessary information? > + save = save == 0 ? 0x0100 : 0x0000; if (save) flags = BIT(8); > + if (boot) > + flags |= 0x02; > + if (awake) > + flags |= 0x08; > + if (sleep) > + flags |= 0x20; > + if (keyboard) > + flags |= 0x80; Use BIT() for flags. ... > + err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, > + ASUS_WMI_DEVID_TUF_RGB_STATE, 0xBD | save | (flags << 16), 0, &ret); Why not provide flags to be a full 32-bit value? Also 0xBD can be lower-cased and explained somehow? ... > + No need for a blank line. > +static DEVICE_ATTR_WO(keyboard_rgb_state); ... > + > +static DEVICE_ATTR_RO(keyboard_rgb_state_index); Ditto and same for many other similar cases. ... > #define ASUS_WMI_DSTS_STATUS_BIT 0x00000001 BIT(0) ? (This might require to add bits.h inclusion)
Hi Andy >> + flags = 0; > > This can be done before 'if (boot)' Okay done. > >> + if (sscanf(buf, "%hhd %hhd %hhd %hhd %hhd", &save, &boot, >> &awake, &sleep, &keyboard) != 5) >> + return -EINVAL; > > Same Q here: wouldn't it be better to put each of the parameters to a > separate sysfs node? Or look at the LED ABI (that what Pavel mentioned > for multi-color patterns) and see if there are already some > established ways of how to represent necessary information? Same argument I make for the RGB mode nodes. But here I think it's probably even more pertinent. The reasons I would like to keep this as one node are: - It's separate to the RGB part - We can't read the device to set defaults on boot - Because of the above, if we set a default and the user wants to change perhaps "sleep", then we're going to have to write some incorrect guess data since the write requires all the flags at once - One way to improve the UX is to add _show, but then this has to display incorrect data on boot - We end up with 5 more nodes The same reasons above apply to the RGB nodes, which right now I'm of two minds about. We'll see which way the RGB mode patch goes after some daily use. > >> + save = save == 0 ? 0x0100 : 0x0000; > > if (save) > flags = BIT(8); I didn't know about BIT(). Will do. > >> + if (boot) >> + flags |= 0x02; >> + if (awake) >> + flags |= 0x08; >> + if (sleep) >> + flags |= 0x20; >> + if (keyboard) >> + flags |= 0x80; > > Use BIT() for flags. > > ... > >> + err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, >> + ASUS_WMI_DEVID_TUF_RGB_STATE, 0xBD | save | >> (flags << 16), 0, &ret); > > Why not provide flags to be a full 32-bit value? > > Also 0xBD can be lower-cased and explained somehow? Done, as is the rest of comments Kind regards, Luke. >
On Tue, Aug 9, 2022 at 1:27 AM Luke Jones <luke@ljones.dev> wrote: ... > >> + if (sscanf(buf, "%hhd %hhd %hhd %hhd %hhd", &save, &boot, > >> &awake, &sleep, &keyboard) != 5) > >> + return -EINVAL; > > > > Same Q here: wouldn't it be better to put each of the parameters to a > > separate sysfs node? Or look at the LED ABI (that what Pavel mentioned > > for multi-color patterns) and see if there are already some > > established ways of how to represent necessary information? > > Same argument I make for the RGB mode nodes. But here I think it's > probably even more pertinent. The reasons I would like to keep this as > one node are: > > - It's separate to the RGB part > - We can't read the device to set defaults on boot Hmm... Maybe it's done via one of the WMI calls? > - Because of the above, if we set a default and the user wants to > change perhaps "sleep", then we're going to have to write some > incorrect guess data since the write requires all the flags at once > - One way to improve the UX is to add _show, but then this has to > display incorrect data on boot > - We end up with 5 more nodes > > The same reasons above apply to the RGB nodes, which right now I'm of > two minds about. We'll see which way the RGB mode patch goes after some > daily use. I just realized that in previous mail I mentioned Device Tree which is irrelevant here. We can't use it on x86 traditional platforms, so it means that platform should somehow pass the data to the OS one way or another. If there is no way to read back (bad designed interfaces), then we can only reset to whatever user provides.
G'day Andy, On Tue, 2022-08-09 at 10:29 +0200, Andy Shevchenko wrote: > On Tue, Aug 9, 2022 at 1:27 AM Luke Jones <luke@ljones.dev> wrote: > > ... > > > > > + if (sscanf(buf, "%hhd %hhd %hhd %hhd %hhd", &save, > > > > &boot, > > > > &awake, &sleep, &keyboard) != 5) > > > > + return -EINVAL; > > > > > > Same Q here: wouldn't it be better to put each of the parameters > > > to a > > > separate sysfs node? Or look at the LED ABI (that what Pavel > > > mentioned > > > for multi-color patterns) and see if there are already some > > > established ways of how to represent necessary information? > > > > Same argument I make for the RGB mode nodes. But here I think it's > > probably even more pertinent. The reasons I would like to keep this > > as > > one node are: > > > > - It's separate to the RGB part > > - We can't read the device to set defaults on boot > > Hmm... Maybe it's done via one of the WMI calls? That was my hope, but I'm unable to find one. I'm fairly certain that this set of keyboards uses the same controller as the USB connected one (the USB one has two versions in circulation also), and I've not been able to find any packet data that indicates the USB ones send a "supported". Checking with `fwts wmi -` reveals nothing (all passes). I've emailed my contact in the ROG engineering team at ASUS to see if they can provide any insight. > > > - Because of the above, if we set a default and the user wants to > > change perhaps "sleep", then we're going to have to write some > > incorrect guess data since the write requires all the flags at once > > - One way to improve the UX is to add _show, but then this has to > > display incorrect data on boot > > - We end up with 5 more nodes > > > > The same reasons above apply to the RGB nodes, which right now I'm > > of > > two minds about. We'll see which way the RGB mode patch goes after > > some > > daily use. > > I just realized that in previous mail I mentioned Device Tree which > is > irrelevant here. We can't use it on x86 traditional platforms, so it > means that platform should somehow pass the data to the OS one way or > another. If there is no way to read back (bad designed interfaces), > then we can only reset to whatever user provides. > Umm.. Do you mean: - load module - module sets a default (all on) - user or userspace-util sets user preference? Given that the system daemon I develop (asusd + asusctl) is in very wide use, I guess it's not such a big issue to both split these nodes out and set a default.. I guess I'll go ahead and keep the expectation that the reworked RGB-mode patch sets. It seems to me that the appropriate way to do the "write-out" for both mode and state is to have nodes: - keyboard_rgb_mode_apply - keyboard_rgb_state_apply - Input 0 = set (doesn't stick on boot), 1 = save going with the above I should rename existing nodes, especially the current *_save node. And this brings me to my next issue: currently behaviour for the *_apply is: - write 0 applies, but doesn't stick - write 1 applies, and sticks on boot - reading the *_apply nodes will show 0/1 - if already "1", you still need to overwrite with "1" to apply. This doesn't seem appropriate does it? Should there be a WO node for *_apply, and another node for *_save (which would store/show the setting)? I'm inclined to think so, and that this will add quite a bit of clutter (6 nodes for state, 4 for mode). Kind regards, Luke.
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index 4c2bdd9dac30..9b2c54726955 100644 --- a/drivers/platform/x86/asus-wmi.c +++ b/drivers/platform/x86/asus-wmi.c @@ -246,6 +246,7 @@ struct asus_wmi { bool dgpu_disable_available; bool dgpu_disable; + bool keyboard_rgb_state_available; bool keyboard_rgb_mode_available; struct keyboard_rgb_led keyboard_rgb_mode; @@ -820,6 +821,68 @@ static ssize_t keyboard_rgb_mode_index_show(struct device *device, static DEVICE_ATTR_RO(keyboard_rgb_mode_index); +/* TUF Laptop Keyboard RGB States *********************************************/ +static int keyboard_rgb_state_check_present(struct asus_wmi *asus) +{ + u32 result; + int err; + + asus->keyboard_rgb_state_available = false; + + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_TUF_RGB_STATE, &result); + if (err) { + if (err == -ENODEV) + return 0; + return err; + } + + if (result & ASUS_WMI_DSTS_PRESENCE_BIT) + asus->keyboard_rgb_state_available = true; + + return 0; +} + +static ssize_t keyboard_rgb_state_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + u8 flags, save, boot, awake, sleep, keyboard; + int err; + u32 ret; + + flags = 0; + if (sscanf(buf, "%hhd %hhd %hhd %hhd %hhd", &save, &boot, &awake, &sleep, &keyboard) != 5) + return -EINVAL; + + save = save == 0 ? 0x0100 : 0x0000; + if (boot) + flags |= 0x02; + if (awake) + flags |= 0x08; + if (sleep) + flags |= 0x20; + if (keyboard) + flags |= 0x80; + + err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, + ASUS_WMI_DEVID_TUF_RGB_STATE, 0xBD | save | (flags << 16), 0, &ret); + if (err) + return err; + + return count; +} + +static DEVICE_ATTR_WO(keyboard_rgb_state); + +static ssize_t keyboard_rgb_state_index_show(struct device *device, + struct device_attribute *attr, + char *buf) +{ + return sysfs_emit(buf, "%s\n", "save boot awake sleep keyboard"); +} + +static DEVICE_ATTR_RO(keyboard_rgb_state_index); + /* Battery ********************************************************************/ /* The battery maximum charging percentage */ @@ -3412,6 +3475,8 @@ static struct attribute *platform_attributes[] = { &dev_attr_dgpu_disable.attr, &dev_attr_keyboard_rgb_mode.attr, &dev_attr_keyboard_rgb_mode_index.attr, + &dev_attr_keyboard_rgb_state.attr, + &dev_attr_keyboard_rgb_state_index.attr, &dev_attr_lid_resume.attr, &dev_attr_als_enable.attr, &dev_attr_fan_boost_mode.attr, @@ -3446,6 +3511,10 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj, ok = asus->keyboard_rgb_mode_available; else if (attr == &dev_attr_keyboard_rgb_mode_index.attr) ok = asus->keyboard_rgb_mode_available; + else if (attr == &dev_attr_keyboard_rgb_state.attr) + ok = asus->keyboard_rgb_state_available; + else if (attr == &dev_attr_keyboard_rgb_state_index.attr) + ok = asus->keyboard_rgb_state_available; else if (attr == &dev_attr_fan_boost_mode.attr) ok = asus->fan_boost_mode_available; else if (attr == &dev_attr_throttle_thermal_policy.attr) @@ -3719,6 +3788,10 @@ static int asus_wmi_add(struct platform_device *pdev) if (err) goto fail_keyboard_rgb_mode; + err = keyboard_rgb_state_check_present(asus); + if (err) + goto fail_keyboard_rgb_state; + err = fan_boost_mode_check_present(asus); if (err) goto fail_fan_boost_mode; @@ -3834,6 +3907,7 @@ static int asus_wmi_add(struct platform_device *pdev) fail_egpu_enable: fail_dgpu_disable: fail_keyboard_rgb_mode: +fail_keyboard_rgb_state: fail_platform: fail_panel_od: kfree(asus); diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h index d63c9945a17d..b5c966798ef8 100644 --- a/include/linux/platform_data/x86/asus-wmi.h +++ b/include/linux/platform_data/x86/asus-wmi.h @@ -100,6 +100,8 @@ /* TUF laptop RGB control */ #define ASUS_WMI_DEVID_TUF_RGB_MODE 0x00100056 +/* TUF laptop RGB state control */ +#define ASUS_WMI_DEVID_TUF_RGB_STATE 0x00100057 /* DSTS masks */ #define ASUS_WMI_DSTS_STATUS_BIT 0x00000001
Adds support for setting various power states of TUF keyboards. These states are combinations of: - boot, set if a boot animation is shown on keyboard - awake, set if the keyboard LEDs are visible while laptop is on - sleep, set if an animation is displayed while the laptop is suspended - keyboard (unknown effect) Adds two sysfs attributes to asus-nb-wmi: - keyboard_rgb_state - keyboard_rgb_state_index Signed-off-by: Luke D. Jones <luke@ljones.dev> --- drivers/platform/x86/asus-wmi.c | 74 ++++++++++++++++++++++ include/linux/platform_data/x86/asus-wmi.h | 2 + 2 files changed, 76 insertions(+)