[PATCH v3 00/11] asus-wmi: Support of ASUS TUF Gaming series laptops

[PATCH v3 00/11] asus-wmi: Support of ASUS TUF Gaming series laptops

[Yurii Pavlovskyi]

Hi,

this is the third version of the patch series. 

Changelog:
v3: 
  * Use devm_* function in patch 01
  * Detect DSTS/DCTS using _UID in patch 04
  * Detect event queue by _UID as well in patch 05
  * Rename poll function in patch 05
  * Fix terminology in patches 09 and 10
  * Correct commit messages
v2:
  * Fix logging

INTRODUCTION
The support for this laptop series is currently non-existent, as the
asus-nb-wmi driver (which is essentially configuration for asus-wmi) fails
to load and multiple ACPI errors are logged in dmesg. This patch series
adds pretty comprehensive support for these relatively new laptops, adds
some code organization, and fixes a couple of bugs in the asus-wmi module.

Original message from V1/V2:
https://lkml.org/lkml/2019/4/10/973

It is really long, so I will not copy it completely here, please refer
to the original for notes on design decisions and existing minor issues
(other than quirks, which should be hopefully solved now).

Yurii Pavlovskyi (11):
  platform/x86: asus-wmi: Fix hwmon device cleanup
  platform/x86: asus-wmi: Fix preserving keyboard backlight intensity on
    load
  platform/x86: asus-wmi: Increase the input buffer size of WMI methods
  platform/x86: asus-wmi: Improve DSTS WMI method ID detection
  platform/x86: asus-wmi: Support WMI event queue
  platform/x86: asus-nb-wmi: Add microphone mute key code
  platform/x86: asus-wmi: Organize code into sections
  platform/x86: asus-wmi: Enhance detection of thermal data
  platform/x86: asus-wmi: Control RGB keyboard backlight
  platform/x86: asus-wmi: Switch fan boost mode
  platform/x86: asus-wmi: Do not disable keyboard backlight on unloading

 .../ABI/testing/sysfs-platform-asus-wmi       |  71 ++
 drivers/platform/x86/asus-nb-wmi.c            |   3 +-
 drivers/platform/x86/asus-wmi.c               | 797 +++++++++++++++---
 drivers/platform/x86/wmi.c                    |  19 +
 include/linux/acpi.h                          |   1 +
 include/linux/platform_data/x86/asus-wmi.h    |   7 +-
 6 files changed, 797 insertions(+), 101 deletions(-)

-- 
2.17.1

[PATCH v3 01/11] platform/x86: asus-wmi: Fix hwmon device cleanup

[Yurii Pavlovskyi]

The driver does not clean up the hwmon device on exit or error. To
reproduce the bug, repeat rmmod, insmod to verify that device number
/sys/devices/platform/asus-nb-wmi/hwmon/hwmon?? grows every time. Replace
call for registering device with devm_* version that unregisters it
automatically.

Signed-off-by: Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com>
---
 drivers/platform/x86/asus-wmi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index ee1fa93708ec..d865eb95054c 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -1425,9 +1425,10 @@ static int asus_wmi_hwmon_init(struct asus_wmi *asus)
 {
 	struct device *hwmon;
 
-	hwmon = hwmon_device_register_with_groups(&asus->platform_device->dev,
-						  "asus", asus,
-						  hwmon_attribute_groups);
+	hwmon = devm_hwmon_device_register_with_groups(
+			&asus->platform_device->dev, "asus", asus,
+			hwmon_attribute_groups);
+
 	if (IS_ERR(hwmon)) {
 		pr_err("Could not register asus hwmon device\n");
 		return PTR_ERR(hwmon);
-- 
2.17.1

[PATCH v3 02/11] platform/x86: asus-wmi: Fix preserving keyboard backlight intensity on load

[Yurii Pavlovskyi]

The error code and return value are mixed up. The intensity is always set
to 0 on load as kbd_led_read returns either 0 or negative value. To
reproduce set backlight to maximum, reload driver and try to increase it
using keyboard hotkey, the intensity will drop as a result. Correct the
implementation.

Signed-off-by: Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com>
---
 drivers/platform/x86/asus-wmi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index d865eb95054c..731ffd382426 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -590,8 +590,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
 			goto error;
 	}
 
-	led_val = kbd_led_read(asus, NULL, NULL);
-	if (led_val >= 0) {
+	if (!kbd_led_read(asus, &led_val, NULL)) {
 		asus->kbd_led_wk = led_val;
 		asus->kbd_led.name = "asus::kbd_backlight";
 		asus->kbd_led.flags = LED_BRIGHT_HW_CHANGED;
-- 
2.17.1

[PATCH v3 03/11] platform/x86: asus-wmi: Increase the input buffer size of WMI methods

[Yurii Pavlovskyi]

The asus-nb-wmi driver is matched by WMI alias but fails to load on TUF
Gaming series laptops producing multiple ACPI errors in the kernel log.

The input buffer for WMI method invocation size is 2 dwords, whereas
3 are expected by this model.

FX505GM:
..
Method (WMNB, 3, Serialized)
{
    P8XH (Zero, 0x11)
    CreateDWordField (Arg2, Zero, IIA0)
    CreateDWordField (Arg2, 0x04, IIA1)
    CreateDWordField (Arg2, 0x08, IIA2)
    Local0 = (Arg1 & 0xFFFFFFFF)
    ...

Compare with older K54C:
...
Method (WMNB, 3, NotSerialized)
{
    CreateDWordField (Arg2, 0x00, IIA0)
    CreateDWordField (Arg2, 0x04, IIA1)
    Local0 = (Arg1 & 0xFFFFFFFF)
    ...

Increase buffer size to 3 dwords. No negative consequences of this change
are expected, as the input buffer size is not verified. The original
function is replaced by a wrapper for a new method passing value 0 for the
last parameter. The new function will be used to control RGB keyboard
backlight.

Signed-off-by: Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com>
---
One of current kernel errors:
ACPI BIOS Error (bug): AE_AML_BUFFER_LIMIT, Field [IIA2] at bit offset/
	length 64/32 exceeds size of target Buffer (64 bits)
	(20190215/dsopcode-203)
[ 4528.573948] No Local Variables are initialized for Method [WMNB]
[ 4528.573949] Initialized Arguments for Method [WMNB]:  (3 arguments
	defined for method invocation)
[ 4528.573950]   Arg0:   00000000bd1bea5a <Obj>
	Integer 0000000000000000
[ 4528.573952]   Arg1:   00000000d414dc53 <Obj>
	Integer 000000004E464741
[ 4528.573954]   Arg2:   00000000fcefea4b <Obj>
	Buffer(8) F0 95 08 00 00 00 00 00
[ 4528.573959] ACPI Error: Aborting method \_SB.ATKD.WMNB due to previous
	error (AE_AML_BUFFER_LIMIT) (20190215/psparse-531)
[ 4528.686425] asus-nb-wmi: probe of asus-nb-wmi failed with error -5
---
 drivers/platform/x86/asus-wmi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 731ffd382426..ba04737ece0d 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -95,6 +95,7 @@ static bool ashs_present(void)
 struct bios_args {
 	u32 arg0;
 	u32 arg1;
+	u32 arg2; /* At least TUF Gaming series uses 3 dword input buffer. */
 } __packed;
 
 /*
@@ -219,11 +220,13 @@ static void asus_wmi_input_exit(struct asus_wmi *asus)
 	asus->inputdev = NULL;
 }
 
-int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval)
+static int asus_wmi_evaluate_method_3dw(u32 method_id, u32 arg0, u32 arg1,
+		u32 arg2, u32 *retval)
 {
 	struct bios_args args = {
 		.arg0 = arg0,
 		.arg1 = arg1,
+		.arg2 = arg2
 	};
 	struct acpi_buffer input = { (acpi_size) sizeof(args), &args };
 	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -255,6 +258,11 @@ int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval)
 
 	return 0;
 }
+
+int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval)
+{
+	return asus_wmi_evaluate_method_3dw(method_id, arg0, arg1, 0, retval);
+}
 EXPORT_SYMBOL_GPL(asus_wmi_evaluate_method);
 
 static int asus_wmi_evaluate_method_agfn(const struct acpi_buffer args)
-- 
2.17.1

[PATCH v3 04/11] platform/x86: asus-wmi: Improve DSTS WMI method ID detection

[Yurii Pavlovskyi]

The DSTS method detection mistakenly selects DCTS instead of DSTS if
nothing is returned when the method ID is not defined in WMNB. As a result,
the control of keyboard backlight is not functional for TUF Gaming series
laptops. Implement another detection method instead.

There is evidence that DCTS is handled by ACPI WMI devices that have _UID
ASUSWMI, whereas none of the devices without ASUSWMI respond to DCTS and
DSTS is used instead [1]. To check the _UID a new method is added to wmi.h
/ wmi.c. It returns _UID of the ACPI WMI device that declares WMI object
with given GUID.

Generally, it is possible that multiple PNP0C14 ACPI devices are present in
the system as mentioned in the commit message of commit bff431e49ff5
("ACPI: WMI: Add ACPI-WMI mapping driver").

Therefore the _UID is checked for given GUID that maps to a specific ACPI
device, to which it is also mapped by other methods of wmi module.

DSDT examples:

FX505GM:
Method (WMNB, 3, Serialized)
{ ...
    If ((Local0 == 0x53545344))
    {
        ...
        Return (Zero)
    }
    ...
    // No return
}

K54C:
Method (WMNB, 3, Serialized)
{ ...
    If ((Local0 == 0x53545344))
    {
        ...
        Return (0x02)
    }
    ...
    Return (0xFFFFFFFE)
}

[1] https://lkml.org/lkml/2019/4/11/322

Signed-off-by: Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com>
Suggested-by: Daniel Drake <drake@endlessm.com>
---
 drivers/platform/x86/asus-wmi.c            | 20 ++++++++++++++++++--
 drivers/platform/x86/wmi.c                 | 19 +++++++++++++++++++
 include/linux/acpi.h                       |  1 +
 include/linux/platform_data/x86/asus-wmi.h |  4 ++--
 4 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index ba04737ece0d..266d0eda5476 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -80,6 +80,8 @@ MODULE_LICENSE("GPL");
 #define USB_INTEL_XUSB2PR		0xD0
 #define PCI_DEVICE_ID_INTEL_LYNXPOINT_LP_XHCI	0x9c31
 
+#define ASUS_ACPI_UID_ASUSWMI		"ASUSWMI"
+
 static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
 
 static bool ashs_present(void)
@@ -1847,6 +1849,7 @@ static int asus_wmi_sysfs_init(struct platform_device *device)
 static int asus_wmi_platform_init(struct asus_wmi *asus)
 {
 	int rv;
+	char *wmi_uid;
 
 	/* INIT enable hotkeys on some models */
 	if (!asus_wmi_evaluate_method(ASUS_WMI_METHODID_INIT, 0, 0, &rv))
@@ -1875,11 +1878,24 @@ static int asus_wmi_platform_init(struct asus_wmi *asus)
 	 * Note, on most Eeepc, there is no way to check if a method exist
 	 * or note, while on notebooks, they returns 0xFFFFFFFE on failure,
 	 * but once again, SPEC may probably be used for that kind of things.
+	 *
+	 * Additionally at least TUF Gaming series laptops return nothing for
+	 * unknown methods, so the detection in this way is not possible.
+	 *
+	 * There is strong indication that only ACPI WMI devices that have _UID
+	 * equal to "ASUSWMI" use DCTS whereas those with "ATK" use DSTS.
 	 */
-	if (!asus_wmi_evaluate_method(ASUS_WMI_METHODID_DSTS, 0, 0, NULL))
+	wmi_uid = wmi_get_acpi_device_uid(ASUS_WMI_MGMT_GUID);
+	if (!wmi_uid)
+		return -ENODEV;
+
+	if (!strcmp(wmi_uid, ASUS_ACPI_UID_ASUSWMI)) {
+		pr_info("Detected ASUSWMI, use DCTS\n");
 		asus->dsts_id = ASUS_WMI_METHODID_DSTS;
-	else
+	} else {
+		pr_info("Detected %s, not ASUSWMI, use DSTS\n", wmi_uid);
 		asus->dsts_id = ASUS_WMI_METHODID_DSTS2;
+	}
 
 	/* CWAP allow to define the behavior of the Fn+F2 key,
 	 * this method doesn't seems to be present on Eee PCs */
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 7b26b6ccf1a0..b08ffb769cbe 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -635,6 +635,25 @@ bool wmi_has_guid(const char *guid_string)
 }
 EXPORT_SYMBOL_GPL(wmi_has_guid);
 
+/**
+ * wmi_get_acpi_device_uid() - Get _UID name of ACPI device that defines GUID
+ * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
+ *
+ * Find the _UID of ACPI device associated with this WMI GUID.
+ *
+ * Return: The ACPI _UID field value or NULL if the WMI GUID was not found
+ */
+char *wmi_get_acpi_device_uid(const char *guid_string)
+{
+	struct wmi_block *wblock = NULL;
+
+	if (!find_guid(guid_string, &wblock))
+		return NULL;
+
+	return acpi_device_uid(wblock->acpi_device);
+}
+EXPORT_SYMBOL_GPL(wmi_get_acpi_device_uid);
+
 static struct wmi_block *dev_to_wblock(struct device *dev)
 {
 	return container_of(dev, struct wmi_block, dev.dev);
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d5dcebd7aad3..d31c7fd66f97 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -376,6 +376,7 @@ extern acpi_status wmi_install_notify_handler(const char *guid,
 extern acpi_status wmi_remove_notify_handler(const char *guid);
 extern acpi_status wmi_get_event_data(u32 event, struct acpi_buffer *out);
 extern bool wmi_has_guid(const char *guid);
+extern char *wmi_get_acpi_device_uid(const char *guid);
 
 #endif	/* CONFIG_ACPI_WMI */
 
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 53dfc2541960..a5fe7e68944b 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -18,8 +18,8 @@
 #define ASUS_WMI_METHODID_GDSP		0x50534447 /* Get DiSPlay output */
 #define ASUS_WMI_METHODID_DEVP		0x50564544 /* DEVice Policy */
 #define ASUS_WMI_METHODID_OSVR		0x5256534F /* OS VeRsion */
-#define ASUS_WMI_METHODID_DSTS		0x53544344 /* Device STatuS */
-#define ASUS_WMI_METHODID_DSTS2		0x53545344 /* Device STatuS #2*/
+#define ASUS_WMI_METHODID_DSTS		0x53544344 /* Device STatuS (DCTS) */
+#define ASUS_WMI_METHODID_DSTS2		0x53545344 /* Device STatuS (DSTS) */
 #define ASUS_WMI_METHODID_BSTS		0x53545342 /* Bios STatuS ? */
 #define ASUS_WMI_METHODID_DEVS		0x53564544 /* DEVice Set */
 #define ASUS_WMI_METHODID_CFVS		0x53564643 /* CPU Frequency Volt Set */
-- 
2.17.1

[PATCH v3 05/11] platform/x86: asus-wmi: Support WMI event queue

[Yurii Pavlovskyi]

Event codes are expected to be retrieved from a queue on at least some
models. Specifically, very likely the ACPI WMI devices with _UID ATK are
queued whereas those with ASUSWMI are not [1].

The WMI event codes are pushed into a circular buffer queue. After the INIT
method is called, ACPI code is allowed to push events into this buffer.
The INIT method cannot be reverted. If the module is unloaded and an event
(such as hotkey press) gets emitted before inserting it back the events get
processed delayed by one or if the queue overflows, additionally delayed by
about 3 seconds.

It might be considered a minor issue and no normal user would likely
observe this (there is little reason unloading the driver), but it does
significantly frustrate a developer who is unlucky enough to encounter
this. Therefore, the fallback to unqueued behavior occurs whenever
something unexpected happens.

The fix flushes the old key codes out of the queue on load. After receiving
event the queue is read until either ..FFFF or 1 is encountered. Also as
noted in [1] it is checked whether notify code is equal to 0xFF before
enabling queue processing in WMI notify handler.

DSDT examples:

FX505GM
Device (ATKD)
{ ..
    Name (ATKQ, Package (0x10)
    {
        0xFFFFFFFF, ..
    }

    Method (IANQ, 1, Serialized)
    {
        If ((AQNO >= 0x10))
        {
            Local0 = 0x64
            While ((Local0 && (AQNO >= 0x10)))
            {
                Local0--
                Sleep (0x0A)
            }
            ...
        ..
        AQTI++
        AQTI &= 0x0F
        ATKQ [AQTI] = Arg0
        ...
    }

    Method (GANQ, 0, Serialized)
    {
        ..
        If (AQNO)
        {
            ...
            Local0 = DerefOf (ATKQ [AQHI])
            AQHI++
            AQHI &= 0x0F
            Return (Local0)
        }

        Return (One)
    }

This code is almost identical to K54C, which does return Ones on empty
queue.

K54C:
Method (GANQ, 0, Serialized)
{
    If (AQNO)
    {
        ...
        Return (Local0)
    }

    Return (Ones)
}

[1] https://lkml.org/lkml/2019/4/12/104

Signed-off-by: Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com>
Suggested-by: Daniel Drake <drake@endlessm.com>
---
 drivers/platform/x86/asus-wmi.c | 136 +++++++++++++++++++++++++-------
 1 file changed, 108 insertions(+), 28 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 266d0eda5476..f782dac4cbe7 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -81,6 +81,13 @@ MODULE_LICENSE("GPL");
 #define PCI_DEVICE_ID_INTEL_LYNXPOINT_LP_XHCI	0x9c31
 
 #define ASUS_ACPI_UID_ASUSWMI		"ASUSWMI"
+#define ASUS_ACPI_UID_ATK		"ATK"
+
+#define WMI_EVENT_QUEUE_SIZE		0x10
+#define WMI_EVENT_QUEUE_END		0x1
+#define WMI_EVENT_MASK			0xFFFF
+/* The WMI hotkey event value is always the same. */
+#define WMI_EVENT_VALUE_ATK		0xFF
 
 static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
 
@@ -145,6 +152,7 @@ struct asus_wmi {
 	int dsts_id;
 	int spec;
 	int sfun;
+	bool wmi_event_queue;
 
 	struct input_dev *inputdev;
 	struct backlight_device *backlight_device;
@@ -1629,77 +1637,129 @@ static int is_display_toggle(int code)
 	return 0;
 }
 
-static void asus_wmi_notify(u32 value, void *context)
+static int asus_wmi_get_next_event(u32 value)
 {
-	struct asus_wmi *asus = context;
-	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
 	union acpi_object *obj;
 	acpi_status status;
-	int code;
-	int orig_code;
-	unsigned int key_value = 1;
-	bool autorelease = 1;
+	int code = -EIO;
 
-	status = wmi_get_event_data(value, &response);
-	if (status != AE_OK) {
-		pr_err("bad event status 0x%x\n", status);
-		return;
+	status = wmi_get_event_data(value, &output);
+	if (ACPI_FAILURE(status)) {
+		pr_warn("Failed to get WMI event code: %s\n",
+				acpi_format_exception(status));
+		return code;
 	}
 
-	obj = (union acpi_object *)response.pointer;
+	obj = (union acpi_object *)output.pointer;
 
-	if (!obj || obj->type != ACPI_TYPE_INTEGER)
-		goto exit;
+	if (obj && obj->type == ACPI_TYPE_INTEGER)
+		code = (int)(obj->integer.value & WMI_EVENT_MASK);
+
+	kfree(obj);
+	return code;
+}
+
+static void asus_wmi_handle_notify_code(int code, struct asus_wmi *asus)
+{
+	int orig_code;
+	unsigned int key_value = 1;
+	bool autorelease = 1;
 
-	code = obj->integer.value;
 	orig_code = code;
 
 	if (asus->driver->key_filter) {
 		asus->driver->key_filter(asus->driver, &code, &key_value,
 					 &autorelease);
 		if (code == ASUS_WMI_KEY_IGNORE)
-			goto exit;
+			return;
 	}
 
 	if (code >= NOTIFY_BRNUP_MIN && code <= NOTIFY_BRNUP_MAX)
 		code = ASUS_WMI_BRN_UP;
-	else if (code >= NOTIFY_BRNDOWN_MIN &&
-		 code <= NOTIFY_BRNDOWN_MAX)
+	else if (code >= NOTIFY_BRNDOWN_MIN && code <= NOTIFY_BRNDOWN_MAX)
 		code = ASUS_WMI_BRN_DOWN;
 
 	if (code == ASUS_WMI_BRN_DOWN || code == ASUS_WMI_BRN_UP) {
 		if (acpi_video_get_backlight_type() == acpi_backlight_vendor) {
 			asus_wmi_backlight_notify(asus, orig_code);
-			goto exit;
+			return;
 		}
 	}
 
 	if (code == NOTIFY_KBD_BRTUP) {
 		kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
-		goto exit;
+		return;
 	}
 	if (code == NOTIFY_KBD_BRTDWN) {
 		kbd_led_set_by_kbd(asus, asus->kbd_led_wk - 1);
-		goto exit;
+		return;
 	}
 	if (code == NOTIFY_KBD_BRTTOGGLE) {
 		if (asus->kbd_led_wk == asus->kbd_led.max_brightness)
 			kbd_led_set_by_kbd(asus, 0);
 		else
 			kbd_led_set_by_kbd(asus, asus->kbd_led_wk + 1);
-		goto exit;
+		return;
 	}
 
-	if (is_display_toggle(code) &&
-	    asus->driver->quirks->no_display_toggle)
-		goto exit;
+	if (is_display_toggle(code) && asus->driver->quirks->no_display_toggle)
+		return;
 
 	if (!sparse_keymap_report_event(asus->inputdev, code,
 					key_value, autorelease))
 		pr_info("Unknown key %x pressed\n", code);
+}
 
-exit:
-	kfree(obj);
+static void asus_wmi_notify(u32 value, void *context)
+{
+	struct asus_wmi *asus = context;
+	int code;
+	int i;
+
+	for (i = 0; i < WMI_EVENT_QUEUE_SIZE + 1; i++) {
+		code = asus_wmi_get_next_event(value);
+
+		if (code < 0) {
+			pr_warn("Failed to get event code: 0x%x\n", code);
+			return;
+		}
+
+		if (code == WMI_EVENT_QUEUE_END || code == WMI_EVENT_MASK)
+			return;
+
+		asus_wmi_handle_notify_code(code, asus);
+
+		/*
+		 * Double check that queue is present:
+		 * ATK (with queue) uses 0xff, ASUSWMI (without) 0xd2.
+		 */
+		if (!asus->wmi_event_queue || value != WMI_EVENT_VALUE_ATK)
+			return;
+	}
+
+	pr_warn("Failed to process event queue, last code: 0x%x\n", code);
+}
+
+static int asus_wmi_notify_queue_flush(struct asus_wmi *asus)
+{
+	int code;
+	int i;
+
+	for (i = 0; i < WMI_EVENT_QUEUE_SIZE + 1; i++) {
+		code = asus_wmi_get_next_event(WMI_EVENT_VALUE_ATK);
+
+		if (code < 0) {
+			pr_warn("Failed to get event during flush: %d\n", code);
+			return code;
+		}
+
+		if (code == WMI_EVENT_QUEUE_END || code == WMI_EVENT_MASK)
+			return 0;
+	}
+
+	pr_warn("Failed to flush event queue\n");
+	return -EIO;
 }
 
 /*
@@ -1850,6 +1910,7 @@ static int asus_wmi_platform_init(struct asus_wmi *asus)
 {
 	int rv;
 	char *wmi_uid;
+	int err;
 
 	/* INIT enable hotkeys on some models */
 	if (!asus_wmi_evaluate_method(ASUS_WMI_METHODID_INIT, 0, 0, &rv))
@@ -1897,6 +1958,24 @@ static int asus_wmi_platform_init(struct asus_wmi *asus)
 		asus->dsts_id = ASUS_WMI_METHODID_DSTS2;
 	}
 
+	/*
+	 * Some devices can have multiple event codes stored in a queue before
+	 * the module load if it was unloaded intermittently after calling
+	 * the INIT method (enables event handling). The WMI notify handler is
+	 * expected to retrieve all event codes until a retrieved code equals
+	 * queue end marker (One or Ones). Old codes are flushed from the queue
+	 * upon module load. Not enabling this when it should be has minimal
+	 * visible impact so fall back if anything goes wrong.
+	 */
+	wmi_uid = wmi_get_acpi_device_uid(asus->driver->event_guid);
+	if (wmi_uid && !strcmp(wmi_uid, ASUS_ACPI_UID_ATK)) {
+		pr_info("Detected ATK, enable event queue\n");
+
+		err = asus_wmi_notify_queue_flush(asus);
+		if (!err)
+			asus->wmi_event_queue = true;
+	}
+
 	/* CWAP allow to define the behavior of the Fn+F2 key,
 	 * this method doesn't seems to be present on Eee PCs */
 	if (asus->driver->quirks->wapf >= 0)
@@ -2155,8 +2234,9 @@ static int asus_wmi_add(struct platform_device *pdev)
 		err = asus_wmi_backlight_init(asus);
 		if (err && err != -ENODEV)
 			goto fail_backlight;
-	} else
+	} else {
 		err = asus_wmi_set_devstate(ASUS_WMI_DEVID_BACKLIGHT, 2, NULL);
+	}
 
 	status = wmi_install_notify_handler(asus->driver->event_guid,
 					    asus_wmi_notify, asus);
-- 
2.17.1

[PATCH v3 06/11] platform/x86: asus-nb-wmi: Add microphone mute key code

[Yurii Pavlovskyi]

The microphone mute key is missing from sparse keymap. It is present on
FX505GM and possibly other laptops. Add the missing code.

Also, comment on the fan mode switch key, which has the same code as the
already used key.

Signed-off-by: Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com>
---
 drivers/platform/x86/asus-nb-wmi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c
index b6f2ff95c3ed..d2399ce0b3cd 100644
--- a/drivers/platform/x86/asus-nb-wmi.c
+++ b/drivers/platform/x86/asus-nb-wmi.c
@@ -468,6 +468,7 @@ static const struct key_entry asus_nb_wmi_keymap[] = {
 	{ KE_KEY, 0x6B, { KEY_TOUCHPAD_TOGGLE } },
 	{ KE_IGNORE, 0x6E, },  /* Low Battery notification */
 	{ KE_KEY, 0x7a, { KEY_ALS_TOGGLE } }, /* Ambient Light Sensor Toggle */
+	{ KE_KEY, 0x7c, { KEY_MICMUTE } },
 	{ KE_KEY, 0x7D, { KEY_BLUETOOTH } }, /* Bluetooth Enable */
 	{ KE_KEY, 0x7E, { KEY_BLUETOOTH } }, /* Bluetooth Disable */
 	{ KE_KEY, 0x82, { KEY_CAMERA } },
@@ -482,7 +483,7 @@ static const struct key_entry asus_nb_wmi_keymap[] = {
 	{ KE_KEY, 0x92, { KEY_SWITCHVIDEOMODE } }, /* SDSP CRT + TV + DVI */
 	{ KE_KEY, 0x93, { KEY_SWITCHVIDEOMODE } }, /* SDSP LCD + CRT + TV + DVI */
 	{ KE_KEY, 0x95, { KEY_MEDIA } },
-	{ KE_KEY, 0x99, { KEY_PHONE } },
+	{ KE_KEY, 0x99, { KEY_PHONE } }, /* Conflicts with fan mode switch */
 	{ KE_KEY, 0xA0, { KEY_SWITCHVIDEOMODE } }, /* SDSP HDMI only */
 	{ KE_KEY, 0xA1, { KEY_SWITCHVIDEOMODE } }, /* SDSP LCD + HDMI */
 	{ KE_KEY, 0xA2, { KEY_SWITCHVIDEOMODE } }, /* SDSP CRT + HDMI */
-- 
2.17.1

[PATCH v3 07/11] platform/x86: asus-wmi: Organize code into sections

[Yurii Pavlovskyi]

The driver has grown pretty big and will grow more, which makes it hard to
navigate and understand. Add uniform comments to the code and ensure that
it is sorted into logical sections.

Signed-off-by: Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com>
---
 drivers/platform/x86/asus-wmi.c | 94 ++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 48 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index f782dac4cbe7..e69e55635afb 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -193,6 +193,8 @@ struct asus_wmi {
 	struct asus_wmi_driver *driver;
 };
 
+/* Input **********************************************************************/
+
 static int asus_wmi_input_init(struct asus_wmi *asus)
 {
 	int err;
@@ -230,6 +232,8 @@ static void asus_wmi_input_exit(struct asus_wmi *asus)
 	asus->inputdev = NULL;
 }
 
+/* WMI ************************************************************************/
+
 static int asus_wmi_evaluate_method_3dw(u32 method_id, u32 arg0, u32 arg1,
 		u32 arg2, u32 *retval)
 {
@@ -248,7 +252,7 @@ static int asus_wmi_evaluate_method_3dw(u32 method_id, u32 arg0, u32 arg1,
 				     &input, &output);
 
 	if (ACPI_FAILURE(status))
-		goto exit;
+		return -EIO;
 
 	obj = (union acpi_object *)output.pointer;
 	if (obj && obj->type == ACPI_TYPE_INTEGER)
@@ -259,10 +263,6 @@ static int asus_wmi_evaluate_method_3dw(u32 method_id, u32 arg0, u32 arg1,
 
 	kfree(obj);
 
-exit:
-	if (ACPI_FAILURE(status))
-		return -EIO;
-
 	if (tmp == ASUS_WMI_UNSUPPORTED_METHOD)
 		return -ENODEV;
 
@@ -346,9 +346,8 @@ static int asus_wmi_get_devstate_simple(struct asus_wmi *asus, u32 dev_id)
 					  ASUS_WMI_DSTS_STATUS_BIT);
 }
 
-/*
- * LEDs
- */
+/* LEDs ***********************************************************************/
+
 /*
  * These functions actually update the LED's, and are called from a
  * workqueue. By doing this as separate work rather than when the LED
@@ -658,6 +657,7 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
 	return rv;
 }
 
+/* RF *************************************************************************/
 
 /*
  * PCI hotplug (for wlan rfkill)
@@ -1080,6 +1080,8 @@ static int asus_wmi_rfkill_init(struct asus_wmi *asus)
 	return result;
 }
 
+/* Quirks *********************************************************************/
+
 static void asus_wmi_set_xusb2pr(struct asus_wmi *asus)
 {
 	struct pci_dev *xhci_pdev;
@@ -1112,9 +1114,8 @@ static void asus_wmi_set_als(void)
 	asus_wmi_set_devstate(ASUS_WMI_DEVID_ALS_ENABLE, 1, NULL);
 }
 
-/*
- * Hwmon device
- */
+/* Hwmon device ***************************************************************/
+
 static int asus_hwmon_agfn_fan_speed_read(struct asus_wmi *asus, int fan,
 					  int *speed)
 {
@@ -1390,7 +1391,6 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
 	else if (attr == &dev_attr_temp1_input.attr)
 		dev_id = ASUS_WMI_DEVID_THERMAL_CTRL;
 
-
 	if (attr == &dev_attr_fan1_input.attr
 	    || attr == &dev_attr_fan1_label.attr
 	    || attr == &dev_attr_pwm1.attr
@@ -1453,9 +1453,27 @@ static int asus_wmi_hwmon_init(struct asus_wmi *asus)
 	return 0;
 }
 
-/*
- * Backlight
- */
+static int asus_wmi_fan_init(struct asus_wmi *asus)
+{
+	int status;
+
+	asus->asus_hwmon_pwm = -1;
+	asus->asus_hwmon_num_fans = -1;
+	asus->asus_hwmon_fan_manual_mode = false;
+
+	status = asus_hwmon_get_fan_number(asus, &asus->asus_hwmon_num_fans);
+	if (status) {
+		asus->asus_hwmon_num_fans = 0;
+		pr_warn("Could not determine number of fans: %d\n", status);
+		return -ENXIO;
+	}
+
+	pr_info("Number of fans: %d\n", asus->asus_hwmon_num_fans);
+	return 0;
+}
+
+/* Backlight ******************************************************************/
+
 static int read_backlight_power(struct asus_wmi *asus)
 {
 	int ret;
@@ -1637,6 +1655,8 @@ static int is_display_toggle(int code)
 	return 0;
 }
 
+/* WMI events *****************************************************************/
+
 static int asus_wmi_get_next_event(u32 value)
 {
 	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
@@ -1762,9 +1782,8 @@ static int asus_wmi_notify_queue_flush(struct asus_wmi *asus)
 	return -EIO;
 }
 
-/*
- * Sys helpers
- */
+/* Sysfs **********************************************************************/
+
 static int parse_arg(const char *buf, unsigned long count, int *val)
 {
 	if (!count)
@@ -1903,9 +1922,8 @@ static int asus_wmi_sysfs_init(struct platform_device *device)
 	return sysfs_create_group(&device->dev.kobj, &platform_attribute_group);
 }
 
-/*
- * Platform device
- */
+/* Platform device ************************************************************/
+
 static int asus_wmi_platform_init(struct asus_wmi *asus)
 {
 	int rv;
@@ -1990,9 +2008,8 @@ static void asus_wmi_platform_exit(struct asus_wmi *asus)
 	asus_wmi_sysfs_exit(asus->platform_device);
 }
 
-/*
- * debugfs
- */
+/* debugfs ********************************************************************/
+
 struct asus_wmi_debugfs_node {
 	struct asus_wmi *asus;
 	char *name;
@@ -2139,28 +2156,8 @@ static int asus_wmi_debugfs_init(struct asus_wmi *asus)
 	return -ENOMEM;
 }
 
-static int asus_wmi_fan_init(struct asus_wmi *asus)
-{
-	int status;
-
-	asus->asus_hwmon_pwm = -1;
-	asus->asus_hwmon_num_fans = -1;
-	asus->asus_hwmon_fan_manual_mode = false;
-
-	status = asus_hwmon_get_fan_number(asus, &asus->asus_hwmon_num_fans);
-	if (status) {
-		asus->asus_hwmon_num_fans = 0;
-		pr_warn("Could not determine number of fans: %d\n", status);
-		return -ENXIO;
-	}
-
-	pr_info("Number of fans: %d\n", asus->asus_hwmon_num_fans);
-	return 0;
-}
+/* Init / exit ****************************************************************/
 
-/*
- * WMI Driver
- */
 static int asus_wmi_add(struct platform_device *pdev)
 {
 	struct platform_driver *pdrv = to_platform_driver(pdev->dev.driver);
@@ -2288,9 +2285,8 @@ static int asus_wmi_remove(struct platform_device *device)
 	return 0;
 }
 
-/*
- * Platform driver - hibernate/resume callbacks
- */
+/* Platform driver - hibernate/resume callbacks *******************************/
+
 static int asus_hotk_thaw(struct device *device)
 {
 	struct asus_wmi *asus = dev_get_drvdata(device);
@@ -2362,6 +2358,8 @@ static const struct dev_pm_ops asus_pm_ops = {
 	.resume = asus_hotk_resume,
 };
 
+/* Registration ***************************************************************/
+
 static int asus_wmi_probe(struct platform_device *pdev)
 {
 	struct platform_driver *pdrv = to_platform_driver(pdev->dev.driver);
-- 
2.17.1

[PATCH v3 08/11] platform/x86: asus-wmi: Enhance detection of thermal data

[Yurii Pavlovskyi]

The obviously wrong value 1 for temperature device ID in this driver is
returned by at least some devices, including TUF Gaming series laptops,
instead of 0 as expected previously. Observable effect is that a
temp1_input in hwmon reads temperature near absolute zero.

* Consider 0.1 K an erroneous value in addition to 0 K.
* Refactor detection of thermal input availability to a separate function.

Signed-off-by: Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com>
---
 drivers/platform/x86/asus-wmi.c | 45 ++++++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index e69e55635afb..1b8272374660 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -178,6 +178,7 @@ struct asus_wmi {
 	struct asus_rfkill gps;
 	struct asus_rfkill uwb;
 
+	bool asus_hwmon_thermal_available;
 	bool asus_hwmon_fan_manual_mode;
 	int asus_hwmon_num_fans;
 	int asus_hwmon_pwm;
@@ -1375,6 +1376,32 @@ static struct attribute *hwmon_attributes[] = {
 	NULL
 };
 
+static int asus_hwmon_check_thermal_available(struct asus_wmi *asus)
+{
+	u32 value = ASUS_WMI_UNSUPPORTED_METHOD;
+	int err;
+
+	asus->asus_hwmon_thermal_available = false;
+	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_THERMAL_CTRL, &value);
+
+	if (err < 0) {
+		if (err == -ENODEV)
+			return 0;
+
+		return err;
+	}
+
+	/*
+	 * If the temperature value in deci-Kelvin is near the absolute
+	 * zero temperature, something is clearly wrong.
+	 */
+	if (!value || value == 1)
+		return 0;
+
+	asus->asus_hwmon_thermal_available = true;
+	return 0;
+}
+
 static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
 					  struct attribute *attr, int idx)
 {
@@ -1388,8 +1415,6 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
 
 	if (attr == &dev_attr_pwm1.attr)
 		dev_id = ASUS_WMI_DEVID_FAN_CTRL;
-	else if (attr == &dev_attr_temp1_input.attr)
-		dev_id = ASUS_WMI_DEVID_THERMAL_CTRL;
 
 	if (attr == &dev_attr_fan1_input.attr
 	    || attr == &dev_attr_fan1_label.attr
@@ -1414,15 +1439,13 @@ static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
 		 * - reverved bits are non-zero
 		 * - sfun and presence bit are not set
 		 */
-		if (value == ASUS_WMI_UNSUPPORTED_METHOD || value & 0xFFF80000
+		if (value == ASUS_WMI_UNSUPPORTED_METHOD || (value & 0xFFF80000)
 		    || (!asus->sfun && !(value & ASUS_WMI_DSTS_PRESENCE_BIT)))
 			ok = false;
 		else
 			ok = fan_attr <= asus->asus_hwmon_num_fans;
-	} else if (dev_id == ASUS_WMI_DEVID_THERMAL_CTRL) {
-		/* If value is zero, something is clearly wrong */
-		if (!value)
-			ok = false;
+	} else if (attr == &dev_attr_temp1_input.attr) {
+		ok = asus->asus_hwmon_thermal_available;
 	} else if (fan_attr <= asus->asus_hwmon_num_fans && fan_attr != -1) {
 		ok = true;
 	} else {
@@ -1469,6 +1492,14 @@ static int asus_wmi_fan_init(struct asus_wmi *asus)
 	}
 
 	pr_info("Number of fans: %d\n", asus->asus_hwmon_num_fans);
+
+	status = asus_hwmon_check_thermal_available(asus);
+	if (status) {
+		pr_warn("Could not check if thermal available: %d\n", status);
+		return -ENXIO;
+	}
+
+	pr_info("Thermal available: %d\n", asus->asus_hwmon_thermal_available);
 	return 0;
 }
 
-- 
2.17.1

[PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight

[Yurii Pavlovskyi]

The WMI exposes two methods for controlling RGB keyboard backlight, which
allows controlling:
* RGB components in range 00 - ff,
* Switch between 4 effects,
* Switch between 3 effect speed modes,
* Separately enable the backlight on boot, in the awake state (after driver
  load), in sleep mode, and probably in something called shutdown mode (no
  observable effects of enabling it are known so far).

The configuration should be written to several sysfs parameter buffers
which are then written via WMI by writing either 1 or 2 to the "kbbl_set"
parameter. When reading the buffers the last written value is returned.

If the 2 is written to "kbbl_set", the parameters will be reset on reboot
(temporary mode), 1 is permanent mode, parameters are retained.

The calls use new 3-dword input buffer method call.

The functionality is only enabled if corresponding DSTS methods return
exact valid values.

The following script demonstrates usage:

echo Red [00 - ff]
echo 33 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_red
echo Green [00 - ff]
echo ff > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_green
echo Blue [00 - ff]
echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_blue
echo Mode: 0 - static color, 1 - breathing, 2 - color cycle, 3 - strobing
echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_mode
echo Speed for modes 1 and 2: 0 - slow, 1 - medium, 2 - fast
echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_speed
echo Enable: 02 - on boot, before module load, 08 - awake, 20 - sleep,
echo 2a or ff to set all
echo 2a > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_flags
echo Save: 1 - permanently, 2 - temporarily, reset after reboot
echo 1 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_set

Signed-off-by: Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com>
---
 .../ABI/testing/sysfs-platform-asus-wmi       |  61 ++++
 drivers/platform/x86/asus-wmi.c               | 331 ++++++++++++++++++
 include/linux/platform_data/x86/asus-wmi.h    |   2 +
 3 files changed, 394 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 019e1e29370e..1cc54d5e3e10 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -36,3 +36,64 @@ KernelVersion:	3.5
 Contact:	"AceLan Kao" <acelan.kao@canonical.com>
 Description:
 		Resume on lid open. 1 means on, 0 means off.
+
+What:		/sys/devices/platform/<platform>/kbbl/kbbl_red
+Date:		Apr 2019
+KernelVersion:	5.1
+Contact:	"Yurii Pavlovskyi" <yurii.pavlovskyi@gmail.com>
+Description:
+		RGB keyboard backlight red component: 00 .. ff.
+
+What:		/sys/devices/platform/<platform>/kbbl/kbbl_green
+Date:		Apr 2019
+KernelVersion:	5.1
+Contact:	"Yurii Pavlovskyi" <yurii.pavlovskyi@gmail.com>
+Description:
+		RGB keyboard backlight green component: 00 .. ff.
+
+What:		/sys/devices/platform/<platform>/kbbl/kbbl_blue
+Date:		Apr 2019
+KernelVersion:	5.1
+Contact:	"Yurii Pavlovskyi" <yurii.pavlovskyi@gmail.com>
+Description:
+		RGB keyboard backlight blue component: 00 .. ff.
+
+What:		/sys/devices/platform/<platform>/kbbl/kbbl_mode
+Date:		Apr 2019
+KernelVersion:	5.1
+Contact:	"Yurii Pavlovskyi" <yurii.pavlovskyi@gmail.com>
+Description:
+		RGB keyboard backlight mode:
+			* 0 - static color,
+			* 1 - breathing,
+			* 2 - color cycle,
+			* 3 - strobing.
+
+What:		/sys/devices/platform/<platform>/kbbl/kbbl_speed
+Date:		Apr 2019
+KernelVersion:	5.1
+Contact:	"Yurii Pavlovskyi" <yurii.pavlovskyi@gmail.com>
+Description:
+		RGB keyboard backlight speed for modes 1 and 2:
+			* 0 - slow,
+			* 1 - medium,
+			* 2 - fast.
+
+What:		/sys/devices/platform/<platform>/kbbl/kbbl_flags
+Date:		Apr 2019
+KernelVersion:	5.1
+Contact:	"Yurii Pavlovskyi" <yurii.pavlovskyi@gmail.com>
+Description:
+		RGB keyboard backlight enable flags (2a to enable everything), OR of:
+			* 02 - on boot (until module load),
+			* 08 - awake,
+			* 20 - sleep.
+
+What:		/sys/devices/platform/<platform>/kbbl/kbbl_set
+Date:		Apr 2019
+KernelVersion:	5.1
+Contact:	"Yurii Pavlovskyi" <yurii.pavlovskyi@gmail.com>
+Description:
+		Write changed RGB keyboard backlight parameters:
+			* 1 - permanently,
+			* 2 - temporarily.
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 1b8272374660..0a32079336d8 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -148,6 +148,21 @@ struct asus_rfkill {
 	u32 dev_id;
 };
 
+struct asus_kbbl_rgb {
+	u8 kbbl_red;
+	u8 kbbl_green;
+	u8 kbbl_blue;
+	u8 kbbl_mode;
+	u8 kbbl_speed;
+
+	u8 kbbl_set_red;
+	u8 kbbl_set_green;
+	u8 kbbl_set_blue;
+	u8 kbbl_set_mode;
+	u8 kbbl_set_speed;
+	u8 kbbl_set_flags;
+};
+
 struct asus_wmi {
 	int dsts_id;
 	int spec;
@@ -183,6 +198,9 @@ struct asus_wmi {
 	int asus_hwmon_num_fans;
 	int asus_hwmon_pwm;
 
+	bool kbbl_rgb_available;
+	struct asus_kbbl_rgb kbbl_rgb;
+
 	struct hotplug_slot hotplug_slot;
 	struct mutex hotplug_lock;
 	struct mutex wmi_lock;
@@ -658,6 +676,312 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
 	return rv;
 }
 
+/* RGB keyboard backlight *****************************************************/
+
+static ssize_t show_u8(u8 value, char *buf)
+{
+	return scnprintf(buf, PAGE_SIZE, "%02x\n", value);
+}
+
+static ssize_t store_u8(u8 *value, const char *buf, int count)
+{
+	int err;
+	u8 result;
+
+	err = kstrtou8(buf, 16, &result);
+	if (err < 0) {
+		pr_warn("Trying to store invalid value\n");
+		return err;
+	}
+
+	*value = result;
+
+	return count;
+}
+
+static ssize_t kbbl_red_show(struct device *dev, struct device_attribute *attr,
+		char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return show_u8(asus->kbbl_rgb.kbbl_red, buf);
+}
+
+static ssize_t kbbl_red_store(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return store_u8(&asus->kbbl_rgb.kbbl_set_red, buf, count);
+}
+
+static ssize_t kbbl_green_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return show_u8(asus->kbbl_rgb.kbbl_green, buf);
+}
+
+static ssize_t kbbl_green_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return store_u8(&asus->kbbl_rgb.kbbl_set_green, buf, count);
+}
+
+static ssize_t kbbl_blue_show(struct device *dev, struct device_attribute *attr,
+		char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return show_u8(asus->kbbl_rgb.kbbl_blue, buf);
+}
+
+static ssize_t kbbl_blue_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return store_u8(&asus->kbbl_rgb.kbbl_set_blue, buf, count);
+}
+
+static ssize_t kbbl_mode_show(struct device *dev, struct device_attribute *attr,
+		char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return show_u8(asus->kbbl_rgb.kbbl_mode, buf);
+}
+
+static ssize_t kbbl_mode_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return store_u8(&asus->kbbl_rgb.kbbl_set_mode, buf, count);
+}
+
+static ssize_t kbbl_speed_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return show_u8(asus->kbbl_rgb.kbbl_speed, buf);
+}
+
+static ssize_t kbbl_speed_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return store_u8(&asus->kbbl_rgb.kbbl_set_speed, buf, count);
+}
+
+static ssize_t kbbl_flags_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return show_u8(asus->kbbl_rgb.kbbl_set_flags, buf);
+}
+
+static ssize_t kbbl_flags_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return store_u8(&asus->kbbl_rgb.kbbl_set_flags, buf, count);
+}
+
+static ssize_t kbbl_set_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return scnprintf(buf, PAGE_SIZE,
+			"Write to configure RGB keyboard backlight\n");
+}
+
+static int kbbl_rgb_write(struct asus_wmi *asus, int persistent)
+{
+	int err;
+	u32 retval;
+	u8 speed_byte;
+	u8 mode_byte;
+	u8 speed;
+	u8 mode;
+
+	speed = asus->kbbl_rgb.kbbl_set_speed;
+	switch (speed) {
+	case 0:
+	default:
+		speed_byte = 0xe1; // slow
+		speed = 0;
+		break;
+	case 1:
+		speed_byte = 0xeb; // medium
+		break;
+	case 2:
+		speed_byte = 0xf5; // fast
+		break;
+	}
+
+	mode = asus->kbbl_rgb.kbbl_set_mode;
+	switch (mode) {
+	case 0:
+	default:
+		mode_byte = 0x00; // static color
+		mode = 0;
+		break;
+	case 1:
+		mode_byte = 0x01; // breathing
+		break;
+	case 2:
+		mode_byte = 0x02; // color cycle
+		break;
+	case 3:
+		mode_byte = 0x0a; // strobing
+		break;
+	}
+
+	err = asus_wmi_evaluate_method_3dw(ASUS_WMI_METHODID_DEVS,
+		ASUS_WMI_DEVID_KBD_RGB,
+		(persistent ? 0xb4 : 0xb3) |
+		(mode_byte << 8) |
+		(asus->kbbl_rgb.kbbl_set_red << 16) |
+		(asus->kbbl_rgb.kbbl_set_green << 24),
+		(asus->kbbl_rgb.kbbl_set_blue) |
+		(speed_byte << 8), &retval);
+	if (err) {
+		pr_warn("RGB keyboard device 1, write error: %d\n", err);
+		return err;
+	}
+
+	if (retval != 1) {
+		pr_warn("RGB keyboard device 1, write error (retval): %x\n",
+				retval);
+		return -EIO;
+	}
+
+	err = asus_wmi_evaluate_method_3dw(ASUS_WMI_METHODID_DEVS,
+		ASUS_WMI_DEVID_KBD_RGB2,
+		(0xbd) |
+		(asus->kbbl_rgb.kbbl_set_flags << 16) |
+		(persistent ? 0x0100 : 0x0000), 0, &retval);
+	if (err) {
+		pr_warn("RGB keyboard device 2, write error: %d\n", err);
+		return err;
+	}
+
+	if (retval != 1) {
+		pr_warn("RGB keyboard device 2, write error (retval): %x\n",
+				retval);
+		return -EIO;
+	}
+
+	asus->kbbl_rgb.kbbl_red = asus->kbbl_rgb.kbbl_set_red;
+	asus->kbbl_rgb.kbbl_green = asus->kbbl_rgb.kbbl_set_green;
+	asus->kbbl_rgb.kbbl_blue = asus->kbbl_rgb.kbbl_set_blue;
+	asus->kbbl_rgb.kbbl_mode = mode;
+	asus->kbbl_rgb.kbbl_speed = speed;
+
+	return 0;
+}
+
+static ssize_t kbbl_set_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	u8 value;
+	struct asus_wmi *asus;
+	int result;
+
+	asus = dev_get_drvdata(dev);
+	result = store_u8(&value, buf, count);
+	if (result < 0)
+		return result;
+
+	if (value == 1)
+		kbbl_rgb_write(asus, 1);
+	else if (value == 2)
+		kbbl_rgb_write(asus, 0);
+
+	return count;
+}
+
+/* RGB values: 00 .. ff */
+static DEVICE_ATTR_RW(kbbl_red);
+static DEVICE_ATTR_RW(kbbl_green);
+static DEVICE_ATTR_RW(kbbl_blue);
+
+/*
+ * Color modes: 0 - static color, 1 - breathing, 2 - color cycle, 3 - strobing
+ */
+static DEVICE_ATTR_RW(kbbl_mode);
+
+/* Speed for modes 1 and 2: 0 - slow, 1 - medium, 2 - fast */
+static DEVICE_ATTR_RW(kbbl_speed);
+
+/*
+ * Enable: 02 - on boot (until module load) | 08 - awake | 20 - sleep
+ * (2a or ff to enable everything)
+ *
+ * Logically 80 would be shutdown, but no visible effects of this option
+ * were observed so far
+ */
+static DEVICE_ATTR_RW(kbbl_flags);
+
+/* Write data: 1 - permanently, 2 - temporarily (reset after reboot) */
+static DEVICE_ATTR_RW(kbbl_set);
+
+static struct attribute *rgbkb_sysfs_attributes[] = {
+	&dev_attr_kbbl_red.attr,
+	&dev_attr_kbbl_green.attr,
+	&dev_attr_kbbl_blue.attr,
+	&dev_attr_kbbl_mode.attr,
+	&dev_attr_kbbl_speed.attr,
+	&dev_attr_kbbl_flags.attr,
+	&dev_attr_kbbl_set.attr,
+	NULL,
+};
+
+static const struct attribute_group kbbl_attribute_group = {
+	.name = "kbbl",
+	.attrs = rgbkb_sysfs_attributes
+};
+
+static int kbbl_rgb_init(struct asus_wmi *asus)
+{
+	int err;
+
+	err = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_KBD_RGB);
+	if (err) {
+		if (err == -ENODEV)
+			return 0;
+		else
+			return err;
+	}
+
+	err = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_KBD_RGB2);
+	if (err) {
+		if (err == -ENODEV)
+			return 0;
+		else
+			return err;
+	}
+
+	asus->kbbl_rgb_available = true;
+	return sysfs_create_group(&asus->platform_device->dev.kobj,
+			&kbbl_attribute_group);
+}
+
+static void kbbl_rgb_exit(struct asus_wmi *asus)
+{
+	if (asus->kbbl_rgb_available) {
+		sysfs_remove_group(&asus->platform_device->dev.kobj,
+				&kbbl_attribute_group);
+	}
+}
+
 /* RF *************************************************************************/
 
 /*
@@ -2230,6 +2554,10 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_leds;
 
+	err = kbbl_rgb_init(asus);
+	if (err)
+		goto fail_rgbkb;
+
 	asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_WLAN, &result);
 	if (result & (ASUS_WMI_DSTS_PRESENCE_BIT | ASUS_WMI_DSTS_USER_BIT))
 		asus->driver->wlan_ctrl_by_user = 1;
@@ -2287,6 +2615,8 @@ static int asus_wmi_add(struct platform_device *pdev)
 fail_backlight:
 	asus_wmi_rfkill_exit(asus);
 fail_rfkill:
+	kbbl_rgb_exit(asus);
+fail_rgbkb:
 	asus_wmi_led_exit(asus);
 fail_leds:
 fail_hwmon:
@@ -2307,6 +2637,7 @@ static int asus_wmi_remove(struct platform_device *device)
 	asus_wmi_backlight_exit(asus);
 	asus_wmi_input_exit(asus);
 	asus_wmi_led_exit(asus);
+	kbbl_rgb_exit(asus);
 	asus_wmi_rfkill_exit(asus);
 	asus_wmi_debugfs_exit(asus);
 	asus_wmi_platform_exit(asus);
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index a5fe7e68944b..c8c6e939e196 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -57,6 +57,8 @@
 #define ASUS_WMI_DEVID_KBD_BACKLIGHT	0x00050021
 #define ASUS_WMI_DEVID_LIGHT_SENSOR	0x00050022 /* ?? */
 #define ASUS_WMI_DEVID_LIGHTBAR		0x00050025
+#define ASUS_WMI_DEVID_KBD_RGB		0x00100056
+#define ASUS_WMI_DEVID_KBD_RGB2		0x00100057
 
 /* Misc */
 #define ASUS_WMI_DEVID_CAMERA		0x00060013
-- 
2.17.1

[PATCH v3 10/11] platform/x86: asus-wmi: Switch fan boost mode

[Yurii Pavlovskyi]

The WMI exposes a write-only device ID where up to three fan modes modes
can be switched on some laptops (TUF Gaming FX505GM). There is a hotkey
combination Fn-F5 that does have a fan icon which is designed to toggle
between fan modes. The DSTS of the device ID returns information about the
presence of the device and the presence of each of the two additional fan
modes as a bitmask (0x01 - overboost present, 0x02 - silent present) [1].

Add a SysFS entry that reads the last written value and updates value in
WMI on write and a hotkey handler that toggles the modes taking into
account their availability according to DSTS.

Modes:
* 0x00 - normal or balanced,
* 0x01 - overboost, increased fan RPM,
* 0x02 - silent, decreased fan RPM

[1] https://lkml.org/lkml/2019/4/12/110

Signed-off-by: Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com>
Suggested-by: Daniel Drake <drake@endlessm.com>
---
 .../ABI/testing/sysfs-platform-asus-wmi       |  10 ++
 drivers/platform/x86/asus-wmi.c               | 149 +++++++++++++++++-
 include/linux/platform_data/x86/asus-wmi.h    |   1 +
 3 files changed, 152 insertions(+), 8 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 1cc54d5e3e10..6f396c4eabdc 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -97,3 +97,13 @@ Description:
 		Write changed RGB keyboard backlight parameters:
 			* 1 - permanently,
 			* 2 - temporarily.
+
+What:		/sys/devices/platform/<platform>/fan_mode
+Date:		Apr 2019
+KernelVersion:	5.1
+Contact:	"Yurii Pavlovskyi" <yurii.pavlovskyi@gmail.com>
+Description:
+		Fan boost mode:
+			* 0 - normal,
+			* 1 - overboost,
+			* 2 - silent
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 0a32079336d8..7974283b7b12 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -69,6 +69,7 @@ MODULE_LICENSE("GPL");
 #define NOTIFY_KBD_BRTUP		0xc4
 #define NOTIFY_KBD_BRTDWN		0xc5
 #define NOTIFY_KBD_BRTTOGGLE		0xc7
+#define NOTIFY_KBD_FBM			0x99
 
 #define ASUS_FAN_DESC			"cpu_fan"
 #define ASUS_FAN_MFUN			0x13
@@ -77,6 +78,13 @@ MODULE_LICENSE("GPL");
 #define ASUS_FAN_CTRL_MANUAL		1
 #define ASUS_FAN_CTRL_AUTO		2
 
+#define ASUS_FAN_MODE_NORMAL		0
+#define ASUS_FAN_MODE_OVERBOOST		1
+#define ASUS_FAN_MODE_OVERBOOST_MASK	0x01
+#define ASUS_FAN_MODE_SILENT		2
+#define ASUS_FAN_MODE_SILENT_MASK	0x02
+#define ASUS_FAN_MODES_MASK		0x03
+
 #define USB_INTEL_XUSB2PR		0xD0
 #define PCI_DEVICE_ID_INTEL_LYNXPOINT_LP_XHCI	0x9c31
 
@@ -198,6 +206,10 @@ struct asus_wmi {
 	int asus_hwmon_num_fans;
 	int asus_hwmon_pwm;
 
+	bool fan_mode_available;
+	u8 fan_mode_mask;
+	u8 fan_mode;
+
 	bool kbbl_rgb_available;
 	struct asus_kbbl_rgb kbbl_rgb;
 
@@ -1827,6 +1839,114 @@ static int asus_wmi_fan_init(struct asus_wmi *asus)
 	return 0;
 }
 
+/* Fan mode *******************************************************************/
+
+static int fan_mode_check_present(struct asus_wmi *asus)
+{
+	u32 result;
+	int err;
+
+	asus->fan_mode_available = false;
+
+	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_FAN_MODE, &result);
+	if (err) {
+		if (err == -ENODEV)
+			return 0;
+		else
+			return err;
+	}
+
+	if ((result & ASUS_WMI_DSTS_PRESENCE_BIT) &&
+			(result & ASUS_FAN_MODES_MASK)) {
+		asus->fan_mode_available = true;
+		asus->fan_mode_mask = result & ASUS_FAN_MODES_MASK;
+	}
+
+	return 0;
+}
+
+static int fan_mode_write(struct asus_wmi *asus)
+{
+	int err;
+	u8 value;
+	u32 retval;
+
+	value = asus->fan_mode;
+
+	pr_info("Set fan mode: %u\n", value);
+	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_FAN_MODE, value, &retval);
+
+	if (err) {
+		pr_warn("Failed to set fan mode: %d\n", err);
+		return err;
+	}
+
+	if (retval != 1) {
+		pr_warn("Failed to set fan mode (retval): 0x%x\n", retval);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int fan_mode_switch_next(struct asus_wmi *asus)
+{
+	if (asus->fan_mode == ASUS_FAN_MODE_NORMAL) {
+		if (asus->fan_mode_mask & ASUS_FAN_MODE_OVERBOOST_MASK)
+			asus->fan_mode = ASUS_FAN_MODE_OVERBOOST;
+		else if (asus->fan_mode_mask & ASUS_FAN_MODE_SILENT_MASK)
+			asus->fan_mode = ASUS_FAN_MODE_SILENT;
+	} else if (asus->fan_mode == ASUS_FAN_MODE_OVERBOOST) {
+		if (asus->fan_mode_mask & ASUS_FAN_MODE_SILENT_MASK)
+			asus->fan_mode = ASUS_FAN_MODE_SILENT;
+		else
+			asus->fan_mode = ASUS_FAN_MODE_NORMAL;
+	} else {
+		asus->fan_mode = ASUS_FAN_MODE_NORMAL;
+	}
+
+	return fan_mode_write(asus);
+}
+
+static ssize_t fan_mode_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	return show_u8(asus->fan_mode, buf);
+}
+
+static ssize_t fan_mode_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	int result;
+	u8 new_mode;
+
+	struct asus_wmi *asus = dev_get_drvdata(dev);
+
+	result = store_u8(&new_mode, buf, count);
+	if (result < 0)
+		return result;
+
+	if (new_mode == ASUS_FAN_MODE_OVERBOOST) {
+		if (!(asus->fan_mode_mask & ASUS_FAN_MODE_OVERBOOST_MASK))
+			return -EINVAL;
+	} else if (new_mode == ASUS_FAN_MODE_SILENT) {
+		if (!(asus->fan_mode_mask & ASUS_FAN_MODE_SILENT_MASK))
+			return -EINVAL;
+	} else if (new_mode != ASUS_FAN_MODE_NORMAL) {
+		return -EINVAL;
+	}
+
+	asus->fan_mode = new_mode;
+	fan_mode_write(asus);
+
+	return result;
+}
+
+// Fan mode: 0 - normal, 1 - overboost, 2 - silent
+static DEVICE_ATTR_RW(fan_mode);
+
 /* Backlight ******************************************************************/
 
 static int read_backlight_power(struct asus_wmi *asus)
@@ -2078,6 +2198,11 @@ static void asus_wmi_handle_notify_code(int code, struct asus_wmi *asus)
 		return;
 	}
 
+	if (asus->fan_mode_available && code == NOTIFY_KBD_FBM) {
+		fan_mode_switch_next(asus);
+		return;
+	}
+
 	if (is_display_toggle(code) && asus->driver->quirks->no_display_toggle)
 		return;
 
@@ -2234,6 +2359,7 @@ static struct attribute *platform_attributes[] = {
 	&dev_attr_touchpad.attr,
 	&dev_attr_lid_resume.attr,
 	&dev_attr_als_enable.attr,
+	&dev_attr_fan_mode.attr,
 	NULL
 };
 
@@ -2255,6 +2381,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
 		devid = ASUS_WMI_DEVID_LID_RESUME;
 	else if (attr == &dev_attr_als_enable.attr)
 		devid = ASUS_WMI_DEVID_ALS_ENABLE;
+	else if (attr == &dev_attr_fan_mode.attr)
+		ok = asus->fan_mode_available;
 
 	if (devid != -1)
 		ok = !(asus_wmi_get_devstate_simple(asus, devid) < 0);
@@ -2355,12 +2483,7 @@ static int asus_wmi_platform_init(struct asus_wmi *asus)
 		asus_wmi_set_devstate(ASUS_WMI_DEVID_CWAP,
 				      asus->driver->quirks->wapf, NULL);
 
-	return asus_wmi_sysfs_init(asus->platform_device);
-}
-
-static void asus_wmi_platform_exit(struct asus_wmi *asus)
-{
-	asus_wmi_sysfs_exit(asus->platform_device);
+	return 0;
 }
 
 /* debugfs ********************************************************************/
@@ -2539,6 +2662,14 @@ static int asus_wmi_add(struct platform_device *pdev)
 	if (err)
 		goto fail_platform;
 
+	err = fan_mode_check_present(asus);
+	if (err)
+		goto fail_fan_mode;
+
+	err = asus_wmi_sysfs_init(asus->platform_device);
+	if (err)
+		goto fail_sysfs;
+
 	err = asus_wmi_input_init(asus);
 	if (err)
 		goto fail_input;
@@ -2622,7 +2753,9 @@ static int asus_wmi_add(struct platform_device *pdev)
 fail_hwmon:
 	asus_wmi_input_exit(asus);
 fail_input:
-	asus_wmi_platform_exit(asus);
+	asus_wmi_sysfs_exit(asus->platform_device);
+fail_sysfs:
+fail_fan_mode:
 fail_platform:
 	kfree(asus);
 	return err;
@@ -2640,7 +2773,7 @@ static int asus_wmi_remove(struct platform_device *device)
 	kbbl_rgb_exit(asus);
 	asus_wmi_rfkill_exit(asus);
 	asus_wmi_debugfs_exit(asus);
-	asus_wmi_platform_exit(asus);
+	asus_wmi_sysfs_exit(asus->platform_device);
 	asus_hwmon_fan_set_auto(asus);
 
 	kfree(asus);
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index c8c6e939e196..fdf5839f64ad 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -59,6 +59,7 @@
 #define ASUS_WMI_DEVID_LIGHTBAR		0x00050025
 #define ASUS_WMI_DEVID_KBD_RGB		0x00100056
 #define ASUS_WMI_DEVID_KBD_RGB2		0x00100057
+#define ASUS_WMI_DEVID_FAN_MODE		0x00110018
 
 /* Misc */
 #define ASUS_WMI_DEVID_CAMERA		0x00060013
-- 
2.17.1

[PATCH v3 11/11] platform/x86: asus-wmi: Do not disable keyboard backlight on unloading

[Yurii Pavlovskyi]

The keyboard backlight is automatically disabled when the module is
unloaded as it is exposed as a ledclass device. Change this behavior to
ignore setting brightness when the device is in unloading state.

Signed-off-by: Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com>
---
 drivers/platform/x86/asus-wmi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 7974283b7b12..02c9639bf54f 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -483,6 +483,10 @@ static void do_kbd_led_set(struct led_classdev *led_cdev, int value)
 static void kbd_led_set(struct led_classdev *led_cdev,
 			enum led_brightness value)
 {
+	/* Prevent disabling keyboard backlight on module unregister */
+	if (led_cdev->flags & LED_UNREGISTERING)
+		return;
+
 	do_kbd_led_set(led_cdev, value);
 }
 
-- 
2.17.1

RE: [PATCH v3 08/11] platform/x86: asus-wmi: Enhance detection of thermal data

[Pawnikar, Sumeet R]

>-----Original Message-----
>From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-
>x86-owner@vger.kernel.org] On Behalf Of Yurii Pavlovskyi
>Sent: Friday, April 19, 2019 3:43 PM
>Cc: Corentin Chary <corentin.chary@gmail.com>; Darren Hart
><dvhart@infradead.org>; Andy Shevchenko <andy@infradead.org>; Daniel
>Drake <drake@endlessm.com>; acpi4asus-user@lists.sourceforge.net;
>platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: [PATCH v3 08/11] platform/x86: asus-wmi: Enhance detection of
>thermal data
>
>The obviously wrong value 1 for temperature device ID in this driver is
>returned by at least some devices, including TUF Gaming series laptops,
>instead of 0 as expected previously. Observable effect is that a temp1_input
>in hwmon reads temperature near absolute zero.
>
>* Consider 0.1 K an erroneous value in addition to 0 K.
>* Refactor detection of thermal input availability to a separate function.
>
>Signed-off-by: Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com>
>---
> drivers/platform/x86/asus-wmi.c | 45 ++++++++++++++++++++++++++++-----
> 1 file changed, 38 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-
>wmi.c index e69e55635afb..1b8272374660 100644
>--- a/drivers/platform/x86/asus-wmi.c
>+++ b/drivers/platform/x86/asus-wmi.c
>@@ -178,6 +178,7 @@ struct asus_wmi {
> 	struct asus_rfkill gps;
> 	struct asus_rfkill uwb;
>
>+	bool asus_hwmon_thermal_available;
> 	bool asus_hwmon_fan_manual_mode;
> 	int asus_hwmon_num_fans;
> 	int asus_hwmon_pwm;
>@@ -1375,6 +1376,32 @@ static struct attribute *hwmon_attributes[] = {
> 	NULL
> };
>
>+static int asus_hwmon_check_thermal_available(struct asus_wmi *asus) {
>+	u32 value = ASUS_WMI_UNSUPPORTED_METHOD;
>+	int err;
>+
>+	asus->asus_hwmon_thermal_available = false;
>+	err = asus_wmi_get_devstate(asus,
>ASUS_WMI_DEVID_THERMAL_CTRL,
>+&value);
>+
>+	if (err < 0) {
>+		if (err == -ENODEV)
>+			return 0;
>+
>+		return err;
>+	}
>+
>+	/*
>+	 * If the temperature value in deci-Kelvin is near the absolute
>+	 * zero temperature, something is clearly wrong.
>+	 */
>+	if (!value || value == 1)
>+		return 0;
Do you still need to return 0 in case of wrong/failure case ? 
Shouldn't you return error here ? 

>+
>+	asus->asus_hwmon_thermal_available = true;
>+	return 0;
>+}
>+
> static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
> 					  struct attribute *attr, int idx)  { @@ -
>1388,8 +1415,6 @@ static umode_t asus_hwmon_sysfs_is_visible(struct
>kobject *kobj,
>
> 	if (attr == &dev_attr_pwm1.attr)
> 		dev_id = ASUS_WMI_DEVID_FAN_CTRL;
>-	else if (attr == &dev_attr_temp1_input.attr)
>-		dev_id = ASUS_WMI_DEVID_THERMAL_CTRL;
>
> 	if (attr == &dev_attr_fan1_input.attr
> 	    || attr == &dev_attr_fan1_label.attr @@ -1414,15 +1439,13 @@
>static umode_t asus_hwmon_sysfs_is_visible(struct kobject *kobj,
> 		 * - reverved bits are non-zero
> 		 * - sfun and presence bit are not set
> 		 */
>-		if (value == ASUS_WMI_UNSUPPORTED_METHOD || value &
>0xFFF80000
>+		if (value == ASUS_WMI_UNSUPPORTED_METHOD || (value &
>0xFFF80000)
> 		    || (!asus->sfun && !(value &
>ASUS_WMI_DSTS_PRESENCE_BIT)))
> 			ok = false;
> 		else
> 			ok = fan_attr <= asus->asus_hwmon_num_fans;
>-	} else if (dev_id == ASUS_WMI_DEVID_THERMAL_CTRL) {
>-		/* If value is zero, something is clearly wrong */
>-		if (!value)
>-			ok = false;
>+	} else if (attr == &dev_attr_temp1_input.attr) {
>+		ok = asus->asus_hwmon_thermal_available;
> 	} else if (fan_attr <= asus->asus_hwmon_num_fans && fan_attr != -1)
>{
> 		ok = true;
> 	} else {
>@@ -1469,6 +1492,14 @@ static int asus_wmi_fan_init(struct asus_wmi
>*asus)
> 	}
>
> 	pr_info("Number of fans: %d\n", asus->asus_hwmon_num_fans);
>+
>+	status = asus_hwmon_check_thermal_available(asus);
>+	if (status) {
>+		pr_warn("Could not check if thermal available: %d\n", status);
>+		return -ENXIO;
>+	}
>+
>+	pr_info("Thermal available: %d\n",
>+asus->asus_hwmon_thermal_available);
> 	return 0;
> }
>
>--
>2.17.1

Re: [PATCH v3 08/11] platform/x86: asus-wmi: Enhance detection of thermal data

[Yurii Pavlovskyi]

On 24.04.19 20:25, Pawnikar, Sumeet R wrote:
>> +	/*
>> +	 * If the temperature value in deci-Kelvin is near the absolute
>> +	 * zero temperature, something is clearly wrong.
>> +	 */
>> +	if (!value || value == 1)
>> +		return 0;
> Do you still need to return 0 in case of wrong/failure case ? 
> Shouldn't you return error here ? 

Yes, I think 0 is right there, it's not an error condition in the check
itself (such as IO or memory error). The function returns two values: check
result in asus->asus_hwmon_thermal_available and error code.

This is still a successful negative result. The 0 or 1 (as added in this
patch) there merely indicates that the temperature measurement is not
available this way. It is safe to assume that no device that does provide
temperature will return 0.1 K. The temperature measurement is not critical
for driver operation so the driver proceeds without it.

One could return ENODEV when the check result is negative, but that would
just move the actual check and assignment further to asus_wmi_fan_init.

This is how it might look like in DSDT in this case:
Method (TMPR, 0, NotSerialized)
{
    Return (One)
}
..
If ((IIA0 == 0x00110011))
{
    Return ((TMPR () & 0xFFFF))
}

This is indeed the right method ID but it's nothing there.

Regards,
Yurii

Re: [PATCH v3 01/11] platform/x86: asus-wmi: Fix hwmon device cleanup

[Andy Shevchenko]

On Fri, Apr 19, 2019 at 1:00 PM Yurii Pavlovskyi
<yurii.pavlovskyi@gmail.com> wrote:
>
> The driver does not clean up the hwmon device on exit or error. To
> reproduce the bug, repeat rmmod, insmod to verify that device number
> /sys/devices/platform/asus-nb-wmi/hwmon/hwmon?? grows every time. Replace
> call for registering device with devm_* version that unregisters it
> automatically.

>         struct device *hwmon;
>
> -       hwmon = hwmon_device_register_with_groups(&asus->platform_device->dev,
> -                                                 "asus", asus,
> -                                                 hwmon_attribute_groups);
> +       hwmon = devm_hwmon_device_register_with_groups(
> +                       &asus->platform_device->dev, "asus", asus,
> +                       hwmon_attribute_groups);
> +

Temporary variable would help with readability, i.e.

struct device *dev = &asus->platform_device->dev;
...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 00/11] asus-wmi: Support of ASUS TUF Gaming series laptops

[Andy Shevchenko]

On Fri, Apr 19, 2019 at 12:57 PM Yurii Pavlovskyi
<yurii.pavlovskyi@gmail.com> wrote:
>
> Hi,
>
> this is the third version of the patch series.

Doesn't apply to the for-next branch.
Individual comments on the patches are coming as well.

>
> Changelog:
> v3:
>   * Use devm_* function in patch 01
>   * Detect DSTS/DCTS using _UID in patch 04
>   * Detect event queue by _UID as well in patch 05
>   * Rename poll function in patch 05
>   * Fix terminology in patches 09 and 10
>   * Correct commit messages
> v2:
>   * Fix logging
>
> INTRODUCTION
> The support for this laptop series is currently non-existent, as the
> asus-nb-wmi driver (which is essentially configuration for asus-wmi) fails
> to load and multiple ACPI errors are logged in dmesg. This patch series
> adds pretty comprehensive support for these relatively new laptops, adds
> some code organization, and fixes a couple of bugs in the asus-wmi module.
>
> Original message from V1/V2:
> https://lkml.org/lkml/2019/4/10/973
>
> It is really long, so I will not copy it completely here, please refer
> to the original for notes on design decisions and existing minor issues
> (other than quirks, which should be hopefully solved now).
>
> Yurii Pavlovskyi (11):
>   platform/x86: asus-wmi: Fix hwmon device cleanup
>   platform/x86: asus-wmi: Fix preserving keyboard backlight intensity on
>     load
>   platform/x86: asus-wmi: Increase the input buffer size of WMI methods
>   platform/x86: asus-wmi: Improve DSTS WMI method ID detection
>   platform/x86: asus-wmi: Support WMI event queue
>   platform/x86: asus-nb-wmi: Add microphone mute key code
>   platform/x86: asus-wmi: Organize code into sections
>   platform/x86: asus-wmi: Enhance detection of thermal data
>   platform/x86: asus-wmi: Control RGB keyboard backlight
>   platform/x86: asus-wmi: Switch fan boost mode
>   platform/x86: asus-wmi: Do not disable keyboard backlight on unloading
>
>  .../ABI/testing/sysfs-platform-asus-wmi       |  71 ++
>  drivers/platform/x86/asus-nb-wmi.c            |   3 +-
>  drivers/platform/x86/asus-wmi.c               | 797 +++++++++++++++---
>  drivers/platform/x86/wmi.c                    |  19 +
>  include/linux/acpi.h                          |   1 +
>  include/linux/platform_data/x86/asus-wmi.h    |   7 +-
>  6 files changed, 797 insertions(+), 101 deletions(-)
>
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 03/11] platform/x86: asus-wmi: Increase the input buffer size of WMI methods

[Andy Shevchenko]

On Fri, Apr 19, 2019 at 1:07 PM Yurii Pavlovskyi
<yurii.pavlovskyi@gmail.com> wrote:
>
> The asus-nb-wmi driver is matched by WMI alias but fails to load on TUF
> Gaming series laptops producing multiple ACPI errors in the kernel log.
>
> The input buffer for WMI method invocation size is 2 dwords, whereas
> 3 are expected by this model.
>
> FX505GM:
> ..
> Method (WMNB, 3, Serialized)
> {
>     P8XH (Zero, 0x11)
>     CreateDWordField (Arg2, Zero, IIA0)
>     CreateDWordField (Arg2, 0x04, IIA1)
>     CreateDWordField (Arg2, 0x08, IIA2)
>     Local0 = (Arg1 & 0xFFFFFFFF)
>     ...
>
> Compare with older K54C:
> ...
> Method (WMNB, 3, NotSerialized)
> {
>     CreateDWordField (Arg2, 0x00, IIA0)
>     CreateDWordField (Arg2, 0x04, IIA1)
>     Local0 = (Arg1 & 0xFFFFFFFF)
>     ...
>
> Increase buffer size to 3 dwords. No negative consequences of this change
> are expected, as the input buffer size is not verified. The original
> function is replaced by a wrapper for a new method passing value 0 for the
> last parameter. The new function will be used to control RGB keyboard
> backlight.


> -int asus_wmi_evaluate_method(u32 method_id, u32 arg0, u32 arg1, u32 *retval)
> +static int asus_wmi_evaluate_method_3dw(u32 method_id, u32 arg0, u32 arg1,
> +               u32 arg2, u32 *retval)

I would name as "..._method3" and move arg2 to previous line

>  {
>         struct bios_args args = {
>                 .arg0 = arg0,
>                 .arg1 = arg1,
> +               .arg2 = arg2

Keep comma, it will help in the future, like above helped you here.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 04/11] platform/x86: asus-wmi: Improve DSTS WMI method ID detection

[Andy Shevchenko]

On Fri, Apr 19, 2019 at 1:08 PM Yurii Pavlovskyi
<yurii.pavlovskyi@gmail.com> wrote:
>
> The DSTS method detection mistakenly selects DCTS instead of DSTS if
> nothing is returned when the method ID is not defined in WMNB. As a result,
> the control of keyboard backlight is not functional for TUF Gaming series
> laptops. Implement another detection method instead.
>
> There is evidence that DCTS is handled by ACPI WMI devices that have _UID
> ASUSWMI, whereas none of the devices without ASUSWMI respond to DCTS and
> DSTS is used instead [1]. To check the _UID a new method is added to wmi.h
> / wmi.c. It returns _UID of the ACPI WMI device that declares WMI object
> with given GUID.
>
> Generally, it is possible that multiple PNP0C14 ACPI devices are present in
> the system as mentioned in the commit message of commit bff431e49ff5
> ("ACPI: WMI: Add ACPI-WMI mapping driver").
>
> Therefore the _UID is checked for given GUID that maps to a specific ACPI
> device, to which it is also mapped by other methods of wmi module.
>
> DSDT examples:
>
> FX505GM:
> Method (WMNB, 3, Serialized)
> { ...
>     If ((Local0 == 0x53545344))
>     {
>         ...
>         Return (Zero)
>     }
>     ...
>     // No return
> }
>
> K54C:
> Method (WMNB, 3, Serialized)
> { ...
>     If ((Local0 == 0x53545344))
>     {
>         ...
>         Return (0x02)
>     }
>     ...
>     Return (0xFFFFFFFE)
> }
>

> [1] https://lkml.org/lkml/2019/4/11/322

Link: ...?

>         int rv;
> +       char *wmi_uid;

Keep them in reversed spruce order.



> +       if (!strcmp(wmi_uid, ASUS_ACPI_UID_ASUSWMI)) {

> +               pr_info("Detected ASUSWMI, use DCTS\n");

dev_info()?

>                 asus->dsts_id = ASUS_WMI_METHODID_DSTS;
> -       else
> +       } else {

> +               pr_info("Detected %s, not ASUSWMI, use DSTS\n", wmi_uid);

Ditto.

> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c

> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h

This change should go separately.


> -#define ASUS_WMI_METHODID_DSTS         0x53544344 /* Device STatuS */
> -#define ASUS_WMI_METHODID_DSTS2                0x53545344 /* Device STatuS #2*/

> +#define ASUS_WMI_METHODID_DSTS         0x53544344 /* Device STatuS (DCTS) */

It's not clear from the description what 'C' stands for.
Is there any specification which describes the difference and actual
abbreviations?

> +#define ASUS_WMI_METHODID_DSTS2                0x53545344 /* Device STatuS (DSTS) */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 07/11] platform/x86: asus-wmi: Organize code into sections

[Andy Shevchenko]

On Fri, Apr 19, 2019 at 1:12 PM Yurii Pavlovskyi
<yurii.pavlovskyi@gmail.com> wrote:
>
> The driver has grown pretty big and will grow more, which makes it hard to
> navigate and understand. Add uniform comments to the code and ensure that
> it is sorted into logical sections.

It does slightly more than described. Please, split out to the
separate patch(es) what is not suit to above description.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 05/11] platform/x86: asus-wmi: Support WMI event queue

[Andy Shevchenko]

On Fri, Apr 19, 2019 at 1:10 PM Yurii Pavlovskyi
<yurii.pavlovskyi@gmail.com> wrote:
>
> Event codes are expected to be retrieved from a queue on at least some
> models. Specifically, very likely the ACPI WMI devices with _UID ATK are
> queued whereas those with ASUSWMI are not [1].
>
> The WMI event codes are pushed into a circular buffer queue. After the INIT
> method is called, ACPI code is allowed to push events into this buffer.
> The INIT method cannot be reverted. If the module is unloaded and an event
> (such as hotkey press) gets emitted before inserting it back the events get
> processed delayed by one or if the queue overflows, additionally delayed by
> about 3 seconds.
>
> It might be considered a minor issue and no normal user would likely
> observe this (there is little reason unloading the driver), but it does
> significantly frustrate a developer who is unlucky enough to encounter
> this. Therefore, the fallback to unqueued behavior occurs whenever
> something unexpected happens.
>
> The fix flushes the old key codes out of the queue on load. After receiving
> event the queue is read until either ..FFFF or 1 is encountered. Also as
> noted in [1] it is checked whether notify code is equal to 0xFF before
> enabling queue processing in WMI notify handler.

It's rather a big change. Can it be split to smaller pieces?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/11] platform/x86: asus-wmi: Enhance detection of thermal data

[Andy Shevchenko]

On Fri, Apr 19, 2019 at 1:12 PM Yurii Pavlovskyi
<yurii.pavlovskyi@gmail.com> wrote:
>
> The obviously wrong value 1 for temperature device ID in this driver is
> returned by at least some devices, including TUF Gaming series laptops,
> instead of 0 as expected previously. Observable effect is that a
> temp1_input in hwmon reads temperature near absolute zero.
>
> * Consider 0.1 K an erroneous value in addition to 0 K.
> * Refactor detection of thermal input availability to a separate function.


> +static int asus_hwmon_check_thermal_available(struct asus_wmi *asus)
> +{
> +       u32 value = ASUS_WMI_UNSUPPORTED_METHOD;
> +       int err;
> +
> +       asus->asus_hwmon_thermal_available = false;

> +       err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_THERMAL_CTRL, &value);

> +

The blank line probably should go before a call, but definitely not here.

> +       if (err < 0) {

This needs comment why -ENODEV considered as non-fatal.
> +               if (err == -ENODEV)
> +                       return 0;

> +
> +               return err;
> +       }

> +
> +       /*
> +        * If the temperature value in deci-Kelvin is near the absolute
> +        * zero temperature, something is clearly wrong.
> +        */
> +       if (!value || value == 1)
> +               return 0;
> +
> +       asus->asus_hwmon_thermal_available = true;
> +       return 0;
> +}

>         if (attr == &dev_attr_pwm1.attr)
>                 dev_id = ASUS_WMI_DEVID_FAN_CTRL;

> -       else if (attr == &dev_attr_temp1_input.attr)
> -               dev_id = ASUS_WMI_DEVID_THERMAL_CTRL;

I don't see how this change affects the user output or driver
behaviour. Why is it done?

> -               if (value == ASUS_WMI_UNSUPPORTED_METHOD || value & 0xFFF80000
> +               if (value == ASUS_WMI_UNSUPPORTED_METHOD || (value & 0xFFF80000)

Seems like a bug fix and thus should be a separate commit predecessing
the series.

> -       } else if (dev_id == ASUS_WMI_DEVID_THERMAL_CTRL) {

> +       } else if (attr == &dev_attr_temp1_input.attr) {

So, I don't see why you change this line.

> +               pr_warn("Could not check if thermal available: %d\n", status);

> +       pr_info("Thermal available: %d\n", asus->asus_hwmon_thermal_available);

dev_*() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight

[Andy Shevchenko]

On Fri, Apr 19, 2019 at 1:14 PM Yurii Pavlovskyi
<yurii.pavlovskyi@gmail.com> wrote:
>
> The WMI exposes two methods for controlling RGB keyboard backlight, which
> allows controlling:
> * RGB components in range 00 - ff,
> * Switch between 4 effects,
> * Switch between 3 effect speed modes,
> * Separately enable the backlight on boot, in the awake state (after driver
>   load), in sleep mode, and probably in something called shutdown mode (no
>   observable effects of enabling it are known so far).
>
> The configuration should be written to several sysfs parameter buffers
> which are then written via WMI by writing either 1 or 2 to the "kbbl_set"
> parameter. When reading the buffers the last written value is returned.
>
> If the 2 is written to "kbbl_set", the parameters will be reset on reboot
> (temporary mode), 1 is permanent mode, parameters are retained.
>
> The calls use new 3-dword input buffer method call.
>
> The functionality is only enabled if corresponding DSTS methods return
> exact valid values.
>
> The following script demonstrates usage:
>
> echo Red [00 - ff]
> echo 33 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_red
> echo Green [00 - ff]
> echo ff > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_green
> echo Blue [00 - ff]
> echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_blue
> echo Mode: 0 - static color, 1 - breathing, 2 - color cycle, 3 - strobing
> echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_mode
> echo Speed for modes 1 and 2: 0 - slow, 1 - medium, 2 - fast
> echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_speed
> echo Enable: 02 - on boot, before module load, 08 - awake, 20 - sleep,
> echo 2a or ff to set all
> echo 2a > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_flags
> echo Save: 1 - permanently, 2 - temporarily, reset after reboot
> echo 1 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_set
>

Shouldn't be the LED subsystem driver for this?

> Signed-off-by: Yurii Pavlovskyi <yurii.pavlovskyi@gmail.com>
> ---
>  .../ABI/testing/sysfs-platform-asus-wmi       |  61 ++++
>  drivers/platform/x86/asus-wmi.c               | 331 ++++++++++++++++++
>  include/linux/platform_data/x86/asus-wmi.h    |   2 +
>  3 files changed, 394 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index 019e1e29370e..1cc54d5e3e10 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -36,3 +36,64 @@ KernelVersion:       3.5
>  Contact:       "AceLan Kao" <acelan.kao@canonical.com>
>  Description:
>                 Resume on lid open. 1 means on, 0 means off.
> +
> +What:          /sys/devices/platform/<platform>/kbbl/kbbl_red
> +Date:          Apr 2019
> +KernelVersion: 5.1
> +Contact:       "Yurii Pavlovskyi" <yurii.pavlovskyi@gmail.com>
> +Description:
> +               RGB keyboard backlight red component: 00 .. ff.
> +
> +What:          /sys/devices/platform/<platform>/kbbl/kbbl_green
> +Date:          Apr 2019
> +KernelVersion: 5.1
> +Contact:       "Yurii Pavlovskyi" <yurii.pavlovskyi@gmail.com>
> +Description:
> +               RGB keyboard backlight green component: 00 .. ff.
> +
> +What:          /sys/devices/platform/<platform>/kbbl/kbbl_blue
> +Date:          Apr 2019
> +KernelVersion: 5.1
> +Contact:       "Yurii Pavlovskyi" <yurii.pavlovskyi@gmail.com>
> +Description:
> +               RGB keyboard backlight blue component: 00 .. ff.
> +
> +What:          /sys/devices/platform/<platform>/kbbl/kbbl_mode
> +Date:          Apr 2019
> +KernelVersion: 5.1
> +Contact:       "Yurii Pavlovskyi" <yurii.pavlovskyi@gmail.com>
> +Description:
> +               RGB keyboard backlight mode:
> +                       * 0 - static color,
> +                       * 1 - breathing,
> +                       * 2 - color cycle,
> +                       * 3 - strobing.
> +
> +What:          /sys/devices/platform/<platform>/kbbl/kbbl_speed
> +Date:          Apr 2019
> +KernelVersion: 5.1
> +Contact:       "Yurii Pavlovskyi" <yurii.pavlovskyi@gmail.com>
> +Description:
> +               RGB keyboard backlight speed for modes 1 and 2:
> +                       * 0 - slow,
> +                       * 1 - medium,
> +                       * 2 - fast.
> +
> +What:          /sys/devices/platform/<platform>/kbbl/kbbl_flags
> +Date:          Apr 2019
> +KernelVersion: 5.1
> +Contact:       "Yurii Pavlovskyi" <yurii.pavlovskyi@gmail.com>
> +Description:
> +               RGB keyboard backlight enable flags (2a to enable everything), OR of:
> +                       * 02 - on boot (until module load),
> +                       * 08 - awake,
> +                       * 20 - sleep.
> +
> +What:          /sys/devices/platform/<platform>/kbbl/kbbl_set
> +Date:          Apr 2019
> +KernelVersion: 5.1
> +Contact:       "Yurii Pavlovskyi" <yurii.pavlovskyi@gmail.com>
> +Description:
> +               Write changed RGB keyboard backlight parameters:
> +                       * 1 - permanently,
> +                       * 2 - temporarily.
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 1b8272374660..0a32079336d8 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -148,6 +148,21 @@ struct asus_rfkill {
>         u32 dev_id;
>  };
>
> +struct asus_kbbl_rgb {
> +       u8 kbbl_red;
> +       u8 kbbl_green;
> +       u8 kbbl_blue;
> +       u8 kbbl_mode;
> +       u8 kbbl_speed;
> +
> +       u8 kbbl_set_red;
> +       u8 kbbl_set_green;
> +       u8 kbbl_set_blue;
> +       u8 kbbl_set_mode;
> +       u8 kbbl_set_speed;
> +       u8 kbbl_set_flags;
> +};
> +
>  struct asus_wmi {
>         int dsts_id;
>         int spec;
> @@ -183,6 +198,9 @@ struct asus_wmi {
>         int asus_hwmon_num_fans;
>         int asus_hwmon_pwm;
>
> +       bool kbbl_rgb_available;
> +       struct asus_kbbl_rgb kbbl_rgb;
> +
>         struct hotplug_slot hotplug_slot;
>         struct mutex hotplug_lock;
>         struct mutex wmi_lock;
> @@ -658,6 +676,312 @@ static int asus_wmi_led_init(struct asus_wmi *asus)
>         return rv;
>  }
>
> +/* RGB keyboard backlight *****************************************************/
> +
> +static ssize_t show_u8(u8 value, char *buf)
> +{
> +       return scnprintf(buf, PAGE_SIZE, "%02x\n", value);
> +}
> +
> +static ssize_t store_u8(u8 *value, const char *buf, int count)
> +{
> +       int err;
> +       u8 result;
> +
> +       err = kstrtou8(buf, 16, &result);
> +       if (err < 0) {
> +               pr_warn("Trying to store invalid value\n");
> +               return err;
> +       }
> +
> +       *value = result;
> +
> +       return count;
> +}
> +
> +static ssize_t kbbl_red_show(struct device *dev, struct device_attribute *attr,
> +               char *buf)
> +{
> +       struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +       return show_u8(asus->kbbl_rgb.kbbl_red, buf);
> +}
> +
> +static ssize_t kbbl_red_store(struct device *dev, struct device_attribute *attr,
> +               const char *buf, size_t count)
> +{
> +       struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +       return store_u8(&asus->kbbl_rgb.kbbl_set_red, buf, count);
> +}
> +
> +static ssize_t kbbl_green_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +       return show_u8(asus->kbbl_rgb.kbbl_green, buf);
> +}
> +
> +static ssize_t kbbl_green_store(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t count)
> +{
> +       struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +       return store_u8(&asus->kbbl_rgb.kbbl_set_green, buf, count);
> +}
> +
> +static ssize_t kbbl_blue_show(struct device *dev, struct device_attribute *attr,
> +               char *buf)
> +{
> +       struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +       return show_u8(asus->kbbl_rgb.kbbl_blue, buf);
> +}
> +
> +static ssize_t kbbl_blue_store(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t count)
> +{
> +       struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +       return store_u8(&asus->kbbl_rgb.kbbl_set_blue, buf, count);
> +}
> +
> +static ssize_t kbbl_mode_show(struct device *dev, struct device_attribute *attr,
> +               char *buf)
> +{
> +       struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +       return show_u8(asus->kbbl_rgb.kbbl_mode, buf);
> +}
> +
> +static ssize_t kbbl_mode_store(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t count)
> +{
> +       struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +       return store_u8(&asus->kbbl_rgb.kbbl_set_mode, buf, count);
> +}
> +
> +static ssize_t kbbl_speed_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +       return show_u8(asus->kbbl_rgb.kbbl_speed, buf);
> +}
> +
> +static ssize_t kbbl_speed_store(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t count)
> +{
> +       struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +       return store_u8(&asus->kbbl_rgb.kbbl_set_speed, buf, count);
> +}
> +
> +static ssize_t kbbl_flags_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +       return show_u8(asus->kbbl_rgb.kbbl_set_flags, buf);
> +}
> +
> +static ssize_t kbbl_flags_store(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t count)
> +{
> +       struct asus_wmi *asus = dev_get_drvdata(dev);
> +
> +       return store_u8(&asus->kbbl_rgb.kbbl_set_flags, buf, count);
> +}
> +
> +static ssize_t kbbl_set_show(struct device *dev,
> +               struct device_attribute *attr, char *buf)
> +{
> +       return scnprintf(buf, PAGE_SIZE,
> +                       "Write to configure RGB keyboard backlight\n");
> +}
> +
> +static int kbbl_rgb_write(struct asus_wmi *asus, int persistent)
> +{
> +       int err;
> +       u32 retval;
> +       u8 speed_byte;
> +       u8 mode_byte;
> +       u8 speed;
> +       u8 mode;
> +
> +       speed = asus->kbbl_rgb.kbbl_set_speed;
> +       switch (speed) {
> +       case 0:
> +       default:
> +               speed_byte = 0xe1; // slow
> +               speed = 0;
> +               break;
> +       case 1:
> +               speed_byte = 0xeb; // medium
> +               break;
> +       case 2:
> +               speed_byte = 0xf5; // fast
> +               break;
> +       }
> +
> +       mode = asus->kbbl_rgb.kbbl_set_mode;
> +       switch (mode) {
> +       case 0:
> +       default:
> +               mode_byte = 0x00; // static color
> +               mode = 0;
> +               break;
> +       case 1:
> +               mode_byte = 0x01; // breathing
> +               break;
> +       case 2:
> +               mode_byte = 0x02; // color cycle
> +               break;
> +       case 3:
> +               mode_byte = 0x0a; // strobing
> +               break;
> +       }
> +
> +       err = asus_wmi_evaluate_method_3dw(ASUS_WMI_METHODID_DEVS,
> +               ASUS_WMI_DEVID_KBD_RGB,
> +               (persistent ? 0xb4 : 0xb3) |
> +               (mode_byte << 8) |
> +               (asus->kbbl_rgb.kbbl_set_red << 16) |
> +               (asus->kbbl_rgb.kbbl_set_green << 24),
> +               (asus->kbbl_rgb.kbbl_set_blue) |
> +               (speed_byte << 8), &retval);
> +       if (err) {
> +               pr_warn("RGB keyboard device 1, write error: %d\n", err);
> +               return err;
> +       }
> +
> +       if (retval != 1) {
> +               pr_warn("RGB keyboard device 1, write error (retval): %x\n",
> +                               retval);
> +               return -EIO;
> +       }
> +
> +       err = asus_wmi_evaluate_method_3dw(ASUS_WMI_METHODID_DEVS,
> +               ASUS_WMI_DEVID_KBD_RGB2,
> +               (0xbd) |
> +               (asus->kbbl_rgb.kbbl_set_flags << 16) |
> +               (persistent ? 0x0100 : 0x0000), 0, &retval);
> +       if (err) {
> +               pr_warn("RGB keyboard device 2, write error: %d\n", err);
> +               return err;
> +       }
> +
> +       if (retval != 1) {
> +               pr_warn("RGB keyboard device 2, write error (retval): %x\n",
> +                               retval);
> +               return -EIO;
> +       }
> +
> +       asus->kbbl_rgb.kbbl_red = asus->kbbl_rgb.kbbl_set_red;
> +       asus->kbbl_rgb.kbbl_green = asus->kbbl_rgb.kbbl_set_green;
> +       asus->kbbl_rgb.kbbl_blue = asus->kbbl_rgb.kbbl_set_blue;
> +       asus->kbbl_rgb.kbbl_mode = mode;
> +       asus->kbbl_rgb.kbbl_speed = speed;
> +
> +       return 0;
> +}
> +
> +static ssize_t kbbl_set_store(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t count)
> +{
> +       u8 value;
> +       struct asus_wmi *asus;
> +       int result;
> +
> +       asus = dev_get_drvdata(dev);
> +       result = store_u8(&value, buf, count);
> +       if (result < 0)
> +               return result;
> +
> +       if (value == 1)
> +               kbbl_rgb_write(asus, 1);
> +       else if (value == 2)
> +               kbbl_rgb_write(asus, 0);
> +
> +       return count;
> +}
> +
> +/* RGB values: 00 .. ff */
> +static DEVICE_ATTR_RW(kbbl_red);
> +static DEVICE_ATTR_RW(kbbl_green);
> +static DEVICE_ATTR_RW(kbbl_blue);
> +
> +/*
> + * Color modes: 0 - static color, 1 - breathing, 2 - color cycle, 3 - strobing
> + */
> +static DEVICE_ATTR_RW(kbbl_mode);
> +
> +/* Speed for modes 1 and 2: 0 - slow, 1 - medium, 2 - fast */
> +static DEVICE_ATTR_RW(kbbl_speed);
> +
> +/*
> + * Enable: 02 - on boot (until module load) | 08 - awake | 20 - sleep
> + * (2a or ff to enable everything)
> + *
> + * Logically 80 would be shutdown, but no visible effects of this option
> + * were observed so far
> + */
> +static DEVICE_ATTR_RW(kbbl_flags);
> +
> +/* Write data: 1 - permanently, 2 - temporarily (reset after reboot) */
> +static DEVICE_ATTR_RW(kbbl_set);
> +
> +static struct attribute *rgbkb_sysfs_attributes[] = {
> +       &dev_attr_kbbl_red.attr,
> +       &dev_attr_kbbl_green.attr,
> +       &dev_attr_kbbl_blue.attr,
> +       &dev_attr_kbbl_mode.attr,
> +       &dev_attr_kbbl_speed.attr,
> +       &dev_attr_kbbl_flags.attr,
> +       &dev_attr_kbbl_set.attr,
> +       NULL,
> +};
> +
> +static const struct attribute_group kbbl_attribute_group = {
> +       .name = "kbbl",
> +       .attrs = rgbkb_sysfs_attributes
> +};
> +
> +static int kbbl_rgb_init(struct asus_wmi *asus)
> +{
> +       int err;
> +
> +       err = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_KBD_RGB);
> +       if (err) {
> +               if (err == -ENODEV)
> +                       return 0;
> +               else
> +                       return err;
> +       }
> +
> +       err = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_KBD_RGB2);
> +       if (err) {
> +               if (err == -ENODEV)
> +                       return 0;
> +               else
> +                       return err;
> +       }
> +
> +       asus->kbbl_rgb_available = true;
> +       return sysfs_create_group(&asus->platform_device->dev.kobj,
> +                       &kbbl_attribute_group);
> +}
> +
> +static void kbbl_rgb_exit(struct asus_wmi *asus)
> +{
> +       if (asus->kbbl_rgb_available) {
> +               sysfs_remove_group(&asus->platform_device->dev.kobj,
> +                               &kbbl_attribute_group);
> +       }
> +}
> +
>  /* RF *************************************************************************/
>
>  /*
> @@ -2230,6 +2554,10 @@ static int asus_wmi_add(struct platform_device *pdev)
>         if (err)
>                 goto fail_leds;
>
> +       err = kbbl_rgb_init(asus);
> +       if (err)
> +               goto fail_rgbkb;
> +
>         asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_WLAN, &result);
>         if (result & (ASUS_WMI_DSTS_PRESENCE_BIT | ASUS_WMI_DSTS_USER_BIT))
>                 asus->driver->wlan_ctrl_by_user = 1;
> @@ -2287,6 +2615,8 @@ static int asus_wmi_add(struct platform_device *pdev)
>  fail_backlight:
>         asus_wmi_rfkill_exit(asus);
>  fail_rfkill:
> +       kbbl_rgb_exit(asus);
> +fail_rgbkb:
>         asus_wmi_led_exit(asus);
>  fail_leds:
>  fail_hwmon:
> @@ -2307,6 +2637,7 @@ static int asus_wmi_remove(struct platform_device *device)
>         asus_wmi_backlight_exit(asus);
>         asus_wmi_input_exit(asus);
>         asus_wmi_led_exit(asus);
> +       kbbl_rgb_exit(asus);
>         asus_wmi_rfkill_exit(asus);
>         asus_wmi_debugfs_exit(asus);
>         asus_wmi_platform_exit(asus);
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index a5fe7e68944b..c8c6e939e196 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -57,6 +57,8 @@
>  #define ASUS_WMI_DEVID_KBD_BACKLIGHT   0x00050021
>  #define ASUS_WMI_DEVID_LIGHT_SENSOR    0x00050022 /* ?? */
>  #define ASUS_WMI_DEVID_LIGHTBAR                0x00050025
> +#define ASUS_WMI_DEVID_KBD_RGB         0x00100056
> +#define ASUS_WMI_DEVID_KBD_RGB2                0x00100057
>
>  /* Misc */
>  #define ASUS_WMI_DEVID_CAMERA          0x00060013
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2210 bytes --]

Hi!

> > The WMI exposes two methods for controlling RGB keyboard backlight, which
> > allows controlling:
> > * RGB components in range 00 - ff,
> > * Switch between 4 effects,
> > * Switch between 3 effect speed modes,
> > * Separately enable the backlight on boot, in the awake state (after driver
> >   load), in sleep mode, and probably in something called shutdown mode (no
> >   observable effects of enabling it are known so far).
> >
> > The configuration should be written to several sysfs parameter buffers
> > which are then written via WMI by writing either 1 or 2 to the "kbbl_set"
> > parameter. When reading the buffers the last written value is returned.
> >
> > If the 2 is written to "kbbl_set", the parameters will be reset on reboot
> > (temporary mode), 1 is permanent mode, parameters are retained.
> >
> > The calls use new 3-dword input buffer method call.
> >
> > The functionality is only enabled if corresponding DSTS methods return
> > exact valid values.
> >
> > The following script demonstrates usage:
> >
> > echo Red [00 - ff]
> > echo 33 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_red
> > echo Green [00 - ff]
> > echo ff > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_green
> > echo Blue [00 - ff]
> > echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_blue
> > echo Mode: 0 - static color, 1 - breathing, 2 - color cycle, 3 - strobing
> > echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_mode
> > echo Speed for modes 1 and 2: 0 - slow, 1 - medium, 2 - fast
> > echo 0 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_speed
> > echo Enable: 02 - on boot, before module load, 08 - awake, 20 - sleep,
> > echo 2a or ff to set all
> > echo 2a > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_flags
> > echo Save: 1 - permanently, 2 - temporarily, reset after reboot
> > echo 1 > /sys/devices/platform/asus-nb-wmi/kbbl/kbbl_set
> >
> 
> Shouldn't be the LED subsystem driver for this?

Yes, please. We have common interface for LED drivers; this needs to
use it.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 04/11] platform/x86: asus-wmi: Improve DSTS WMI method ID detection

[Daniel Drake]

> > -#define ASUS_WMI_METHODID_DSTS         0x53544344 /* Device STatuS */
> > -#define ASUS_WMI_METHODID_DSTS2                0x53545344 /* Device STatuS #2*/
>
> > +#define ASUS_WMI_METHODID_DSTS         0x53544344 /* Device STatuS (DCTS) */
>
> It's not clear from the description what 'C' stands for.
> Is there any specification which describes the difference and actual
> abbreviations?

The (recent) spec I have here doesn't mention 0x53544344 (DCTS).
However the spec changelog does mention that EEEPC stuff was removed
from the spec a while ago.
The spec does mention 0x53545344 (DSTS), labelled as "Get device status".

For clarity I think the constants could be renamed as
ASUS_WMI_METHODID_DCTS and ASUS_WMI_METHODID_DSTS.

Daniel

Re: [PATCH v3 04/11] platform/x86: asus-wmi: Improve DSTS WMI method ID detection

[Yurii Pavlovskyi]

On 08.05.19 15:36, Andy Shevchenko wrote:> On Fri, Apr 19, 2019 at 1:08 PM
Yurii Pavlovskyi
> <yurii.pavlovskyi@gmail.com> wrote:
>>         int rv;
>> +       char *wmi_uid;
>
> Keep them in reversed spruce order.

What do you mean by that? Do you mean like this?
+ char *wmi_uid;
int rv;
int err;

>> +#define ASUS_WMI_METHODID_DSTS         0x53544344 /* Device STatuS
(DCTS) */
>
> It's not clear from the description what 'C' stands for.
> Is there any specification which describes the difference and actual
> abbreviations?
>
Not that I know of. Daniel had written some research in the previous
version thread regarding where it is used, but as I understand from current
implementation the functional difference is not really there, as it serves
the same purpose as DSTS, just for another hardware.

On 09.05.19 08:08, Daniel Drake wrote:
> For clarity I think the constants could be renamed as
> ASUS_WMI_METHODID_DCTS and ASUS_WMI_METHODID_DSTS.
>
Agree, that'll be better.

Thanks,
Yurii

Re: [PATCH v3 05/11] platform/x86: asus-wmi: Support WMI event queue

[Yurii Pavlovskyi]

On 08.05.19 15:47, Andy Shevchenko wrote:
> On Fri, Apr 19, 2019 at 1:10 PM Yurii Pavlovskyi
> <yurii.pavlovskyi@gmail.com> wrote:
> 
> It's rather a big change. Can it be split to smaller pieces?
> 
I will then split this into refactoring event code handling in separate
methods and actual event queue support patches.

Re: [PATCH v3 08/11] platform/x86: asus-wmi: Enhance detection of thermal data

[Yurii Pavlovskyi]

On 08.05.19 15:58, Andy Shevchenko wrote:
> On Fri, Apr 19, 2019 at 1:12 PM Yurii Pavlovskyi
> <yurii.pavlovskyi@gmail.com> wrote:
> 
>> -               if (value == ASUS_WMI_UNSUPPORTED_METHOD || value & 0xFFF80000
>> +               if (value == ASUS_WMI_UNSUPPORTED_METHOD || (value & 0xFFF80000)
> 
> Seems like a bug fix and thus should be a separate commit predecessing
> the series.
The previous one should theoretically work as well, just thought that would
help readability, will revert this.

>> -       else if (attr == &dev_attr_temp1_input.attr)
>> -               dev_id = ASUS_WMI_DEVID_THERMAL_CTRL;
>
> I don't see how this change affects the user output or driver
> behaviour. Why is it done?
> 
>> -       } else if (dev_id == ASUS_WMI_DEVID_THERMAL_CTRL) {
> 
>> +       } else if (attr == &dev_attr_temp1_input.attr) {
> 
> So, I don't see why you change this line.
> 
Yes, looking at this patch now I'd guess the refactoring there is really
misguided as it adds a lot more code than it removes, will drop it
completely and just add a new condition to the current check instead in
next version:
-		/* If value is zero, something is clearly wrong */
-		if (!value)
+		if (!value || value == 1)

Thanks,
Yurii

Re: [PATCH v3 08/11] platform/x86: asus-wmi: Enhance detection of thermal data

[Andy Shevchenko]

On Thu, May 9, 2019 at 8:49 PM Yurii Pavlovskyi
<yurii.pavlovskyi@gmail.com> wrote:
> On 08.05.19 15:58, Andy Shevchenko wrote:

> Yes, looking at this patch now I'd guess the refactoring there is really
> misguided as it adds a lot more code than it removes, will drop it
> completely and just add a new condition to the current check instead in
> next version:
> -               /* If value is zero, something is clearly wrong */
> -               if (!value)
> +               if (!value || value == 1)

Perhaps here makes sense to explicitly show value == 0.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 04/11] platform/x86: asus-wmi: Improve DSTS WMI method ID detection

[Andy Shevchenko]

On Thu, May 9, 2019 at 8:29 PM Yurii Pavlovskyi
<yurii.pavlovskyi@gmail.com> wrote:
> On 08.05.19 15:36, Andy Shevchenko wrote:> On Fri, Apr 19, 2019 at 1:08 PM
> Yurii Pavlovskyi
> > <yurii.pavlovskyi@gmail.com> wrote:
> >>         int rv;
> >> +       char *wmi_uid;
> >
> > Keep them in reversed spruce order.
>
> What do you mean by that? Do you mean like this?

> + char *wmi_uid;
> int rv;

Yes.

> int err;

Don't see this in the above, though.

> >> +#define ASUS_WMI_METHODID_DSTS         0x53544344 /* Device STatuS
> (DCTS) */
> >
> > It's not clear from the description what 'C' stands for.
> > Is there any specification which describes the difference and actual
> > abbreviations?
> >
> Not that I know of. Daniel had written some research in the previous
> version thread regarding where it is used, but as I understand from current
> implementation the functional difference is not really there, as it serves
> the same purpose as DSTS, just for another hardware.
>
> On 09.05.19 08:08, Daniel Drake wrote:
> > For clarity I think the constants could be renamed as
> > ASUS_WMI_METHODID_DCTS and ASUS_WMI_METHODID_DSTS.
> >
> Agree, that'll be better.

Also makes sense, but then fix up capitalization in the comment
(perhaps "Device status" would be good in both cases).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight

[Yurii Pavlovskyi]

First of all, thanks to Andy for all the review comments!

I will implement all the ones that I didn't directly answer on as well and
update this series shortly.

Regarding this patch,

On 08.05.19 19:12, Pavel Machek wrote:
>> Shouldn't be the LED subsystem driver for this?
> 
> Yes, please. We have common interface for LED drivers; this needs to
> use it.

That is indeed a better option and I did in fact considered this first and
even did a test implementation. The discoveries were:
1. The WMI methods are write-only and only written all at once in a
transaction manner (also invoking solely first RGB-interface method has no
effect until some other keyboard backlight method is called).
2. In addition to RGB there are several control values, which switch
effects, speed and enable or disable the backlight under specific
conditions or switch whether it is set temporarily or permanently (not that
these are critical functionalities, but for the sake of completeness).
3. The EC is really slow
# time bash -c "echo 1 > /sys/devices/platform/faustus/kbbl_set"

real	0m0,691s
user	0m0,000s
sys	0m0,691s

(please ignore the sysfs-path there, it's essentially the same code running
as in this patch). It is consistently same for both temporary and permanent
configuration. Writing after every change would take about (6+)x of that.
Not that it's that unbearable though as it is not likely to be done often.

I was not quite happy with that implementation so I opted for writing sort
of sysfs wrapper instead that would allow same sort of transactions as
provided by BIOS. I agree that it's non-standard solution.

If I understood correctly, the typical current RGB led_class devices from
the Linux tree currently provide channels as separate LEDs. There are also
blink / pattern options present, I guess one could misuse them for setting
effects and speed. So one could make 3 devices for RGB + 3 for awake,
sleep, boot modes + 1 for setting effect / speed.

I'd guess the end solution might be also either something like combination
of both approaches (RGB leds + separate sysfs interface) or some extension
of the led_class device interface. Dropping support of the non-essential
features for the sake of uniformity of ABI would also be an option to
consider (exposing just three RGB LEDs with brightness only), not happy one
though.

In any case this looks like it might need some additional research,
discussion, development, and a pair of iterations so I tend to separate
this patch from the series and post it extra after the others are through
to avoid dragging 10+ patches around.

Any suggestions on how to do this properly would be appreciated. That's the
best I could come up with at the moment.

Thanks,
Yurii

Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight

[Dan Murphy]

Yurii

On 5/9/19 2:04 PM, Yurii Pavlovskyi wrote:
> First of all, thanks to Andy for all the review comments!
> 
> I will implement all the ones that I didn't directly answer on as well and
> update this series shortly.
> 
> Regarding this patch,
> 
> On 08.05.19 19:12, Pavel Machek wrote:
>>> Shouldn't be the LED subsystem driver for this?
>>
>> Yes, please. We have common interface for LED drivers; this needs to
>> use it.
> 
> That is indeed a better option and I did in fact considered this first and
> even did a test implementation. The discoveries were:
> 1. The WMI methods are write-only and only written all at once in a
> transaction manner (also invoking solely first RGB-interface method has no
> effect until some other keyboard backlight method is called).
> 2. In addition to RGB there are several control values, which switch
> effects, speed and enable or disable the backlight under specific
> conditions or switch whether it is set temporarily or permanently (not that
> these are critical functionalities, but for the sake of completeness).
> 3. The EC is really slow
> # time bash -c "echo 1 > /sys/devices/platform/faustus/kbbl_set"
> 
> real	0m0,691s
> user	0m0,000s
> sys	0m0,691s
> 
> (please ignore the sysfs-path there, it's essentially the same code running
> as in this patch). It is consistently same for both temporary and permanent
> configuration. Writing after every change would take about (6+)x of that.
> Not that it's that unbearable though as it is not likely to be done often.
> 
> I was not quite happy with that implementation so I opted for writing sort
> of sysfs wrapper instead that would allow same sort of transactions as
> provided by BIOS. I agree that it's non-standard solution.
> 
> If I understood correctly, the typical current RGB led_class devices from
> the Linux tree currently provide channels as separate LEDs. There are also
> blink / pattern options present, I guess one could misuse them for setting
> effects and speed. So one could make 3 devices for RGB + 3 for awake,
> sleep, boot modes + 1 for setting effect / speed.
> 
> I'd guess the end solution might be also either something like combination
> of both approaches (RGB leds + separate sysfs interface) or some extension
> of the led_class device interface. Dropping support of the non-essential
> features for the sake of uniformity of ABI would also be an option to
> consider (exposing just three RGB LEDs with brightness only), not happy one
> though.
> 
> In any case this looks like it might need some additional research,
> discussion, development, and a pair of iterations so I tend to separate
> this patch from the series and post it extra after the others are through
> to avoid dragging 10+ patches around.
> 
> Any suggestions on how to do this properly would be appreciated. That's the
> best I could come up with at the moment.
> 

We are working on a framework for this.

Please see this series
https://lore.kernel.org/patchwork/project/lkml/list/?series=390141

It is still a work in progress

> Thanks,
> Yurii
> 

Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight

[Andy Shevchenko]

On Thu, May 9, 2019 at 11:45 PM Dan Murphy <dmurphy@ti.com> wrote:
> On 5/9/19 2:04 PM, Yurii Pavlovskyi wrote:
> We are working on a framework for this.
>
> Please see this series
> https://lore.kernel.org/patchwork/project/lkml/list/?series=390141
>
> It is still a work in progress

Side question:
Have you considered to convert existing color LED controllers? (It
seems to me that your proposal lacks of the idea to keep back
compatibility with the existing controllers whre user may create a
sysfs node based on the arbitrary label, while it's good to have
multicolor infrastructure like in your proposal. Did I miss
something?)


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight

[Dan Murphy]

Andy

On 5/9/19 4:06 PM, Andy Shevchenko wrote:
> On Thu, May 9, 2019 at 11:45 PM Dan Murphy <dmurphy@ti.com> wrote:
>> On 5/9/19 2:04 PM, Yurii Pavlovskyi wrote:
>> We are working on a framework for this.
>>
>> Please see this series
>> https://lore.kernel.org/patchwork/project/lkml/list/?series=390141
>>
>> It is still a work in progress
> 
> Side question:
> Have you considered to convert existing color LED controllers? (It
> seems to me that your proposal lacks of the idea to keep back
> compatibility with the existing controllers whre user may create a
> sysfs node based on the arbitrary label, while it's good to have
> multicolor infrastructure like in your proposal. Did I miss
> something?)
> 
> 

Yes that is part of the work that is in progress.
The LED driver should be able to register either a single color LED or a group of colored LEDs.

This can be based on a firmware entry and which LED framework the driver chooses to register to. Either the
multicolor framework or the base LED framework.  Of course we can put this in code and keep it
out of the firmware nodes again thats why it is wip.

I have convert a couple of drivers over in my testing that support RGB modules or have a RGB cluter used to mix
colors.

If the product wants to expose a single red LED via the label then they use legacy registration.
If the product wants to expose RGBW as a single group then the multicolor framework should be registered too.


Dan

Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 993 bytes --]

On Fri 2019-05-10 00:06:11, Andy Shevchenko wrote:
> On Thu, May 9, 2019 at 11:45 PM Dan Murphy <dmurphy@ti.com> wrote:
> > On 5/9/19 2:04 PM, Yurii Pavlovskyi wrote:
> > We are working on a framework for this.
> >
> > Please see this series
> > https://lore.kernel.org/patchwork/project/lkml/list/?series=390141
> >
> > It is still a work in progress
> 
> Side question:
> Have you considered to convert existing color LED controllers? (It
> seems to me that your proposal lacks of the idea to keep back
> compatibility with the existing controllers whre user may create a
> sysfs node based on the arbitrary label, while it's good to have
> multicolor infrastructure like in your proposal. Did I miss
> something?)

That's undecided at the moment. We have enough fun trying to figure
out reasonable interface...


									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 09/11] platform/x86: asus-wmi: Control RGB keyboard backlight

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3147 bytes --]

Hi!

> >> Yes, please. We have common interface for LED drivers; this needs to
> >> use it.
> > 
> > That is indeed a better option and I did in fact considered this first and
> > even did a test implementation. The discoveries were:
> > 1. The WMI methods are write-only and only written all at once in a
> > transaction manner (also invoking solely first RGB-interface method has no
> > effect until some other keyboard backlight method is called).

Write-only is not a problem, right? Nor should be transaction. Just
cache the values in kernel.

> > 2. In addition to RGB there are several control values, which switch
> > effects, speed and enable or disable the backlight under specific
> > conditions or switch whether it is set temporarily or permanently (not that
> > these are critical functionalities, but for the sake of
> > completeness).

Yep, lets ignore that for now.

> > 3. The EC is really slow
> > # time bash -c "echo 1 > /sys/devices/platform/faustus/kbbl_set"
> > 
> > real	0m0,691s
> > user	0m0,000s
> > sys	0m0,691s
> > 
> > (please ignore the sysfs-path there, it's essentially the same code running
> > as in this patch). It is consistently same for both temporary and permanent
> > configuration. Writing after every change would take about (6+)x of that.
> > Not that it's that unbearable though as it is not likely to be
> > done often.

Yup, this is quite ugly.

What about simply ignoring changes as they happen, and then setting
RGB channels when nothing changes for 10msec?

> > I was not quite happy with that implementation so I opted for writing sort
> > of sysfs wrapper instead that would allow same sort of transactions as
> > provided by BIOS. I agree that it's non-standard solution.
> > 
> > If I understood correctly, the typical current RGB led_class devices from
> > the Linux tree currently provide channels as separate LEDs. There are also
> > blink / pattern options present, I guess one could misuse them for setting
> > effects and speed. So one could make 3 devices for RGB + 3 for awake,
> > sleep, boot modes + 1 for setting effect / speed.

Take a look at "pattern" trigger. That should give you effect/speed
options. .. for single channel.

> > I'd guess the end solution might be also either something like combination
> > of both approaches (RGB leds + separate sysfs interface) or some extension
> > of the led_class device interface. Dropping support of the non-essential
> > features for the sake of uniformity of ABI would also be an option to
> > consider (exposing just three RGB LEDs with brightness only), not happy one
> > though.
> > 
> > In any case this looks like it might need some additional research,
> > discussion, development, and a pair of iterations so I tend to separate
> > this patch from the series and post it extra after the others are through
> > to avoid dragging 10+ patches around.

Separate patch certainly makes sense.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]