This basically looks ok, sorry it took a while to review it.
It needs a respin on at least one point, and I'd like to see
a few other bits of the probe (and remove) logic streamlined;
see below.
- Dave
> +int usb_hcd_orion_probe(const struct hc_driver *driver,
> + struct platform_device *pdev)
> +{
> + struct usb_hcd *hcd;
> + struct resource *res;
> + int irq, retval;
> +
> + pr_debug("Initializing Orion-SoC USB Host Controller\n");
> +
> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (!res) {
> + dev_err(&pdev->dev,
> + "Found HC with no IRQ. Check %s setup!\n",
> + pdev->dev.bus_id);
> + return -ENODEV;
> + }
> + irq = res->start;
Cleaner: irq = platform_get_irq(pdev, 0); if (irq <= 0) { ... }
Then find the mem resource early too, for simpler cleanup.
> +
> + hcd = usb_create_hcd(driver, &pdev->dev, pdev->dev.bus_id);
> + if (!hcd) {
> + retval = -ENOMEM;
> + goto err1;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev,
> + "Found HC with no register addr. Check %s setup!\n",
> + pdev->dev.bus_id);
> + retval = -ENODEV;
> + goto err2;
> + }
> + hcd->rsrc_start = res->start;
> + hcd->rsrc_len = res->end - res->start + 1;
> + /*
> + * All of this is not really necessary, since all Orion
> + * registers are already ioremap by arch setup.
> + */
Yeah, but there's no way to pass physical addresses as
platform resources. Some folk also say it's not good
style to set up big static mappings; I'm not sure I'd
agree.
> + if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len,
> + driver->description)) {
> + dev_dbg(&pdev->dev, "controller already in use\n");
> + retval = -EBUSY;
> + goto err2;
> + }
> + hcd->regs = ioremap(hcd->rsrc_start, hcd->rsrc_len);
... but I don't think there's anything wrong with having
platform-specific ioremap logic (re)use static mappings,
so this wouldn't need to cost new page table entries.
> + if (hcd->regs == NULL) {
> + dev_dbg(&pdev->dev, "error mapping memory\n");
> + retval = -EFAULT;
> + goto err3;
> + }
> +
> + /*
> + * setup Orion USB controller
> + */
> + orion_usb_setup(hcd);
> +
> + retval = usb_add_hcd(hcd, irq, IRQF_SHARED);
Unless someone audits all the code paths, IRQF_DISABLED too.
Not having that recently triggered some bugs.
There were both HCD (all of them!) and usbcore paths, so
that audit likely won't happen soon...
> + if (retval != 0)
> + goto err4;
> +
> + return retval;
> +
> +err4:
> + iounmap(hcd->regs);
> +err3:
> + release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
> +err2:
> + usb_put_hcd(hcd);
> +err1:
> + dev_err(&pdev->dev, "init %s fail, %d\n",
> + pdev->dev.bus_id, retval);
> +
> + return retval;
> +}
> +
> +/*
> + * Reverses the effect of usb_hcd_orion_probe()
> + */
> +void usb_hcd_orion_remove(struct usb_hcd *hcd, struct platform_device *pdev)
> +{
> + usb_remove_hcd(hcd);
> + iounmap(hcd->regs);
> + release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
> + usb_put_hcd(hcd);
> +}
> +
> +static int ehci_orion_setup(struct usb_hcd *hcd)
> +{
> + struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> + int retval;
> +
> + /*
> + * registers start at offset 0x100
> + */
> + ehci->caps = hcd->regs + 0x100;
> + ehci->regs = hcd->regs + 0x100 +
> + HC_LENGTH(ehci_readl(ehci, &ehci->caps->hc_capbase));
> +
> + /*
> + * cache this readonly data; minimize chip reads
> + */
> + ehci->hcs_params = ehci_readl(ehci, &ehci->caps->hcs_params);
I'd have done all that along with the ioremap in probe(). :)
> +
> + retval = ehci_halt(ehci);
> + if (retval)
> + return retval;
> +
> + /*
> + * data structure init
> + */
> + retval = ehci_init(hcd);
> + if (retval)
> + return retval;
> +
> + ehci->is_tdi_rh_tt = 1;
> + ehci->sbrn = 0x20;
And those two assignments in probe() too. Maybe even
the ehci_init(), though I vaguely recall some reason
not to do that so early in some configurations.
> +
> + ehci_reset(ehci);
> + ehci_port_power(ehci, 0);
> +
> + return retval;
> +}
> +
> +static const struct hc_driver ehci_orion_hc_driver = {
> + .description = hcd_name,
> + .product_desc = "Marvell Orion EHCI",
> + .hcd_priv_size = sizeof(struct ehci_hcd),
> +
> + /*
> + * generic hardware linkage
> + */
> + .irq = ehci_irq,
> + .flags = HCD_USB2,
HCD_MEMORY | HCD_USB2,
... though that may not matter here.
> +
> + /*
> + * basic lifecycle operations
> + */
> + .reset = ehci_orion_setup,
> + .start = ehci_run,
> +#ifdef CONFIG_PM
> + .suspend = ehci_bus_suspend,
> + .resume = ehci_bus_resume,
> +#endif
> + .stop = ehci_stop,
> + .shutdown = ehci_shutdown,
> +
> + /*
> + * managing i/o requests and associated device resources
> + */
> + .urb_enqueue = ehci_urb_enqueue,
> + .urb_dequeue = ehci_urb_dequeue,
> + .endpoint_disable = ehci_endpoint_disable,
> +
> + /*
> + * scheduling support
> + */
> + .get_frame_number = ehci_get_frame,
> +
> + /*
> + * root hub support
> + */
> + .hub_status_data = ehci_hub_status_data,
> + .hub_control = ehci_hub_control,
> + .bus_suspend = ehci_bus_suspend,
> + .bus_resume = ehci_bus_resume,
> +};
> +
> +static int ehci_orion_drv_probe(struct platform_device *pdev)
> +{
> + if (usb_disabled())
> + return -ENODEV;
> +
> + return usb_hcd_orion_probe(&ehci_orion_hc_driver, pdev);
And I'd have *all* the probe() logic here; and all the remove()
logic below.
This particular split-it-in-two-parts idiom came from some
legacy code factoring, which AFAICT is no longer needed.
It'd be nice to see that idiom vanish, so things like probe
and remove logic don't look so obfuscated.
> +}
> +
> +static int ehci_orion_drv_remove(struct platform_device *pdev)
> +{
> + struct usb_hcd *hcd = platform_get_drvdata(pdev);
> +
> + usb_hcd_orion_remove(hcd, pdev);
> +
> + return 0;
> +}
> +
> +MODULE_ALIAS("orion-ehci");
> +
> +static struct platform_driver ehci_orion_driver = {
> + .probe = ehci_orion_drv_probe,
> + .remove = ehci_orion_drv_remove,
If this is any kind of normal platform bus arrangement, you
should mark the remove() method __exit and this pointer
should be wrapped as __exit_p().
> + .shutdown = usb_hcd_platform_shutdown,
> + .driver.name = "orion-ehci",
> +};
> --
> 1.5.3.6.2064.g2e22f-dirty
>
>
-
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
