[PATCH 01/12] HID: wiimote: Add Nintendo Wii Remote driver stub

Subsystems: hid core layer, the rest, wiimote hid driver

STALE5530d

19 messages, 3 authors, 2011-07-11 · open the first message on its own page

[PATCH 01/12] HID: wiimote: Add Nintendo Wii Remote driver stub

From: David Herrmann <hidden>
Date: 2011-06-27 14:30:50

Add stub driver for the Nintendo Wii Remote. The wii remote uses
the HID protocol to communicate with the host over bluetooth. Hence,
add dependency for HIDP and place driver in hid subsystem.

Signed-off-by: David Herrmann <redacted>
---
 drivers/hid/Kconfig       |    6 ++++++
 drivers/hid/Makefile      |    1 +
 drivers/hid/hid-wiimote.c |   32 ++++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hid/hid-wiimote.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 67d2a75..e84bdd8 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig

@@ -567,6 +567,12 @@ config HID_WACOM_POWER_SUPPLY
 	  Say Y here if you want to enable power supply status monitoring for
 	  Wacom Bluetooth devices.
 
+config HID_WIIMOTE
+	tristate "Nintendo Wii Remote support"
+	depends on BT_HIDP
+	---help---
+	Support for the Nintendo Wii Remote bluetooth device.
+
 config HID_ZEROPLUS
 	tristate "Zeroplus based game controller support"
 	depends on USB_HID

diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index f8cc4ea..4ed9bed 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile

@@ -73,6 +73,7 @@ obj-$(CONFIG_HID_ZEROPLUS)	+= hid-zpff.o
 obj-$(CONFIG_HID_ZYDACRON)	+= hid-zydacron.o
 obj-$(CONFIG_HID_WACOM)		+= hid-wacom.o
 obj-$(CONFIG_HID_WALTOP)	+= hid-waltop.o
+obj-$(CONFIG_HID_WIIMOTE)	+= hid-wiimote.o
 
 obj-$(CONFIG_USB_HID)		+= usbhid/
 obj-$(CONFIG_USB_MOUSE)		+= usbhid/

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
new file mode 100644
index 0000000..8a770e6
--- /dev/null
+++ b/drivers/hid/hid-wiimote.c

@@ -0,0 +1,32 @@
+/*
+ * HID driver for Nintendo Wiimote devices
+ * Copyright (c) 2011 David Herrmann
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include <linux/module.h>
+
+#define WIIMOTE_VERSION "0.1"
+#define WIIMOTE_NAME "Nintendo Wii Remote"
+
+static int __init wiimote_init(void)
+{
+	return 0;
+}
+
+static void __exit wiimote_exit(void)
+{
+}
+
+module_init(wiimote_init);
+module_exit(wiimote_exit);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("David Herrmann <dh.herrmann@gmail.com>");
+MODULE_DESCRIPTION(WIIMOTE_NAME " Device Driver");
+MODULE_VERSION(WIIMOTE_VERSION);

-- 
1.7.5.2

[PATCH 03/12] HID: wiimote: Add wiimote device structure

From: David Herrmann <hidden>
Date: 2011-06-27 14:30:53

Allocate wiimote device structure with all wiimote related data
when registering new wiimote devices.

Signed-off-by: David Herrmann <redacted>
---
 drivers/hid/hid-wiimote.c |   41 +++++++++++++++++++++++++++++++++++++++--
 1 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index ed4fe18..ff7cf12 100644
--- a/drivers/hid/hid-wiimote.c
+++ b/drivers/hid/hid-wiimote.c

@@ -17,6 +17,10 @@
 #define WIIMOTE_VERSION "0.1"
 #define WIIMOTE_NAME "Nintendo Wii Remote"
 
+struct wiimote_data {
+	struct hid_device *hdev;
+};
+
 static int wiimote_hid_event(struct hid_device *hdev, struct hid_report *report,
 							u8 *raw_data, int size)
 {

@@ -26,31 +30,64 @@ static int wiimote_hid_event(struct hid_device *hdev, struct hid_report *report,
 	return 0;
 }
 
+static struct wiimote_data *wiimote_create(struct hid_device *hdev)
+{
+	struct wiimote_data *wdata;
+
+	wdata = kzalloc(sizeof(*wdata), GFP_KERNEL);
+	if (!wdata)
+		return NULL;
+
+	wdata->hdev = hdev;
+	hid_set_drvdata(hdev, wdata);
+
+	return wdata;
+}
+
+static void wiimote_destroy(struct wiimote_data *wdata)
+{
+	kfree(wdata);
+}
+
 static int wiimote_hid_probe(struct hid_device *hdev,
 				const struct hid_device_id *id)
 {
+	struct wiimote_data *wdata;
 	int ret;
 
+	wdata = wiimote_create(hdev);
+	if (!wdata) {
+		hid_err(hdev, "Can't alloc device\n");
+		return -ENOMEM;
+	}
+
 	ret = hid_parse(hdev);
 	if (ret) {
 		hid_err(hdev, "HID parse failed\n");
-		return ret;
+		goto err;
 	}
 
 	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
 	if (ret) {
 		hid_err(hdev, "HW start failed\n");
-		return ret;
+		goto err;
 	}
 
 	hid_info(hdev, "New device registered\n");
 	return 0;
+
+err:
+	wiimote_destroy(wdata);
+	return ret;
 }
 
 static void wiimote_hid_remove(struct hid_device *hdev)
 {
+	struct wiimote_data *wdata = hid_get_drvdata(hdev);
+
 	hid_info(hdev, "Device removed\n");
 	hid_hw_stop(hdev);
+	wiimote_destroy(wdata);
 }
 
 static const struct hid_device_id wiimote_hid_devices[] = {

-- 
1.7.5.2

[PATCH 02/12] HID: wiimote: Register wiimote hid driver stub

From: David Herrmann <hidden>
Date: 2011-06-27 14:30:53

The wiimote uses a fake HID protocol. Hence, we need to prevent
HIDINPUT and HIDDEV from parsing wiimote data and instead parse
raw hid events.
Add VID/PID to hid-core so the special driver is loaded on new
wiimotes.

Signed-off-by: David Herrmann <redacted>
---
 drivers/hid/hid-core.c    |    1 +
 drivers/hid/hid-ids.h     |    3 ++
 drivers/hid/hid-wiimote.c |   62 ++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 65 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index c957c4b..4482556 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c

@@ -1502,6 +1502,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ZYDACRON, USB_DEVICE_ID_ZYDACRON_REMOTE_CONTROL) },
 
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_PRESENTER_8K_BT) },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_WIIMOTE) },
 	{ }
 };

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 0b374a6..480ce6e 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h

@@ -491,6 +491,9 @@
 #define USB_VENDOR_ID_NEXTWINDOW	0x1926
 #define USB_DEVICE_ID_NEXTWINDOW_TOUCHSCREEN	0x0003
 
+#define USB_VENDOR_ID_NINTENDO		0x057e
+#define USB_DEVICE_ID_NINTENDO_WIIMOTE	0x0306
+
 #define USB_VENDOR_ID_NTRIG		0x1b96
 #define USB_DEVICE_ID_NTRIG_TOUCH_SCREEN   0x0001
 #define USB_DEVICE_ID_NTRIG_TOUCH_SCREEN_1   0x0003

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index 8a770e6..ed4fe18 100644
--- a/drivers/hid/hid-wiimote.c
+++ b/drivers/hid/hid-wiimote.c

@@ -10,18 +10,78 @@
  * any later version.
  */
 
+#include <linux/hid.h>
 #include <linux/module.h>
+#include "hid-ids.h"
 
 #define WIIMOTE_VERSION "0.1"
 #define WIIMOTE_NAME "Nintendo Wii Remote"
 
-static int __init wiimote_init(void)
+static int wiimote_hid_event(struct hid_device *hdev, struct hid_report *report,
+							u8 *raw_data, int size)
+{
+	if (size < 1)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int wiimote_hid_probe(struct hid_device *hdev,
+				const struct hid_device_id *id)
 {
+	int ret;
+
+	ret = hid_parse(hdev);
+	if (ret) {
+		hid_err(hdev, "HID parse failed\n");
+		return ret;
+	}
+
+	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+	if (ret) {
+		hid_err(hdev, "HW start failed\n");
+		return ret;
+	}
+
+	hid_info(hdev, "New device registered\n");
 	return 0;
 }
 
+static void wiimote_hid_remove(struct hid_device *hdev)
+{
+	hid_info(hdev, "Device removed\n");
+	hid_hw_stop(hdev);
+}
+
+static const struct hid_device_id wiimote_hid_devices[] = {
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO,
+				USB_DEVICE_ID_NINTENDO_WIIMOTE) },
+	{ }
+};
+MODULE_DEVICE_TABLE(hid, wiimote_hid_devices);
+
+static struct hid_driver wiimote_hid_driver = {
+	.name = "wiimote",
+	.id_table = wiimote_hid_devices,
+	.probe = wiimote_hid_probe,
+	.remove = wiimote_hid_remove,
+	.raw_event = wiimote_hid_event,
+};
+
+static int __init wiimote_init(void)
+{
+	int ret;
+
+	ret = hid_register_driver(&wiimote_hid_driver);
+	if (ret)
+		pr_err("Can't register wiimote hid driver\n");
+
+	return ret;
+}
+
 static void __exit wiimote_exit(void)
 {
+	hid_unregister_driver(&wiimote_hid_driver);
 }
 
 module_init(wiimote_init);

-- 
1.7.5.2

[PATCH 04/12] HID: wiimote: Register input device in wiimote hid driver

From: David Herrmann <hidden>
Date: 2011-06-27 14:30:54

Register input device so the wiimote can report input events on
it. We do not use HIDINPUT because the wiimote does not provide any
descriptor table which might be used by HIDINPUT. So we avoid
having HIDINPUT parse the wiimote descriptor and create unrelated
or unknown event flags. Instead we register our own input device
that we have full control of.

Signed-off-by: David Herrmann <redacted>
---
 drivers/hid/hid-wiimote.c |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index ff7cf12..deaf232 100644
--- a/drivers/hid/hid-wiimote.c
+++ b/drivers/hid/hid-wiimote.c

@@ -10,7 +10,9 @@
  * any later version.
  */
 
+#include <linux/device.h>
 #include <linux/hid.h>
+#include <linux/input.h>
 #include <linux/module.h>
 #include "hid-ids.h"

@@ -19,8 +21,15 @@
 
 struct wiimote_data {
 	struct hid_device *hdev;
+	struct input_dev *input;
 };
 
+static int wiimote_input_event(struct input_dev *dev, unsigned int type,
+						unsigned int code, int value)
+{
+	return 0;
+}
+
 static int wiimote_hid_event(struct hid_device *hdev, struct hid_report *report,
 							u8 *raw_data, int size)
 {

@@ -38,9 +47,24 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev)
 	if (!wdata)
 		return NULL;
 
+	wdata->input = input_allocate_device();
+	if (!wdata->input) {
+		kfree(wdata);
+		return NULL;
+	}
+
 	wdata->hdev = hdev;
 	hid_set_drvdata(hdev, wdata);
 
+	input_set_drvdata(wdata->input, wdata);
+	wdata->input->event = wiimote_input_event;
+	wdata->input->dev.parent = &wdata->hdev->dev;
+	wdata->input->id.bustype = wdata->hdev->bus;
+	wdata->input->id.vendor = wdata->hdev->vendor;
+	wdata->input->id.product = wdata->hdev->product;
+	wdata->input->id.version = wdata->hdev->version;
+	wdata->input->name = WIIMOTE_NAME;
+
 	return wdata;
 }

@@ -73,10 +97,19 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 		goto err;
 	}
 
+	ret = input_register_device(wdata->input);
+	if (ret) {
+		hid_err(hdev, "Cannot register input device\n");
+		goto err_stop;
+	}
+
 	hid_info(hdev, "New device registered\n");
 	return 0;
 
+err_stop:
+	hid_hw_stop(hdev);
 err:
+	input_free_device(wdata->input);
 	wiimote_destroy(wdata);
 	return ret;
 }

@@ -87,6 +120,7 @@ static void wiimote_hid_remove(struct hid_device *hdev)
 
 	hid_info(hdev, "Device removed\n");
 	hid_hw_stop(hdev);
+	input_unregister_device(wdata->input);
 	wiimote_destroy(wdata);
 }

-- 
1.7.5.2

[PATCH 06/12] HID: wiimote: Add wiimote send function

From: David Herrmann <hidden>
Date: 2011-06-27 14:30:59

The wiimote driver needs to send raw output reports to the wiimote
device. Otherwise we could not manage the peripherals of the wiimote
or perform memory operations on the wiimote.
We cannot use hidinput_input_event of the lowlevel hid driver, since
this does not accept raw input. Therefore, we need to use the same
function that hidraw uses to send output. Side effect is, the raw
output function is not buffered and can sleep.

Signed-off-by: David Herrmann <redacted>
---
 drivers/hid/hid-wiimote.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index 3416f69..811ed89 100644
--- a/drivers/hid/hid-wiimote.c
+++ b/drivers/hid/hid-wiimote.c

@@ -26,6 +26,25 @@ struct wiimote_data {
 	struct input_dev *input;
 };
 
+static ssize_t wiimote_hid_send(struct hid_device *hdev, __u8 *buffer,
+								size_t count)
+{
+	__u8 *buf;
+	ssize_t ret;
+
+	if (!hdev->hid_output_raw_report)
+		return -ENODEV;
+
+	buf = kmemdup(buffer, count, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ret = hdev->hid_output_raw_report(hdev, buf, count, HID_OUTPUT_REPORT);
+
+	kfree(buf);
+	return ret;
+}
+
 static int wiimote_input_event(struct input_dev *dev, unsigned int type,
 						unsigned int code, int value)
 {

-- 
1.7.5.2

[PATCH 07/12] HID: wiimote: Add output queue for wiimote driver

From: David Herrmann <hidden>
Date: 2011-06-27 14:31:01

The raw hid output function that is supported by bluetooth low-level
hid driver does not provide an output queue and also may sleep. The
wiimote driver, though, may need to send data in atomic context so
this patch adds a buffered output queue for the wiimote driver.

We use the shared workqueue to send our buffer to the hid device.
There is always only one active worker which reschedules itself until
the wiimote queue is empty. This prevents the worker from occupying
the shared workqueue for too long. If our queue is full, every further
output is discarded.

Special care is needed in the deinitialization routine. When
wiimote_hid_remove is called, HID input is already disabled, but HID
output may still be used from our worker and is then discarded by the
lower HID layers. Therefore, we can safely disable the input layer since it
is the only layer that still sends input events.
Future sysfs attributes must be freed before unregistering input to
avoid the sysfs handlers to send input events to a non-existing input
layer.

Signed-off-by: David Herrmann <redacted>
---
 drivers/hid/hid-wiimote.c |   86 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 86 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index 811ed89..c2b91fb 100644
--- a/drivers/hid/hid-wiimote.c
+++ b/drivers/hid/hid-wiimote.c

@@ -15,15 +15,28 @@
 #include <linux/hid.h>
 #include <linux/input.h>
 #include <linux/module.h>
+#include <linux/spinlock.h>
 #include "hid-ids.h"
 
 #define WIIMOTE_VERSION "0.1"
 #define WIIMOTE_NAME "Nintendo Wii Remote"
+#define WIIMOTE_BUFSIZE 32
+
+struct wiimote_buf {
+	__u8 data[HID_MAX_BUFFER_SIZE];
+	size_t size;
+};
 
 struct wiimote_data {
 	atomic_t ready;
 	struct hid_device *hdev;
 	struct input_dev *input;
+
+	spinlock_t qlock;
+	__u8 head;
+	__u8 tail;
+	struct wiimote_buf outq[WIIMOTE_BUFSIZE];
+	struct work_struct worker;
 };
 
 static ssize_t wiimote_hid_send(struct hid_device *hdev, __u8 *buffer,

@@ -45,6 +58,73 @@ static ssize_t wiimote_hid_send(struct hid_device *hdev, __u8 *buffer,
 	return ret;
 }
 
+static void wiimote_worker(struct work_struct *work)
+{
+	struct wiimote_data *wdata = container_of(work, struct wiimote_data,
+									worker);
+	unsigned long flags;
+
+	spin_lock_irqsave(&wdata->qlock, flags);
+
+	if (wdata->head != wdata->tail) {
+		spin_unlock_irqrestore(&wdata->qlock, flags);
+		wiimote_hid_send(wdata->hdev, wdata->outq[wdata->tail].data,
+						wdata->outq[wdata->tail].size);
+		spin_lock_irqsave(&wdata->qlock, flags);
+
+		wdata->tail = (wdata->tail + 1) % WIIMOTE_BUFSIZE;
+
+		/*
+		 * We are using the shared workqueue so send at most one whole
+		 * buffer and then reschedule ourself so we do not occupy the
+		 * shared workqueue for too long.
+		 */
+		if (wdata->head != wdata->tail)
+			schedule_work(&wdata->worker);
+	}
+
+	spin_unlock_irqrestore(&wdata->qlock, flags);
+}
+
+static void wiimote_queue(struct wiimote_data *wdata, const __u8 *buffer,
+								size_t count)
+{
+	unsigned long flags;
+	__u8 newhead;
+
+	if (count > HID_MAX_BUFFER_SIZE) {
+		hid_warn(wdata->hdev, "Sending too large output report\n");
+		return;
+	}
+
+	/*
+	 * Copy new request into our output queue and check whether the
+	 * queue is full. If it is full, discard this request.
+	 * If it is empty we need to start a new worker that will
+	 * send out the buffer to the hid device.
+	 * If the queue is not empty, then there must be a worker
+	 * that is currently sending out our buffer and this worker
+	 * will reschedule itself until the queue is empty.
+	 */
+
+	spin_lock_irqsave(&wdata->qlock, flags);
+
+	memcpy(wdata->outq[wdata->head].data, buffer, count);
+	wdata->outq[wdata->head].size = count;
+	newhead = (wdata->head + 1) % WIIMOTE_BUFSIZE;
+
+	if (wdata->head == wdata->tail) {
+		wdata->head = newhead;
+		schedule_work(&wdata->worker);
+	} else if (newhead != wdata->tail) {
+		wdata->head = newhead;
+	} else {
+		hid_warn(wdata->hdev, "Output queue is full");
+	}
+
+	spin_unlock_irqrestore(&wdata->qlock, flags);
+}
+
 static int wiimote_input_event(struct input_dev *dev, unsigned int type,
 						unsigned int code, int value)
 {

@@ -100,6 +180,9 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev)
 	wdata->input->id.version = wdata->hdev->version;
 	wdata->input->name = WIIMOTE_NAME;
 
+	spin_lock_init(&wdata->qlock);
+	INIT_WORK(&wdata->worker, wiimote_worker);
+
 	return wdata;
 }

@@ -157,8 +240,11 @@ static void wiimote_hid_remove(struct hid_device *hdev)
 	struct wiimote_data *wdata = hid_get_drvdata(hdev);
 
 	hid_info(hdev, "Device removed\n");
+
 	hid_hw_stop(hdev);
 	input_unregister_device(wdata->input);
+
+	cancel_work_sync(&wdata->worker);
 	wiimote_destroy(wdata);
 }

-- 
1.7.5.2

[PATCH 08/12] HID: wiimote: Add wiimote event handler

From: David Herrmann <hidden>
Date: 2011-06-27 14:31:03

Create array of all event handlers and call each handler when we
receive the related event.

Signed-off-by: David Herrmann <redacted>
---
 drivers/hid/hid-wiimote.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index c2b91fb..9ae333d 100644
--- a/drivers/hid/hid-wiimote.c
+++ b/drivers/hid/hid-wiimote.c

@@ -138,10 +138,22 @@ static int wiimote_input_event(struct input_dev *dev, unsigned int type,
 	return 0;
 }
 
+struct wiiproto_handler {
+	__u8 id;
+	size_t size;
+	void (*func)(struct wiimote_data *wdata, const __u8 *payload);
+};
+
+static struct wiiproto_handler handlers[] = {
+	{ .id = 0 }
+};
+
 static int wiimote_hid_event(struct hid_device *hdev, struct hid_report *report,
 							u8 *raw_data, int size)
 {
 	struct wiimote_data *wdata = hid_get_drvdata(hdev);
+	struct wiiproto_handler *h;
+	int i;
 
 	if (!atomic_read(&wdata->ready))
 		return -EBUSY;

@@ -151,6 +163,12 @@ static int wiimote_hid_event(struct hid_device *hdev, struct hid_report *report,
 	if (size < 1)
 		return -EINVAL;
 
+	for (i = 0; handlers[i].id; ++i) {
+		h = &handlers[i];
+		if (h->id == raw_data[0] && h->size < size)
+			h->func(wdata, &raw_data[1]);
+	}
+
 	return 0;
 }

-- 
1.7.5.2

[PATCH 09/12] HID: wiimote: Add wiimote input button parser

From: David Herrmann <hidden>
Date: 2011-06-27 14:31:05

Parse input report 0x30 from the wiimote as button input. We need to
send events for all buttons on every input report because the wiimote
does not send events for single buttons but always for all buttons
to us. The input layer, however, filters redundant events.

Signed-off-by: David Herrmann <redacted>
---
 drivers/hid/hid-wiimote.c |   66 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index 9ae333d..3049fc8 100644
--- a/drivers/hid/hid-wiimote.c
+++ b/drivers/hid/hid-wiimote.c

@@ -39,6 +39,39 @@ struct wiimote_data {
 	struct work_struct worker;
 };
 
+enum wiiproto_reqs {
+	WIIPROTO_REQ_DRM_K = 0x30,
+};
+
+enum wiiproto_keys {
+	WIIPROTO_KEY_LEFT,
+	WIIPROTO_KEY_RIGHT,
+	WIIPROTO_KEY_UP,
+	WIIPROTO_KEY_DOWN,
+	WIIPROTO_KEY_PLUS,
+	WIIPROTO_KEY_MINUS,
+	WIIPROTO_KEY_ONE,
+	WIIPROTO_KEY_TWO,
+	WIIPROTO_KEY_A,
+	WIIPROTO_KEY_B,
+	WIIPROTO_KEY_HOME,
+	WIIPROTO_KEY_COUNT
+};
+
+static __u16 wiiproto_keymap[] = {
+	KEY_LEFT,	/* WIIPROTO_KEY_LEFT */
+	KEY_RIGHT,	/* WIIPROTO_KEY_RIGHT */
+	KEY_UP,		/* WIIPROTO_KEY_UP */
+	KEY_DOWN,	/* WIIPROTO_KEY_DOWN */
+	KEY_NEXT,	/* WIIPROTO_KEY_PLUS */
+	KEY_PREVIOUS,	/* WIIPROTO_KEY_MINUS */
+	BTN_1,		/* WIIPROTO_KEY_ONE */
+	BTN_2,		/* WIIPROTO_KEY_TWO */
+	BTN_A,		/* WIIPROTO_KEY_A */
+	BTN_B,		/* WIIPROTO_KEY_B */
+	BTN_MODE,	/* WIIPROTO_KEY_HOME */
+};
+
 static ssize_t wiimote_hid_send(struct hid_device *hdev, __u8 *buffer,
 								size_t count)
 {

@@ -138,6 +171,33 @@ static int wiimote_input_event(struct input_dev *dev, unsigned int type,
 	return 0;
 }
 
+static void handler_keys(struct wiimote_data *wdata, const __u8 *payload)
+{
+	input_event(wdata->input, EV_KEY, wiiproto_keymap[WIIPROTO_KEY_LEFT],
+							!!(payload[0] & 0x01));
+	input_event(wdata->input, EV_KEY, wiiproto_keymap[WIIPROTO_KEY_RIGHT],
+							!!(payload[0] & 0x02));
+	input_event(wdata->input, EV_KEY, wiiproto_keymap[WIIPROTO_KEY_DOWN],
+							!!(payload[0] & 0x04));
+	input_event(wdata->input, EV_KEY, wiiproto_keymap[WIIPROTO_KEY_UP],
+							!!(payload[0] & 0x08));
+	input_event(wdata->input, EV_KEY, wiiproto_keymap[WIIPROTO_KEY_PLUS],
+							!!(payload[0] & 0x10));
+	input_event(wdata->input, EV_KEY, wiiproto_keymap[WIIPROTO_KEY_TWO],
+							!!(payload[1] & 0x01));
+	input_event(wdata->input, EV_KEY, wiiproto_keymap[WIIPROTO_KEY_ONE],
+							!!(payload[1] & 0x02));
+	input_event(wdata->input, EV_KEY, wiiproto_keymap[WIIPROTO_KEY_B],
+							!!(payload[1] & 0x04));
+	input_event(wdata->input, EV_KEY, wiiproto_keymap[WIIPROTO_KEY_A],
+							!!(payload[1] & 0x08));
+	input_event(wdata->input, EV_KEY, wiiproto_keymap[WIIPROTO_KEY_MINUS],
+							!!(payload[1] & 0x10));
+	input_event(wdata->input, EV_KEY, wiiproto_keymap[WIIPROTO_KEY_HOME],
+							!!(payload[1] & 0x80));
+	input_event(wdata->input, EV_SYN, SYN_REPORT, 0);
+}
+
 struct wiiproto_handler {
 	__u8 id;
 	size_t size;

@@ -145,6 +205,7 @@ struct wiiproto_handler {
 };
 
 static struct wiiproto_handler handlers[] = {
+	{ .id = WIIPROTO_REQ_DRM_K, .size = 2, .func = handler_keys },
 	{ .id = 0 }
 };

@@ -175,6 +236,7 @@ static int wiimote_hid_event(struct hid_device *hdev, struct hid_report *report,
 static struct wiimote_data *wiimote_create(struct hid_device *hdev)
 {
 	struct wiimote_data *wdata;
+	int i;
 
 	wdata = kzalloc(sizeof(*wdata), GFP_KERNEL);
 	if (!wdata)

@@ -198,6 +260,10 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev)
 	wdata->input->id.version = wdata->hdev->version;
 	wdata->input->name = WIIMOTE_NAME;
 
+	set_bit(EV_KEY, wdata->input->evbit);
+	for (i = 0; i < WIIPROTO_KEY_COUNT; ++i)
+		set_bit(wiiproto_keymap[i], wdata->input->keybit);
+
 	spin_lock_init(&wdata->qlock);
 	INIT_WORK(&wdata->worker, wiimote_worker);

-- 
1.7.5.2

[PATCH 10/12] HID: wiimote: Add wiimote led request

From: David Herrmann <hidden>
Date: 2011-06-27 14:31:07

Add new request that sets the leds on the target device. Also, per
default, set led1 after initializing a device.

Signed-off-by: David Herrmann <redacted>
---
 drivers/hid/hid-wiimote.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index 3049fc8..400a2d9 100644
--- a/drivers/hid/hid-wiimote.c
+++ b/drivers/hid/hid-wiimote.c

@@ -39,7 +39,13 @@ struct wiimote_data {
 	struct work_struct worker;
 };
 
+#define WIIPROTO_FLAG_LED1 0x01
+#define WIIPROTO_FLAG_LED2 0x02
+#define WIIPROTO_FLAG_LED3 0x04
+#define WIIPROTO_FLAG_LED4 0x08
+
 enum wiiproto_reqs {
+	WIIPROTO_REQ_LED = 0x11,
 	WIIPROTO_REQ_DRM_K = 0x30,
 };

@@ -158,6 +164,25 @@ static void wiimote_queue(struct wiimote_data *wdata, const __u8 *buffer,
 	spin_unlock_irqrestore(&wdata->qlock, flags);
 }
 
+static void wiiproto_req_leds(struct wiimote_data *wdata, int leds)
+{
+	__u8 cmd[2];
+
+	cmd[0] = WIIPROTO_REQ_LED;
+	cmd[1] = 0;
+
+	if (leds & WIIPROTO_FLAG_LED1)
+		cmd[1] |= 0x10;
+	if (leds & WIIPROTO_FLAG_LED2)
+		cmd[1] |= 0x20;
+	if (leds & WIIPROTO_FLAG_LED3)
+		cmd[1] |= 0x40;
+	if (leds & WIIPROTO_FLAG_LED4)
+		cmd[1] |= 0x80;
+
+	wiimote_queue(wdata, cmd, sizeof(cmd));
+}
+
 static int wiimote_input_event(struct input_dev *dev, unsigned int type,
 						unsigned int code, int value)
 {

@@ -309,6 +334,7 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 	smp_wmb();
 	atomic_set(&wdata->ready, 1);
 	hid_info(hdev, "New device registered\n");
+	wiiproto_req_leds(wdata, WIIPROTO_FLAG_LED1);
 	return 0;
 
 err_stop:

-- 
1.7.5.2

[PATCH 11/12] HID: wiimote: Cache wiimote led state

From: David Herrmann <hidden>
Date: 2011-06-27 14:31:08

Save the current state of the leds in the wiimote data structure. This
allows us to discard new led requests that wouldn't change anything.
Protect the whole state structure by a spinlock. Every wiiproto_*
function expects this spinlock to be held when called.

Signed-off-by: David Herrmann <redacted>
---
 drivers/hid/hid-wiimote.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index 400a2d9..29a5b97 100644
--- a/drivers/hid/hid-wiimote.c
+++ b/drivers/hid/hid-wiimote.c

@@ -27,6 +27,11 @@ struct wiimote_buf {
 	size_t size;
 };
 
+struct wiimote_state {
+	spinlock_t lock;
+	__u8 flags;
+};
+
 struct wiimote_data {
 	atomic_t ready;
 	struct hid_device *hdev;

@@ -37,12 +42,16 @@ struct wiimote_data {
 	__u8 tail;
 	struct wiimote_buf outq[WIIMOTE_BUFSIZE];
 	struct work_struct worker;
+
+	struct wiimote_state state;
 };
 
 #define WIIPROTO_FLAG_LED1 0x01
 #define WIIPROTO_FLAG_LED2 0x02
 #define WIIPROTO_FLAG_LED3 0x04
 #define WIIPROTO_FLAG_LED4 0x08
+#define WIIPROTO_FLAGS_LEDS (WIIPROTO_FLAG_LED1 | WIIPROTO_FLAG_LED2 | \
+					WIIPROTO_FLAG_LED3 | WIIPROTO_FLAG_LED4)
 
 enum wiiproto_reqs {
 	WIIPROTO_REQ_LED = 0x11,

@@ -168,6 +177,11 @@ static void wiiproto_req_leds(struct wiimote_data *wdata, int leds)
 {
 	__u8 cmd[2];
 
+	leds &= WIIPROTO_FLAGS_LEDS;
+	if ((wdata->state.flags & WIIPROTO_FLAGS_LEDS) == leds)
+		return;
+	wdata->state.flags = (wdata->state.flags & ~WIIPROTO_FLAGS_LEDS) | leds;
+
 	cmd[0] = WIIPROTO_REQ_LED;
 	cmd[1] = 0;

@@ -240,6 +254,7 @@ static int wiimote_hid_event(struct hid_device *hdev, struct hid_report *report,
 	struct wiimote_data *wdata = hid_get_drvdata(hdev);
 	struct wiiproto_handler *h;
 	int i;
+	unsigned long flags;
 
 	if (!atomic_read(&wdata->ready))
 		return -EBUSY;

@@ -249,12 +264,16 @@ static int wiimote_hid_event(struct hid_device *hdev, struct hid_report *report,
 	if (size < 1)
 		return -EINVAL;
 
+	spin_lock_irqsave(&wdata->state.lock, flags);
+
 	for (i = 0; handlers[i].id; ++i) {
 		h = &handlers[i];
 		if (h->id == raw_data[0] && h->size < size)
 			h->func(wdata, &raw_data[1]);
 	}
 
+	spin_unlock_irqrestore(&wdata->state.lock, flags);
+
 	return 0;
 }

@@ -292,6 +311,8 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev)
 	spin_lock_init(&wdata->qlock);
 	INIT_WORK(&wdata->worker, wiimote_worker);
 
+	spin_lock_init(&wdata->state.lock);
+
 	return wdata;
 }

@@ -334,7 +355,12 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 	smp_wmb();
 	atomic_set(&wdata->ready, 1);
 	hid_info(hdev, "New device registered\n");
+
+	/* by default set led1 after device initialization */
+	spin_lock_irq(&wdata->state.lock);
 	wiiproto_req_leds(wdata, WIIPROTO_FLAG_LED1);
+	spin_unlock_irq(&wdata->state.lock);
+
 	return 0;
 
 err_stop:

-- 
1.7.5.2

[PATCH 12/12] HID: wiimote: Add sysfs support to wiimote driver

From: David Herrmann <hidden>
Date: 2011-06-27 14:31:10

Add sysfs files for each led of the wiimote. Writing 1 to the file
enables the led and 0 disables the led.

We do not need memory barriers when checking wdata->ready since we use
a spinlock directly after it.

Signed-off-by: David Herrmann <redacted>
---
 Documentation/ABI/testing/sysfs-driver-hid-wiimote |   10 +++
 drivers/hid/hid-wiimote.c                          |   74 ++++++++++++++++++++
 2 files changed, 84 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-wiimote

diff --git a/Documentation/ABI/testing/sysfs-driver-hid-wiimote b/Documentation/ABI/testing/sysfs-driver-hid-wiimote
new file mode 100644
index 0000000..5d5a16e
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-hid-wiimote

@@ -0,0 +1,10 @@
+What:		/sys/bus/hid/drivers/wiimote/<dev>/led1
+What:		/sys/bus/hid/drivers/wiimote/<dev>/led2
+What:		/sys/bus/hid/drivers/wiimote/<dev>/led3
+What:		/sys/bus/hid/drivers/wiimote/<dev>/led4
+Date:		July 2011
+KernelVersion:	3.1
+Contact:	David Herrmann <dh.herrmann@googlemail.com>
+Description:	Make it possible to set/get current led state. Reading from it
+		returns 0 if led is off and 1 if it is on. Writing 0 to it
+		disables the led, writing 1 enables it.

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index 29a5b97..8c1c3b1 100644
--- a/drivers/hid/hid-wiimote.c
+++ b/drivers/hid/hid-wiimote.c

@@ -87,6 +87,9 @@ static __u16 wiiproto_keymap[] = {
 	BTN_MODE,	/* WIIPROTO_KEY_HOME */
 };
 
+#define dev_to_wii(pdev) hid_get_drvdata(container_of(pdev, struct hid_device, \
+									dev))
+
 static ssize_t wiimote_hid_send(struct hid_device *hdev, __u8 *buffer,
 								size_t count)
 {

@@ -197,6 +200,55 @@ static void wiiproto_req_leds(struct wiimote_data *wdata, int leds)
 	wiimote_queue(wdata, cmd, sizeof(cmd));
 }
 
+#define wiifs_led_show_set(num)						\
+static ssize_t wiifs_led_show_##num(struct device *dev,			\
+			struct device_attribute *attr, char *buf)	\
+{									\
+	struct wiimote_data *wdata = dev_to_wii(dev);			\
+	unsigned long flags;						\
+	int state;							\
+									\
+	if (!atomic_read(&wdata->ready))				\
+		return -EBUSY;						\
+									\
+	spin_lock_irqsave(&wdata->state.lock, flags);			\
+	state = !!(wdata->state.flags & WIIPROTO_FLAG_LED##num);	\
+	spin_unlock_irqrestore(&wdata->state.lock, flags);		\
+									\
+	return sprintf(buf, "%d\n", state);				\
+}									\
+static ssize_t wiifs_led_set_##num(struct device *dev,			\
+	struct device_attribute *attr, const char *buf, size_t count)	\
+{									\
+	struct wiimote_data *wdata = dev_to_wii(dev);			\
+	int tmp = simple_strtoul(buf, NULL, 10);			\
+	unsigned long flags;						\
+	__u8 state;							\
+									\
+	if (!atomic_read(&wdata->ready))				\
+		return -EBUSY;						\
+									\
+	spin_lock_irqsave(&wdata->state.lock, flags);			\
+									\
+	state = wdata->state.flags;					\
+									\
+	if (tmp)							\
+		wiiproto_req_leds(wdata, state | WIIPROTO_FLAG_LED##num);\
+	else								\
+		wiiproto_req_leds(wdata, state & ~WIIPROTO_FLAG_LED##num);\
+									\
+	spin_unlock_irqrestore(&wdata->state.lock, flags);		\
+									\
+	return count;							\
+}									\
+static DEVICE_ATTR(led##num, S_IRUGO | S_IWUSR, wiifs_led_show_##num,	\
+						wiifs_led_set_##num)
+
+wiifs_led_show_set(1);
+wiifs_led_show_set(2);
+wiifs_led_show_set(3);
+wiifs_led_show_set(4);
+
 static int wiimote_input_event(struct input_dev *dev, unsigned int type,
 						unsigned int code, int value)
 {

@@ -333,6 +385,19 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 		return -ENOMEM;
 	}
 
+	ret = device_create_file(&hdev->dev, &dev_attr_led1);
+	if (ret)
+		goto err;
+	ret = device_create_file(&hdev->dev, &dev_attr_led2);
+	if (ret)
+		goto err;
+	ret = device_create_file(&hdev->dev, &dev_attr_led3);
+	if (ret)
+		goto err;
+	ret = device_create_file(&hdev->dev, &dev_attr_led4);
+	if (ret)
+		goto err;
+
 	ret = hid_parse(hdev);
 	if (ret) {
 		hid_err(hdev, "HID parse failed\n");

@@ -367,6 +432,10 @@ err_stop:
 	hid_hw_stop(hdev);
 err:
 	input_free_device(wdata->input);
+	device_remove_file(&hdev->dev, &dev_attr_led1);
+	device_remove_file(&hdev->dev, &dev_attr_led2);
+	device_remove_file(&hdev->dev, &dev_attr_led3);
+	device_remove_file(&hdev->dev, &dev_attr_led4);
 	wiimote_destroy(wdata);
 	return ret;
 }

@@ -377,6 +446,11 @@ static void wiimote_hid_remove(struct hid_device *hdev)
 
 	hid_info(hdev, "Device removed\n");
 
+	device_remove_file(&hdev->dev, &dev_attr_led1);
+	device_remove_file(&hdev->dev, &dev_attr_led2);
+	device_remove_file(&hdev->dev, &dev_attr_led3);
+	device_remove_file(&hdev->dev, &dev_attr_led4);
+
 	hid_hw_stop(hdev);
 	input_unregister_device(wdata->input);

-- 
1.7.5.2

[PATCH 05/12] HID: wiimote: Synchronize wiimote input and hid event handling

From: David Herrmann <hidden>
Date: 2011-06-27 14:31:46

The wiimote first starts HID hardware and then registers the input
device. We need to synchronize the startup so no event handler will
start parsing events when the wiimote device is not ready, yet.

Signed-off-by: David Herrmann <redacted>
---
 drivers/hid/hid-wiimote.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index deaf232..3416f69 100644
--- a/drivers/hid/hid-wiimote.c
+++ b/drivers/hid/hid-wiimote.c

@@ -10,6 +10,7 @@
  * any later version.
  */
 
+#include <linux/atomic.h>
 #include <linux/device.h>
 #include <linux/hid.h>
 #include <linux/input.h>

@@ -20,6 +21,7 @@
 #define WIIMOTE_NAME "Nintendo Wii Remote"
 
 struct wiimote_data {
+	atomic_t ready;
 	struct hid_device *hdev;
 	struct input_dev *input;
 };

@@ -27,12 +29,26 @@ struct wiimote_data {
 static int wiimote_input_event(struct input_dev *dev, unsigned int type,
 						unsigned int code, int value)
 {
+	struct wiimote_data *wdata = input_get_drvdata(dev);
+
+	if (!atomic_read(&wdata->ready))
+		return -EBUSY;
+	/* smp_rmb: Make sure wdata->xy is available when wdata->ready is 1 */
+	smp_rmb();
+
 	return 0;
 }
 
 static int wiimote_hid_event(struct hid_device *hdev, struct hid_report *report,
 							u8 *raw_data, int size)
 {
+	struct wiimote_data *wdata = hid_get_drvdata(hdev);
+
+	if (!atomic_read(&wdata->ready))
+		return -EBUSY;
+	/* smp_rmb: Make sure wdata->xy is available when wdata->ready is 1 */
+	smp_rmb();
+
 	if (size < 1)
 		return -EINVAL;

@@ -103,6 +119,9 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 		goto err_stop;
 	}
 
+	/* smp_wmb: Write wdata->xy first before wdata->ready is set to 1 */
+	smp_wmb();
+	atomic_set(&wdata->ready, 1);
 	hid_info(hdev, "New device registered\n");
 	return 0;

-- 
1.7.5.2

Re: [PATCH 07/12] HID: wiimote: Add output queue for wiimote driver

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2011-06-28 04:51:29


avid Herrmann [off-list ref] wrote:

The raw hid output function that is supported by bluetooth low-level
hid driver does not provide an output queue and also may sleep. The
wiimote driver, though, may need to send data in atomic context so
this patch adds a buffered output queue for the wiimote driver.

We use the shared workqueue to send our buffer to the hid device.
There is always only one active worker which reschedules itself until
the wiimote queue is empty. This prevents the worker from occupying
the shared workqueue for too long.

With CWQ this is not a concern anymore, you should be able to flush your buffer at once.
-- 
Dmitry

Re: [PATCH 05/12] HID: wiimote: Synchronize wiimote input and hid event handling

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2011-06-28 05:49:30


David Herrmann [off-list ref] wrote:

The wiimote first starts HID hardware and then registers the input
device. We need to synchronize the startup so no event handler will
start parsing events when the wiimote device is not ready, yet.

I believe this is generic HID layer problem. Would it be possible to fix this issue there?

Thanks.

-- 
Dmitry

Re: [PATCH 09/12] HID: wiimote: Add wiimote input button parser

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2011-06-28 05:52:27

David Herrmann [off-list ref] wrote:

+	input_event(wdata->input, EV_KEY, wiiproto_keymap[WIIPROTO_KEY_LEFT],
+							!!(payload[0] & 0x01));

input_report_key()

+	input_event(wdata->input, EV_SYN, SYN_REPORT, 0);

input_sync()

Thanks.


-- 
Dmitry

Re: [PATCH 05/12] HID: wiimote: Synchronize wiimote input and hid event handling

From: David Herrmann <hidden>
Date: 2011-06-29 12:39:08

On Tue, Jun 28, 2011 at 7:49 AM, Dmitry Torokhov
[off-list ref] wrote:


David Herrmann [off-list ref] wrote:

quoted

The wiimote first starts HID hardware and then registers the input
device. We need to synchronize the startup so no event handler will
start parsing events when the wiimote device is not ready, yet.

I believe this is generic HID layer problem. Would it be possible to fix this issue there?

The point is, I do not use HIDINPUT/HIDDEV (see commit message 2/12).
The wiimote has no valid report-descriptor table so hidinput/hiddev do
not make sense. I could set up a valid report-descriptor-table, use
input_mapping, input_mapped, report_fixup callbacks and then use
HIDINPUT. However, that would be far more complex than my current
solution, which is creating my own input instance and also my own
synchronization.
Furthermore, the wiimote provides input/output features that cannot be
handled by the generic HID handler, so I would need to extend the
input_mapping, anyway.

But, thanks to your hint, I looked at hid_hw_start() again and as far
as I can see the synchronization issue exists there, too.
The ll_driver->start() function is called first and after that, the
input instance is registered.
If the ll_driver reports an event before the input instance is
registered, the hidinput/other-driver will call input_event() on an
invalid input instance (probably even a NULL dereference).
This may not be a problem for usbhid, because usbhid_open() has to be
called first, however, hidp (bluetooth hid) ignores the *_open()
callback and therefore may fail.

I will look at this again and send a separate mail to this list,
however, I don't think this is related to my driver? Or do you think I
should replace my input-handling with HIDINPUT to be more homogeneous
with the other hid drivers?

Thanks.

--
Dmitry

Regards
David

Re: [PATCH 09/12] HID: wiimote: Add wiimote input button parser

From: David Herrmann <hidden>
Date: 2011-06-29 12:43:33

On Tue, Jun 28, 2011 at 7:52 AM, Dmitry Torokhov
[off-list ref] wrote:

David Herrmann [off-list ref] wrote:

quoted

+      input_event(wdata->input, EV_KEY, wiiproto_keymap[WIIPROTO_KEY_LEFT],
+                                                      !!(payload[0] & 0x01));

input_report_key()

quoted

+      input_event(wdata->input, EV_SYN, SYN_REPORT, 0);

input_sync()

Thanks.


--
Dmitry

Thanks, I've fixed that.
David
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/12] HID: wiimote: Add output queue for wiimote driver

From: David Herrmann <hidden>
Date: 2011-06-29 12:53:50

On Tue, Jun 28, 2011 at 6:51 AM, Dmitry Torokhov
[off-list ref] wrote:


avid Herrmann [off-list ref] wrote:

quoted

The raw hid output function that is supported by bluetooth low-level
hid driver does not provide an output queue and also may sleep. The
wiimote driver, though, may need to send data in atomic context so
this patch adds a buffered output queue for the wiimote driver.

We use the shared workqueue to send our buffer to the hid device.
There is always only one active worker which reschedules itself until
the wiimote queue is empty. This prevents the worker from occupying
the shared workqueue for too long.

With CWQ this is not a concern anymore, you should be able to flush your buffer at once.

Thanks, I wasn't sure about this. I've now changed:
-       if (wdata->head != wdata->tail) {
+       while (wdata->head != wdata->tail) {
and removed the if(...) schedule_work(...);

--
Dmitry

Regards
David

Re: [PATCH 05/12] HID: wiimote: Synchronize wiimote input and hid event handling

From: Jiri Kosina <hidden>
Date: 2011-07-11 12:29:26

On Wed, 29 Jun 2011, David Herrmann wrote:

But, thanks to your hint, I looked at hid_hw_start() again and as far
as I can see the synchronization issue exists there, too.
The ll_driver->start() function is called first and after that, the
input instance is registered.
If the ll_driver reports an event before the input instance is
registered, the hidinput/other-driver will call input_event() on an
invalid input instance (probably even a NULL dereference).
This may not be a problem for usbhid, because usbhid_open() has to be
called first, however, hidp (bluetooth hid) ignores the *_open()
callback and therefore may fail.

Indeed, this looks like a proper analysis. Thanks a lot for looking into 
it.

Are you planning on submitting the fix for that? Otherwise I'll add it to 
my TODO list, independently on the wiimote driver.

Thanks,

-- 
Jiri Kosina
SUSE Labs

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help