On Wednesday 20 June 2007, Haavard Skinnemoen wrote:
> This is a driver for the Atmel USBA UDC which can be found integrated
> on AT32AP700x AVR32 processors.
Hmm, scripts/checkpatch.pl reports a few issues (see at the end).
Mostly extraneous brackets; but please fix.
Also, it won't build against 2.6.23-rc1 ... that's more serious!!
In the header file:
* I'm still not a fan of those bitfield macros -- I think they
obfuscate what's going on. Especially for one-bit fields!!
* Please make the to_usba_X() things into inline functions, so
any type errors will obviously be reported by GCC and so folk
don't need to guess.
* Most of those ep_is_X() #defines should expand in-line; they
don't shrink code or promote clarity.
* The DEBUG_FS ifdefs should be ifdeffed USB_GADGET_DEBUG_FILES
(updating that to not rely purely on PROC_FS?) or somesuch.
Kconfig, Makefile:
* Please put this in alphabetical order; let's not worsen things!
You want this to be the default, not the discrete Renesas chip,
in any case ...
Driver ".c" file (partial comments)
* I see DMA_ADDR_INVALID == MAX_DMA_ADDRESS ... bug waiting to happen?!
* Those debug macros belong in the header file, ditto the other #defines
* The {alloc,free}_buffer() routines need to be deleted. (They
were incorrect in any case, since they didn't return dma-coherent
memory. Like many implementations. Which is why those methods
are now gone.)
* Building it showed other errors too:
drivers/usb/gadget/atmel_usba_udc.c: In function ‘do_test_mode’:
drivers/usb/gadget/atmel_usba_udc.c:1246: error: ‘pdev’ undeclared (first use
in this function)
drivers/usb/gadget/atmel_usba_udc.c:1246: error: (Each undeclared identifier is
reported only once
drivers/usb/gadget/atmel_usba_udc.c:1246: error: for each function it appears
in.)
drivers/usb/gadget/atmel_usba_udc.c: In function ‘usba_udc_probe’:
drivers/usb/gadget/atmel_usba_udc.c:1985: warning: ‘deprecated_irq_flag’ is
deprecated (declared at include/linux/interrupt.h:66)
drivers/usb/gadget/atmel_usba_udc.c:2000: error: ‘GPIO_PIN_NONE’ undeclared
(first use in t
* SA_SAMPLE_RANDOM is a bad idea here anyway ... the host can
easily control that source of "random" data.
* I see a fair number of four-space and other odd indents... tabs only!
* If you don't handle remote wakeup requests, report errors ... but
of course, setting that feature to "off" should work! :)
* Use platform_driver_probe(), shrink the runtime footprint; and use
__init not __devinit.
* Likewise, in this case use __exit and friends not __devexit
* Request any vbus irq in probe(); it can be disabled until the
gadget driver is ready.
* Your gadget driver registration code omits the sanity checks the
other drivers have ... which means gadget driver bugs will show
up late, and maybe in unfriendly ways (like oopsing).
* Likewise it assumes there will be an unbind() method ... but it's
OK to not have one of thise, especially for static linking.
Well, that's all for a quick surface scan.
If you can get me a version resolving the issues noted here,
I can start to get closer to a technical review.
- Dave
p.s. Random note: this is almost the smallest of the high speed
drivers, at least so far as bytes in main C file go:
86768 amd5536udc.c
79169 net2280.c
66463 fsl_usb2_udc.c
50965 atmel_usba_udc.c
41080 m66592-udc.c
And musb_hdrc is bigger than all of those, even if you discount
the host side support. ;)
========== scripts/checkpatch.pl output
WARNING: declaring multiple variables together should be avoided
#202: FILE: drivers/usb/gadget/atmel_usba_udc.c:117:
+ size_t len, remaining, actual = 0;
ERROR: braces {} are not necessary for single statement blocks
#498: FILE: drivers/usb/gadget/atmel_usba_udc.c:413:
+ } else if (transaction_len == ep->ep.maxpacket
+ && req->req.zero) {
+ req->last_transaction = 0;
+ }
WARNING: declaring multiple variables together should be avoided
#571: FILE: drivers/usb/gadget/atmel_usba_udc.c:486:
+ unsigned int bytecount, nr_busy;
WARNING: declaring multiple variables together should be avoided
#662: FILE: drivers/usb/gadget/atmel_usba_udc.c:577:
+ unsigned long flags, ept_cfg, maxpacket;
ERROR: braces {} are not necessary for single statement blocks
#743: FILE: drivers/usb/gadget/atmel_usba_udc.c:658:
+ } else {
+ usba_writel(udc, INT_ENB,
+ (usba_readl(udc, INT_ENB)
+ | USBA_BF(EPT_INT, 1 << ep->index)));
+ }
ERROR: braces {} are not necessary for single statement blocks
#1075: FILE: drivers/usb/gadget/atmel_usba_udc.c:990:
+ if (!list_empty(&ep->queue)
+ || ((value && ep_is_in(ep)
+ && (usba_ep_readl(ep, STA)
+ & USBA_BF(BUSY_BANKS, -1L))))) {
+ ret = -EAGAIN;
+ } else {
ERROR: Macros with complex values should be enclosed in parenthesis
#1135: FILE: drivers/usb/gadget/atmel_usba_udc.c:1050:
+{ \
ERROR: no space between function name and open parenthesis '('
#1221: FILE: drivers/usb/gadget/atmel_usba_udc.c:1136:
+ list_for_each_entry (ep, &udc->gadget.ep_list, ep.ep_list) {
ERROR: braces {} are not necessary for single statement blocks
#1378: FILE: drivers/usb/gadget/atmel_usba_udc.c:1293:
+ if (crq->bRequestType == (USB_DIR_IN | USB_RECIP_DEVICE)) {
+ /* Self-powered, no remote wakeup */
+ status = __constant_cpu_to_le16(1 << 0);
+ } else if (crq->bRequestType
ERROR: braces {} are not necessary for single statement blocks
#1381: FILE: drivers/usb/gadget/atmel_usba_udc.c:1296:
+ } else if (crq->bRequestType
+ == (USB_DIR_IN | USB_RECIP_INTERFACE)) {
+ status = __constant_cpu_to_le16(0);
+ } else if (crq->bRequestType
ERROR: braces {} are not necessary for single statement blocks
#1395: FILE: drivers/usb/gadget/atmel_usba_udc.c:1310:
+ } else {
+ goto delegate;
+ }
ERROR: braces {} are not necessary for single statement blocks
#1410: FILE: drivers/usb/gadget/atmel_usba_udc.c:1325:
+ if (feature_is_dev_remote_wakeup(crq)) {
+ /* TODO: Handle REMOTE_WAKEUP */
+ } else {
ERROR: braces {} are not necessary for single statement blocks
#1412: FILE: drivers/usb/gadget/atmel_usba_udc.c:1327:
+ } else {
+ /* Can't CLEAR_FEATURE TEST_MODE */
+ goto stall;
+ }
ERROR: braces {} are not necessary for single statement blocks
#1429: FILE: drivers/usb/gadget/atmel_usba_udc.c:1344:
+ } else {
+ goto delegate;
+ }
ERROR: braces {} are not necessary for single statement blocks
#1444: FILE: drivers/usb/gadget/atmel_usba_udc.c:1359:
+ } else if (feature_is_dev_remote_wakeup(crq)) {
+ /* TODO: Handle REMOTE_WAKEUP */
+ } else {
ERROR: braces {} are not necessary for single statement blocks
#1446: FILE: drivers/usb/gadget/atmel_usba_udc.c:1361:
+ } else {
+ goto stall;
+ }
ERROR: braces {} are not necessary for single statement blocks
#1678: FILE: drivers/usb/gadget/atmel_usba_udc.c:1593:
+ if (ret < 0) {
+ /* Let the host know that we failed */
+ set_protocol_stall(udc, ep);
+ }
WARNING: declaring multiple variables together should be avoided
#1744: FILE: drivers/usb/gadget/atmel_usba_udc.c:1659:
+ u32 status, control, pending;
WARNING: declaring multiple variables together should be avoided
#2004: FILE: drivers/usb/gadget/atmel_usba_udc.c:1919:
+ int irq, ret, i;
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
