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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
