From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1949900AbcBTBZG (ORCPT ); Fri, 19 Feb 2016 20:25:06 -0500 Received: from bombadil.infradead.org ([198.137.202.9]:36098 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1424612AbcBTBZD (ORCPT ); Fri, 19 Feb 2016 20:25:03 -0500 Date: Fri, 19 Feb 2016 17:24:57 -0800 From: Darren Hart To: =?utf-8?B?TWljaGHFgiBLxJlwaWXFhA==?= Cc: Matthew Garrett , Pali =?iso-8859-1?Q?Roh=E1r?= , Darek Stojaczyk , platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131 Message-ID: <20160220012457.GC23707@dvhart-mobl5.amr.corp.intel.com> References: <20160121090401.GR7192@pali> <1455634230-1487-1-git-send-email-kernel@kempniu.pl> <1455634230-1487-4-git-send-email-kernel@kempniu.pl> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1455634230-1487-4-git-send-email-kernel@kempniu.pl> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Feb 16, 2016 at 03:50:28PM +0100, Michał Kępień wrote: > On some laptop models (e.g. Dell Vostro V131), WMI events are not > generated until a specific SMBIOS request is issued to register an event > listener [1]. As there seems to be no ACPI method or SMBIOS request to > determine without possible side effects whether a given machine needs to > issue this SMBIOS request in order to receive WMI events, DMI matching > is used to whitelist the models which need it. > > [1] https://lists.us.dell.com/pipermail/libsmbios-devel/2015-July/000612.html > > Signed-off-by: Michał Kępień > --- > drivers/platform/x86/Kconfig | 1 + > drivers/platform/x86/dell-wmi.c | 49 +++++++++++++++++++++++++++++++++++++++ > 2 files changed, 50 insertions(+) > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 3e4d9c3..5ceb53a 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -122,6 +122,7 @@ config DELL_WMI > depends on ACPI_WMI > depends on INPUT > depends on ACPI_VIDEO || ACPI_VIDEO = n > + depends on DELL_SMBIOS > select INPUT_SPARSEKMAP > ---help--- > Say Y here if you want to support WMI-based hotkeys on Dell laptops. > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c > index 368e193..ca8233a 100644 > --- a/drivers/platform/x86/dell-wmi.c > +++ b/drivers/platform/x86/dell-wmi.c > @@ -37,6 +37,7 @@ > #include > #include > #include > +#include "dell-smbios.h" > > MODULE_AUTHOR("Matthew Garrett "); > MODULE_AUTHOR("Pali Rohár "); > @@ -47,10 +48,29 @@ MODULE_LICENSE("GPL"); > #define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492" > > static u32 dell_wmi_interface_version; > +static bool wmi_requires_smbios_request; > > MODULE_ALIAS("wmi:"DELL_EVENT_GUID); > MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID); > > +static int __init dmi_matched(const struct dmi_system_id *dmi) > +{ > + wmi_requires_smbios_request = 1; > + return 1; > +} > + > +static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = { > + { > + .callback = dmi_matched, > + .ident = "Dell Vostro V131", > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), > + DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"), > + }, > + }, > + { } > +}; > + > /* > * Certain keys are flagged as KE_IGNORE. All of these are either > * notifications (rather than requests for change) or are also sent > @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void) > { > int err; > acpi_status status; > + struct calling_interface_buffer *buffer; Please place the longest line first, and move int err to the last declaration. When changing declarations of local variables, please use "Reverse Christmas Tree" order (longest line to shortest line) wherever possible. > > if (!wmi_has_guid(DELL_EVENT_GUID) || > !wmi_has_guid(DELL_DESCRIPTOR_GUID)) { > @@ -538,12 +559,40 @@ static int __init dell_wmi_init(void) > return -ENODEV; > } > > + dmi_check_system(dell_wmi_smbios_list); > + > + if (wmi_requires_smbios_request) { > + buffer = dell_smbios_get_buffer(); > + buffer->input[0] = 0x10000; > + buffer->input[1] = 0x51534554; > + buffer->input[3] = 0x1; > + dell_smbios_send_request(17, 3); > + err = buffer->output[0]; > + dell_smbios_release_buffer(); > + if (err) { > + pr_err("Failed to enable WMI (error %d)\n", err); > + wmi_remove_notify_handler(DELL_EVENT_GUID); > + dell_wmi_input_destroy(); > + return dell_smbios_error(err); > + } > + } > + > return 0; > } > module_init(dell_wmi_init); > > static void __exit dell_wmi_exit(void) > { > + struct calling_interface_buffer *buffer; > + > + if (wmi_requires_smbios_request) { > + buffer = dell_smbios_get_buffer(); > + buffer->input[0] = 0x10000; > + buffer->input[1] = 0x51534554; > + dell_smbios_send_request(17, 3); > + dell_smbios_release_buffer(); > + } > + Pali's point about documenting the hardcoded values and eliminating the code duplication with a function (inline) is a good one. Otherwise, this series looks good to me. Looking forward to merging v4. -- Darren Hart Intel Open Source Technology Center