Quoting [EMAIL PROTECTED] <[EMAIL PROTECTED]>: > I have written a linux driver for XBOX controllers. It is version 0.0.2, so > considered pre-alpha, but already fairly functional. Check it out at: > http://www.bmx-chemnitz.de/Zeugs/xpad.tar.bz2 > > A short description is included.
I had a look, and though I am not very familiar with HID, some general
comments follow.
- If you plan to submit the driver to the kernel, then a proper patch
against the current tree must be provided. This is because it is not
enough to include C code, you also need to integrate the driver into
the kernel configuration. The maintainer can't do that because
he is not likely to have the device to test his changes.
- Indentation in the driver does not correspond to Linux standard. See
linux/Documentation/... for details and emacs settings.
> Don't worry if the vendor and/or product ID don't match, those are easy
> to add to the driver. You could do it yourself, just add the appropriate
> line into the list after the line
> '} xpad_device[] = {'
> , but before the line that says
> '{ 0x0000, 0x0000, "unknown...." }'.
You need to add driver parameters (vendorID=xxx productID=yyy). Users
don't recompile kernels. Add that to probe().
> /* Use our own dbg macro */
> #undef dbg
> #define dbg(format, arg...) do { if (debug) printk(KERN_DEBUG __FILE__
> ": " format "\n" , ## arg); } while (0)
That is bad. Create another symbol, do not reuse dbg() - it is confusing
and inconsistent (dbg() in different drivers will behave differently).
Redefining standard API calls is one of biggest taboos, comparable in its
insanity and confusion only to redefining operators in C++ ;-)
> /* Module paramaters */
> MODULE_PARM(debug, "i");
> MODULE_PARM_DESC(debug, "Debug enabled or not");
a) paramEters (typo)
b) which value enables or disables debug? Be specific, it's easy.
> { 0x05fd, 0x107a, "InterAct XBOX HID controller (Germany)" },
Those numbers are duplicated elsewhere in the code. #define them.
> epIrqIn = interface->altsetting[0].endpoint + 0;
If you want to indicate that the very first endpoint shall be used here, a
comment would be not wasted. If you want an input endpoint, either search
for it and grab the first one that matches, or check that the hardcoded
endpoint is indeed Interrupt,In - or else the driver will fail with all
sorts of funny errors if a differently laid out device is connected. If
you implement the search then your driver will be able to handle more
devices, and without changes too.
Cheers,
Dmitri
--
panic("esp: penguin esp state.");
(Panic message in the kernel.)
msg07546/pgp00000.pgp
Description: PGP signature
