[PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Mike Murphy]

[-- Attachment #1: Type: text/plain, Size: 2036 bytes --]

Greetings,

The attached patch is a result of a few days of hacking to get better
Linux support for Xbox 360 wireless controllers. Major functional
improvements include support for the LEDs on the wireless controllers
(which are auto-set to the controller number as they are on the
official console), added support for rumble effects on wireless
controllers, added support for dead zones on all supported
controllers, and added support for a square axis mapping for the left
stick on all supported controllers. A large amount of the bulk in this
patch is from the addition of a sysfs interface to retrieve
information and adjust the dead zone/square axis settings.

I also fixed a few bugs:

- Removed the bulk urb altogether for setting the LED display. This
operation is now done using the irq_out urb in xpad_send_led_command
(via a shared work queue task to get out of the interrupt handler when
processing the presence change packet).

- Submission of urbs while holding a mutex is now GFP_ATOMIC

- The odata_mutex is now locked when changing the odata for sending a
force feedback effect.

There is still a bug somewhere in the force feedback code, I believe
in either ff-memless or lower, that causes a "warn_on_slowpath"
message in the log and an occasional "scheduling while atomic" BUG. I
have thus far been unable to isolate it.

This code has been tested only with a wireless 360 receiver, 4
controllers (individually and concurrently), and a RedOctane wireless
guitar. It needs more testing with a broader range of devices,
especially wired devices.

Patch generated against 2.6.28.2 (no changes in relevant in-kernel
driver from 2.6.28.2 to 2.6.28.5, so should apply against latest
stable).

Comments are appreciated. I am _NOT_ subscribed to the LKML, so please
CC me directly.

Thanks,
Mike
-- 
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838   Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph

[-- Attachment #2: xpad.patch --]
[-- Type: text/x-patch, Size: 36410 bytes --]

--- origdrv/drivers/input/joystick/xpad.c	2009-02-14 22:39:20.000000000 -0500
+++ newdrv/drivers/input/joystick/xpad.c	2009-02-14 22:41:27.000000000 -0500
@@ -1,5 +1,8 @@
 /*
- * X-Box gamepad driver
+ * Xbox gamepad driver with Xbox 360 wired/wireless support
+ *
+ * Last Modified:	14 February 2009
+ *			Mike Murphy <mamurph@cs.clemson.edu>
  *
  * Copyright (c) 2002 Marko Friedemann <mfr@bmx-chemnitz.de>
  *               2004 Oliver Schwartz <Oliver.Schwartz@gmx.de>,
@@ -9,6 +12,7 @@
  *               2005 Dominic Cerquetti <binary1230@yahoo.com>
  *               2006 Adam Buchbinder <adam.buchbinder@gmail.com>
  *               2007 Jan Kratochvil <honza@jikos.cz>
+ *               2009 Clemson University
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License as
@@ -30,6 +34,7 @@
  *  - the iForce driver    drivers/char/joystick/iforce.c
  *  - the skeleton-driver  drivers/usb/usb-skeleton.c
  *  - Xbox 360 information http://www.free60.org/wiki/Gamepad
+ *  - xboxdrv docs         http://pingus.seul.org/~grumbel/xboxdrv/
  *
  * Thanks to:
  *  - ITO Takayuki for providing essential xpad information on his website
@@ -38,10 +43,16 @@
  *  - XBOX Linux project - extra USB id's
  *
  * TODO:
- *  - fine tune axes (especially trigger axes)
  *  - fix "analog" buttons (reported as digital now)
- *  - get rumble working
  *  - need USB IDs for other dance pads
+ *  - fix input descriptor leak (requires finding cause)
+ *    - to reproduce behavior:
+ *      $ ls /sys/class/input
+ *      # rmmod xpad
+ *      # insmod ./xpad.ko
+ *      $ ls /sys/class/input
+ *  - fix warn_on_slowpath and occasional scheduling while atomic BUGs when
+ *    using rumble (bug results from use of a timer somewhere in ff-memless).
  *
  * History:
  *
@@ -69,7 +80,34 @@
  *  - pass the module paramater 'dpad_to_buttons' to force
  *    the D-PAD to map to buttons if your pad is not detected
  *
- * Later changes can be tracked in SCM.
+ * 2009-02-02 : Wireless 360 controller fixes
+ *  - followed PROTOCOL description from xboxdrv userspace driver
+ *  - LED and rumble support added for wireless 360 controller (protocol
+ *    is different from wired!)
+ *
+ * 2009-02-06 : Axis handling improvements
+ *  - unified handler for left and right sticks
+ *  - initial support for dead zones
+ *
+ * 2009-02-07 : More wireless 360 controller fixes
+ *  - removed bulk urb completely
+ *  - use xpad_send_led_command to set controller number on LED display
+ *    (wireless 360 controller)
+ *  - fixed urb submission while holding mutex to GFP_ATOMIC
+ *  - dead_zone is now an adjustable module parameter
+ *
+ * 2009-02-12 : Scaling for dead zone and square axis support
+ *  - axes now scale from 0 to 32767 starting at edge of dead zone
+ *  - increased default dead zone to 8192
+ *  - initial square axis support (reliable only with left stick)
+ *
+ * 2009-02-13 : Disable square axis for right stick
+ *  - square axis applies to left stick only
+ *
+ * 2009-02-14 : Added sysfs interface
+ *  - dead zones and square axis settings can now be made per-controller
+ *  - removed dead_zone and square_axis module parameters (use sysfs)
+ *  - new square axis algorithm
  */
 
 #include <linux/kernel.h>
@@ -78,9 +116,14 @@
 #include <linux/stat.h>
 #include <linux/module.h>
 #include <linux/usb/input.h>
+#include <linux/workqueue.h>
+#include <linux/kobject.h>
+#include <linux/sysfs.h>
+#include <linux/string.h>
+
 
 #define DRIVER_AUTHOR "Marko Friedemann <mfr@bmx-chemnitz.de>"
-#define DRIVER_DESC "X-Box pad driver"
+#define DRIVER_DESC "X-Box/360 pad driver"
 
 #define XPAD_PKT_LEN 32
 
@@ -90,62 +133,127 @@
 #define MAP_DPAD_TO_AXES       1
 #define MAP_DPAD_UNKNOWN       2
 
+/* Type of controller *interface* (original, wired 360, wireless 360) */
 #define XTYPE_XBOX        0
 #define XTYPE_XBOX360     1
 #define XTYPE_XBOX360W    2
 #define XTYPE_UNKNOWN     3
 
-static int dpad_to_buttons;
+/* Type of controller (e.g. pad, guitar, other input device) */
+#define XCONTROLLER_TYPE_NONE		0
+#define XCONTROLLER_TYPE_PAD		1
+#define XCONTROLLER_TYPE_GUITAR		2
+#define XCONTROLLER_TYPE_DANCE_PAD	3
+#define XCONTROLLER_TYPE_OTHER		255
+
+
+/* The Xbox 360 controllers have sensitive sticks that often do not center
+ * exactly. A dead zone causes stick events below a certain threshhold to be
+ * reported as zero.
+ *
+ * The default dead zone size is 8192, which was obtained by testing a
+ * wireless 360 controller with jstest(1) and consulting gaming forums for
+ * a recommended dead zone for this controller. The consensus opinion was
+ * 0.25 (on a scale from 0 to 1), which corresponds to 8192 (out of 32767).
+ */
+#define XDEAD_ZONE_DEFAULT   8192
+
+/* Default axes for the sticks are radial */
+#define XSQUARE_AXIS_DEFAULT 0
+
+static int dpad_to_buttons = 0;
 module_param(dpad_to_buttons, bool, S_IRUGO);
 MODULE_PARM_DESC(dpad_to_buttons, "Map D-PAD to buttons rather than axes for unknown pads");
 
+
 static const struct xpad_device {
 	u16 idVendor;
 	u16 idProduct;
 	char *name;
 	u8 dpad_mapping;
 	u8 xtype;
+	u8 controller_type;
 } xpad_device[] = {
-	{ 0x045e, 0x0202, "Microsoft X-Box pad v1 (US)", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x045e, 0x0289, "Microsoft X-Box pad v2 (US)", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x045e, 0x0285, "Microsoft X-Box pad (Japan)", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x045e, 0x0287, "Microsoft Xbox Controller S", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x045e, 0x0719, "Xbox 360 Wireless Receiver", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX360W },
-	{ 0x0c12, 0x8809, "RedOctane Xbox Dance Pad", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX },
-	{ 0x044f, 0x0f07, "Thrustmaster, Inc. Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x046d, 0xc242, "Logitech Chillstream Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX360 },
-	{ 0x046d, 0xca84, "Logitech Xbox Cordless Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x046d, 0xca88, "Logitech Compact Controller for Xbox", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x05fd, 0x1007, "Mad Catz Controller (unverified)", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x05fd, 0x107a, "InterAct 'PowerPad Pro' X-Box pad (Germany)", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0738, 0x4516, "Mad Catz Control Pad", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0738, 0x4522, "Mad Catz LumiCON", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0738, 0x4526, "Mad Catz Control Pad Pro", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0738, 0x4536, "Mad Catz MicroCON", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0738, 0x4540, "Mad Catz Beat Pad", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX },
-	{ 0x0738, 0x4556, "Mad Catz Lynx Wireless Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0738, 0x4716, "Mad Catz Wired Xbox 360 Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX360 },
-	{ 0x0738, 0x6040, "Mad Catz Beat Pad Pro", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX },
-	{ 0x0c12, 0x8802, "Zeroplus Xbox Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0c12, 0x880a, "Pelican Eclipse PL-2023", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0c12, 0x8810, "Zeroplus Xbox Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0c12, 0x9902, "HAMA VibraX - *FAULTY HARDWARE*", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0e4c, 0x1097, "Radica Gamester Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0e4c, 0x2390, "Radica Games Jtech Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0e6f, 0x0003, "Logic3 Freebird wireless Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0e6f, 0x0005, "Eclipse wireless Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0e6f, 0x0006, "Edge wireless Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0e6f, 0x0006, "Pelican 'TSZ' Wired Xbox 360 Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX360 },
-	{ 0x0e8f, 0x0201, "SmartJoy Frag Xpad/PS2 adaptor", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0f30, 0x0202, "Joytech Advanced Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0f30, 0x8888, "BigBen XBMiniPad Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x102c, 0xff0c, "Joytech Wireless Advanced Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x12ab, 0x8809, "Xbox DDR dancepad", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX },
-	{ 0x1430, 0x4748, "RedOctane Guitar Hero X-plorer", MAP_DPAD_TO_AXES, XTYPE_XBOX360 },
-	{ 0x1430, 0x8888, "TX6500+ Dance Pad (first generation)", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX },
-	{ 0x045e, 0x028e, "Microsoft X-Box 360 pad", MAP_DPAD_TO_AXES, XTYPE_XBOX360 },
-	{ 0xffff, 0xffff, "Chinese-made Xbox Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX },
-	{ 0x0000, 0x0000, "Generic X-Box pad", MAP_DPAD_UNKNOWN, XTYPE_UNKNOWN }
+	{ 0x045e, 0x0202, "Microsoft X-Box pad v1 (US)", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x045e, 0x0289, "Microsoft X-Box pad v2 (US)", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x045e, 0x0285, "Microsoft X-Box pad (Japan)", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x045e, 0x0287, "Microsoft Xbox Controller S", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x045e, 0x0719, "Xbox 360 Wireless Receiver", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX360W,
+		XCONTROLLER_TYPE_NONE },
+	{ 0x0c12, 0x8809, "RedOctane Xbox Dance Pad", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX,
+		XCONTROLLER_TYPE_DANCE_PAD },
+	{ 0x044f, 0x0f07, "Thrustmaster, Inc. Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x046d, 0xc242, "Logitech Chillstream Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX360,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x046d, 0xca84, "Logitech Xbox Cordless Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x046d, 0xca88, "Logitech Compact Controller for Xbox", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x05fd, 0x1007, "Mad Catz Controller (unverified)", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x05fd, 0x107a, "InterAct 'PowerPad Pro' X-Box pad (Germany)", MAP_DPAD_TO_AXES,
+		XTYPE_XBOX, XCONTROLLER_TYPE_PAD },
+	{ 0x0738, 0x4516, "Mad Catz Control Pad", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0738, 0x4522, "Mad Catz LumiCON", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0738, 0x4526, "Mad Catz Control Pad Pro", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0738, 0x4536, "Mad Catz MicroCON", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0738, 0x4540, "Mad Catz Beat Pad", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX,
+		XCONTROLLER_TYPE_DANCE_PAD },
+	{ 0x0738, 0x4556, "Mad Catz Lynx Wireless Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0738, 0x4716, "Mad Catz Wired Xbox 360 Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX360,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0738, 0x6040, "Mad Catz Beat Pad Pro", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX,
+		XCONTROLLER_TYPE_DANCE_PAD },
+	{ 0x0c12, 0x8802, "Zeroplus Xbox Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0c12, 0x880a, "Pelican Eclipse PL-2023", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0c12, 0x8810, "Zeroplus Xbox Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0c12, 0x9902, "HAMA VibraX - *FAULTY HARDWARE*", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0e4c, 0x1097, "Radica Gamester Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0e4c, 0x2390, "Radica Games Jtech Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0e6f, 0x0003, "Logic3 Freebird wireless Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0e6f, 0x0005, "Eclipse wireless Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0e6f, 0x0006, "Edge wireless Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0e6f, 0x0006, "Pelican 'TSZ' Wired Xbox 360 Controller", MAP_DPAD_TO_AXES,
+		XTYPE_XBOX360, XCONTROLLER_TYPE_PAD },
+	{ 0x0e8f, 0x0201, "SmartJoy Frag Xpad/PS2 adaptor", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0f30, 0x0202, "Joytech Advanced Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0f30, 0x8888, "BigBen XBMiniPad Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x102c, 0xff0c, "Joytech Wireless Advanced Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x12ab, 0x8809, "Xbox DDR dancepad", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX,
+		XCONTROLLER_TYPE_DANCE_PAD },
+	{ 0x1430, 0x4748, "RedOctane Guitar Hero X-plorer", MAP_DPAD_TO_AXES, XTYPE_XBOX360,
+		XCONTROLLER_TYPE_GUITAR },
+	{ 0x1430, 0x8888, "TX6500+ Dance Pad (first generation)", MAP_DPAD_TO_BUTTONS, XTYPE_XBOX,
+		XCONTROLLER_TYPE_DANCE_PAD },
+	{ 0x045e, 0x028e, "Microsoft X-Box 360 pad", MAP_DPAD_TO_AXES, XTYPE_XBOX360,
+		XCONTROLLER_TYPE_PAD },
+	{ 0xffff, 0xffff, "Chinese-made Xbox Controller", MAP_DPAD_TO_AXES, XTYPE_XBOX,
+		XCONTROLLER_TYPE_PAD },
+	{ 0x0000, 0x0000, "Generic X-Box pad", MAP_DPAD_UNKNOWN, XTYPE_UNKNOWN,
+		XCONTROLLER_TYPE_PAD }
 };
 
 /* buttons shared with xbox and xbox360 */
@@ -213,19 +321,80 @@
 
 MODULE_DEVICE_TABLE (usb, xpad_table);
 
+
+/* Wireless 360 device identification. See lengthy notes about kobject data
+ * below.
+ */
+static const struct w360_id {
+	u32 id_bytes;
+	u8 controller_type;
+} w360_id[] = {
+	{ 0x40415001, XCONTROLLER_TYPE_PAD },
+	{ 0x44305107, XCONTROLLER_TYPE_GUITAR },
+	{ 0x00000000, XCONTROLLER_TYPE_NONE }
+};
+
+
+/* kobject data for sysfs. Some of these fields are read-only, mostly for
+ * later use with userspace applications to recognize individual controllers.
+ * The dead_zone and square_axis settings can be changed "on the fly" and are
+ * effective immediately.
+ *
+ * Most of the read-only fields are to support *wireless* 360 controllers.
+ * The controller_number is used to set the LED, while pad_present tracks
+ * whether the correpsonding controller is connected to the wireless receiver.
+ * Controller type applies to all models (wired and wireless), and tracks
+ * whether the device is a pad, guitar, etc. for later userspace use.
+ *
+ * The controller_unique_id is a field of bits in a particular wireless
+ * packet that SEEM to correspond to some kind of unique ID built into the
+ * controller. Using usbmon (see Documentation/usb/usbmon.txt), I tried 4
+ * gamepads and a guitar, and I got the following 5 packets at connection
+ * time for each device, respectively.
+ *
+ * 000f00f0 00ccfd27 0060e226 63700010 13e3201d 30034001 5001ffff ff
+ * 000f00f0 f0ccfd27 0060d8c4 e9600009 13e7201d 30034001 5001ffff ff
+ * 000f00f0 00ccfd27 0060578b 82f00010 13e3201d 30034001 5001ffff ff
+ * 000f00f0 f0ccfd27 0060da1c b1500009 13e7201d 30034001 5001ffff ff
+ * 000f00f0 f0ccfd27 006002d1 71d10000 13e3201d 30034430 5107ffff ff
+ *
+ * From this trace data, I concocted the following (potentially incorrect)
+ * scheme for detecting type and unique ID:
+ *
+ * ******** xx****xx xxxxxxxx xxxx**xx **xx**** ****tttt tttt**** **
+ *                |  unique id |                    |  type |
+ *
+ * Further testing over a wider variety of devices is probably needed to
+ * determine if changes need to be made to this scheme.
+ */
+struct xpad_data {
+	struct kobject kobj;		/* kobject for sysfs */
+	int controller_number;		/* controller number (1-4) for 360w. ro */
+	int pad_present;                /* 360w pad presence detection. ro */
+	int controller_type;            /* controller type. ro */
+	char controller_unique_id[17];  /* unique ID of controller (360w). ro */
+	unsigned int dead_zone;         /* dead zone for both sticks. rw */
+	int square_axis;                /* square axis for left stick. rw */
+};
+#define to_xpad_data(k) container_of(k, struct xpad_data, kobj)
+
+/* attribute structure for sysfs */
+struct xpad_attribute {
+	struct attribute attr;
+	ssize_t (*show)(struct xpad_data *xd, struct xpad_attribute *attr, char *buf);
+	ssize_t (*store)(struct xpad_data *xd, struct xpad_attribute *attr, const char *buf,
+		size_t count);
+};
+#define to_xpad_attr(k) container_of(k, struct xpad_attribute, attr)
+
 struct usb_xpad {
 	struct input_dev *dev;		/* input device interface */
 	struct usb_device *udev;	/* usb device */
 
-	int pad_present;
-
 	struct urb *irq_in;		/* urb for interrupt in report */
 	unsigned char *idata;		/* input data */
 	dma_addr_t idata_dma;
 
-	struct urb *bulk_out;
-	unsigned char *bdata;
-
 #if defined(CONFIG_JOYSTICK_XPAD_FF) || defined(CONFIG_JOYSTICK_XPAD_LEDS)
 	struct urb *irq_out;		/* urb for interrupt out report */
 	unsigned char *odata;		/* output data */
@@ -241,8 +410,286 @@
 
 	int dpad_mapping;		/* map d-pad to buttons or to axes */
 	int xtype;			/* type of xbox device */
+	
+	/* Work structure for moving the call to xpad_send_led_command
+	 * outside the interrupt handler for packet processing */
+	struct work_struct work;
+	
+	/* Controller data is now in a kobject for use with sysfs */
+	struct xpad_data *controller_data;
 };
 
+
+/****************************************************************************/
+/*
+ * SysFs interface implemented using Documentation/kobject.txt and examples in
+ * samples/kobject.
+ */
+static ssize_t xpad_attr_show(struct kobject *kobj,
+			     struct attribute *attr,
+			     char *buf)
+{
+	struct xpad_attribute *attribute;
+	struct xpad_data *xd;
+
+	attribute = to_xpad_attr(attr);
+	xd = to_xpad_data(kobj);
+
+	if (!attribute->show)
+		return -EIO;
+
+	return attribute->show(xd, attribute, buf);
+}
+
+static ssize_t xpad_attr_store(struct kobject *kobj,
+			      struct attribute *attr,
+			      const char *buf, size_t len)
+{
+	struct xpad_attribute *attribute;
+	struct xpad_data *xd;
+
+	attribute = to_xpad_attr(attr);
+	xd = to_xpad_data(kobj);
+
+	if (!attribute->store)
+		return -EIO;
+
+	return attribute->store(xd, attribute, buf, len);
+}
+
+static struct sysfs_ops xpad_sysfs_ops = {
+	.show = xpad_attr_show,
+	.store = xpad_attr_store,
+};
+
+static void xpad_release(struct kobject *kobj)
+{
+	struct xpad_data *xd;
+	xd = to_xpad_data(kobj);
+	kfree(xd);
+}
+
+static ssize_t xpad_show_dead_zone(struct xpad_data *xd,
+	struct xpad_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", xd->dead_zone);
+}
+
+static ssize_t xpad_store_dead_zone(struct xpad_data *xd,
+	struct xpad_attribute *attr, const char *buf, size_t count)
+{
+	int newdz;
+	sscanf(buf, "%d", &newdz);
+	if ((newdz >= 0) && (newdz < 32760)) {
+		xd->dead_zone = newdz;
+	}
+	return count;
+}
+
+static ssize_t xpad_show_square_axis(struct xpad_data *xd,
+	struct xpad_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", xd->square_axis);
+}
+
+static ssize_t xpad_store_square_axis(struct xpad_data *xd,
+	struct xpad_attribute *attr, const char *buf, size_t count)
+{
+	int newaxis = 0;
+	sscanf(buf, "%d", &newaxis);
+	xd->square_axis = (newaxis) ? 1 : 0;
+	return count;
+}
+
+/* read-only attrs */
+static ssize_t xpad_show_int(struct xpad_data *xd, struct xpad_attribute *attr,
+	char *buf)
+{
+	int value;
+	if (!strcmp(attr->attr.name, "controller_number"))
+		value = xd->controller_number;
+	else if (!strcmp(attr->attr.name, "pad_present"))
+		value = xd->pad_present;
+	else if (!strcmp(attr->attr.name, "controller_type"))
+		value = xd->controller_type;
+	else
+		value = 0;
+	return sprintf(buf, "%d\n", value);
+}
+
+static ssize_t xpad_show_id(struct xpad_data *xd, struct xpad_attribute *attr,
+	char *buf)
+{
+	return sprintf(buf, "%s\n", xd->controller_unique_id);
+}
+
+/* maps */
+static struct xpad_attribute controller_number_attr =
+	__ATTR(controller_number, 0444, xpad_show_int, NULL);
+static struct xpad_attribute pad_present_attr =
+	__ATTR(pad_present, 0444, xpad_show_int, NULL);
+static struct xpad_attribute controller_type_attr =
+	__ATTR(controller_type, 0444, xpad_show_int, NULL);
+static struct xpad_attribute controller_unique_id_attr =
+	__ATTR(controller_unique_id, 0444, xpad_show_id, NULL);
+
+/* rw maps */
+static struct xpad_attribute dead_zone_attr =
+	__ATTR(dead_zone, 0666, xpad_show_dead_zone, xpad_store_dead_zone);
+static struct xpad_attribute square_axis_attr =
+	__ATTR(square_axis, 0666, xpad_show_square_axis, xpad_store_square_axis);
+
+
+static struct attribute *xpad_default_attrs[] = {
+	&controller_number_attr.attr,
+	&pad_present_attr.attr,
+	&controller_type_attr.attr,
+	&controller_unique_id_attr.attr,
+	&dead_zone_attr.attr,
+	&square_axis_attr.attr,
+	NULL
+};
+
+static struct kobj_type xpad_ktype = {
+	.sysfs_ops = &xpad_sysfs_ops,
+	.release = xpad_release,
+	.default_attrs = xpad_default_attrs,
+};
+
+static struct xpad_data *xpad_create_data(const char *name, struct kobject *parent) {
+	struct xpad_data *data = NULL;
+	int check;
+	
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return NULL;
+	
+	check = kobject_init_and_add(&data->kobj, &xpad_ktype, parent, "%s", name);
+	if (check) {
+		kobject_put(&data->kobj);
+		return NULL;
+	}
+	
+	kobject_uevent(&data->kobj, KOBJ_ADD);
+	return data;
+}
+
+static void xpad_destroy_data(struct xpad_data *data)
+{
+	kobject_put(&data->kobj);
+}
+
+/*****************************************************************************/
+
+
+/* Need prototype due to module order */
+static void xpad_send_led_command(struct usb_xpad *xpad, int command);
+
+
+/*
+ *	xpad_work_controller
+ *
+ *	Submits command to set pad number on LED display of wireless 360
+ *	controllers. The shared workqueue is used for this purpose, so that
+ *	the interrupt handler is kept short.
+ */
+static void xpad_work_controller(struct work_struct *w)
+{
+   struct usb_xpad * xpad = container_of(w, struct usb_xpad, work);
+   xpad_send_led_command(xpad, xpad->controller_data->controller_number + 1);
+}
+
+
+/*
+ *	xpad_process_sticks
+ *
+ *	Handles stick input, accounting for dead zones and square axes. Based
+ *	on the original handlers for the Xbox and Xbox 360 in
+ *	xpad_process_packet and xpad360_process_packet, but unified to avoid
+ *	duplication.
+ *
+ *	Whenever a dead zone is used, each axis is scaled so that moving the
+ *	stick slightly out of the dead zone range results in a low axis
+ *	value in jstest(1), while moving the stick to the maximum position
+ *	along any axis still results in 32767.
+ *
+ *	Square axis algorithm: for the LEFT STICK ONLY (X and Y axes), the
+ *	coordinate system can be set to be more like classic controllers
+ *	and PC joysticks. This mapping is accomplished conceptually by
+ *	inscribing a square inside the radial opening of the plastic case
+ *	on the controller, which has a radius of 1.0 on a floating-point
+ *	scale (23170 on the integer scale). As a result, the X and Y axis
+ *	will have a smaller range, reaching 32767 at the point that was
+ *	formerly 23170. In practice, the implementation is fairly complex
+ *	to understand, due to the need to integrate it with the dead
+ *	zone scaling.
+ */
+static void xpad_process_sticks(struct usb_xpad *xpad, unsigned char *data)
+{
+	struct input_dev *dev = xpad->dev;
+	int coords[4];    /* x, y, rx, ry */
+	int x_offset, y_offset, rx_offset, ry_offset;
+	int tmp, adjust, c;
+	int abs_magnitude;
+	int dead_zone, square_axis;
+	
+	dead_zone = xpad->controller_data->dead_zone;
+	square_axis = xpad->controller_data->square_axis;
+	
+	if (xpad->xtype == XTYPE_XBOX) {
+		x_offset = 12;
+		y_offset = 14;
+		rx_offset = 16;
+		ry_offset = 18;
+	}
+	else {
+		x_offset = 6;
+		y_offset = 8;
+		rx_offset = 10;
+		ry_offset = 12;
+	}
+	
+	coords[0] = (__s16) le16_to_cpup((__le16 *)(data + x_offset));
+	coords[1] = ~(__s16) le16_to_cpup((__le16 *)(data + y_offset));
+	coords[2] = (__s16) le16_to_cpup((__le16 *)(data + rx_offset));
+	coords[3] = ~(__s16) le16_to_cpup((__le16 *)(data + ry_offset));
+	
+	/* Adjustment for dead zone and square axis */
+	for (c = 0; c < 4; c++) {
+		adjust = 0;
+		abs_magnitude = (int) coords[c];
+		if (abs_magnitude < 0)
+			abs_magnitude = -abs_magnitude;
+		
+		if (dead_zone || square_axis) {
+			adjust = (square_axis && (c < 2)) ? 23170 : 32767;
+			/* Careful: removing the following check will result
+			 * in division by zero if the dead_zone is zero... */
+			if (dead_zone)
+				adjust = (adjust - dead_zone) / dead_zone;
+			if (adjust < 0) {
+				adjust = 0;
+			}
+		}
+	
+		if (abs_magnitude < dead_zone) {
+			coords[c] = 0;
+		}
+		else if (adjust) {
+			tmp = ((abs_magnitude - dead_zone) / adjust) + abs_magnitude - dead_zone;
+			if (tmp > 32767) {
+				tmp = 32767;
+			}
+			coords[c] = (coords[c] < 0) ? -tmp : tmp;
+		}
+	}
+	
+	input_report_abs(dev, ABS_X, (__s16) coords[0]);
+	input_report_abs(dev, ABS_Y, (__s16) coords[1]);
+	input_report_abs(dev, ABS_RX, (__s16) coords[2]);
+	input_report_abs(dev, ABS_RY, (__s16) coords[3]);
+}
+
 /*
  *	xpad_process_packet
  *
@@ -256,18 +703,9 @@
 static void xpad_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
 {
 	struct input_dev *dev = xpad->dev;
-
-	/* left stick */
-	input_report_abs(dev, ABS_X,
-			 (__s16) le16_to_cpup((__le16 *)(data + 12)));
-	input_report_abs(dev, ABS_Y,
-			 ~(__s16) le16_to_cpup((__le16 *)(data + 14)));
-
-	/* right stick */
-	input_report_abs(dev, ABS_RX,
-			 (__s16) le16_to_cpup((__le16 *)(data + 16)));
-	input_report_abs(dev, ABS_RY,
-			 ~(__s16) le16_to_cpup((__le16 *)(data + 18)));
+	
+	/* left and right sticks */
+	xpad_process_sticks(xpad, data);
 
 	/* triggers left/right */
 	input_report_abs(dev, ABS_Z, data[10]);
@@ -350,18 +788,9 @@
 	input_report_key(dev, BTN_TL,	data[3] & 0x01);
 	input_report_key(dev, BTN_TR,	data[3] & 0x02);
 	input_report_key(dev, BTN_MODE,	data[3] & 0x04);
-
-	/* left stick */
-	input_report_abs(dev, ABS_X,
-			 (__s16) le16_to_cpup((__le16 *)(data + 6)));
-	input_report_abs(dev, ABS_Y,
-			 ~(__s16) le16_to_cpup((__le16 *)(data + 8)));
-
-	/* right stick */
-	input_report_abs(dev, ABS_RX,
-			 (__s16) le16_to_cpup((__le16 *)(data + 10)));
-	input_report_abs(dev, ABS_RY,
-			 ~(__s16) le16_to_cpup((__le16 *)(data + 12)));
+	
+	/* left and right sticks */
+	xpad_process_sticks(xpad, data);
 
 	/* triggers left/right */
 	input_report_abs(dev, ABS_Z, data[4]);
@@ -379,6 +808,7 @@
  * Byte.Bit
  * 00.1 - Status change: The controller or headset has connected/disconnected
  *                       Bits 01.7 and 01.6 are valid
+ * 01.f - Some kind of unique identifier message (see above)
  * 01.7 - Controller present
  * 01.6 - Headset present
  * 01.1 - Pad state (Bytes 4+) valid
@@ -387,20 +817,50 @@
 
 static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
 {
-	/* Presence change */
+	int padnum = 0;
+	u32 id;
+	int i;
+
+	/* Presence change */	
 	if (data[0] & 0x08) {
+		padnum = xpad->controller_data->controller_number;
 		if (data[1] & 0x80) {
-			xpad->pad_present = 1;
-			usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
-		} else
-			xpad->pad_present = 0;
+			printk(KERN_INFO "Wireless Xbox 360 pad #%d present\n", padnum);
+			xpad->controller_data->pad_present = 1;
+			
+			INIT_WORK(&xpad->work, &xpad_work_controller);
+			schedule_work(&xpad->work);
+		}
+		else {
+			printk(KERN_INFO "Wireless Xbox 360 pad #%d disconnected\n", padnum);
+			xpad->controller_data->pad_present = 0;
+			xpad->controller_data->controller_unique_id[0] = '\0';
+			xpad->controller_data->controller_type = XCONTROLLER_TYPE_NONE;
+		}
 	}
 
-	/* Valid pad data */
-	if (!(data[1] & 0x1))
-		return;
-
-	xpad360_process_packet(xpad, cmd, &data[4]);
+	/* Process packets according to type */
+	if (data[1] == 0x0f) {
+		if (! xpad->controller_data->controller_unique_id[0]) {
+			snprintf(xpad->controller_data->controller_unique_id, 17,
+				"%02x%02x%02x%02x%02x%02x%02x%02x",
+				data[8], data[9], data[10], data[11],
+				data[12], data[13], data[14], data[15]);
+			
+			/* Identify controller type */
+			id = (data[22] << 24) + (data[23] << 16) + (data[24] << 8) + data[25];
+			xpad->controller_data->controller_type = XCONTROLLER_TYPE_OTHER;
+			for (i = 0; w360_id[i].id_bytes; i++) {
+				if (id == w360_id[i].id_bytes) {
+					xpad->controller_data->controller_type =
+						w360_id[i].controller_type;
+				}
+			}
+		}
+	}
+	else if (data[1] & 0x1) {
+		xpad360_process_packet(xpad, cmd, &data[4]);
+	}
 }
 
 static void xpad_irq_in(struct urb *urb)
@@ -445,23 +905,6 @@
 		     __func__, retval);
 }
 
-static void xpad_bulk_out(struct urb *urb)
-{
-	switch (urb->status) {
-	case 0:
-		/* success */
-		break;
-	case -ECONNRESET:
-	case -ENOENT:
-	case -ESHUTDOWN:
-		/* this urb is terminated, clean up */
-		dbg("%s - urb shutting down with status: %d", __func__, urb->status);
-		break;
-	default:
-		dbg("%s - nonzero urb status received: %d", __func__, urb->status);
-	}
-}
-
 #if defined(CONFIG_JOYSTICK_XPAD_FF) || defined(CONFIG_JOYSTICK_XPAD_LEDS)
 static void xpad_irq_out(struct urb *urb)
 {
@@ -498,7 +941,7 @@
 	struct usb_endpoint_descriptor *ep_irq_out;
 	int error = -ENOMEM;
 
-	if (xpad->xtype != XTYPE_XBOX360)
+	if ((xpad->xtype != XTYPE_XBOX360) && (xpad->xtype != XTYPE_XBOX360W))
 		return 0;
 
 	xpad->odata = usb_buffer_alloc(xpad->udev, XPAD_PKT_LEN,
@@ -528,13 +971,13 @@
 
 static void xpad_stop_output(struct usb_xpad *xpad)
 {
-	if (xpad->xtype == XTYPE_XBOX360)
+	if ((xpad->xtype == XTYPE_XBOX360) || (xpad->xtype == XTYPE_XBOX360W))
 		usb_kill_urb(xpad->irq_out);
 }
 
 static void xpad_deinit_output(struct usb_xpad *xpad)
 {
-	if (xpad->xtype == XTYPE_XBOX360) {
+	if ((xpad->xtype == XTYPE_XBOX360) || (xpad->xtype == XTYPE_XBOX360W)) {
 		usb_free_urb(xpad->irq_out);
 		usb_buffer_free(xpad->udev, XPAD_PKT_LEN,
 				xpad->odata, xpad->odata_dma);
@@ -547,6 +990,11 @@
 #endif
 
 #ifdef CONFIG_JOYSTICK_XPAD_FF
+
+/* Rumble support for wireless controllers follows protocol description
+ * from xboxdrv userspace driver:
+ *       http://pingus.seul.org/~grumbel/xboxdrv/
+ */
 static int xpad_play_effect(struct input_dev *dev, void *data,
 			    struct ff_effect *effect)
 {
@@ -555,16 +1003,35 @@
 	if (effect->type == FF_RUMBLE) {
 		__u16 strong = effect->u.rumble.strong_magnitude;
 		__u16 weak = effect->u.rumble.weak_magnitude;
-		xpad->odata[0] = 0x00;
-		xpad->odata[1] = 0x08;
-		xpad->odata[2] = 0x00;
-		xpad->odata[3] = strong / 256;
-		xpad->odata[4] = weak / 256;
-		xpad->odata[5] = 0x00;
-		xpad->odata[6] = 0x00;
-		xpad->odata[7] = 0x00;
-		xpad->irq_out->transfer_buffer_length = 8;
-		usb_submit_urb(xpad->irq_out, GFP_KERNEL);
+		mutex_lock(&xpad->odata_mutex);
+		if (xpad->xtype == XTYPE_XBOX360W) {
+			xpad->odata[0] = 0x00;
+			xpad->odata[1] = 0x01;
+			xpad->odata[2] = 0x0f;
+			xpad->odata[3] = 0xc0;
+			xpad->odata[4] = 0x00;
+			xpad->odata[5] = strong / 256;
+			xpad->odata[6] = weak / 256;
+			xpad->odata[7] = 0x00;
+			xpad->odata[8] = 0x00;
+			xpad->odata[9] = 0x00;
+			xpad->odata[10] = 0x00;
+			xpad->odata[11] = 0x00;
+			xpad->irq_out->transfer_buffer_length = 12;
+		}
+		else {
+			xpad->odata[0] = 0x00;
+			xpad->odata[1] = 0x08;
+			xpad->odata[2] = 0x00;
+			xpad->odata[3] = strong / 256;
+			xpad->odata[4] = weak / 256;
+			xpad->odata[5] = 0x00;
+			xpad->odata[6] = 0x00;
+			xpad->odata[7] = 0x00;
+			xpad->irq_out->transfer_buffer_length = 8;
+		}
+		usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
+		mutex_unlock(&xpad->odata_mutex);
 	}
 
 	return 0;
@@ -572,7 +1039,7 @@
 
 static int xpad_init_ff(struct usb_xpad *xpad)
 {
-	if (xpad->xtype != XTYPE_XBOX360)
+	if ((xpad->xtype != XTYPE_XBOX360) && (xpad->xtype != XTYPE_XBOX360W))
 		return 0;
 
 	input_set_capability(xpad->dev, EV_FF, FF_RUMBLE);
@@ -593,15 +1060,36 @@
 	struct usb_xpad *xpad;
 };
 
+/* XBox 360 wireless controller follows protocol from xboxdrv userspace
+ * driver:
+ *    http://pingus.seul.org/~grumbel/xboxdrv/
+ */
 static void xpad_send_led_command(struct usb_xpad *xpad, int command)
 {
 	if (command >= 0 && command < 14) {
 		mutex_lock(&xpad->odata_mutex);
-		xpad->odata[0] = 0x01;
-		xpad->odata[1] = 0x03;
-		xpad->odata[2] = command;
-		xpad->irq_out->transfer_buffer_length = 3;
-		usb_submit_urb(xpad->irq_out, GFP_KERNEL);
+		if (xpad->xtype == XTYPE_XBOX360W) {
+			xpad->odata[0] = 0x00;
+			xpad->odata[1] = 0x00;
+			xpad->odata[2] = 0x08;
+			xpad->odata[3] = 0x40 + (command % 0x0e);
+			xpad->odata[4] = 0x00;
+			xpad->odata[5] = 0x00;
+			xpad->odata[6] = 0x00;
+			xpad->odata[7] = 0x00;
+			xpad->odata[8] = 0x00;
+			xpad->odata[9] = 0x00;
+			xpad->odata[10] = 0x00;
+			xpad->odata[11] = 0x00;
+			xpad->irq_out->transfer_buffer_length = 12;
+		}
+		else {
+			xpad->odata[0] = 0x01;
+			xpad->odata[1] = 0x03;
+			xpad->odata[2] = command;
+			xpad->irq_out->transfer_buffer_length = 3;
+		}
+		usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
 		mutex_unlock(&xpad->odata_mutex);
 	}
 }
@@ -615,6 +1103,7 @@
 	xpad_send_led_command(xpad_led->xpad, value);
 }
 
+
 static int xpad_led_probe(struct usb_xpad *xpad)
 {
 	static atomic_t led_seq	= ATOMIC_INIT(0);
@@ -623,7 +1112,7 @@
 	struct led_classdev *led_cdev;
 	int error;
 
-	if (xpad->xtype != XTYPE_XBOX360)
+	if ((xpad->xtype != XTYPE_XBOX360) && (xpad->xtype != XTYPE_XBOX360W))
 		return 0;
 
 	xpad->led = led = kzalloc(sizeof(struct xpad_led), GFP_KERNEL);
@@ -721,6 +1210,7 @@
 	struct usb_xpad *xpad;
 	struct input_dev *input_dev;
 	struct usb_endpoint_descriptor *ep_irq_in;
+	int controller_type;
 	int i;
 	int error = -ENOMEM;
 
@@ -747,6 +1237,7 @@
 	xpad->udev = udev;
 	xpad->dpad_mapping = xpad_device[i].dpad_mapping;
 	xpad->xtype = xpad_device[i].xtype;
+	controller_type = xpad_device[i].controller_type;
 	if (xpad->dpad_mapping == MAP_DPAD_UNKNOWN)
 		xpad->dpad_mapping = !dpad_to_buttons;
 	if (xpad->xtype == XTYPE_UNKNOWN) {
@@ -819,6 +1310,22 @@
 		goto fail4;
 
 	usb_set_intfdata(intf, xpad);
+	
+	/* Set up data object */
+	if (!(&input_dev->dev.kobj)) {
+		error = 99;
+		goto fail5;
+	}
+	xpad->controller_data = xpad_create_data("xpad", &input_dev->dev.kobj);
+	if (!xpad->controller_data) {
+		error = 100;
+		goto fail5;
+	}
+	
+	/* Set defaults common to all controller types */
+	xpad->controller_data->controller_type = controller_type;
+	xpad->controller_data->dead_zone = XDEAD_ZONE_DEFAULT;
+	xpad->controller_data->square_axis = XSQUARE_AXIS_DEFAULT;
 
 	/*
 	 * Submit the int URB immediatly rather than waiting for open
@@ -828,48 +1335,26 @@
 	 * we're waiting for.
 	 */
 	if (xpad->xtype == XTYPE_XBOX360W) {
+		xpad->controller_data->pad_present = 0;
+		xpad->controller_data->controller_unique_id[0] = '\0';
+		xpad->controller_data->controller_number =
+			(intf->cur_altsetting->desc.bInterfaceNumber / 2) + 1;
 		xpad->irq_in->dev = xpad->udev;
 		error = usb_submit_urb(xpad->irq_in, GFP_KERNEL);
 		if (error)
-			goto fail4;
-
-		/*
-		 * Setup the message to set the LEDs on the
-		 * controller when it shows up
-		 */
-		xpad->bulk_out = usb_alloc_urb(0, GFP_KERNEL);
-		if(!xpad->bulk_out)
 			goto fail5;
-
-		xpad->bdata = kzalloc(XPAD_PKT_LEN, GFP_KERNEL);
-		if(!xpad->bdata)
-			goto fail6;
-
-		xpad->bdata[2] = 0x08;
-		switch (intf->cur_altsetting->desc.bInterfaceNumber) {
-		case 0:
-			xpad->bdata[3] = 0x42;
-			break;
-		case 2:
-			xpad->bdata[3] = 0x43;
-			break;
-		case 4:
-			xpad->bdata[3] = 0x44;
-			break;
-		case 6:
-			xpad->bdata[3] = 0x45;
-		}
-
-		ep_irq_in = &intf->cur_altsetting->endpoint[1].desc;
-		usb_fill_bulk_urb(xpad->bulk_out, udev,
-				usb_sndbulkpipe(udev, ep_irq_in->bEndpointAddress),
-				xpad->bdata, XPAD_PKT_LEN, xpad_bulk_out, xpad);
+	}
+	else {
+		xpad->controller_data->pad_present = 1;
+		strncpy(xpad->controller_data->controller_unique_id, "wired", 17);
+		xpad->controller_data->controller_number = 0;
 	}
 
 	return 0;
 
- fail6:	usb_free_urb(xpad->bulk_out);
- fail5:	usb_kill_urb(xpad->irq_in);
+ fail5: usb_set_intfdata(intf, NULL);
+ 	input_unregister_device(xpad->dev);
+ 	xpad_led_disconnect(xpad);
  fail4:	usb_free_urb(xpad->irq_in);
  fail3:	xpad_deinit_output(xpad);
  fail2:	usb_buffer_free(udev, XPAD_PKT_LEN, xpad->idata, xpad->idata_dma);
@@ -885,17 +1370,17 @@
 
 	usb_set_intfdata(intf, NULL);
 	if (xpad) {
+		xpad_destroy_data(xpad->controller_data);
 		xpad_led_disconnect(xpad);
 		input_unregister_device(xpad->dev);
 		xpad_deinit_output(xpad);
 		if (xpad->xtype == XTYPE_XBOX360W) {
-			usb_kill_urb(xpad->bulk_out);
-			usb_free_urb(xpad->bulk_out);
 			usb_kill_urb(xpad->irq_in);
 		}
 		usb_free_urb(xpad->irq_in);
 		usb_buffer_free(xpad->udev, XPAD_PKT_LEN,
 				xpad->idata, xpad->idata_dma);
+		input_free_device(xpad->dev);
 		kfree(xpad);
 	}
 }

Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Oliver Neukum]

Am Sunday 15 February 2009 05:08:46 schrieb Mike Murphy:
> Greetings,
> 
> The attached patch is a result of a few days of hacking to get better
> Linux support for Xbox 360 wireless controllers. Major functional
> improvements include support for the LEDs on the wireless controllers
> (which are auto-set to the controller number as they are on the
> official console), added support for rumble effects on wireless
> controllers, added support for dead zones on all supported
> controllers, and added support for a square axis mapping for the left
> stick on all supported controllers. A large amount of the bulk in this
> patch is from the addition of a sysfs interface to retrieve
> information and adjust the dead zone/square axis settings.
[..]
> Patch generated against 2.6.28.2 (no changes in relevant in-kernel
> driver from 2.6.28.2 to 2.6.28.5, so should apply against latest
> stable).
> 
> Comments are appreciated. I am _NOT_ subscribed to the LKML, so please
> CC me directly.

1. You need to check the returns of sscanf
2. This is very ugly:

+/* read-only attrs */
+static ssize_t xpad_show_int(struct xpad_data *xd, struct xpad_attribute *attr,
+	char *buf)
+{
+	int value;
+	if (!strcmp(attr->attr.name, "controller_number"))
+		value = xd->controller_number;
+	else if (!strcmp(attr->attr.name, "pad_present"))
+		value = xd->pad_present;
+	else if (!strcmp(attr->attr.name, "controller_type"))
+		value = xd->controller_type;
+	else
+		value = 0;
+	return sprintf(buf, "%d\n", value);
+}

3. Possible memory leak in error case:

+static struct xpad_data *xpad_create_data(const char *name, struct kobject *parent) {
+	struct xpad_data *data = NULL;
+	int check;
+	
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return NULL;
+	
+	check = kobject_init_and_add(&data->kobj, &xpad_ktype, parent, "%s", name);
+	if (check) {
+		kobject_put(&data->kobj);
+		return NULL;
+	}

4. Why the cpup variety?

+	coords[0] = (__s16) le16_to_cpup((__le16 *)(data + x_offset));

5. What happens if this work is already scheduled?

 	if (data[0] & 0x08) {
+		padnum = xpad->controller_data->controller_number;
 		if (data[1] & 0x80) {
-			xpad->pad_present = 1;
-			usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
-		} else
-			xpad->pad_present = 0;
+			printk(KERN_INFO "Wireless Xbox 360 pad #%d present\n", padnum);
+			xpad->controller_data->pad_present = 1;
+			
+			INIT_WORK(&xpad->work, &xpad_work_controller);
+			schedule_work(&xpad->work);

6. No GFP_ATOMIC. If you can take a mutex you can sleep.
+		usb_submit_urb(xpad->irq_out, GFP_ATOMIC);

	Regards
		Oliver

Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Mike Murphy]

On Mon, Feb 16, 2009 at 3:31 AM, Oliver Neukum <oliver@neukum.org> wrote:
...
>
> 1. You need to check the returns of sscanf

Will add... this is currently preliminary and not very well tested.

> 2. This is very ugly:
>
> +/* read-only attrs */
> +static ssize_t xpad_show_int(struct xpad_data *xd, struct xpad_attribute *attr,
> +       char *buf)
> +{
> +       int value;
> +       if (!strcmp(attr->attr.name, "controller_number"))
> +               value = xd->controller_number;
> +       else if (!strcmp(attr->attr.name, "pad_present"))
> +               value = xd->pad_present;
> +       else if (!strcmp(attr->attr.name, "controller_type"))
> +               value = xd->controller_type;
> +       else
> +               value = 0;
> +       return sprintf(buf, "%d\n", value);
> +}

The above code is basically following the example in
samples/kobject/kset-example.c. I broke the rest of the sysfs stuff
out such that it uses separate functions for show/store, which
definitely looks cleaner. However, given the large amount of code that
results, I'm starting to think that re-factoring and pulling the sysfs
code out to a separate file might be useful.

>
> 3. Possible memory leak in error case:
>
> +static struct xpad_data *xpad_create_data(const char *name, struct kobject *parent) {
> +       struct xpad_data *data = NULL;
> +       int check;
> +
> +       data = kzalloc(sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return NULL;
> +
> +       check = kobject_init_and_add(&data->kobj, &xpad_ktype, parent, "%s", name);
> +       if (check) {
> +               kobject_put(&data->kobj);
> +               return NULL;
> +       }
>

My understanding from Documentation/kobject.txt is that the
kobject_put in the 2nd error check will set the kobj's reference
counter to zero, eventually causing the kobject core to call my
cleanup function for the ktype (xpad_release) and free the memory. Is
this not correct? I find the sysfs docs to be fairly thin... and sysfs
seems to be substantially more complex than procfs or ioctls would be
for the same purpose. However, everything I read suggested that sysfs
is the "best" way to go in a modern kernel.

> 4. Why the cpup variety?
>
> +       coords[0] = (__s16) le16_to_cpup((__le16 *)(data + x_offset));
>

The cpup cast is in the original stable driver
(drivers/input/joystick/xpad.c), and I didn't question it.

> 5. What happens if this work is already scheduled?
>
>        if (data[0] & 0x08) {
> +               padnum = xpad->controller_data->controller_number;
>                if (data[1] & 0x80) {
> -                       xpad->pad_present = 1;
> -                       usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
> -               } else
> -                       xpad->pad_present = 0;
> +                       printk(KERN_INFO "Wireless Xbox 360 pad #%d present\n", padnum);
> +                       xpad->controller_data->pad_present = 1;
> +
> +                       INIT_WORK(&xpad->work, &xpad_work_controller);
> +                       schedule_work(&xpad->work);
>

I'm still a little fuzzy on this... in theory, I could see that
INIT_WORK would clobber the existing work structures while they wait
in the queue (thought about changing to PREPARE_WORK).

However, in practice, this work queue trick is only used when a
wireless 360 controller connects to the receiver. There is 1 instance
of struct usb_xpad per wireless controller (4 total, since the
receiver exposes 4 controller slots), and each instance has a separate
struct work_struct. So two things have to happen to reschedule the
work before it completes:

1. The user has to remove the battery pack from the controller,
reinstall the battery pack, and re-activate the controller by pushing
and holding the center button for at least 1 second.

2. The kernel has to be busy enough not to have completed the work in
the ~2 seconds a human could have done (1).

I need a bit of guidance from someone who has a better understanding
of the work queues to have a good solution to this one. Is switching
to PREPARE_WORK sufficient (with an INIT_WORK somewhere in
xpad_probe)? Or is a more involved solution needed?

> 6. No GFP_ATOMIC. If you can take a mutex you can sleep.
> +               usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
>

Per the "Linux Device Drivers" book (O'Reilly, 3rd ed), the claim is
made that submissions while holding a mutex should be GFP_ATOMIC. My
tests seemed to verify this claim... as sending LED commands
GFP_KERNEL while holding the mutex resulted in BUGs (scheduling while
atomic) in dmesg. Switching those GFP_KERNELs to GFP_ATOMICs
eliminated that particular BUG.

>        Regards
>                Oliver

Thanks for your reply... I will keep working on the driver as time
allows. This is really the first driver on which I've done any
substantial hacking, and my formal kernel-level programming training
was on an older version of the FreeBSD kernel, so I'm having to learn
things as I go. I'm trying to develop based off the latest stable
sources, so the outdated nature of most of the reference material I
have is not helping matters.

Thanks,
Mike
-- 
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838   Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph

Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Oliver Neukum]

Am Monday 16 February 2009 14:22:05 schrieb Mike Murphy:
> >
> > 3. Possible memory leak in error case:
> >
> > +static struct xpad_data *xpad_create_data(const char *name, struct kobject *parent) {
> > +       struct xpad_data *data = NULL;
> > +       int check;
> > +
> > +       data = kzalloc(sizeof(*data), GFP_KERNEL);
> > +       if (!data)
> > +               return NULL;
> > +
> > +       check = kobject_init_and_add(&data->kobj, &xpad_ktype, parent, "%s", name);
> > +       if (check) {
> > +               kobject_put(&data->kobj);
> > +               return NULL;
> > +       }
> >
> 
> My understanding from Documentation/kobject.txt is that the
> kobject_put in the 2nd error check will set the kobj's reference
> counter to zero, eventually causing the kobject core to call my
> cleanup function for the ktype (xpad_release) and free the memory. Is
> this not correct? I find the sysfs docs to be fairly thin... and sysfs

I don't know. I also find that documentation thin.
Please add that the memory is freed elsewhere.

> seems to be substantially more complex than procfs or ioctls would be
> for the same purpose. However, everything I read suggested that sysfs
> is the "best" way to go in a modern kernel.
> 
> > 4. Why the cpup variety?
> >
> > +       coords[0] = (__s16) le16_to_cpup((__le16 *)(data + x_offset));
> >
> 
> The cpup cast is in the original stable driver
> (drivers/input/joystick/xpad.c), and I didn't question it.
> 
> > 5. What happens if this work is already scheduled?
> >
> >        if (data[0] & 0x08) {
> > +               padnum = xpad->controller_data->controller_number;
> >                if (data[1] & 0x80) {
> > -                       xpad->pad_present = 1;
> > -                       usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
> > -               } else
> > -                       xpad->pad_present = 0;
> > +                       printk(KERN_INFO "Wireless Xbox 360 pad #%d present\n", padnum);
> > +                       xpad->controller_data->pad_present = 1;
> > +
> > +                       INIT_WORK(&xpad->work, &xpad_work_controller);
> > +                       schedule_work(&xpad->work);
> >

> 1. The user has to remove the battery pack from the controller,
> reinstall the battery pack, and re-activate the controller by pushing
> and holding the center button for at least 1 second.
> 
> 2. The kernel has to be busy enough not to have completed the work in
> the ~2 seconds a human could have done (1).

Also what happens if the work is scheduled  and you unplug?

> I need a bit of guidance from someone who has a better understanding
> of the work queues to have a good solution to this one. Is switching
> to PREPARE_WORK sufficient (with an INIT_WORK somewhere in
> xpad_probe)? Or is a more involved solution needed?
> 
> > 6. No GFP_ATOMIC. If you can take a mutex you can sleep.
> > +               usb_submit_urb(xpad->irq_out, GFP_ATOMIC);
> >
> 
> Per the "Linux Device Drivers" book (O'Reilly, 3rd ed), the claim is
> made that submissions while holding a mutex should be GFP_ATOMIC. My

That is not correct. It is true while holding a spinlock, not a mutex.

> tests seemed to verify this claim... as sending LED commands
> GFP_KERNEL while holding the mutex resulted in BUGs (scheduling while
> atomic) in dmesg. Switching those GFP_KERNELs to GFP_ATOMICs
> eliminated that particular BUG.

Please post that BUG.

	Regards
		Oliver

Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Greg KH]

On Mon, Feb 16, 2009 at 08:22:05AM -0500, Mike Murphy wrote:
> On Mon, Feb 16, 2009 at 3:31 AM, Oliver Neukum <oliver@neukum.org> wrote:
> ...
> >
> > 1. You need to check the returns of sscanf
> 
> Will add... this is currently preliminary and not very well tested.
> 
> > 2. This is very ugly:
> >
> > +/* read-only attrs */
> > +static ssize_t xpad_show_int(struct xpad_data *xd, struct xpad_attribute *attr,
> > +       char *buf)
> > +{
> > +       int value;
> > +       if (!strcmp(attr->attr.name, "controller_number"))
> > +               value = xd->controller_number;
> > +       else if (!strcmp(attr->attr.name, "pad_present"))
> > +               value = xd->pad_present;
> > +       else if (!strcmp(attr->attr.name, "controller_type"))
> > +               value = xd->controller_type;
> > +       else
> > +               value = 0;
> > +       return sprintf(buf, "%d\n", value);
> > +}
> 
> The above code is basically following the example in
> samples/kobject/kset-example.c. I broke the rest of the sysfs stuff
> out such that it uses separate functions for show/store, which
> definitely looks cleaner. However, given the large amount of code that
> results, I'm starting to think that re-factoring and pulling the sysfs
> code out to a separate file might be useful.
> 
> >
> > 3. Possible memory leak in error case:
> >
> > +static struct xpad_data *xpad_create_data(const char *name, struct kobject *parent) {
> > +       struct xpad_data *data = NULL;
> > +       int check;
> > +
> > +       data = kzalloc(sizeof(*data), GFP_KERNEL);
> > +       if (!data)
> > +               return NULL;
> > +
> > +       check = kobject_init_and_add(&data->kobj, &xpad_ktype, parent, "%s", name);
> > +       if (check) {
> > +               kobject_put(&data->kobj);
> > +               return NULL;
> > +       }
> >
> 
> My understanding from Documentation/kobject.txt is that the
> kobject_put in the 2nd error check will set the kobj's reference
> counter to zero, eventually causing the kobject core to call my
> cleanup function for the ktype (xpad_release) and free the memory.

That is correct.

The bigger question is why are you using a struct kobject in the first
place?  As you are a device, just use a struct device.  What are you
trying to do in sysfs here to make you want to use a "raw" kobject in a
driver?

thanks,

greg k-h

Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Mike Murphy]

On Mon, Feb 16, 2009 at 11:13 AM, Greg KH <greg@kroah.com> wrote:
...
>
> The bigger question is why are you using a struct kobject in the first
> place?  As you are a device, just use a struct device.  What are you
> trying to do in sysfs here to make you want to use a "raw" kobject in a
> driver?
>
> thanks,
>
> greg k-h
>

The purpose of the struct xpad_data (which contains the struct
kobject) is to expose parameters that can be read and/or tweaked at
runtime. At the moment, the adjustable parameters are the size of the
dead zone around the center of the stick, and the choice of whether to
map to a square axis. Read-only data include a unique identifier for
each pad (wireless) and an indication as to whether the wireless pad
is actually connected.

The idea behind this interface is to enable a future userspace
application, likely using HAL and D-BUS, to receive events whenever a
wireless pad connects, and to identify the particular pad in question
(specifically, the unique ID of the pad). The user could then, for
example, have a different dead zone size for each pad, based upon the
actual manufacturing variations in the sticks on that pad (some appear
more precise at centering than others). Ideally, this functionality
could be designed in a "common" way so that the userspace code could
eventually work with other, non-xpad gaming devices.

So I really just wanted a way to get some parameters to userspace. The
consensus opinion of everything I read was that /proc additions were
frowned upon for cluttering up /proc with kernel stuff, ioctls were
viewed negatively because of their inconsistent nature from device to
device, and that sysfs was the best solution for this problem.

I didn't use a struct device because of the existing struct usb_xpad
that was created on a per-gamepad basis (not necessarily per device,
as the wireless 360 adapter is one physical device but exposes 4
gamepads by exposing 4 logical devices [and then each gamepad, in
turn, is technically a USB hub that can have other stuff
attached...]). I tried not to break existing functionality.
Additionally, struct usb_xpad contains two device pointers: one to the
actual USB device, and one to an input device (see source of the
in-tree xpad.c). So I followed your kobject.txt documentation and
samples to create a new object whose sole purpose in life is to expose
the sysfs interface, without interfering with the existing device
entries in the driver. I'm not sure I see a clean way to use a single
struct device here....

Thanks,
Mike
-- 
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838   Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph

Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Greg KH]

On Mon, Feb 16, 2009 at 01:09:00PM -0500, Mike Murphy wrote:
> On Mon, Feb 16, 2009 at 11:13 AM, Greg KH <greg@kroah.com> wrote:
> ...
> >
> > The bigger question is why are you using a struct kobject in the first
> > place?  As you are a device, just use a struct device.  What are you
> > trying to do in sysfs here to make you want to use a "raw" kobject in a
> > driver?
> >
> > thanks,
> >
> > greg k-h
> >
> 
> The purpose of the struct xpad_data (which contains the struct
> kobject) is to expose parameters that can be read and/or tweaked at
> runtime. At the moment, the adjustable parameters are the size of the
> dead zone around the center of the stick, and the choice of whether to
> map to a square axis. Read-only data include a unique identifier for
> each pad (wireless) and an indication as to whether the wireless pad
> is actually connected.

Then hang those parameters off the struct device you already have.

> The idea behind this interface is to enable a future userspace
> application, likely using HAL and D-BUS, to receive events whenever a
> wireless pad connects, and to identify the particular pad in question
> (specifically, the unique ID of the pad). The user could then, for
> example, have a different dead zone size for each pad, based upon the
> actual manufacturing variations in the sticks on that pad (some appear
> more precise at centering than others). Ideally, this functionality
> could be designed in a "common" way so that the userspace code could
> eventually work with other, non-xpad gaming devices.

Have you documented them in Documentation/ABI?

> I didn't use a struct device because of the existing struct usb_xpad
> that was created on a per-gamepad basis (not necessarily per device,
> as the wireless 360 adapter is one physical device but exposes 4
> gamepads by exposing 4 logical devices [and then each gamepad, in
> turn, is technically a USB hub that can have other stuff
> attached...]).

Put it on the logical device, as given to you.

> I tried not to break existing functionality.  Additionally, struct
> usb_xpad contains two device pointers: one to the actual USB device,
> and one to an input device (see source of the in-tree xpad.c). So I
> followed your kobject.txt documentation and samples to create a new
> object whose sole purpose in life is to expose the sysfs interface,
> without interfering with the existing device entries in the driver.
> I'm not sure I see a clean way to use a single struct device here....

Put it on the input device, which is what is the per-device thing.  It's
much simpler than creating a new struct kobject.  You can even create a
subdirectory for your attributes if you use an attribute group (which
you should be doing anyway, it's much simpler that way.)

And document the attributes please.

thanks,

greg k-h

Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Mike Murphy]

On Mon, Feb 16, 2009 at 1:59 PM, Greg KH <greg@kroah.com> wrote:
>
> Put it on the logical device, as given to you.
>
>> I tried not to break existing functionality.  Additionally, struct
>> usb_xpad contains two device pointers: one to the actual USB device,
>> and one to an input device (see source of the in-tree xpad.c). So I
>> followed your kobject.txt documentation and samples to create a new
>> object whose sole purpose in life is to expose the sysfs interface,
>> without interfering with the existing device entries in the driver.
>> I'm not sure I see a clean way to use a single struct device here....
>
> Put it on the input device, which is what is the per-device thing.  It's
> much simpler than creating a new struct kobject.  You can even create a
> subdirectory for your attributes if you use an attribute group (which
> you should be doing anyway, it's much simpler that way.)
>

OK, one thing I'm not clear on: is there a clean API for adding
attributes to an existing struct device, or do I need to "subclass" it
(the C containment and delegation approach)?

This may take me a few days or a week or so, depending on how things
go. It's dissertation proposal season....

> And document the attributes please.
>

Will do... still need to nail down what the interface should look like.

> thanks,
>
> greg k-h
>

Thanks,
Mike
-- 
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838   Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph

Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Greg KH]

On Mon, Feb 16, 2009 at 02:30:01PM -0500, Mike Murphy wrote:
> On Mon, Feb 16, 2009 at 1:59 PM, Greg KH <greg@kroah.com> wrote:
> >
> > Put it on the logical device, as given to you.
> >
> >> I tried not to break existing functionality.  Additionally, struct
> >> usb_xpad contains two device pointers: one to the actual USB device,
> >> and one to an input device (see source of the in-tree xpad.c). So I
> >> followed your kobject.txt documentation and samples to create a new
> >> object whose sole purpose in life is to expose the sysfs interface,
> >> without interfering with the existing device entries in the driver.
> >> I'm not sure I see a clean way to use a single struct device here....
> >
> > Put it on the input device, which is what is the per-device thing.  It's
> > much simpler than creating a new struct kobject.  You can even create a
> > subdirectory for your attributes if you use an attribute group (which
> > you should be doing anyway, it's much simpler that way.)
> >
> 
> OK, one thing I'm not clear on: is there a clean API for adding
> attributes to an existing struct device, or do I need to "subclass" it
> (the C containment and delegation approach)?

device_create_file()

Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Mike Murphy]

On Mon, Feb 16, 2009 at 3:22 PM, Greg KH <greg@kroah.com> wrote:
>> >
>> > Put it on the input device, which is what is the per-device thing.  It's
>> > much simpler than creating a new struct kobject.  You can even create a
>> > subdirectory for your attributes if you use an attribute group (which
>> > you should be doing anyway, it's much simpler that way.)
>> >
>>
>> OK, one thing I'm not clear on: is there a clean API for adding
>> attributes to an existing struct device, or do I need to "subclass" it
>> (the C containment and delegation approach)?
>
> device_create_file()
>

In the process of trying to re-factor the code for the above changes,
I ran into the problem that the show and store functions for sysfs
expect to be passed a struct device *. I can get to struct input_dev *
without problems (since struct input_dev contains the struct device,
and I can simply use container_of indirectly via to_input_dev).
However, I can't seem to get back to struct usb_xpad, because that
structure defines 2 pointers to devices, rather than simply embedding
the devices:

struct usb_xpad {
	struct input_dev *dev;		/* input device interface */
	struct usb_device *udev;	/* usb device */
...
};

This is not my code... it was set up this way in the stable xpad driver.

So it looks like I'm stuck with a struct input_dev * pointer to the
input device, a struct device * pointer in the show/store handlers,
and no way to get back to struct usb_xpad * with the container_of
macro. Unless, of course, there is something I don't know about
container_of (or another macro I can use in this instance).

Worse, I can't simply change the struct input_dev *dev to struct
input_dev dev, because the input subsystem's input_register_device
expects to have a struct input_dev * pointer allocated with
input_allocate_device, which does both a kzalloc AND initialization.
Trying to hack my driver by incorporating the initialization logic in
input_allocate_device would be stupid, since the result would be
unmaintainable. So the route out of this mess, going in the direction
of device attributes, would be to modify drivers/input/input.c to
factor the initialization away from the allocation.

So which is the lesser of two evils? Allocating a new kobject, or
mucking around with more of the input subsystem? Or am I missing
something?

Thanks,
Mike
-- 
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838   Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph

Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Greg KH]

On Mon, Feb 16, 2009 at 09:53:54PM -0500, Mike Murphy wrote:
> struct usb_xpad {
> 	struct input_dev *dev;		/* input device interface */
> 	struct usb_device *udev;	/* usb device */
> ...
> };
> 
> This is not my code... it was set up this way in the stable xpad driver.

And it is correct :)

> So it looks like I'm stuck with a struct input_dev * pointer to the
> input device, a struct device * pointer in the show/store handlers,
> and no way to get back to struct usb_xpad * with the container_of
> macro. Unless, of course, there is something I don't know about
> container_of (or another macro I can use in this instance).

input_set_drvdata() and input_get_drvdata() is what you are looking for.

Hope this helps,

greg k-h

Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Mike Murphy]

On Mon, Feb 16, 2009 at 10:18 PM, Greg KH <greg@kroah.com> wrote:
>
> input_set_drvdata() and input_get_drvdata() is what you are looking for.
>

Thanks! That solved the problem at least as far as getting the entries
to show up in sysfs... though somehow now I'm having problems with the
I/O buffers, and a cat of the sysfs file shows no output (not even a
blank line). I added a couple printk's to one of the routines, and the
values are being read correctly from the data structure, but no
working I/O (cannot set the values either, per printk results). The
dmesg output shows the printk results, so the functions are getting
called. Even the count from sprintf is correct, and the resulting
buffer looks like it should... though with the extra blank line I
added in the last printk:

static ssize_t xpad_show_dead_zone(struct device *dev, char *buf)
{
	struct usb_xpad *xpad = to_xpad(dev);
	int count;
	printk(KERN_INFO "Dead zone is %d\n", xpad->dead_zone);
	count = sprintf(buf, "%d\n", xpad->dead_zone);
	printk(KERN_INFO "Count is %d\n", count);
	printk(KERN_INFO "Buffer is %s\n", buf);
	return count;
}

Unless that is some common error that is obvious from its description,
I will have to chase the bug down tomorrow or Wednesday. It's getting
a bit late here.

Thanks,
Mike
-- 
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838   Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph

Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Mike Murphy]

On Mon, Feb 16, 2009 at 11:57 PM, Mike Murphy <mamurph@cs.clemson.edu> wrote:
> Unless that is some common error that is obvious from its description,
> I will have to chase the bug down tomorrow or Wednesday. It's getting
> a bit late here.
>

Fixed... the function signature was wrong. Should have been:

static ssize_t xpad_show_dead_zone(struct device *dev, struct
device_attribute *attr, char *buf)

Incidentally, both Documentation/driver-model/device.txt and
Documentation/filesystems/sysfs.txt appear to be wrong. The former
defines the device attribute structure as:

struct device_attribute {
        struct attribute        attr;
        ssize_t (*show)(struct device * dev, char * buf, size_t count,
loff_t off);
        ssize_t (*store)(struct device * dev, const char * buf, size_t
count, loff_t off);
};

while the latter (that I followed last night) defines it as:

struct device_attribute {
        struct attribute        attr;
        ssize_t (*show)(struct device * dev, char * buf);
        ssize_t (*store)(struct device * dev, const char * buf);
};

I finally got the correct one out of include/linux/device.h:

struct device_attribute {
	struct attribute	attr;
	ssize_t (*show)(struct device *dev, struct device_attribute *attr,
			char *buf);
	ssize_t (*store)(struct device *dev, struct device_attribute *attr,
			 const char *buf, size_t count);
};

Thanks,
Mike
-- 
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838   Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph

Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Greg KH]

On Tue, Feb 17, 2009 at 01:27:48PM -0500, Mike Murphy wrote:
> On Mon, Feb 16, 2009 at 11:57 PM, Mike Murphy <mamurph@cs.clemson.edu> wrote:
> > Unless that is some common error that is obvious from its description,
> > I will have to chase the bug down tomorrow or Wednesday. It's getting
> > a bit late here.
> >
> 
> Fixed... the function signature was wrong. Should have been:

Ah, always pay attention to complier warnings :)

> static ssize_t xpad_show_dead_zone(struct device *dev, struct
> device_attribute *attr, char *buf)
> 
> Incidentally, both Documentation/driver-model/device.txt and
> Documentation/filesystems/sysfs.txt appear to be wrong. The former
> defines the device attribute structure as:

<snip>

Care to send a patch to fix up the documentation?

thanks,

greg k-h

Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Mike Murphy]

On Mon, Feb 16, 2009 at 10:21 AM, Oliver Neukum <oliver@neukum.org> wrote:
>> tests seemed to verify this claim... as sending LED commands
>> GFP_KERNEL while holding the mutex resulted in BUGs (scheduling while
>> atomic) in dmesg. Switching those GFP_KERNELs to GFP_ATOMICs
>> eliminated that particular BUG.
>
> Please post that BUG.
>

I switched back to GFP_KERNEL on the functions in question.

After further testing, I have determined that the BUG only occurs on
my Ubuntu (2.6.27) system, and only when using the rumble function. I
am unable to reproduce the BUG on my Arch (2.6.28) system, so it is
either an artifact of some patch to this kernel, or it got fixed
somewhere between 2.6.27 and 2.6.28.

Based on this analysis, and (more importantly) an inability to
reproduce the behavior on a more vanilla kernel, I have removed the
comments about it from the TODO section of the driver. It isn't a true
kernel bug at least with a more recent stable kernel.

Mike
-- 
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838   Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph