This is a patch series to add RGB support for ASUS laptops.
The laptops with this RGB tend to be the TUF series of gamer laptops.
The first step is initial bringup of support using the multicolor LED API.
These types of keyboards implement a slightly more complex interface than
just RGB control however - they also have modes with can be static LED,
blinking, rainbow, color cycles, and more. They also have some custom
animations that can play depending on device state, such as suspended
playing a fancy colour cycle, or playing a "wave" animation.
Two of the patches add support for these features.
The last patch adds documentation in:
Documentation/ABI/testing/sysfs-platform-asus-wmi
Some notes:
- this patch series obsoletes the previous RGB patches by myself
- it is not possible to add attribute groups to multicolor LED as
they get overwritten by `led_multicolor_groups` in
`led_classdev_multicolor_register_ext`.
- the methods for RGB control do not provide a way to fetch exisiting
state, so these methods are WO.
- There is an existing `asus::kbd_backlight`, this provides a 4-step
brightness to the RGB (off,low,med,high) individually to multicolor.
I was unsure of the effect of adding a similar path so have used the
`asus::multicolour::kbd_backlight` name to be clear about purpose.
If the `asus::kbd_backlight` is off, then no RGB is shown at all.
Changelog:
- V2: patch 1: asus-wmi - RGB
+ shorten a few lines
+ move unused keyboard_rgb_mode_available to next patch
+ remove tuf_rgb_brightness_get() is it was only returning current
led_classdev brightness, not reading it from device
+ remove unnecessary setting of brightness on multicolor init
+ set brightness_get to null for TUF RGB
+ actually use the member subled_info in keyboard_rgb_led struct and
not the leftover dynamic allocation (now removed)
- V2: patch 2: asus-wmi RGB mode control
+ tuf_rgb_brightness_set() was still using hardcoded save/mode/speed
from testing. This is now using the pre-set default.
+ asus_wmi_led_init(), set speed value to a correct value
+ keyboard_rgb_mode_store() return count, not 0
+ correctly unregister the mulicolor led on module exit
+ use switch/case in keyboard_rgb_mode_store() for speed
+ remove a single line bracket block
- V2: patch 3: asus-wmi RGB power control
+ Try to fix the indent warning from buildbot
+ Use correct date on added API docs
+ Add missing panel_od doc
+ correctly label attribute for keyboard_rgb_state
- V2: patch 4: documentation
+ Add commit message
- V2: patch 5: attributes using sysfs_emit:
+ Add commit message
- V2: patch 6: new patch, dgpu_only mode
+ A previous patch I submitted "0006-asus-wmi-Add-support-for-dGPU-only-mode.patch"
is now part of this series due to merge conflicts caused by the
previous 5 patches
Previous patches obsoleted:
https://lkml.org/lkml/2022/8/3/885
https://lkml.org/lkml/2022/8/3/886
https://lkml.org/lkml/2022/8/3/44
I may not have listed all.
Luke D. Jones (6):
asus-wmi: Implement TUF laptop keyboard RGB control
asus-wmi: Implement TUF laptop keyboard LED modes
asus-wmi: Implement TUF laptop keyboard power states
asus-wmi: Document previously added attributes
asus-wmi: Convert all attr-show to use sysfs_emit
asus-wmi: Add support for dGPU-only mode
.../ABI/testing/sysfs-platform-asus-wmi | 68 ++++
drivers/platform/x86/asus-wmi.c | 343 +++++++++++++++++-
include/linux/platform_data/x86/asus-wmi.h | 8 +
3 files changed, 412 insertions(+), 7 deletions(-)
--
2.37.1
Documents some previously added attributes:
- dgpu_disable
- egpu_enable
- panel_od
- keyboard_rgb_mode
- keyboard_rgb_state
Signed-off-by: Luke D. Jones <[email protected]>
---
.../ABI/testing/sysfs-platform-asus-wmi | 59 +++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 04885738cf15..66b262476d92 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -57,3 +57,62 @@ Description:
* 0 - default,
* 1 - overboost,
* 2 - silent
+
+What: /sys/devices/platform/<platform>/dgpu_disable
+Date: Aug 2022
+KernelVersion: 5.17
+Contact: "Luke Jones" <[email protected]>
+Description:
+ Disable discrete GPU:
+ * 0 - Enable dGPU,
+ * 1 - Disable dGPU,
+
+What: /sys/devices/platform/<platform>/egpu_enable
+Date: Aug 2022
+KernelVersion: 5.17
+Contact: "Luke Jones" <[email protected]>
+Description:
+ Enable the external GPU paired with ROG X-Flow laptops.
+ Toggling this setting will also trigger ACPI to disable the dGPU:
+ * 0 - Disable,
+ * 1 - Enable,
+
+What: /sys/devices/platform/<platform>/panel_od
+Date: Aug 2022
+KernelVersion: 5.17
+Contact: "Luke Jones" <[email protected]>
+Description:
+ Enable an LCD response-time boost to reduce or remove ghosting:
+ * 0 - Disable,
+ * 1 - Enable,
+
+What: /sys/devices/platform/<platform>/keyboard_rgb_mode
+Date: Aug 2022
+KernelVersion: 6.0
+Contact: "Luke Jones" <[email protected]>
+Description:
+ Set some RGB keyboard modes and features (write-only).
+
+ The accepted input is "save mode speed", where "n n n" options
+ are:
+ * save - 0 or 1, if 0 then settings are not retained on boot
+ * mode - 0 to 12, each is an RGB such as static, rainbow, pulse.
+ Not all keyboards accept every mode.
+ * speed - 0, 1, 2, equal to low, medium, high.
+ Only applies to certain modes.
+
+What: /sys/devices/platform/<platform>/keyboard_rgb_state
+Date: Aug 2022
+KernelVersion: 6.0
+Contact: "Luke Jones" <[email protected]>
+Description:
+ Set some RGB keyboard power states (write-only).
+
+ The accepted input is "boot awake sleep keyboard", where "n n n n n"
+ options are:
+ * save - 0 or 1, if 0 then settings are not retained on boot
+ * boot - 0 or 1, controls if a boot animation is shown
+ * awake - 0 or 1, controls if the keyboard LED are on during awake
+ * sleep - 0 or 1, controls if a suspended animation is shown.
+ This is only active if the AC is connected.
+ * keyboard - 0 or 1, unknown what effect this really has
--
2.37.1
This changes all *_show attributes in asus-wmi.c to use sysfs_emit
instead of the older method of writing to the output buffer manually.
Signed-off-by: Luke D. Jones <[email protected]>
---
drivers/platform/x86/asus-wmi.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 9b2c54726955..b9e5d87e3e18 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -919,7 +919,7 @@ static ssize_t charge_control_end_threshold_show(struct device *device,
struct device_attribute *attr,
char *buf)
{
- return sprintf(buf, "%d\n", charge_end_threshold);
+ return sysfs_emit(buf, "%d\n", charge_end_threshold);
}
static DEVICE_ATTR_RW(charge_control_end_threshold);
@@ -2009,7 +2009,7 @@ static ssize_t pwm1_show(struct device *dev,
value = -1;
}
- return sprintf(buf, "%d\n", value);
+ return sysfs_emit(buf, "%d\n", value);
}
static ssize_t pwm1_store(struct device *dev,
@@ -2069,7 +2069,7 @@ static ssize_t fan1_input_show(struct device *dev,
return -ENXIO;
}
- return sprintf(buf, "%d\n", value < 0 ? -1 : value*100);
+ return sysfs_emit(buf, "%d\n", value < 0 ? -1 : value*100);
}
static ssize_t pwm1_enable_show(struct device *dev,
@@ -2087,7 +2087,7 @@ static ssize_t pwm1_enable_show(struct device *dev,
* in practice on X532FL at least (the bit is always 0) and there's
* also nothing in the DSDT to indicate that this behaviour exists.
*/
- return sprintf(buf, "%d\n", asus->fan_pwm_mode);
+ return sysfs_emit(buf, "%d\n", asus->fan_pwm_mode);
}
static ssize_t pwm1_enable_store(struct device *dev,
@@ -2155,7 +2155,7 @@ static ssize_t fan1_label_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- return sprintf(buf, "%s\n", ASUS_FAN_DESC);
+ return sysfs_emit(buf, "%s\n", ASUS_FAN_DESC);
}
static ssize_t asus_hwmon_temp1(struct device *dev,
@@ -2348,7 +2348,7 @@ static ssize_t fan_boost_mode_show(struct device *dev,
{
struct asus_wmi *asus = dev_get_drvdata(dev);
- return scnprintf(buf, PAGE_SIZE, "%d\n", asus->fan_boost_mode);
+ return sysfs_emit(buf, "%d\n", asus->fan_boost_mode);
}
static ssize_t fan_boost_mode_store(struct device *dev,
@@ -2901,7 +2901,7 @@ static ssize_t throttle_thermal_policy_show(struct device *dev,
struct asus_wmi *asus = dev_get_drvdata(dev);
u8 mode = asus->throttle_thermal_policy_mode;
- return scnprintf(buf, PAGE_SIZE, "%d\n", mode);
+ return sysfs_emit(buf, "%d\n", mode);
}
static ssize_t throttle_thermal_policy_store(struct device *dev,
--
2.37.1
Adds support for a dGPU-only mode on some laptops where when enabled
the boot GPU is the dGPU, and the iGPU is not visible.
Signed-off-by: Luke D. Jones <[email protected]>
---
.../ABI/testing/sysfs-platform-asus-wmi | 9 ++
drivers/platform/x86/asus-wmi.c | 92 +++++++++++++++++++
include/linux/platform_data/x86/asus-wmi.h | 3 +
3 files changed, 104 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 66b262476d92..93d111a65313 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -77,6 +77,15 @@ Description:
* 0 - Disable,
* 1 - Enable,
+What: /sys/devices/platform/<platform>/dgpu_only
+Date: Aug 2022
+KernelVersion: 6.0
+Contact: "Luke Jones" <[email protected]>
+Description:
+ Set the dGPU to be the only GPU available:
+ * 0 - Disable,
+ * 1 - Enable,
+
What: /sys/devices/platform/<platform>/panel_od
Date: Aug 2022
KernelVersion: 5.17
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index b9e5d87e3e18..840299828512 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -246,6 +246,9 @@ struct asus_wmi {
bool dgpu_disable_available;
bool dgpu_disable;
+ bool dgpu_only_available;
+ bool dgpu_only;
+
bool keyboard_rgb_state_available;
bool keyboard_rgb_mode_available;
struct keyboard_rgb_led keyboard_rgb_mode;
@@ -750,6 +753,87 @@ static ssize_t egpu_enable_store(struct device *dev,
static DEVICE_ATTR_RW(egpu_enable);
+/* dedicated GPU only *********************************************************/
+static int dgpu_only_check_present(struct asus_wmi *asus)
+{
+ u32 result;
+ int err;
+
+ asus->dgpu_only_available = false;
+
+ err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_DGPU, &result);
+ if (err) {
+ if (err == -ENODEV)
+ return 0;
+ return err;
+ }
+
+ if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
+ asus->dgpu_only_available = true;
+ asus->dgpu_only = result & ASUS_WMI_DSTS_STATUS_BIT;
+ }
+
+ return 0;
+}
+
+static int dgpu_only_write(struct asus_wmi *asus)
+{
+ u32 retval;
+ u8 value;
+ int err;
+
+ /* Don't rely on type conversion */
+ value = asus->dgpu_only ? 1 : 0;
+
+ err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval);
+ if (err) {
+ pr_warn("Failed to set dGPU-only mode: %d\n", err);
+ return err;
+ }
+
+ if (retval > 1) {
+ pr_warn("Failed to set dGPU-only mode (retval): 0x%x\n", retval);
+ return -EIO;
+ }
+
+ sysfs_notify(&asus->platform_device->dev.kobj, NULL, "dgpu_only");
+
+ return 0;
+}
+
+static ssize_t dgpu_only_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct asus_wmi *asus = dev_get_drvdata(dev);
+ u8 mode = asus->dgpu_only;
+
+ return sysfs_emit(buf, "%d\n", mode);
+}
+
+static ssize_t dgpu_only_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ bool enable;
+ int result;
+
+ struct asus_wmi *asus = dev_get_drvdata(dev);
+
+ result = kstrtobool(buf, &enable);
+ if (result)
+ return result;
+
+ asus->dgpu_only = enable;
+
+ result = dgpu_only_write(asus);
+ if (result)
+ return result;
+
+ return count;
+}
+
+static DEVICE_ATTR_RW(dgpu_only);
+
/* TUF Laptop Keyboard RGB Modes **********************************************/
static int keyboard_rgb_mode_check_present(struct asus_wmi *asus)
{
@@ -3473,6 +3557,7 @@ static struct attribute *platform_attributes[] = {
&dev_attr_touchpad.attr,
&dev_attr_egpu_enable.attr,
&dev_attr_dgpu_disable.attr,
+ &dev_attr_dgpu_only.attr,
&dev_attr_keyboard_rgb_mode.attr,
&dev_attr_keyboard_rgb_mode_index.attr,
&dev_attr_keyboard_rgb_state.attr,
@@ -3507,6 +3592,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
ok = asus->egpu_enable_available;
else if (attr == &dev_attr_dgpu_disable.attr)
ok = asus->dgpu_disable_available;
+ else if (attr == &dev_attr_dgpu_only.attr)
+ ok = asus->dgpu_only_available;
else if (attr == &dev_attr_keyboard_rgb_mode.attr)
ok = asus->keyboard_rgb_mode_available;
else if (attr == &dev_attr_keyboard_rgb_mode_index.attr)
@@ -3784,6 +3871,10 @@ static int asus_wmi_add(struct platform_device *pdev)
if (err)
goto fail_dgpu_disable;
+ err = dgpu_only_check_present(asus);
+ if (err)
+ goto fail_dgpu_only;
+
err = keyboard_rgb_mode_check_present(asus);
if (err)
goto fail_keyboard_rgb_mode;
@@ -3906,6 +3997,7 @@ static int asus_wmi_add(struct platform_device *pdev)
fail_fan_boost_mode:
fail_egpu_enable:
fail_dgpu_disable:
+fail_dgpu_only:
fail_keyboard_rgb_mode:
fail_keyboard_rgb_state:
fail_platform:
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index b5c966798ef8..76b0756a0666 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -98,6 +98,9 @@
/* dgpu on/off */
#define ASUS_WMI_DEVID_DGPU 0x00090020
+/* Dedicated GPU only. When active the dGPU will be the only visible GPU */
+#define ASUS_WMI_DEVID_DEDICATED 0x00090016
+
/* TUF laptop RGB control */
#define ASUS_WMI_DEVID_TUF_RGB_MODE 0x00100056
/* TUF laptop RGB state control */
--
2.37.1
Adds support for changing the laptop keyboard LED modes. These
are visible effects such as static, rainbow, pulsing, colour cycles.
Two sysfs attributes are added to asus-nb-wmi:
- keyboard_rgb_mode
- keyboard_rgb_mode_index
Signed-off-by: Luke D. Jones <[email protected]>
---
drivers/platform/x86/asus-wmi.c | 83 +++++++++++++++++++++++++++++++++
1 file changed, 83 insertions(+)
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 233e73f4313d..4c2bdd9dac30 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_mode_available;
struct keyboard_rgb_led keyboard_rgb_mode;
bool throttle_thermal_policy_available;
@@ -748,6 +749,77 @@ static ssize_t egpu_enable_store(struct device *dev,
static DEVICE_ATTR_RW(egpu_enable);
+/* TUF Laptop Keyboard RGB Modes **********************************************/
+static int keyboard_rgb_mode_check_present(struct asus_wmi *asus)
+{
+ u32 result;
+ int err;
+
+ asus->keyboard_rgb_mode_available = false;
+
+ err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_TUF_RGB_MODE, &result);
+ if (err) {
+ if (err == -ENODEV)
+ return 0;
+ return err;
+ }
+
+ if (result & ASUS_WMI_DSTS_PRESENCE_BIT)
+ asus->keyboard_rgb_mode_available = true;
+
+ return 0;
+}
+
+static ssize_t keyboard_rgb_mode_store(struct device *device,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ u8 save, mode, speed;
+ int err;
+
+ struct asus_wmi *asus = dev_get_drvdata(device);
+ struct led_classdev *cdev = &asus->keyboard_rgb_mode.dev.led_cdev;
+
+ if (sscanf(buf, "%hhd %hhd %hhd", &save, &mode, &speed) != 3)
+ return -EINVAL;
+
+ asus->keyboard_rgb_mode.save = !!save;
+
+ /* These are the known usable modes across all TUF/ROG */
+ asus->keyboard_rgb_mode.mode = mode < 12 && mode != 9 ? mode : 0x0a;
+
+ switch (speed) {
+ case 0:
+ asus->keyboard_rgb_mode.speed = 0xe1;
+ break;
+ case 1:
+ asus->keyboard_rgb_mode.speed = 0xeb;
+ break;
+ case 2:
+ asus->keyboard_rgb_mode.speed = 0xf5;
+ break;
+ default:
+ asus->keyboard_rgb_mode.speed = 0xeb;
+ }
+
+ err = tuf_rgb_brightness_set(cdev, cdev->brightness);
+ if (err)
+ return err;
+
+ return count;
+}
+
+static DEVICE_ATTR_WO(keyboard_rgb_mode);
+
+static ssize_t keyboard_rgb_mode_index_show(struct device *device,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return sysfs_emit(buf, "%s\n", "save mode speed");
+}
+
+static DEVICE_ATTR_RO(keyboard_rgb_mode_index);
+
/* Battery ********************************************************************/
/* The battery maximum charging percentage */
@@ -3338,6 +3410,8 @@ static struct attribute *platform_attributes[] = {
&dev_attr_touchpad.attr,
&dev_attr_egpu_enable.attr,
&dev_attr_dgpu_disable.attr,
+ &dev_attr_keyboard_rgb_mode.attr,
+ &dev_attr_keyboard_rgb_mode_index.attr,
&dev_attr_lid_resume.attr,
&dev_attr_als_enable.attr,
&dev_attr_fan_boost_mode.attr,
@@ -3368,6 +3442,10 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
ok = asus->egpu_enable_available;
else if (attr == &dev_attr_dgpu_disable.attr)
ok = asus->dgpu_disable_available;
+ else if (attr == &dev_attr_keyboard_rgb_mode.attr)
+ 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_fan_boost_mode.attr)
ok = asus->fan_boost_mode_available;
else if (attr == &dev_attr_throttle_thermal_policy.attr)
@@ -3637,6 +3715,10 @@ static int asus_wmi_add(struct platform_device *pdev)
if (err)
goto fail_dgpu_disable;
+ err = keyboard_rgb_mode_check_present(asus);
+ if (err)
+ goto fail_keyboard_rgb_mode;
+
err = fan_boost_mode_check_present(asus);
if (err)
goto fail_fan_boost_mode;
@@ -3751,6 +3833,7 @@ static int asus_wmi_add(struct platform_device *pdev)
fail_fan_boost_mode:
fail_egpu_enable:
fail_dgpu_disable:
+fail_keyboard_rgb_mode:
fail_platform:
fail_panel_od:
kfree(asus);
--
2.37.1
Adds support for TUF laptop RGB control via the multicolor LED API.
As this is the bas for adjusting only the RGB values, it sets the
default mode of the keyboard to static since there is no way to read
any existing settings from the device. These defaults overwrite the
booted state of the keyboard when the module is loaded.
Signed-off-by: Luke D. Jones <[email protected]>
---
drivers/platform/x86/asus-wmi.c | 80 ++++++++++++++++++++++
include/linux/platform_data/x86/asus-wmi.h | 3 +
2 files changed, 83 insertions(+)
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 0e7fbed8a50d..233e73f4313d 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -25,6 +25,7 @@
#include <linux/input/sparse-keymap.h>
#include <linux/kernel.h>
#include <linux/leds.h>
+#include <linux/led-class-multicolor.h>
#include <linux/module.h>
#include <linux/pci.h>
#include <linux/pci_hotplug.h>
@@ -117,6 +118,9 @@ static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
static int throttle_thermal_policy_write(struct asus_wmi *);
+static int tuf_rgb_brightness_set(struct led_classdev *cdev,
+ enum led_brightness brightness);
+
static bool ashs_present(void)
{
int i = 0;
@@ -190,6 +194,14 @@ struct fan_curve_data {
u8 percents[FAN_CURVE_POINTS];
};
+struct keyboard_rgb_led {
+ struct mc_subled subled_info[3]; /* r g b */
+ struct led_classdev_mc dev;
+ u8 save;
+ u8 mode;
+ u8 speed;
+};
+
struct asus_wmi {
int dsts_id;
int spec;
@@ -234,6 +246,8 @@ struct asus_wmi {
bool dgpu_disable_available;
bool dgpu_disable;
+ struct keyboard_rgb_led keyboard_rgb_mode;
+
bool throttle_thermal_policy_available;
u8 throttle_thermal_policy_mode;
@@ -1028,12 +1042,40 @@ static enum led_brightness lightbar_led_get(struct led_classdev *led_cdev)
return result & ASUS_WMI_DSTS_LIGHTBAR_MASK;
}
+static int tuf_rgb_brightness_set(struct led_classdev *cdev,
+ enum led_brightness brightness)
+{
+ struct led_classdev_mc *mc_cdev = lcdev_to_mccdev(cdev);
+ struct keyboard_rgb_led *rgb = container_of(mc_cdev, struct keyboard_rgb_led, dev);
+ struct asus_wmi *asus = container_of(rgb, struct asus_wmi, keyboard_rgb_mode);
+ struct device *dev = &asus->platform_device->dev;
+ u8 r, g, b;
+ int err;
+ u32 ret;
+
+ led_mc_calc_color_components(mc_cdev, brightness);
+ r = mc_cdev->subled_info[0].brightness;
+ g = mc_cdev->subled_info[1].brightness;
+ b = mc_cdev->subled_info[2].brightness;
+
+ /* Writing out requires some defaults. This will overwrite boot mode */
+ err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, ASUS_WMI_DEVID_TUF_RGB_MODE,
+ rgb->save | (rgb->mode << 8) | (r << 16) | (g << 24),
+ (b) | (rgb->speed << 8),
+ &ret);
+ if (err)
+ dev_err(dev, "Unable to set TUF RGB data?\n");
+
+ return err;
+}
+
static void asus_wmi_led_exit(struct asus_wmi *asus)
{
led_classdev_unregister(&asus->kbd_led);
led_classdev_unregister(&asus->tpd_led);
led_classdev_unregister(&asus->wlan_led);
led_classdev_unregister(&asus->lightbar_led);
+ led_classdev_multicolor_unregister(&asus->keyboard_rgb_mode.dev);
if (asus->led_workqueue)
destroy_workqueue(asus->led_workqueue);
@@ -1105,6 +1147,44 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
&asus->lightbar_led);
}
+ if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE)) {
+ struct mc_subled *mc_led_info = asus->keyboard_rgb_mode.subled_info;
+ struct led_classdev_mc *mc_cdev = &asus->keyboard_rgb_mode.dev;
+ struct device *dev = &asus->platform_device->dev;
+ u8 led_brightness = 255;
+
+ /*
+ * 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.
+ */
+ mc_cdev->led_cdev.name = "asus::multicolour::kbd_backlight";
+ mc_cdev->led_cdev.flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
+ mc_cdev->led_cdev.brightness_set_blocking = tuf_rgb_brightness_set;
+ mc_cdev->led_cdev.brightness_get = NULL;
+
+ mc_cdev->subled_info = mc_led_info;
+ mc_led_info[0].intensity = 127;
+ mc_led_info[0].color_index = LED_COLOR_ID_RED;
+ mc_led_info[1].color_index = LED_COLOR_ID_GREEN;
+ mc_led_info[2].color_index = LED_COLOR_ID_BLUE;
+ mc_cdev->num_colors = 3;
+
+ /*
+ * It's not possible to get last set data from device so set defaults
+ * to make it safe for a user to change either RGB or modes.
+ */
+ asus->keyboard_rgb_mode.save = 1;
+ asus->keyboard_rgb_mode.mode = 0;
+ asus->keyboard_rgb_mode.speed = 0xeb;
+
+ mc_cdev->led_cdev.brightness = led_brightness;
+ mc_cdev->led_cdev.max_brightness = led_brightness;
+ led_mc_calc_color_components(mc_cdev, led_brightness);
+
+ rv = led_classdev_multicolor_register(dev, mc_cdev);
+ }
+
error:
if (rv)
asus_wmi_led_exit(asus);
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index a571b47ff362..d63c9945a17d 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -98,6 +98,9 @@
/* dgpu on/off */
#define ASUS_WMI_DEVID_DGPU 0x00090020
+/* TUF laptop RGB control */
+#define ASUS_WMI_DEVID_TUF_RGB_MODE 0x00100056
+
/* DSTS masks */
#define ASUS_WMI_DSTS_STATUS_BIT 0x00000001
#define ASUS_WMI_DSTS_UNKNOWN_BIT 0x00000002
--
2.37.1
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 <[email protected]>
---
drivers/platform/x86/asus-wmi.c | 74 ++++++++++++++++++++++
include/linux/platform_data/x86/asus-wmi.h | 2 +
2 files changed, 76 insertions(+)
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
--
2.37.1
My apologies to: Pavel, Barnab?s, Andy, I should have CC'ed you all in
from the start. I have a wee bit going on and forgot to do so.
Kind regards,
Luke.
On Mon, Aug 8 2022 at 15:04:14 +1200, Luke D. Jones <[email protected]>
wrote:
> This is a patch series to add RGB support for ASUS laptops.
> The laptops with this RGB tend to be the TUF series of gamer laptops.
>
> The first step is initial bringup of support using the multicolor LED
> API.
>
> These types of keyboards implement a slightly more complex interface
> than
> just RGB control however - they also have modes with can be static
> LED,
> blinking, rainbow, color cycles, and more. They also have some custom
> animations that can play depending on device state, such as suspended
> playing a fancy colour cycle, or playing a "wave" animation.
>
> Two of the patches add support for these features.
>
> The last patch adds documentation in:
> Documentation/ABI/testing/sysfs-platform-asus-wmi
>
> Some notes:
>
> - this patch series obsoletes the previous RGB patches by myself
>
> - it is not possible to add attribute groups to multicolor LED as
> they get overwritten by `led_multicolor_groups` in
> `led_classdev_multicolor_register_ext`.
>
> - the methods for RGB control do not provide a way to fetch exisiting
> state, so these methods are WO.
>
> - There is an existing `asus::kbd_backlight`, this provides a 4-step
> brightness to the RGB (off,low,med,high) individually to multicolor.
> I was unsure of the effect of adding a similar path so have used the
> `asus::multicolour::kbd_backlight` name to be clear about purpose.
> If the `asus::kbd_backlight` is off, then no RGB is shown at all.
>
> Changelog:
> - V2: patch 1: asus-wmi - RGB
> + shorten a few lines
> + move unused keyboard_rgb_mode_available to next patch
> + remove tuf_rgb_brightness_get() is it was only returning current
> led_classdev brightness, not reading it from device
> + remove unnecessary setting of brightness on multicolor init
> + set brightness_get to null for TUF RGB
> + actually use the member subled_info in keyboard_rgb_led struct and
> not the leftover dynamic allocation (now removed)
> - V2: patch 2: asus-wmi RGB mode control
> + tuf_rgb_brightness_set() was still using hardcoded save/mode/speed
> from testing. This is now using the pre-set default.
> + asus_wmi_led_init(), set speed value to a correct value
> + keyboard_rgb_mode_store() return count, not 0
> + correctly unregister the mulicolor led on module exit
> + use switch/case in keyboard_rgb_mode_store() for speed
> + remove a single line bracket block
> - V2: patch 3: asus-wmi RGB power control
> + Try to fix the indent warning from buildbot
> + Use correct date on added API docs
> + Add missing panel_od doc
> + correctly label attribute for keyboard_rgb_state
> - V2: patch 4: documentation
> + Add commit message
> - V2: patch 5: attributes using sysfs_emit:
> + Add commit message
> - V2: patch 6: new patch, dgpu_only mode
> + A previous patch I submitted
> "0006-asus-wmi-Add-support-for-dGPU-only-mode.patch"
> is now part of this series due to merge conflicts caused by the
> previous 5 patches
>
> Previous patches obsoleted:
> https://lkml.org/lkml/2022/8/3/885
> https://lkml.org/lkml/2022/8/3/886
> https://lkml.org/lkml/2022/8/3/44
> I may not have listed all.
>
> Luke D. Jones (6):
> asus-wmi: Implement TUF laptop keyboard RGB control
> asus-wmi: Implement TUF laptop keyboard LED modes
> asus-wmi: Implement TUF laptop keyboard power states
> asus-wmi: Document previously added attributes
> asus-wmi: Convert all attr-show to use sysfs_emit
> asus-wmi: Add support for dGPU-only mode
>
> .../ABI/testing/sysfs-platform-asus-wmi | 68 ++++
> drivers/platform/x86/asus-wmi.c | 343
> +++++++++++++++++-
> include/linux/platform_data/x86/asus-wmi.h | 8 +
> 3 files changed, 412 insertions(+), 7 deletions(-)
>
> --
> 2.37.1
>
It appears I have included the wrong dgpu-only patch. This one has
mistakes in it and early testing...
I will include the fixed one in the next version after review.
Kind regards,
Luke.
On Mon, Aug 8 2022 at 15:04:20 +1200, Luke D. Jones <[email protected]>
wrote:
> Adds support for a dGPU-only mode on some laptops where when enabled
> the boot GPU is the dGPU, and the iGPU is not visible.
>
> Signed-off-by: Luke D. Jones <[email protected]>
> ---
> .../ABI/testing/sysfs-platform-asus-wmi | 9 ++
> drivers/platform/x86/asus-wmi.c | 92
> +++++++++++++++++++
> include/linux/platform_data/x86/asus-wmi.h | 3 +
> 3 files changed, 104 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index 66b262476d92..93d111a65313 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -77,6 +77,15 @@ Description:
> * 0 - Disable,
> * 1 - Enable,
>
> +What: /sys/devices/platform/<platform>/dgpu_only
> +Date: Aug 2022
> +KernelVersion: 6.0
> +Contact: "Luke Jones" <[email protected]>
> +Description:
> + Set the dGPU to be the only GPU available:
> + * 0 - Disable,
> + * 1 - Enable,
> +
> What: /sys/devices/platform/<platform>/panel_od
> Date: Aug 2022
> KernelVersion: 5.17
> diff --git a/drivers/platform/x86/asus-wmi.c
> b/drivers/platform/x86/asus-wmi.c
> index b9e5d87e3e18..840299828512 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -246,6 +246,9 @@ struct asus_wmi {
> bool dgpu_disable_available;
> bool dgpu_disable;
>
> + bool dgpu_only_available;
> + bool dgpu_only;
> +
> bool keyboard_rgb_state_available;
> bool keyboard_rgb_mode_available;
> struct keyboard_rgb_led keyboard_rgb_mode;
> @@ -750,6 +753,87 @@ static ssize_t egpu_enable_store(struct device
> *dev,
>
> static DEVICE_ATTR_RW(egpu_enable);
>
> +/* dedicated GPU only
> *********************************************************/
> +static int dgpu_only_check_present(struct asus_wmi *asus)
> +{
> + u32 result;
> + int err;
> +
> + asus->dgpu_only_available = false;
> +
> + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_DGPU, &result);
> + if (err) {
> + if (err == -ENODEV)
> + return 0;
> + return err;
> + }
> +
> + if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
> + asus->dgpu_only_available = true;
> + asus->dgpu_only = result & ASUS_WMI_DSTS_STATUS_BIT;
> + }
> +
> + return 0;
> +}
> +
> +static int dgpu_only_write(struct asus_wmi *asus)
> +{
> + u32 retval;
> + u8 value;
> + int err;
> +
> + /* Don't rely on type conversion */
> + value = asus->dgpu_only ? 1 : 0;
> +
> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_DGPU, value, &retval);
> + if (err) {
> + pr_warn("Failed to set dGPU-only mode: %d\n", err);
> + return err;
> + }
> +
> + if (retval > 1) {
> + pr_warn("Failed to set dGPU-only mode (retval): 0x%x\n", retval);
> + return -EIO;
> + }
> +
> + sysfs_notify(&asus->platform_device->dev.kobj, NULL, "dgpu_only");
> +
> + return 0;
> +}
> +
> +static ssize_t dgpu_only_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct asus_wmi *asus = dev_get_drvdata(dev);
> + u8 mode = asus->dgpu_only;
> +
> + return sysfs_emit(buf, "%d\n", mode);
> +}
> +
> +static ssize_t dgpu_only_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + bool enable;
> + int result;
> +
> + struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> + result = kstrtobool(buf, &enable);
> + if (result)
> + return result;
> +
> + asus->dgpu_only = enable;
> +
> + result = dgpu_only_write(asus);
> + if (result)
> + return result;
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR_RW(dgpu_only);
> +
> /* TUF Laptop Keyboard RGB Modes
> **********************************************/
> static int keyboard_rgb_mode_check_present(struct asus_wmi *asus)
> {
> @@ -3473,6 +3557,7 @@ static struct attribute *platform_attributes[]
> = {
> &dev_attr_touchpad.attr,
> &dev_attr_egpu_enable.attr,
> &dev_attr_dgpu_disable.attr,
> + &dev_attr_dgpu_only.attr,
> &dev_attr_keyboard_rgb_mode.attr,
> &dev_attr_keyboard_rgb_mode_index.attr,
> &dev_attr_keyboard_rgb_state.attr,
> @@ -3507,6 +3592,8 @@ static umode_t asus_sysfs_is_visible(struct
> kobject *kobj,
> ok = asus->egpu_enable_available;
> else if (attr == &dev_attr_dgpu_disable.attr)
> ok = asus->dgpu_disable_available;
> + else if (attr == &dev_attr_dgpu_only.attr)
> + ok = asus->dgpu_only_available;
> else if (attr == &dev_attr_keyboard_rgb_mode.attr)
> ok = asus->keyboard_rgb_mode_available;
> else if (attr == &dev_attr_keyboard_rgb_mode_index.attr)
> @@ -3784,6 +3871,10 @@ static int asus_wmi_add(struct platform_device
> *pdev)
> if (err)
> goto fail_dgpu_disable;
>
> + err = dgpu_only_check_present(asus);
> + if (err)
> + goto fail_dgpu_only;
> +
> err = keyboard_rgb_mode_check_present(asus);
> if (err)
> goto fail_keyboard_rgb_mode;
> @@ -3906,6 +3997,7 @@ static int asus_wmi_add(struct platform_device
> *pdev)
> fail_fan_boost_mode:
> fail_egpu_enable:
> fail_dgpu_disable:
> +fail_dgpu_only:
> fail_keyboard_rgb_mode:
> fail_keyboard_rgb_state:
> fail_platform:
> diff --git a/include/linux/platform_data/x86/asus-wmi.h
> b/include/linux/platform_data/x86/asus-wmi.h
> index b5c966798ef8..76b0756a0666 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -98,6 +98,9 @@
> /* dgpu on/off */
> #define ASUS_WMI_DEVID_DGPU 0x00090020
>
> +/* Dedicated GPU only. When active the dGPU will be the only visible
> GPU */
> +#define ASUS_WMI_DEVID_DEDICATED 0x00090016
> +
> /* TUF laptop RGB control */
> #define ASUS_WMI_DEVID_TUF_RGB_MODE 0x00100056
> /* TUF laptop RGB state control */
> --
> 2.37.1
>
On Mon, Aug 8, 2022 at 5:10 AM Luke D. Jones <[email protected]> wrote:
>
> Adds support for a dGPU-only mode on some laptops where when enabled
> the boot GPU is the dGPU, and the iGPU is not visible.
the enabled boot (If I understood the intention correctly of the phrase)
...
> +What: /sys/devices/platform/<platform>/dgpu_only
> +Date: Aug 2022
Not sure, I would put September for sure
> +KernelVersion: 6.0
As in a parallel review it appears that this should be 6.1, merge
window for 6.0 is ongoing and this series definitely is out of scope
of it.
> +Contact: "Luke Jones" <[email protected]>
> +Description:
> + Set the dGPU to be the only GPU available:
> + * 0 - Disable,
> + * 1 - Enable,
Since you mentioned the patch is wrong, the rest may not be reviewed.
We will wait for a new version.
--
With Best Regards,
Andy Shevchenko
On Mon, Aug 8, 2022 at 5:07 AM Luke D. Jones <[email protected]> wrote:
>
> Adds support for changing the laptop keyboard LED modes. These
> are visible effects such as static, rainbow, pulsing, colour cycles.
>
> Two sysfs attributes are added to asus-nb-wmi:
> - keyboard_rgb_mode
> - keyboard_rgb_mode_index
...
> + if (sscanf(buf, "%hhd %hhd %hhd", &save, &mode, &speed) != 3)
> + return -EINVAL;
Same comment as per v1.
...
> + asus->keyboard_rgb_mode.mode = mode < 12 && mode != 9 ? mode : 0x0a;
Same comment as per v1.
...
> + switch (speed) {
> + case 0:
> + asus->keyboard_rgb_mode.speed = 0xe1;
> + break;
> + case 1:
> + asus->keyboard_rgb_mode.speed = 0xeb;
> + break;
> + case 2:
> + asus->keyboard_rgb_mode.speed = 0xf5;
> + break;
> + default:
> + asus->keyboard_rgb_mode.speed = 0xeb;
break;
> + }
...
> +
A blank line is not needed here.
> +static DEVICE_ATTR_WO(keyboard_rgb_mode);
--
With Best Regards,
Andy Shevchenko
On Mon, Aug 8, 2022 at 5:08 AM Luke D. Jones <[email protected]> wrote:
>
> This changes all *_show attributes in asus-wmi.c to use sysfs_emit
We refer to functions as func().
> instead of the older method of writing to the output buffer manually.
...
> - return sprintf(buf, "%d\n", value < 0 ? -1 : value*100);
> + return sysfs_emit(buf, "%d\n", value < 0 ? -1 : value*100);
While at it, put spaces around *: 'value * 100'.
--
With Best Regards,
Andy Shevchenko
On Mon, Aug 8, 2022 at 5:09 AM Luke D. Jones <[email protected]> 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)
--
With Best Regards,
Andy Shevchenko
On Mon, Aug 8, 2022 at 5:09 AM Luke D. Jones <[email protected]> wrote:
>
> Documents some previously added attributes:
> - dgpu_disable
> - egpu_enable
> - panel_od
> - keyboard_rgb_mode
> - keyboard_rgb_state
...
> +What: /sys/devices/platform/<platform>/dgpu_disable
> +Date: Aug 2022
> +KernelVersion: 5.17
> +Contact: "Luke Jones" <[email protected]>
> +Description:
> + Disable discrete GPU:
> + * 0 - Enable dGPU,
> + * 1 - Disable dGPU,
> +
> +What: /sys/devices/platform/<platform>/egpu_enable
> +Date: Aug 2022
> +KernelVersion: 5.17
> +Contact: "Luke Jones" <[email protected]>
> +Description:
> + Enable the external GPU paired with ROG X-Flow laptops.
> + Toggling this setting will also trigger ACPI to disable the dGPU:
> + * 0 - Disable,
> + * 1 - Enable,
> +
> +What: /sys/devices/platform/<platform>/panel_od
> +Date: Aug 2022
> +KernelVersion: 5.17
> +Contact: "Luke Jones" <[email protected]>
> +Description:
> + Enable an LCD response-time boost to reduce or remove ghosting:
> + * 0 - Disable,
> + * 1 - Enable,
These should be in separate patch(es) with the corresponding Fixes
tags. (The latter may not be so important, though. I leave it to Hans
to decide)
...
> +What: /sys/devices/platform/<platform>/keyboard_rgb_mode
> +Date: Aug 2022
> +KernelVersion: 6.0
These should go separately. Dunno if it should be a separate
documentation change for the both, or should it be split and
reattached to the respective patches from the series. Up to Hans.
> +Contact: "Luke Jones" <[email protected]>
> +Description:
> + Set some RGB keyboard modes and features (write-only).
> +
> + The accepted input is "save mode speed", where "n n n" options
> + are:
> + * save - 0 or 1, if 0 then settings are not retained on boot
> + * mode - 0 to 12, each is an RGB such as static, rainbow, pulse.
> + Not all keyboards accept every mode.
> + * speed - 0, 1, 2, equal to low, medium, high.
> + Only applies to certain modes.
> +
> +What: /sys/devices/platform/<platform>/keyboard_rgb_state
> +Date: Aug 2022
> +KernelVersion: 6.0
> +Contact: "Luke Jones" <[email protected]>
> +Description:
> + Set some RGB keyboard power states (write-only).
> +
> + The accepted input is "boot awake sleep keyboard", where "n n n n n"
> + options are:
> + * save - 0 or 1, if 0 then settings are not retained on boot
> + * boot - 0 or 1, controls if a boot animation is shown
> + * awake - 0 or 1, controls if the keyboard LED are on during awake
> + * sleep - 0 or 1, controls if a suspended animation is shown.
> + This is only active if the AC is connected.
> + * keyboard - 0 or 1, unknown what effect this really has
--
With Best Regards,
Andy Shevchenko
Hi Andy,
>
>> + if (sscanf(buf, "%hhd %hhd %hhd", &save, &mode, &speed) !=
>> 3)
>> + return -EINVAL;
>
> Same comment as per v1.
>
You wrote:
> Usually we have three separate nodes for that, but they are kinda
> hidden in one driver, so I don't care much.
I think that is the wrong direction to take. Doing so would mean that
every write to one of these values has to write-out to device. I don't
know how long writes on an i2c device take, but on the USB connected
versions they are 1ms, which means that to individually set colour,
save, mode, speed (also direction and sometimes a 2nd colour on USB)
adds up to 4-6ms - and I don't know what sort of impact that has in the
kernel itself, but I do know that users expect there to be fancy
effects available on par with Windows (like audio equalizer visuals on
the RGB, something that is in progress in asusctl).
Using multicolor LED class already breaks away from having a single
packet write, but the gain in API scope was worth the compromise.
Hopefully we can keep the single set of parameters here?
Pavel suggested using triggers, I've yet to look at that, but will do
so after finalising this.
I suppose one alternative would be to store speed and mode as
attributes, but not write out until the "save" node is written to? So
this raises the question of: we can't read from device, and speed+mode
must be saved in module for use with "save" now, should I then allow
showing these values in a _show? On fresh boot they will be incorrect..
I'm going to go ahead and split those parameters in to individual nodes
now anyway - it may help with later work using triggers.
> ...
>
>> + asus->keyboard_rgb_mode.mode = mode < 12 && mode != 9 ?
>> mode : 0x0a;
>
> Same comment as per v1.
>
I missed it sorry. Done now.
> ...
>
>> + switch (speed) {
>> + case 0:
>> + asus->keyboard_rgb_mode.speed = 0xe1;
>> + break;
>> + case 1:
>> + asus->keyboard_rgb_mode.speed = 0xeb;
>> + break;
>> + case 2:
>> + asus->keyboard_rgb_mode.speed = 0xf5;
>> + break;
>> + default:
>> + asus->keyboard_rgb_mode.speed = 0xeb;
>
> break;
Done
>
>> + }
>
> ...
>
>> +
>
> A blank line is not needed here.
Okay thanks, I'll go through previous patches and check this.
Kind regards,
Luke.
>
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 Mon, Aug 8, 2022 at 11:43 PM Luke Jones <[email protected]> wrote:
...
> >> + if (sscanf(buf, "%hhd %hhd %hhd", &save, &mode, &speed) !=
> >> 3)
> >> + return -EINVAL;
> >
> > Same comment as per v1.
>
> You wrote:
>
> > Usually we have three separate nodes for that, but they are kinda
> > hidden in one driver, so I don't care much.
>
> I think that is the wrong direction to take. Doing so would mean that
> every write to one of these values has to write-out to device. I don't
> know how long writes on an i2c device take, but on the USB connected
> versions they are 1ms, which means that to individually set colour,
> save, mode, speed (also direction and sometimes a 2nd colour on USB)
> adds up to 4-6ms - and I don't know what sort of impact that has in the
> kernel itself, but I do know that users expect there to be fancy
> effects available on par with Windows (like audio equalizer visuals on
> the RGB, something that is in progress in asusctl).
This is a good justification, but easy to workaround by using another
node like "submit" or unifying it with one of the existing nodes
implicitly (like setting savee will submit all changes at once).
> Using multicolor LED class already breaks away from having a single
> packet write, but the gain in API scope was worth the compromise.
> Hopefully we can keep the single set of parameters here?
>
> Pavel suggested using triggers, I've yet to look at that, but will do
> so after finalising this.
The thing is you can't "finalize" this and go to another type of ABI,
because we come with two ABIs - we may not drop the old one once it's
established (yes, there are exceptions, but it's rare). So, knowing
that we would drop an ABI we mustn't introduce it in the first place.
> I suppose one alternative would be to store speed and mode as
> attributes, but not write out until the "save" node is written to?
Yes!
> So
> this raises the question of: we can't read from device, and speed+mode
> must be saved in module for use with "save" now, should I then allow
> showing these values in a _show? On fresh boot they will be incorrect..
Yep, they should be reset either by hardware, or provided somehow
(device tree?) from the platform knowing what firmware sets and what
kernel may use if you don't want to change the settings.
> I'm going to go ahead and split those parameters in to individual nodes
> now anyway - it may help with later work using triggers.
Okay!
--
With Best Regards,
Andy Shevchenko
On Tue, Aug 9, 2022 at 1:27 AM Luke Jones <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
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 <[email protected]> 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.