Hi Dimitri,

Thanks for taking the time to review.  My reponses are inline.

-Brian

* Dmitry Torokhov [off-list ref] [2007-05-29 20:03]:

Thnk you for the patch. Looking at the code I do not see one device
supporintg multiple controllers... You have one input device per usb
device/interface and the properties of said device are known
beforehand. So I would just create and register input device right
there in xboxrcvr_probe() and get rid of your build and teardown
works. The fact that is it a wireless device and at transmitter may at
times be out of range is not important.

Actually, I rather liked the fact that the input devices only show up when
there is a controller connected to the receiver, since there can be anywhere
from 0-4 controllers alive.  This way there are no device nodes for
non-existant controllers.

Overall I would like support for this wireless receiver to be
incorporated into xpad.c driver.

Knew that was coming :).  Should be able to manage it.  Since the wireless
model returns a expanded data format it needs to be handled specially.
Another flag in the xpad_device struct?

I also have some more comments, see below.

quoted

+
+static unsigned long debug = 0;

You do not need to initialize statig variables to 0. It only increases
size of the image.

ok.

quoted

+module_param(debug, ulong, 0644);
+MODULE_PARM_DESC(debug, "debug level");
+

quoted

+
+       if(debug) {

Please use spaces between statement (if,for, while) and opening paren.
.

ok.

quoted

+
+       /* Valid pad data */
+       if(!(sdata[1] & 0x1) || !xboxrcvr->open)
+               return;

Why are we generating interrupts if device is not used? Have the
->open() method submit urbs and then you don't need to check it here.

A side effect of submitting the int urb as soon as a pad is detected.

quoted

+
+       /* FIXME: Yuck.  Need to make sure this doesn't get truncated */
+       usb_make_path(xboxrcvr->udev, path, sizeof(xboxrcvr->phys));
+       snprintf(xboxrcvr->phys, sizeof(xboxrcvr->phys), "%s/input%d", 
path, xboxrcvr->id);
+       snprintf(xboxrcvr->uniq, sizeof(xboxrcvr->uniq), "xpad%d", 
xboxrcvr->id >> 1);

Uniq is used to carry identificator unique for the device, not within
a particular system. It is property of device itself and should not
change if moved to a different box so do not try to come up with
artificial data for it.

Removed.  That got put in early as something for udev to key off of.  Turns
out I don't need it anyway of course.

quoted

+
+       input_dev->name     = xboxrcvr_device[0].name;

Hmm....

Heh.  That came from xpad.c which looped through the device IDs and then
attached the name of the matched device in the input dev.  Since my driver
only has one ID I removed that loop and put a 0 in there. :)

quoted

+       input_dev->phys     = xboxrcvr->phys;
+       input_dev->uniq     = xboxrcvr->uniq;
+       input_dev->cdev.dev = &(xboxrcvr->intf->dev);

Please use

input_dev->dev.parent = &xboxrcvr->intf->dev;

- input devices are meving moved from class_device to struct device.

quoted

+       input_dev->private  = xboxrcvr;

input_set_drvdata(input_dev, xboxrcvr); And use input_get_drvdata()
instead of accessing dev->private directly.

ok

Dmitry

Thanks again,
-Brian