Re: [PATCH V2] usb: gadget: bcm63xx UDC driver

[Sebastian Andrzej Siewior <sebastian@breakpoint.cc>]

On Mon, Aug 20, 2012 at 06:49:36PM -0700, Kevin Cernekee wrote:
> Driver for the "USB20D" / "USBD" block on BCM6328, BCM6368, BCM6816,
> BCM6362, BCM3383, and others.

looks good btw.

> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 51ab5fd..01efd9d 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -160,6 +160,19 @@ config USB_ATMEL_USBA
>  	  USBA is the integrated high-speed USB Device controller on
>  	  the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
>  
> +config USB_BCM63XX_UDC
> +	tristate "Broadcom BCM63xx Peripheral Controller"
> +	select USB_GADGET_DUALSPEED
> +	depends on BCM63XX
> +	help
> +	   Many Broadcom BCM63xx chipsets (such as the BCM6328) have a
> +	   high speed USB Device Port with support for four fixed endpoints
> +	   (plus endpoint zero).
> +
> +	   Say "y" to link the driver statically, or "m" to build a
> +	   dynamically linked module called "bcm63xx_udc" and force
> +	   all gadget drivers to also be dynamically linked.
please drop the "force all gadget drivers to also be dynamically linked" part.
We may want to change this one day :)

> +
>  config USB_FSL_USB2
>  	tristate "Freescale Highspeed USB DR Peripheral Controller"
>  	depends on FSL_SOC || ARCH_MXC
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 3fd8cd0..d84f923 100644

> diff --git a/drivers/usb/gadget/bcm63xx_udc.c b/drivers/usb/gadget/bcm63xx_udc.c
> new file mode 100644
> index 0000000..a44352b
> --- /dev/null
> +++ b/drivers/usb/gadget/bcm63xx_udc.c
...

> +static bool use_fullspeed;
> +module_param(use_fullspeed, bool, S_IRUGO);
> +MODULE_PARM_DESC(use_fullspeed, "true for fullspeed only");

How important is this option? Maybe this should become a generic option?

> +static int bcm63xx_udc_start(struct usb_gadget *gadget,
> +		struct usb_gadget_driver *driver)
> +{
> +	struct bcm63xx_udc *udc = gadget_to_udc(gadget);
> +	unsigned long flags;
> +
> +	if (!driver || driver->max_speed < USB_SPEED_HIGH ||
Hmm. But if you set use_fullspeed isn't this kinda legal?

> +static int __devinit bcm63xx_udc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct bcm63xx_usbd_platform_data *pd = dev->platform_data;
> +	struct bcm63xx_udc *udc;
> +	struct resource *res;
> +	int rc = -ENOMEM, i, irq;
> +
> +	udc = devm_kzalloc(dev, sizeof(*udc), GFP_KERNEL);
> +	if (!udc) {
> +		dev_err(dev, "cannot allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	platform_set_drvdata(pdev, udc);
> +	udc->dev = dev;
> +	udc->pd = pd;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "error finding USBD resource\n");
> +		return -ENXIO;
> +	}
> +	udc->usbd_regs = devm_request_and_ioremap(dev, res);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res) {
> +		dev_err(dev, "error finding IUDMA resource\n");
> +		return -ENXIO;
> +	}
> +	udc->iudma_regs = devm_request_and_ioremap(dev, res);
> +
> +	if (!udc->usbd_regs || !udc->iudma_regs) {
> +		dev_err(dev, "error requesting resources\n");
> +		return -ENXIO;
> +	}
> +
> +	spin_lock_init(&udc->lock);
> +	dev_set_name(&udc->gadget.dev, "gadget");
> +
> +	udc->gadget.ops = &bcm63xx_udc_ops;
> +	udc->gadget.name = dev_name(dev);
> +	udc->gadget.dev.parent = dev;
> +	udc->gadget.dev.release = bcm63xx_udc_gadget_release;
> +	udc->gadget.dev.dma_mask = dev->dma_mask;
> +
> +	if (!pd->use_fullspeed && !use_fullspeed)
so it is a good advice to have pd not set to NULL :)

Sebastian