Re: [PATCH v5] usb: gadget: Add UDC driver for Aeroflex Gaisler GRUSBDC

[Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2455 bytes --]

Hi,

On Mon, Dec 23, 2013 at 12:57:38PM +0100, Andreas Larsson wrote:
> This adds an UDC driver for GRUSBDC USB Device Controller cores available in the
> GRLIB VHDL IP core library. The driver only supports DMA mode.
> 
> Signed-off-by: Andreas Larsson <andreas-FkzTOoA/JUlBDgjK7y7TUQ@public.gmane.org>

You have a few Sparse Warnings:

drivers/usb/gadget/gr_udc.c:1255:13: warning: incorrect type in argument 1 (different address spaces)
drivers/usb/gadget/gr_udc.c:1255:13:    expected void [noderef] <asn:2>*<noident>
drivers/usb/gadget/gr_udc.c:1255:13:    got unsigned int *<noident>
drivers/usb/gadget/gr_udc.c:1284:16: warning: incorrect type in argument 1 (different address spaces)
drivers/usb/gadget/gr_udc.c:1284:16:    expected void [noderef] <asn:2>*<noident>
drivers/usb/gadget/gr_udc.c:1284:16:    got unsigned int *<noident>
drivers/usb/gadget/gr_udc.c:1136:28: warning: context imbalance in 'gr_ep0_setup' - unexpected unlock
drivers/usb/gadget/gr_udc.c:1052:1: warning: "/*" within comment [-Wcomment]
 /* Must be called with dev->lock held and irqs disabled

Two of them I fixed below:

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index ee220f3..5eb36af 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -218,7 +218,7 @@ config USB_FOTG210_UDC
 
 config USB_GR_UDC
        tristate "Aeroflex Gaisler GRUSBDC USB Peripheral Controller Driver"
-       depends on OF && HAS_DMA
+       depends on HAS_DMA
        help
           Select this to support Aeroflex Gaisler GRUSBDC cores from the GRLIB
 	  VHDL IP core library.
diff --git a/drivers/usb/gadget/gr_udc.c b/drivers/usb/gadget/gr_udc.c
index 4f99a61..9ac378a 100644
--- a/drivers/usb/gadget/gr_udc.c
+++ b/drivers/usb/gadget/gr_udc.c
@@ -1049,9 +1049,11 @@ static void gr_ep0out_requeue(struct gr_udc *dev)
 /*
  * The main function dealing with setup requests on ep0.
  *
-/* Must be called with dev->lock held and irqs disabled
+ * Must be called with dev->lock held and irqs disabled
  */
 static void gr_ep0_setup(struct gr_udc *dev, struct gr_request *req)
+	__releases(&dev->lock)
+	__acquires(&dev->lock)
 {
 	union {
 		struct usb_ctrlrequest ctrl;

The other two are seem like they would require a much larger code
change. Why are you passing integers as arguments to IO access
functions ? You should be passing void __iomem *.

Dropping for now.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]