Message-ID: <74ac9bf130074e0a8f86a7904783d091@ausx13mpc120.AMER.DELL.COM>
Date:   Fri, 29 Jun 2018 16:44:26 +0000
From:   <Mario.Limonciello@...l.com>
To:     <srinivas.pandruvada@...ux.intel.com>, <alex.hung@...onical.com>,
        <dvhart@...radead.org>, <andy@...radead.org>
CC:     <platform-driver-x86@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <rjw@...ysocki.net>
Subject: RE: [PATCH v2] platform/x86: intel-hid: Add support for Device
 Specific Methods

> -----Original Message-----
> From: Srinivas Pandruvada [mailto:srinivas.pandruvada@...ux.intel.com]
> Sent: Thursday, June 28, 2018 1:19 PM
> To: alex.hung@...onical.com; dvhart@...radead.org; andy@...radead.org
> Cc: platform-driver-x86@...r.kernel.org; linux-kernel@...r.kernel.org;
> rjw@...ysocki.net; Limonciello, Mario; Srinivas Pandruvada
> Subject: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific
> Methods
> 
> In some of the recent platforms, it is possible that stand alone methods
> for HEBC() or other methods used in this driver may not exist. In this
> case intel-hid driver will fail to load and power button will not be
> functional.
> 
> It is also possible that some quirks in this driver added for some
> platforms may have same issue in loading intel-hid driver.
> 
> There is an update to the ACPI details for the HID event filter driver.
> In the updated specification a _DSM is added, which has separate function
> indexes for each of the previous stand alone methods.
> 
> This change brings in support for the _DSM and allows usage of function
> index for corresponding stand alone methods.
> 
> Details of Device Specific Method:
> 
> Intel HID Event Filter Driver _DSM UUID:
> eeec56b3-4442-408f-a792-4edd4d758054
> 
> • Function index 0: Returns a buffer with a bit-field representing the
> supported function IDs.
> 
> Function Index	ASL Object
> --------------------------------
> 1		BTNL
> 2		HDMM
> 3		HDSM
> 4		HDEM
> 5		BTNS
> 6		BTNE
> 7		HEBC
> 8		VGBS
> 9		HEBC
> 
> One significant change is to query the supported methods implemented on
> the platform. So the previous HEBC() has two variants. HEBC v1 and
> HEBC v2. The v2 version allowed further define which of the 5-button
> are actually defined by the platform. HEBC v2 support is only available
> via new DSM.
> 
> v1 Button details:
> Bits [0] - Rotation Lock, Num Lock, Home, End, Page Up,
> Page Down
> Bits [1] - Wireless Radio Control
> Bits [2] - System Power Down
> Bits [3] - System Hibernate
> Bits [4] - System Sleep/ System Wake
> Bits [5] - Scan Next Track
> Bits [6] - Scan Previous Track
> Bits [7] - Stop
> Bits [8] - Play/Pause
> Bits [9] - Mute
> Bits [10] - Volume Increment
> Bits [11] - Volume Decrement
> Bits [12] - Display Brightness Increment
> Bits [13] - Display Brightness Decrement
> Bits [14] - Lock Tablet
> Bits [15] - Release Tablet
> Bits [16] - Toggle Bezel
> Bits [17] - 5 button array
> Bits [18-31] - reserved
> 
> v2 Buttom details:
> Bits [0-16] - Same as v1 version
> Bits [17] - 5 button array
> Bits [18] – Power Button
> Bits [19] - W Home Button
> Bits [20] - Volume Up Button
> Bits [21] - Volume Down Button
> Bits [22] – Rotation Lock Button
> Bits [23-31] – reserved
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>

I verified this on XPS 9370 FW 1.3.2 (which contains this alternate HEBC interface).
Power button works again for wakeup from s2idle.

Tested-by: Mario Limonciello <mario.limonciello@...l.com>

> ---
> Accidently sent the patch without change of version, so please ignore
> the previous one sent today.
> v2:
> 	Minor changes suggested by Andy
> 
>  drivers/platform/x86/intel-hid.c | 178 ++++++++++++++++++++++++++++++++++----
> -
>  1 file changed, 157 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
> index b5adba227783..6cf9b7fa5bf0 100644
> --- a/drivers/platform/x86/intel-hid.c
> +++ b/drivers/platform/x86/intel-hid.c
> @@ -96,13 +96,140 @@ struct intel_hid_priv {
>  	bool wakeup_mode;
>  };
> 
> -static int intel_hid_set_enable(struct device *device, bool enable)
> +#define HID_EVENT_FILTER_UUID	"eeec56b3-4442-408f-a792-
> 4edd4d758054"
> +
> +enum intel_hid_dsm_fn_codes {
> +	INTEL_HID_DSM_FN_INVALID,
> +	INTEL_HID_DSM_BTNL_FN,
> +	INTEL_HID_DSM_HDMM_FN,
> +	INTEL_HID_DSM_HDSM_FN,
> +	INTEL_HID_DSM_HDEM_FN,
> +	INTEL_HID_DSM_BTNS_FN,
> +	INTEL_HID_DSM_BTNE_FN,
> +	INTEL_HID_DSM_HEBC_V1_FN,
> +	INTEL_HID_DSM_VGBS_FN,
> +	INTEL_HID_DSM_HEBC_V2_FN,
> +	INTEL_HID_DSM_FN_MAX
> +};
> +
> +static const char *intel_hid_dsm_fn_to_method[INTEL_HID_DSM_FN_MAX] = {
> +	NULL,
> +	"BTNL",
> +	"HDMM",
> +	"HDSM",
> +	"HDEM",
> +	"BTNS",
> +	"BTNE",
> +	"HEBC",
> +	"VGBS",
> +	"HEBC"
> +};
> +
> +static unsigned long long intel_hid_dsm_fn_mask;
> +static guid_t intel_dsm_guid;
> +
> +static bool intel_hid_execute_method(acpi_handle handle,
> +				     enum intel_hid_dsm_fn_codes fn_index,
> +				     unsigned long long arg)
>  {
> +	union acpi_object *obj, argv4, req;
>  	acpi_status status;
> +	char *method_name;
> 
> -	status = acpi_execute_simple_method(ACPI_HANDLE(device), "HDSM",
> -					    enable);
> -	if (ACPI_FAILURE(status)) {
> +	if (fn_index <= INTEL_HID_DSM_FN_INVALID ||
> +	    fn_index >= INTEL_HID_DSM_FN_MAX)
> +		return false;
> +
> +	method_name = (char *)intel_hid_dsm_fn_to_method[fn_index];
> +
> +	if (!(intel_hid_dsm_fn_mask & fn_index))
> +		goto skip_dsm_exec;
> +
> +	/* All methods expects a package with one integer element */
> +	req.type = ACPI_TYPE_INTEGER;
> +	req.integer.value = arg;
> +
> +	argv4.type = ACPI_TYPE_PACKAGE;
> +	argv4.package.count = 1;
> +	argv4.package.elements = &req;
> +
> +	obj = acpi_evaluate_dsm(handle, &intel_dsm_guid, 1, fn_index, &argv4);
> +	if (obj) {
> +		acpi_handle_debug(handle, "Exec DSM Fn code: %d[%s]
> success\n",
> +				  fn_index, method_name);
> +		ACPI_FREE(obj);
> +		return true;
> +	}
> +
> +skip_dsm_exec:
> +	status = acpi_execute_simple_method(handle, method_name, arg);
> +	if (ACPI_SUCCESS(status))
> +		return true;
> +
> +	return false;
> +}
> +
> +static bool intel_hid_evaluate_method(acpi_handle handle,
> +				      enum intel_hid_dsm_fn_codes fn_index,
> +				      unsigned long long *result)
> +{
> +	union acpi_object *obj;
> +	acpi_status status;
> +	char *method_name;
> +
> +	if (fn_index <= INTEL_HID_DSM_FN_INVALID ||
> +	    fn_index >= INTEL_HID_DSM_FN_MAX)
> +		return false;
> +
> +	method_name = (char *)intel_hid_dsm_fn_to_method[fn_index];
> +
> +	if (!(intel_hid_dsm_fn_mask & fn_index))
> +		goto skip_dsm_eval;
> +
> +	obj = acpi_evaluate_dsm_typed(handle, &intel_dsm_guid,
> +				      1, fn_index,
> +				      NULL,  ACPI_TYPE_INTEGER);
> +	if (obj) {
> +		*result = obj->integer.value;
> +		acpi_handle_debug(handle,
> +				  "Eval DSM Fn code: %d[%s] results: 0x%llx\n",
> +				  fn_index, method_name, *result);
> +		ACPI_FREE(obj);
> +		return true;
> +	}
> +
> +skip_dsm_eval:
> +	status = acpi_evaluate_integer(handle, method_name, NULL, result);
> +	if (ACPI_SUCCESS(status))
> +		return true;
> +
> +	return false;
> +}
> +
> +static void intel_hid_init_dsm(acpi_handle handle)
> +{
> +	union acpi_object *obj;
> +
> +	guid_parse(HID_EVENT_FILTER_UUID, &intel_dsm_guid);
> +
> +	obj = acpi_evaluate_dsm_typed(handle, &intel_dsm_guid, 1, 0, NULL,
> +				      ACPI_TYPE_BUFFER);
> +	if (obj) {
> +		intel_hid_dsm_fn_mask = *obj->buffer.pointer;
> +		ACPI_FREE(obj);
> +	}
> +
> +	acpi_handle_debug(handle, "intel_hid_dsm_fn_mask = %llx\n",
> +			  intel_hid_dsm_fn_mask);
> +}
> +
> +static int intel_hid_set_enable(struct device *device, bool enable)
> +{
> +	acpi_handle handle = ACPI_HANDLE(device);
> +
> +	/* Enable|disable features - power button is always enabled */
> +	if (!intel_hid_execute_method(handle, INTEL_HID_DSM_HDSM_FN,
> +				      enable)) {
>  		dev_warn(device, "failed to %sable hotkeys\n",
>  			 enable ? "en" : "dis");
>  		return -EIO;
> @@ -129,9 +256,8 @@ static void intel_button_array_enable(struct device
> *device, bool enable)
>  	}
> 
>  	/* Enable|disable features - power button is always enabled */
> -	status = acpi_execute_simple_method(handle, "BTNE",
> -					    enable ? button_cap : 1);
> -	if (ACPI_FAILURE(status))
> +	if (!intel_hid_execute_method(handle, INTEL_HID_DSM_BTNE_FN,
> +				      enable ? button_cap : 1))
>  		dev_warn(device, "failed to set button capability\n");
>  }
> 
> @@ -217,7 +343,6 @@ static void notify_handler(acpi_handle handle, u32 event,
> void *context)
>  	struct platform_device *device = context;
>  	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
>  	unsigned long long ev_index;
> -	acpi_status status;
> 
>  	if (priv->wakeup_mode) {
>  		/*
> @@ -269,8 +394,8 @@ static void notify_handler(acpi_handle handle, u32 event,
> void *context)
>  		return;
>  	}
> 
> -	status = acpi_evaluate_integer(handle, "HDEM", NULL, &ev_index);
> -	if (ACPI_FAILURE(status)) {
> +	if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_HDEM_FN,
> +				       &ev_index)) {
>  		dev_warn(&device->dev, "failed to get event index\n");
>  		return;
>  	}
> @@ -284,17 +409,24 @@ static bool button_array_present(struct platform_device
> *device)
>  {
>  	acpi_handle handle = ACPI_HANDLE(&device->dev);
>  	unsigned long long event_cap;
> -	acpi_status status;
> -	bool supported = false;
> 
> -	status = acpi_evaluate_integer(handle, "HEBC", NULL, &event_cap);
> -	if (ACPI_SUCCESS(status) && (event_cap & 0x20000))
> -		supported = true;
> +	if (intel_hid_evaluate_method(handle, INTEL_HID_DSM_HEBC_V2_FN,
> +				      &event_cap)) {
> +		/* Check presence of 5 button array or v2 power button */
> +		if (event_cap & 0x60000)
> +			return true;
> +	}
> +
> +	if (intel_hid_evaluate_method(handle, INTEL_HID_DSM_HEBC_V1_FN,
> +				      &event_cap)) {
> +		if (event_cap & 0x20000)
> +			return true;
> +	}
> 
>  	if (dmi_check_system(button_array_table))
> -		supported = true;
> +		return true;
> 
> -	return supported;
> +	return false;
>  }
> 
>  static int intel_hid_probe(struct platform_device *device)
> @@ -305,8 +437,9 @@ static int intel_hid_probe(struct platform_device *device)
>  	acpi_status status;
>  	int err;
> 
> -	status = acpi_evaluate_integer(handle, "HDMM", NULL, &mode);
> -	if (ACPI_FAILURE(status)) {
> +	intel_hid_init_dsm(handle);
> +
> +	if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_HDMM_FN,
> &mode)) {
>  		dev_warn(&device->dev, "failed to read mode\n");
>  		return -ENODEV;
>  	}
> @@ -352,13 +485,16 @@ static int intel_hid_probe(struct platform_device
> *device)
>  		goto err_remove_notify;
> 
>  	if (priv->array) {
> +		unsigned long long dummy;
> +
>  		intel_button_array_enable(&device->dev, true);
> 
>  		/* Call button load method to enable HID power button */
> -		status = acpi_evaluate_object(handle, "BTNL", NULL, NULL);
> -		if (ACPI_FAILURE(status))
> +		if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_BTNL_FN,
> +					       &dummy)) {
>  			dev_warn(&device->dev,
>  				 "failed to enable HID power button\n");
> +		}
>  	}
> 
>  	device_init_wakeup(&device->dev, true);
> --
> 2.14.1

Powered by blists - more mailing lists