| Message ID | 201105141929.00404.kode54@morbo.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Chris, On Sat, May 14, 2011 at 07:29:00PM -0700, Chris Moeller wrote: > 1) It removes the bulk send interface and replaces it with a permanent instance of the send interface, which fixes sending LED commands to XBox360 Wireless Controllers, including the initial player number indicator so the light ring will stop flashing endlessly. > 2) It implements rumble support for XBox360 Wireless Controllers. > Thank you for the patch, please find some comments below. > Signed-off-by: Chris Moeller <kode54@gmail.com> > > --- > > I could not separate the two features into different patches because the rumble support change depends on initialization and shutdown changes that are necessary for both features. > It is OK if patches are dependent upon each other. In this case I'd recommend adding XBox360 Wireless rumble first, keeping the rest of the driver structure, and then reworking LED support, which requires more changes. > Resending this patch which KMail managed to munge because I did not know that it line wraps to fit the composer window regardless of the line wrapping setting. Let's see if a maximized window on 1920x1080 is big enough to get this out without wrapping anything, shall we? > > I'm not sure if it matters, but I found the documentation necessary to implement the packet for XBox360 Wireless Controller rumble support in the source code to the drivers here: > > http://www.katch.ne.jp/~morii/driver/x360wc/index.html > > Neither the page nor the documentation included with the driver or source code, nor the source code itself, make any mention of a license. Does this matter much in the case of deriving knowledge of a single 12 byte data structure? > As long as the code was not used I think this should be OK. > --- linux/drivers/input/joystick/xpad.c.orig 2011-03-14 18:20:32.000000000 -0700 > +++ linux/drivers/input/joystick/xpad.c 2011-05-14 18:41:08.000000000 -0700 > @@ -250,20 +250,17 @@ struct usb_xpad { > struct usb_device *udev; /* usb device */ > > int pad_present; > + > + int interface_number; > > 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 */ > dma_addr_t odata_dma; > struct mutex odata_mutex; > -#endif > > #if defined(CONFIG_JOYSTICK_XPAD_LEDS) > struct xpad_led *led; > @@ -432,13 +429,15 @@ static void xpad360_process_packet(struc > * > */ > > +static void xpad_send_led_command(struct usb_xpad *xpad, int command, int mutex); > + > 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); > + xpad_send_led_command(xpad, 0x42 + ((xpad->interface_number / 2) & 3), 0); I'd prefer the "command" calculation be hidden in xpad_send_led_command(), i.e. it should accept led number and figure out the proper command byte based on device type. > } else > xpad->pad_present = 0; > } > @@ -492,24 +491,6 @@ exit: > __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) > { > int retval, status; > @@ -545,9 +526,6 @@ static int xpad_init_output(struct usb_i > struct usb_endpoint_descriptor *ep_irq_out; > int error; > > - if (xpad->xtype != XTYPE_XBOX360 && xpad->xtype != XTYPE_XBOX) > - return 0; > - > xpad->odata = usb_alloc_coherent(xpad->udev, XPAD_PKT_LEN, > GFP_KERNEL, &xpad->odata_dma); > if (!xpad->odata) { > @@ -579,23 +557,15 @@ static int xpad_init_output(struct usb_i > > static void xpad_stop_output(struct usb_xpad *xpad) > { > - if (xpad->xtype == XTYPE_XBOX360 || xpad->xtype == XTYPE_XBOX) > - usb_kill_urb(xpad->irq_out); > + usb_kill_urb(xpad->irq_out); > } > > static void xpad_deinit_output(struct usb_xpad *xpad) > { > - if (xpad->xtype == XTYPE_XBOX360 || xpad->xtype == XTYPE_XBOX) { > - usb_free_urb(xpad->irq_out); > - usb_free_coherent(xpad->udev, XPAD_PKT_LEN, > - xpad->odata, xpad->odata_dma); > - } > + usb_free_urb(xpad->irq_out); > + usb_free_coherent(xpad->udev, XPAD_PKT_LEN, > + 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) > @@ -632,6 +602,23 @@ static int xpad_play_effect(struct input > > return usb_submit_urb(xpad->irq_out, GFP_ATOMIC); > > + case 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; > + > + return usb_submit_urb(xpad->irq_out, GFP_ATOMIC); > + > default: > dbg("%s - rumble command sent to unsupported xpad type: %d", > __func__, xpad->xtype); > @@ -644,7 +631,7 @@ static int xpad_play_effect(struct input > > static int xpad_init_ff(struct usb_xpad *xpad) > { > - if (xpad->xtype != XTYPE_XBOX360 && xpad->xtype != XTYPE_XBOX) > + if (xpad->xtype != XTYPE_XBOX360 && xpad->xtype != XTYPE_XBOX && xpad->xtype != XTYPE_XBOX360W) This should probably be: if (xpad->xtype == XTYPE_UNKNOWN) > return 0; > > input_set_capability(xpad->dev, EV_FF, FF_RUMBLE); > @@ -665,16 +652,33 @@ struct xpad_led { > struct usb_xpad *xpad; > }; > > -static void xpad_send_led_command(struct usb_xpad *xpad, int command) > +static void xpad_send_led_command(struct usb_xpad *xpad, int command, int mutex) > { > if (command >= 0 && command < 14) { > - mutex_lock(&xpad->odata_mutex); > + if (mutex) mutex_lock(&xpad->odata_mutex); No, this conditional locking will not do. What happens if xpad_send_led_command() gets called from both xpad360w_process_packet() and xpad_led_set()? There is also bigger problem - we are using the same URB/buffers as FF playback so we need to make sure we do not stomp on each other toes. Thanks.
On Mon, May 16, 2011 at 12:37 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > It is OK if patches are dependent upon each other. In this case I'd > recommend adding XBox360 Wireless rumble first, keeping the rest of the > driver structure, and then reworking LED support, which requires more > changes. Implementing rumble alone for the 360 Wireless pad will probably be a lot easier, and may even make it in. I'll step up to do that, assuming you don't plan to just take the command structure I posted and post a patch yourself. Maybe a lack of hardware prevents this, or maybe this should be a collaborative effort and I can do it? Hmm... > As long as the code was not used I think this should be OK. Yeah, I only used the command structure, which I presume belongs to Microsoft anyway, since it's their hardware. > I'd prefer the "command" calculation be hidden in > xpad_send_led_command(), i.e. it should accept led number and figure out > the proper command byte based on device type. Yeah, I can do that too. > This should probably be: > > if (xpad->xtype == XTYPE_UNKNOWN) Makes sense. I was only accounting for the possibility of other device types making it into the known devices set and making it through that condition check. Of course, that's not a problem at this point. > No, this conditional locking will not do. What happens if > xpad_send_led_command() gets called from both xpad360w_process_packet() > and xpad_led_set()? There is also bigger problem - we are using the same > URB/buffers as FF playback so we need to make sure we do not stomp on > each other toes. Yeah, maybe I should have made another IRQ output buffer for the LED commands. I just could not make the bulk output work at all. Although I'm not sure if that matters, if both the external LED interface and the FF playback lock the output buffer before using it. Also, the reason for the conditional locking is because the LED setting function needs to be called from within the driver itself for the player assignment that occurs when a wireless pad is added. Locking in that case causes the driver to hang when a wireless pad is connected. If you'll check, the condition to lock is cleared when called from within the driver lock in that case, but set so it always attempts to lock when it's called from the external interface functions. There's probably a better way of doing that, though. -Chris -- 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
--- linux/drivers/input/joystick/xpad.c.orig 2011-03-14 18:20:32.000000000 -0700 +++ linux/drivers/input/joystick/xpad.c 2011-05-14 18:41:08.000000000 -0700 @@ -250,20 +250,17 @@ struct usb_xpad { struct usb_device *udev; /* usb device */ int pad_present; + + int interface_number; 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 */ dma_addr_t odata_dma; struct mutex odata_mutex; -#endif #if defined(CONFIG_JOYSTICK_XPAD_LEDS) struct xpad_led *led; @@ -432,13 +429,15 @@ static void xpad360_process_packet(struc * */ +static void xpad_send_led_command(struct usb_xpad *xpad, int command, int mutex); + 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); + xpad_send_led_command(xpad, 0x42 + ((xpad->interface_number / 2) & 3), 0); } else xpad->pad_present = 0; } @@ -492,24 +491,6 @@ exit: __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) { int retval, status; @@ -545,9 +526,6 @@ static int xpad_init_output(struct usb_i struct usb_endpoint_descriptor *ep_irq_out; int error; - if (xpad->xtype != XTYPE_XBOX360 && xpad->xtype != XTYPE_XBOX) - return 0; - xpad->odata = usb_alloc_coherent(xpad->udev, XPAD_PKT_LEN, GFP_KERNEL, &xpad->odata_dma); if (!xpad->odata) { @@ -579,23 +557,15 @@ static int xpad_init_output(struct usb_i static void xpad_stop_output(struct usb_xpad *xpad) { - if (xpad->xtype == XTYPE_XBOX360 || xpad->xtype == XTYPE_XBOX) - usb_kill_urb(xpad->irq_out); + usb_kill_urb(xpad->irq_out); } static void xpad_deinit_output(struct usb_xpad *xpad) { - if (xpad->xtype == XTYPE_XBOX360 || xpad->xtype == XTYPE_XBOX) { - usb_free_urb(xpad->irq_out); - usb_free_coherent(xpad->udev, XPAD_PKT_LEN, - xpad->odata, xpad->odata_dma); - } + usb_free_urb(xpad->irq_out); + usb_free_coherent(xpad->udev, XPAD_PKT_LEN, + 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) @@ -632,6 +602,23 @@ static int xpad_play_effect(struct input return usb_submit_urb(xpad->irq_out, GFP_ATOMIC); + case 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; + + return usb_submit_urb(xpad->irq_out, GFP_ATOMIC); + default: dbg("%s - rumble command sent to unsupported xpad type: %d", __func__, xpad->xtype); @@ -644,7 +631,7 @@ static int xpad_play_effect(struct input static int xpad_init_ff(struct usb_xpad *xpad) { - if (xpad->xtype != XTYPE_XBOX360 && xpad->xtype != XTYPE_XBOX) + if (xpad->xtype != XTYPE_XBOX360 && xpad->xtype != XTYPE_XBOX && xpad->xtype != XTYPE_XBOX360W) return 0; input_set_capability(xpad->dev, EV_FF, FF_RUMBLE); @@ -665,16 +652,33 @@ struct xpad_led { struct usb_xpad *xpad; }; -static void xpad_send_led_command(struct usb_xpad *xpad, int command) +static void xpad_send_led_command(struct usb_xpad *xpad, int command, int mutex) { if (command >= 0 && command < 14) { - mutex_lock(&xpad->odata_mutex); + if (mutex) 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); - mutex_unlock(&xpad->odata_mutex); + if (mutex) mutex_unlock(&xpad->odata_mutex); + } else if (command >= 0x40 && command <= 0x4F) { + if (mutex) mutex_lock(&xpad->odata_mutex); + xpad->odata[0] = 0x00; + xpad->odata[1] = 0x00; + xpad->odata[2] = 0x08; + xpad->odata[3] = command; + 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); + if (mutex) mutex_unlock(&xpad->odata_mutex); } } @@ -684,7 +688,7 @@ static void xpad_led_set(struct led_clas struct xpad_led *xpad_led = container_of(led_cdev, struct xpad_led, led_cdev); - xpad_send_led_command(xpad_led->xpad, value); + xpad_send_led_command(xpad_led->xpad, value, 1); } static int xpad_led_probe(struct usb_xpad *xpad) @@ -721,7 +725,7 @@ static int xpad_led_probe(struct usb_xpa /* * Light up the segment corresponding to controller number */ - xpad_send_led_command(xpad, (led_no % 4) + 2); + xpad_send_led_command(xpad, (led_no % 4) + 2, 1); return 0; } @@ -920,45 +924,11 @@ static int xpad_probe(struct usb_interfa goto fail6; usb_set_intfdata(intf, xpad); + + xpad->interface_number = intf->cur_altsetting->desc.bInterfaceNumber; if (xpad->xtype == XTYPE_XBOX360W) { /* - * 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) { - error = -ENOMEM; - goto fail7; - } - - xpad->bdata = kzalloc(XPAD_PKT_LEN, GFP_KERNEL); - if (!xpad->bdata) { - error = -ENOMEM; - goto fail8; - } - - 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); - - /* * Submit the int URB immediately rather than waiting for open * because we get status messages from the device whether * or not any controllers are attached. In fact, it's @@ -968,13 +938,11 @@ static int xpad_probe(struct usb_interfa xpad->irq_in->dev = xpad->udev; error = usb_submit_urb(xpad->irq_in, GFP_KERNEL); if (error) - goto fail9; + goto fail7; } 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); @@ -998,8 +966,6 @@ static void xpad_disconnect(struct usb_i 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); } @@ -1007,7 +973,6 @@ static void xpad_disconnect(struct usb_i usb_free_coherent(xpad->udev, XPAD_PKT_LEN, xpad->idata, xpad->idata_dma); - kfree(xpad->bdata); kfree(xpad); usb_set_intfdata(intf, NULL);
1) It removes the bulk send interface and replaces it with a permanent instance of the send interface, which fixes sending LED commands to XBox360 Wireless Controllers, including the initial player number indicator so the light ring will stop flashing endlessly. 2) It implements rumble support for XBox360 Wireless Controllers. Signed-off-by: Chris Moeller <kode54@gmail.com> --- I could not separate the two features into different patches because the rumble support change depends on initialization and shutdown changes that are necessary for both features. Resending this patch which KMail managed to munge because I did not know that it line wraps to fit the composer window regardless of the line wrapping setting. Let's see if a maximized window on 1920x1080 is big enough to get this out without wrapping anything, shall we? I'm not sure if it matters, but I found the documentation necessary to implement the packet for XBox360 Wireless Controller rumble support in the source code to the drivers here: http://www.katch.ne.jp/~morii/driver/x360wc/index.html Neither the page nor the documentation included with the driver or source code, nor the source code itself, make any mention of a license. Does this matter much in the case of deriving knowledge of a single 12 byte data structure? -- 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