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@xxxxxxxxxxxxxxxxxxx> wrote:
From: "Pierre-Loup A. Griffais" <pgriffais@xxxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxx>
Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
