linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] platform/x86: acer-wmi: Add Turbo Mode support for Acer PH315-53
@ 2021-08-10 23:39 JafarAkhondali
  2021-08-11  4:07 ` kernel test robot
  2021-08-12  7:59 ` Hans de Goede
  0 siblings, 2 replies; 4+ messages in thread
From: JafarAkhondali @ 2021-08-10 23:39 UTC (permalink / raw)
  To: Hans de Goede, jlee, linux-kernel, platform-driver-x86, mgross
  Cc: JafarAkhondali

Hi,

The Acer Predator Helios series (usually denoted by PHxxx-yy) features
a special key above the keyboard named "TURBO". The turbo key does 3
things:
1. Set all fan's speeds to TURBO mode
2. Overclocks the CPU and GPU in the safe range
3. Turn on an LED just below the turbo button

All of these actions are done by WMI function calls, and there is no
custom OC level for turbo. It acts as a flag for enabling turbo
mode instead of telling processors to use 1.3x of power.

I've run some benchmark tests and it worked fine:

GpuTest 0.7.0
http://www.geeks3d.com

Module: FurMark
Normal mode Score: 7289 points (FPS: 121)
Turbo mode Score: 7675 points (FPS: 127)
Settings:
- 1920x1080 fullscreen
- antialiasing: Off
- duration: 60000 ms

Renderer:
- GeForce RTX 2060/PCIe/SSE2
- OpenGL: 4.6.0 NVIDIA 460.32.03

This feature is presented by Acer officially and should not harm
hardware in any case.

A challenging part of implementing this feature is that calling
overclocking the function requires knowing the exact count of fans for CPU
and GPU for each model, which to the best of my knowledge is not
available in the kernel.

So after checking the official PredatorSense application methods, it
turned out they have provided the software the list of fans in each model.
I have access to the mentioned list, and all similar PH-iii-jj can be
added easily by matching "DMI_PRODUCT_NAME".

Creating a separate file for the gaming interface was not possible because
the current WMI event GUID is needed for the turbo button, and it's not
possible to register multiple functions on the same event GUID.


Signed-off-by: JafarAkhondali <jafar.akhoondali@gmail.com>
---
 Changes in v4:
 - Fix Indents
 - Make functions return early

 Changes in v3:
 - Remove usages of gaming_interface
 - Add ACPI output for u32 buffer length
 - Remove set_u64 and get_u64 functions
 - Remove unrelated whitespace changes for to this patch

 Changes in v2:
 - Fix formatting problems


 drivers/platform/x86/acer-wmi.c | 183 ++++++++++++++++++++++++++++++++
 1 file changed, 183 insertions(+)

diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index 85db9403cc14..183faa6db4b2 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -60,6 +60,11 @@ MODULE_LICENSE("GPL");
 #define ACER_WMID_GET_THREEG_METHODID		10
 #define ACER_WMID_SET_THREEG_METHODID		11
 
+#define ACER_WMID_SET_GAMING_LED_METHODID 2
+#define ACER_WMID_GET_GAMING_LED_METHODID 4
+#define ACER_WMID_SET_GAMING_FAN_BEHAVIOR 14
+#define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
+
 /*
  * Acer ACPI method GUIDs
  */
@@ -68,6 +73,7 @@ MODULE_LICENSE("GPL");
 #define WMID_GUID1		"6AF4F258-B401-42FD-BE91-3D4AC2D7C0D3"
 #define WMID_GUID2		"95764E09-FB56-4E83-B31A-37761F60994A"
 #define WMID_GUID3		"61EF69EA-865C-4BC3-A502-A0DEBA0CB531"
+#define WMID_GUID4		"7A4DDFE7-5B5D-40B4-8595-4408E0CC7F56"
 
 /*
  * Acer ACPI event GUIDs
@@ -81,6 +87,7 @@ MODULE_ALIAS("wmi:676AA15E-6A47-4D9F-A2CC-1E6D18D14026");
 enum acer_wmi_event_ids {
 	WMID_HOTKEY_EVENT = 0x1,
 	WMID_ACCEL_OR_KBD_DOCK_EVENT = 0x5,
+	WMID_GAMING_TURBO_KEY_EVENT = 0x7,
 };
 
 static const struct key_entry acer_wmi_keymap[] __initconst = {
@@ -215,6 +222,9 @@ struct hotkey_function_type_aa {
 #define ACER_CAP_THREEG			BIT(4)
 #define ACER_CAP_SET_FUNCTION_MODE	BIT(5)
 #define ACER_CAP_KBD_DOCK		BIT(6)
+#define ACER_CAP_TURBO_OC     BIT(7)
+#define ACER_CAP_TURBO_LED     BIT(8)
+#define ACER_CAP_TURBO_FAN     BIT(9)
 
 /*
  * Interface type flags
@@ -301,6 +311,9 @@ struct quirk_entry {
 	u8 mailled;
 	s8 brightness;
 	u8 bluetooth;
+	u8 turbo;
+	u8 cpu_fans;
+	u8 gpu_fans;
 };
 
 static struct quirk_entry *quirks;
@@ -312,6 +325,10 @@ static void __init set_quirks(void)
 
 	if (quirks->brightness)
 		interface->capability |= ACER_CAP_BRIGHTNESS;
+
+	if (quirks->turbo)
+		interface->capability |= ACER_CAP_TURBO_OC | ACER_CAP_TURBO_LED
+					 | ACER_CAP_TURBO_FAN;
 }
 
 static int __init dmi_matched(const struct dmi_system_id *dmi)
@@ -340,6 +357,12 @@ static struct quirk_entry quirk_acer_travelmate_2490 = {
 	.mailled = 1,
 };
 
+static struct quirk_entry quirk_acer_predator_ph315_53 = {
+	.turbo = 1,
+	.cpu_fans = 1,
+	.gpu_fans = 1,
+};
+
 /* This AMW0 laptop has no bluetooth */
 static struct quirk_entry quirk_medion_md_98300 = {
 	.wireless = 1,
@@ -507,6 +530,15 @@ static const struct dmi_system_id acer_quirks[] __initconst = {
 		},
 		.driver_data = &quirk_acer_travelmate_2490,
 	},
+	{
+		.callback = dmi_matched,
+		.ident = "Acer Predator PH315-53",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Predator PH315-53"),
+		},
+		.driver_data = &quirk_acer_predator_ph315_53,
+	},
 	{
 		.callback = set_force_caps,
 		.ident = "Acer Aspire Switch 10E SW3-016",
@@ -1344,6 +1376,93 @@ static struct wmi_interface wmid_v2_interface = {
 	.type = ACER_WMID_v2,
 };
 
+/*
+ * WMID Gaming interface
+ */
+
+static acpi_status
+WMI_gaming_execute_u64(u32 method_id, u64 in, u64 *out)
+{
+	struct acpi_buffer input = { (acpi_size) sizeof(u64), (void *)(&in) };
+	struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+	u32 tmp = 0;
+	acpi_status status;
+
+	status = wmi_evaluate_method(WMID_GUID4, 0, method_id, &input, &result);
+
+	if (ACPI_FAILURE(status))
+		return status;
+	obj = (union acpi_object *) result.pointer;
+
+	if (obj) {
+		if (obj->type == ACPI_TYPE_BUFFER) {
+			if (obj->buffer.length == sizeof(u32))
+				tmp = *((u32 *) obj->buffer.pointer);
+			else if (obj->buffer.length == sizeof(u64))
+				tmp = *((u64 *) obj->buffer.pointer);
+		} else if (obj->type == ACPI_TYPE_INTEGER) {
+			tmp = (u64) obj->integer.value;
+		}
+	}
+
+	if (out)
+		*out = tmp;
+
+	kfree(result.pointer);
+
+	return status;
+}
+
+static acpi_status WMID_gaming_set_u64(u64 value, u32 cap)
+{
+	u32 method_id = 0;
+
+	if (!(interface->capability & cap))
+		return AE_BAD_PARAMETER;
+
+	switch (cap) {
+	case ACER_CAP_TURBO_LED:
+		method_id = ACER_WMID_SET_GAMING_LED_METHODID;
+		break;
+	case ACER_CAP_TURBO_FAN:
+		method_id = ACER_WMID_SET_GAMING_FAN_BEHAVIOR;
+		break;
+	case ACER_CAP_TURBO_OC:
+		method_id = ACER_WMID_SET_GAMING_MISC_SETTING_METHODID;
+		break;
+	default:
+		return AE_BAD_PARAMETER;
+	}
+
+	return WMI_gaming_execute_u64(method_id, value, NULL);
+}
+
+static acpi_status WMID_gaming_get_u64(u64 *value, u32 cap)
+{
+	acpi_status status;
+	u64 result;
+	u64 input;
+	u32 method_id;
+
+	if (!(interface->capability & cap))
+		return AE_BAD_PARAMETER;
+
+	switch (cap) {
+	case ACER_CAP_TURBO_LED:
+		method_id = ACER_WMID_GET_GAMING_LED_METHODID;
+		input = 0x1;
+		break;
+	default:
+		return AE_BAD_PARAMETER;
+	}
+	status = WMI_gaming_execute_u64(method_id, input, &result);
+	if (ACPI_SUCCESS(status))
+		*value = (u64) result;
+
+	return status;
+}
+
 /*
  * Generic Device (interface-independent)
  */
@@ -1575,6 +1694,66 @@ static int acer_gsensor_event(void)
 	return 0;
 }
 
+/*
+ *  Predator series turbo button
+ */
+static int acer_toggle_turbo(void)
+{
+	/* Get current state from turbo button */
+	u64 turbo_led_state, gpu_fan_config1, gpu_fan_config2;
+	u8 i;
+
+	if (ACPI_FAILURE(WMID_gaming_get_u64(&turbo_led_state, ACER_CAP_TURBO_LED)))
+		return -1;
+
+	if (turbo_led_state) {
+		// turns off turbo led
+		WMID_gaming_set_u64(0x1, ACER_CAP_TURBO_LED);
+
+		// set FAN mode to auto
+		if (quirks->cpu_fans > 0)
+			gpu_fan_config2 |= 1;
+		for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
+			gpu_fan_config2 |= 1 << (i + 1);
+		for (i = 0; i < quirks->gpu_fans; ++i)
+			gpu_fan_config2 |= 1 << (i + 3);
+		if (quirks->cpu_fans > 0)
+			gpu_fan_config1 |= 1;
+		for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
+			gpu_fan_config1 |= 1 << (2 * i + 2);
+		for (i = 0; i < quirks->gpu_fans; ++i)
+			gpu_fan_config1 |= 1 << (2 * i + 6);
+		WMID_gaming_set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);
+
+		// set OC to normal
+		WMID_gaming_set_u64(0x5, ACER_CAP_TURBO_OC);
+		WMID_gaming_set_u64(0x7, ACER_CAP_TURBO_OC);
+	} else {
+		// turn on turbo led
+		WMID_gaming_set_u64(0x10001, ACER_CAP_TURBO_LED);
+
+		// set FAN to turbo mode
+		if (quirks->cpu_fans > 0)
+			gpu_fan_config2 |= 1;
+		for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
+			gpu_fan_config2 |= 1 << (i + 1);
+		for (i = 0; i < quirks->gpu_fans; ++i)
+			gpu_fan_config2 |= 1 << (i + 3);
+		if (quirks->cpu_fans > 0)
+			gpu_fan_config1 |= 2;
+		for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
+			gpu_fan_config1 |= 2 << (2 * i + 2);
+		for (i = 0; i < quirks->gpu_fans; ++i)
+			gpu_fan_config1 |= 2 << (2 * i + 6);
+		WMID_gaming_set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);
+
+		// set OC to turbo mode
+		WMID_gaming_set_u64(0x205, ACER_CAP_TURBO_OC);
+		WMID_gaming_set_u64(0x207, ACER_CAP_TURBO_OC);
+	}
+	return turbo_led_state;
+}
+
 /*
  * Switch series keyboard dock status
  */
@@ -1872,6 +2051,10 @@ static void acer_wmi_notify(u32 value, void *context)
 		acer_gsensor_event();
 		acer_kbd_dock_event(&return_value);
 		break;
+	case WMID_GAMING_TURBO_KEY_EVENT:
+		if (return_value.key_num == 0x4)
+			acer_toggle_turbo();
+		break;
 	default:
 		pr_warn("Unknown function number - %d - %d\n",
 			return_value.function, return_value.key_num);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] platform/x86: acer-wmi: Add Turbo Mode support for Acer PH315-53
  2021-08-10 23:39 [PATCH v4] platform/x86: acer-wmi: Add Turbo Mode support for Acer PH315-53 JafarAkhondali
@ 2021-08-11  4:07 ` kernel test robot
  2021-08-12  7:59 ` Hans de Goede
  1 sibling, 0 replies; 4+ messages in thread
From: kernel test robot @ 2021-08-11  4:07 UTC (permalink / raw)
  To: JafarAkhondali, Hans de Goede, jlee, linux-kernel,
	platform-driver-x86, mgross
  Cc: clang-built-linux, kbuild-all, JafarAkhondali

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

Hi JafarAkhondali,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.14-rc5 next-20210810]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/JafarAkhondali/platform-x86-acer-wmi-Add-Turbo-Mode-support-for-Acer-PH315-53/20210811-074352
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 9e723c5380c6e14fb91a8b6950563d040674afdb
config: x86_64-randconfig-c001-20210810 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project d39ebdae674c8efc84ebe8dc32716ec353220530)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/c3ff29407b252eec4ee917d22c3776858234f328
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review JafarAkhondali/platform-x86-acer-wmi-Add-Turbo-Mode-support-for-Acer-PH315-53/20210811-074352
        git checkout c3ff29407b252eec4ee917d22c3776858234f328
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/platform/x86/acer-wmi.c:1715:4: warning: variable 'gpu_fan_config2' is uninitialized when used here [-Wuninitialized]
                           gpu_fan_config2 |= 1;
                           ^~~~~~~~~~~~~~~
   drivers/platform/x86/acer-wmi.c:1703:55: note: initialize the variable 'gpu_fan_config2' to silence this warning
           u64 turbo_led_state, gpu_fan_config1, gpu_fan_config2;
                                                                ^
                                                                 = 0
>> drivers/platform/x86/acer-wmi.c:1721:4: warning: variable 'gpu_fan_config1' is uninitialized when used here [-Wuninitialized]
                           gpu_fan_config1 |= 1;
                           ^~~~~~~~~~~~~~~
   drivers/platform/x86/acer-wmi.c:1703:38: note: initialize the variable 'gpu_fan_config1' to silence this warning
           u64 turbo_led_state, gpu_fan_config1, gpu_fan_config2;
                                               ^
                                                = 0
   2 warnings generated.


vim +/gpu_fan_config2 +1715 drivers/platform/x86/acer-wmi.c

  1696	
  1697	/*
  1698	 *  Predator series turbo button
  1699	 */
  1700	static int acer_toggle_turbo(void)
  1701	{
  1702		/* Get current state from turbo button */
  1703		u64 turbo_led_state, gpu_fan_config1, gpu_fan_config2;
  1704		u8 i;
  1705	
  1706		if (ACPI_FAILURE(WMID_gaming_get_u64(&turbo_led_state, ACER_CAP_TURBO_LED)))
  1707			return -1;
  1708	
  1709		if (turbo_led_state) {
  1710			// turns off turbo led
  1711			WMID_gaming_set_u64(0x1, ACER_CAP_TURBO_LED);
  1712	
  1713			// set FAN mode to auto
  1714			if (quirks->cpu_fans > 0)
> 1715				gpu_fan_config2 |= 1;
  1716			for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
  1717				gpu_fan_config2 |= 1 << (i + 1);
  1718			for (i = 0; i < quirks->gpu_fans; ++i)
  1719				gpu_fan_config2 |= 1 << (i + 3);
  1720			if (quirks->cpu_fans > 0)
> 1721				gpu_fan_config1 |= 1;
  1722			for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
  1723				gpu_fan_config1 |= 1 << (2 * i + 2);
  1724			for (i = 0; i < quirks->gpu_fans; ++i)
  1725				gpu_fan_config1 |= 1 << (2 * i + 6);
  1726			WMID_gaming_set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);
  1727	
  1728			// set OC to normal
  1729			WMID_gaming_set_u64(0x5, ACER_CAP_TURBO_OC);
  1730			WMID_gaming_set_u64(0x7, ACER_CAP_TURBO_OC);
  1731		} else {
  1732			// turn on turbo led
  1733			WMID_gaming_set_u64(0x10001, ACER_CAP_TURBO_LED);
  1734	
  1735			// set FAN to turbo mode
  1736			if (quirks->cpu_fans > 0)
  1737				gpu_fan_config2 |= 1;
  1738			for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
  1739				gpu_fan_config2 |= 1 << (i + 1);
  1740			for (i = 0; i < quirks->gpu_fans; ++i)
  1741				gpu_fan_config2 |= 1 << (i + 3);
  1742			if (quirks->cpu_fans > 0)
  1743				gpu_fan_config1 |= 2;
  1744			for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
  1745				gpu_fan_config1 |= 2 << (2 * i + 2);
  1746			for (i = 0; i < quirks->gpu_fans; ++i)
  1747				gpu_fan_config1 |= 2 << (2 * i + 6);
  1748			WMID_gaming_set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);
  1749	
  1750			// set OC to turbo mode
  1751			WMID_gaming_set_u64(0x205, ACER_CAP_TURBO_OC);
  1752			WMID_gaming_set_u64(0x207, ACER_CAP_TURBO_OC);
  1753		}
  1754		return turbo_led_state;
  1755	}
  1756	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36357 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] platform/x86: acer-wmi: Add Turbo Mode support for Acer PH315-53
  2021-08-10 23:39 [PATCH v4] platform/x86: acer-wmi: Add Turbo Mode support for Acer PH315-53 JafarAkhondali
  2021-08-11  4:07 ` kernel test robot
@ 2021-08-12  7:59 ` Hans de Goede
  2021-08-12 12:01   ` Jafar Akhondali
  1 sibling, 1 reply; 4+ messages in thread
From: Hans de Goede @ 2021-08-12  7:59 UTC (permalink / raw)
  To: JafarAkhondali, jlee, linux-kernel, platform-driver-x86, mgross

Hi Jafar,

On 8/11/21 1:39 AM, JafarAkhondali wrote:
> Hi,
> 
> The Acer Predator Helios series (usually denoted by PHxxx-yy) features
> a special key above the keyboard named "TURBO". The turbo key does 3
> things:
> 1. Set all fan's speeds to TURBO mode
> 2. Overclocks the CPU and GPU in the safe range
> 3. Turn on an LED just below the turbo button
> 
> All of these actions are done by WMI function calls, and there is no
> custom OC level for turbo. It acts as a flag for enabling turbo
> mode instead of telling processors to use 1.3x of power.
> 
> I've run some benchmark tests and it worked fine:
> 
> GpuTest 0.7.0
> http://www.geeks3d.com
> 
> Module: FurMark
> Normal mode Score: 7289 points (FPS: 121)
> Turbo mode Score: 7675 points (FPS: 127)
> Settings:
> - 1920x1080 fullscreen
> - antialiasing: Off
> - duration: 60000 ms
> 
> Renderer:
> - GeForce RTX 2060/PCIe/SSE2
> - OpenGL: 4.6.0 NVIDIA 460.32.03
> 
> This feature is presented by Acer officially and should not harm
> hardware in any case.
> 
> A challenging part of implementing this feature is that calling
> overclocking the function requires knowing the exact count of fans for CPU
> and GPU for each model, which to the best of my knowledge is not
> available in the kernel.
> 
> So after checking the official PredatorSense application methods, it
> turned out they have provided the software the list of fans in each model.
> I have access to the mentioned list, and all similar PH-iii-jj can be
> added easily by matching "DMI_PRODUCT_NAME".
> 
> Creating a separate file for the gaming interface was not possible because
> the current WMI event GUID is needed for the turbo button, and it's not
> possible to register multiple functions on the same event GUID.
> 
> 
> Signed-off-by: JafarAkhondali <jafar.akhoondali@gmail.com>
> ---
>  Changes in v4:
>  - Fix Indents
>  - Make functions return early

This is looking good, the kernel-buildbot (which finally has managed
to build the patch now that the indents are fixed) found one last
small thing to fix.

And while looking at that I also found one last small thing to
improve, see my comments inline / below.

So we will need one more version and then this should be ready to
get merged.



>  Changes in v3:
>  - Remove usages of gaming_interface
>  - Add ACPI output for u32 buffer length
>  - Remove set_u64 and get_u64 functions
>  - Remove unrelated whitespace changes for to this patch
> 
>  Changes in v2:
>  - Fix formatting problems
> 
> 
>  drivers/platform/x86/acer-wmi.c | 183 ++++++++++++++++++++++++++++++++
>  1 file changed, 183 insertions(+)
> 
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index 85db9403cc14..183faa6db4b2 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -60,6 +60,11 @@ MODULE_LICENSE("GPL");
>  #define ACER_WMID_GET_THREEG_METHODID		10
>  #define ACER_WMID_SET_THREEG_METHODID		11
>  
> +#define ACER_WMID_SET_GAMING_LED_METHODID 2
> +#define ACER_WMID_GET_GAMING_LED_METHODID 4
> +#define ACER_WMID_SET_GAMING_FAN_BEHAVIOR 14
> +#define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
> +
>  /*
>   * Acer ACPI method GUIDs
>   */
> @@ -68,6 +73,7 @@ MODULE_LICENSE("GPL");
>  #define WMID_GUID1		"6AF4F258-B401-42FD-BE91-3D4AC2D7C0D3"
>  #define WMID_GUID2		"95764E09-FB56-4E83-B31A-37761F60994A"
>  #define WMID_GUID3		"61EF69EA-865C-4BC3-A502-A0DEBA0CB531"
> +#define WMID_GUID4		"7A4DDFE7-5B5D-40B4-8595-4408E0CC7F56"
>  
>  /*
>   * Acer ACPI event GUIDs
> @@ -81,6 +87,7 @@ MODULE_ALIAS("wmi:676AA15E-6A47-4D9F-A2CC-1E6D18D14026");
>  enum acer_wmi_event_ids {
>  	WMID_HOTKEY_EVENT = 0x1,
>  	WMID_ACCEL_OR_KBD_DOCK_EVENT = 0x5,
> +	WMID_GAMING_TURBO_KEY_EVENT = 0x7,
>  };
>  
>  static const struct key_entry acer_wmi_keymap[] __initconst = {
> @@ -215,6 +222,9 @@ struct hotkey_function_type_aa {
>  #define ACER_CAP_THREEG			BIT(4)
>  #define ACER_CAP_SET_FUNCTION_MODE	BIT(5)
>  #define ACER_CAP_KBD_DOCK		BIT(6)
> +#define ACER_CAP_TURBO_OC     BIT(7)
> +#define ACER_CAP_TURBO_LED     BIT(8)
> +#define ACER_CAP_TURBO_FAN     BIT(9)
>  
>  /*
>   * Interface type flags
> @@ -301,6 +311,9 @@ struct quirk_entry {
>  	u8 mailled;
>  	s8 brightness;
>  	u8 bluetooth;
> +	u8 turbo;
> +	u8 cpu_fans;
> +	u8 gpu_fans;
>  };
>  
>  static struct quirk_entry *quirks;
> @@ -312,6 +325,10 @@ static void __init set_quirks(void)
>  
>  	if (quirks->brightness)
>  		interface->capability |= ACER_CAP_BRIGHTNESS;
> +
> +	if (quirks->turbo)
> +		interface->capability |= ACER_CAP_TURBO_OC | ACER_CAP_TURBO_LED
> +					 | ACER_CAP_TURBO_FAN;
>  }
>  
>  static int __init dmi_matched(const struct dmi_system_id *dmi)
> @@ -340,6 +357,12 @@ static struct quirk_entry quirk_acer_travelmate_2490 = {
>  	.mailled = 1,
>  };
>  
> +static struct quirk_entry quirk_acer_predator_ph315_53 = {
> +	.turbo = 1,
> +	.cpu_fans = 1,
> +	.gpu_fans = 1,
> +};
> +
>  /* This AMW0 laptop has no bluetooth */
>  static struct quirk_entry quirk_medion_md_98300 = {
>  	.wireless = 1,
> @@ -507,6 +530,15 @@ static const struct dmi_system_id acer_quirks[] __initconst = {
>  		},
>  		.driver_data = &quirk_acer_travelmate_2490,
>  	},
> +	{
> +		.callback = dmi_matched,
> +		.ident = "Acer Predator PH315-53",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Predator PH315-53"),
> +		},
> +		.driver_data = &quirk_acer_predator_ph315_53,
> +	},
>  	{
>  		.callback = set_force_caps,
>  		.ident = "Acer Aspire Switch 10E SW3-016",
> @@ -1344,6 +1376,93 @@ static struct wmi_interface wmid_v2_interface = {
>  	.type = ACER_WMID_v2,
>  };
>  
> +/*
> + * WMID Gaming interface
> + */
> +
> +static acpi_status
> +WMI_gaming_execute_u64(u32 method_id, u64 in, u64 *out)
> +{
> +	struct acpi_buffer input = { (acpi_size) sizeof(u64), (void *)(&in) };
> +	struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +	u32 tmp = 0;
> +	acpi_status status;
> +
> +	status = wmi_evaluate_method(WMID_GUID4, 0, method_id, &input, &result);
> +
> +	if (ACPI_FAILURE(status))
> +		return status;
> +	obj = (union acpi_object *) result.pointer;
> +
> +	if (obj) {
> +		if (obj->type == ACPI_TYPE_BUFFER) {
> +			if (obj->buffer.length == sizeof(u32))
> +				tmp = *((u32 *) obj->buffer.pointer);
> +			else if (obj->buffer.length == sizeof(u64))
> +				tmp = *((u64 *) obj->buffer.pointer);
> +		} else if (obj->type == ACPI_TYPE_INTEGER) {
> +			tmp = (u64) obj->integer.value;
> +		}
> +	}
> +
> +	if (out)
> +		*out = tmp;
> +
> +	kfree(result.pointer);
> +
> +	return status;
> +}
> +
> +static acpi_status WMID_gaming_set_u64(u64 value, u32 cap)
> +{
> +	u32 method_id = 0;
> +
> +	if (!(interface->capability & cap))
> +		return AE_BAD_PARAMETER;
> +
> +	switch (cap) {
> +	case ACER_CAP_TURBO_LED:
> +		method_id = ACER_WMID_SET_GAMING_LED_METHODID;
> +		break;
> +	case ACER_CAP_TURBO_FAN:
> +		method_id = ACER_WMID_SET_GAMING_FAN_BEHAVIOR;
> +		break;
> +	case ACER_CAP_TURBO_OC:
> +		method_id = ACER_WMID_SET_GAMING_MISC_SETTING_METHODID;
> +		break;
> +	default:
> +		return AE_BAD_PARAMETER;
> +	}
> +
> +	return WMI_gaming_execute_u64(method_id, value, NULL);
> +}
> +
> +static acpi_status WMID_gaming_get_u64(u64 *value, u32 cap)
> +{
> +	acpi_status status;
> +	u64 result;
> +	u64 input;
> +	u32 method_id;
> +
> +	if (!(interface->capability & cap))
> +		return AE_BAD_PARAMETER;
> +
> +	switch (cap) {
> +	case ACER_CAP_TURBO_LED:
> +		method_id = ACER_WMID_GET_GAMING_LED_METHODID;
> +		input = 0x1;
> +		break;
> +	default:
> +		return AE_BAD_PARAMETER;
> +	}
> +	status = WMI_gaming_execute_u64(method_id, input, &result);
> +	if (ACPI_SUCCESS(status))
> +		*value = (u64) result;
> +
> +	return status;
> +}
> +
>  /*
>   * Generic Device (interface-independent)
>   */
> @@ -1575,6 +1694,66 @@ static int acer_gsensor_event(void)
>  	return 0;
>  }
>  
> +/*
> + *  Predator series turbo button
> + */
> +static int acer_toggle_turbo(void)
> +{
> +	/* Get current state from turbo button */
> +	u64 turbo_led_state, gpu_fan_config1, gpu_fan_config2;

As the buildbot pointed out these should be initialized to 0.


> +	u8 i;
> +
> +	if (ACPI_FAILURE(WMID_gaming_get_u64(&turbo_led_state, ACER_CAP_TURBO_LED)))
> +		return -1;
> +
> +	if (turbo_led_state) {
> +		// turns off turbo led
> +		WMID_gaming_set_u64(0x1, ACER_CAP_TURBO_LED);
> +> +		// set FAN mode to auto

From here...

> +		if (quirks->cpu_fans > 0)
> +			gpu_fan_config2 |= 1;
> +		for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
> +			gpu_fan_config2 |= 1 << (i + 1);
> +		for (i = 0; i < quirks->gpu_fans; ++i)
> +			gpu_fan_config2 |= 1 << (i + 3);
> +		if (quirks->cpu_fans > 0)
> +			gpu_fan_config1 |= 1;
> +		for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
> +			gpu_fan_config1 |= 1 << (2 * i + 2);
> +		for (i = 0; i < quirks->gpu_fans; ++i)
> +			gpu_fan_config1 |= 1 << (2 * i + 6);
> +		WMID_gaming_set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);

Till here...

> +
> +		// set OC to normal
> +		WMID_gaming_set_u64(0x5, ACER_CAP_TURBO_OC);
> +		WMID_gaming_set_u64(0x7, ACER_CAP_TURBO_OC);
> +	} else {
> +		// turn on turbo led
> +		WMID_gaming_set_u64(0x10001, ACER_CAP_TURBO_LED);
> +
> +		// set FAN to turbo mode

The code is identical as the code from here...

> +		if (quirks->cpu_fans > 0)
> +			gpu_fan_config2 |= 1;
> +		for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
> +			gpu_fan_config2 |= 1 << (i + 1);
> +		for (i = 0; i < quirks->gpu_fans; ++i)
> +			gpu_fan_config2 |= 1 << (i + 3);
> +		if (quirks->cpu_fans > 0)
> +			gpu_fan_config1 |= 2;
> +		for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
> +			gpu_fan_config1 |= 2 << (2 * i + 2);
> +		for (i = 0; i < quirks->gpu_fans; ++i)
> +			gpu_fan_config1 |= 2 << (2 * i + 6);
> +		WMID_gaming_set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);

Till here. With the exception of the value being or-ed into gpu_fan_config1, being 1 in the
"// set FAN mode to auto" case and 2 in the "// set FAN to turbo mode" mode case.

Please add a new helper for this like this:

void WMID_gaming_set_fan_mode(u8 value)
{
	u64 gpu_fan_config1 = 0, gpu_fan_config2 = 0;
	int i;

	if (quirks->cpu_fans > 0)
		gpu_fan_config2 |= 1;
	for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
		gpu_fan_config2 |= 1 << (i + 1);
	for (i = 0; i < quirks->gpu_fans; ++i)
		gpu_fan_config2 |= 1 << (i + 3);
	if (quirks->cpu_fans > 0)
		gpu_fan_config1 |= value;
	for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
		gpu_fan_config1 |= value << (2 * i + 2);
	for (i = 0; i < quirks->gpu_fans; ++i)
		gpu_fan_config1 |= value << (2 * i + 6);
	WMID_gaming_set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);
}

And call that in both places to remove the duplication.


Also I just noticed that you are using c++ style '//' comments here. Please use
standard C-style '/* ... */' comments instead like we do every where else in
the drivers/platform/x86 code.

Thanks & Regards,

Hans



> +
> +		// set OC to turbo mode
> +		WMID_gaming_set_u64(0x205, ACER_CAP_TURBO_OC);
> +		WMID_gaming_set_u64(0x207, ACER_CAP_TURBO_OC);
> +	}
> +	return turbo_led_state;
> +}
> +
>  /*
>   * Switch series keyboard dock status
>   */
> @@ -1872,6 +2051,10 @@ static void acer_wmi_notify(u32 value, void *context)
>  		acer_gsensor_event();
>  		acer_kbd_dock_event(&return_value);
>  		break;
> +	case WMID_GAMING_TURBO_KEY_EVENT:
> +		if (return_value.key_num == 0x4)
> +			acer_toggle_turbo();
> +		break;
>  	default:
>  		pr_warn("Unknown function number - %d - %d\n",
>  			return_value.function, return_value.key_num);
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] platform/x86: acer-wmi: Add Turbo Mode support for Acer PH315-53
  2021-08-12  7:59 ` Hans de Goede
@ 2021-08-12 12:01   ` Jafar Akhondali
  0 siblings, 0 replies; 4+ messages in thread
From: Jafar Akhondali @ 2021-08-12 12:01 UTC (permalink / raw)
  To: Hans de Goede; +Cc: jlee, linux-kernel, platform-driver-x86, mgross

Hi Hans,
Thanks for the comments. You are indeed right.
I'll fix them and send v5 ASAP.

Best,

Jafar


On Thu, Aug 12, 2021 at 12:29 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Jafar,
>
> On 8/11/21 1:39 AM, JafarAkhondali wrote:
> > Hi,
> >
> > The Acer Predator Helios series (usually denoted by PHxxx-yy) features
> > a special key above the keyboard named "TURBO". The turbo key does 3
> > things:
> > 1. Set all fan's speeds to TURBO mode
> > 2. Overclocks the CPU and GPU in the safe range
> > 3. Turn on an LED just below the turbo button
> >
> > All of these actions are done by WMI function calls, and there is no
> > custom OC level for turbo. It acts as a flag for enabling turbo
> > mode instead of telling processors to use 1.3x of power.
> >
> > I've run some benchmark tests and it worked fine:
> >
> > GpuTest 0.7.0
> > http://www.geeks3d.com
> >
> > Module: FurMark
> > Normal mode Score: 7289 points (FPS: 121)
> > Turbo mode Score: 7675 points (FPS: 127)
> > Settings:
> > - 1920x1080 fullscreen
> > - antialiasing: Off
> > - duration: 60000 ms
> >
> > Renderer:
> > - GeForce RTX 2060/PCIe/SSE2
> > - OpenGL: 4.6.0 NVIDIA 460.32.03
> >
> > This feature is presented by Acer officially and should not harm
> > hardware in any case.
> >
> > A challenging part of implementing this feature is that calling
> > overclocking the function requires knowing the exact count of fans for CPU
> > and GPU for each model, which to the best of my knowledge is not
> > available in the kernel.
> >
> > So after checking the official PredatorSense application methods, it
> > turned out they have provided the software the list of fans in each model.
> > I have access to the mentioned list, and all similar PH-iii-jj can be
> > added easily by matching "DMI_PRODUCT_NAME".
> >
> > Creating a separate file for the gaming interface was not possible because
> > the current WMI event GUID is needed for the turbo button, and it's not
> > possible to register multiple functions on the same event GUID.
> >
> >
> > Signed-off-by: JafarAkhondali <jafar.akhoondali@gmail.com>
> > ---
> >  Changes in v4:
> >  - Fix Indents
> >  - Make functions return early
>
> This is looking good, the kernel-buildbot (which finally has managed
> to build the patch now that the indents are fixed) found one last
> small thing to fix.
>
> And while looking at that I also found one last small thing to
> improve, see my comments inline / below.
>
> So we will need one more version and then this should be ready to
> get merged.
>
>
>
> >  Changes in v3:
> >  - Remove usages of gaming_interface
> >  - Add ACPI output for u32 buffer length
> >  - Remove set_u64 and get_u64 functions
> >  - Remove unrelated whitespace changes for to this patch
> >
> >  Changes in v2:
> >  - Fix formatting problems
> >
> >
> >  drivers/platform/x86/acer-wmi.c | 183 ++++++++++++++++++++++++++++++++
> >  1 file changed, 183 insertions(+)
> >
> > diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> > index 85db9403cc14..183faa6db4b2 100644
> > --- a/drivers/platform/x86/acer-wmi.c
> > +++ b/drivers/platform/x86/acer-wmi.c
> > @@ -60,6 +60,11 @@ MODULE_LICENSE("GPL");
> >  #define ACER_WMID_GET_THREEG_METHODID                10
> >  #define ACER_WMID_SET_THREEG_METHODID                11
> >
> > +#define ACER_WMID_SET_GAMING_LED_METHODID 2
> > +#define ACER_WMID_GET_GAMING_LED_METHODID 4
> > +#define ACER_WMID_SET_GAMING_FAN_BEHAVIOR 14
> > +#define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
> > +
> >  /*
> >   * Acer ACPI method GUIDs
> >   */
> > @@ -68,6 +73,7 @@ MODULE_LICENSE("GPL");
> >  #define WMID_GUID1           "6AF4F258-B401-42FD-BE91-3D4AC2D7C0D3"
> >  #define WMID_GUID2           "95764E09-FB56-4E83-B31A-37761F60994A"
> >  #define WMID_GUID3           "61EF69EA-865C-4BC3-A502-A0DEBA0CB531"
> > +#define WMID_GUID4           "7A4DDFE7-5B5D-40B4-8595-4408E0CC7F56"
> >
> >  /*
> >   * Acer ACPI event GUIDs
> > @@ -81,6 +87,7 @@ MODULE_ALIAS("wmi:676AA15E-6A47-4D9F-A2CC-1E6D18D14026");
> >  enum acer_wmi_event_ids {
> >       WMID_HOTKEY_EVENT = 0x1,
> >       WMID_ACCEL_OR_KBD_DOCK_EVENT = 0x5,
> > +     WMID_GAMING_TURBO_KEY_EVENT = 0x7,
> >  };
> >
> >  static const struct key_entry acer_wmi_keymap[] __initconst = {
> > @@ -215,6 +222,9 @@ struct hotkey_function_type_aa {
> >  #define ACER_CAP_THREEG                      BIT(4)
> >  #define ACER_CAP_SET_FUNCTION_MODE   BIT(5)
> >  #define ACER_CAP_KBD_DOCK            BIT(6)
> > +#define ACER_CAP_TURBO_OC     BIT(7)
> > +#define ACER_CAP_TURBO_LED     BIT(8)
> > +#define ACER_CAP_TURBO_FAN     BIT(9)
> >
> >  /*
> >   * Interface type flags
> > @@ -301,6 +311,9 @@ struct quirk_entry {
> >       u8 mailled;
> >       s8 brightness;
> >       u8 bluetooth;
> > +     u8 turbo;
> > +     u8 cpu_fans;
> > +     u8 gpu_fans;
> >  };
> >
> >  static struct quirk_entry *quirks;
> > @@ -312,6 +325,10 @@ static void __init set_quirks(void)
> >
> >       if (quirks->brightness)
> >               interface->capability |= ACER_CAP_BRIGHTNESS;
> > +
> > +     if (quirks->turbo)
> > +             interface->capability |= ACER_CAP_TURBO_OC | ACER_CAP_TURBO_LED
> > +                                      | ACER_CAP_TURBO_FAN;
> >  }
> >
> >  static int __init dmi_matched(const struct dmi_system_id *dmi)
> > @@ -340,6 +357,12 @@ static struct quirk_entry quirk_acer_travelmate_2490 = {
> >       .mailled = 1,
> >  };
> >
> > +static struct quirk_entry quirk_acer_predator_ph315_53 = {
> > +     .turbo = 1,
> > +     .cpu_fans = 1,
> > +     .gpu_fans = 1,
> > +};
> > +
> >  /* This AMW0 laptop has no bluetooth */
> >  static struct quirk_entry quirk_medion_md_98300 = {
> >       .wireless = 1,
> > @@ -507,6 +530,15 @@ static const struct dmi_system_id acer_quirks[] __initconst = {
> >               },
> >               .driver_data = &quirk_acer_travelmate_2490,
> >       },
> > +     {
> > +             .callback = dmi_matched,
> > +             .ident = "Acer Predator PH315-53",
> > +             .matches = {
> > +                     DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
> > +                     DMI_MATCH(DMI_PRODUCT_NAME, "Predator PH315-53"),
> > +             },
> > +             .driver_data = &quirk_acer_predator_ph315_53,
> > +     },
> >       {
> >               .callback = set_force_caps,
> >               .ident = "Acer Aspire Switch 10E SW3-016",
> > @@ -1344,6 +1376,93 @@ static struct wmi_interface wmid_v2_interface = {
> >       .type = ACER_WMID_v2,
> >  };
> >
> > +/*
> > + * WMID Gaming interface
> > + */
> > +
> > +static acpi_status
> > +WMI_gaming_execute_u64(u32 method_id, u64 in, u64 *out)
> > +{
> > +     struct acpi_buffer input = { (acpi_size) sizeof(u64), (void *)(&in) };
> > +     struct acpi_buffer result = { ACPI_ALLOCATE_BUFFER, NULL };
> > +     union acpi_object *obj;
> > +     u32 tmp = 0;
> > +     acpi_status status;
> > +
> > +     status = wmi_evaluate_method(WMID_GUID4, 0, method_id, &input, &result);
> > +
> > +     if (ACPI_FAILURE(status))
> > +             return status;
> > +     obj = (union acpi_object *) result.pointer;
> > +
> > +     if (obj) {
> > +             if (obj->type == ACPI_TYPE_BUFFER) {
> > +                     if (obj->buffer.length == sizeof(u32))
> > +                             tmp = *((u32 *) obj->buffer.pointer);
> > +                     else if (obj->buffer.length == sizeof(u64))
> > +                             tmp = *((u64 *) obj->buffer.pointer);
> > +             } else if (obj->type == ACPI_TYPE_INTEGER) {
> > +                     tmp = (u64) obj->integer.value;
> > +             }
> > +     }
> > +
> > +     if (out)
> > +             *out = tmp;
> > +
> > +     kfree(result.pointer);
> > +
> > +     return status;
> > +}
> > +
> > +static acpi_status WMID_gaming_set_u64(u64 value, u32 cap)
> > +{
> > +     u32 method_id = 0;
> > +
> > +     if (!(interface->capability & cap))
> > +             return AE_BAD_PARAMETER;
> > +
> > +     switch (cap) {
> > +     case ACER_CAP_TURBO_LED:
> > +             method_id = ACER_WMID_SET_GAMING_LED_METHODID;
> > +             break;
> > +     case ACER_CAP_TURBO_FAN:
> > +             method_id = ACER_WMID_SET_GAMING_FAN_BEHAVIOR;
> > +             break;
> > +     case ACER_CAP_TURBO_OC:
> > +             method_id = ACER_WMID_SET_GAMING_MISC_SETTING_METHODID;
> > +             break;
> > +     default:
> > +             return AE_BAD_PARAMETER;
> > +     }
> > +
> > +     return WMI_gaming_execute_u64(method_id, value, NULL);
> > +}
> > +
> > +static acpi_status WMID_gaming_get_u64(u64 *value, u32 cap)
> > +{
> > +     acpi_status status;
> > +     u64 result;
> > +     u64 input;
> > +     u32 method_id;
> > +
> > +     if (!(interface->capability & cap))
> > +             return AE_BAD_PARAMETER;
> > +
> > +     switch (cap) {
> > +     case ACER_CAP_TURBO_LED:
> > +             method_id = ACER_WMID_GET_GAMING_LED_METHODID;
> > +             input = 0x1;
> > +             break;
> > +     default:
> > +             return AE_BAD_PARAMETER;
> > +     }
> > +     status = WMI_gaming_execute_u64(method_id, input, &result);
> > +     if (ACPI_SUCCESS(status))
> > +             *value = (u64) result;
> > +
> > +     return status;
> > +}
> > +
> >  /*
> >   * Generic Device (interface-independent)
> >   */
> > @@ -1575,6 +1694,66 @@ static int acer_gsensor_event(void)
> >       return 0;
> >  }
> >
> > +/*
> > + *  Predator series turbo button
> > + */
> > +static int acer_toggle_turbo(void)
> > +{
> > +     /* Get current state from turbo button */
> > +     u64 turbo_led_state, gpu_fan_config1, gpu_fan_config2;
>
> As the buildbot pointed out these should be initialized to 0.
>
>
> > +     u8 i;
> > +
> > +     if (ACPI_FAILURE(WMID_gaming_get_u64(&turbo_led_state, ACER_CAP_TURBO_LED)))
> > +             return -1;
> > +
> > +     if (turbo_led_state) {
> > +             // turns off turbo led
> > +             WMID_gaming_set_u64(0x1, ACER_CAP_TURBO_LED);
> > +> +          // set FAN mode to auto
>
> From here...
>
> > +             if (quirks->cpu_fans > 0)
> > +                     gpu_fan_config2 |= 1;
> > +             for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
> > +                     gpu_fan_config2 |= 1 << (i + 1);
> > +             for (i = 0; i < quirks->gpu_fans; ++i)
> > +                     gpu_fan_config2 |= 1 << (i + 3);
> > +             if (quirks->cpu_fans > 0)
> > +                     gpu_fan_config1 |= 1;
> > +             for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
> > +                     gpu_fan_config1 |= 1 << (2 * i + 2);
> > +             for (i = 0; i < quirks->gpu_fans; ++i)
> > +                     gpu_fan_config1 |= 1 << (2 * i + 6);
> > +             WMID_gaming_set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);
>
> Till here...
>
> > +
> > +             // set OC to normal
> > +             WMID_gaming_set_u64(0x5, ACER_CAP_TURBO_OC);
> > +             WMID_gaming_set_u64(0x7, ACER_CAP_TURBO_OC);
> > +     } else {
> > +             // turn on turbo led
> > +             WMID_gaming_set_u64(0x10001, ACER_CAP_TURBO_LED);
> > +
> > +             // set FAN to turbo mode
>
> The code is identical as the code from here...
>
> > +             if (quirks->cpu_fans > 0)
> > +                     gpu_fan_config2 |= 1;
> > +             for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
> > +                     gpu_fan_config2 |= 1 << (i + 1);
> > +             for (i = 0; i < quirks->gpu_fans; ++i)
> > +                     gpu_fan_config2 |= 1 << (i + 3);
> > +             if (quirks->cpu_fans > 0)
> > +                     gpu_fan_config1 |= 2;
> > +             for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
> > +                     gpu_fan_config1 |= 2 << (2 * i + 2);
> > +             for (i = 0; i < quirks->gpu_fans; ++i)
> > +                     gpu_fan_config1 |= 2 << (2 * i + 6);
> > +             WMID_gaming_set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);
>
> Till here. With the exception of the value being or-ed into gpu_fan_config1, being 1 in the
> "// set FAN mode to auto" case and 2 in the "// set FAN to turbo mode" mode case.
>
> Please add a new helper for this like this:
>
> void WMID_gaming_set_fan_mode(u8 value)
> {
>         u64 gpu_fan_config1 = 0, gpu_fan_config2 = 0;
>         int i;
>
>         if (quirks->cpu_fans > 0)
>                 gpu_fan_config2 |= 1;
>         for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
>                 gpu_fan_config2 |= 1 << (i + 1);
>         for (i = 0; i < quirks->gpu_fans; ++i)
>                 gpu_fan_config2 |= 1 << (i + 3);
>         if (quirks->cpu_fans > 0)
>                 gpu_fan_config1 |= value;
>         for (i = 0; i < (quirks->cpu_fans + quirks->gpu_fans); ++i)
>                 gpu_fan_config1 |= value << (2 * i + 2);
>         for (i = 0; i < quirks->gpu_fans; ++i)
>                 gpu_fan_config1 |= value << (2 * i + 6);
>         WMID_gaming_set_u64(gpu_fan_config2 | gpu_fan_config1 << 16, ACER_CAP_TURBO_FAN);
> }
>
> And call that in both places to remove the duplication.
>
>
> Also I just noticed that you are using c++ style '//' comments here. Please use
> standard C-style '/* ... */' comments instead like we do every where else in
> the drivers/platform/x86 code.
>
> Thanks & Regards,
>
> Hans
>
>
>
> > +
> > +             // set OC to turbo mode
> > +             WMID_gaming_set_u64(0x205, ACER_CAP_TURBO_OC);
> > +             WMID_gaming_set_u64(0x207, ACER_CAP_TURBO_OC);
> > +     }
> > +     return turbo_led_state;
> > +}
> > +
> >  /*
> >   * Switch series keyboard dock status
> >   */
> > @@ -1872,6 +2051,10 @@ static void acer_wmi_notify(u32 value, void *context)
> >               acer_gsensor_event();
> >               acer_kbd_dock_event(&return_value);
> >               break;
> > +     case WMID_GAMING_TURBO_KEY_EVENT:
> > +             if (return_value.key_num == 0x4)
> > +                     acer_toggle_turbo();
> > +             break;
> >       default:
> >               pr_warn("Unknown function number - %d - %d\n",
> >                       return_value.function, return_value.key_num);
> >
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-08-12 12:01 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-10 23:39 [PATCH v4] platform/x86: acer-wmi: Add Turbo Mode support for Acer PH315-53 JafarAkhondali
2021-08-11  4:07 ` kernel test robot
2021-08-12  7:59 ` Hans de Goede
2021-08-12 12:01   ` Jafar Akhondali

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).