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.
One patch adds documentation in:
Documentation/ABI/testing/sysfs-platform-asus-wmi
for some features that were added previously.
The final patch adds support for a particular MUX switch found only
on a few ROG laptops. This patch is added to this series due to some
conflicts in merge caused by the RGB patch series.
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:
- V3: patch 2: asus-wmi RGB mode control
+ Split save, speed, mode in to separate nodes
+ Remove the _index node as it is not required with the above nodes
+ Cleanup of a one-line ternary
+ rename asus->keyboard_rgb_mode to keyboard_rgb_led to be clearer about purpose
+ Attach documentation to this patch
- V3: patch 3:
+ Use BIT() in place of previous hex for flags
+ Comment on purpose of 0xbd in state write
+ Attach documentation to this patch
- V3: patch 6: asus-wmi: hardware GPU MUX:
+ Include the correct patch for this feature
- 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: Support the hardware GPU MUX on some laptops
.../ABI/testing/sysfs-platform-asus-wmi | 83 ++++
drivers/platform/x86/asus-wmi.c | 368 +++++++++++++++++-
include/linux/platform_data/x86/asus-wmi.h | 8 +
3 files changed, 452 insertions(+), 7 deletions(-)
--
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]>
---
.../ABI/testing/sysfs-platform-asus-wmi | 18 ++++-
drivers/platform/x86/asus-wmi.c | 72 +++++++++++++++++++
include/linux/platform_data/x86/asus-wmi.h | 2 +
3 files changed, 91 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index a9128fa5cc65..3e3f2dcf9bfa 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -86,4 +86,20 @@ Description:
until the keyboard_rgb_save attribute is set (write-only):
* 0 - slow
* 1 - medium
- * 2 - fast
\ No newline at end of file
+ * 2 - fast
+
+What: /sys/devices/platform/<platform>/keyboard_rgb_state
+Date: Aug 2022
+KernelVersion: 6.1
+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
\ No newline at end of file
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index fa0cc2895a66..719223804c0e 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_led;
@@ -845,6 +846,66 @@ static ssize_t keyboard_rgb_speed_store(struct device *device,
}
static DEVICE_ATTR_WO(keyboard_rgb_speed);
+/* 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 */
@@ -3438,6 +3499,8 @@ static struct attribute *platform_attributes[] = {
&dev_attr_keyboard_rgb_save.attr,
&dev_attr_keyboard_rgb_mode.attr,
&dev_attr_keyboard_rgb_speed.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,
@@ -3474,6 +3537,10 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
ok = asus->keyboard_rgb_mode_available;
else if (attr == &dev_attr_keyboard_rgb_speed.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)
@@ -3747,6 +3814,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;
@@ -3862,6 +3933,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
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 719223804c0e..78f1f3af1b12 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -942,7 +942,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);
@@ -2032,7 +2032,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,
@@ -2092,7 +2092,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,
@@ -2110,7 +2110,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,
@@ -2178,7 +2178,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,
@@ -2371,7 +2371,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,
@@ -2924,7 +2924,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
Support the hardware GPU MUX switch available on some models. This
switch can toggle the MUX between:
- 0, Dedicated mode
- 1, Optimus mode
Optimus mode is the regular iGPU + dGPU available, while dedicated
mode switches the system to have only the dGPU available.
Signed-off-by: Luke D. Jones <[email protected]>
---
.../ABI/testing/sysfs-platform-asus-wmi | 9 ++
drivers/platform/x86/asus-wmi.c | 91 +++++++++++++++++++
include/linux/platform_data/x86/asus-wmi.h | 3 +
3 files changed, 103 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 541dbfbbbb26..d483bc3cb2e6 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -67,6 +67,15 @@ Description:
* 0 - Disable,
* 1 - Enable,
+What: /sys/devices/platform/<platform>/gpu_mux_mode
+Date: Aug 2022
+KernelVersion: 6.0
+Contact: "Luke Jones" <[email protected]>
+Description:
+ Switch the GPU used by the hardware MUX:
+ * 0 - Dedicated GPU,
+ * 1 - Optimus mode,
+
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 78f1f3af1b12..c5fa21370481 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 gpu_mux_mode_available;
+ bool gpu_mux_mode;
+
bool keyboard_rgb_state_available;
bool keyboard_rgb_mode_available;
struct keyboard_rgb_led keyboard_rgb_led;
@@ -750,6 +753,86 @@ static ssize_t egpu_enable_store(struct device *dev,
static DEVICE_ATTR_RW(egpu_enable);
+/* gpu mux switch *************************************************************/
+static int gpu_mux_mode_check_present(struct asus_wmi *asus)
+{
+ u32 result;
+ int err;
+
+ asus->gpu_mux_mode_available = false;
+
+ err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_GPU_MUX, &result);
+ if (err) {
+ if (err == -ENODEV)
+ return 0;
+ return err;
+ }
+
+ if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
+ asus->gpu_mux_mode_available = true;
+ asus->gpu_mux_mode = result & ASUS_WMI_DSTS_STATUS_BIT;
+ }
+
+ return 0;
+}
+
+static int gpu_mux_mode_write(struct asus_wmi *asus)
+{
+ u32 retval;
+ u8 value;
+ int err;
+
+ /* Don't rely on type conversion */
+ value = asus->gpu_mux_mode ? 1 : 0;
+
+ err = asus_wmi_set_devstate(ASUS_WMI_DEVID_GPU_MUX, 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, "gpu_mux_mode");
+
+ return 0;
+}
+
+static ssize_t gpu_mux_mode_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct asus_wmi *asus = dev_get_drvdata(dev);
+ u8 mode = asus->gpu_mux_mode;
+
+ return sysfs_emit(buf, "%d\n", mode);
+}
+
+static ssize_t gpu_mux_mode_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ bool optimus;
+ int result;
+
+ struct asus_wmi *asus = dev_get_drvdata(dev);
+
+ result = kstrtobool(buf, &optimus);
+ if (result)
+ return result;
+
+ asus->gpu_mux_mode = optimus;
+
+ result = gpu_mux_mode_write(asus);
+ if (result)
+ return result;
+
+ return count;
+}
+static DEVICE_ATTR_RW(gpu_mux_mode);
+
/* TUF Laptop Keyboard RGB Modes **********************************************/
static int keyboard_rgb_check_present(struct asus_wmi *asus)
{
@@ -3496,6 +3579,7 @@ static struct attribute *platform_attributes[] = {
&dev_attr_touchpad.attr,
&dev_attr_egpu_enable.attr,
&dev_attr_dgpu_disable.attr,
+ &dev_attr_gpu_mux_mode.attr,
&dev_attr_keyboard_rgb_save.attr,
&dev_attr_keyboard_rgb_mode.attr,
&dev_attr_keyboard_rgb_speed.attr,
@@ -3531,6 +3615,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_gpu_mux_mode.attr)
+ ok = asus->gpu_mux_mode_available;
else if (attr == &dev_attr_keyboard_rgb_save.attr)
ok = asus->keyboard_rgb_mode_available;
else if (attr == &dev_attr_keyboard_rgb_mode.attr)
@@ -3810,6 +3896,10 @@ static int asus_wmi_add(struct platform_device *pdev)
if (err)
goto fail_dgpu_disable;
+ err = gpu_mux_mode_check_present(asus);
+ if (err)
+ goto fail_gpu_mux_mode;
+
err = keyboard_rgb_check_present(asus);
if (err)
goto fail_keyboard_rgb_mode;
@@ -3932,6 +4022,7 @@ static int asus_wmi_add(struct platform_device *pdev)
fail_fan_boost_mode:
fail_egpu_enable:
fail_dgpu_disable:
+fail_gpu_mux_mode:
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..3faeb98f6ea9 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
+/* gpu mux switch, 0 = dGPU, 1 = Optimus */
+#define ASUS_WMI_DEVID_GPU_MUX 0x00090016
+
/* TUF laptop RGB control */
#define ASUS_WMI_DEVID_TUF_RGB_MODE 0x00100056
/* TUF laptop RGB state control */
--
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
Documents some previously added attributes:
- dgpu_disable
- egpu_enable
- panel_od
Signed-off-by: Luke D. Jones <[email protected]>
---
.../ABI/testing/sysfs-platform-asus-wmi | 30 ++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 3e3f2dcf9bfa..541dbfbbbb26 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -48,6 +48,34 @@ Description:
* 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>/throttle_thermal_policy
Date: Dec 2019
KernelVersion: 5.6
@@ -102,4 +130,4 @@ Description:
* 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
\ No newline at end of file
+ * keyboard - 0 or 1, unknown what effect this really has
--
2.37.1
Adds support for changing the laptop keyboard LED modes. These
are visible effects such as static, rainbow, pulsing, colour cycles.
These sysfs attributes are added to asus-nb-wmi:
- keyboard_rgb_save
- keyboard_rgb_mode
- keyboard_rgb_speed
Signed-off-by: Luke D. Jones <[email protected]>
---
.../ABI/testing/sysfs-platform-asus-wmi | 30 +++++
drivers/platform/x86/asus-wmi.c | 127 ++++++++++++++++--
2 files changed, 149 insertions(+), 8 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 04885738cf15..a9128fa5cc65 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -57,3 +57,33 @@ Description:
* 0 - default,
* 1 - overboost,
* 2 - silent
+
+What: /sys/devices/platform/<platform>/keyboard_rgb_save
+Date: Aug 2022
+KernelVersion: 6.1
+Contact: "Luke Jones" <[email protected]>
+Description:
+ Set or save the RGB mode details (write-only):
+ * 0 - set, the settings will be lost on boot
+ * 1 - save, the settings will be retained on boot
+
+What: /sys/devices/platform/<platform>/keyboard_rgb_mode
+Date: Aug 2022
+KernelVersion: 6.1
+Contact: "Luke Jones" <[email protected]>
+Description:
+ Set the mode of the RGB keyboard, the mode will not apply until the
+ keyboard_rgb_save attribute is set (write-only):
+ * 0 to 12, each is an RGB such as static, rainbow, pulse.
+ Not all keyboards accept every mode.
+
+What: /sys/devices/platform/<platform>/keyboard_rgb_speed
+Date: Aug 2022
+KernelVersion: 6.1
+Contact: "Luke Jones" <[email protected]>
+Description:
+ Set the speed of the selected RGB effect, the speed will not apply
+ until the keyboard_rgb_save attribute is set (write-only):
+ * 0 - slow
+ * 1 - medium
+ * 2 - fast
\ No newline at end of file
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 233e73f4313d..fa0cc2895a66 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -246,7 +246,8 @@ struct asus_wmi {
bool dgpu_disable_available;
bool dgpu_disable;
- struct keyboard_rgb_led keyboard_rgb_mode;
+ bool keyboard_rgb_mode_available;
+ struct keyboard_rgb_led keyboard_rgb_led;
bool throttle_thermal_policy_available;
u8 throttle_thermal_policy_mode;
@@ -748,6 +749,102 @@ static ssize_t egpu_enable_store(struct device *dev,
static DEVICE_ATTR_RW(egpu_enable);
+/* TUF Laptop Keyboard RGB Modes **********************************************/
+static int keyboard_rgb_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_save_store(struct device *device,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ u8 save;
+ int err;
+
+ struct asus_wmi *asus = dev_get_drvdata(device);
+ struct led_classdev *cdev = &asus->keyboard_rgb_led.dev.led_cdev;
+
+ if (sscanf(buf, "%hhd", &save) != 1)
+ return -EINVAL;
+
+ asus->keyboard_rgb_led.save = !!save;
+
+ err = tuf_rgb_brightness_set(cdev, cdev->brightness);
+ if (err)
+ return err;
+
+ return count;
+}
+static DEVICE_ATTR_WO(keyboard_rgb_save);
+
+static ssize_t keyboard_rgb_mode_store(struct device *device,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ u8 mode;
+
+ struct asus_wmi *asus = dev_get_drvdata(device);
+
+ if (sscanf(buf, "%hhd", &mode) != 1)
+ return -EINVAL;
+
+ /* These are the known usable modes across all TUF/ROG */
+ if (mode >= 12 || mode == 10)
+ asus->keyboard_rgb_led.mode = 10;
+ else
+ asus->keyboard_rgb_led.mode = mode;
+
+ return count;
+}
+static DEVICE_ATTR_WO(keyboard_rgb_mode);
+
+
+static ssize_t keyboard_rgb_speed_store(struct device *device,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ u8 speed;
+
+ struct asus_wmi *asus = dev_get_drvdata(device);
+
+ if (sscanf(buf, "%hhd", &speed) != 1)
+ return -EINVAL;
+
+ switch (speed) {
+ case 0:
+ asus->keyboard_rgb_led.speed = 0xe1;
+ break;
+ case 1:
+ asus->keyboard_rgb_led.speed = 0xeb;
+ break;
+ case 2:
+ asus->keyboard_rgb_led.speed = 0xf5;
+ break;
+ default:
+ asus->keyboard_rgb_led.speed = 0xeb;
+ break;
+ }
+
+ return count;
+}
+static DEVICE_ATTR_WO(keyboard_rgb_speed);
+
/* Battery ********************************************************************/
/* The battery maximum charging percentage */
@@ -1047,7 +1144,7 @@ static int tuf_rgb_brightness_set(struct led_classdev *cdev,
{
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 asus_wmi *asus = container_of(rgb, struct asus_wmi, keyboard_rgb_led);
struct device *dev = &asus->platform_device->dev;
u8 r, g, b;
int err;
@@ -1075,7 +1172,7 @@ static void asus_wmi_led_exit(struct asus_wmi *asus)
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);
+ led_classdev_multicolor_unregister(&asus->keyboard_rgb_led.dev);
if (asus->led_workqueue)
destroy_workqueue(asus->led_workqueue);
@@ -1148,8 +1245,8 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
}
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 mc_subled *mc_led_info = asus->keyboard_rgb_led.subled_info;
+ struct led_classdev_mc *mc_cdev = &asus->keyboard_rgb_led.dev;
struct device *dev = &asus->platform_device->dev;
u8 led_brightness = 255;
@@ -1174,9 +1271,9 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
* 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;
+ asus->keyboard_rgb_led.save = 1;
+ asus->keyboard_rgb_led.mode = 0;
+ asus->keyboard_rgb_led.speed = 0xeb;
mc_cdev->led_cdev.brightness = led_brightness;
mc_cdev->led_cdev.max_brightness = led_brightness;
@@ -3338,6 +3435,9 @@ static struct attribute *platform_attributes[] = {
&dev_attr_touchpad.attr,
&dev_attr_egpu_enable.attr,
&dev_attr_dgpu_disable.attr,
+ &dev_attr_keyboard_rgb_save.attr,
+ &dev_attr_keyboard_rgb_mode.attr,
+ &dev_attr_keyboard_rgb_speed.attr,
&dev_attr_lid_resume.attr,
&dev_attr_als_enable.attr,
&dev_attr_fan_boost_mode.attr,
@@ -3368,6 +3468,12 @@ 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_save.attr)
+ ok = asus->keyboard_rgb_mode_available;
+ else if (attr == &dev_attr_keyboard_rgb_mode.attr)
+ ok = asus->keyboard_rgb_mode_available;
+ else if (attr == &dev_attr_keyboard_rgb_speed.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 +3743,10 @@ static int asus_wmi_add(struct platform_device *pdev)
if (err)
goto fail_dgpu_disable;
+ err = keyboard_rgb_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 +3861,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
*sighs*
I swear I ran checkpatch. I just doublechecked and ran again, getting
this:
----------------------------------------------------------------
./v3-0002-asus-wmi-Implement-TUF-laptop-keyboard-LED-modes.patch
----------------------------------------------------------------
WARNING: Prefer kstrto<type> to single variable sscanf
#108: FILE: drivers/platform/x86/asus-wmi.c:783:
+ if (sscanf(buf, "%hhd", &save) != 1)
+ return -EINVAL;
WARNING: Prefer kstrto<type> to single variable sscanf
#129: FILE: drivers/platform/x86/asus-wmi.c:804:
+ if (sscanf(buf, "%hhd", &mode) != 1)
+ return -EINVAL;
WARNING: suspect code indent for conditional statements (8, 10)
#133: FILE: drivers/platform/x86/asus-wmi.c:808:
+ if (mode >= 12 || mode == 10)
+ asus->keyboard_rgb_led.mode = 10;
WARNING: suspect code indent for conditional statements (8, 10)
#135: FILE: drivers/platform/x86/asus-wmi.c:810:
+ else
+ asus->keyboard_rgb_led.mode = mode;
WARNING: Prefer kstrto<type> to single variable sscanf
#151: FILE: drivers/platform/x86/asus-wmi.c:826:
+ if (sscanf(buf, "%hhd", &speed) != 1)
+ return -EINVAL;
I will fix, and wait for review of V3 before submitting hopefully the
final version.
Sorry everyone.
On Tue, Aug 9 2022 at 14:50:50 +1200, 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.
>
> These sysfs attributes are added to asus-nb-wmi:
> - keyboard_rgb_save
> - keyboard_rgb_mode
> - keyboard_rgb_speed
>
> Signed-off-by: Luke D. Jones <[email protected]>
> ---
> .../ABI/testing/sysfs-platform-asus-wmi | 30 +++++
> drivers/platform/x86/asus-wmi.c | 127
> ++++++++++++++++--
> 2 files changed, 149 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index 04885738cf15..a9128fa5cc65 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -57,3 +57,33 @@ Description:
> * 0 - default,
> * 1 - overboost,
> * 2 - silent
> +
> +What: /sys/devices/platform/<platform>/keyboard_rgb_save
> +Date: Aug 2022
> +KernelVersion: 6.1
> +Contact: "Luke Jones" <[email protected]>
> +Description:
> + Set or save the RGB mode details (write-only):
> + * 0 - set, the settings will be lost on boot
> + * 1 - save, the settings will be retained on boot
> +
> +What: /sys/devices/platform/<platform>/keyboard_rgb_mode
> +Date: Aug 2022
> +KernelVersion: 6.1
> +Contact: "Luke Jones" <[email protected]>
> +Description:
> + Set the mode of the RGB keyboard, the mode will not apply until the
> + keyboard_rgb_save attribute is set (write-only):
> + * 0 to 12, each is an RGB such as static, rainbow, pulse.
> + Not all keyboards accept every mode.
> +
> +What: /sys/devices/platform/<platform>/keyboard_rgb_speed
> +Date: Aug 2022
> +KernelVersion: 6.1
> +Contact: "Luke Jones" <[email protected]>
> +Description:
> + Set the speed of the selected RGB effect, the speed will not apply
> + until the keyboard_rgb_save attribute is set (write-only):
> + * 0 - slow
> + * 1 - medium
> + * 2 - fast
> \ No newline at end of file
> diff --git a/drivers/platform/x86/asus-wmi.c
> b/drivers/platform/x86/asus-wmi.c
> index 233e73f4313d..fa0cc2895a66 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -246,7 +246,8 @@ struct asus_wmi {
> bool dgpu_disable_available;
> bool dgpu_disable;
>
> - struct keyboard_rgb_led keyboard_rgb_mode;
> + bool keyboard_rgb_mode_available;
> + struct keyboard_rgb_led keyboard_rgb_led;
>
> bool throttle_thermal_policy_available;
> u8 throttle_thermal_policy_mode;
> @@ -748,6 +749,102 @@ static ssize_t egpu_enable_store(struct device
> *dev,
>
> static DEVICE_ATTR_RW(egpu_enable);
>
> +/* TUF Laptop Keyboard RGB Modes
> **********************************************/
> +static int keyboard_rgb_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_save_store(struct device *device,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + u8 save;
> + int err;
> +
> + struct asus_wmi *asus = dev_get_drvdata(device);
> + struct led_classdev *cdev = &asus->keyboard_rgb_led.dev.led_cdev;
> +
> + if (sscanf(buf, "%hhd", &save) != 1)
> + return -EINVAL;
> +
> + asus->keyboard_rgb_led.save = !!save;
> +
> + err = tuf_rgb_brightness_set(cdev, cdev->brightness);
> + if (err)
> + return err;
> +
> + return count;
> +}
> +static DEVICE_ATTR_WO(keyboard_rgb_save);
> +
> +static ssize_t keyboard_rgb_mode_store(struct device *device,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + u8 mode;
> +
> + struct asus_wmi *asus = dev_get_drvdata(device);
> +
> + if (sscanf(buf, "%hhd", &mode) != 1)
> + return -EINVAL;
> +
> + /* These are the known usable modes across all TUF/ROG */
> + if (mode >= 12 || mode == 10)
> + asus->keyboard_rgb_led.mode = 10;
> + else
> + asus->keyboard_rgb_led.mode = mode;
> +
> + return count;
> +}
> +static DEVICE_ATTR_WO(keyboard_rgb_mode);
> +
> +
> +static ssize_t keyboard_rgb_speed_store(struct device *device,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + u8 speed;
> +
> + struct asus_wmi *asus = dev_get_drvdata(device);
> +
> + if (sscanf(buf, "%hhd", &speed) != 1)
> + return -EINVAL;
> +
> + switch (speed) {
> + case 0:
> + asus->keyboard_rgb_led.speed = 0xe1;
> + break;
> + case 1:
> + asus->keyboard_rgb_led.speed = 0xeb;
> + break;
> + case 2:
> + asus->keyboard_rgb_led.speed = 0xf5;
> + break;
> + default:
> + asus->keyboard_rgb_led.speed = 0xeb;
> + break;
> + }
> +
> + return count;
> +}
> +static DEVICE_ATTR_WO(keyboard_rgb_speed);
> +
> /* Battery
> ********************************************************************/
>
> /* The battery maximum charging percentage */
> @@ -1047,7 +1144,7 @@ static int tuf_rgb_brightness_set(struct
> led_classdev *cdev,
> {
> 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 asus_wmi *asus = container_of(rgb, struct asus_wmi,
> keyboard_rgb_led);
> struct device *dev = &asus->platform_device->dev;
> u8 r, g, b;
> int err;
> @@ -1075,7 +1172,7 @@ static void asus_wmi_led_exit(struct asus_wmi
> *asus)
> 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);
> + led_classdev_multicolor_unregister(&asus->keyboard_rgb_led.dev);
>
> if (asus->led_workqueue)
> destroy_workqueue(asus->led_workqueue);
> @@ -1148,8 +1245,8 @@ static int asus_wmi_led_init(struct asus_wmi
> *asus)
> }
>
> 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 mc_subled *mc_led_info = asus->keyboard_rgb_led.subled_info;
> + struct led_classdev_mc *mc_cdev = &asus->keyboard_rgb_led.dev;
> struct device *dev = &asus->platform_device->dev;
> u8 led_brightness = 255;
>
> @@ -1174,9 +1271,9 @@ static int asus_wmi_led_init(struct asus_wmi
> *asus)
> * 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;
> + asus->keyboard_rgb_led.save = 1;
> + asus->keyboard_rgb_led.mode = 0;
> + asus->keyboard_rgb_led.speed = 0xeb;
>
> mc_cdev->led_cdev.brightness = led_brightness;
> mc_cdev->led_cdev.max_brightness = led_brightness;
> @@ -3338,6 +3435,9 @@ static struct attribute *platform_attributes[]
> = {
> &dev_attr_touchpad.attr,
> &dev_attr_egpu_enable.attr,
> &dev_attr_dgpu_disable.attr,
> + &dev_attr_keyboard_rgb_save.attr,
> + &dev_attr_keyboard_rgb_mode.attr,
> + &dev_attr_keyboard_rgb_speed.attr,
> &dev_attr_lid_resume.attr,
> &dev_attr_als_enable.attr,
> &dev_attr_fan_boost_mode.attr,
> @@ -3368,6 +3468,12 @@ 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_save.attr)
> + ok = asus->keyboard_rgb_mode_available;
> + else if (attr == &dev_attr_keyboard_rgb_mode.attr)
> + ok = asus->keyboard_rgb_mode_available;
> + else if (attr == &dev_attr_keyboard_rgb_speed.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 +3743,10 @@ static int asus_wmi_add(struct platform_device
> *pdev)
> if (err)
> goto fail_dgpu_disable;
>
> + err = keyboard_rgb_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 +3861,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
>
Hi all,
This patch still needs some work. I've been analysing the various dumps
I've collected over the past 2 years.
Some laptops return a `0xFFFFFFFE` in response to query of this method,
and do not have a corresponding set method. To work around it I was
using `if (result == 0xFFFFFFFE) return 0;`, but, I'm unsure if
`0xFFFFFFFE` is actually a valid response in the first place. Is it?
Additionally to this, I should have been reading the devstate in the
related _show(), not returning the stored value. And this should be
done for the egpu_enable and dgpu_disable attributes also.
Lastly, some laptops have a valid return for the getter, but no setter
method.. I'm not sure what to do about this.
Are there any issues with me adding more patches to this series? In
particular I think I need to add patches for the above mentioned
things, and I should add the "asus-wmi: Modify behaviour of Fn+F5 fan
key" patches too, I'm beginning to get merge conflicts in testing, and
all the work I'm doing is becoming unwieldy.
Kind regards,
Luke.
On Tue, Aug 9 2022 at 14:50:54 +1200, Luke D. Jones <[email protected]>
wrote:
> Support the hardware GPU MUX switch available on some models. This
> switch can toggle the MUX between:
>
> - 0, Dedicated mode
> - 1, Optimus mode
>
> Optimus mode is the regular iGPU + dGPU available, while dedicated
> mode switches the system to have only the dGPU available.
>
> Signed-off-by: Luke D. Jones <[email protected]>
> ---
> .../ABI/testing/sysfs-platform-asus-wmi | 9 ++
> drivers/platform/x86/asus-wmi.c | 91
> +++++++++++++++++++
> include/linux/platform_data/x86/asus-wmi.h | 3 +
> 3 files changed, 103 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index 541dbfbbbb26..d483bc3cb2e6 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -67,6 +67,15 @@ Description:
> * 0 - Disable,
> * 1 - Enable,
>
> +What: /sys/devices/platform/<platform>/gpu_mux_mode
> +Date: Aug 2022
> +KernelVersion: 6.0
> +Contact: "Luke Jones" <[email protected]>
> +Description:
> + Switch the GPU used by the hardware MUX:
> + * 0 - Dedicated GPU,
> + * 1 - Optimus mode,
> +
> 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 78f1f3af1b12..c5fa21370481 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 gpu_mux_mode_available;
> + bool gpu_mux_mode;
> +
> bool keyboard_rgb_state_available;
> bool keyboard_rgb_mode_available;
> struct keyboard_rgb_led keyboard_rgb_led;
> @@ -750,6 +753,86 @@ static ssize_t egpu_enable_store(struct device
> *dev,
>
> static DEVICE_ATTR_RW(egpu_enable);
>
> +/* gpu mux switch
> *************************************************************/
> +static int gpu_mux_mode_check_present(struct asus_wmi *asus)
> +{
> + u32 result;
> + int err;
> +
> + asus->gpu_mux_mode_available = false;
> +
> + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_GPU_MUX, &result);
> + if (err) {
> + if (err == -ENODEV)
> + return 0;
> + return err;
> + }
> +
> + if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
> + asus->gpu_mux_mode_available = true;
> + asus->gpu_mux_mode = result & ASUS_WMI_DSTS_STATUS_BIT;
> + }
> +
> + return 0;
> +}
> +
> +static int gpu_mux_mode_write(struct asus_wmi *asus)
> +{
> + u32 retval;
> + u8 value;
> + int err;
> +
> + /* Don't rely on type conversion */
> + value = asus->gpu_mux_mode ? 1 : 0;
> +
> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_GPU_MUX, 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,
> "gpu_mux_mode");
> +
> + return 0;
> +}
> +
> +static ssize_t gpu_mux_mode_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct asus_wmi *asus = dev_get_drvdata(dev);
> + u8 mode = asus->gpu_mux_mode;
> +
> + return sysfs_emit(buf, "%d\n", mode);
> +}
> +
> +static ssize_t gpu_mux_mode_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + bool optimus;
> + int result;
> +
> + struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> + result = kstrtobool(buf, &optimus);
> + if (result)
> + return result;
> +
> + asus->gpu_mux_mode = optimus;
> +
> + result = gpu_mux_mode_write(asus);
> + if (result)
> + return result;
> +
> + return count;
> +}
> +static DEVICE_ATTR_RW(gpu_mux_mode);
> +
> /* TUF Laptop Keyboard RGB Modes
> **********************************************/
> static int keyboard_rgb_check_present(struct asus_wmi *asus)
> {
> @@ -3496,6 +3579,7 @@ static struct attribute *platform_attributes[]
> = {
> &dev_attr_touchpad.attr,
> &dev_attr_egpu_enable.attr,
> &dev_attr_dgpu_disable.attr,
> + &dev_attr_gpu_mux_mode.attr,
> &dev_attr_keyboard_rgb_save.attr,
> &dev_attr_keyboard_rgb_mode.attr,
> &dev_attr_keyboard_rgb_speed.attr,
> @@ -3531,6 +3615,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_gpu_mux_mode.attr)
> + ok = asus->gpu_mux_mode_available;
> else if (attr == &dev_attr_keyboard_rgb_save.attr)
> ok = asus->keyboard_rgb_mode_available;
> else if (attr == &dev_attr_keyboard_rgb_mode.attr)
> @@ -3810,6 +3896,10 @@ static int asus_wmi_add(struct platform_device
> *pdev)
> if (err)
> goto fail_dgpu_disable;
>
> + err = gpu_mux_mode_check_present(asus);
> + if (err)
> + goto fail_gpu_mux_mode;
> +
> err = keyboard_rgb_check_present(asus);
> if (err)
> goto fail_keyboard_rgb_mode;
> @@ -3932,6 +4022,7 @@ static int asus_wmi_add(struct platform_device
> *pdev)
> fail_fan_boost_mode:
> fail_egpu_enable:
> fail_dgpu_disable:
> +fail_gpu_mux_mode:
> 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..3faeb98f6ea9 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
>
> +/* gpu mux switch, 0 = dGPU, 1 = Optimus */
> +#define ASUS_WMI_DEVID_GPU_MUX 0x00090016
> +
> /* TUF laptop RGB control */
> #define ASUS_WMI_DEVID_TUF_RGB_MODE 0x00100056
> /* TUF laptop RGB state control */
> --
> 2.37.1
>
On Tue, Aug 9, 2022 at 4:51 AM Luke D. Jones <[email protected]> wrote:
>
> 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.
...
> + 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),
Too many parentheses.
> + &ret);
> + if (err)
> + dev_err(dev, "Unable to set TUF RGB data?\n");
> +
> + return err;
How ret is being used?
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Tue, Aug 9 2022 at 10:52:21 +0200, Andy Shevchenko
<[email protected]> wrote:
> On Tue, Aug 9, 2022 at 4:51 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
>
> ...
>
>> until the keyboard_rgb_save attribute is set
>> (write-only):
>> * 0 - slow
>> * 1 - medium
>> - * 2 - fast
>> \ No newline at end of file
>
> ^^^
>
>> + * 2 - fast
>> +
>> +What: /sys/devices/platform/<platform>/keyboard_rgb_state
>> +Date: Aug 2022
>> +KernelVersion: 6.1
>> +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
>> \ No newline at end of file
>
> ^^^
>
> Be sure of not repeating the same mistake. I.e. add a newline as
> suggested.
>
>
>> + flags = 0;
>
> Seems you ignored my comment here...
>
>> + if (sscanf(buf, "%hhd %hhd %hhd %hhd %hhd", &save, &boot,
>> &awake, &sleep, &keyboard) != 5
>> + return -EINVAL;
>
>> + save = save == 0 ? 0x0100 : 0x0000;
>
> ...and here...
>
>> + if (boot)
>> + flags |= 0x02;
>> + if (awake)
>> + flags |= 0x08;
>> + if (sleep)
>> + flags |= 0x20;
>> + if (keyboard)
>> + flags |= 0x80;
>
> ...and here.
>
Umm... I know for sure I fixed all those. I must have screwed up a git
rebase :(
I'll fix again.
On Tue, Aug 9, 2022 at 4:51 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
...
> until the keyboard_rgb_save attribute is set (write-only):
> * 0 - slow
> * 1 - medium
> - * 2 - fast
> \ No newline at end of file
^^^
> + * 2 - fast
> +
> +What: /sys/devices/platform/<platform>/keyboard_rgb_state
> +Date: Aug 2022
> +KernelVersion: 6.1
> +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
> \ No newline at end of file
^^^
Be sure of not repeating the same mistake. I.e. add a newline as suggested.
> + flags = 0;
Seems you ignored my comment here...
> + if (sscanf(buf, "%hhd %hhd %hhd %hhd %hhd", &save, &boot, &awake, &sleep, &keyboard) != 5
> + return -EINVAL;
> + save = save == 0 ? 0x0100 : 0x0000;
...and here...
> + if (boot)
> + flags |= 0x02;
> + if (awake)
> + flags |= 0x08;
> + if (sleep)
> + flags |= 0x20;
> + if (keyboard)
> + flags |= 0x80;
...and here.
...
> + 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;
Also see another comment about how ret is supposed to be used.
--
With Best Regards,
Andy Shevchenko
On Tue, Aug 9, 2022 at 4:51 AM Luke D. Jones <[email protected]> wrote:
>
> Documents some previously added attributes:
> - dgpu_disable
> - egpu_enable
> - panel_od
Try to find a commit that introduced thouse and add the respective Fixes tag.
...
> - * keyboard - 0 or 1, unknown what effect this really has
> \ No newline at end of file
> + * keyboard - 0 or 1, unknown what effect this really has
Should be part of another patch.
--
With Best Regards,
Andy Shevchenko
On Tue, Aug 9, 2022 at 4:51 AM 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.
>
> One patch adds documentation in:
> Documentation/ABI/testing/sysfs-platform-asus-wmi
> for some features that were added previously.
>
> The final patch adds support for a particular MUX switch found only
> on a few ROG laptops. This patch is added to this series due to some
> conflicts in merge caused by the RGB patch series.
>
> 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.
It seems you send too many new versions of the series too fast.
Submitting Patches [1] suggest one week gap between series. I would
recommend reading that document in full and carefully to understand
the Linux kernel process of proposing and submitting changes.
[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#don-t-get-discouraged-or-impatient
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Tue, Aug 9 2022 at 10:46:33 +0200, Andy Shevchenko
<[email protected]> wrote:
> On Tue, Aug 9, 2022 at 4:51 AM Luke D. Jones <[email protected]> wrote:
>>
>> 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.
>
> ...
>
>> + 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),
>
> Too many parentheses.
Uh, yeah. I was unable to find concrete info on this. I at one point
experienced an issue where the order of operations *without*
parentheses ended up as "x | y << (8 | z) << 16". But now I'm not even
sure if I remember that correctly. I see the order here
https://www.cs.uic.edu/~i109/Notes/COperatorPrecedenceTable.pdf
I'll do as said and test it to be certain.
>
>> + &ret);
>> + if (err)
>> + dev_err(dev, "Unable to set TUF RGB data?\n");
>> +
>> + return err;
>
> How ret is being used?
Damn.. fixed now.
>
> --
> With Best Regards,
> Andy Shevchenko
Hi,
On Tue, 2022-08-09 at 11:22 +0200, Andy Shevchenko wrote:
> On Tue, Aug 9, 2022 at 4:51 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.
> >
> > These sysfs attributes are added to asus-nb-wmi:
> > - keyboard_rgb_save
> > - keyboard_rgb_mode
> > - keyboard_rgb_speed
>
> ...
>
> > +What: /sys/devices/platform/<platform>/keyboard_rgb_speed
> > +Date: Aug 2022
> > +KernelVersion: 6.1
> > +Contact: "Luke Jones" <[email protected]>
> > +Description:
> > + Set the speed of the selected RGB effect, the speed
> > will not apply
> > + until the keyboard_rgb_save attribute is set
> > (write-only):
> > + * 0 - slow
> > + * 1 - medium
> > + * 2 - fast
>
> > \ No newline at end of file
>
> ^^^
>
> ...
>
> > + u8 save;
> > + int err;
>
> > +
> > + struct asus_wmi *asus = dev_get_drvdata(device);
> > + struct led_classdev *cdev = &asus-
> > >keyboard_rgb_led.dev.led_cdev;
>
> No blank line in the definition block and try to keep "the longest
> line first", a.k.a. reversed xmas tree ordering.
>
> ...
>
> > + u8 mode;
> > +
> > + struct asus_wmi *asus = dev_get_drvdata(device);
>
> Ditto.
>
> I would really recommend you spending some time to read the existing
> code (better recent one) and look for the common patterns.
>
> ...
>
> > + /* These are the known usable modes across all TUF/ROG */
> > + if (mode >= 12 || mode == 10)
>
> The second condition was different in previous versions. Or am I
> confused by another patch series?
>
It's a mistake on my part..
> > + asus->keyboard_rgb_led.mode = 10;
> > + else
> > + asus->keyboard_rgb_led.mode = mode;
>
> ...
>
> > +
> > +
>
> Single blank line is enough.
>
> ...
>
> > - struct mc_subled *mc_led_info = asus-
> > >keyboard_rgb_mode.subled_info;
> > - struct led_classdev_mc *mc_cdev = &asus-
> > >keyboard_rgb_mode.dev;
> > + struct mc_subled *mc_led_info = asus-
> > >keyboard_rgb_led.subled_info;
> > + struct led_classdev_mc *mc_cdev = &asus-
> > >keyboard_rgb_led.dev;
>
> Not sure why this change happened...
I thought I wrote this in changelog. Either way, it is meant to be in
first patch. git can be damned frustrating to use.
I will try to fix all these little things and let the series bake at
least a week before next version.
Kind regards,
Luke.
On Tue, Aug 9, 2022 at 4:51 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.
>
> These sysfs attributes are added to asus-nb-wmi:
> - keyboard_rgb_save
> - keyboard_rgb_mode
> - keyboard_rgb_speed
...
> +What: /sys/devices/platform/<platform>/keyboard_rgb_speed
> +Date: Aug 2022
> +KernelVersion: 6.1
> +Contact: "Luke Jones" <[email protected]>
> +Description:
> + Set the speed of the selected RGB effect, the speed will not apply
> + until the keyboard_rgb_save attribute is set (write-only):
> + * 0 - slow
> + * 1 - medium
> + * 2 - fast
> \ No newline at end of file
^^^
...
> + u8 save;
> + int err;
> +
> + struct asus_wmi *asus = dev_get_drvdata(device);
> + struct led_classdev *cdev = &asus->keyboard_rgb_led.dev.led_cdev;
No blank line in the definition block and try to keep "the longest
line first", a.k.a. reversed xmas tree ordering.
...
> + u8 mode;
> +
> + struct asus_wmi *asus = dev_get_drvdata(device);
Ditto.
I would really recommend you spending some time to read the existing
code (better recent one) and look for the common patterns.
...
> + /* These are the known usable modes across all TUF/ROG */
> + if (mode >= 12 || mode == 10)
The second condition was different in previous versions. Or am I
confused by another patch series?
> + asus->keyboard_rgb_led.mode = 10;
> + else
> + asus->keyboard_rgb_led.mode = mode;
...
> +
> +
Single blank line is enough.
...
> - struct mc_subled *mc_led_info = asus->keyboard_rgb_mode.subled_info;
> - struct led_classdev_mc *mc_cdev = &asus->keyboard_rgb_mode.dev;
> + struct mc_subled *mc_led_info = asus->keyboard_rgb_led.subled_info;
> + struct led_classdev_mc *mc_cdev = &asus->keyboard_rgb_led.dev;
Not sure why this change happened...
> - asus->keyboard_rgb_mode.save = 1;
> - asus->keyboard_rgb_mode.mode = 0;
> - asus->keyboard_rgb_mode.speed = 0xeb;
> + asus->keyboard_rgb_led.save = 1;
> + asus->keyboard_rgb_led.mode = 0;
> + asus->keyboard_rgb_led.speed = 0xeb;
...and this.
Is it some kind of renaming? Can you split it to another patch if it
was initially like that?
--
With Best Regards,
Andy Shevchenko
On Tue, 2022-08-09 at 11:29 +0200, Andy Shevchenko wrote:
> On Tue, Aug 9, 2022 at 10:56 AM Luke Jones <[email protected]> wrote:
> > On Tue, Aug 9 2022 at 10:46:33 +0200, Andy Shevchenko
> > <[email protected]> wrote:
> > > On Tue, Aug 9, 2022 at 4:51 AM Luke D. Jones <[email protected]>
> > > wrote:
>
> ...
>
> > > > + 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),
> > >
> > > Too many parentheses.
> >
> > Uh, yeah. I was unable to find concrete info on this. I at one
> > point
> > experienced an issue where the order of operations *without*
> > parentheses ended up as "x | y << (8 | z) << 16". But now I'm not
> > even
> > sure if I remember that correctly. I see the order here
> > https://www.cs.uic.edu/~i109/Notes/COperatorPrecedenceTable.pdf
> >
> > I'll do as said and test it to be certain.
>
> I'm talking about the '(b)' part. The rest is okay to me.
>
Understood, thanks.
On Tue, Aug 9, 2022 at 10:56 AM Luke Jones <[email protected]> wrote:
> On Tue, Aug 9 2022 at 10:46:33 +0200, Andy Shevchenko
> <[email protected]> wrote:
> > On Tue, Aug 9, 2022 at 4:51 AM Luke D. Jones <[email protected]> wrote:
...
> >> + 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),
> >
> > Too many parentheses.
>
> Uh, yeah. I was unable to find concrete info on this. I at one point
> experienced an issue where the order of operations *without*
> parentheses ended up as "x | y << (8 | z) << 16". But now I'm not even
> sure if I remember that correctly. I see the order here
> https://www.cs.uic.edu/~i109/Notes/COperatorPrecedenceTable.pdf
>
> I'll do as said and test it to be certain.
I'm talking about the '(b)' part. The rest is okay to me.
--
With Best Regards,
Andy Shevchenko
On Tue, Aug 9, 2022 at 4:51 AM Luke D. Jones <[email protected]> wrote:
>
> 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.
This looks good to me,
Reviewed-by: Andy Shevchenko <[email protected]>
(Carry the tag with a new version of the series in this patch)
Hans, I guess this one can be applied so it will be less of a burden
on reviewing the rest.
> 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 719223804c0e..78f1f3af1b12 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -942,7 +942,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);
> @@ -2032,7 +2032,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,
> @@ -2092,7 +2092,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,
> @@ -2110,7 +2110,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,
> @@ -2178,7 +2178,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,
> @@ -2371,7 +2371,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,
> @@ -2924,7 +2924,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
>
--
With Best Regards,
Andy Shevchenko
Hi!
> 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.
> + 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.
> + */
Ouch. Lets not do that? If rgb interface is available, hide the 3
level one, or something.
> + 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.
Thanks and best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
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.
Hi,
On 8/9/22 04:50, Luke D. Jones wrote:
> Support the hardware GPU MUX switch available on some models. This
> switch can toggle the MUX between:
>
> - 0, Dedicated mode
> - 1, Optimus mode
>
> Optimus mode is the regular iGPU + dGPU available, while dedicated
> mode switches the system to have only the dGPU available.
>
> Signed-off-by: Luke D. Jones <[email protected]>
I see that you have replied to this that it needs more work.
Besides it needing more work, ideally this should hook into
the existing vga-switcheroo mechanism for this. Can you take
a look at that please?
I think this might be the first non GPU driver doing vga-
switcheroo stuff. So this may be something to discuss
on the dri-devel list.
And if things don't work out we can always go back to
just using an asus-wmi specific sysfs file for this as
is done in this patch.
Regards,
Hans
> ---
> .../ABI/testing/sysfs-platform-asus-wmi | 9 ++
> drivers/platform/x86/asus-wmi.c | 91 +++++++++++++++++++
> include/linux/platform_data/x86/asus-wmi.h | 3 +
> 3 files changed, 103 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index 541dbfbbbb26..d483bc3cb2e6 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -67,6 +67,15 @@ Description:
> * 0 - Disable,
> * 1 - Enable,
>
> +What: /sys/devices/platform/<platform>/gpu_mux_mode
> +Date: Aug 2022
> +KernelVersion: 6.0
> +Contact: "Luke Jones" <[email protected]>
> +Description:
> + Switch the GPU used by the hardware MUX:
> + * 0 - Dedicated GPU,
> + * 1 - Optimus mode,
> +
> 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 78f1f3af1b12..c5fa21370481 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 gpu_mux_mode_available;
> + bool gpu_mux_mode;
> +
> bool keyboard_rgb_state_available;
> bool keyboard_rgb_mode_available;
> struct keyboard_rgb_led keyboard_rgb_led;
> @@ -750,6 +753,86 @@ static ssize_t egpu_enable_store(struct device *dev,
>
> static DEVICE_ATTR_RW(egpu_enable);
>
> +/* gpu mux switch *************************************************************/
> +static int gpu_mux_mode_check_present(struct asus_wmi *asus)
> +{
> + u32 result;
> + int err;
> +
> + asus->gpu_mux_mode_available = false;
> +
> + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_GPU_MUX, &result);
> + if (err) {
> + if (err == -ENODEV)
> + return 0;
> + return err;
> + }
> +
> + if (result & ASUS_WMI_DSTS_PRESENCE_BIT) {
> + asus->gpu_mux_mode_available = true;
> + asus->gpu_mux_mode = result & ASUS_WMI_DSTS_STATUS_BIT;
> + }
> +
> + return 0;
> +}
> +
> +static int gpu_mux_mode_write(struct asus_wmi *asus)
> +{
> + u32 retval;
> + u8 value;
> + int err;
> +
> + /* Don't rely on type conversion */
> + value = asus->gpu_mux_mode ? 1 : 0;
> +
> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_GPU_MUX, 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, "gpu_mux_mode");
> +
> + return 0;
> +}
> +
> +static ssize_t gpu_mux_mode_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct asus_wmi *asus = dev_get_drvdata(dev);
> + u8 mode = asus->gpu_mux_mode;
> +
> + return sysfs_emit(buf, "%d\n", mode);
> +}
> +
> +static ssize_t gpu_mux_mode_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + bool optimus;
> + int result;
> +
> + struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> + result = kstrtobool(buf, &optimus);
> + if (result)
> + return result;
> +
> + asus->gpu_mux_mode = optimus;
> +
> + result = gpu_mux_mode_write(asus);
> + if (result)
> + return result;
> +
> + return count;
> +}
> +static DEVICE_ATTR_RW(gpu_mux_mode);
> +
> /* TUF Laptop Keyboard RGB Modes **********************************************/
> static int keyboard_rgb_check_present(struct asus_wmi *asus)
> {
> @@ -3496,6 +3579,7 @@ static struct attribute *platform_attributes[] = {
> &dev_attr_touchpad.attr,
> &dev_attr_egpu_enable.attr,
> &dev_attr_dgpu_disable.attr,
> + &dev_attr_gpu_mux_mode.attr,
> &dev_attr_keyboard_rgb_save.attr,
> &dev_attr_keyboard_rgb_mode.attr,
> &dev_attr_keyboard_rgb_speed.attr,
> @@ -3531,6 +3615,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_gpu_mux_mode.attr)
> + ok = asus->gpu_mux_mode_available;
> else if (attr == &dev_attr_keyboard_rgb_save.attr)
> ok = asus->keyboard_rgb_mode_available;
> else if (attr == &dev_attr_keyboard_rgb_mode.attr)
> @@ -3810,6 +3896,10 @@ static int asus_wmi_add(struct platform_device *pdev)
> if (err)
> goto fail_dgpu_disable;
>
> + err = gpu_mux_mode_check_present(asus);
> + if (err)
> + goto fail_gpu_mux_mode;
> +
> err = keyboard_rgb_check_present(asus);
> if (err)
> goto fail_keyboard_rgb_mode;
> @@ -3932,6 +4022,7 @@ static int asus_wmi_add(struct platform_device *pdev)
> fail_fan_boost_mode:
> fail_egpu_enable:
> fail_dgpu_disable:
> +fail_gpu_mux_mode:
> 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..3faeb98f6ea9 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
>
> +/* gpu mux switch, 0 = dGPU, 1 = Optimus */
> +#define ASUS_WMI_DEVID_GPU_MUX 0x00090016
> +
> /* TUF laptop RGB control */
> #define ASUS_WMI_DEVID_TUF_RGB_MODE 0x00100056
> /* TUF laptop RGB state control */
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. 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
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).
> 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
>
Hi,
On 8/9/22 11:25, Andy Shevchenko wrote:
> On Tue, Aug 9, 2022 at 4:51 AM Luke D. Jones <[email protected]> wrote:
>>
>> Documents some previously added attributes:
>> - dgpu_disable
>> - egpu_enable
>> - panel_od
>
> Try to find a commit that introduced thouse and add the respective Fixes tag.
>
> ...
>
>> - * keyboard - 0 or 1, unknown what effect this really has
>> \ No newline at end of file
>> + * keyboard - 0 or 1, unknown what effect this really has
>
> Should be part of another patch.
Ack to both remarks.
Luke please make this patch one of the first patches in your next
version / patch-series and then add the doucmentation of the new
kbd RGB backlight attributes in the patches where you are also
adding those actual attributes.
Regards,
Hans
Hi,
On 8/9/22 11:27, Andy Shevchenko wrote:
> On Tue, Aug 9, 2022 at 4:51 AM Luke D. Jones <[email protected]> wrote:
>>
>> 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.
>
> This looks good to me,
> Reviewed-by: Andy Shevchenko <[email protected]>
>
> (Carry the tag with a new version of the series in this patch)
>
> Hans, I guess this one can be applied so it will be less of a burden
> on reviewing the rest.
Thanks for the review. I agree that this one can be merged
already, so I have just merged it:
Thank you for your patch, I've applied this patch to my review-hans
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.
Patches which are added to review-hans now are intended for
the next rc1. This branch will get rebased to the next rc1 when
it is out and after the rebasing the contents of review-hans
will be pushed to the platform-drivers-x86/for-next branch.
Regards,
Hans
>
>> 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 719223804c0e..78f1f3af1b12 100644
>> --- a/drivers/platform/x86/asus-wmi.c
>> +++ b/drivers/platform/x86/asus-wmi.c
>> @@ -942,7 +942,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);
>> @@ -2032,7 +2032,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,
>> @@ -2092,7 +2092,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,
>> @@ -2110,7 +2110,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,
>> @@ -2178,7 +2178,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,
>> @@ -2371,7 +2371,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,
>> @@ -2924,7 +2924,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
>>
>
>
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.
Hi,
On Thu, 2022-08-11 at 17:08 +0200, Hans de Goede wrote:
> Hi,
>
> On 8/9/22 11:25, Andy Shevchenko wrote:
> > On Tue, Aug 9, 2022 at 4:51 AM Luke D. Jones <[email protected]>
> > wrote:
> > >
> > > Documents some previously added attributes:
> > > - dgpu_disable
> > > - egpu_enable
> > > - panel_od
> >
> > Try to find a commit that introduced thouse and add the respective
> > Fixes tag.
> >
> > ...
> >
> > > - * keyboard - 0 or 1, unknown what effect
> > > this really has
> > > \ No newline at end of file
> > > + * keyboard - 0 or 1, unknown what effect
> > > this really has
> >
> > Should be part of another patch.
>
> Ack to both remarks.
>
> Luke please make this patch one of the first patches in your next
> version / patch-series and then add the doucmentation of the new
> kbd RGB backlight attributes in the patches where you are also
> adding those actual attributes.
Yep, done. Everything is looking much cleaner too.
Hi Hans,
On Thu, 2022-08-11 at 15:53 +0200, Hans de Goede wrote:
> Hi,
>
> On 8/9/22 04:50, Luke D. Jones wrote:
> > Support the hardware GPU MUX switch available on some models. This
> > switch can toggle the MUX between:
> >
> > - 0, Dedicated mode
> > - 1, Optimus mode
> >
> > Optimus mode is the regular iGPU + dGPU available, while dedicated
> > mode switches the system to have only the dGPU available.
> >
> > Signed-off-by: Luke D. Jones <[email protected]>
>
> I see that you have replied to this that it needs more work.
>
> Besides it needing more work, ideally this should hook into
> the existing vga-switcheroo mechanism for this. Can you take
> a look at that please?
>
> I think this might be the first non GPU driver doing vga-
> switcheroo stuff. So this may be something to discuss
> on the dri-devel list.
I'm not sure how this would work. In typical ASUS fashion they do non-
standard stuff. This switch is a basic toggle that requires a reboot to
enable after writing to the ACPI method, after reboot the dGPU becomes
the only visible GPU on the system and (this GPU) can not be suspended.
In short: it toggles the laptop from discrete-only mode, and optimus
mode, requiring a reboot to switch.
From what I understand of switcheroo it is more to manage having dual
(or more) GPU available during runtime, and manage the power states,
offload etc.
I have a vastly improved patch for this prepared now. Because of how
the actual feature works (and the above explanation) it must be under
the asus-nb-wmi sysfs (next to the dgpu_disable and egpu_enable toggles
which are also unusual and non-standard work-arounds of Windows
issues).
Kind regards,
Luke.
Hi,
On 8/12/22 00:01, Luke Jones wrote:
> Hi Hans,
>
> On Thu, 2022-08-11 at 15:53 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 8/9/22 04:50, Luke D. Jones wrote:
>>> Support the hardware GPU MUX switch available on some models. This
>>> switch can toggle the MUX between:
>>>
>>> - 0, Dedicated mode
>>> - 1, Optimus mode
>>>
>>> Optimus mode is the regular iGPU + dGPU available, while dedicated
>>> mode switches the system to have only the dGPU available.
>>>
>>> Signed-off-by: Luke D. Jones <[email protected]>
>>
>> I see that you have replied to this that it needs more work.
>>
>> Besides it needing more work, ideally this should hook into
>> the existing vga-switcheroo mechanism for this. Can you take
>> a look at that please?
>>
>> I think this might be the first non GPU driver doing vga-
>> switcheroo stuff. So this may be something to discuss
>> on the dri-devel list.
>
> I'm not sure how this would work. In typical ASUS fashion they do non-
> standard stuff. This switch is a basic toggle that requires a reboot to
> enable after writing to the ACPI method, after reboot the dGPU becomes
> the only visible GPU on the system and (this GPU) can not be suspended.
>
> In short: it toggles the laptop from discrete-only mode, and optimus
> mode, requiring a reboot to switch.
>
> From what I understand of switcheroo it is more to manage having dual
> (or more) GPU available during runtime, and manage the power states,
> offload etc.
Right, I did not realize this requires a reboot, that would be
something to mention in the Documentation bits accompanying the patch.
This is also a reason why it is good to have the docs update in
the same patch as adding the functionality, because the docs may
help with reviewing.
Anyways I agree that if this requires a reboot then using
the vga switcheroo stuff is not applicable. So we can just go with
a simple(ish) asus-wmi sysfs attribute.
> I have a vastly improved patch for this prepared now. Because of how
> the actual feature works (and the above explanation) it must be under
> the asus-nb-wmi sysfs (next to the dgpu_disable and egpu_enable toggles
> which are also unusual and non-standard work-arounds of Windows
> issues).
Ack, sounds good.
Regards,
Hans
Hi,
On 2022-08-12 09:59+0200, Hans de Goede wrote:
> Date: Fri, 12 Aug 2022 09:59:29 +0200
> From: Hans de Goede <[email protected]>
> To: Luke Jones <[email protected]>
> Cc: [email protected], [email protected], [email protected],
> [email protected], [email protected]
> Subject: Re: [PATCH v3 6/6] asus-wmi: Support the hardware GPU MUX on some
> laptops
> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
> Thunderbird/91.12.0
>
> Hi,
>
> On 8/12/22 00:01, Luke Jones wrote:
> > Hi Hans,
> >
> > On Thu, 2022-08-11 at 15:53 +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 8/9/22 04:50, Luke D. Jones wrote:
> >>> Support the hardware GPU MUX switch available on some models. This
> >>> switch can toggle the MUX between:
> >>>
> >>> - 0, Dedicated mode
> >>> - 1, Optimus mode
> >>>
> >>> Optimus mode is the regular iGPU + dGPU available, while dedicated
> >>> mode switches the system to have only the dGPU available.
> >>>
> >>> Signed-off-by: Luke D. Jones <[email protected]>
> >>
> >> I see that you have replied to this that it needs more work.
> >>
> >> Besides it needing more work, ideally this should hook into
> >> the existing vga-switcheroo mechanism for this. Can you take
> >> a look at that please?
> >>
> >> I think this might be the first non GPU driver doing vga-
> >> switcheroo stuff. So this may be something to discuss
> >> on the dri-devel list.
> >
> > I'm not sure how this would work. In typical ASUS fashion they do non-
> > standard stuff. This switch is a basic toggle that requires a reboot to
> > enable after writing to the ACPI method, after reboot the dGPU becomes
> > the only visible GPU on the system and (this GPU) can not be suspended.
> >
> > In short: it toggles the laptop from discrete-only mode, and optimus
> > mode, requiring a reboot to switch.
> >
> > From what I understand of switcheroo it is more to manage having dual
> > (or more) GPU available during runtime, and manage the power states,
> > offload etc.
>
> Right, I did not realize this requires a reboot, that would be
> something to mention in the Documentation bits accompanying the patch.
>
> This is also a reason why it is good to have the docs update in
> the same patch as adding the functionality, because the docs may
> help with reviewing.
>
> Anyways I agree that if this requires a reboot then using
> the vga switcheroo stuff is not applicable. So we can just go with
> a simple(ish) asus-wmi sysfs attribute.
Would this not fit the existing "firmware-attributes" class?
It even has a flag to signal that a reboot is required after an attribute has
been changed.
Maybe it is overkill to use it only for this, though.
> > I have a vastly improved patch for this prepared now. Because of how
> > the actual feature works (and the above explanation) it must be under
> > the asus-nb-wmi sysfs (next to the dgpu_disable and egpu_enable toggles
> > which are also unusual and non-standard work-arounds of Windows
> > issues).
>
> Ack, sounds good.
Thomas
Hi,
On 8/12/22 10:31, Thomas Weißschuh wrote:
> Hi,
>
> On 2022-08-12 09:59+0200, Hans de Goede wrote:
>> Date: Fri, 12 Aug 2022 09:59:29 +0200
>> From: Hans de Goede <[email protected]>
>> To: Luke Jones <[email protected]>
>> Cc: [email protected], [email protected], [email protected],
>> [email protected], [email protected]
>> Subject: Re: [PATCH v3 6/6] asus-wmi: Support the hardware GPU MUX on some
>> laptops
>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
>> Thunderbird/91.12.0
>>
>> Hi,
>>
>> On 8/12/22 00:01, Luke Jones wrote:
>>> Hi Hans,
>>>
>>> On Thu, 2022-08-11 at 15:53 +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 8/9/22 04:50, Luke D. Jones wrote:
>>>>> Support the hardware GPU MUX switch available on some models. This
>>>>> switch can toggle the MUX between:
>>>>>
>>>>> - 0, Dedicated mode
>>>>> - 1, Optimus mode
>>>>>
>>>>> Optimus mode is the regular iGPU + dGPU available, while dedicated
>>>>> mode switches the system to have only the dGPU available.
>>>>>
>>>>> Signed-off-by: Luke D. Jones <[email protected]>
>>>>
>>>> I see that you have replied to this that it needs more work.
>>>>
>>>> Besides it needing more work, ideally this should hook into
>>>> the existing vga-switcheroo mechanism for this. Can you take
>>>> a look at that please?
>>>>
>>>> I think this might be the first non GPU driver doing vga-
>>>> switcheroo stuff. So this may be something to discuss
>>>> on the dri-devel list.
>>>
>>> I'm not sure how this would work. In typical ASUS fashion they do non-
>>> standard stuff. This switch is a basic toggle that requires a reboot to
>>> enable after writing to the ACPI method, after reboot the dGPU becomes
>>> the only visible GPU on the system and (this GPU) can not be suspended.
>>>
>>> In short: it toggles the laptop from discrete-only mode, and optimus
>>> mode, requiring a reboot to switch.
>>>
>>> From what I understand of switcheroo it is more to manage having dual
>>> (or more) GPU available during runtime, and manage the power states,
>>> offload etc.
>>
>> Right, I did not realize this requires a reboot, that would be
>> something to mention in the Documentation bits accompanying the patch.
>>
>> This is also a reason why it is good to have the docs update in
>> the same patch as adding the functionality, because the docs may
>> help with reviewing.
>>
>> Anyways I agree that if this requires a reboot then using
>> the vga switcheroo stuff is not applicable. So we can just go with
>> a simple(ish) asus-wmi sysfs attribute.
>
> Would this not fit the existing "firmware-attributes" class?
> It even has a flag to signal that a reboot is required after an attribute has
> been changed.
Yes it sounds like a BIOS setting is being toggled from within
Linux, which would normally be done through the
"firmware-attributes" class, but all existing "firmware-attributes"
class drivers allow changing all BIOS setting not just a single
setting, so using the "firmware-attributes" class here is not really
appropriate.
> Maybe it is overkill to use it only for this, though.
Right :)
Regards,
Hans
>>> I have a vastly improved patch for this prepared now. Because of how
>>> the actual feature works (and the above explanation) it must be under
>>> the asus-nb-wmi sysfs (next to the dgpu_disable and egpu_enable toggles
>>> which are also unusual and non-standard work-arounds of Windows
>>> issues).
>>
>> Ack, sounds good.
>
> Thomas
>