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@xxxxxxxxxxxxx> 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:
signature.asc
Description: Digital signature
