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-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
