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

[Joe Perches <joe@perches.com>]

On Thu, 2014-05-15 at 14:28 +0200, Ricardo Ribalda Delgado wrote:
> This patch adds support for the PLX USB3380 and USB3382.

A few more trivial notes, all can be addressed
as follow-on patches or perhaps even ignored
altogether.

> diff --git a/drivers/usb/gadget/net2280.c b/drivers/usb/gadget/net2280.c

[]

> @@ -90,11 +97,12 @@ static const char *const ep_name [] = {
>   */
>  static bool use_dma = 1;
>  static bool use_dma_chaining = 0;
> +static bool use_msi = 1;

Nicer to use true/false instead of 1/0
 
[]

> @@ -161,11 +172,20 @@ net2280_enable (struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>  	if ((desc->bEndpointAddress & 0x0f) == EP_DONTUSE)
>  		return -EDOM;
>  
> +	if (dev->pdev->vendor == 0x10b5) {

There are several tests of vendor that
could use #define constants instead of
magic numbers

Also, perhaps these parts will be used
by other vendors and might have to be
expanded.

>  	/* sanity check ep-e/ep-f since their fifos are small */
>  	max = usb_endpoint_maxp (desc) & 0x1fff;
> -	if (ep->num > 4 && max > 64)
> +	if (ep->num > 4 && max > 64 && (dev->pdev->vendor == 0x17cc))
>  		return -ERANGE;

Maybe too specific to one vendor ID?
 
> +

Superfluous blank line

> @@ -176,7 +196,8 @@ net2280_enable (struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>  	ep->out_overflow = 0;
>  
>  	/* set speed-dependent max packet; may kick in high bandwidth */
> -	set_idx_reg (dev->regs, REG_EP_MAXPKT (dev, ep->num), max);
> +	set_idx_reg(dev->regs, (dev->enhanced_mode) ? ep_enhanced[ep->num]
> +					: REG_EP_MAXPKT(dev, ep->num), max);

Maybe a static inline/macro helper function for this?

	dev->enhanced_mode ? ep_enhanced[ep->num] : REG_EP_MAXPKT(dev, ep->num)
 
> @@ -226,11 +267,13 @@ net2280_enable (struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
[]
>  	/* enable irqs */
>  	if (!ep->dma) {				/* pio, per-packet */
> -		tmp = (1 << ep->num) | readl (&dev->regs->pciirqenb0);
> +		tmp = (dev->pdev->vendor == 0x17cc)?(1 << ep->num)
> +						   : (1 << ep_bit[ep->num]);

And another static inline/macro helper for this?

> @@ -251,8 +294,10 @@ net2280_enable (struct usb_ep *_ep, const struct usb_endpoint_descriptor *desc)
>  			tmp = (1 << SHORT_PACKET_TRANSFERRED_INTERRUPT_ENABLE);
>  			writel (tmp, &ep->regs->ep_irqenb);
>  
> -			tmp = (1 << ep->num) | readl (&dev->regs->pciirqenb0);
> -			writel (tmp, &dev->regs->pciirqenb0);
> +			tmp = (dev->pdev->vendor == 0x17cc)?(1 << ep->num)
> +						: (1 << ep_bit[ep->num]);

This one is used twice and should use at
least the same formatting.

[]

> @@ -361,6 +407,55 @@ static void ep_reset (struct net2280_regs __iomem *regs, struct net2280_ep *ep)
[]
> +	writel((1 << SHORT_PACKET_OUT_DONE_INTERRUPT) |
> +	       (1 << SHORT_PACKET_TRANSFERRED_INTERRUPT) |
> +	       (1 << FIFO_OVERFLOW) |
> +	       (1 << DATA_PACKET_RECEIVED_INTERRUPT) |
> +	       (1 << DATA_PACKET_TRANSMITTED_INTERRUPT) |
> +	       (1 << DATA_OUT_PING_TOKEN_INTERRUPT) |
> +	       (1 << DATA_IN_TOKEN_INTERRUPT), &ep->regs->ep_stat);
> +}

Perhaps these should use the BIT macro

	writel((BIT(SHORT_PACKET_OUT_DONE_INTERRUPT) |
		BIT(SHORT_PACKET_TRANSFERRED_INTERRUPT) |
		BIT(FIFO_OVERFLOW) |
etc...
> 
@@ -374,13 +469,17 @@ static int net2280_disable (struct usb_ep *_ep)
>  
>  	spin_lock_irqsave (&ep->dev->lock, flags);
>  	nuke (ep);
> -	ep_reset (ep->dev->regs, ep);
> +
> +	if (ep->dev->pdev->vendor == 0x10b5)

Another vendor ID that could be a #define

> @@ -874,8 +990,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)) {

here too

> +static void defect7374_workaround(struct net2280 *dev, struct usb_ctrlrequest r)
[]
> +	if (ack_wait_timeout >= DEFECT_7374_NUMBEROF_MAX_WAIT_LOOPS) {
> +		ERROR(dev, "FAIL: Defect 7374 workaround waited but failed");
> +		ERROR(dev, "to detect SS host's data phase ACK.");
> +		ERROR(dev, "PL_EP_STATUS_1(23:16):.Expected from 0x11 to 0x16");
> +		ERROR(dev, "got 0x%2.2x.\n", state >> STATE);

I'd use just 2 output lines/ERROR calls here.

> +	} else {
> +		WARNING(dev, "INFO: Defect 7374 workaround waited about\n");
> +		WARNING(dev, "%duSec for Control Read Data Phase ACK\n",
> +			DEFECT_7374_PROCESSOR_WAIT_TIME * ack_wait_timeout);

and just 1 here