Re: [PATCH] USB: Add support for Xilinx USB host controller

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Tue, 2009-09-15 at 16:10 -0600, Julie Zhu wrote:
> Add bus glue driver for Xilinx USB host controller. The controller can be
> configured as HS only or HS/FS hybrid. The driver uses the device tree file
> to configure the driver according to the setting in the hardware system.
> 
> This driver has been tested with usbtest using the NET2280 PCI card.
> 
> Signed-off-by: Julie Zhu <julie.zhu@xilinx.com>

Hi !

First, this is a very clean piece of code, thanks.

Just a few minor nits:

> static int ehci_xilinx_port_handed_over(struct usb_hcd *hcd, int portnum)
> +{
> +	dev_warn(hcd->self.controller, "port %d cannot be enabled\n", portnum);
> +	if (hcd->has_tt) {
> +		dev_warn(hcd->self.controller,
> +			"Maybe you have connected an LS device?\n");
> +
> +		dev_warn(hcd->self.controller,
> +			"We do not support LS devices\n");
> +	} else {
> +		dev_warn(hcd->self.controller,
> +			"Maybe your device is not an HS device?\n");
> +		dev_warn(hcd->self.controller,
> +			"The USB host controller does not support FS or "
> +			"LS devices\n");
> +		dev_warn(hcd->self.controller,
> +			"You can reconfigure the host controller to have "
> +			"FS support\n");
> +	}
> +
> +	return 0;
> +}

I'm not sure the final users would know what "FS", "LS" or "HS" mean
here, it might be worth being a -tad- more verbose :-)

 .../...

> +
> +/**
> + * ehci_hcd_xilinx_of_remove - shutdown hcd and release resources
> + * @op:		pointer to of_device structure that is to be removed
> + *
> + * Remove the hcd structure, and release resources that has been requested
> + * during probe.
> + */
> +static int ehci_hcd_xilinx_of_remove(struct of_device *op)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(&op->dev);
> +	dev_set_drvdata(&op->dev, NULL);
> +
> +	dev_dbg(&op->dev, "stopping XILINX-OF USB Controller\n");
> +
> +	usb_remove_hcd(hcd);
> +
> +	iounmap(hcd->regs);
> +	irq_dispose_mapping(hcd->irq);

You don't need to dispose of the irq mapping, and in fact, it could be
harmful if the interrupt is shared, as we don't refcount the mapping
users. Just remove the line above. The mapping doesn't really use
resources (well, it depends on your PIC but even then, it's minor) so
it's better, once a HW IRQ number has been associated to a linux IRQ
number, to keep that association for the lifetime of the kernel.

Cheers,
Ben.