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

[Sebastian Andrzej Siewior <sebastian@breakpoint.cc>]

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