>>>>> "Grant" == Grant Likely <[EMAIL PROTECTED]> writes:
Grant> Okay, I've had a chance to read through this. I haven't Grant> tested it, but I don't see anything I strongly disagree with. Grant> I've just got a few editorial comments below and a question. Ok, great. Grant> The question is about the device structure which used to be provided Grant> by the platform device instances and now there just uses the c67x00's Grant> device struct. I was under the impression that each USB HCD needs to Grant> have it's own struct device. I take it that's not true? Not afaik. In fact, I changed this based on feedback from David back when I first time posted the patch series: http://article.gmane.org/gmane.linux.usb.devel/53496 But I don't have a board with host connectors on both SIEs, so I haven't actually been able to test it. Grant> Otherwise: Grant> Acked-by: Grant Likely <[EMAIL PROTECTED]> Grant> Cheers, Grant> g. >> >> diff --git a/drivers/usb/c67x00/c67x00-drv.c >> b/drivers/usb/c67x00/c67x00-drv.c >> index 0f0720a..c2ea3b6 100644 >> --- a/drivers/usb/c67x00/c67x00-drv.c >> +++ b/drivers/usb/c67x00/c67x00-drv.c >> @@ -203,19 +175,19 @@ static int __devinit c67x00_drv_probe(struct >> platform_device *pdev) >> } >> >> for (i = 0; i < C67X00_SIES; i++) >> - c67x00_probe_sie(&c67x00->sie[i]); >> + c67x00_probe_sie(&c67x00->sie[i], c67x00, i); >> >> platform_set_drvdata(pdev, c67x00); >> >> return 0; >> >> - reset_failed: >> +reset_failed: >> free_irq(res2->start, c67x00); >> - request_irq_failed: >> +request_irq_failed: >> iounmap(c67x00->hpi.base); >> - map_failed: >> +map_failed: >> release_mem_region(res->start, res->end - res->start + 1); >> - request_mem_failed: >> +request_mem_failed: Grant> A single space should be preserved in front of the labels. Doing so Grant> means that git-diff picks up the function name instead of the label Grant> when generating output. Ok. Emacs doesn't seem to do this by default. >> diff --git a/drivers/usb/c67x00/c67x00-hcd.c >> b/drivers/usb/c67x00/c67x00-hcd.c >> index 3d0b77e..4afb291 100644 >> --- a/drivers/usb/c67x00/c67x00-hcd.c >> +++ b/drivers/usb/c67x00/c67x00-hcd.c >> @@ -368,23 +383,26 @@ int c67x00_hcd_probe(struct c67x00_sie *sie) >> goto err2; >> } >> >> + spin_lock_irqsave(&sie->lock, flags); >> + sie->private_data = c67x00; >> + sie->irq = c67x00_hcd_irq; >> + spin_unlock_irqrestore(&sie->lock, flags); >> + >> return retval; >> - err2: >> +err2: >> c67x00_sched_stop_scheduler(c67x00); >> - err1: >> +err1: >> usb_put_hcd(hcd); >> - err0: >> +err0: Grant> Ditto on the labels >> diff --git a/drivers/usb/c67x00/c67x00-hcd.h >> b/drivers/usb/c67x00/c67x00-hcd.h >> index 5b35f01..daee4cd 100644 >> --- a/drivers/usb/c67x00/c67x00-hcd.h >> +++ b/drivers/usb/c67x00/c67x00-hcd.h >> @@ -132,6 +147,6 @@ void c67x00_sched_kick(struct c67x00_hcd *c67x00); >> int c67x00_sched_start_scheduler(struct c67x00_hcd *c67x00); >> void c67x00_sched_stop_scheduler(struct c67x00_hcd *c67x00); >> >> -#define c67x00_hcd_dev(x) (c67x00_hcd_to_hcd(x)->self.controller) >> +#define c67x00_dev(x) (c67x00_hcd_to_hcd(x)->self.controller) Grant> Can the name c67x00_hcd_dev() be used instead? This macro is used Grant> with 'struct c67x00_hcd', not 'struct c67x00' and I find the code less Grant> confusing if the macro name reflects that. Well, we can. I didn't copy your name change over as the hcds don't have their own struct device in my series, but I don't feel strongly about the issue. >> >> #endif /* _USB_C67X00_HCD_H */ >> diff --git a/drivers/usb/c67x00/c67x00-sched.c >> b/drivers/usb/c67x00/c67x00-sched.c >> index 35d7318..3140d89 100644 >> --- a/drivers/usb/c67x00/c67x00-sched.c >> +++ b/drivers/usb/c67x00/c67x00-sched.c >> - /* Something went wrong; unwind the allocations */ >> - err_epdata: >> +err_epdata: >> kfree(urbp); >> - err_urbp: >> +err_urbp: >> usb_hcd_unlink_urb_from_ep(hcd, urb); >> - err_not_linked: >> +err_not_linked: Grant> ditto Grant> Cheers, Grant> g. Thanks for the feedback, I'll post an updated series. -- Bye, Peter Korsgaard - To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
