RE: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

From: Mario.Limonciello
Date: Mon Jul 02 2018 - 09:51:24 EST

Next message: Geert Uytterhoeven: "Re: [PATCH] m68k/mac: Enable PDMA for PowerBook 500 series"
Previous message: David Miller: "Re: [PATCH V2 net-next 0/9] net: hns3: a few code improvements"
In reply to: Andy Shevchenko: "Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods"
Next in thread: Andy Shevchenko: "Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@xxxxxxxxx]
> Sent: Monday, July 2, 2018 7:41 AM
> To: Srinivas Pandruvada
> Cc: Limonciello, Mario; Alex Hung; Darren Hart; Andy Shevchenko; Platform Driver;
> Linux Kernel Mailing List; Rafael J. Wysocki
> Subject: Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific
> Methods
>
> On Fri, Jun 29, 2018 at 9:41 PM, Srinivas Pandruvada
> <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> > On Fri, 2018-06-29 at 16:44 +0000, Mario.Limonciello@xxxxxxxx wrote:
> >> >
> >
> > [...]
> >
> >> 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@xxxxxxxx>
> >>
> > So there are some customers who will have issue with power button
> > without this patch, so it should be also marked for stable also.
> > Also this may be a candidate for 4.18-rcX.
> >
>
> I'm not sure Greg will take this selling point for rather big patch.
> From changelog, honestly, I don't see any regression description,
> looks like "it wasn't working before change anyway".
>

Just for adding some context.

Some platforms have moved to different interface in ASL in FW upgrade
due to deficiencies/bugs present with old interface. So yes it's platform FW
change in behavior that leads to Linux kernel regression. Windows driver has supported
both interfaces for a long time. Linux kernel however doesn't support this interface until now.

> For now, I pushed this to my review and testing queue as is, thanks!

If not stable I think it would at least be ideal to try to bring this to 4.18-rcX if possible for
compatibility with more platforms that will come with this other interface instead.

>
> > Thanks,
> > Srinivas
> >
> >> > ---
> >> > 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
> >>
> >>
>
>
>
> --
> With Best Regards,
> Andy Shevchenko

Next message: Geert Uytterhoeven: "Re: [PATCH] m68k/mac: Enable PDMA for PowerBook 500 series"
Previous message: David Miller: "Re: [PATCH V2 net-next 0/9] net: hns3: a few code improvements"
In reply to: Andy Shevchenko: "Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods"
Next in thread: Andy Shevchenko: "Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]