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

[Ricardo Ribalda Delgado]

Hello Joe

 Thanks for your comments. I have already prepared a v3 with your
comments. I wait some hours for more comments and then I resubmit the
patch to avoid spamming the list.

Thanks for your time!

On Wed, May 14, 2014 at 7:59 PM, Joe Perches <joe@xxxxxxxxxxx> wrote:
> 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...
>          */
>
>
>



-- 
Ricardo Ribalda
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/