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

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 2693 bytes --]

On Sun, Aug 19, 2012 at 01:53:26PM -0700, Kevin Cernekee wrote:
> On Sun, Aug 19, 2012 at 1:17 PM, Sebastian Andrzej Siewior
> <sebastian@breakpoint.cc> wrote:
> > On Sat, Aug 18, 2012 at 10:18:01AM -0700, Kevin Cernekee wrote:
> >
> > This is a quick look :)
> 
> Thanks for the review.
> 
> >> +     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.
> 
> OK, I will change it to warn + return IRQ_NONE, instead of BUG().
> 
> That situation shouldn't ever happen anyway.  It would mean that our
> ISR is getting called with somebody else's IRQ number, or the iudma
> structs were corrupted.
> 
> Probe does bail out if any of the IRQ resources are missing.
> 
> >> +     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().
> 
> There are 6 IUDMA channels, and each one always has a dedicated
> interrupt line.  IRQ resource #0 is the control (vbus/speed/cfg/etc.)
> IRQ, and IRQ resources #1-6 are the IUDMA (IN/OUT data) IRQs.  Maybe
> it would be good to add a longer comment to clarify this?

If you actually have separate IRQ lines, you should request_irq() for
each line, which will again render the for loop pointless.

> An earlier iteration of the code had passed in an IRQ range, which
> worked for 6328, but then it was pointed out that the IRQ numbers are
> not contiguous on all platforms.  So 7 individual resources are indeed
> necessary.

correct. But nevertheless, you should have a separate request_irq() for
each line, continuous or not continuous.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]