Messages in this thread Patch in this message | | | Subject | Re: [PATCH 2/2 v4] usb: gadget: net2280: Add support for PLX USB338X | From | Joe Perches <> | Date | Thu, 15 May 2014 11:40:27 -0700 |
| |
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
| |