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: Felipe Balbi <balbi@ti.com>
Date: Mon, 20 Aug 2012 10:24:27 +0300
Cc: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>, balbi@ti.com, ralf@linux-mips.org, linux-mips@linux-mips.org, linux-usb@vger.kernel.org
In-reply-to: <CAJiQ=7CADk_75U5=OQH8vXA=xtj-U=TbBhXzC8JfUGbYEKmxng@mail.gmail.com>
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> <20120819201714.GA3152@breakpoint.cc> <CAJiQ=7CADk_75U5=OQH8vXA=xtj-U=TbBhXzC8JfUGbYEKmxng@mail.gmail.com>
Reply-to: balbi@ti.com
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.5.21 (2010-09-15)
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: signature.asc
Description: Digital signature

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