Hi Dimitri,
Thanks for taking the time to review. My reponses are inline.
-Brian
* Dmitry Torokhov <[EMAIL PROTECTED]> [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.
>
> >+
> >+static unsigned long debug = 0;
>
> You do not need to initialize statig variables to 0. It only increases
> size of the image.
ok.
>
> >+module_param(debug, ulong, 0644);
> >+MODULE_PARM_DESC(debug, "debug level");
> >+
>
> >+
> >+ if(debug) {
>
> Please use spaces between statement (if,for, while) and opening paren.
>.
ok.
> >+
> >+ /* 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.
>
> >+
> >+ /* 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.
>
> >+
> >+ 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. :)
> >+ 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.
>
> >+ 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
