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?
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.
