[PATCH 0/7] Input: xpad: Fix wireless controller connection and leds

[PATCH 0/7] Input: xpad: Fix wireless controller connection and leds

[Greg Kroah-Hartman]

Here's a series of patches from Pierre-Loup and I that rework the xpad
driver to fix the issue where when a wireless xpad controller is plugged
in, 4 joystick devices are created, no matter how many are really
attached to the system.  Pierre-Loup's patches fix this by dynamically
creating the devices only when they are found by the wireless
controller.

Bonus is that the LEDs now work correctly with this series too, when
mixing wireless and wired, we can properly identify which device is
which on the X-LED.  We also now name the LED device based on the
joystick id, not on an atomic number that keeps incrementing, never
decrementing when removed from the system.

Note, patch 4/7 is a crazy hack to get the minor number of the joystick
id that we have registered with the system.  Pierre-Loup came up with
this, and I really can't figure out any other way to do it, the joydev
layer doesn't even know this information, otherwise I would have added
it to that layer so the xpad driver could call it.  Am I missing
something here that we could do instead?

The first patch in the series fixes a really annoying bug when plugging
in one of these controllers to a USB 3 system.  The URB type is
incorrect so the xhci driver complains about it, rightly so.  I think
the original code was incorrect, but left it alone incase there is
really a crazy device with a bulk endpoint instead of interrupt.

Also, even with this series applied, the xpad driver needs a bunch of
work.  The LED device seems pointless as it doesn't actually work, and
given that the number/name of the LED device was impossible to actually
map to the proper xpad device, there's no way userspace code could ever
actually use it.  I think it should be removed entirely.  There's also
some other USB things that should be cleaned up, the bulk urb doesn't
need to be always running (with lots of disconnect/connect cycles you
can get a warning about it running when trying to submit it again), and
the urb callbacks seem a bit strange.  I'll work on those issues next...

This series was based on a larger patch from Pierre-Loup that I broke up
into pieces, and added some of my own where needed to resolve other
issues.

[PATCH 1/7] Input: xpad: use proper endpoint type

[Greg Kroah-Hartman]

The xpad wireless endpoint is not a bulk endpoint on my devices, but
rather an interrupt one, so the USB core complains when it is submitted.
I'm guessing that the author really did mean that this should be an
interrupt urb, but as there are a zillion different xpad devices out
there, let's cover out bases and handle both bulk and interrupt
endpoints just as easily.

Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/input/joystick/xpad.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 603fe0dd3682..517829f6a58b 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -1003,9 +1003,19 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 		}
 
 		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);
+		if (usb_endpoint_is_bulk_out(ep_irq_in)) {
+			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 {
+			usb_fill_int_urb(xpad->bulk_out, udev,
+					 usb_sndintpipe(udev,
+							ep_irq_in->bEndpointAddress),
+					 xpad->bdata, XPAD_PKT_LEN,
+					 xpad_bulk_out, xpad, 0);
+		}
 
 		/*
 		 * Submit the int URB immediately rather than waiting for open
-- 
1.8.5.3

[PATCH 2/7] Input: xpad: set the LEDs properly on XBox Wireless controllers

[Greg Kroah-Hartman]

From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>

Add the logic to set the LEDs on XBox Wireless controllers.  Command
sequence found by sniffing the Windows data stream when plugging the
device in.

Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/input/joystick/xpad.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 517829f6a58b..aabff9140aaa 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -715,15 +715,37 @@ struct xpad_led {
 
 static void xpad_send_led_command(struct usb_xpad *xpad, int command)
 {
-	if (command >= 0 && command < 14) {
-		mutex_lock(&xpad->odata_mutex);
+	if (command > 15)
+		return;
+
+	mutex_lock(&xpad->odata_mutex);
+
+	switch (xpad->xtype) {
+	case XTYPE_XBOX360:
 		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);
-		mutex_unlock(&xpad->odata_mutex);
+		break;
+	case 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;
+		break;
 	}
+
+	usb_submit_urb(xpad->irq_out, GFP_KERNEL);
+	mutex_unlock(&xpad->odata_mutex);
 }
 
 static void xpad_led_set(struct led_classdev *led_cdev,
@@ -743,7 +765,7 @@ static int xpad_led_probe(struct usb_xpad *xpad)
 	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);
-- 
1.8.5.3

[PATCH 3/7] Input: xpad: move the input device creation to a new function

[Greg Kroah-Hartman]

From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>

To allow us to later create / destroy the input device from the urb
callback, we need to initialize the input device from a separate
function.  So pull that logic out now to make later patches more
"obvious" as to what they do.

Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/input/joystick/xpad.c | 171 ++++++++++++++++++++++++------------------
 1 file changed, 97 insertions(+), 74 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index aabff9140aaa..5b5a84dae54a 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -293,6 +293,7 @@ struct usb_xpad {
 
 	int mapping;			/* map d-pad to buttons or to axes */
 	int xtype;			/* type of xbox device */
+	const char *name;		/* name of the device */
 };
 
 /*
@@ -858,70 +859,21 @@ static void xpad_set_up_abs(struct input_dev *input_dev, signed short abs)
 	}
 }
 
-static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id)
+static int xpad_init_input(struct usb_xpad *xpad)
 {
-	struct usb_device *udev = interface_to_usbdev(intf);
-	struct usb_xpad *xpad;
 	struct input_dev *input_dev;
-	struct usb_endpoint_descriptor *ep_irq_in;
 	int i, error;
 
-	for (i = 0; xpad_device[i].idVendor; i++) {
-		if ((le16_to_cpu(udev->descriptor.idVendor) == xpad_device[i].idVendor) &&
-		    (le16_to_cpu(udev->descriptor.idProduct) == xpad_device[i].idProduct))
-			break;
-	}
-
-	xpad = kzalloc(sizeof(struct usb_xpad), GFP_KERNEL);
 	input_dev = input_allocate_device();
-	if (!xpad || !input_dev) {
-		error = -ENOMEM;
-		goto fail1;
-	}
-
-	xpad->idata = usb_alloc_coherent(udev, XPAD_PKT_LEN,
-					 GFP_KERNEL, &xpad->idata_dma);
-	if (!xpad->idata) {
-		error = -ENOMEM;
-		goto fail1;
-	}
-
-	xpad->irq_in = usb_alloc_urb(0, GFP_KERNEL);
-	if (!xpad->irq_in) {
-		error = -ENOMEM;
-		goto fail2;
-	}
-
-	xpad->udev = udev;
-	xpad->intf = intf;
-	xpad->mapping = xpad_device[i].mapping;
-	xpad->xtype = xpad_device[i].xtype;
-
-	if (xpad->xtype == XTYPE_UNKNOWN) {
-		if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) {
-			if (intf->cur_altsetting->desc.bInterfaceProtocol == 129)
-				xpad->xtype = XTYPE_XBOX360W;
-			else
-				xpad->xtype = XTYPE_XBOX360;
-		} else
-			xpad->xtype = XTYPE_XBOX;
-
-		if (dpad_to_buttons)
-			xpad->mapping |= MAP_DPAD_TO_BUTTONS;
-		if (triggers_to_buttons)
-			xpad->mapping |= MAP_TRIGGERS_TO_BUTTONS;
-		if (sticks_to_null)
-			xpad->mapping |= MAP_STICKS_TO_NULL;
-	}
+	if (!input_dev)
+		return -ENOMEM;
 
 	xpad->dev = input_dev;
-	usb_make_path(udev, xpad->phys, sizeof(xpad->phys));
-	strlcat(xpad->phys, "/input0", sizeof(xpad->phys));
 
-	input_dev->name = xpad_device[i].name;
+	input_dev->name = xpad->name;
 	input_dev->phys = xpad->phys;
-	usb_to_input_id(udev, &input_dev->id);
-	input_dev->dev.parent = &intf->dev;
+	usb_to_input_id(xpad->udev, &input_dev->id);
+	input_dev->dev.parent = &xpad->intf->dev;
 
 	input_set_drvdata(input_dev, xpad);
 
@@ -966,17 +918,92 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 			xpad_set_up_abs(input_dev, xpad_abs_triggers[i]);
 	}
 
-	error = xpad_init_output(intf, xpad);
-	if (error)
-		goto fail3;
-
 	error = xpad_init_ff(xpad);
 	if (error)
-		goto fail4;
+		goto fail_init_ff;
 
 	error = xpad_led_probe(xpad);
 	if (error)
-		goto fail5;
+		goto fail_init_led;
+
+	error = input_register_device(xpad->dev);
+	if (error)
+		goto fail_input_register;
+
+	return 0;
+
+fail_input_register:
+	xpad_led_disconnect(xpad);
+
+fail_init_led:
+	input_ff_destroy(input_dev);
+
+fail_init_ff:
+	input_free_device(input_dev);
+	return error;
+}
+
+static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id)
+{
+	struct usb_device *udev = interface_to_usbdev(intf);
+	struct usb_xpad *xpad;
+	struct usb_endpoint_descriptor *ep_irq_in;
+	int i, error;
+
+	for (i = 0; xpad_device[i].idVendor; i++) {
+		if ((le16_to_cpu(udev->descriptor.idVendor) == xpad_device[i].idVendor) &&
+		    (le16_to_cpu(udev->descriptor.idProduct) == xpad_device[i].idProduct))
+			break;
+	}
+
+	xpad = kzalloc(sizeof(struct usb_xpad), GFP_KERNEL);
+	if (!xpad) {
+		error = -ENOMEM;
+		goto fail1;
+	}
+
+	usb_make_path(udev, xpad->phys, sizeof(xpad->phys));
+	strlcat(xpad->phys, "/input0", sizeof(xpad->phys));
+
+	xpad->idata = usb_alloc_coherent(udev, XPAD_PKT_LEN,
+					 GFP_KERNEL, &xpad->idata_dma);
+	if (!xpad->idata) {
+		error = -ENOMEM;
+		goto fail1;
+	}
+
+	xpad->irq_in = usb_alloc_urb(0, GFP_KERNEL);
+	if (!xpad->irq_in) {
+		error = -ENOMEM;
+		goto fail2;
+	}
+
+	xpad->udev = udev;
+	xpad->intf = intf;
+	xpad->mapping = xpad_device[i].mapping;
+	xpad->xtype = xpad_device[i].xtype;
+	xpad->name = xpad_device[i].name;
+
+	if (xpad->xtype == XTYPE_UNKNOWN) {
+		if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) {
+			if (intf->cur_altsetting->desc.bInterfaceProtocol == 129)
+				xpad->xtype = XTYPE_XBOX360W;
+			else
+				xpad->xtype = XTYPE_XBOX360;
+		} else
+			xpad->xtype = XTYPE_XBOX;
+
+		if (dpad_to_buttons)
+			xpad->mapping |= MAP_DPAD_TO_BUTTONS;
+		if (triggers_to_buttons)
+			xpad->mapping |= MAP_TRIGGERS_TO_BUTTONS;
+		if (sticks_to_null)
+			xpad->mapping |= MAP_STICKS_TO_NULL;
+	}
+
+	error = xpad_init_output(intf, xpad);
+	if (error)
+		goto fail3;
 
 	ep_irq_in = &intf->cur_altsetting->endpoint[0].desc;
 	usb_fill_int_urb(xpad->irq_in, udev,
@@ -986,10 +1013,6 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 	xpad->irq_in->transfer_dma = xpad->idata_dma;
 	xpad->irq_in->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
 
-	error = input_register_device(xpad->dev);
-	if (error)
-		goto fail6;
-
 	usb_set_intfdata(intf, xpad);
 
 	if (xpad->xtype == XTYPE_XBOX360W) {
@@ -1000,7 +1023,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 		xpad->bulk_out = usb_alloc_urb(0, GFP_KERNEL);
 		if (!xpad->bulk_out) {
 			error = -ENOMEM;
-			goto fail7;
+			goto fail4;
 		}
 
 		xpad->bdata = kzalloc(XPAD_PKT_LEN, GFP_KERNEL);
@@ -1048,24 +1071,24 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 		 */
 		xpad->irq_in->dev = xpad->udev;
 		error = usb_submit_urb(xpad->irq_in, GFP_KERNEL);
-		if (error)
+		if (error) {
+			usb_kill_urb(xpad->irq_in);
 			goto fail9;
+		}
 	}
+	xpad->pad_present = 1;
+	error = xpad_init_input(xpad);
+	if (error)
+		goto fail9;
 
 	return 0;
 
  fail9:	kfree(xpad->bdata);
  fail8:	usb_free_urb(xpad->bulk_out);
- fail7:	input_unregister_device(input_dev);
-	input_dev = NULL;
- fail6:	xpad_led_disconnect(xpad);
- fail5:	if (input_dev)
-		input_ff_destroy(input_dev);
  fail4:	xpad_deinit_output(xpad);
  fail3:	usb_free_urb(xpad->irq_in);
  fail2:	usb_free_coherent(udev, XPAD_PKT_LEN, xpad->idata, xpad->idata_dma);
- fail1:	input_free_device(input_dev);
-	kfree(xpad);
+ fail1:	kfree(xpad);
 	return error;
 
 }
-- 
1.8.5.3

[PATCH 4/7] Input: xpad: Set the correct LED number

[Greg Kroah-Hartman]

From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>

The LED number should not just be incremented every time, that doesn't
work, set the number based on the number it actually is.

Note, the joydev subsystem doesn't allow us to easily find out the minor
number, so we have to walk all devices in order to find a joystick
device by looking at the name of the device.  Odds are, this isn't the
best way to do it, but I don't know of any other way at the moment.

Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/input/joystick/xpad.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 5b5a84dae54a..7997ae89a877 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -293,6 +293,7 @@ struct usb_xpad {
 
 	int mapping;			/* map d-pad to buttons or to axes */
 	int xtype;			/* type of xbox device */
+	int joydev_id;			/* the minor of the device */
 	const char *name;		/* name of the device */
 };
 
@@ -789,11 +790,6 @@ static int xpad_led_probe(struct usb_xpad *xpad)
 		return error;
 	}
 
-	/*
-	 * Light up the segment corresponding to controller number
-	 */
-	xpad_send_led_command(xpad, (led_no % 4) + 2);
-
 	return 0;
 }
 
@@ -809,6 +805,7 @@ static void xpad_led_disconnect(struct usb_xpad *xpad)
 #else
 static int xpad_led_probe(struct usb_xpad *xpad) { return 0; }
 static void xpad_led_disconnect(struct usb_xpad *xpad) { }
+static void xpad_send_led_command(struct usb_xpad *xpad, int command) { }
 #endif
 
 
@@ -859,9 +856,17 @@ static void xpad_set_up_abs(struct input_dev *input_dev, signed short abs)
 	}
 }
 
+static int xpad_find_joydev(struct device *dev, void *data)
+{
+	if (strstr(dev_name(dev), "js"))
+		return 1;
+	return 0;
+}
+
 static int xpad_init_input(struct usb_xpad *xpad)
 {
 	struct input_dev *input_dev;
+	struct device *joydev_dev;
 	int i, error;
 
 	input_dev = input_allocate_device();
@@ -930,6 +935,17 @@ static int xpad_init_input(struct usb_xpad *xpad)
 	if (error)
 		goto fail_input_register;
 
+	joydev_dev = device_find_child(&xpad->dev->dev, NULL,
+				       xpad_find_joydev);
+	if (joydev_dev) {
+		dev_dbg(&xpad->dev->dev, "Found xpad with minor %i\n",
+			MINOR(joydev_dev->devt));
+		xpad->joydev_id = MINOR(joydev_dev->devt);
+
+		/* Light up the segment corresponding to controller number */
+		xpad_send_led_command(xpad, (xpad->joydev_id % 4) + 2);
+	}
+
 	return 0;
 
 fail_input_register:
-- 
1.8.5.3

[PATCH 5/7] Input: xpad: disconnect all Wireless controllers at init

[Greg Kroah-Hartman]

From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>

We initializing the driver/device, we really don't know how many
controllers are connected.  So send a "disconnect all" command to the
base-station, and let the user pair the controllers in the order in
which they want them assigned.

Note, this means we now do not "preallocate" all 4 devices when a single
wireless base station is seen, but require the device to be properly
connected to the base station before that can happen.  The allocation of
the device happens in the next patch, not here, so in a way, this patch
breaks all wireless devices...

Because we want to talk to the device, we have to set up the output urbs
no matter if we have CONFIG_JOYSTICK_XPAD_FF or
CONFIG_JOYSTICK_XPAD_LEDS enabled not, so take out the code that allows
those variables and code to be disabled (it's so small it should never
matter...)

Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/input/joystick/xpad.c | 46 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 7997ae89a877..7a07b95790d7 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -278,12 +278,10 @@ struct usb_xpad {
 	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 */
 	dma_addr_t odata_dma;
 	struct mutex odata_mutex;
-#endif
 
 #if defined(CONFIG_JOYSTICK_XPAD_LEDS)
 	struct xpad_led *led;
@@ -537,7 +535,6 @@ static void xpad_bulk_out(struct urb *urb)
 	}
 }
 
-#if defined(CONFIG_JOYSTICK_XPAD_FF) || defined(CONFIG_JOYSTICK_XPAD_LEDS)
 static void xpad_irq_out(struct urb *urb)
 {
 	struct usb_xpad *xpad = urb->context;
@@ -623,11 +620,6 @@ static void xpad_deinit_output(struct usb_xpad *xpad)
 				xpad->odata, xpad->odata_dma);
 	}
 }
-#else
-static int xpad_init_output(struct usb_interface *intf, struct usb_xpad *xpad) { return 0; }
-static void xpad_deinit_output(struct usb_xpad *xpad) {}
-static void xpad_stop_output(struct usb_xpad *xpad) {}
-#endif
 
 #ifdef CONFIG_JOYSTICK_XPAD_FF
 static int xpad_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect)
@@ -1091,11 +1083,41 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 			usb_kill_urb(xpad->irq_in);
 			goto fail9;
 		}
+
+		/*
+		 * We don't know how to check the controller state at driver
+		 * load, so just disconnect them all, requiring the user to
+		 * repair the device in the order they want them used.  Good
+		 * point is that we don't connect devices in "random" order,
+		 * but the extra step seems a bit harsh as other operating
+		 * systems don't require this at the moment.
+		 *
+		 * Power-off packet information came from an OS-X
+		 * reverse-engineered driver located at:
+		 * http://tattiebogle.net/index.php/ProjectRoot/Xbox360Controller/OsxDriver#toc1
+		 */
+		mutex_lock(&xpad->odata_mutex);
+		xpad->odata[0] = 0x00;
+		xpad->odata[1] = 0x00;
+		xpad->odata[2] = 0x08;
+		xpad->odata[3] = 0xC0;
+		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;
+		usb_submit_urb(xpad->irq_out, GFP_KERNEL);
+		mutex_unlock(&xpad->odata_mutex);
+	} else {
+		xpad->pad_present = 1;
+		error = xpad_init_input(xpad);
+		if (error)
+			goto fail9;
 	}
-	xpad->pad_present = 1;
-	error = xpad_init_input(xpad);
-	if (error)
-		goto fail9;
 
 	return 0;
 
-- 
1.8.5.3

[PATCH 6/7] Input: xpad: handle "present" and "gone" correctly

[Greg Kroah-Hartman]

From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>

Handle the "a new device is present" message properly by dynamically
creating the input device at this point in time.  This requires a
workqueue as we are in interrupt context when we learn about this.

Also properly disconnect any devices that we are told are removed.

Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/input/joystick/xpad.c | 50 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 40 insertions(+), 10 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index 7a07b95790d7..d342d41a7a0d 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -293,8 +293,11 @@ struct usb_xpad {
 	int xtype;			/* type of xbox device */
 	int joydev_id;			/* the minor of the device */
 	const char *name;		/* name of the device */
+	struct work_struct work;	/* to init/remove device from callback */
 };
 
+static int xpad_init_input(struct usb_xpad *xpad);
+
 /*
  *	xpad_process_packet
  *
@@ -437,6 +440,22 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
 	input_sync(dev);
 }
 
+static void presence_work_function(struct work_struct *work)
+{
+	struct usb_xpad *xpad = container_of(work, struct usb_xpad, work);
+	int error;
+
+	if (xpad->pad_present) {
+		error = xpad_init_input(xpad);
+		if (error) {
+			/* complain only, not much else we can do here */
+			dev_err(&xpad->dev->dev, "unable to init device\n");
+		}
+	} else {
+		input_unregister_device(xpad->dev);
+	}
+}
+
 /*
  * xpad360w_process_packet
  *
@@ -451,16 +470,22 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
  * 01.1 - Pad state (Bytes 4+) valid
  *
  */
-
 static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
 {
 	/* Presence change */
 	if (data[0] & 0x08) {
 		if (data[1] & 0x80) {
-			xpad->pad_present = 1;
-			usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
-		} else
-			xpad->pad_present = 0;
+			if (!xpad->pad_present) {
+				xpad->pad_present = 1;
+				usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
+				schedule_work(&xpad->work);
+			}
+		} else {
+			if (xpad->pad_present) {
+				xpad->pad_present = 0;
+				schedule_work(&xpad->work);
+			}
+		}
 	}
 
 	/* Valid pad data */
@@ -507,10 +532,13 @@ static void xpad_irq_in(struct urb *urb)
 	}
 
 exit:
-	retval = usb_submit_urb(urb, GFP_ATOMIC);
-	if (retval)
-		dev_err(dev, "%s - usb_submit_urb failed with result %d\n",
-			__func__, retval);
+	if (xpad->pad_present) {
+		retval = usb_submit_urb(urb, GFP_ATOMIC);
+		if (retval)
+			dev_err(dev,
+				"%s - usb_submit_urb failed with result %d\n",
+				__func__, retval);
+	}
 }
 
 static void xpad_bulk_out(struct urb *urb)
@@ -991,6 +1019,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
 	xpad->mapping = xpad_device[i].mapping;
 	xpad->xtype = xpad_device[i].xtype;
 	xpad->name = xpad_device[i].name;
+	INIT_WORK(&xpad->work, presence_work_function);
 
 	if (xpad->xtype == XTYPE_UNKNOWN) {
 		if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) {
@@ -1136,7 +1165,8 @@ static void xpad_disconnect(struct usb_interface *intf)
 	struct usb_xpad *xpad = usb_get_intfdata (intf);
 
 	xpad_led_disconnect(xpad);
-	input_unregister_device(xpad->dev);
+	if (xpad->pad_present)
+		input_unregister_device(xpad->dev);
 	xpad_deinit_output(xpad);
 
 	if (xpad->xtype == XTYPE_XBOX360W) {
-- 
1.8.5.3

[PATCH 7/7] Input: xpad: properly name the LED class devices

[Greg Kroah-Hartman]

Don't just increment the LED device number, but use the joystick id so
that you have a chance to associate the LED device to the correct xpad
device by the name, instead of having to use the sysfs tree, which
really doesn't work.

Cc: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/input/joystick/xpad.c | 40 +++++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 23 deletions(-)

diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index d342d41a7a0d..ae156de46a12 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -781,8 +781,6 @@ static void xpad_led_set(struct led_classdev *led_cdev,
 
 static int xpad_led_probe(struct usb_xpad *xpad)
 {
-	static atomic_t led_seq	= ATOMIC_INIT(0);
-	long led_no;
 	struct xpad_led *led;
 	struct led_classdev *led_cdev;
 	int error;
@@ -794,9 +792,7 @@ static int xpad_led_probe(struct usb_xpad *xpad)
 	if (!led)
 		return -ENOMEM;
 
-	led_no = (long)atomic_inc_return(&led_seq) - 1;
-
-	snprintf(led->name, sizeof(led->name), "xpad%ld", led_no);
+	snprintf(led->name, sizeof(led->name), "xpad%d", xpad->joydev_id);
 	led->xpad = xpad;
 
 	led_cdev = &led->led_cdev;
@@ -944,16 +940,17 @@ static int xpad_init_input(struct usb_xpad *xpad)
 	}
 
 	error = xpad_init_ff(xpad);
-	if (error)
-		goto fail_init_ff;
-
-	error = xpad_led_probe(xpad);
-	if (error)
-		goto fail_init_led;
+	if (error) {
+		input_free_device(input_dev);
+		return error;
+	}
 
 	error = input_register_device(xpad->dev);
-	if (error)
-		goto fail_input_register;
+	if (error) {
+		input_ff_destroy(input_dev);
+		input_free_device(input_dev);
+		return error;
+	}
 
 	joydev_dev = device_find_child(&xpad->dev->dev, NULL,
 				       xpad_find_joydev);
@@ -966,17 +963,14 @@ static int xpad_init_input(struct usb_xpad *xpad)
 		xpad_send_led_command(xpad, (xpad->joydev_id % 4) + 2);
 	}
 
-	return 0;
-
-fail_input_register:
-	xpad_led_disconnect(xpad);
-
-fail_init_led:
-	input_ff_destroy(input_dev);
+	error = xpad_led_probe(xpad);
+	if (error) {
+		input_ff_destroy(input_dev);
+		input_unregister_device(input_dev);
+		return error;
+	}
 
-fail_init_ff:
-	input_free_device(input_dev);
-	return error;
+	return 0;
 }
 
 static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id)
-- 
1.8.5.3

Re: [PATCH 4/7] Input: xpad: Set the correct LED number

[Greg Kroah-Hartman]

On Fri, Jan 31, 2014 at 02:03:31PM +0100, Greg Kroah-Hartman wrote:
> From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> 
> The LED number should not just be incremented every time, that doesn't
> work, set the number based on the number it actually is.
> 
> Note, the joydev subsystem doesn't allow us to easily find out the minor
> number, so we have to walk all devices in order to find a joystick
> device by looking at the name of the device.  Odds are, this isn't the
> best way to do it, but I don't know of any other way at the moment.
> 
> Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/input/joystick/xpad.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)

I'll preempt David's NAK of this patch before he gets a chance to catch
up on his email from being at FOSDEM :)

I spent some time talking with David about this, and I think I know a
better way to handle this instead of the "walk the joystick devices"
mess that should work for systems that don't even use joydev (which
"modern" systems shouldn't be using anyway.)

So I'll do a respin of this patch series in a week or so when I catch up
with my other stuff and send it out again...

But, if someone wants to review the other 6 in this series, that would
be great.

thanks,

greg k-h

Re: [PATCH 2/7] Input: xpad: set the LEDs properly on XBox Wireless controllers

[David Herrmann]

Hi

On Fri, Jan 31, 2014 at 2:03 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
>
> Add the logic to set the LEDs on XBox Wireless controllers.  Command
> sequence found by sniffing the Windows data stream when plugging the
> device in.
>
> Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/input/joystick/xpad.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 517829f6a58b..aabff9140aaa 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -715,15 +715,37 @@ struct xpad_led {
>
>  static void xpad_send_led_command(struct usb_xpad *xpad, int command)
>  {
> -       if (command >= 0 && command < 14) {
> -               mutex_lock(&xpad->odata_mutex);
> +       if (command > 15)
> +               return;

That's really weird. The "command" argument is used to control which
of the LEDs are enabled, but the underlying led_classdev passes the
brightness value here. Shouldn't we have one led_classdev device for
each LED and make "max_brightness"==1 so it's a boolean value?
I do that for wiimotes so you end up with 4 sysfs entries, one for each LED.

Anyhow, you change "command < 14" to "command > 15" here, is this
intentional also for the XTYPE_XBOX360 path?

> +
> +       mutex_lock(&xpad->odata_mutex);
> +
> +       switch (xpad->xtype) {
> +       case XTYPE_XBOX360:
>                 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);
> -               mutex_unlock(&xpad->odata_mutex);
> +               break;
> +       case XTYPE_XBOX360W:
> +               xpad->odata[0] = 0x00;
> +               xpad->odata[1] = 0x00;
> +               xpad->odata[2] = 0x08;
> +               xpad->odata[3] = 0x40 + (command % 0x0e);

This basically makes /sys/..../led/brightness a "circular" value here.
Seems weird, but acceptable. But if you bail-out early above with
"command > 15", this here is equivalent to "command & 0x0e", right?

How about removing the "if (command > 15)" above and make both paths
use "(command % 0x0e)"? Anyhow, besides changing the XTYPE_XBOX360
path, patch looks good.

Thanks
David

> +               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;
> +               break;
>         }
> +
> +       usb_submit_urb(xpad->irq_out, GFP_KERNEL);
> +       mutex_unlock(&xpad->odata_mutex);
>  }
>
>  static void xpad_led_set(struct led_classdev *led_cdev,
> @@ -743,7 +765,7 @@ static int xpad_led_probe(struct usb_xpad *xpad)
>         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);
> --
> 1.8.5.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/7] Input: xpad: handle "present" and "gone" correctly

[David Herrmann]

Hi

On Fri, Jan 31, 2014 at 2:03 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
>
> Handle the "a new device is present" message properly by dynamically
> creating the input device at this point in time.  This requires a
> workqueue as we are in interrupt context when we learn about this.
>
> Also properly disconnect any devices that we are told are removed.
>
> Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/input/joystick/xpad.c | 50 ++++++++++++++++++++++++++++++++++---------
>  1 file changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 7a07b95790d7..d342d41a7a0d 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -293,8 +293,11 @@ struct usb_xpad {
>         int xtype;                      /* type of xbox device */
>         int joydev_id;                  /* the minor of the device */
>         const char *name;               /* name of the device */
> +       struct work_struct work;        /* to init/remove device from callback */
>  };
>
> +static int xpad_init_input(struct usb_xpad *xpad);
> +
>  /*
>   *     xpad_process_packet
>   *
> @@ -437,6 +440,22 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
>         input_sync(dev);
>  }
>
> +static void presence_work_function(struct work_struct *work)
> +{
> +       struct usb_xpad *xpad = container_of(work, struct usb_xpad, work);
> +       int error;
> +
> +       if (xpad->pad_present) {
> +               error = xpad_init_input(xpad);
> +               if (error) {
> +                       /* complain only, not much else we can do here */
> +                       dev_err(&xpad->dev->dev, "unable to init device\n");
> +               }
> +       } else {
> +               input_unregister_device(xpad->dev);
> +       }
> +}
> +
>  /*
>   * xpad360w_process_packet
>   *
> @@ -451,16 +470,22 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
>   * 01.1 - Pad state (Bytes 4+) valid
>   *
>   */
> -
>  static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
>  {
>         /* Presence change */
>         if (data[0] & 0x08) {
>                 if (data[1] & 0x80) {
> -                       xpad->pad_present = 1;
> -                       usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
> -               } else
> -                       xpad->pad_present = 0;
> +                       if (!xpad->pad_present) {
> +                               xpad->pad_present = 1;
> +                               usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
> +                               schedule_work(&xpad->work);
> +                       }
> +               } else {
> +                       if (xpad->pad_present) {
> +                               xpad->pad_present = 0;
> +                               schedule_work(&xpad->work);
> +                       }
> +               }
>         }
>
>         /* Valid pad data */
> @@ -507,10 +532,13 @@ static void xpad_irq_in(struct urb *urb)
>         }
>
>  exit:
> -       retval = usb_submit_urb(urb, GFP_ATOMIC);
> -       if (retval)
> -               dev_err(dev, "%s - usb_submit_urb failed with result %d\n",
> -                       __func__, retval);
> +       if (xpad->pad_present) {
> +               retval = usb_submit_urb(urb, GFP_ATOMIC);
> +               if (retval)
> +                       dev_err(dev,
> +                               "%s - usb_submit_urb failed with result %d\n",
> +                               __func__, retval);
> +       }
>  }
>
>  static void xpad_bulk_out(struct urb *urb)
> @@ -991,6 +1019,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
>         xpad->mapping = xpad_device[i].mapping;
>         xpad->xtype = xpad_device[i].xtype;
>         xpad->name = xpad_device[i].name;
> +       INIT_WORK(&xpad->work, presence_work_function);
>
>         if (xpad->xtype == XTYPE_UNKNOWN) {
>                 if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) {
> @@ -1136,7 +1165,8 @@ static void xpad_disconnect(struct usb_interface *intf)
>         struct usb_xpad *xpad = usb_get_intfdata (intf);
>
>         xpad_led_disconnect(xpad);
> -       input_unregister_device(xpad->dev);

You need cancel_work_sync(&xpad->work) here.
And I think you need to do that *after* killing the urbs. Otherwise,
the work might get rescheduled in parallel to this disconnect
callback.

Thanks
David

> +       if (xpad->pad_present)
> +               input_unregister_device(xpad->dev);
>         xpad_deinit_output(xpad);
>
>         if (xpad->xtype == XTYPE_XBOX360W) {
> --
> 1.8.5.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/7] Input: xpad: properly name the LED class devices

[David Herrmann]

Hi

On Fri, Jan 31, 2014 at 2:03 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> Don't just increment the LED device number, but use the joystick id so
> that you have a chance to associate the LED device to the correct xpad
> device by the name, instead of having to use the sysfs tree, which
> really doesn't work.
>
> Cc: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/input/joystick/xpad.c | 40 +++++++++++++++++-----------------------
>  1 file changed, 17 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index d342d41a7a0d..ae156de46a12 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -781,8 +781,6 @@ static void xpad_led_set(struct led_classdev *led_cdev,
>
>  static int xpad_led_probe(struct usb_xpad *xpad)
>  {
> -       static atomic_t led_seq = ATOMIC_INIT(0);
> -       long led_no;
>         struct xpad_led *led;
>         struct led_classdev *led_cdev;
>         int error;
> @@ -794,9 +792,7 @@ static int xpad_led_probe(struct usb_xpad *xpad)
>         if (!led)
>                 return -ENOMEM;
>
> -       led_no = (long)atomic_inc_return(&led_seq) - 1;
> -
> -       snprintf(led->name, sizeof(led->name), "xpad%ld", led_no);
> +       snprintf(led->name, sizeof(led->name), "xpad%d", xpad->joydev_id);

I guess that patch should be dropped, too?
Why not use the usb-interface here? It's quite common to use bt-mac
addresses for BT devices, so something similar for USB seems fine to
me.

Thanks
David

>         led->xpad = xpad;
>
>         led_cdev = &led->led_cdev;
> @@ -944,16 +940,17 @@ static int xpad_init_input(struct usb_xpad *xpad)
>         }
>
>         error = xpad_init_ff(xpad);
> -       if (error)
> -               goto fail_init_ff;
> -
> -       error = xpad_led_probe(xpad);
> -       if (error)
> -               goto fail_init_led;
> +       if (error) {
> +               input_free_device(input_dev);
> +               return error;
> +       }
>
>         error = input_register_device(xpad->dev);
> -       if (error)
> -               goto fail_input_register;
> +       if (error) {
> +               input_ff_destroy(input_dev);
> +               input_free_device(input_dev);
> +               return error;
> +       }
>
>         joydev_dev = device_find_child(&xpad->dev->dev, NULL,
>                                        xpad_find_joydev);
> @@ -966,17 +963,14 @@ static int xpad_init_input(struct usb_xpad *xpad)
>                 xpad_send_led_command(xpad, (xpad->joydev_id % 4) + 2);
>         }
>
> -       return 0;
> -
> -fail_input_register:
> -       xpad_led_disconnect(xpad);
> -
> -fail_init_led:
> -       input_ff_destroy(input_dev);
> +       error = xpad_led_probe(xpad);
> +       if (error) {
> +               input_ff_destroy(input_dev);
> +               input_unregister_device(input_dev);
> +               return error;
> +       }
>
> -fail_init_ff:
> -       input_free_device(input_dev);
> -       return error;
> +       return 0;
>  }
>
>  static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id)
> --
> 1.8.5.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/7] Input: xpad: properly name the LED class devices

[Greg Kroah-Hartman]

On Mon, Feb 03, 2014 at 06:39:20PM +0100, David Herrmann wrote:
> Hi
> 
> On Fri, Jan 31, 2014 at 2:03 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > Don't just increment the LED device number, but use the joystick id so
> > that you have a chance to associate the LED device to the correct xpad
> > device by the name, instead of having to use the sysfs tree, which
> > really doesn't work.
> >
> > Cc: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  drivers/input/joystick/xpad.c | 40 +++++++++++++++++-----------------------
> >  1 file changed, 17 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> > index d342d41a7a0d..ae156de46a12 100644
> > --- a/drivers/input/joystick/xpad.c
> > +++ b/drivers/input/joystick/xpad.c
> > @@ -781,8 +781,6 @@ static void xpad_led_set(struct led_classdev *led_cdev,
> >
> >  static int xpad_led_probe(struct usb_xpad *xpad)
> >  {
> > -       static atomic_t led_seq = ATOMIC_INIT(0);
> > -       long led_no;
> >         struct xpad_led *led;
> >         struct led_classdev *led_cdev;
> >         int error;
> > @@ -794,9 +792,7 @@ static int xpad_led_probe(struct usb_xpad *xpad)
> >         if (!led)
> >                 return -ENOMEM;
> >
> > -       led_no = (long)atomic_inc_return(&led_seq) - 1;
> > -
> > -       snprintf(led->name, sizeof(led->name), "xpad%ld", led_no);
> > +       snprintf(led->name, sizeof(led->name), "xpad%d", xpad->joydev_id);
> 
> I guess that patch should be dropped, too?

Yes it should.

> Why not use the usb-interface here? It's quite common to use bt-mac
> addresses for BT devices, so something similar for USB seems fine to
> me.

I don't know if the interface number of the device corrisponds to the
"number" of the order that the devices are connected to the wireless
basestation.  I'll have to do some debugging to determine this first...

But yes, we should use something like that instead of the joydev minor
number, especially if we don't want people to use joydev anymore :)

thanks,

greg k-h

Re: [PATCH 6/7] Input: xpad: handle "present" and "gone" correctly

[Greg Kroah-Hartman]

On Mon, Feb 03, 2014 at 06:37:13PM +0100, David Herrmann wrote:
> Hi
> 
> On Fri, Jan 31, 2014 at 2:03 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> >
> > Handle the "a new device is present" message properly by dynamically
> > creating the input device at this point in time.  This requires a
> > workqueue as we are in interrupt context when we learn about this.
> >
> > Also properly disconnect any devices that we are told are removed.
> >
> > Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  drivers/input/joystick/xpad.c | 50 ++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 40 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> > index 7a07b95790d7..d342d41a7a0d 100644
> > --- a/drivers/input/joystick/xpad.c
> > +++ b/drivers/input/joystick/xpad.c
> > @@ -293,8 +293,11 @@ struct usb_xpad {
> >         int xtype;                      /* type of xbox device */
> >         int joydev_id;                  /* the minor of the device */
> >         const char *name;               /* name of the device */
> > +       struct work_struct work;        /* to init/remove device from callback */
> >  };
> >
> > +static int xpad_init_input(struct usb_xpad *xpad);
> > +
> >  /*
> >   *     xpad_process_packet
> >   *
> > @@ -437,6 +440,22 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
> >         input_sync(dev);
> >  }
> >
> > +static void presence_work_function(struct work_struct *work)
> > +{
> > +       struct usb_xpad *xpad = container_of(work, struct usb_xpad, work);
> > +       int error;
> > +
> > +       if (xpad->pad_present) {
> > +               error = xpad_init_input(xpad);
> > +               if (error) {
> > +                       /* complain only, not much else we can do here */
> > +                       dev_err(&xpad->dev->dev, "unable to init device\n");
> > +               }
> > +       } else {
> > +               input_unregister_device(xpad->dev);
> > +       }
> > +}
> > +
> >  /*
> >   * xpad360w_process_packet
> >   *
> > @@ -451,16 +470,22 @@ static void xpad360_process_packet(struct usb_xpad *xpad,
> >   * 01.1 - Pad state (Bytes 4+) valid
> >   *
> >   */
> > -
> >  static void xpad360w_process_packet(struct usb_xpad *xpad, u16 cmd, unsigned char *data)
> >  {
> >         /* Presence change */
> >         if (data[0] & 0x08) {
> >                 if (data[1] & 0x80) {
> > -                       xpad->pad_present = 1;
> > -                       usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
> > -               } else
> > -                       xpad->pad_present = 0;
> > +                       if (!xpad->pad_present) {
> > +                               xpad->pad_present = 1;
> > +                               usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
> > +                               schedule_work(&xpad->work);
> > +                       }
> > +               } else {
> > +                       if (xpad->pad_present) {
> > +                               xpad->pad_present = 0;
> > +                               schedule_work(&xpad->work);
> > +                       }
> > +               }
> >         }
> >
> >         /* Valid pad data */
> > @@ -507,10 +532,13 @@ static void xpad_irq_in(struct urb *urb)
> >         }
> >
> >  exit:
> > -       retval = usb_submit_urb(urb, GFP_ATOMIC);
> > -       if (retval)
> > -               dev_err(dev, "%s - usb_submit_urb failed with result %d\n",
> > -                       __func__, retval);
> > +       if (xpad->pad_present) {
> > +               retval = usb_submit_urb(urb, GFP_ATOMIC);
> > +               if (retval)
> > +                       dev_err(dev,
> > +                               "%s - usb_submit_urb failed with result %d\n",
> > +                               __func__, retval);
> > +       }
> >  }
> >
> >  static void xpad_bulk_out(struct urb *urb)
> > @@ -991,6 +1019,7 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
> >         xpad->mapping = xpad_device[i].mapping;
> >         xpad->xtype = xpad_device[i].xtype;
> >         xpad->name = xpad_device[i].name;
> > +       INIT_WORK(&xpad->work, presence_work_function);
> >
> >         if (xpad->xtype == XTYPE_UNKNOWN) {
> >                 if (intf->cur_altsetting->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) {
> > @@ -1136,7 +1165,8 @@ static void xpad_disconnect(struct usb_interface *intf)
> >         struct usb_xpad *xpad = usb_get_intfdata (intf);
> >
> >         xpad_led_disconnect(xpad);
> > -       input_unregister_device(xpad->dev);
> 
> You need cancel_work_sync(&xpad->work) here.
> And I think you need to do that *after* killing the urbs. Otherwise,
> the work might get rescheduled in parallel to this disconnect
> callback.

Ah, you are right, that should fix a USB core warning of submitting an
urb when the device has been removed that I saw a few times in testing.
Thanks for pointing it out.

greg k-h

Re: [PATCH 2/7] Input: xpad: set the LEDs properly on XBox Wireless controllers

[Greg Kroah-Hartman]

On Mon, Feb 03, 2014 at 06:31:29PM +0100, David Herrmann wrote:
> Hi
> 
> On Fri, Jan 31, 2014 at 2:03 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> >
> > Add the logic to set the LEDs on XBox Wireless controllers.  Command
> > sequence found by sniffing the Windows data stream when plugging the
> > device in.
> >
> > Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  drivers/input/joystick/xpad.c | 32 +++++++++++++++++++++++++++-----
> >  1 file changed, 27 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> > index 517829f6a58b..aabff9140aaa 100644
> > --- a/drivers/input/joystick/xpad.c
> > +++ b/drivers/input/joystick/xpad.c
> > @@ -715,15 +715,37 @@ struct xpad_led {
> >
> >  static void xpad_send_led_command(struct usb_xpad *xpad, int command)
> >  {
> > -       if (command >= 0 && command < 14) {
> > -               mutex_lock(&xpad->odata_mutex);
> > +       if (command > 15)
> > +               return;
> 
> That's really weird. The "command" argument is used to control which
> of the LEDs are enabled, but the underlying led_classdev passes the
> brightness value here. Shouldn't we have one led_classdev device for
> each LED and make "max_brightness"==1 so it's a boolean value?
> I do that for wiimotes so you end up with 4 sysfs entries, one for each LED.

That would make more sense, but would require a userspace daemon to be
setting the LED values.  Is there such a thing out there?

I agree the "write a value of 4 and it turns on led 4" does not match
well with the "brightness" file description at all, I don't think that's
good.

> Anyhow, you change "command < 14" to "command > 15" here, is this
> intentional also for the XTYPE_XBOX360 path?

I don't know, Pierre-Loup?

> > +
> > +       mutex_lock(&xpad->odata_mutex);
> > +
> > +       switch (xpad->xtype) {
> > +       case XTYPE_XBOX360:
> >                 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);
> > -               mutex_unlock(&xpad->odata_mutex);
> > +               break;
> > +       case XTYPE_XBOX360W:
> > +               xpad->odata[0] = 0x00;
> > +               xpad->odata[1] = 0x00;
> > +               xpad->odata[2] = 0x08;
> > +               xpad->odata[3] = 0x40 + (command % 0x0e);
> 
> This basically makes /sys/..../led/brightness a "circular" value here.
> Seems weird, but acceptable. But if you bail-out early above with
> "command > 15", this here is equivalent to "command & 0x0e", right?
> 
> How about removing the "if (command > 15)" above and make both paths
> use "(command % 0x0e)"? Anyhow, besides changing the XTYPE_XBOX360
> path, patch looks good.

That sounds good, will do.

Many thanks for the review,

greg k-h

Re: [PATCH 5/7] Input: xpad: disconnect all Wireless controllers at init

[Pierre-Loup A. Griffais]

There's apparently a packet you can send to the receiver that prompts it 
to immediately send feedback about all the paired controllers that would 
be ideal as a replacement for this kludge. I will test it and send out a 
replacement patch for 5/7 in that series if it turns out to work well. 
Thanks a lot to Zachary Lund for pointing this out.

On 01/31/2014 05:03 AM, Greg Kroah-Hartman wrote:
> From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
>
> We initializing the driver/device, we really don't know how many
> controllers are connected.  So send a "disconnect all" command to the
> base-station, and let the user pair the controllers in the order in
> which they want them assigned.
>
> Note, this means we now do not "preallocate" all 4 devices when a single
> wireless base station is seen, but require the device to be properly
> connected to the base station before that can happen.  The allocation of
> the device happens in the next patch, not here, so in a way, this patch
> breaks all wireless devices...
>
> Because we want to talk to the device, we have to set up the output urbs
> no matter if we have CONFIG_JOYSTICK_XPAD_FF or
> CONFIG_JOYSTICK_XPAD_LEDS enabled not, so take out the code that allows
> those variables and code to be disabled (it's so small it should never
> matter...)
>
> Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>   drivers/input/joystick/xpad.c | 46 ++++++++++++++++++++++++++++++++-----------
>   1 file changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 7997ae89a877..7a07b95790d7 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -278,12 +278,10 @@ struct usb_xpad {
>   	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 */
>   	dma_addr_t odata_dma;
>   	struct mutex odata_mutex;
> -#endif
>
>   #if defined(CONFIG_JOYSTICK_XPAD_LEDS)
>   	struct xpad_led *led;
> @@ -537,7 +535,6 @@ static void xpad_bulk_out(struct urb *urb)
>   	}
>   }
>
> -#if defined(CONFIG_JOYSTICK_XPAD_FF) || defined(CONFIG_JOYSTICK_XPAD_LEDS)
>   static void xpad_irq_out(struct urb *urb)
>   {
>   	struct usb_xpad *xpad = urb->context;
> @@ -623,11 +620,6 @@ static void xpad_deinit_output(struct usb_xpad *xpad)
>   				xpad->odata, xpad->odata_dma);
>   	}
>   }
> -#else
> -static int xpad_init_output(struct usb_interface *intf, struct usb_xpad *xpad) { return 0; }
> -static void xpad_deinit_output(struct usb_xpad *xpad) {}
> -static void xpad_stop_output(struct usb_xpad *xpad) {}
> -#endif
>
>   #ifdef CONFIG_JOYSTICK_XPAD_FF
>   static int xpad_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect)
> @@ -1091,11 +1083,41 @@ static int xpad_probe(struct usb_interface *intf, const struct usb_device_id *id
>   			usb_kill_urb(xpad->irq_in);
>   			goto fail9;
>   		}
> +
> +		/*
> +		 * We don't know how to check the controller state at driver
> +		 * load, so just disconnect them all, requiring the user to
> +		 * repair the device in the order they want them used.  Good
> +		 * point is that we don't connect devices in "random" order,
> +		 * but the extra step seems a bit harsh as other operating
> +		 * systems don't require this at the moment.
> +		 *
> +		 * Power-off packet information came from an OS-X
> +		 * reverse-engineered driver located at:
> +		 * http://tattiebogle.net/index.php/ProjectRoot/Xbox360Controller/OsxDriver#toc1
> +		 */
> +		mutex_lock(&xpad->odata_mutex);
> +		xpad->odata[0] = 0x00;
> +		xpad->odata[1] = 0x00;
> +		xpad->odata[2] = 0x08;
> +		xpad->odata[3] = 0xC0;
> +		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;
> +		usb_submit_urb(xpad->irq_out, GFP_KERNEL);
> +		mutex_unlock(&xpad->odata_mutex);
> +	} else {
> +		xpad->pad_present = 1;
> +		error = xpad_init_input(xpad);
> +		if (error)
> +			goto fail9;
>   	}
> -	xpad->pad_present = 1;
> -	error = xpad_init_input(xpad);
> -	if (error)
> -		goto fail9;
>
>   	return 0;
>
>

Re: [PATCH 2/7] Input: xpad: set the LEDs properly on XBox Wireless controllers

[Pierre-Loup A. Griffais]

On 02/03/2014 11:48 AM, Greg Kroah-Hartman wrote:
> On Mon, Feb 03, 2014 at 06:31:29PM +0100, David Herrmann wrote:
>> Hi
>>
>> On Fri, Jan 31, 2014 at 2:03 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>> From: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
>>>
>>> Add the logic to set the LEDs on XBox Wireless controllers.  Command
>>> sequence found by sniffing the Windows data stream when plugging the
>>> device in.
>>>
>>> Signed-off-by: "Pierre-Loup A. Griffais" <pgriffais@valvesoftware.com>
>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> ---
>>>   drivers/input/joystick/xpad.c | 32 +++++++++++++++++++++++++++-----
>>>   1 file changed, 27 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
>>> index 517829f6a58b..aabff9140aaa 100644
>>> --- a/drivers/input/joystick/xpad.c
>>> +++ b/drivers/input/joystick/xpad.c
>>> @@ -715,15 +715,37 @@ struct xpad_led {
>>>
>>>   static void xpad_send_led_command(struct usb_xpad *xpad, int command)
>>>   {
>>> -       if (command >= 0 && command < 14) {
>>> -               mutex_lock(&xpad->odata_mutex);
>>> +       if (command > 15)
>>> +               return;
>>
>> That's really weird. The "command" argument is used to control which
>> of the LEDs are enabled, but the underlying led_classdev passes the
>> brightness value here. Shouldn't we have one led_classdev device for
>> each LED and make "max_brightness"==1 so it's a boolean value?
>> I do that for wiimotes so you end up with 4 sysfs entries, one for each LED.
>
> That would make more sense, but would require a userspace daemon to be
> setting the LED values.  Is there such a thing out there?

I don't believe so, and it would be very nice if the driver could do 
that much by itself (ideally with less hackery than what I came up 
with!) without needing distros to package a daemon just to make sure the 
controllers light up to reflect the right slot.

>
> I agree the "write a value of 4 and it turns on led 4" does not match
> well with the "brightness" file description at all, I don't think that's
> good.

The interface to the HW is as follows (taken from the output of 'xboxdrv 
--help-led'):

    0: off
    1: all blinking
    2: 1/top-left blink, then on
    3: 2/top-right blink, then on
    4: 3/bottom-left blink, then on
    5: 4/bottom-right blink, then on
    6: 1/top-left on
    7: 2/top-right on
    8: 3/bottom-left on
    9: 4/bottom-right on
   10: rotate
   11: blink
   12: blink slower
   13: rotate with two lights
   14: blink
   15: blink once

Since this was all exposed as-is through 'brightness' before, should it 
just be left alone in case people already rely on this behavior?

>
>> Anyhow, you change "command < 14" to "command > 15" here, is this
>> intentional also for the XTYPE_XBOX360 path?
>
> I don't know, Pierre-Loup?

LED command 15 corresponds to 'blink once' for both variants AFAIK, 
which is why I changed that code originally. It definitely wasn't a 
critical part of the patch and what proposed below sounds reasonable 
instead.

Thanks,
  - Pierre-Loup

>
>>> +
>>> +       mutex_lock(&xpad->odata_mutex);
>>> +
>>> +       switch (xpad->xtype) {
>>> +       case XTYPE_XBOX360:
>>>                  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);
>>> -               mutex_unlock(&xpad->odata_mutex);
>>> +               break;
>>> +       case XTYPE_XBOX360W:
>>> +               xpad->odata[0] = 0x00;
>>> +               xpad->odata[1] = 0x00;
>>> +               xpad->odata[2] = 0x08;
>>> +               xpad->odata[3] = 0x40 + (command % 0x0e);
>>
>> This basically makes /sys/..../led/brightness a "circular" value here.
>> Seems weird, but acceptable. But if you bail-out early above with
>> "command > 15", this here is equivalent to "command & 0x0e", right?
>>
>> How about removing the "if (command > 15)" above and make both paths
>> use "(command % 0x0e)"? Anyhow, besides changing the XTYPE_XBOX360
>> path, patch looks good.
>
> That sounds good, will do.
>
> Many thanks for the review,
>
> greg k-h
>

Re: [PATCH 0/7] Input: xpad: Fix wireless controller connection and leds

[Zachary Lund]

Greg Kroah-Hartman <gregkh <at> linuxfoundation.org> writes:

> 
> Here's a series of patches from Pierre-Loup and I that rework the xpad
> driver to fix the issue where when a wireless xpad controller is plugged
> in, 4 joystick devices are created, no matter how many are really
> attached to the system.  Pierre-Loup's patches fix this by dynamically
> creating the devices only when they are found by the wireless
> controller.
> 
> Bonus is that the LEDs now work correctly with this series too, when
> mixing wireless and wired, we can properly identify which device is
> which on the X-LED.  We also now name the LED device based on the
> joystick id, not on an atomic number that keeps incrementing, never
> decrementing when removed from the system.
> 
> Note, patch 4/7 is a crazy hack to get the minor number of the joystick
> id that we have registered with the system.  Pierre-Loup came up with
> this, and I really can't figure out any other way to do it, the joydev
> layer doesn't even know this information, otherwise I would have added
> it to that layer so the xpad driver could call it.  Am I missing
> something here that we could do instead?
> 
> The first patch in the series fixes a really annoying bug when plugging
> in one of these controllers to a USB 3 system.  The URB type is
> incorrect so the xhci driver complains about it, rightly so.  I think
> the original code was incorrect, but left it alone incase there is
> really a crazy device with a bulk endpoint instead of interrupt.
> 
> Also, even with this series applied, the xpad driver needs a bunch of
> work.  The LED device seems pointless as it doesn't actually work, and
> given that the number/name of the LED device was impossible to actually
> map to the proper xpad device, there's no way userspace code could ever
> actually use it.  I think it should be removed entirely.  There's also
> some other USB things that should be cleaned up, the bulk urb doesn't
> need to be always running (with lots of disconnect/connect cycles you
> can get a warning about it running when trying to submit it again), and
> the urb callbacks seem a bit strange.  I'll work on those issues next...
> 
> This series was based on a larger patch from Pierre-Loup that I broke up
> into pieces, and added some of my own where needed to resolve other
> issues.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo <at> vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Since it may help, I'll add a link to my driver here. I have no intention of 
trying to mainstream this driver and have used it personally for slightly 
better support than what's provided now. I've recently done even more work 
on it for the fun of it (not sure if I'm cut out for kernel hacking...).

Use at your own risk:
https://github.com/computerquip/xpad360wr

I find that the main problem with any of this is that I don't know how to 
query features e.g. whether or not a device actually has FF capabilities. 
It's currently just assumed to have FF capabilities. I suspect its in the 
announce packet for wireless devices. For wired, it's just matched up 
against a table of devices to determine what features it has.