Re: [patch v4 0/4] Cypress c67x00 (EZ-Host/EZ-OTG) support

[Peter Korsgaard <jacmet@sunsite.dk>]

>>>>> "Grant" == Grant Likely <grant.likely@secretlab.ca> 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 <grant.likely@secretlab.ca>

 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