diff mbox

[16/16] platform/x86: dell-wmi: Convert to the WMI bus infrastructure

Message ID 4a4da770c8d0af7e3f31e79261b43573d70a6ea6.1495862272.git.dvhart@infradead.org (mailing list archive)
State Accepted, archived
Delegated to: Darren Hart
Headers show

Commit Message

Darren Hart May 27, 2017, 5:31 a.m. UTC
From: Andy Lutomirski <luto@kernel.org>

Move some initialization out of _init and into _probe.
Update signatures and logic to use the wmi bus and device structures.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
[dvhart: drop deprecated sparse_keymap_free, order declarations, add commit msg]
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Mario Limonciello <mario_limonciello@dell.com>
Cc: Pali Rohár <pali.rohar@gmail.com>
Cc: Rafael Wysocki <rjw@rjwysocki.net>
Cc: linux-kernel@vger.kernel.org
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Darren Hart (VMware) <dvhart@infradead.org>
---
 drivers/platform/x86/dell-wmi.c | 136 +++++++++++++++++++++-------------------
 1 file changed, 70 insertions(+), 66 deletions(-)

Comments

Pali Rohár May 27, 2017, 10:50 a.m. UTC | #1
On Saturday 27 May 2017 07:31:30 Darren Hart wrote:
> -	dell_wmi_input_dev->name = "Dell WMI hotkeys";
> -	dell_wmi_input_dev->phys = "wmi/input0";
> -	dell_wmi_input_dev->id.bustype = BUS_HOST;
> +	priv->input_dev->name = "Dell WMI hotkeys";
> +	priv->input_dev->id.bustype = BUS_HOST;

Is not there BUS_WMI, or something like that? (Just asking)
Andy Lutomirski May 27, 2017, 4:04 p.m. UTC | #2
On Sat, May 27, 2017 at 3:50 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Saturday 27 May 2017 07:31:30 Darren Hart wrote:
>> -     dell_wmi_input_dev->name = "Dell WMI hotkeys";
>> -     dell_wmi_input_dev->phys = "wmi/input0";
>> -     dell_wmi_input_dev->id.bustype = BUS_HOST;
>> +     priv->input_dev->name = "Dell WMI hotkeys";
>> +     priv->input_dev->id.bustype = BUS_HOST;
>
> Is not there BUS_WMI, or something like that? (Just asking)
>

Jiri and/or Dmitry, what is bustype for, anyway?  I suppose we could
add BUS_PLATFORM.
Dmitry Torokhov May 27, 2017, 4:17 p.m. UTC | #3
On May 27, 2017 9:04:38 AM PDT, Andy Lutomirski <luto@kernel.org> wrote:
>On Sat, May 27, 2017 at 3:50 AM, Pali Rohár <pali.rohar@gmail.com>
>wrote:
>> On Saturday 27 May 2017 07:31:30 Darren Hart wrote:
>>> -     dell_wmi_input_dev->name = "Dell WMI hotkeys";
>>> -     dell_wmi_input_dev->phys = "wmi/input0";
>>> -     dell_wmi_input_dev->id.bustype = BUS_HOST;
>>> +     priv->input_dev->name = "Dell WMI hotkeys";
>>> +     priv->input_dev->id.bustype = BUS_HOST;
>>
>> Is not there BUS_WMI, or something like that? (Just asking)
>>
>
>Jiri and/or Dmitry, what is bustype for, anyway? 

The bus type could be used to help further  identifying device if it used same vendor/product for spi and i2c, for example, but there are not many if them. I'm not sure if anyone actually makes decisions based on it, but it is part of abi now.

>I suppose we could add BUS_PLATFORM.

What would be the difference from BUS_HOST?


Thanks.
Andy Lutomirski May 27, 2017, 6:40 p.m. UTC | #4
On Sat, May 27, 2017 at 9:17 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On May 27, 2017 9:04:38 AM PDT, Andy Lutomirski <luto@kernel.org> wrote:
>>On Sat, May 27, 2017 at 3:50 AM, Pali Rohár <pali.rohar@gmail.com>
>>wrote:
>>> On Saturday 27 May 2017 07:31:30 Darren Hart wrote:
>>>> -     dell_wmi_input_dev->name = "Dell WMI hotkeys";
>>>> -     dell_wmi_input_dev->phys = "wmi/input0";
>>>> -     dell_wmi_input_dev->id.bustype = BUS_HOST;
>>>> +     priv->input_dev->name = "Dell WMI hotkeys";
>>>> +     priv->input_dev->id.bustype = BUS_HOST;
>>>
>>> Is not there BUS_WMI, or something like that? (Just asking)
>>>
>>
>>Jiri and/or Dmitry, what is bustype for, anyway?
>
> The bus type could be used to help further  identifying device if it used same vendor/product for spi and i2c, for example, but there are not many if them. I'm not sure if anyone actually makes decisions based on it, but it is part of abi now.
>
>>I suppose we could add BUS_PLATFORM.
>
> What would be the difference from BUS_HOST?
>

If BUS_HOST means that the device is part of the host as opposed to
being plugged in, then it seems entirely reasonable.
Dmitry Torokhov May 30, 2017, 2:45 a.m. UTC | #5
On Sat, May 27, 2017 at 11:40:52AM -0700, Andy Lutomirski wrote:
> On Sat, May 27, 2017 at 9:17 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On May 27, 2017 9:04:38 AM PDT, Andy Lutomirski <luto@kernel.org> wrote:
> >>On Sat, May 27, 2017 at 3:50 AM, Pali Rohár <pali.rohar@gmail.com>
> >>wrote:
> >>> On Saturday 27 May 2017 07:31:30 Darren Hart wrote:
> >>>> -     dell_wmi_input_dev->name = "Dell WMI hotkeys";
> >>>> -     dell_wmi_input_dev->phys = "wmi/input0";
> >>>> -     dell_wmi_input_dev->id.bustype = BUS_HOST;
> >>>> +     priv->input_dev->name = "Dell WMI hotkeys";
> >>>> +     priv->input_dev->id.bustype = BUS_HOST;
> >>>
> >>> Is not there BUS_WMI, or something like that? (Just asking)
> >>>
> >>
> >>Jiri and/or Dmitry, what is bustype for, anyway?
> >
> > The bus type could be used to help further  identifying device if it used same vendor/product for spi and i2c, for example, but there are not many if them. I'm not sure if anyone actually makes decisions based on it, but it is part of abi now.
> >
> >>I suppose we could add BUS_PLATFORM.
> >
> > What would be the difference from BUS_HOST?
> >
> 
> If BUS_HOST means that the device is part of the host as opposed to
> being plugged in, then it seems entirely reasonable.

Yes, it basically means platform-specific interface.
Darren Hart June 6, 2017, 3:04 a.m. UTC | #6
On Mon, May 29, 2017 at 07:45:05PM -0700, Dmitry Torokhov wrote:
> On Sat, May 27, 2017 at 11:40:52AM -0700, Andy Lutomirski wrote:
> > On Sat, May 27, 2017 at 9:17 AM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On May 27, 2017 9:04:38 AM PDT, Andy Lutomirski <luto@kernel.org> wrote:
> > >>On Sat, May 27, 2017 at 3:50 AM, Pali Rohár <pali.rohar@gmail.com>
> > >>wrote:
> > >>> On Saturday 27 May 2017 07:31:30 Darren Hart wrote:
> > >>>> -     dell_wmi_input_dev->name = "Dell WMI hotkeys";
> > >>>> -     dell_wmi_input_dev->phys = "wmi/input0";
> > >>>> -     dell_wmi_input_dev->id.bustype = BUS_HOST;
> > >>>> +     priv->input_dev->name = "Dell WMI hotkeys";
> > >>>> +     priv->input_dev->id.bustype = BUS_HOST;
> > >>>
> > >>> Is not there BUS_WMI, or something like that? (Just asking)
> > >>>
> > >>
> > >>Jiri and/or Dmitry, what is bustype for, anyway?
> > >
> > > The bus type could be used to help further  identifying device if it used same vendor/product for spi and i2c, for example, but there are not many if them. I'm not sure if anyone actually makes decisions based on it, but it is part of abi now.
> > >
> > >>I suppose we could add BUS_PLATFORM.
> > >
> > > What would be the difference from BUS_HOST?
> > >
> > 
> > If BUS_HOST means that the device is part of the host as opposed to
> > being plugged in, then it seems entirely reasonable.
> 
> Yes, it basically means platform-specific interface.
> 

I'm going to leave this as BUS_HOST then. Thanks everyone.
diff mbox

Patch

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 8a64c79..badc01e 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -36,6 +36,7 @@ 
 #include <linux/acpi.h>
 #include <linux/string.h>
 #include <linux/dmi.h>
+#include <linux/wmi.h>
 #include <acpi/video.h>
 #include "dell-smbios.h"
 
@@ -53,6 +54,10 @@  static bool wmi_requires_smbios_request;
 MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
 MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
 
+struct dell_wmi_priv {
+	struct input_dev *input_dev;
+};
+
 static int __init dmi_matched(const struct dmi_system_id *dmi)
 {
 	wmi_requires_smbios_request = 1;
@@ -86,7 +91,7 @@  static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
  * notifications (rather than requests for change) or are also sent
  * via the keyboard controller so should not be sent again.
  */
-static const struct key_entry dell_wmi_keymap_type_0000[] __initconst = {
+static const struct key_entry dell_wmi_keymap_type_0000[] = {
 	{ KE_IGNORE, 0x003a, { KEY_CAPSLOCK } },
 
 	/* Key code is followed by brightness level */
@@ -207,7 +212,7 @@  struct dell_dmi_results {
 };
 
 /* Uninitialized entries here are KEY_RESERVED == 0. */
-static const u16 bios_to_linux_keycode[256] __initconst = {
+static const u16 bios_to_linux_keycode[256] = {
 	[0]	= KEY_MEDIA,
 	[1]	= KEY_NEXTSONG,
 	[2]	= KEY_PLAYPAUSE,
@@ -256,7 +261,7 @@  static const u16 bios_to_linux_keycode[256] __initconst = {
  * These are applied if the 0xB2 DMI hotkey table is present and doesn't
  * override them.
  */
-static const struct key_entry dell_wmi_keymap_type_0010[] __initconst = {
+static const struct key_entry dell_wmi_keymap_type_0010[] = {
 	/* Fn-lock */
 	{ KE_IGNORE, 0x151, { KEY_RESERVED } },
 
@@ -289,7 +294,7 @@  static const struct key_entry dell_wmi_keymap_type_0010[] __initconst = {
 /*
  * Keymap for WMI events of type 0x0011
  */
-static const struct key_entry dell_wmi_keymap_type_0011[] __initconst = {
+static const struct key_entry dell_wmi_keymap_type_0011[] = {
 	/* Battery unplugged */
 	{ KE_IGNORE, 0xfff0, { KEY_RESERVED } },
 
@@ -304,13 +309,12 @@  static const struct key_entry dell_wmi_keymap_type_0011[] __initconst = {
 	{ KE_IGNORE, 0x02f6, { KEY_RESERVED } },
 };
 
-static struct input_dev *dell_wmi_input_dev;
-
-static void dell_wmi_process_key(int type, int code)
+static void dell_wmi_process_key(struct wmi_device *wdev, int type, int code)
 {
+	struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
 	const struct key_entry *key;
 
-	key = sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
+	key = sparse_keymap_entry_from_scancode(priv->input_dev,
 						(type << 16) | code);
 	if (!key) {
 		pr_info("Unknown key with type 0x%04x and code 0x%04x pressed\n",
@@ -333,33 +337,18 @@  static void dell_wmi_process_key(int type, int code)
 		dell_laptop_call_notifier(
 			DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED, NULL);
 
-	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
+	sparse_keymap_report_entry(priv->input_dev, key, 1, true);
 }
 
-static void dell_wmi_notify(u32 value, void *context)
+static void dell_wmi_notify(struct wmi_device *wdev,
+			    union acpi_object *obj)
 {
-	struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER, NULL };
-	union acpi_object *obj;
-	acpi_status status;
-	acpi_size buffer_size;
 	u16 *buffer_entry, *buffer_end;
+	acpi_size buffer_size;
 	int len, i;
 
-	status = wmi_get_event_data(value, &response);
-	if (status != AE_OK) {
-		pr_warn("bad event status 0x%x\n", status);
-		return;
-	}
-
-	obj = (union acpi_object *)response.pointer;
-	if (!obj) {
-		pr_warn("no response\n");
-		return;
-	}
-
 	if (obj->type != ACPI_TYPE_BUFFER) {
 		pr_warn("bad response type %x\n", obj->type);
-		kfree(obj);
 		return;
 	}
 
@@ -404,13 +393,14 @@  static void dell_wmi_notify(u32 value, void *context)
 		switch (buffer_entry[1]) {
 		case 0x0000: /* One key pressed or event occurred */
 			if (len > 2)
-				dell_wmi_process_key(0x0000, buffer_entry[2]);
+				dell_wmi_process_key(wdev, 0x0000,
+						     buffer_entry[2]);
 			/* Other entries could contain additional information */
 			break;
 		case 0x0010: /* Sequence of keys pressed */
 		case 0x0011: /* Sequence of events occurred */
 			for (i = 2; i < len; ++i)
-				dell_wmi_process_key(buffer_entry[1],
+				dell_wmi_process_key(wdev, buffer_entry[1],
 						     buffer_entry[i]);
 			break;
 		default: /* Unknown event */
@@ -423,7 +413,6 @@  static void dell_wmi_notify(u32 value, void *context)
 
 	}
 
-	kfree(obj);
 }
 
 static bool have_scancode(u32 scancode, const struct key_entry *keymap, int len)
@@ -437,9 +426,7 @@  static bool have_scancode(u32 scancode, const struct key_entry *keymap, int len)
 	return false;
 }
 
-static void __init handle_dmi_entry(const struct dmi_header *dm,
-				    void *opaque)
-
+static void handle_dmi_entry(const struct dmi_header *dm, void *opaque)
 {
 	struct dell_dmi_results *results = opaque;
 	struct dell_bios_hotkey_table *table;
@@ -509,19 +496,20 @@  static void __init handle_dmi_entry(const struct dmi_header *dm,
 	results->keymap_size = pos;
 }
 
-static int __init dell_wmi_input_setup(void)
+static int dell_wmi_input_setup(struct wmi_device *wdev)
 {
+	struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
 	struct dell_dmi_results dmi_results = {};
 	struct key_entry *keymap;
 	int err, i, pos = 0;
 
-	dell_wmi_input_dev = input_allocate_device();
-	if (!dell_wmi_input_dev)
+	priv->input_dev = input_allocate_device();
+	if (!priv->input_dev)
 		return -ENOMEM;
 
-	dell_wmi_input_dev->name = "Dell WMI hotkeys";
-	dell_wmi_input_dev->phys = "wmi/input0";
-	dell_wmi_input_dev->id.bustype = BUS_HOST;
+	priv->input_dev->name = "Dell WMI hotkeys";
+	priv->input_dev->id.bustype = BUS_HOST;
+	priv->input_dev->dev.parent = &wdev->dev;
 
 	if (dmi_walk(handle_dmi_entry, &dmi_results)) {
 		/*
@@ -596,7 +584,7 @@  static int __init dell_wmi_input_setup(void)
 
 	keymap[pos].type = KE_END;
 
-	err = sparse_keymap_setup(dell_wmi_input_dev, keymap, NULL);
+	err = sparse_keymap_setup(priv->input_dev, keymap, NULL);
 	/*
 	 * Sparse keymap library makes a copy of keymap so we don't need the
 	 * original one that was allocated.
@@ -605,17 +593,24 @@  static int __init dell_wmi_input_setup(void)
 	if (err)
 		goto err_free_dev;
 
-	err = input_register_device(dell_wmi_input_dev);
+	err = input_register_device(priv->input_dev);
 	if (err)
 		goto err_free_dev;
 
 	return 0;
 
  err_free_dev:
-	input_free_device(dell_wmi_input_dev);
+	input_free_device(priv->input_dev);
 	return err;
 }
 
+static void dell_wmi_input_destroy(struct wmi_device *wdev)
+{
+	struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
+
+	input_unregister_device(priv->input_dev);
+}
+
 /*
  * Descriptor buffer is 128 byte long and contains:
  *
@@ -714,46 +709,55 @@  static int dell_wmi_events_set_enabled(bool enable)
 	return dell_smbios_error(ret);
 }
 
+static int dell_wmi_probe(struct wmi_device *wdev)
+{
+	struct dell_wmi_priv *priv = devm_kzalloc(
+		&wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
+
+	dev_set_drvdata(&wdev->dev, priv);
+
+	return dell_wmi_input_setup(wdev);
+}
+
+static int dell_wmi_remove(struct wmi_device *wdev)
+{
+	dell_wmi_input_destroy(wdev);
+	return 0;
+}
+static const struct wmi_device_id dell_wmi_id_table[] = {
+	{ .guid_string = DELL_EVENT_GUID },
+	{ },
+};
+
+static struct wmi_driver dell_wmi_driver = {
+	.driver = {
+		.name = "dell-wmi",
+	},
+	.id_table = dell_wmi_id_table,
+	.probe = dell_wmi_probe,
+	.remove = dell_wmi_remove,
+	.notify = dell_wmi_notify,
+};
+
 static int __init dell_wmi_init(void)
 {
 	int err;
-	acpi_status status;
-
-	if (!wmi_has_guid(DELL_EVENT_GUID) ||
-	    !wmi_has_guid(DELL_DESCRIPTOR_GUID)) {
-		pr_warn("Dell WMI GUID were not found\n");
-		return -ENODEV;
-	}
 
 	err = dell_wmi_check_descriptor_buffer();
 	if (err)
 		return err;
 
-	err = dell_wmi_input_setup();
-	if (err)
-		return err;
-
-	status = wmi_install_notify_handler(DELL_EVENT_GUID,
-					 dell_wmi_notify, NULL);
-	if (ACPI_FAILURE(status)) {
-		input_unregister_device(dell_wmi_input_dev);
-		pr_err("Unable to register notify handler - %d\n", status);
-		return -ENODEV;
-	}
-
 	dmi_check_system(dell_wmi_smbios_list);
 
 	if (wmi_requires_smbios_request) {
 		err = dell_wmi_events_set_enabled(true);
 		if (err) {
 			pr_err("Failed to enable WMI events\n");
-			wmi_remove_notify_handler(DELL_EVENT_GUID);
-			input_unregister_device(dell_wmi_input_dev);
 			return err;
 		}
 	}
 
-	return 0;
+	return wmi_driver_register(&dell_wmi_driver);
 }
 module_init(dell_wmi_init);
 
@@ -761,7 +765,7 @@  static void __exit dell_wmi_exit(void)
 {
 	if (wmi_requires_smbios_request)
 		dell_wmi_events_set_enabled(false);
-	wmi_remove_notify_handler(DELL_EVENT_GUID);
-	input_unregister_device(dell_wmi_input_dev);
+
+	wmi_driver_unregister(&dell_wmi_driver);
 }
 module_exit(dell_wmi_exit);