[RFC 00/12] Nintendo Wii Remote Device Driver

[RFC 00/12] Nintendo Wii Remote Device Driver

[David Herrmann]

Hi

I currently work on an HID driver for the Nintendo Wii Remote as Bluez GSoC
project. Following, the basic driver implementation with wiimote button-input
and led support.

The Wii Remote (further wiimote) is a small wireless device that communicates
with the host via bluetooth. It uses the HID ports but does not provide any
proper HID descriptor sets and doesn't really fit into the HID class so a
special driver is needed to control the device.

Wiimote device:
 http://en.wikipedia.org/wiki/Wiimote

Bluetooth details and reverse-engineered protocol:
 https://github.com/dvdhrm/xwiimote/blob/master/doc/PROTOCOL

Protocol details (reverse engineered):
 http://wiibrew.org/wiki/Wiimote


Since the protocol used by the wiimote is fairly complex and does not work
well with HIDINPUT, I tried to copy the hidraw driver. I need to send complex
initialization sequences and memory writes to the wiimote which do not fit
into the "struct input_event" and thus cannot be fed to the basic output_report
function.
Instead I use the same function that HIDRAW uses to insert raw hid commands.
However, this function is unbuffered so I added an own buffer to the driver.

This patchset includes basic input-button parsing and provides the information
via evdev and also provides LED support which can be set via:
 echo [1/0] >/sys/bus/hid/devices/<dev>/led1 or led2, led3, led4

My tree is available at (based on jikos/master):
 https://github.com/dvdhrm/xwiimote_kernel
It gets rebased on jikos/master regularily.

Userspace tools/documentation are available at:
 https://github.com/dvdhrm/xwiimote

This is still a work in progress. Any comments are appreciated. My tree already
includes further peripheral support for the wiimote like IR-cam and accelerometer,
however, these peripherals get fairly complex in initialization sequences and
other stuff, so I excluded them from this first patchset.

Any comments are appreciated.
Kind regards

David

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

[David Herrmann]

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 <dh.herrmann@googlemail.com>
---
 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

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

[David Herrmann]

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 <dh.herrmann@googlemail.com>
---
 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

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

[David Herrmann]

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

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
 drivers/hid/hid-wiimote.c |   36 ++++++++++++++++++++++++++++++++++--
 1 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index ed4fe18..27a4261 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,59 @@ 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 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:
+	kfree(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);
+	kfree(wdata);
 }
 
 static const struct hid_device_id wiimote_hid_devices[] = {
-- 
1.7.5.2

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

[David Herrmann]

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 <dh.herrmann@googlemail.com>
---
 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 27a4261..9fa4fe1 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;
 }
 
@@ -68,10 +92,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);
 	kfree(wdata);
 	return ret;
 }
@@ -82,6 +115,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);
 	kfree(wdata);
 }
 
-- 
1.7.5.2

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

[David Herrmann]

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 <dh.herrmann@googlemail.com>
---
 drivers/hid/hid-wiimote.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index 9fa4fe1..4dbb2c1 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,22 @@ 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;
+
 	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;
+
 	if (size < 1)
 		return -EINVAL;
 
@@ -98,6 +110,7 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 		goto err_stop;
 	}
 
+	atomic_set(&wdata->ready, 1);
 	hid_info(hdev, "New device registered\n");
 	return 0;
 
-- 
1.7.5.2

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

[David Herrmann]

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 <dh.herrmann@googlemail.com>
---
 drivers/hid/hid-wiimote.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index 4dbb2c1..f249940 100644
--- a/drivers/hid/hid-wiimote.c
+++ b/drivers/hid/hid-wiimote.c
@@ -26,6 +26,16 @@ struct wiimote_data {
 	struct input_dev *input;
 };
 
+static ssize_t wiimote_hid_send(struct hid_device *hdev, __u8 *buffer,
+								size_t count)
+{
+	if (!hdev->hid_output_raw_report)
+		return -ENODEV;
+
+	return hdev->hid_output_raw_report(hdev, buffer, count,
+							HID_OUTPUT_REPORT);
+}
+
 static int wiimote_input_event(struct input_dev *dev, unsigned int type,
 						unsigned int code, int value)
 {
-- 
1.7.5.2

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

[David Herrmann]

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.

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
 drivers/hid/hid-wiimote.c |  104 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 104 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index f249940..687a469 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,
@@ -36,6 +49,78 @@ static ssize_t wiimote_hid_send(struct hid_device *hdev, __u8 *buffer,
 							HID_OUTPUT_REPORT);
 }
 
+static void wiimote_worker(struct work_struct *work)
+{
+	struct wiimote_data *wdata = container_of(work, struct wiimote_data,
+									worker);
+
+	if (!atomic_read(&wdata->ready))
+		return;
+
+	spin_lock(&wdata->qlock);
+
+	if (wdata->head != wdata->tail) {
+		spin_unlock(&wdata->qlock);
+		wiimote_hid_send(wdata->hdev, wdata->outq[wdata->tail].data,
+						wdata->outq[wdata->tail].size);
+		spin_lock(&wdata->qlock);
+
+		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(&wdata->qlock);
+}
+
+static void wiimote_queue(struct wiimote_data *wdata, const __u8 *buffer,
+								size_t count)
+{
+	unsigned long flags;
+	__u8 newhead;
+
+	if (!atomic_read(&wdata->ready))
+		return;
+
+	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)
 {
@@ -87,6 +172,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;
 }
 
@@ -137,8 +225,24 @@ static void wiimote_hid_remove(struct hid_device *hdev)
 	struct wiimote_data *wdata = hid_get_drvdata(hdev);
 
 	hid_info(hdev, "Device removed\n");
+
+	/*
+	 * We handle events from HID, input and sysfs and
+	 * send requests to HID and input. So stop sysfs
+	 * first, then lock HID output and stop HID.
+	 * Now only input is left so stop it and free the
+	 * the device data.
+	 */
+
+	atomic_set(&wdata->ready, 0);
+	cancel_work_sync(&wdata->worker);
+
 	hid_hw_stop(hdev);
 	input_unregister_device(wdata->input);
+
+	/* the worker may have been rescheduled so wait again for it */
+	cancel_work_sync(&wdata->worker);
+
 	kfree(wdata);
 }
 
-- 
1.7.5.2

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

[David Herrmann]

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

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
 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 687a469..0104e41 100644
--- a/drivers/hid/hid-wiimote.c
+++ b/drivers/hid/hid-wiimote.c
@@ -132,10 +132,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;
@@ -143,6 +155,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

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

[David Herrmann]

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 <dh.herrmann@googlemail.com>
---
 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 0104e41..4c3f5eb 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)
 {
@@ -132,6 +165,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;
@@ -139,6 +199,7 @@ struct wiiproto_handler {
 };
 
 static struct wiiproto_handler handlers[] = {
+	{ .id = WIIPROTO_REQ_DRM_K, .size = 2, .func = handler_keys },
 	{ .id = 0 }
 };
 
@@ -167,6 +228,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)
@@ -190,6 +252,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

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

[David Herrmann]

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 <dh.herrmann@googlemail.com>
---
 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 4c3f5eb..fbdd7ef 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,
 };
 
@@ -154,6 +160,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)
 {
@@ -294,6 +319,7 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 
 	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

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

[David Herrmann]

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 <dh.herrmann@googlemail.com>
---
 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 fbdd7ef..eb86c5d 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,
@@ -164,6 +173,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;
 
@@ -234,6 +248,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;
@@ -241,12 +256,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;
 }
 
@@ -284,6 +303,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;
 }
 
@@ -319,7 +340,12 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 
 	atomic_set(&wdata->ready, 1);
 	hid_info(hdev, "New device registered\n");
+
+	/* by default set led1 after device initialization */
+	spin_lock(&wdata->state.lock);
 	wiiproto_req_leds(wdata, WIIPROTO_FLAG_LED1);
+	spin_unlock(&wdata->state.lock);
+
 	return 0;
 
 err_stop:
-- 
1.7.5.2

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

[David Herrmann]

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

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
---
 drivers/hid/hid-wiimote.c |   74 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 74 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index eb86c5d..0d93d93 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)
 {
@@ -193,6 +196,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)
 {
@@ -320,6 +372,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");
@@ -352,6 +417,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);
 	kfree(wdata);
 	return ret;
 }
@@ -370,6 +439,11 @@ static void wiimote_hid_remove(struct hid_device *hdev)
 	 * the device data.
 	 */
 
+	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);
+
 	atomic_set(&wdata->ready, 0);
 	cancel_work_sync(&wdata->worker);
 
-- 
1.7.5.2

Re: [RFC 03/12] HID: wiimote: Add wiimote device structure

[Oliver Neukum]

Am Mittwoch, 15. Juni 2011, 01:45:48 schrieb David Herrmann:
>  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:
> +       kfree(wdata);
> +       return ret;
>  }


If you create wiimote_create(), you should for symmetry's sake also
create wiimote_destroy() encapsulating the kfree.

	Regards
		Oliver

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

[Oliver Neukum]

Am Mittwoch, 15. Juni 2011, 01:45:50 schrieb David Herrmann:
> 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.

Hi,

here the fun begins. Using atomic ops is not for the faint of heart.
The problem is that atomic_t guarantees atomicity, not ordering.
Tasks on on other CPUs reading wdata->ready==1 does not mean
that they'll read anything else written before that as it was written
or the old value. You need smp barriers.

> 
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> ---
>  drivers/hid/hid-wiimote.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
> index 9fa4fe1..4dbb2c1 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,22 @@ 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;
> +

Here an 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;
> +
>  	if (size < 1)
>  		return -EINVAL;
>  
> @@ -98,6 +110,7 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  		goto err_stop;
>  	}
>

Here an smp_wmb()
It is redundant because input_register_device takes a spinlock which
implies a barrier, but you'll make the automated checking tools happier.  
> +	atomic_set(&wdata->ready, 1);
>  	hid_info(hdev, "New device registered\n");
>  	return 0;

	Regards
		Oliver

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

[Oliver Neukum]

Am Mittwoch, 15. Juni 2011, 01:45:52 schrieb David Herrmann:

> +struct wiimote_buf {
> +	__u8 data[HID_MAX_BUFFER_SIZE];
> +	size_t size;
> +};

As this probably goes out over USB -> BT
you are violating the DMA buffer constraints.
Every buffer must be separately allocated with kmalloc().

>  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,
> @@ -36,6 +49,78 @@ static ssize_t wiimote_hid_send(struct hid_device *hdev, __u8 *buffer,
>  							HID_OUTPUT_REPORT);
>  }
>  
> +static void wiimote_worker(struct work_struct *work)
> +{
> +	struct wiimote_data *wdata = container_of(work, struct wiimote_data,
> +									worker);
> +
> +	if (!atomic_read(&wdata->ready))
> +		return;
> +
> +	spin_lock(&wdata->qlock);

You want _irqsave

> +	if (wdata->head != wdata->tail) {
> +		spin_unlock(&wdata->qlock);
> +		wiimote_hid_send(wdata->hdev, wdata->outq[wdata->tail].data,
> +						wdata->outq[wdata->tail].size);
> +		spin_lock(&wdata->qlock);
> +
> +		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(&wdata->qlock);
> +}

	Regards
		Oliver

Re: [RFC 11/12] HID: wiimote: Cache wiimote led state

[Oliver Neukum]

Am Mittwoch, 15. Juni 2011, 01:45:56 schrieb David Herrmann:
> @@ -319,7 +340,12 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  
>         atomic_set(&wdata->ready, 1);
>         hid_info(hdev, "New device registered\n");
> +
> +       /* by default set led1 after device initialization */
> +       spin_lock(&wdata->state.lock);

spin_lock_irq() or you might deadlock.

>         wiiproto_req_leds(wdata, WIIPROTO_FLAG_LED1);
> +       spin_unlock(&wdata->state.lock);
> +
>         return 0;

	Regards
		Oliver

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

[Jiri Kosina]

On Wed, 15 Jun 2011, Oliver Neukum wrote:

> > Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
> > ---
> >  drivers/hid/hid-wiimote.c |   13 +++++++++++++
> >  1 files changed, 13 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
> > index 9fa4fe1..4dbb2c1 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,22 @@ 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;
> > +
> 
> Here an 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;
> > +
> >  	if (size < 1)
> >  		return -EINVAL;
> >  
> > @@ -98,6 +110,7 @@ static int wiimote_hid_probe(struct hid_device *hdev,
> >  		goto err_stop;
> >  	}
> >
> 
> Here an smp_wmb()
> It is redundant because input_register_device takes a spinlock which
> implies a barrier, but you'll make the automated checking tools happier.  

Yeah, spinlock implies SMP barrier implicitly.

I don't think adding barriers just to make some buggy automatic checker 
happy is worth it.

Thanks for the review, Oliver.

-- 
Jiri Kosina
SUSE Labs

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

[Oliver Neukum]

Am Donnerstag, 16. Juni 2011, 11:20:18 schrieb Jiri Kosina:
> > Here an smp_wmb()
> > It is redundant because input_register_device takes a spinlock which
> > implies a barrier, but you'll make the automated checking tools happier.  
> 
> Yeah, spinlock implies SMP barrier implicitly.
> 
> I don't think adding barriers just to make some buggy automatic checker 
> happy is worth it.

Fine by me as long as the problem is understood.

	Regards
		Oliver

-- 
- - - 
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany 
- - - 
--
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: [RFC 05/12] HID: wiimote: Synchronize wiimote input and hid event handling

[David Herrmann]

On Thu, Jun 16, 2011 at 12:18 PM, Oliver Neukum <oneukum@suse.de> wrote:
> Am Donnerstag, 16. Juni 2011, 11:20:18 schrieb Jiri Kosina:
>> > Here an smp_wmb()
>> > It is redundant because input_register_device takes a spinlock which
>> > implies a barrier, but you'll make the automated checking tools happier.
>>
>> Yeah, spinlock implies SMP barrier implicitly.
>>
>> I don't think adding barriers just to make some buggy automatic checker
>> happy is worth it.
>
> Fine by me as long as the problem is understood.

Does that mean that I can omit the smp_rmb(), too? I do not like
relying on input_register_device to use a spinlock. It makes reading
the code hard, so I added both barriers. Should I remove them again or
keep them?

Thank you for reviewing. I have fixed all your concerns. I will resend
the patches if there are no more comments on the code as [PATCH] in
few days.

Regards
David

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

[Oliver Neukum]

Am Donnerstag, 16. Juni 2011, 12:29:40 schrieb David Herrmann:
> On Thu, Jun 16, 2011 at 12:18 PM, Oliver Neukum <oneukum@suse.de> wrote:
> > Am Donnerstag, 16. Juni 2011, 11:20:18 schrieb Jiri Kosina:
> >> > Here an smp_wmb()
> >> > It is redundant because input_register_device takes a spinlock which
> >> > implies a barrier, but you'll make the automated checking tools happier.
> >>
> >> Yeah, spinlock implies SMP barrier implicitly.
> >>
> >> I don't think adding barriers just to make some buggy automatic checker
> >> happy is worth it.
> >
> > Fine by me as long as the problem is understood.
> 
> Does that mean that I can omit the smp_rmb(), too? I do not like
> relying on input_register_device to use a spinlock. It makes reading
> the code hard, so I added both barriers. Should I remove them again or
> keep them?

The full explanation is in Documentation/atomic-ops.txt
Basically, whenever you do

if (!descriptor->flag)
	return -ERR;

descriptor->pointer->member = expression; //or anything else with the pointer

you need smp_rmb() in between

If you do

if (!descriptor->flag)
	return -ERR;

// no use of pointer here

spin_lock(&lock);

descriptor->pointer->member = expression; //or anything else with the pointer

You are save and can do without the smp_rmb()

But, you still need smp_rmb() if you do:

if (!descriptor->flag)
	return -ERR;

// no use of pointer here

spin_lock(&descriptor->pointer->lock);

All clear? I must say I stopped listing and checking all the places you use
the flag, but I think some were buggy.

	Regards
		Oliver

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

[Jiri Kosina]

On Wed, 15 Jun 2011, David Herrmann wrote:

> Add sysfs files for each led of the wiimote. Writing 1 to the file
> enables the led and 0 disables the led.
> 
> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>

As sysfs is part of kernel ABI, please document it in Documentation/ABI.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [RFC 00/12] Nintendo Wii Remote Device Driver

[Jiri Kosina]

On Wed, 15 Jun 2011, David Herrmann wrote:

> I currently work on an HID driver for the Nintendo Wii Remote as Bluez GSoC
> project. Following, the basic driver implementation with wiimote button-input
> and led support.

David,

generally, I like the driver. And I like the way you are submitting it in 
gradual self-contained steps in the series.

Thanks.

Once you incorporate Oliver's concerns, please re-submit it. I'd be happy 
to merge it when the review has been completed.

-- 
Jiri Kosina
SUSE Labs

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

[David Herrmann]

On Thu, Jun 16, 2011 at 8:35 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 15 Jun 2011, David Herrmann wrote:
>
>> Add sysfs files for each led of the wiimote. Writing 1 to the file
>> enables the led and 0 disables the led.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>
>
> As sysfs is part of kernel ABI, please document it in Documentation/ABI.

I've already documented it in the distribution package:
https://github.com/dvdhrm/xwiimote/blob/master/doc/INTERFACE_SYSFS
I forgot to include it in the patch, I now added the important parts.

> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs

Thanks
David