On Tue, Nov 30, 2010 at 09:41:10PM +0300, Sergei Shtylyov wrote:
> Hello.
>
> Sarah Sharp wrote:
>
> >>>Refactor out the code in usb_add_hcd() to request the IRQ line for the
> >>>HCD. This will only need to be called once for the two xHCI roothubs, so
> >>>it's easier to refactor it into a function, rather than wrapping the long
> >>>if-else block into another if statement.
>
> >>>Signed-off-by: Sarah Sharp<sarah.a.sharp@xxxxxxxxxxxxxxx>
> >>[...]
>
> >>>diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> >>>index 02ce408..fda5a11 100644
> >>>--- a/drivers/usb/core/hcd.c
> >>>+++ b/drivers/usb/core/hcd.c
> >>>@@ -2197,6 +2197,44 @@ void usb_put_hcd (struct usb_hcd *hcd)
> >>> }
> >>> EXPORT_SYMBOL_GPL(usb_put_hcd);
> >>>
> >>>+static int usb_hcd_request_irqs(struct usb_hcd *hcd,
> >>>+ unsigned int irqnum, unsigned long irqflags)
> >>>+{
> >>>+ int retval;
> >>>+
> >>>+ if (hcd->driver->irq) {
> >>>+
> >>>+ /* IRQF_DISABLED doesn't work as advertised when used together
> >>>+ * with IRQF_SHARED. As usb_hcd_irq() will always disable
> >>>+ * interrupts we can remove it here.
> >>>+ */
> >>>+ if (irqflags & IRQF_SHARED)
> >>>+ irqflags &= ~IRQF_DISABLED;
> >>>+
> >>>+ snprintf(hcd->irq_descr, sizeof(hcd->irq_descr), "%s:usb%d",
> >>>+ hcd->driver->description, hcd->self.busnum);
> >>>+ if ((retval = request_irq(irqnum,&usb_hcd_irq, irqflags,
>
> >> IIRC, checkpatch.pl complains about assignment being a part of
> >>the *if* operator...
>
> I meant to type "statement". "Operator" would be a mechanical
> translation from Russian. :-)
>
> >Yes, that's not my code, I just moved it. I might create a patch before
> >this one to fix the original code, but IMO the fix shouldn't be part of
> >the refactoring patch.
>
> I'd disagree, as the change needed is not that serious.
Changing the code you're refactoring in one patch is a great way to
introduce subtle bugs. People see "refactoring patch" and skim the
additions, and possibly miss something critical that looks *almost* the
same as the old patch. Plus it makes it harder to review. So I'll err
on the safe side and make it a new patch.
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
