Features Download
From: Haavard Skinnemoen <hskinnemoen <at> atmel.com>
Subject: [PATCH v2 0/2] Atmel USBA UDC driver, take 2
Newsgroups: gmane.linux.usb.devel
Date: Wednesday 26th September 2007 16:28:55 UTC (over 10 years ago)
Hi David,

Thanks for reviewing the driver, and sorry for taking so long to
respond to your comments. Here's a new version of the driver which
should take care of most of your comments, and also includes support
for isochronous transfers and a bug fix.

I've made the following changes since the previous patch:
      usba: Kill compile errors and warnings
      usba: Turn to_usba_* into inline functions
      usba: Eliminate most of the ep_is_X() macros
      usba: Introduce CONFIG_USB_GADGET_DEBUG_FS and use it
      usba: Keep Kconfig entries in alphabetical order
      usba: Move #defines into header file
      usba: Kill __devinit and __devexit
      usba: Request Vbus irq during probe
      usba: Fix endpoint definitions in usba_ep array
      usba: Fix configuration of high bandwidth isochronous endpoints
      usba: Use correct number of transactions per microframe
      usba: Don't reset data toggle when clearing stall feature on ep0

Checkpatch still has a couple of complaints, but they are IMHO wrong:

WARNING: braces {} are not necessary for single statement blocks
#498: FILE: drivers/usb/gadget/atmel_usba_udc.c:385:
+       } else if (transaction_len == ep->ep.maxpacket
+                  && req->req.zero) {
+               req->last_transaction = 0;
+       }

This is correct according to Documentation/CodingStyle. If one branch
in a conditional statement needs braces, they should be used in all of

ERROR: Macros with complex values should be enclosed in parenthesis
#1114: FILE: drivers/usb/gadget/atmel_usba_udc.c:1001:
+{                                                              \

This is a struct body definition, so using parentheses would be wrong.

WARNING: no space between function name and open parenthesis '('
#1200: FILE: drivers/usb/gadget/atmel_usba_udc.c:1087:
+       list_for_each_entry (ep, &udc->gadget.ep_list, ep.ep_list) {

list_for_each_entry has basically the same semantics as the 'for'
keyword, which _should_ have a space there.

WARNING: braces {} are not necessary for single statement blocks
#1359: FILE: drivers/usb/gadget/atmel_usba_udc.c:1246:
+               } else if (crq->bRequestType
+                          == (USB_DIR_IN | USB_RECIP_INTERFACE)) {
+                       status = __constant_cpu_to_le16(0);
+               } else if (crq->bRequestType

Same as the first one.

Finally, I haven't yet addressed some of your comments...

>  * I'm still not a fan of those bitfield macros -- I think they
>    obfuscate what's going on.  Especially for one-bit fields!!

I've fixed the one-bit fields, but not the rest. I'm not really sure
what to do about them.

>  * I see DMA_ADDR_INVALID == MAX_DMA_ADDRESS ... bug waiting to happen?!

Possibly. The problem is that I need a way to tell whether or not
higher-level drivers have mapped the buffer. Should I reduce
MAX_DMA_ADDRESS just to reserve room for some invalid addresses?

>  * I see a fair number of four-space and other odd indents... tabs only!

You mean spaces for alignment isn't allowed?

>  * If you don't handle remote wakeup requests, report errors ... but
>    of course, setting that feature to "off" should work!  :)

The suspend/resume/remote-wakeup logic probably needs an
overhaul. I'll have a look at that soonish.

>  * 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).

I'll have to look a bit closer at that.

>  * Likewise it assumes there will be an unbind() method ... but it's
>    OK to not have one of thise, especially for static linking.


Ok, that's all for now. If you want to wait until I've addressed the
remaining issues before continuing the review, that's fine; I'll
hopefully be able to sort out the remaining issues early next
week. Just wanted to let you know that I am actually working on the


This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
[email protected]
To unsubscribe, use the last form field at:
CD: 114ms