linux-mips
[Top] [All Lists]

Re: [PATCH] usb: gadget: bcm63xx UDC driver

To: Kevin Cernekee <cernekee@gmail.com>
Subject: Re: [PATCH] usb: gadget: bcm63xx UDC driver
From: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Date: Sun, 19 Aug 2012 22:17:14 +0200
Cc: balbi@ti.com, ralf@linux-mips.org, linux-mips@linux-mips.org, linux-usb@vger.kernel.org
In-reply-to: <97cb21b8063a02a9664baf8b749ae200@localhost>
List-archive: <http://www.linux-mips.org/archives/linux-mips/>
List-help: <mailto:ecartis@linux-mips.org?Subject=help>
List-id: linux-mips <linux-mips.eddie.linux-mips.org>
List-owner: <mailto:ralf@linux-mips.org>
List-post: <mailto:linux-mips@linux-mips.org>
List-software: Ecartis version 1.0.0
List-subscribe: <mailto:ecartis@linux-mips.org?subject=subscribe%20linux-mips>
List-unsubscribe: <mailto:ecartis@linux-mips.org?subject=unsubscribe%20linux-mips>
References: <97cb21b8063a02a9664baf8b749ae200@localhost>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.5.21 (2010-09-15)
On Sat, Aug 18, 2012 at 10:18:01AM -0700, Kevin Cernekee wrote:

This is a quick look :)

> diff --git a/drivers/usb/gadget/bcm63xx_udc.c 
> b/drivers/usb/gadget/bcm63xx_udc.c
> new file mode 100644
> index 0000000..da68f43
> --- /dev/null
> +++ b/drivers/usb/gadget/bcm63xx_udc.c
<snip>

> +static irqreturn_t bcm63xx_udc_data_isr(int irq, void *dev_id)
> +{
> +     struct bcm63xx_udc *udc = dev_id;
> +     struct bcm63xx_ep *bep;
> +     struct iudma_ch *iudma = NULL;
> +     struct usb_request *req = NULL;
> +     struct bcm63xx_req *breq = NULL;
> +     int is_done = 0, rc, i;
> +
> +     spin_lock(&udc->lock);
> +
> +     for (i = 0; i < NUM_IUDMA; i++)
> +             if (udc->iudma[i].irq == irq)
> +                     iudma = &udc->iudma[i];
> +     BUG_ON(!iudma);

This is rough. Please don't do this. Bail out in probe or print an error here
and return with IRQ_NONE and time will close this irq.

<snip>

> +static int __devinit bcm63xx_udc_probe(struct platform_device *pdev)
> +{

<snip>

> +     for (i = 0; i < NUM_IUDMA + 1; i++) {
> +             int irq = platform_get_irq(pdev, i);
> +             if (irq < 0) {
> +                     dev_err(dev, "missing IRQ resource #%d\n", i);
> +                     goto out_uninit;
> +             }
> +             if (devm_request_irq(dev, irq,
> +                 i ? &bcm63xx_udc_data_isr : &bcm63xx_udc_ctrl_isr,
> +                 0, dev_name(dev), udc) < 0) {
> +                     dev_err(dev, "error requesting IRQ #%d\n", irq);
> +                     goto out_uninit;
> +             }
> +             if (i > 0)
> +                     udc->iudma[i - 1].irq = irq;
> +     }

According to this code, i in iudma[] can be in 1..5. You could have more than
one IRQ. The comment above this for loop is point less. So I think if you can
only have _one_ idma irq than you could remove the for loop in
bcm63xx_udc_data_isr().

Sebastian

<Prev in Thread] Current Thread [Next in Thread>