On Tue, 15 Mar 2011, Matthieu CASTET wrote:
> Even if the usb gadget framework is limited to work with one driver, it
> could be useful to have a kernel build with more than driver.
> This allow to make generic kernel that work with different udc controller.
>
> The only blocker to do that is usb_gadget_register_driver
> and usb_gadget_unregister_driver function are declared in each driver.
>
> For avoiding that a redirection is done for these functions :
> At probe time the driver register them (usb_gadget_register and
> usb_gadget_unregister), and the generic usb_gadget_register_driver and
> usb_gadget_unregister_driver call these callback.
> We pass struct *usb_gadget in usb_gadget_register and usb_gadget_unregister
> for flexibility (we can latter do a more complex dispatcher).
>
> Signed-off-by: Matthieu CASTET <matthieu.castet@xxxxxxxxxx>
> Ack-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
Actually, there are a few things in this patch which should be
improved. See below...
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -68,7 +68,7 @@ obj-$(CONFIG_USB_OTG_UTILS) += usb/otg/
> obj-$(CONFIG_USB) += usb/
> obj-$(CONFIG_USB_MUSB_HDRC) += usb/musb/
> obj-$(CONFIG_PCI) += usb/
> -obj-$(CONFIG_USB_GADGET) += usb/gadget/
> +obj-y += usb/gadget/
Why was this changed? And why doesn't the spacing match the lines
above?
> obj-$(CONFIG_SERIO) += input/serio/
> obj-$(CONFIG_GAMEPORT) += input/gameport/
> obj-$(CONFIG_INPUT) += input/
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index d500996..d088bb0 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -559,6 +559,12 @@ config USB_CI13XXX_MSM
> default USB_GADGET
> select USB_GADGET_SELECTED
>
> +config USB_GADGET_MULTIUDC
> + boolean "multi USB Device Port"
> + select USB_GADGET_SELECTED
> + help
> + Allow to build more than one udc.
> +
> #
> # LAST -- dummy/emulated controller
> #
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 55f5e8a..5798697 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -29,6 +29,8 @@ obj-$(CONFIG_USB_PXA_U2O) += mv_udc.o
> mv_udc-y := mv_udc_core.o mv_udc_phy.o
> obj-$(CONFIG_USB_CI13XXX_MSM) += ci13xxx_msm.o
>
> +obj-$(CONFIG_USB_GADGET_MULTIUDC) += core_udc.o
> +
> #
> # USB gadget drivers
> #
> diff --git a/drivers/usb/gadget/core_udc.c b/drivers/usb/gadget/core_udc.c
> new file mode 100644
> index 0000000..8339883
> --- /dev/null
> +++ b/drivers/usb/gadget/core_udc.c
> @@ -0,0 +1,61 @@
> +/*
> + * Copyright (C) 2010 Matthieu CASTET <matthieu.castet@xxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; either version 2.1 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/errno.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/platform_device.h>
> +#include <linux/usb/ch9.h>
> +#include <linux/usb/gadget.h>
> +
> +static struct usb_gadget *usb_gadget_udc;
> +
> +int usb_gadget_register(struct usb_gadget *gadget)
This name is confusing because it doesn't mention "udc". Change it to
usb_gadget_register_udc_driver. And don't forget to change the name
wherever it occurs in comments as well as in the code!
> +{
> + if (!gadget->udc ||
> + !gadget->udc->probe_driver || !gadget->udc->unregister_driver)
> + return -EINVAL;
> +
> + if (usb_gadget_udc)
> + return -EBUSY;
> +
> + usb_gadget_udc = gadget;
In theory this should be protected by a mutex. But it probably doesn't
matter.
> +
> + return device_register(&gadget->dev);
You can't do this. If device_register() fails, you have to clean up by
calling put_device() -- but the caller has no way of knowing what to
do. Therefore you have to check the return code from device_register()
and clean up here before returning.
> +}
> +EXPORT_SYMBOL(usb_gadget_register);
> +
> +void usb_gadget_unregister(struct usb_gadget *gadget)
Change this name to usb_gadget_unregister_udc_driver.
> +{
> + if (!usb_gadget_udc || usb_gadget_udc != gadget)
> + return;
> +
> + usb_gadget_udc = NULL;
> +
> + device_unregister(&gadget->dev);
What happens if a gadget driver is currently registered for this
gadget? It will have no way to unregister, because
usb_gadget_unregister_driver() will return -ENODEV without doing
anything.
> +}
> +EXPORT_SYMBOL(usb_gadget_unregister);
> +
> +int usb_gadget_probe_driver(struct usb_gadget_driver *driver, int (*bind)(struct usb_gadget *))
Wrap this long line after the first parameter.
> +{
> + if (!usb_gadget_udc)
> + return -ENODEV;
> + return usb_gadget_udc->udc->probe_driver(driver, bind, usb_gadget_udc);
> +}
> +EXPORT_SYMBOL (usb_gadget_probe_driver);
> +
> +int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
> +{
> + if (!usb_gadget_udc)
> + return -ENODEV;
> + return usb_gadget_udc->udc->unregister_driver(driver, usb_gadget_udc);
> +}
> +EXPORT_SYMBOL (usb_gadget_unregister_driver);
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 006412c..5391c18 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -432,6 +432,33 @@ struct usb_gadget_ops {
> unsigned code, unsigned long param);
> };
>
> +#ifdef CONFIG_USB_GADGET_MULTIUDC
> +struct usb_gadget_driver;
> +/**
> + * struct usb_gadget_udc - driver for udc device
> + * @probe_driver:register a gadget driver
> + * @unregister_driver:unregister a gadget driver
> + *
> + * @see usb_gadget_register_driver and usb_gadget_unregister_driver
Do you really want the '@' before "see"? Is this a kerneldoc markup
I'm not familiar with?
Since this structure represents a udc device driver, it should be
called struct usb_gadget_udc_driver.
> + */
> +struct usb_gadget_udc {
> + int (*probe_driver) (struct usb_gadget_driver *driver, int (*bind)(struct usb_gadget *), struct usb_gadget *gadget);
> + int (*unregister_driver) (struct usb_gadget_driver *driver, struct usb_gadget *gadget);
Again, wrap these long lines.
> +};
> +
> +/**
> + * usb_gadget_register - register a udc driver
> + * @gadget:the driver being registered
> + */
> +int usb_gadget_register(struct usb_gadget *gadget);
> +
> +/**
> + * usb_gadget_unregister - unregister a udc driver
> + * @gadget:the driver being registered
> + */
> +void usb_gadget_unregister(struct usb_gadget *gadget);
> +#endif
> +
> /**
> * struct usb_gadget - represents a usb slave device
> * @ops: Function pointers used to access hardware-specific operations.
> @@ -487,6 +514,9 @@ struct usb_gadget {
> unsigned a_hnp_support:1;
> unsigned a_alt_hnp_support:1;
> const char *name;
> +#ifdef CONFIG_USB_GADGET_MULTIUDC
> + struct usb_gadget_udc *udc;
> +#endif
The spacing should agree with the surrounding lines.
> struct device dev;
> };
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
