Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <adba7928-1ca7-4076-ba8f-32276b8fea5c@redhat.com>
Date:   Mon, 27 Nov 2023 12:43:33 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 1/3] platform/x86: acer-wmi: Add platform profile and
 mode key support for Predator PHN16-71
Content-Language: en-US
To:     SungHwan Jung <onenowy@gmail.com>,
        =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        "Lee, Chun-Yi" <jlee@suse.com>, Jean Delvare <jdelvare@suse.com>,
        Guenter Roeck <linux@roeck-us.net>
Cc:     platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hwmon@vger.kernel.org
References: <20231124094122.100707-1-onenowy@gmail.com>
 <20231124094122.100707-2-onenowy@gmail.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20231124094122.100707-2-onenowy@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

Thank you for your patch.

On 11/24/23 10:41, SungHwan Jung wrote:
>  The Acer Predator PHN16-71 has the mode key that is used to rotate
> thermal modes or toggle turbo mode with predator sense app (ver. 4) on
> windows.
> This patch includes platform profile and the mode key support for the
> device and also includes a small fix for "WMI_gaming_execute_u64"
> function.
> 
> Signed-off-by: SungHwan Jung <onenowy@gmail.com>
> ---
>  drivers/platform/x86/acer-wmi.c | 268 +++++++++++++++++++++++++++++++-
>  1 file changed, 267 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index 0e472aa9b..e3650dce0 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -20,6 +20,7 @@
>  #include <linux/backlight.h>
>  #include <linux/leds.h>
>  #include <linux/platform_device.h>
> +#include <linux/platform_profile.h>
>  #include <linux/acpi.h>
>  #include <linux/i8042.h>
>  #include <linux/rfkill.h>
> @@ -62,9 +63,12 @@ MODULE_LICENSE("GPL");
>  
>  #define ACER_WMID_SET_GAMING_LED_METHODID 2
>  #define ACER_WMID_GET_GAMING_LED_METHODID 4
> +#define ACER_WMID_GET_GAMING_SYS_INFO_METHODID 5
>  #define ACER_WMID_SET_GAMING_FAN_BEHAVIOR 14
>  #define ACER_WMID_SET_GAMING_MISC_SETTING_METHODID 22
>  
> +#define ACER_PREDATOR_V4_THERMAL_PROFILE_EC_OFFSET 0x54
> +
>  /*
>   * Acer ACPI method GUIDs
>   */
> @@ -90,6 +94,10 @@ enum acer_wmi_event_ids {
>  	WMID_GAMING_TURBO_KEY_EVENT = 0x7,
>  };
>  
> +enum acer_wmi_predator_v4_sys_info_command {
> +	ACER_WMID_CMD_GET_PREDATOR_V4_BAT_STATUS = 0x02,
> +};
> +
>  static const struct key_entry acer_wmi_keymap[] __initconst = {
>  	{KE_KEY, 0x01, {KEY_WLAN} },     /* WiFi */
>  	{KE_KEY, 0x03, {KEY_WLAN} },     /* WiFi */
> @@ -232,6 +240,7 @@ struct hotkey_function_type_aa {
>  #define ACER_CAP_TURBO_OC     BIT(7)
>  #define ACER_CAP_TURBO_LED     BIT(8)
>  #define ACER_CAP_TURBO_FAN     BIT(9)
> +#define ACER_CAP_PLATFORM_PROFILE BIT(10)

When Ilpo set to use tabs for indent here, he meant to do
this in this patch too, I have fixed this up while merging the patch.

I have also found several places where you separate blocks
by multiple empty lines instead of just 1 empty line,
I have also fixed this (reducing things to 1 empty line)
while applying this.

<snip>


> @@ -2245,6 +2499,12 @@ static int acer_platform_probe(struct platform_device *device)
>  	if (err)
>  		goto error_rfkill;
>  
> +	if (has_cap(ACER_CAP_PLATFORM_PROFILE)) {
> +		err = acer_platform_profile_setup();
> +		if (err)
> +			goto error_platform_profile;
> +	}
> +
>  	return err;
>  
>  error_rfkill:

The error handling here is wrong, on error you need to undo all previous steps,
so your error handling needs to add a label above the first (error_rfkill)
label and then undo the last step before the platform-profile-setup,
specifically call acer_rfkill_exit(); from the new label. IOW the code here
should look like this:

        if (has_cap(ACER_CAP_PLATFORM_PROFILE)) {
                err = acer_platform_profile_setup();
                if (err)
                        goto error_platform_profile;
        }

        return err;

error_platform_profile:
        acer_rfkill_exit();
error_rfkill:
        if (has_cap(ACER_CAP_BRIGHTNESS))
                acer_backlight_exit();
	...

I have also fixed this up while merging this patch:

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans