Re: [PATCH 2/2 v2] usb: gadget: net2280: Add support for PLX USB338X

[Joe Perches <joe@perches.com>]

On Wed, 2014-05-14 at 19:39 +0200, Ricardo Ribalda Delgado wrote:
> This patch adds support for the PLX USB3380 and USB3382.
> 
> This driver is based on the driver from the manufacturer.
> 
> Since USB338X is register compatible with NET2280, I thought that it
> would be better to include this hardware into net2280 driver.
> 
> Manufacturer's driver only supported the USB33X, did not follow the
> Kernel Style and contain some trivial errors. This patch has tried to
> address this issues.
> 
> This patch has only been tested on USB338x hardware, but the merge has
> been done trying to not affect the behaviour of NET2280.

trivial notes:

> diff --git a/drivers/usb/gadget/net2280.c b/drivers/usb/gadget/net2280.c
[]
> @@ -148,6 +155,10 @@ net2280_enable (struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>  	struct net2280_ep	*ep;
>  	u32			max, tmp;
>  	unsigned long		flags;
> +	static const u32 ep_bit[9] = { 0, 17, 2, 19, 4, 1, 18, 3, 20 };
[]
> @@ -361,6 +407,56 @@ static void ep_reset (struct net2280_regs __iomem *regs, struct net2280_ep *ep)
[]
> +static void ep_reset_338x(struct net2280_regs __iomem *regs,
> +					struct net2280_ep *ep)
> +{
> +	u32 tmp, dmastat;
> +	static const u32 ep_bit[9] = { 0, 17, 2, 19, 4, 1, 18, 3, 20 };

To avoid duplication, perhaps ep_bit should be
declared static const to the module instead of 
declared in multiple functions.

> @@ -874,8 +991,23 @@ net2280_queue (struct usb_ep *_ep, struct usb_request *_req, gfp_t gfp_flags)
>  
>  	/* kickstart this i/o queue? */
>  	if (list_empty (&ep->queue) && !ep->stopped) {
> +		/* DMA request while EP halted */
> +		if (ep->dma
> +		    && (readl(&ep->regs->ep_rsp) & (1 << CLEAR_ENDPOINT_HALT))
> +			&& (dev->pdev->vendor == 0x10b5)) {

Generally logical continuations are at the end-of-line like
		if (ep->dma &&
		    (readl(&ep->regs->ep_rsp) & (1 << CLEAR_ENDPOINT_HALT)) &&
		    (dev->pdev->vendor == 0x10b5)) {

[]

> @@ -1820,7 +2149,122 @@ static void usb_reinit (struct net2280 *dev)
[]
> +		if (dev->enhanced_mode) {
> +			ep->cfg = &dev->epregs[ne[i]];
> +			ep->regs =
> +			    (struct net2280_ep_regs __iomem
> +			     *)(((unsigned char *)&dev->epregs[ne[i]]) +
> +				ep_reg_addr[i]);

I find this pretty difficult to read.
Temporaries might help.

Or maybe cast ep->cfg like
			ep->regs = (struct net2280_ep_regs __iomem *)
				   ((unsigned char *)ep->cfg + ep_reg_addr[i]);
[]

> +	/* AA_AB Errata. Issue 4. Workaround for SuperSpeed USB
> +	   Hot Reset Exit Handshake may Fail in Specific Case using
> +	   Default Register Settings. Workaround for Enumeration test.
> +	 */

linux-kernel multi-line comment style is generally:

	/*
	 * comments...
	 */