Re: [PATCH 2/3] Driver for the Atmel USBA on-chip UDC

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/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel