| Message ID | 1391173414-6199-3-git-send-email-gregkh@linuxfoundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 -- 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
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 > -- 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
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);