All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] usb: add new usb gadget for ACM and mass storage
@ 2011-09-08 18:24 Klaus Schwarzkopf
  2011-09-08 19:11 ` Greg KH
                   ` (4 more replies)
  0 siblings, 5 replies; 32+ messages in thread
From: Klaus Schwarzkopf @ 2011-09-08 18:24 UTC (permalink / raw)
  To: balbi, gregkh; +Cc: linux-usb, linux-kernel, Klaus Schwarzkopf

This driver provides two functions in one configuration:
a mass storage, and a CDC ACM (serial port) link.
Heavily based on multi.c and cdc2.c

Signed-off-by: Klaus Schwarzkopf <schwarzkopf@sensortherm.de>
---
 drivers/usb/gadget/Kconfig  |   10 ++
 drivers/usb/gadget/Makefile |    2 +
 drivers/usb/gadget/acm_ms.c |  269 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/gadget.h  |    1 +
 4 files changed, 282 insertions(+), 0 deletions(-)
 create mode 100644 drivers/usb/gadget/acm_ms.c

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 029e288..2592ba7 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -959,6 +959,16 @@ config USB_G_NOKIA
 	  It's only really useful for N900 hardware. If you're building
 	  a kernel for N900, say Y or M here. If unsure, say N.
 
+config USB_G_ACM_MS
+	tristate "CDC Composite Device (ACM and mass storage)"
+	depends on BLOCK
+	help
+	  This driver provides two functions in one configuration:
+	  a mass storage, and a CDC ACM (serial port) link.
+
+	  Say "y" to link the driver statically, or "m" to build a
+	  dynamically linked module called "g_acm_ms".
+
 config USB_G_MULTI
 	tristate "Multifunction Composite Gadget (EXPERIMENTAL)"
 	depends on BLOCK && NET
diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index 4fe92b1..3876d8c 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -49,6 +49,7 @@ g_dbgp-y			:= dbgp.o
 g_nokia-y			:= nokia.o
 g_webcam-y			:= webcam.o
 g_ncm-y				:= ncm.o
+g_acm_ms-y			:= acm_ms.o
 
 obj-$(CONFIG_USB_ZERO)		+= g_zero.o
 obj-$(CONFIG_USB_AUDIO)		+= g_audio.o
@@ -67,3 +68,4 @@ obj-$(CONFIG_USB_G_MULTI)	+= g_multi.o
 obj-$(CONFIG_USB_G_NOKIA)	+= g_nokia.o
 obj-$(CONFIG_USB_G_WEBCAM)	+= g_webcam.o
 obj-$(CONFIG_USB_G_NCM)		+= g_ncm.o
+obj-$(CONFIG_USB_G_ACM_MS)	+= g_acm_ms.o
diff --git a/drivers/usb/gadget/acm_ms.c b/drivers/usb/gadget/acm_ms.c
new file mode 100644
index 0000000..64d0f79
--- /dev/null
+++ b/drivers/usb/gadget/acm_ms.c
@@ -0,0 +1,269 @@
+/*
+ * cdc2.c -- CDC Composite driver, with ACM and mass storage support
+ *
+ * Copyright (C) 2008 David Brownell
+ * Copyright (C) 2008 Nokia Corporation
+ * Author: David Brownell
+ * Modified: Klaus Schwarzkopf
+ *
+ * Heavily based on multi.c and cdc2.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/kernel.h>
+#include <linux/utsname.h>
+
+#include "u_serial.h"
+
+
+#define DRIVER_DESC		"CDC Composite Gadget (ACM + MS)"
+#define DRIVER_VERSION		"2011/09/08"
+
+/*-------------------------------------------------------------------------*/
+
+/* DO NOT REUSE THESE IDs with a protocol-incompatible driver!!  Ever!!
+ * Instead:  allocate your own, using normal USB-IF procedures.
+ */
+#define CDC_VENDOR_NUM	0x1d6b	/* Linux Foundation */
+#define CDC_PRODUCT_NUM	0x0106	/* CDC Composite: ACM + MS*/
+
+/*-------------------------------------------------------------------------*/
+
+/*
+ * Kbuild is not very cooperative with respect to linking separately
+ * compiled library objects into one module.  So for now we won't use
+ * separate compilation ... ensuring init/exit sections work to shrink
+ * the runtime footprint, and giving us at least some parts of what
+ * a "gcc --combine ... part1.c part2.c part3.c ... " build would.
+ */
+
+#include "composite.c"
+#include "usbstring.c"
+#include "config.c"
+#include "epautoconf.c"
+#include "u_serial.c"
+#include "f_acm.c"
+#include "f_mass_storage.c"
+
+/*-------------------------------------------------------------------------*/
+
+static struct usb_device_descriptor device_desc = {
+	.bLength =		sizeof device_desc,
+	.bDescriptorType =	USB_DT_DEVICE,
+
+	.bcdUSB =		cpu_to_le16(0x0200),
+
+	.bDeviceClass =		USB_CLASS_COMM,
+	.bDeviceSubClass =	0,
+	.bDeviceProtocol =	0,
+	/* .bMaxPacketSize0 = f(hardware) */
+
+	/* Vendor and product id can be overridden by module parameters.  */
+	.idVendor =		cpu_to_le16(CDC_VENDOR_NUM),
+	.idProduct =		cpu_to_le16(CDC_PRODUCT_NUM),
+	/* .bcdDevice = f(hardware) */
+	/* .iManufacturer = DYNAMIC */
+	/* .iProduct = DYNAMIC */
+	/* NO SERIAL NUMBER */
+	.bNumConfigurations =	1,
+};
+
+static struct usb_otg_descriptor otg_descriptor = {
+	.bLength =		sizeof otg_descriptor,
+	.bDescriptorType =	USB_DT_OTG,
+
+	/* REVISIT SRP-only hardware is possible, although
+	 * it would not be called "OTG" ...
+	 */
+	.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
+};
+
+static const struct usb_descriptor_header *otg_desc[] = {
+	(struct usb_descriptor_header *) &otg_descriptor,
+	NULL,
+};
+
+
+/* string IDs are assigned dynamically */
+
+#define STRING_MANUFACTURER_IDX		0
+#define STRING_PRODUCT_IDX		1
+
+static char manufacturer[50];
+
+static struct usb_string strings_dev[] = {
+	[STRING_MANUFACTURER_IDX].s = manufacturer,
+	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
+	{  } /* end of list */
+};
+
+static struct usb_gadget_strings stringtab_dev = {
+	.language	= 0x0409,	/* en-us */
+	.strings	= strings_dev,
+};
+
+static struct usb_gadget_strings *dev_strings[] = {
+	&stringtab_dev,
+	NULL,
+};
+
+/****************************** Configurations ******************************/
+
+static struct fsg_module_parameters fsg_mod_data = { .stall = 1 };
+FSG_MODULE_PARAMETERS(/* no prefix */, fsg_mod_data);
+
+static struct fsg_common fsg_common;
+
+/*-------------------------------------------------------------------------*/
+
+/*
+ * We _always_ have both CDC ECM and CDC ACM functions.
+ */
+static int __init cdc_do_config(struct usb_configuration *c)
+{
+	int	status;
+
+	if (gadget_is_otg(c->cdev->gadget)) {
+		c->descriptors = otg_desc;
+		c->bmAttributes |= USB_CONFIG_ATT_WAKEUP;
+	}
+
+
+	status = acm_bind_config(c, 0);
+	if (status < 0)
+		return status;
+
+	status = fsg_bind_config(c->cdev, c, &fsg_common);
+	if (status < 0)
+		return status;
+
+	return 0;
+}
+
+static struct usb_configuration cdc_config_driver = {
+	.label			= DRIVER_DESC,
+	.bConfigurationValue	= 1,
+	/* .iConfiguration = DYNAMIC */
+	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
+};
+
+/*-------------------------------------------------------------------------*/
+
+static int __init cdc_bind(struct usb_composite_dev *cdev)
+{
+	int			gcnum;
+	struct usb_gadget	*gadget = cdev->gadget;
+	int			status;
+
+
+	/* set up serial link layer */
+	status = gserial_setup(cdev->gadget, 1);
+	if (status < 0)
+		return status;
+
+	/* set up mass storage function */
+	{
+		void *retp;
+		retp = fsg_common_from_params(&fsg_common, cdev, &fsg_mod_data);
+		if (IS_ERR(retp)) {
+			status = PTR_ERR(retp);
+			goto fail0;
+		}
+	}
+
+	gcnum = usb_gadget_controller_number(gadget);
+	if (gcnum >= 0)
+		device_desc.bcdDevice = cpu_to_le16(0x0300 | gcnum);
+	else {
+		/* We assume that can_support_ecm() tells the truth;
+		 * but if the controller isn't recognized at all then
+		 * that assumption is a bit more likely to be wrong.
+		 */
+		WARNING(cdev, "controller '%s' not recognized; trying %s\n",
+				gadget->name,
+				cdc_config_driver.label);
+		device_desc.bcdDevice =
+			cpu_to_le16(0x0300 | 0x0099);
+	}
+
+
+	/* Allocate string descriptor numbers ... note that string
+	 * contents can be overridden by the composite_dev glue.
+	 */
+
+	/* device descriptor strings: manufacturer, product */
+	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
+		init_utsname()->sysname, init_utsname()->release,
+		gadget->name);
+	status = usb_string_id(cdev);
+	if (status < 0)
+		goto fail1;
+	strings_dev[STRING_MANUFACTURER_IDX].id = status;
+	device_desc.iManufacturer = status;
+
+	status = usb_string_id(cdev);
+	if (status < 0)
+		goto fail1;
+	strings_dev[STRING_PRODUCT_IDX].id = status;
+	device_desc.iProduct = status;
+
+	/* register our configuration */
+	status = usb_add_config(cdev, &cdc_config_driver, cdc_do_config);
+	if (status < 0)
+		goto fail1;
+
+	dev_info(&gadget->dev, "%s, version: " DRIVER_VERSION "\n",
+			DRIVER_DESC);
+	fsg_common_put(&fsg_common);
+	return 0;
+
+	/* error recovery */
+fail1:
+	fsg_common_put(&fsg_common);
+fail0:
+	gserial_cleanup();
+	return status;
+}
+
+static int __exit cdc_unbind(struct usb_composite_dev *cdev)
+{
+	gserial_cleanup();
+
+	return 0;
+}
+
+static struct usb_composite_driver cdc_driver = {
+	.name		= "g_acm_ms",
+	.dev		= &device_desc,
+	.strings	= dev_strings,
+	.unbind		= __exit_p(cdc_unbind),
+};
+
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_AUTHOR("Klaus Schwarzkopf");
+MODULE_LICENSE("GPL");
+
+static int __init init(void)
+{
+	return usb_composite_probe(&cdc_driver, cdc_bind);
+}
+module_init(init);
+
+static void __exit cleanup(void)
+{
+	usb_composite_unregister(&cdc_driver);
+}
+module_exit(cleanup);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index dd1571d..f623f3d 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -15,6 +15,7 @@
 #ifndef __LINUX_USB_GADGET_H
 #define __LINUX_USB_GADGET_H
 
+#include <linux/device.h>
 #include <linux/slab.h>
 
 struct usb_ep;
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-09-08 18:24 [PATCH] usb: add new usb gadget for ACM and mass storage Klaus Schwarzkopf
@ 2011-09-08 19:11 ` Greg KH
  2011-09-09  8:37   ` Klaus Schwarzkopf
  2011-09-09 10:20   ` Michal Nazarewicz
  2011-10-06 12:08 ` Felipe Balbi
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 32+ messages in thread
From: Greg KH @ 2011-09-08 19:11 UTC (permalink / raw)
  To: Klaus Schwarzkopf; +Cc: balbi, linux-usb, linux-kernel

On Thu, Sep 08, 2011 at 08:24:47PM +0200, Klaus Schwarzkopf wrote:
> This driver provides two functions in one configuration:
> a mass storage, and a CDC ACM (serial port) link.
> Heavily based on multi.c and cdc2.c

I thought the "composite" framework make it so that drivers like this
were no longer needed.  Or am I mistaken somehow?

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.

Do you really mean "any later version"?

> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA

These two paragraphs are not needed, and the address is a very bad idea
to include in any file as it changes over time and I don't think you
want to keep up with the address location of the FSF for the next 40+
years.

As an example of this, I am starting to get patches from the FSF to fix
up old addresses, 10 years after moving their office...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-09-08 19:11 ` Greg KH
@ 2011-09-09  8:37   ` Klaus Schwarzkopf
  2011-09-09 10:30     ` Michal Nazarewicz
  2011-09-09 18:45     ` Greg KH
  2011-09-09 10:20   ` Michal Nazarewicz
  1 sibling, 2 replies; 32+ messages in thread
From: Klaus Schwarzkopf @ 2011-09-09  8:37 UTC (permalink / raw)
  To: Greg KH; +Cc: balbi, linux-usb, linux-kernel

Am 08.09.2011 21:11, schrieb Greg KH:
> On Thu, Sep 08, 2011 at 08:24:47PM +0200, Klaus Schwarzkopf wrote:
>> This driver provides two functions in one configuration:
>> a mass storage, and a CDC ACM (serial port) link.
>> Heavily based on multi.c and cdc2.c
>
> I thought the "composite" framework make it so that drivers like this
> were no longer needed.  Or am I mistaken somehow?
>


The "composite" framework enabled drivers with two or more functions. 
For example the cdc2.c driver have support for ECM and ACM.


>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>
> Do you really mean "any later version"?


The acm_ms.c file is heavly based on the cdc2.c file with this 
paragraph. Can i change this without breaking the license?


>
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>
> These two paragraphs are not needed, and the address is a very bad idea
> to include in any file as it changes over time and I don't think you
> want to keep up with the address location of the FSF for the next 40+
> years.
>
> As an example of this, I am starting to get patches from the FSF to fix
> up old addresses, 10 years after moving their office...
>

OK, i remove this lines.

Can i remove this lines in all files of the directory drivers/usb/gadget?


+#define CDC_VENDOR_NUM	0x1d6b	/* Linux Foundation */
+#define CDC_PRODUCT_NUM	0x0106	/* CDC Composite: ACM + MS*/

I used the next free device ID from http://www.linux-usb.org/usb.ids, is 
this ok?

Anything else to do?


Regards,

Klaus

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-09-08 19:11 ` Greg KH
  2011-09-09  8:37   ` Klaus Schwarzkopf
@ 2011-09-09 10:20   ` Michal Nazarewicz
  2011-09-09 18:43     ` Greg KH
  1 sibling, 1 reply; 32+ messages in thread
From: Michal Nazarewicz @ 2011-09-09 10:20 UTC (permalink / raw)
  To: Klaus Schwarzkopf, Greg KH; +Cc: balbi, linux-usb, linux-kernel

> On Thu, Sep 08, 2011 at 08:24:47PM +0200, Klaus Schwarzkopf wrote:
>> This driver provides two functions in one configuration:
>> a mass storage, and a CDC ACM (serial port) link.
>> Heavily based on multi.c and cdc2.c

On Thu, 08 Sep 2011 21:11:46 +0200, Greg KH <gregkh@suse.de> wrote:
> I thought the "composite" framework make it so that drivers like this
> were no longer needed.  Or am I mistaken somehow?

No.  Composite framework only makes it relatively simple to create such
gadgets, but one still needs some glue that binds all the functions into
a single gadget.

This seems like both a blessing and a curse (which I haven't realise when
I was creating g_multi) since with n functions implemented one can easily
came up with 2^n gadgets.

Android went half-step forward and in it you can create a gadget by
specifying list of named functions it is supposed to have.  I'm saying
“half-step” because they have no support for configurations for instance.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-09-09  8:37   ` Klaus Schwarzkopf
@ 2011-09-09 10:30     ` Michal Nazarewicz
  2011-09-09 18:45     ` Greg KH
  1 sibling, 0 replies; 32+ messages in thread
From: Michal Nazarewicz @ 2011-09-09 10:30 UTC (permalink / raw)
  To: Greg KH, Klaus Schwarzkopf; +Cc: balbi, linux-usb, linux-kernel

> Am 08.09.2011 21:11, schrieb Greg KH:
>> Do you really mean "any later version"?

On Fri, 09 Sep 2011 10:37:06 +0200, Klaus Schwarzkopf  
<schwarzkopf@sensortherm.de> wrote:
> The acm_ms.c file is heavly based on the cdc2.c file with this  
> paragraph. Can i change this without breaking the license?

Probably yes, since you're adding your copyright and you can say that
your changes are GPLv2 only and therefore in practice the whole file
becomes GPLv2 only.

If anyone cares about my interpretation and opinion, Greg did not ask
you to change it but rather whether you really mean “any later version”. ;)

>> These two paragraphs are not needed, and the address is a very bad idea
>> to include in any file as it changes over time and I don't think you
>> want to keep up with the address location of the FSF for the next 40+
>> years.

> OK, i remove this lines.
>
> Can i remove this lines in all files of the directory drivers/usb/gadget?

If you do, submit it as separate patch. :)

> +#define CDC_VENDOR_NUM	0x1d6b	/* Linux Foundation */
> +#define CDC_PRODUCT_NUM	0x0106	/* CDC Composite: ACM + MS*/
>
> I used the next free device ID from http://www.linux-usb.org/usb.ids, is  
> this ok?

IIRC, Greg is maintaining lists of IDs, so you need to ask him nicely for
one. ;)

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-09-09 10:20   ` Michal Nazarewicz
@ 2011-09-09 18:43     ` Greg KH
  2011-09-16 17:18       ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 32+ messages in thread
From: Greg KH @ 2011-09-09 18:43 UTC (permalink / raw)
  To: Michal Nazarewicz; +Cc: Klaus Schwarzkopf, balbi, linux-usb, linux-kernel

On Fri, Sep 09, 2011 at 12:20:53PM +0200, Michal Nazarewicz wrote:
> >On Thu, Sep 08, 2011 at 08:24:47PM +0200, Klaus Schwarzkopf wrote:
> >>This driver provides two functions in one configuration:
> >>a mass storage, and a CDC ACM (serial port) link.
> >>Heavily based on multi.c and cdc2.c
> 
> On Thu, 08 Sep 2011 21:11:46 +0200, Greg KH <gregkh@suse.de> wrote:
> >I thought the "composite" framework make it so that drivers like this
> >were no longer needed.  Or am I mistaken somehow?
> 
> No.  Composite framework only makes it relatively simple to create such
> gadgets, but one still needs some glue that binds all the functions into
> a single gadget.

Ah, so this is that glue?

> This seems like both a blessing and a curse (which I haven't realise when
> I was creating g_multi) since with n functions implemented one can easily
> came up with 2^n gadgets.

So we can expect more patches like this adding all of the possible
permutations of the different gadget devices?

If so, ick, I thought that the work that was done was to prevent this
from happening, oh well :(

greg k-h

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-09-09  8:37   ` Klaus Schwarzkopf
  2011-09-09 10:30     ` Michal Nazarewicz
@ 2011-09-09 18:45     ` Greg KH
  1 sibling, 0 replies; 32+ messages in thread
From: Greg KH @ 2011-09-09 18:45 UTC (permalink / raw)
  To: Klaus Schwarzkopf; +Cc: balbi, linux-usb, linux-kernel

On Fri, Sep 09, 2011 at 10:37:06AM +0200, Klaus Schwarzkopf wrote:
> Am 08.09.2011 21:11, schrieb Greg KH:
> >On Thu, Sep 08, 2011 at 08:24:47PM +0200, Klaus Schwarzkopf wrote:
> >>This driver provides two functions in one configuration:
> >>a mass storage, and a CDC ACM (serial port) link.
> >>Heavily based on multi.c and cdc2.c
> >
> >I thought the "composite" framework make it so that drivers like this
> >were no longer needed.  Or am I mistaken somehow?
> >
> 
> 
> The "composite" framework enabled drivers with two or more
> functions. For example the cdc2.c driver have support for ECM and
> ACM.
> 
> 
> >>+ * This program is free software; you can redistribute it and/or modify
> >>+ * it under the terms of the GNU General Public License as published by
> >>+ * the Free Software Foundation; either version 2 of the License, or
> >>+ * (at your option) any later version.
> >
> >Do you really mean "any later version"?
> 
> 
> The acm_ms.c file is heavly based on the cdc2.c file with this
> paragraph. Can i change this without breaking the license?

No, just wanted to make sure that you, and your company (if any) was
positive that this is what you want to do here.

> >>+ * This program is distributed in the hope that it will be useful,
> >>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>+ * GNU General Public License for more details.
> >>+ *
> >>+ * You should have received a copy of the GNU General Public License
> >>+ * along with this program; if not, write to the Free Software
> >>+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> >
> >These two paragraphs are not needed, and the address is a very bad idea
> >to include in any file as it changes over time and I don't think you
> >want to keep up with the address location of the FSF for the next 40+
> >years.
> >
> >As an example of this, I am starting to get patches from the FSF to fix
> >up old addresses, 10 years after moving their office...
> >
> 
> OK, i remove this lines.
> 
> Can i remove this lines in all files of the directory drivers/usb/gadget?

I see you just did, thanks so much :)

> +#define CDC_VENDOR_NUM	0x1d6b	/* Linux Foundation */
> +#define CDC_PRODUCT_NUM	0x0106	/* CDC Composite: ACM + MS*/
> 
> I used the next free device ID from
> http://www.linux-usb.org/usb.ids, is this ok?

No it isn't, you need to ask me to assign you an id.  I'll do that when
I accept the patch for merging into the kernel, and not before.

I'd like to get some of the gadget driver developers to sign off on this
patch before accepting it...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-09-09 18:43     ` Greg KH
@ 2011-09-16 17:18       ` Sebastian Andrzej Siewior
  2011-09-16 17:25         ` Greg KH
                           ` (3 more replies)
  0 siblings, 4 replies; 32+ messages in thread
From: Sebastian Andrzej Siewior @ 2011-09-16 17:18 UTC (permalink / raw)
  To: Greg KH
  Cc: Michal Nazarewicz, Klaus Schwarzkopf, balbi, linux-usb, linux-kernel

* Greg KH | 2011-09-09 11:43:54 [-0700]:

>So we can expect more patches like this adding all of the possible
>permutations of the different gadget devices?
>
>If so, ick, I thought that the work that was done was to prevent this
>from happening, oh well :(

Would it be possible to come up with a gadget-hub driver which can have
multiple gadget attached? This should get rid of this kind of gadget
drivers, right?

>greg k-h

Sebastian

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-09-16 17:18       ` Sebastian Andrzej Siewior
@ 2011-09-16 17:25         ` Greg KH
  2011-09-16 17:30         ` Michal Nazarewicz
                           ` (2 subsequent siblings)
  3 siblings, 0 replies; 32+ messages in thread
From: Greg KH @ 2011-09-16 17:25 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Michal Nazarewicz, Klaus Schwarzkopf, balbi, linux-usb, linux-kernel

On Fri, Sep 16, 2011 at 07:18:25PM +0200, Sebastian Andrzej Siewior wrote:
> * Greg KH | 2011-09-09 11:43:54 [-0700]:
> 
> >So we can expect more patches like this adding all of the possible
> >permutations of the different gadget devices?
> >
> >If so, ick, I thought that the work that was done was to prevent this
> >from happening, oh well :(
> 
> Would it be possible to come up with a gadget-hub driver which can have
> multiple gadget attached? This should get rid of this kind of gadget
> drivers, right?

I don't know, possibly, someone who knows the code better than I would
be the best to determine this.

greg k-h

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-09-16 17:18       ` Sebastian Andrzej Siewior
  2011-09-16 17:25         ` Greg KH
@ 2011-09-16 17:30         ` Michal Nazarewicz
  2011-09-16 18:22         ` Steve Calfee
  2011-09-16 21:22         ` Alan Stern
  3 siblings, 0 replies; 32+ messages in thread
From: Michal Nazarewicz @ 2011-09-16 17:30 UTC (permalink / raw)
  To: Greg KH, Sebastian Andrzej Siewior
  Cc: Klaus Schwarzkopf, balbi, linux-usb, linux-kernel

On Fri, 16 Sep 2011 19:18:25 +0200, Sebastian Andrzej Siewior  
<bigeasy@linutronix.de> wrote:

> * Greg KH | 2011-09-09 11:43:54 [-0700]:
>
>> So we can expect more patches like this adding all of the possible
>> permutations of the different gadget devices?
>>
>> If so, ick, I thought that the work that was done was to prevent this
>> from happening, oh well :(
>
> Would it be possible to come up with a gadget-hub driver which can have
> multiple gadget attached? This should get rid of this kind of gadget
> drivers, right?

Yes, I keep thinking about such a thing for years now...

Instead of a “gadget HUB” which would have to do a lot of translation of
headers and requests, I was thinking more of making USB composite functions
even more structured (ie. adding callback for per-gadget setup,
per-configuration set-up, etc.) and then creating a composite layer which
would be feed with a trivial structure describing which functions are to
be included in which configurations.

Like I've said before, that's almost what Android does, expect it does
not support many configurations.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-09-16 17:18       ` Sebastian Andrzej Siewior
  2011-09-16 17:25         ` Greg KH
  2011-09-16 17:30         ` Michal Nazarewicz
@ 2011-09-16 18:22         ` Steve Calfee
  2011-09-16 18:55           ` Sebastian Andrzej Siewior
  2011-09-16 21:22         ` Alan Stern
  3 siblings, 1 reply; 32+ messages in thread
From: Steve Calfee @ 2011-09-16 18:22 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Greg KH, Michal Nazarewicz, Klaus Schwarzkopf, balbi, linux-usb,
	linux-kernel

On Fri, Sep 16, 2011 at 10:18 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> Would it be possible to come up with a gadget-hub driver which can have
> multiple gadget attached? This should get rid of this kind of gadget
> drivers, right?
>
> Sebastian
> --

Hi,

To literally make a gadget that can emulate a hub (where it could
present port 1 as function x, port 2 as function y etc) would require
special hardware. The (bus speed) translations and (ack/nak) timings
are speced very tightly for hubs (see chapter 11 of the spec). Each
function would have its own usb bus address - otherwise one gadget
could not nak while another transfered data. Each bus address would
have to meet the spec in responding to all signaling.

Think about how the individual functions would announce themselves to
the host - when the port is enabled the available/speed pull ups would
have to be set, but other ports have to be in their old state.
Reset/Suspend signaling would have to be handled only by the selected
function.

So while it would make multiple function gadgets somewhat easier I
don't see how it is possible, do composite and/or compound gadgets
instead.

Regards, Steve

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-09-16 18:22         ` Steve Calfee
@ 2011-09-16 18:55           ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 32+ messages in thread
From: Sebastian Andrzej Siewior @ 2011-09-16 18:55 UTC (permalink / raw)
  To: Steve Calfee
  Cc: Greg KH, Michal Nazarewicz, Klaus Schwarzkopf, balbi, linux-usb,
	linux-kernel

On 09/16/2011 08:22 PM, Steve Calfee wrote:
> Hi,
Hi Steve,

> To literally make a gadget that can emulate a hub (where it could
> present port 1 as function x, port 2 as function y etc) would require
> special hardware. The (bus speed) translations and (ack/nak) timings
> are speced very tightly for hubs (see chapter 11 of the spec). Each
> function would have its own usb bus address - otherwise one gadget
> could not nak while another transfered data. Each bus address would
> have to meet the spec in responding to all signaling.

That is something I haven't figured out. So the plan was to expose each
gadget as an individual port with its own descriptors. The device
number is indeed a problem that can be hardly solved. The Host sets the
address of the device it talks to and the hub routes it properly. This
is someting we can't emulate.
Hmm. So it boils down to an efficient way of providing multiple
interface descriptors without adding a lot of code each time?

> Regards, Steve
Sebastian

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-09-16 17:18       ` Sebastian Andrzej Siewior
                           ` (2 preceding siblings ...)
  2011-09-16 18:22         ` Steve Calfee
@ 2011-09-16 21:22         ` Alan Stern
  3 siblings, 0 replies; 32+ messages in thread
From: Alan Stern @ 2011-09-16 21:22 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Greg KH, Michal Nazarewicz, Klaus Schwarzkopf, balbi, linux-usb,
	linux-kernel

On Fri, 16 Sep 2011, Sebastian Andrzej Siewior wrote:

> * Greg KH | 2011-09-09 11:43:54 [-0700]:
> 
> >So we can expect more patches like this adding all of the possible
> >permutations of the different gadget devices?
> >
> >If so, ick, I thought that the work that was done was to prevent this
> >from happening, oh well :(
> 
> Would it be possible to come up with a gadget-hub driver which can have
> multiple gadget attached? This should get rid of this kind of gadget
> drivers, right?

Instead of implementing a hub, it might be possible to redo the 
composite driver in such a way that the individual function drivers are 
added as separate modules at runtime, rather than all bound together at 
compile time.

The details would be tricky, but I think it could be done.  One 
important point is that somehow you have to inform the composite core 
when all the function drivers have been loaded and initialized -- 
otherwise there's no way to tell when it's safe to enable the pull-ups 
and connect to the host.

Alan Stern


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-09-08 18:24 [PATCH] usb: add new usb gadget for ACM and mass storage Klaus Schwarzkopf
  2011-09-08 19:11 ` Greg KH
@ 2011-10-06 12:08 ` Felipe Balbi
  2011-10-07  8:23   ` Klaus Schwarzkopf
  2011-10-07  8:16 ` [PATCH v2] " Klaus Schwarzkopf
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 32+ messages in thread
From: Felipe Balbi @ 2011-10-06 12:08 UTC (permalink / raw)
  To: Klaus Schwarzkopf; +Cc: balbi, gregkh, linux-usb, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 11222 bytes --]

Hi,

a few style issues below.

On Thu, Sep 08, 2011 at 08:24:47PM +0200, Klaus Schwarzkopf wrote:
> This driver provides two functions in one configuration:
> a mass storage, and a CDC ACM (serial port) link.
> Heavily based on multi.c and cdc2.c
> 
> Signed-off-by: Klaus Schwarzkopf <schwarzkopf@sensortherm.de>
> ---
>  drivers/usb/gadget/Kconfig  |   10 ++
>  drivers/usb/gadget/Makefile |    2 +
>  drivers/usb/gadget/acm_ms.c |  269 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/gadget.h  |    1 +
>  4 files changed, 282 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/usb/gadget/acm_ms.c
> 
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 029e288..2592ba7 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -959,6 +959,16 @@ config USB_G_NOKIA
>  	  It's only really useful for N900 hardware. If you're building
>  	  a kernel for N900, say Y or M here. If unsure, say N.
>  
> +config USB_G_ACM_MS
> +	tristate "CDC Composite Device (ACM and mass storage)"
> +	depends on BLOCK
> +	help
> +	  This driver provides two functions in one configuration:
> +	  a mass storage, and a CDC ACM (serial port) link.
> +
> +	  Say "y" to link the driver statically, or "m" to build a
> +	  dynamically linked module called "g_acm_ms".
> +
>  config USB_G_MULTI
>  	tristate "Multifunction Composite Gadget (EXPERIMENTAL)"
>  	depends on BLOCK && NET
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 4fe92b1..3876d8c 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -49,6 +49,7 @@ g_dbgp-y			:= dbgp.o
>  g_nokia-y			:= nokia.o
>  g_webcam-y			:= webcam.o
>  g_ncm-y				:= ncm.o
> +g_acm_ms-y			:= acm_ms.o
>  
>  obj-$(CONFIG_USB_ZERO)		+= g_zero.o
>  obj-$(CONFIG_USB_AUDIO)		+= g_audio.o
> @@ -67,3 +68,4 @@ obj-$(CONFIG_USB_G_MULTI)	+= g_multi.o
>  obj-$(CONFIG_USB_G_NOKIA)	+= g_nokia.o
>  obj-$(CONFIG_USB_G_WEBCAM)	+= g_webcam.o
>  obj-$(CONFIG_USB_G_NCM)		+= g_ncm.o
> +obj-$(CONFIG_USB_G_ACM_MS)	+= g_acm_ms.o
> diff --git a/drivers/usb/gadget/acm_ms.c b/drivers/usb/gadget/acm_ms.c
> new file mode 100644
> index 0000000..64d0f79
> --- /dev/null
> +++ b/drivers/usb/gadget/acm_ms.c
> @@ -0,0 +1,269 @@
> +/*
> + * cdc2.c -- CDC Composite driver, with ACM and mass storage support
> + *
> + * Copyright (C) 2008 David Brownell
> + * Copyright (C) 2008 Nokia Corporation
> + * Author: David Brownell
> + * Modified: Klaus Schwarzkopf
> + *
> + * Heavily based on multi.c and cdc2.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA

this paragraph is useless.

> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/utsname.h>
> +
> +#include "u_serial.h"
> +
> +

one blank line only.

> +#define DRIVER_DESC		"CDC Composite Gadget (ACM + MS)"
> +#define DRIVER_VERSION		"2011/09/08"
> +
> +/*-------------------------------------------------------------------------*/
> +
> +/* DO NOT REUSE THESE IDs with a protocol-incompatible driver!!  Ever!!
> + * Instead:  allocate your own, using normal USB-IF procedures.
> + */

wrong comment style.

> +#define CDC_VENDOR_NUM	0x1d6b	/* Linux Foundation */
> +#define CDC_PRODUCT_NUM	0x0106	/* CDC Composite: ACM + MS*/

did you ask Greg for this ?

> +/*-------------------------------------------------------------------------*/
> +
> +/*
> + * Kbuild is not very cooperative with respect to linking separately
> + * compiled library objects into one module.  So for now we won't use
> + * separate compilation ... ensuring init/exit sections work to shrink
> + * the runtime footprint, and giving us at least some parts of what
> + * a "gcc --combine ... part1.c part2.c part3.c ... " build would.
> + */
> +
> +#include "composite.c"
> +#include "usbstring.c"
> +#include "config.c"
> +#include "epautoconf.c"
> +#include "u_serial.c"
> +#include "f_acm.c"
> +#include "f_mass_storage.c"
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static struct usb_device_descriptor device_desc = {
> +	.bLength =		sizeof device_desc,
> +	.bDescriptorType =	USB_DT_DEVICE,
> +
> +	.bcdUSB =		cpu_to_le16(0x0200),
> +
> +	.bDeviceClass =		USB_CLASS_COMM,
> +	.bDeviceSubClass =	0,
> +	.bDeviceProtocol =	0,
> +	/* .bMaxPacketSize0 = f(hardware) */
> +
> +	/* Vendor and product id can be overridden by module parameters.  */
> +	.idVendor =		cpu_to_le16(CDC_VENDOR_NUM),
> +	.idProduct =		cpu_to_le16(CDC_PRODUCT_NUM),
> +	/* .bcdDevice = f(hardware) */
> +	/* .iManufacturer = DYNAMIC */
> +	/* .iProduct = DYNAMIC */
> +	/* NO SERIAL NUMBER */

why not ? If someone wants to make a product out of this, they will need
a serial number.

> +	.bNumConfigurations =	1,
> +};
> +
> +static struct usb_otg_descriptor otg_descriptor = {
> +	.bLength =		sizeof otg_descriptor,
> +	.bDescriptorType =	USB_DT_OTG,
> +
> +	/* REVISIT SRP-only hardware is possible, although
> +	 * it would not be called "OTG" ...
> +	 */

comment style is wrong.

> +	.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
> +};
> +
> +static const struct usb_descriptor_header *otg_desc[] = {
> +	(struct usb_descriptor_header *) &otg_descriptor,
> +	NULL,
> +};
> +
> +
> +/* string IDs are assigned dynamically */
> +
> +#define STRING_MANUFACTURER_IDX		0
> +#define STRING_PRODUCT_IDX		1
> +
> +static char manufacturer[50];
> +
> +static struct usb_string strings_dev[] = {
> +	[STRING_MANUFACTURER_IDX].s = manufacturer,
> +	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
> +	{  } /* end of list */
> +};
> +
> +static struct usb_gadget_strings stringtab_dev = {
> +	.language	= 0x0409,	/* en-us */
> +	.strings	= strings_dev,
> +};
> +
> +static struct usb_gadget_strings *dev_strings[] = {
> +	&stringtab_dev,
> +	NULL,
> +};
> +
> +/****************************** Configurations ******************************/
> +
> +static struct fsg_module_parameters fsg_mod_data = { .stall = 1 };
> +FSG_MODULE_PARAMETERS(/* no prefix */, fsg_mod_data);
> +
> +static struct fsg_common fsg_common;
> +
> +/*-------------------------------------------------------------------------*/
> +
> +/*
> + * We _always_ have both CDC ECM and CDC ACM functions.
> + */

comment doesn't match code.

> +static int __init cdc_do_config(struct usb_configuration *c)
> +{
> +	int	status;
> +
> +	if (gadget_is_otg(c->cdev->gadget)) {
> +		c->descriptors = otg_desc;
> +		c->bmAttributes |= USB_CONFIG_ATT_WAKEUP;
> +	}
> +
> +
> +	status = acm_bind_config(c, 0);
> +	if (status < 0)
> +		return status;
> +
> +	status = fsg_bind_config(c->cdev, c, &fsg_common);
> +	if (status < 0)
> +		return status;
> +
> +	return 0;
> +}
> +
> +static struct usb_configuration cdc_config_driver = {
> +	.label			= DRIVER_DESC,
> +	.bConfigurationValue	= 1,
> +	/* .iConfiguration = DYNAMIC */
> +	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
> +};
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static int __init cdc_bind(struct usb_composite_dev *cdev)
> +{
> +	int			gcnum;
> +	struct usb_gadget	*gadget = cdev->gadget;
> +	int			status;
> +
> +
> +	/* set up serial link layer */
> +	status = gserial_setup(cdev->gadget, 1);
> +	if (status < 0)
> +		return status;
> +
> +	/* set up mass storage function */
> +	{
> +		void *retp;
> +		retp = fsg_common_from_params(&fsg_common, cdev, &fsg_mod_data);
> +		if (IS_ERR(retp)) {
> +			status = PTR_ERR(retp);
> +			goto fail0;
> +		}
> +	}

you can define void *retp with the other local variables and remove the
braces.

> +	gcnum = usb_gadget_controller_number(gadget);
> +	if (gcnum >= 0)
> +		device_desc.bcdDevice = cpu_to_le16(0x0300 | gcnum);
> +	else {
> +		/* We assume that can_support_ecm() tells the truth;
> +		 * but if the controller isn't recognized at all then
> +		 * that assumption is a bit more likely to be wrong.
> +		 */
> +		WARNING(cdev, "controller '%s' not recognized; trying %s\n",
> +				gadget->name,
> +				cdc_config_driver.label);
> +		device_desc.bcdDevice =
> +			cpu_to_le16(0x0300 | 0x0099);
> +	}
> +
> +

one blank line only. Also, put braces on both branches.

> +	/* Allocate string descriptor numbers ... note that string
> +	 * contents can be overridden by the composite_dev glue.
> +	 */

wrong comment style

> +
> +	/* device descriptor strings: manufacturer, product */
> +	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
> +		init_utsname()->sysname, init_utsname()->release,
> +		gadget->name);
> +	status = usb_string_id(cdev);
> +	if (status < 0)
> +		goto fail1;
> +	strings_dev[STRING_MANUFACTURER_IDX].id = status;
> +	device_desc.iManufacturer = status;
> +
> +	status = usb_string_id(cdev);
> +	if (status < 0)
> +		goto fail1;
> +	strings_dev[STRING_PRODUCT_IDX].id = status;
> +	device_desc.iProduct = status;
> +
> +	/* register our configuration */
> +	status = usb_add_config(cdev, &cdc_config_driver, cdc_do_config);
> +	if (status < 0)
> +		goto fail1;
> +
> +	dev_info(&gadget->dev, "%s, version: " DRIVER_VERSION "\n",
> +			DRIVER_DESC);
> +	fsg_common_put(&fsg_common);
> +	return 0;
> +
> +	/* error recovery */
> +fail1:
> +	fsg_common_put(&fsg_common);
> +fail0:
> +	gserial_cleanup();
> +	return status;
> +}
> +
> +static int __exit cdc_unbind(struct usb_composite_dev *cdev)
> +{
> +	gserial_cleanup();
> +
> +	return 0;
> +}
> +
> +static struct usb_composite_driver cdc_driver = {
> +	.name		= "g_acm_ms",
> +	.dev		= &device_desc,
> +	.strings	= dev_strings,
> +	.unbind		= __exit_p(cdc_unbind),
> +};
> +
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_AUTHOR("Klaus Schwarzkopf");
> +MODULE_LICENSE("GPL");
> +
> +static int __init init(void)
> +{
> +	return usb_composite_probe(&cdc_driver, cdc_bind);
> +}
> +module_init(init);
> +
> +static void __exit cleanup(void)
> +{
> +	usb_composite_unregister(&cdc_driver);
> +}
> +module_exit(cleanup);
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index dd1571d..f623f3d 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -15,6 +15,7 @@
>  #ifndef __LINUX_USB_GADGET_H
>  #define __LINUX_USB_GADGET_H
>  
> +#include <linux/device.h>

this is not part of $SUBJECT

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2] usb: add new usb gadget for ACM and mass storage
  2011-09-08 18:24 [PATCH] usb: add new usb gadget for ACM and mass storage Klaus Schwarzkopf
  2011-09-08 19:11 ` Greg KH
  2011-10-06 12:08 ` Felipe Balbi
@ 2011-10-07  8:16 ` Klaus Schwarzkopf
  2011-10-07  8:39   ` Felipe Balbi
  2011-10-08  7:44 ` [PATCH v3] " Klaus Schwarzkopf
  2011-10-10  8:32 ` [PATCH v4] " Klaus Schwarzkopf
  4 siblings, 1 reply; 32+ messages in thread
From: Klaus Schwarzkopf @ 2011-10-07  8:16 UTC (permalink / raw)
  To: balbi, gregkh; +Cc: linux-usb, linux-kernel, Klaus Schwarzkopf

This driver provides two functions in one configuration:
a mass storage, and a CDC ACM (serial port) link.
Heavily based on multi.c and cdc2.c

fixed missing header in include/linux/usb/gadget.h

Signed-off-by: Klaus Schwarzkopf <schwarzkopf@sensortherm.de>
---
 drivers/usb/gadget/Kconfig  |   10 ++
 drivers/usb/gadget/Makefile |    2 +
 drivers/usb/gadget/acm_ms.c |  255 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/gadget.h  |    1 +
 4 files changed, 268 insertions(+), 0 deletions(-)
 create mode 100644 drivers/usb/gadget/acm_ms.c

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 029e288..2592ba7 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -959,6 +959,16 @@ config USB_G_NOKIA
 	  It's only really useful for N900 hardware. If you're building
 	  a kernel for N900, say Y or M here. If unsure, say N.
 
+config USB_G_ACM_MS
+	tristate "CDC Composite Device (ACM and mass storage)"
+	depends on BLOCK
+	help
+	  This driver provides two functions in one configuration:
+	  a mass storage, and a CDC ACM (serial port) link.
+
+	  Say "y" to link the driver statically, or "m" to build a
+	  dynamically linked module called "g_acm_ms".
+
 config USB_G_MULTI
 	tristate "Multifunction Composite Gadget (EXPERIMENTAL)"
 	depends on BLOCK && NET
diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index 4fe92b1..3876d8c 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -49,6 +49,7 @@ g_dbgp-y			:= dbgp.o
 g_nokia-y			:= nokia.o
 g_webcam-y			:= webcam.o
 g_ncm-y				:= ncm.o
+g_acm_ms-y			:= acm_ms.o
 
 obj-$(CONFIG_USB_ZERO)		+= g_zero.o
 obj-$(CONFIG_USB_AUDIO)		+= g_audio.o
@@ -67,3 +68,4 @@ obj-$(CONFIG_USB_G_MULTI)	+= g_multi.o
 obj-$(CONFIG_USB_G_NOKIA)	+= g_nokia.o
 obj-$(CONFIG_USB_G_WEBCAM)	+= g_webcam.o
 obj-$(CONFIG_USB_G_NCM)		+= g_ncm.o
+obj-$(CONFIG_USB_G_ACM_MS)	+= g_acm_ms.o
diff --git a/drivers/usb/gadget/acm_ms.c b/drivers/usb/gadget/acm_ms.c
new file mode 100644
index 0000000..28280c4
--- /dev/null
+++ b/drivers/usb/gadget/acm_ms.c
@@ -0,0 +1,255 @@
+/*
+ * cdc2.c -- CDC Composite driver, with ACM and mass storage support
+ *
+ * Copyright (C) 2008 David Brownell
+ * Copyright (C) 2008 Nokia Corporation
+ * Author: David Brownell
+ * Modified: Klaus Schwarzkopf
+ *
+ * Heavily based on multi.c and cdc2.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/utsname.h>
+
+#include "u_serial.h"
+
+#define DRIVER_DESC		"CDC Composite Gadget (ACM + MS)"
+#define DRIVER_VERSION		"2011/10/07"
+
+/*-------------------------------------------------------------------------*/
+
+/*
+ * DO NOT REUSE THESE IDs with a protocol-incompatible driver!!  Ever!!
+ * Instead:  allocate your own, using normal USB-IF procedures.
+ */
+#define CDC_VENDOR_NUM	0x1d6b	/* Linux Foundation */
+#define CDC_PRODUCT_NUM	0x0106	/* CDC Composite: ACM + MS*/
+
+/*-------------------------------------------------------------------------*/
+
+/*
+ * Kbuild is not very cooperative with respect to linking separately
+ * compiled library objects into one module.  So for now we won't use
+ * separate compilation ... ensuring init/exit sections work to shrink
+ * the runtime footprint, and giving us at least some parts of what
+ * a "gcc --combine ... part1.c part2.c part3.c ... " build would.
+ */
+
+#include "composite.c"
+#include "usbstring.c"
+#include "config.c"
+#include "epautoconf.c"
+#include "u_serial.c"
+#include "f_acm.c"
+#include "f_mass_storage.c"
+
+/*-------------------------------------------------------------------------*/
+
+static struct usb_device_descriptor device_desc = {
+	.bLength =		sizeof device_desc,
+	.bDescriptorType =	USB_DT_DEVICE,
+
+	.bcdUSB =		cpu_to_le16(0x0200),
+
+	.bDeviceClass =		USB_CLASS_COMM,
+	.bDeviceSubClass =	0,
+	.bDeviceProtocol =	0,
+	/* .bMaxPacketSize0 = f(hardware) */
+
+	/* Vendor and product id can be overridden by module parameters.  */
+	.idVendor =		cpu_to_le16(CDC_VENDOR_NUM),
+	.idProduct =		cpu_to_le16(CDC_PRODUCT_NUM),
+	/* .bcdDevice = f(hardware) */
+	/* .iManufacturer = DYNAMIC */
+	/* .iProduct = DYNAMIC */
+	/* NO SERIAL NUMBER */
+	.bNumConfigurations =	1,
+};
+
+static struct usb_otg_descriptor otg_descriptor = {
+	.bLength =		sizeof otg_descriptor,
+	.bDescriptorType =	USB_DT_OTG,
+
+	/*
+	 * REVISIT SRP-only hardware is possible, although
+	 * it would not be called "OTG" ...
+	 */
+	.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
+};
+
+static const struct usb_descriptor_header *otg_desc[] = {
+	(struct usb_descriptor_header *) &otg_descriptor,
+	NULL,
+};
+
+
+/* string IDs are assigned dynamically */
+
+#define STRING_MANUFACTURER_IDX		0
+#define STRING_PRODUCT_IDX		1
+
+static char manufacturer[50];
+
+static struct usb_string strings_dev[] = {
+	[STRING_MANUFACTURER_IDX].s = manufacturer,
+	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
+	{  } /* end of list */
+};
+
+static struct usb_gadget_strings stringtab_dev = {
+	.language	= 0x0409,	/* en-us */
+	.strings	= strings_dev,
+};
+
+static struct usb_gadget_strings *dev_strings[] = {
+	&stringtab_dev,
+	NULL,
+};
+
+/****************************** Configurations ******************************/
+
+static struct fsg_module_parameters fsg_mod_data = { .stall = 1 };
+FSG_MODULE_PARAMETERS(/* no prefix */, fsg_mod_data);
+
+static struct fsg_common fsg_common;
+
+/*-------------------------------------------------------------------------*/
+
+/*
+ * We _always_ have both CDC ACM and mass storage functions.
+ */
+static int __init cdc_do_config(struct usb_configuration *c)
+{
+	int	status;
+
+	if (gadget_is_otg(c->cdev->gadget)) {
+		c->descriptors = otg_desc;
+		c->bmAttributes |= USB_CONFIG_ATT_WAKEUP;
+	}
+
+
+	status = acm_bind_config(c, 0);
+	if (status < 0)
+		return status;
+
+	status = fsg_bind_config(c->cdev, c, &fsg_common);
+	if (status < 0)
+		return status;
+
+	return 0;
+}
+
+static struct usb_configuration cdc_config_driver = {
+	.label			= DRIVER_DESC,
+	.bConfigurationValue	= 1,
+	/* .iConfiguration = DYNAMIC */
+	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
+};
+
+/*-------------------------------------------------------------------------*/
+
+static int __init cdc_bind(struct usb_composite_dev *cdev)
+{
+	int			gcnum;
+	struct usb_gadget	*gadget = cdev->gadget;
+	int			status;
+	void			*retp;
+
+	/* set up serial link layer */
+	status = gserial_setup(cdev->gadget, 1);
+	if (status < 0)
+		return status;
+
+	/* set up mass storage function */
+	retp = fsg_common_from_params(&fsg_common, cdev, &fsg_mod_data);
+	if (IS_ERR(retp)) {
+		status = PTR_ERR(retp);
+		goto fail0;
+	}
+
+	/* set bcdDevice */
+	gcnum = usb_gadget_controller_number(gadget);
+	if (gcnum >= 0) {
+		device_desc.bcdDevice = cpu_to_le16(0x0300 | gcnum);
+	} else {
+		WARNING(cdev, "controller '%s' not recognized; trying %s\n",
+				gadget->name,
+				cdc_config_driver.label);
+		device_desc.bcdDevice =
+			cpu_to_le16(0x0300 | 0x0099);
+	}
+
+	/*
+	 * Allocate string descriptor numbers ... note that string
+	 * contents can be overridden by the composite_dev glue.
+	 */
+
+	/* device descriptor strings: manufacturer, product */
+	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
+		init_utsname()->sysname, init_utsname()->release,
+		gadget->name);
+	status = usb_string_id(cdev);
+	if (status < 0)
+		goto fail1;
+	strings_dev[STRING_MANUFACTURER_IDX].id = status;
+	device_desc.iManufacturer = status;
+
+	status = usb_string_id(cdev);
+	if (status < 0)
+		goto fail1;
+	strings_dev[STRING_PRODUCT_IDX].id = status;
+	device_desc.iProduct = status;
+
+	/* register our configuration */
+	status = usb_add_config(cdev, &cdc_config_driver, cdc_do_config);
+	if (status < 0)
+		goto fail1;
+
+	dev_info(&gadget->dev, "%s, version: " DRIVER_VERSION "\n",
+			DRIVER_DESC);
+	fsg_common_put(&fsg_common);
+	return 0;
+
+	/* error recovery */
+fail1:
+	fsg_common_put(&fsg_common);
+fail0:
+	gserial_cleanup();
+	return status;
+}
+
+static int __exit cdc_unbind(struct usb_composite_dev *cdev)
+{
+	gserial_cleanup();
+
+	return 0;
+}
+
+static struct usb_composite_driver cdc_driver = {
+	.name		= "g_acm_ms",
+	.dev		= &device_desc,
+	.strings	= dev_strings,
+	.unbind		= __exit_p(cdc_unbind),
+};
+
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_AUTHOR("Klaus Schwarzkopf");
+MODULE_LICENSE("GPL");
+
+static int __init init(void)
+{
+	return usb_composite_probe(&cdc_driver, cdc_bind);
+}
+module_init(init);
+
+static void __exit cleanup(void)
+{
+	usb_composite_unregister(&cdc_driver);
+}
+module_exit(cleanup);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index dd1571d..f623f3d 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -15,6 +15,7 @@
 #ifndef __LINUX_USB_GADGET_H
 #define __LINUX_USB_GADGET_H
 
+#include <linux/device.h>
 #include <linux/slab.h>
 
 struct usb_ep;
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-10-06 12:08 ` Felipe Balbi
@ 2011-10-07  8:23   ` Klaus Schwarzkopf
  2011-10-07  8:38     ` Felipe Balbi
  2011-10-07 11:11     ` Sergei Shtylyov
  0 siblings, 2 replies; 32+ messages in thread
From: Klaus Schwarzkopf @ 2011-10-07  8:23 UTC (permalink / raw)
  To: balbi; +Cc: gregkh, linux-usb, linux-kernel

Hi,

thanks for your comments. See my patch v2.

Am 06.10.2011 14:08, schrieb Felipe Balbi:

>
>> +#define CDC_VENDOR_NUM	0x1d6b	/* Linux Foundation */
>> +#define CDC_PRODUCT_NUM	0x0106	/* CDC Composite: ACM + MS*/
>
> did you ask Greg for this ?
>

Greg wrote:

I'll do that when I accept the patch for merging into the kernel, and 
not before.


>> +/*-------------------------------------------------------------------------*/
>> +
>> +/*
>> + * Kbuild is not very cooperative with respect to linking separately
>> + * compiled library objects into one module.  So for now we won't use
>> + * separate compilation ... ensuring init/exit sections work to shrink
>> + * the runtime footprint, and giving us at least some parts of what
>> + * a "gcc --combine ... part1.c part2.c part3.c ... " build would.
>> + */
>> +
>> +#include "composite.c"
>> +#include "usbstring.c"
>> +#include "config.c"
>> +#include "epautoconf.c"
>> +#include "u_serial.c"
>> +#include "f_acm.c"
>> +#include "f_mass_storage.c"
>> +
>> +/*-------------------------------------------------------------------------*/
>> +
>> +static struct usb_device_descriptor device_desc = {
>> +	.bLength =		sizeof device_desc,
>> +	.bDescriptorType =	USB_DT_DEVICE,
>> +
>> +	.bcdUSB =		cpu_to_le16(0x0200),
>> +
>> +	.bDeviceClass =		USB_CLASS_COMM,
>> +	.bDeviceSubClass =	0,
>> +	.bDeviceProtocol =	0,
>> +	/* .bMaxPacketSize0 = f(hardware) */
>> +
>> +	/* Vendor and product id can be overridden by module parameters.  */
>> +	.idVendor =		cpu_to_le16(CDC_VENDOR_NUM),
>> +	.idProduct =		cpu_to_le16(CDC_PRODUCT_NUM),
>> +	/* .bcdDevice = f(hardware) */
>> +	/* .iManufacturer = DYNAMIC */
>> +	/* .iProduct = DYNAMIC */
>> +	/* NO SERIAL NUMBER */
>
> why not ? If someone wants to make a product out of this, they will need
> a serial number.
>

If an developer want a serial number, he can change this.

>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index dd1571d..f623f3d 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -15,6 +15,7 @@
>>   #ifndef __LINUX_USB_GADGET_H
>>   #define __LINUX_USB_GADGET_H
>>
>> +#include<linux/device.h>
>
> this is not part of $SUBJECT
>

changed git message


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-10-07  8:23   ` Klaus Schwarzkopf
@ 2011-10-07  8:38     ` Felipe Balbi
  2011-10-07 10:07       ` Klaus Schwarzkopf
  2011-10-07 11:11     ` Sergei Shtylyov
  1 sibling, 1 reply; 32+ messages in thread
From: Felipe Balbi @ 2011-10-07  8:38 UTC (permalink / raw)
  To: Klaus Schwarzkopf; +Cc: balbi, gregkh, linux-usb, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2671 bytes --]

Hi,

On Fri, Oct 07, 2011 at 10:23:10AM +0200, Klaus Schwarzkopf wrote:
> Hi,
> 
> thanks for your comments. See my patch v2.
> 
> Am 06.10.2011 14:08, schrieb Felipe Balbi:
> 
> >
> >>+#define CDC_VENDOR_NUM	0x1d6b	/* Linux Foundation */
> >>+#define CDC_PRODUCT_NUM	0x0106	/* CDC Composite: ACM + MS*/
> >
> >did you ask Greg for this ?
> >
> 
> Greg wrote:
> 
> I'll do that when I accept the patch for merging into the kernel, and
> not before.

ok.

> >>+/*-------------------------------------------------------------------------*/
> >>+
> >>+/*
> >>+ * Kbuild is not very cooperative with respect to linking separately
> >>+ * compiled library objects into one module.  So for now we won't use
> >>+ * separate compilation ... ensuring init/exit sections work to shrink
> >>+ * the runtime footprint, and giving us at least some parts of what
> >>+ * a "gcc --combine ... part1.c part2.c part3.c ... " build would.
> >>+ */
> >>+
> >>+#include "composite.c"
> >>+#include "usbstring.c"
> >>+#include "config.c"
> >>+#include "epautoconf.c"
> >>+#include "u_serial.c"
> >>+#include "f_acm.c"
> >>+#include "f_mass_storage.c"
> >>+
> >>+/*-------------------------------------------------------------------------*/
> >>+
> >>+static struct usb_device_descriptor device_desc = {
> >>+	.bLength =		sizeof device_desc,
> >>+	.bDescriptorType =	USB_DT_DEVICE,
> >>+
> >>+	.bcdUSB =		cpu_to_le16(0x0200),
> >>+
> >>+	.bDeviceClass =		USB_CLASS_COMM,
> >>+	.bDeviceSubClass =	0,
> >>+	.bDeviceProtocol =	0,
> >>+	/* .bMaxPacketSize0 = f(hardware) */
> >>+
> >>+	/* Vendor and product id can be overridden by module parameters.  */
> >>+	.idVendor =		cpu_to_le16(CDC_VENDOR_NUM),
> >>+	.idProduct =		cpu_to_le16(CDC_PRODUCT_NUM),
> >>+	/* .bcdDevice = f(hardware) */
> >>+	/* .iManufacturer = DYNAMIC */
> >>+	/* .iProduct = DYNAMIC */
> >>+	/* NO SERIAL NUMBER */
> >
> >why not ? If someone wants to make a product out of this, they will need
> >a serial number.
> >
> 
> If an developer want a serial number, he can change this.

ok.

> >>diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> >>index dd1571d..f623f3d 100644
> >>--- a/include/linux/usb/gadget.h
> >>+++ b/include/linux/usb/gadget.h
> >>@@ -15,6 +15,7 @@
> >>  #ifndef __LINUX_USB_GADGET_H
> >>  #define __LINUX_USB_GADGET_H
> >>
> >>+#include<linux/device.h>
> >
> >this is not part of $SUBJECT
> >
> 
> changed git message

not enough. adding another header is not part of this patch. You need to
let us know why you need this new header there and you need add proper
spacing there.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2] usb: add new usb gadget for ACM and mass storage
  2011-10-07  8:16 ` [PATCH v2] " Klaus Schwarzkopf
@ 2011-10-07  8:39   ` Felipe Balbi
  0 siblings, 0 replies; 32+ messages in thread
From: Felipe Balbi @ 2011-10-07  8:39 UTC (permalink / raw)
  To: Klaus Schwarzkopf; +Cc: balbi, gregkh, linux-usb, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 315 bytes --]

On Fri, Oct 07, 2011 at 10:16:34AM +0200, Klaus Schwarzkopf wrote:
> This driver provides two functions in one configuration:
> a mass storage, and a CDC ACM (serial port) link.
> Heavily based on multi.c and cdc2.c
> 
> fixed missing header in include/linux/usb/gadget.h

why is it missing ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-10-07  8:38     ` Felipe Balbi
@ 2011-10-07 10:07       ` Klaus Schwarzkopf
  2011-10-07 10:14         ` Felipe Balbi
  0 siblings, 1 reply; 32+ messages in thread
From: Klaus Schwarzkopf @ 2011-10-07 10:07 UTC (permalink / raw)
  To: balbi; +Cc: gregkh, linux-usb, linux-kernel

Hi,


Am 07.10.2011 10:38, schrieb Felipe Balbi:

>>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>>>> index dd1571d..f623f3d 100644
>>>> --- a/include/linux/usb/gadget.h
>>>> +++ b/include/linux/usb/gadget.h
>>>> @@ -15,6 +15,7 @@
>>>>   #ifndef __LINUX_USB_GADGET_H
>>>>   #define __LINUX_USB_GADGET_H
>>>>
>>>> +#include<linux/device.h>
>>>
>>> this is not part of $SUBJECT
>>>
>>
>> changed git message
>
> not enough. adding another header is not part of this patch. You need to
> let us know why you need this new header there and you need add proper
> spacing there.
>


There is a space in the orginal patch.

Should i make a seperate patch for this?

In the header gadget.h are some stuff used, but not declared.

My compile errors without device.h:

   CC      drivers/usb/gadget/acm_ms.o
In file included from include/linux/usb/composite.h:38,
                  from drivers/usb/gadget/u_serial.h:15,
                  from drivers/usb/gadget/acm_ms.c:20:
include/linux/usb/gadget.h:491: error: field ‘dev’ has incomplete type
include/linux/usb/gadget.h: In function ‘set_gadget_data’:
include/linux/usb/gadget.h:495: error: implicit declaration of function 
‘dev_set_drvdata’
include/linux/usb/gadget.h: In function ‘get_gadget_data’:
include/linux/usb/gadget.h:497: error: implicit declaration of function 
‘dev_get_drvdata’
include/linux/usb/gadget.h: In function ‘dev_to_usb_gadget’:
include/linux/usb/gadget.h:500: warning: type defaults to ‘int’ in 
declaration of ‘__mptr’
include/linux/usb/gadget.h:500: warning: initialization from 
incompatible pointer type
include/linux/usb/gadget.h: At top level:
include/linux/usb/gadget.h:781: error: field ‘driver’ has incomplete type
In file included from drivers/usb/gadget/composite.c:26,
                  from drivers/usb/gadget/acm_ms.c:44:
include/linux/device.h:705: error: conflicting types for ‘dev_get_drvdata’
include/linux/usb/gadget.h:497: note: previous implicit declaration of 
‘dev_get_drvdata’ was here
In file included from drivers/usb/gadget/acm_ms.c:44:
drivers/usb/gadget/composite.c:1261: error: field name not in record or 
union initializer
drivers/usb/gadget/composite.c:1261: error: (near initialization for 
‘composite_driver.driver’)
make[3]: *** [drivers/usb/gadget/acm_ms.o] Error 1
make[2]: *** [drivers/usb/gadget] Error 2
make[1]: *** [drivers/usb] Error 2
make: *** [drivers] Error 2


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-10-07 10:07       ` Klaus Schwarzkopf
@ 2011-10-07 10:14         ` Felipe Balbi
  2011-10-07 11:13           ` Sergei Shtylyov
  0 siblings, 1 reply; 32+ messages in thread
From: Felipe Balbi @ 2011-10-07 10:14 UTC (permalink / raw)
  To: Klaus Schwarzkopf; +Cc: balbi, gregkh, linux-usb, linux-kernel, Sergei Shtylyov

[-- Attachment #1: Type: text/plain, Size: 2862 bytes --]

Hi,

On Fri, Oct 07, 2011 at 12:07:00PM +0200, Klaus Schwarzkopf wrote:
> Hi,
> 
> 
> Am 07.10.2011 10:38, schrieb Felipe Balbi:
> 
> >>>>diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> >>>>index dd1571d..f623f3d 100644
> >>>>--- a/include/linux/usb/gadget.h
> >>>>+++ b/include/linux/usb/gadget.h
> >>>>@@ -15,6 +15,7 @@
> >>>>  #ifndef __LINUX_USB_GADGET_H
> >>>>  #define __LINUX_USB_GADGET_H
> >>>>
> >>>>+#include<linux/device.h>
> >>>
> >>>this is not part of $SUBJECT
> >>>
> >>
> >>changed git message
> >
> >not enough. adding another header is not part of this patch. You need to
> >let us know why you need this new header there and you need add proper
> >spacing there.
> >
> 
> 
> There is a space in the orginal patch.
> 
> Should i make a seperate patch for this?
> 
> In the header gadget.h are some stuff used, but not declared.
> 
> My compile errors without device.h:
> 
>   CC      drivers/usb/gadget/acm_ms.o
> In file included from include/linux/usb/composite.h:38,
>                  from drivers/usb/gadget/u_serial.h:15,
>                  from drivers/usb/gadget/acm_ms.c:20:
> include/linux/usb/gadget.h:491: error: field ‘dev’ has incomplete type
> include/linux/usb/gadget.h: In function ‘set_gadget_data’:
> include/linux/usb/gadget.h:495: error: implicit declaration of
> function ‘dev_set_drvdata’
> include/linux/usb/gadget.h: In function ‘get_gadget_data’:
> include/linux/usb/gadget.h:497: error: implicit declaration of
> function ‘dev_get_drvdata’
> include/linux/usb/gadget.h: In function ‘dev_to_usb_gadget’:
> include/linux/usb/gadget.h:500: warning: type defaults to ‘int’ in
> declaration of ‘__mptr’
> include/linux/usb/gadget.h:500: warning: initialization from
> incompatible pointer type
> include/linux/usb/gadget.h: At top level:
> include/linux/usb/gadget.h:781: error: field ‘driver’ has incomplete type
> In file included from drivers/usb/gadget/composite.c:26,
>                  from drivers/usb/gadget/acm_ms.c:44:
> include/linux/device.h:705: error: conflicting types for ‘dev_get_drvdata’
> include/linux/usb/gadget.h:497: note: previous implicit declaration
> of ‘dev_get_drvdata’ was here
> In file included from drivers/usb/gadget/acm_ms.c:44:
> drivers/usb/gadget/composite.c:1261: error: field name not in record
> or union initializer
> drivers/usb/gadget/composite.c:1261: error: (near initialization for
> ‘composite_driver.driver’)
> make[3]: *** [drivers/usb/gadget/acm_ms.o] Error 1
> make[2]: *** [drivers/usb/gadget] Error 2
> make[1]: *** [drivers/usb] Error 2
> make: *** [drivers] Error 2

I believe Sergei had that patch already, but it wasn't applied for some
reason. Sergei, do you happen to remember the outcome of that patch ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-10-07  8:23   ` Klaus Schwarzkopf
  2011-10-07  8:38     ` Felipe Balbi
@ 2011-10-07 11:11     ` Sergei Shtylyov
  1 sibling, 0 replies; 32+ messages in thread
From: Sergei Shtylyov @ 2011-10-07 11:11 UTC (permalink / raw)
  To: Klaus Schwarzkopf; +Cc: balbi, gregkh, linux-usb, linux-kernel

Hello.

On 07-10-2011 12:23, Klaus Schwarzkopf wrote:

>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>>> index dd1571d..f623f3d 100644
>>> --- a/include/linux/usb/gadget.h
>>> +++ b/include/linux/usb/gadget.h
>>> @@ -15,6 +15,7 @@
>>> #ifndef __LINUX_USB_GADGET_H
>>> #define __LINUX_USB_GADGET_H
>>>
>>> +#include<linux/device.h>
>>
>> this is not part of $SUBJECT


> changed git message

1. This should have been a separate patch.
2. Refresh your tree -- this part have been changed long ago with the #include 
added.

WBR, Sergei


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-10-07 10:14         ` Felipe Balbi
@ 2011-10-07 11:13           ` Sergei Shtylyov
  2011-10-07 12:55             ` Felipe Balbi
  0 siblings, 1 reply; 32+ messages in thread
From: Sergei Shtylyov @ 2011-10-07 11:13 UTC (permalink / raw)
  To: balbi; +Cc: Klaus Schwarzkopf, gregkh, linux-usb, linux-kernel

On 07.10.2011 14:14, Felipe Balbi wrote:

>>>>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>>>>>> index dd1571d..f623f3d 100644
>>>>>> --- a/include/linux/usb/gadget.h
>>>>>> +++ b/include/linux/usb/gadget.h
>>>>>> @@ -15,6 +15,7 @@
>>>>>>   #ifndef __LINUX_USB_GADGET_H
>>>>>>   #define __LINUX_USB_GADGET_H
>>>>>>
>>>>>> +#include<linux/device.h>

>>>>> this is not part of $SUBJECT


>>>> changed git message

>>> not enough. adding another header is not part of this patch. You need to
>>> let us know why you need this new header there and you need add proper
>>> spacing there.



>> There is a space in the orginal patch.

>> Should i make a seperate patch for this?

>> In the header gadget.h are some stuff used, but not declared.

>> My compile errors without device.h:

>>    CC      drivers/usb/gadget/acm_ms.o
>> In file included from include/linux/usb/composite.h:38,
>>                   from drivers/usb/gadget/u_serial.h:15,
>>                   from drivers/usb/gadget/acm_ms.c:20:
>> include/linux/usb/gadget.h:491: error: field ‘dev’ has incomplete type
>> include/linux/usb/gadget.h: In function ‘set_gadget_data’:
>> include/linux/usb/gadget.h:495: error: implicit declaration of
>> function ‘dev_set_drvdata’
>> include/linux/usb/gadget.h: In function ‘get_gadget_data’:
>> include/linux/usb/gadget.h:497: error: implicit declaration of
>> function ‘dev_get_drvdata’
>> include/linux/usb/gadget.h: In function ‘dev_to_usb_gadget’:
>> include/linux/usb/gadget.h:500: warning: type defaults to ‘int’ in
>> declaration of ‘__mptr’
>> include/linux/usb/gadget.h:500: warning: initialization from
>> incompatible pointer type
>> include/linux/usb/gadget.h: At top level:
>> include/linux/usb/gadget.h:781: error: field ‘driver’ has incomplete type
>> In file included from drivers/usb/gadget/composite.c:26,
>>                   from drivers/usb/gadget/acm_ms.c:44:
>> include/linux/device.h:705: error: conflicting types for ‘dev_get_drvdata’
>> include/linux/usb/gadget.h:497: note: previous implicit declaration
>> of ‘dev_get_drvdata’ was here
>> In file included from drivers/usb/gadget/acm_ms.c:44:
>> drivers/usb/gadget/composite.c:1261: error: field name not in record
>> or union initializer
>> drivers/usb/gadget/composite.c:1261: error: (near initialization for
>> ‘composite_driver.driver’)
>> make[3]: *** [drivers/usb/gadget/acm_ms.o] Error 1
>> make[2]: *** [drivers/usb/gadget] Error 2
>> make[1]: *** [drivers/usb] Error 2
>> make: *** [drivers] Error 2

> I believe Sergei had that patch already, but it wasn't applied for some
> reason. Sergei, do you happen to remember the outcome of that patch ?

    It has been apllied allright to 3.1-rc1. :-)

WBR, Sergei


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] usb: add new usb gadget for ACM and mass storage
  2011-10-07 11:13           ` Sergei Shtylyov
@ 2011-10-07 12:55             ` Felipe Balbi
  0 siblings, 0 replies; 32+ messages in thread
From: Felipe Balbi @ 2011-10-07 12:55 UTC (permalink / raw)
  To: Sergei Shtylyov; +Cc: balbi, Klaus Schwarzkopf, gregkh, linux-usb, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]

Hi,

On Fri, Oct 07, 2011 at 03:13:50PM +0400, Sergei Shtylyov wrote:
> >>include/linux/device.h:705: error: conflicting types for ‘dev_get_drvdata’
> >>include/linux/usb/gadget.h:497: note: previous implicit declaration
> >>of ‘dev_get_drvdata’ was here
> >>In file included from drivers/usb/gadget/acm_ms.c:44:
> >>drivers/usb/gadget/composite.c:1261: error: field name not in record
> >>or union initializer
> >>drivers/usb/gadget/composite.c:1261: error: (near initialization for
> >>‘composite_driver.driver’)
> >>make[3]: *** [drivers/usb/gadget/acm_ms.o] Error 1
> >>make[2]: *** [drivers/usb/gadget] Error 2
> >>make[1]: *** [drivers/usb] Error 2
> >>make: *** [drivers] Error 2
> 
> >I believe Sergei had that patch already, but it wasn't applied for some
> >reason. Sergei, do you happen to remember the outcome of that patch ?
> 
>    It has been apllied allright to 3.1-rc1. :-)

very true. and I need memory pills as it went through me :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v3] usb: add new usb gadget for ACM and mass storage
  2011-09-08 18:24 [PATCH] usb: add new usb gadget for ACM and mass storage Klaus Schwarzkopf
                   ` (2 preceding siblings ...)
  2011-10-07  8:16 ` [PATCH v2] " Klaus Schwarzkopf
@ 2011-10-08  7:44 ` Klaus Schwarzkopf
  2011-10-10  5:50   ` Felipe Balbi
  2011-10-10  8:32 ` [PATCH v4] " Klaus Schwarzkopf
  4 siblings, 1 reply; 32+ messages in thread
From: Klaus Schwarzkopf @ 2011-10-08  7:44 UTC (permalink / raw)
  To: balbi, gregkh; +Cc: linux-usb, linux-kernel, sshtylyov, Klaus Schwarzkopf

This driver provides two functions in one configuration:
a mass storage, and a CDC ACM (serial port) link.
Heavily based on multi.c and cdc2.c

Signed-off-by: Klaus Schwarzkopf <schwarzkopf@sensortherm.de>
---
 drivers/usb/gadget/Kconfig  |   10 ++
 drivers/usb/gadget/Makefile |    2 +
 drivers/usb/gadget/acm_ms.c |  255 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 267 insertions(+), 0 deletions(-)
 create mode 100644 drivers/usb/gadget/acm_ms.c

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 5a084b9..cebefa6 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -846,6 +846,16 @@ config USB_G_NOKIA
 	  It's only really useful for N900 hardware. If you're building
 	  a kernel for N900, say Y or M here. If unsure, say N.
 
+config USB_G_ACM_MS
+	tristate "CDC Composite Device (ACM and mass storage)"
+	depends on BLOCK
+	help
+	  This driver provides two functions in one configuration:
+	  a mass storage, and a CDC ACM (serial port) link.
+
+	  Say "y" to link the driver statically, or "m" to build a
+	  dynamically linked module called "g_acm_ms".
+
 config USB_G_MULTI
 	tristate "Multifunction Composite Gadget (EXPERIMENTAL)"
 	depends on BLOCK && NET
diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index 9ba725a..8a4e824 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -51,6 +51,7 @@ g_dbgp-y			:= dbgp.o
 g_nokia-y			:= nokia.o
 g_webcam-y			:= webcam.o
 g_ncm-y				:= ncm.o
+g_acm_ms-y			:= acm_ms.o
 
 obj-$(CONFIG_USB_ZERO)		+= g_zero.o
 obj-$(CONFIG_USB_AUDIO)		+= g_audio.o
@@ -69,3 +70,4 @@ obj-$(CONFIG_USB_G_MULTI)	+= g_multi.o
 obj-$(CONFIG_USB_G_NOKIA)	+= g_nokia.o
 obj-$(CONFIG_USB_G_WEBCAM)	+= g_webcam.o
 obj-$(CONFIG_USB_G_NCM)		+= g_ncm.o
+obj-$(CONFIG_USB_G_ACM_MS)	+= g_acm_ms.o
diff --git a/drivers/usb/gadget/acm_ms.c b/drivers/usb/gadget/acm_ms.c
new file mode 100644
index 0000000..28280c4
--- /dev/null
+++ b/drivers/usb/gadget/acm_ms.c
@@ -0,0 +1,255 @@
+/*
+ * cdc2.c -- CDC Composite driver, with ACM and mass storage support
+ *
+ * Copyright (C) 2008 David Brownell
+ * Copyright (C) 2008 Nokia Corporation
+ * Author: David Brownell
+ * Modified: Klaus Schwarzkopf
+ *
+ * Heavily based on multi.c and cdc2.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/utsname.h>
+
+#include "u_serial.h"
+
+#define DRIVER_DESC		"CDC Composite Gadget (ACM + MS)"
+#define DRIVER_VERSION		"2011/10/07"
+
+/*-------------------------------------------------------------------------*/
+
+/*
+ * DO NOT REUSE THESE IDs with a protocol-incompatible driver!!  Ever!!
+ * Instead:  allocate your own, using normal USB-IF procedures.
+ */
+#define CDC_VENDOR_NUM	0x1d6b	/* Linux Foundation */
+#define CDC_PRODUCT_NUM	0x0106	/* CDC Composite: ACM + MS*/
+
+/*-------------------------------------------------------------------------*/
+
+/*
+ * Kbuild is not very cooperative with respect to linking separately
+ * compiled library objects into one module.  So for now we won't use
+ * separate compilation ... ensuring init/exit sections work to shrink
+ * the runtime footprint, and giving us at least some parts of what
+ * a "gcc --combine ... part1.c part2.c part3.c ... " build would.
+ */
+
+#include "composite.c"
+#include "usbstring.c"
+#include "config.c"
+#include "epautoconf.c"
+#include "u_serial.c"
+#include "f_acm.c"
+#include "f_mass_storage.c"
+
+/*-------------------------------------------------------------------------*/
+
+static struct usb_device_descriptor device_desc = {
+	.bLength =		sizeof device_desc,
+	.bDescriptorType =	USB_DT_DEVICE,
+
+	.bcdUSB =		cpu_to_le16(0x0200),
+
+	.bDeviceClass =		USB_CLASS_COMM,
+	.bDeviceSubClass =	0,
+	.bDeviceProtocol =	0,
+	/* .bMaxPacketSize0 = f(hardware) */
+
+	/* Vendor and product id can be overridden by module parameters.  */
+	.idVendor =		cpu_to_le16(CDC_VENDOR_NUM),
+	.idProduct =		cpu_to_le16(CDC_PRODUCT_NUM),
+	/* .bcdDevice = f(hardware) */
+	/* .iManufacturer = DYNAMIC */
+	/* .iProduct = DYNAMIC */
+	/* NO SERIAL NUMBER */
+	.bNumConfigurations =	1,
+};
+
+static struct usb_otg_descriptor otg_descriptor = {
+	.bLength =		sizeof otg_descriptor,
+	.bDescriptorType =	USB_DT_OTG,
+
+	/*
+	 * REVISIT SRP-only hardware is possible, although
+	 * it would not be called "OTG" ...
+	 */
+	.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
+};
+
+static const struct usb_descriptor_header *otg_desc[] = {
+	(struct usb_descriptor_header *) &otg_descriptor,
+	NULL,
+};
+
+
+/* string IDs are assigned dynamically */
+
+#define STRING_MANUFACTURER_IDX		0
+#define STRING_PRODUCT_IDX		1
+
+static char manufacturer[50];
+
+static struct usb_string strings_dev[] = {
+	[STRING_MANUFACTURER_IDX].s = manufacturer,
+	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
+	{  } /* end of list */
+};
+
+static struct usb_gadget_strings stringtab_dev = {
+	.language	= 0x0409,	/* en-us */
+	.strings	= strings_dev,
+};
+
+static struct usb_gadget_strings *dev_strings[] = {
+	&stringtab_dev,
+	NULL,
+};
+
+/****************************** Configurations ******************************/
+
+static struct fsg_module_parameters fsg_mod_data = { .stall = 1 };
+FSG_MODULE_PARAMETERS(/* no prefix */, fsg_mod_data);
+
+static struct fsg_common fsg_common;
+
+/*-------------------------------------------------------------------------*/
+
+/*
+ * We _always_ have both CDC ACM and mass storage functions.
+ */
+static int __init cdc_do_config(struct usb_configuration *c)
+{
+	int	status;
+
+	if (gadget_is_otg(c->cdev->gadget)) {
+		c->descriptors = otg_desc;
+		c->bmAttributes |= USB_CONFIG_ATT_WAKEUP;
+	}
+
+
+	status = acm_bind_config(c, 0);
+	if (status < 0)
+		return status;
+
+	status = fsg_bind_config(c->cdev, c, &fsg_common);
+	if (status < 0)
+		return status;
+
+	return 0;
+}
+
+static struct usb_configuration cdc_config_driver = {
+	.label			= DRIVER_DESC,
+	.bConfigurationValue	= 1,
+	/* .iConfiguration = DYNAMIC */
+	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
+};
+
+/*-------------------------------------------------------------------------*/
+
+static int __init cdc_bind(struct usb_composite_dev *cdev)
+{
+	int			gcnum;
+	struct usb_gadget	*gadget = cdev->gadget;
+	int			status;
+	void			*retp;
+
+	/* set up serial link layer */
+	status = gserial_setup(cdev->gadget, 1);
+	if (status < 0)
+		return status;
+
+	/* set up mass storage function */
+	retp = fsg_common_from_params(&fsg_common, cdev, &fsg_mod_data);
+	if (IS_ERR(retp)) {
+		status = PTR_ERR(retp);
+		goto fail0;
+	}
+
+	/* set bcdDevice */
+	gcnum = usb_gadget_controller_number(gadget);
+	if (gcnum >= 0) {
+		device_desc.bcdDevice = cpu_to_le16(0x0300 | gcnum);
+	} else {
+		WARNING(cdev, "controller '%s' not recognized; trying %s\n",
+				gadget->name,
+				cdc_config_driver.label);
+		device_desc.bcdDevice =
+			cpu_to_le16(0x0300 | 0x0099);
+	}
+
+	/*
+	 * Allocate string descriptor numbers ... note that string
+	 * contents can be overridden by the composite_dev glue.
+	 */
+
+	/* device descriptor strings: manufacturer, product */
+	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
+		init_utsname()->sysname, init_utsname()->release,
+		gadget->name);
+	status = usb_string_id(cdev);
+	if (status < 0)
+		goto fail1;
+	strings_dev[STRING_MANUFACTURER_IDX].id = status;
+	device_desc.iManufacturer = status;
+
+	status = usb_string_id(cdev);
+	if (status < 0)
+		goto fail1;
+	strings_dev[STRING_PRODUCT_IDX].id = status;
+	device_desc.iProduct = status;
+
+	/* register our configuration */
+	status = usb_add_config(cdev, &cdc_config_driver, cdc_do_config);
+	if (status < 0)
+		goto fail1;
+
+	dev_info(&gadget->dev, "%s, version: " DRIVER_VERSION "\n",
+			DRIVER_DESC);
+	fsg_common_put(&fsg_common);
+	return 0;
+
+	/* error recovery */
+fail1:
+	fsg_common_put(&fsg_common);
+fail0:
+	gserial_cleanup();
+	return status;
+}
+
+static int __exit cdc_unbind(struct usb_composite_dev *cdev)
+{
+	gserial_cleanup();
+
+	return 0;
+}
+
+static struct usb_composite_driver cdc_driver = {
+	.name		= "g_acm_ms",
+	.dev		= &device_desc,
+	.strings	= dev_strings,
+	.unbind		= __exit_p(cdc_unbind),
+};
+
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_AUTHOR("Klaus Schwarzkopf");
+MODULE_LICENSE("GPL");
+
+static int __init init(void)
+{
+	return usb_composite_probe(&cdc_driver, cdc_bind);
+}
+module_init(init);
+
+static void __exit cleanup(void)
+{
+	usb_composite_unregister(&cdc_driver);
+}
+module_exit(cleanup);
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH v3] usb: add new usb gadget for ACM and mass storage
  2011-10-08  7:44 ` [PATCH v3] " Klaus Schwarzkopf
@ 2011-10-10  5:50   ` Felipe Balbi
  2011-10-10  8:33     ` Klaus Schwarzkopf
  0 siblings, 1 reply; 32+ messages in thread
From: Felipe Balbi @ 2011-10-10  5:50 UTC (permalink / raw)
  To: Klaus Schwarzkopf; +Cc: balbi, gregkh, linux-usb, linux-kernel, sshtylyov

[-- Attachment #1: Type: text/plain, Size: 9943 bytes --]

Hi,

On Sat, Oct 08, 2011 at 09:44:10AM +0200, Klaus Schwarzkopf wrote:
> This driver provides two functions in one configuration:
> a mass storage, and a CDC ACM (serial port) link.
> Heavily based on multi.c and cdc2.c
> 
> Signed-off-by: Klaus Schwarzkopf <schwarzkopf@sensortherm.de>
> ---
>  drivers/usb/gadget/Kconfig  |   10 ++
>  drivers/usb/gadget/Makefile |    2 +
>  drivers/usb/gadget/acm_ms.c |  255 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 267 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/usb/gadget/acm_ms.c
> 
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 5a084b9..cebefa6 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -846,6 +846,16 @@ config USB_G_NOKIA
>  	  It's only really useful for N900 hardware. If you're building
>  	  a kernel for N900, say Y or M here. If unsure, say N.
>  
> +config USB_G_ACM_MS
> +	tristate "CDC Composite Device (ACM and mass storage)"
> +	depends on BLOCK
> +	help
> +	  This driver provides two functions in one configuration:
> +	  a mass storage, and a CDC ACM (serial port) link.
> +
> +	  Say "y" to link the driver statically, or "m" to build a
> +	  dynamically linked module called "g_acm_ms".
> +
>  config USB_G_MULTI
>  	tristate "Multifunction Composite Gadget (EXPERIMENTAL)"
>  	depends on BLOCK && NET
> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index 9ba725a..8a4e824 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -51,6 +51,7 @@ g_dbgp-y			:= dbgp.o
>  g_nokia-y			:= nokia.o
>  g_webcam-y			:= webcam.o
>  g_ncm-y				:= ncm.o
> +g_acm_ms-y			:= acm_ms.o
>  
>  obj-$(CONFIG_USB_ZERO)		+= g_zero.o
>  obj-$(CONFIG_USB_AUDIO)		+= g_audio.o
> @@ -69,3 +70,4 @@ obj-$(CONFIG_USB_G_MULTI)	+= g_multi.o
>  obj-$(CONFIG_USB_G_NOKIA)	+= g_nokia.o
>  obj-$(CONFIG_USB_G_WEBCAM)	+= g_webcam.o
>  obj-$(CONFIG_USB_G_NCM)		+= g_ncm.o
> +obj-$(CONFIG_USB_G_ACM_MS)	+= g_acm_ms.o
> diff --git a/drivers/usb/gadget/acm_ms.c b/drivers/usb/gadget/acm_ms.c
> new file mode 100644
> index 0000000..28280c4
> --- /dev/null
> +++ b/drivers/usb/gadget/acm_ms.c
> @@ -0,0 +1,255 @@
> +/*
> + * cdc2.c -- CDC Composite driver, with ACM and mass storage support

this is not cdc2.c

> + * Copyright (C) 2008 David Brownell
> + * Copyright (C) 2008 Nokia Corporation
> + * Author: David Brownell
> + * Modified: Klaus Schwarzkopf
> + *
> + * Heavily based on multi.c and cdc2.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/utsname.h>
> +
> +#include "u_serial.h"
> +
> +#define DRIVER_DESC		"CDC Composite Gadget (ACM + MS)"
> +#define DRIVER_VERSION		"2011/10/07"
> +
> +/*-------------------------------------------------------------------------*/
> +
> +/*
> + * DO NOT REUSE THESE IDs with a protocol-incompatible driver!!  Ever!!
> + * Instead:  allocate your own, using normal USB-IF procedures.
> + */
> +#define CDC_VENDOR_NUM	0x1d6b	/* Linux Foundation */
> +#define CDC_PRODUCT_NUM	0x0106	/* CDC Composite: ACM + MS*/
> +
> +/*-------------------------------------------------------------------------*/
> +
> +/*
> + * Kbuild is not very cooperative with respect to linking separately
> + * compiled library objects into one module.  So for now we won't use
> + * separate compilation ... ensuring init/exit sections work to shrink
> + * the runtime footprint, and giving us at least some parts of what
> + * a "gcc --combine ... part1.c part2.c part3.c ... " build would.
> + */
> +
> +#include "composite.c"
> +#include "usbstring.c"
> +#include "config.c"
> +#include "epautoconf.c"
> +#include "u_serial.c"
> +#include "f_acm.c"
> +#include "f_mass_storage.c"
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static struct usb_device_descriptor device_desc = {
> +	.bLength =		sizeof device_desc,
> +	.bDescriptorType =	USB_DT_DEVICE,
> +
> +	.bcdUSB =		cpu_to_le16(0x0200),
> +
> +	.bDeviceClass =		USB_CLASS_COMM,
> +	.bDeviceSubClass =	0,
> +	.bDeviceProtocol =	0,
> +	/* .bMaxPacketSize0 = f(hardware) */
> +
> +	/* Vendor and product id can be overridden by module parameters.  */
> +	.idVendor =		cpu_to_le16(CDC_VENDOR_NUM),
> +	.idProduct =		cpu_to_le16(CDC_PRODUCT_NUM),
> +	/* .bcdDevice = f(hardware) */
> +	/* .iManufacturer = DYNAMIC */
> +	/* .iProduct = DYNAMIC */
> +	/* NO SERIAL NUMBER */
> +	.bNumConfigurations =	1,

bNumConfigurations should be dynamic. And I guess composite.c is already
handling that for free.

> +};
> +
> +static struct usb_otg_descriptor otg_descriptor = {
> +	.bLength =		sizeof otg_descriptor,
> +	.bDescriptorType =	USB_DT_OTG,
> +
> +	/*
> +	 * REVISIT SRP-only hardware is possible, although
> +	 * it would not be called "OTG" ...
> +	 */
> +	.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
> +};
> +
> +static const struct usb_descriptor_header *otg_desc[] = {
> +	(struct usb_descriptor_header *) &otg_descriptor,
> +	NULL,
> +};
> +
> +
> +/* string IDs are assigned dynamically */
> +
> +#define STRING_MANUFACTURER_IDX		0
> +#define STRING_PRODUCT_IDX		1
> +
> +static char manufacturer[50];
> +
> +static struct usb_string strings_dev[] = {
> +	[STRING_MANUFACTURER_IDX].s = manufacturer,
> +	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
> +	{  } /* end of list */
> +};
> +
> +static struct usb_gadget_strings stringtab_dev = {
> +	.language	= 0x0409,	/* en-us */
> +	.strings	= strings_dev,
> +};
> +
> +static struct usb_gadget_strings *dev_strings[] = {
> +	&stringtab_dev,
> +	NULL,
> +};
> +
> +/****************************** Configurations ******************************/
> +
> +static struct fsg_module_parameters fsg_mod_data = { .stall = 1 };
> +FSG_MODULE_PARAMETERS(/* no prefix */, fsg_mod_data);
> +
> +static struct fsg_common fsg_common;
> +
> +/*-------------------------------------------------------------------------*/
> +
> +/*
> + * We _always_ have both CDC ACM and mass storage functions.
> + */
> +static int __init cdc_do_config(struct usb_configuration *c)
> +{
> +	int	status;
> +
> +	if (gadget_is_otg(c->cdev->gadget)) {
> +		c->descriptors = otg_desc;
> +		c->bmAttributes |= USB_CONFIG_ATT_WAKEUP;
> +	}
> +
> +
> +	status = acm_bind_config(c, 0);
> +	if (status < 0)
> +		return status;
> +
> +	status = fsg_bind_config(c->cdev, c, &fsg_common);
> +	if (status < 0)
> +		return status;
> +
> +	return 0;
> +}
> +
> +static struct usb_configuration cdc_config_driver = {
> +	.label			= DRIVER_DESC,
> +	.bConfigurationValue	= 1,
> +	/* .iConfiguration = DYNAMIC */
> +	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
> +};
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static int __init cdc_bind(struct usb_composite_dev *cdev)
> +{
> +	int			gcnum;
> +	struct usb_gadget	*gadget = cdev->gadget;
> +	int			status;
> +	void			*retp;
> +
> +	/* set up serial link layer */
> +	status = gserial_setup(cdev->gadget, 1);
> +	if (status < 0)
> +		return status;
> +
> +	/* set up mass storage function */
> +	retp = fsg_common_from_params(&fsg_common, cdev, &fsg_mod_data);
> +	if (IS_ERR(retp)) {
> +		status = PTR_ERR(retp);
> +		goto fail0;
> +	}
> +
> +	/* set bcdDevice */
> +	gcnum = usb_gadget_controller_number(gadget);
> +	if (gcnum >= 0) {
> +		device_desc.bcdDevice = cpu_to_le16(0x0300 | gcnum);
> +	} else {
> +		WARNING(cdev, "controller '%s' not recognized; trying %s\n",
> +				gadget->name,
> +				cdc_config_driver.label);
> +		device_desc.bcdDevice =
> +			cpu_to_le16(0x0300 | 0x0099);
> +	}
> +
> +	/*
> +	 * Allocate string descriptor numbers ... note that string
> +	 * contents can be overridden by the composite_dev glue.
> +	 */
> +
> +	/* device descriptor strings: manufacturer, product */
> +	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
> +		init_utsname()->sysname, init_utsname()->release,
> +		gadget->name);
> +	status = usb_string_id(cdev);
> +	if (status < 0)
> +		goto fail1;
> +	strings_dev[STRING_MANUFACTURER_IDX].id = status;
> +	device_desc.iManufacturer = status;
> +
> +	status = usb_string_id(cdev);
> +	if (status < 0)
> +		goto fail1;
> +	strings_dev[STRING_PRODUCT_IDX].id = status;
> +	device_desc.iProduct = status;
> +
> +	/* register our configuration */
> +	status = usb_add_config(cdev, &cdc_config_driver, cdc_do_config);
> +	if (status < 0)
> +		goto fail1;
> +
> +	dev_info(&gadget->dev, "%s, version: " DRIVER_VERSION "\n",
> +			DRIVER_DESC);
> +	fsg_common_put(&fsg_common);
> +	return 0;
> +
> +	/* error recovery */
> +fail1:
> +	fsg_common_put(&fsg_common);
> +fail0:
> +	gserial_cleanup();
> +	return status;
> +}
> +
> +static int __exit cdc_unbind(struct usb_composite_dev *cdev)
> +{
> +	gserial_cleanup();

shouldn't you call fsg_common_put() ??

> +	return 0;
> +}
> +
> +static struct usb_composite_driver cdc_driver = {
> +	.name		= "g_acm_ms",
> +	.dev		= &device_desc,
> +	.strings	= dev_strings,
> +	.unbind		= __exit_p(cdc_unbind),
> +};
> +
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_AUTHOR("Klaus Schwarzkopf");

add email as well:

MODULE_AUTHOR("Klaus Schwarzkopf <schwarzkopf@sensortherm.de>")

> +MODULE_LICENSE("GPL");

should this be GPL v2 instead ?

> +
> +static int __init init(void)
> +{
> +	return usb_composite_probe(&cdc_driver, cdc_bind);

please run a sed script changing cdc_ to acm_ms_, or something similar,
at least.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v4] usb: add new usb gadget for ACM and mass storage
  2011-09-08 18:24 [PATCH] usb: add new usb gadget for ACM and mass storage Klaus Schwarzkopf
                   ` (3 preceding siblings ...)
  2011-10-08  7:44 ` [PATCH v3] " Klaus Schwarzkopf
@ 2011-10-10  8:32 ` Klaus Schwarzkopf
  2011-10-13 17:43   ` Felipe Balbi
  4 siblings, 1 reply; 32+ messages in thread
From: Klaus Schwarzkopf @ 2011-10-10  8:32 UTC (permalink / raw)
  To: balbi, gregkh; +Cc: linux-usb, linux-kernel, sshtylyov, Klaus Schwarzkopf

This driver provides two functions in one configuration:
a mass storage, and a ACM (serial port) link.
Heavily based on multi.c and cdc2.c

Signed-off-by: Klaus Schwarzkopf <schwarzkopf@sensortherm.de>
---
 drivers/usb/gadget/Kconfig  |   10 ++
 drivers/usb/gadget/Makefile |    2 +
 drivers/usb/gadget/acm_ms.c |  256 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 268 insertions(+), 0 deletions(-)
 create mode 100644 drivers/usb/gadget/acm_ms.c

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 5a084b9..cebefa6 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -846,6 +846,16 @@ config USB_G_NOKIA
 	  It's only really useful for N900 hardware. If you're building
 	  a kernel for N900, say Y or M here. If unsure, say N.
 
+config USB_G_ACM_MS
+	tristate "CDC Composite Device (ACM and mass storage)"
+	depends on BLOCK
+	help
+	  This driver provides two functions in one configuration:
+	  a mass storage, and a CDC ACM (serial port) link.
+
+	  Say "y" to link the driver statically, or "m" to build a
+	  dynamically linked module called "g_acm_ms".
+
 config USB_G_MULTI
 	tristate "Multifunction Composite Gadget (EXPERIMENTAL)"
 	depends on BLOCK && NET
diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
index 9ba725a..8a4e824 100644
--- a/drivers/usb/gadget/Makefile
+++ b/drivers/usb/gadget/Makefile
@@ -51,6 +51,7 @@ g_dbgp-y			:= dbgp.o
 g_nokia-y			:= nokia.o
 g_webcam-y			:= webcam.o
 g_ncm-y				:= ncm.o
+g_acm_ms-y			:= acm_ms.o
 
 obj-$(CONFIG_USB_ZERO)		+= g_zero.o
 obj-$(CONFIG_USB_AUDIO)		+= g_audio.o
@@ -69,3 +70,4 @@ obj-$(CONFIG_USB_G_MULTI)	+= g_multi.o
 obj-$(CONFIG_USB_G_NOKIA)	+= g_nokia.o
 obj-$(CONFIG_USB_G_WEBCAM)	+= g_webcam.o
 obj-$(CONFIG_USB_G_NCM)		+= g_ncm.o
+obj-$(CONFIG_USB_G_ACM_MS)	+= g_acm_ms.o
diff --git a/drivers/usb/gadget/acm_ms.c b/drivers/usb/gadget/acm_ms.c
new file mode 100644
index 0000000..fdb7aec
--- /dev/null
+++ b/drivers/usb/gadget/acm_ms.c
@@ -0,0 +1,256 @@
+/*
+ * acm_ms.c -- Composite driver, with ACM and mass storage support
+ *
+ * Copyright (C) 2008 David Brownell
+ * Copyright (C) 2008 Nokia Corporation
+ * Author: David Brownell
+ * Modified: Klaus Schwarzkopf <schwarzkopf@sensortherm.de>
+ *
+ * Heavily based on multi.c and cdc2.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/utsname.h>
+
+#include "u_serial.h"
+
+#define DRIVER_DESC		"Composite Gadget (ACM + MS)"
+#define DRIVER_VERSION		"2011/10/10"
+
+/*-------------------------------------------------------------------------*/
+
+/*
+ * DO NOT REUSE THESE IDs with a protocol-incompatible driver!!  Ever!!
+ * Instead:  allocate your own, using normal USB-IF procedures.
+ */
+#define ACM_MS_VENDOR_NUM	0x1d6b	/* Linux Foundation */
+#define ACM_MS_PRODUCT_NUM	0x0106	/* Composite Gadget: ACM + MS*/
+
+/*-------------------------------------------------------------------------*/
+
+/*
+ * Kbuild is not very cooperative with respect to linking separately
+ * compiled library objects into one module.  So for now we won't use
+ * separate compilation ... ensuring init/exit sections work to shrink
+ * the runtime footprint, and giving us at least some parts of what
+ * a "gcc --combine ... part1.c part2.c part3.c ... " build would.
+ */
+
+#include "composite.c"
+#include "usbstring.c"
+#include "config.c"
+#include "epautoconf.c"
+#include "u_serial.c"
+#include "f_acm.c"
+#include "f_mass_storage.c"
+
+/*-------------------------------------------------------------------------*/
+
+static struct usb_device_descriptor device_desc = {
+	.bLength =		sizeof device_desc,
+	.bDescriptorType =	USB_DT_DEVICE,
+
+	.bcdUSB =		cpu_to_le16(0x0200),
+
+	.bDeviceClass =		USB_CLASS_MISC /* 0xEF */,
+	.bDeviceSubClass =	2,
+	.bDeviceProtocol =	1,
+
+	/* .bMaxPacketSize0 = f(hardware) */
+
+	/* Vendor and product id can be overridden by module parameters.  */
+	.idVendor =		cpu_to_le16(ACM_MS_VENDOR_NUM),
+	.idProduct =		cpu_to_le16(ACM_MS_PRODUCT_NUM),
+	/* .bcdDevice = f(hardware) */
+	/* .iManufacturer = DYNAMIC */
+	/* .iProduct = DYNAMIC */
+	/* NO SERIAL NUMBER */
+	/*.bNumConfigurations =	DYNAMIC*/
+};
+
+static struct usb_otg_descriptor otg_descriptor = {
+	.bLength =		sizeof otg_descriptor,
+	.bDescriptorType =	USB_DT_OTG,
+
+	/*
+	 * REVISIT SRP-only hardware is possible, although
+	 * it would not be called "OTG" ...
+	 */
+	.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
+};
+
+static const struct usb_descriptor_header *otg_desc[] = {
+	(struct usb_descriptor_header *) &otg_descriptor,
+	NULL,
+};
+
+
+/* string IDs are assigned dynamically */
+
+#define STRING_MANUFACTURER_IDX		0
+#define STRING_PRODUCT_IDX		1
+
+static char manufacturer[50];
+
+static struct usb_string strings_dev[] = {
+	[STRING_MANUFACTURER_IDX].s = manufacturer,
+	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
+	{  } /* end of list */
+};
+
+static struct usb_gadget_strings stringtab_dev = {
+	.language	= 0x0409,	/* en-us */
+	.strings	= strings_dev,
+};
+
+static struct usb_gadget_strings *dev_strings[] = {
+	&stringtab_dev,
+	NULL,
+};
+
+/****************************** Configurations ******************************/
+
+static struct fsg_module_parameters fsg_mod_data = { .stall = 1 };
+FSG_MODULE_PARAMETERS(/* no prefix */, fsg_mod_data);
+
+static struct fsg_common fsg_common;
+
+/*-------------------------------------------------------------------------*/
+
+/*
+ * We _always_ have both ACM and mass storage functions.
+ */
+static int __init acm_ms_do_config(struct usb_configuration *c)
+{
+	int	status;
+
+	if (gadget_is_otg(c->cdev->gadget)) {
+		c->descriptors = otg_desc;
+		c->bmAttributes |= USB_CONFIG_ATT_WAKEUP;
+	}
+
+
+	status = acm_bind_config(c, 0);
+	if (status < 0)
+		return status;
+
+	status = fsg_bind_config(c->cdev, c, &fsg_common);
+	if (status < 0)
+		return status;
+
+	return 0;
+}
+
+static struct usb_configuration acm_ms_config_driver = {
+	.label			= DRIVER_DESC,
+	.bConfigurationValue	= 1,
+	/* .iConfiguration = DYNAMIC */
+	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
+};
+
+/*-------------------------------------------------------------------------*/
+
+static int __init acm_ms_bind(struct usb_composite_dev *cdev)
+{
+	int			gcnum;
+	struct usb_gadget	*gadget = cdev->gadget;
+	int			status;
+	void			*retp;
+
+	/* set up serial link layer */
+	status = gserial_setup(cdev->gadget, 1);
+	if (status < 0)
+		return status;
+
+	/* set up mass storage function */
+	retp = fsg_common_from_params(&fsg_common, cdev, &fsg_mod_data);
+	if (IS_ERR(retp)) {
+		status = PTR_ERR(retp);
+		goto fail0;
+	}
+
+	/* set bcdDevice */
+	gcnum = usb_gadget_controller_number(gadget);
+	if (gcnum >= 0) {
+		device_desc.bcdDevice = cpu_to_le16(0x0300 | gcnum);
+	} else {
+		WARNING(cdev, "controller '%s' not recognized; trying %s\n",
+				gadget->name,
+				acm_ms_config_driver.label);
+		device_desc.bcdDevice =
+			cpu_to_le16(0x0300 | 0x0099);
+	}
+
+	/*
+	 * Allocate string descriptor numbers ... note that string
+	 * contents can be overridden by the composite_dev glue.
+	 */
+
+	/* device descriptor strings: manufacturer, product */
+	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
+		init_utsname()->sysname, init_utsname()->release,
+		gadget->name);
+	status = usb_string_id(cdev);
+	if (status < 0)
+		goto fail1;
+	strings_dev[STRING_MANUFACTURER_IDX].id = status;
+	device_desc.iManufacturer = status;
+
+	status = usb_string_id(cdev);
+	if (status < 0)
+		goto fail1;
+	strings_dev[STRING_PRODUCT_IDX].id = status;
+	device_desc.iProduct = status;
+
+	/* register our configuration */
+	status = usb_add_config(cdev, &acm_ms_config_driver, acm_ms_do_config);
+	if (status < 0)
+		goto fail1;
+
+	dev_info(&gadget->dev, "%s, version: " DRIVER_VERSION "\n",
+			DRIVER_DESC);
+	fsg_common_put(&fsg_common);
+	return 0;
+
+	/* error recovery */
+fail1:
+	fsg_common_put(&fsg_common);
+fail0:
+	gserial_cleanup();
+	return status;
+}
+
+static int __exit acm_ms_unbind(struct usb_composite_dev *cdev)
+{
+	gserial_cleanup();
+
+	return 0;
+}
+
+static struct usb_composite_driver acm_ms_driver = {
+	.name		= "g_acm_ms",
+	.dev		= &device_desc,
+	.strings	= dev_strings,
+	.unbind		= __exit_p(acm_ms_unbind),
+};
+
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_AUTHOR("Klaus Schwarzkopf <schwarzkopf@sensortherm.de>");
+MODULE_LICENSE("GPL v2");
+
+static int __init init(void)
+{
+	return usb_composite_probe(&acm_ms_driver, acm_ms_bind);
+}
+module_init(init);
+
+static void __exit cleanup(void)
+{
+	usb_composite_unregister(&acm_ms_driver);
+}
+module_exit(cleanup);
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH v3] usb: add new usb gadget for ACM and mass storage
  2011-10-10  5:50   ` Felipe Balbi
@ 2011-10-10  8:33     ` Klaus Schwarzkopf
  2011-10-10  8:49       ` Felipe Balbi
                         ` (2 more replies)
  0 siblings, 3 replies; 32+ messages in thread
From: Klaus Schwarzkopf @ 2011-10-10  8:33 UTC (permalink / raw)
  To: balbi; +Cc: gregkh, linux-usb, linux-kernel, sshtylyov

Hi,


Am 10.10.2011 07:50, schrieb Felipe Balbi:
> Hi,
>
> On Sat, Oct 08, 2011 at 09:44:10AM +0200, Klaus Schwarzkopf wrote:
>> This driver provides two functions in one configuration:
>> a mass storage, and a CDC ACM (serial port) link.
>> Heavily based on multi.c and cdc2.c
>>
>> Signed-off-by: Klaus Schwarzkopf<schwarzkopf@sensortherm.de>
>> ---
>>   drivers/usb/gadget/Kconfig  |   10 ++
>>   drivers/usb/gadget/Makefile |    2 +
>>   drivers/usb/gadget/acm_ms.c |  255 +++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 267 insertions(+), 0 deletions(-)
>>   create mode 100644 drivers/usb/gadget/acm_ms.c
>>
>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>> index 5a084b9..cebefa6 100644
>> --- a/drivers/usb/gadget/Kconfig
>> +++ b/drivers/usb/gadget/Kconfig
>> @@ -846,6 +846,16 @@ config USB_G_NOKIA
>>   	  It's only really useful for N900 hardware. If you're building
>>   	  a kernel for N900, say Y or M here. If unsure, say N.
>>
>> +config USB_G_ACM_MS
>> +	tristate "CDC Composite Device (ACM and mass storage)"
>> +	depends on BLOCK
>> +	help
>> +	  This driver provides two functions in one configuration:
>> +	  a mass storage, and a CDC ACM (serial port) link.
>> +
>> +	  Say "y" to link the driver statically, or "m" to build a
>> +	  dynamically linked module called "g_acm_ms".
>> +
>>   config USB_G_MULTI
>>   	tristate "Multifunction Composite Gadget (EXPERIMENTAL)"
>>   	depends on BLOCK&&  NET
>> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
>> index 9ba725a..8a4e824 100644
>> --- a/drivers/usb/gadget/Makefile
>> +++ b/drivers/usb/gadget/Makefile
>> @@ -51,6 +51,7 @@ g_dbgp-y			:= dbgp.o
>>   g_nokia-y			:= nokia.o
>>   g_webcam-y			:= webcam.o
>>   g_ncm-y				:= ncm.o
>> +g_acm_ms-y			:= acm_ms.o
>>
>>   obj-$(CONFIG_USB_ZERO)		+= g_zero.o
>>   obj-$(CONFIG_USB_AUDIO)		+= g_audio.o
>> @@ -69,3 +70,4 @@ obj-$(CONFIG_USB_G_MULTI)	+= g_multi.o
>>   obj-$(CONFIG_USB_G_NOKIA)	+= g_nokia.o
>>   obj-$(CONFIG_USB_G_WEBCAM)	+= g_webcam.o
>>   obj-$(CONFIG_USB_G_NCM)		+= g_ncm.o
>> +obj-$(CONFIG_USB_G_ACM_MS)	+= g_acm_ms.o
>> diff --git a/drivers/usb/gadget/acm_ms.c b/drivers/usb/gadget/acm_ms.c
>> new file mode 100644
>> index 0000000..28280c4
>> --- /dev/null
>> +++ b/drivers/usb/gadget/acm_ms.c
>> @@ -0,0 +1,255 @@
>> +/*
>> + * cdc2.c -- CDC Composite driver, with ACM and mass storage support
>
> this is not cdc2.c
>
>> + * Copyright (C) 2008 David Brownell
>> + * Copyright (C) 2008 Nokia Corporation
>> + * Author: David Brownell
>> + * Modified: Klaus Schwarzkopf
>> + *
>> + * Heavily based on multi.c and cdc2.c
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +
>> +#include<linux/kernel.h>
>> +#include<linux/utsname.h>
>> +
>> +#include "u_serial.h"
>> +
>> +#define DRIVER_DESC		"CDC Composite Gadget (ACM + MS)"
>> +#define DRIVER_VERSION		"2011/10/07"
>> +
>> +/*-------------------------------------------------------------------------*/
>> +
>> +/*
>> + * DO NOT REUSE THESE IDs with a protocol-incompatible driver!!  Ever!!
>> + * Instead:  allocate your own, using normal USB-IF procedures.
>> + */
>> +#define CDC_VENDOR_NUM	0x1d6b	/* Linux Foundation */
>> +#define CDC_PRODUCT_NUM	0x0106	/* CDC Composite: ACM + MS*/
>> +
>> +/*-------------------------------------------------------------------------*/
>> +
>> +/*
>> + * Kbuild is not very cooperative with respect to linking separately
>> + * compiled library objects into one module.  So for now we won't use
>> + * separate compilation ... ensuring init/exit sections work to shrink
>> + * the runtime footprint, and giving us at least some parts of what
>> + * a "gcc --combine ... part1.c part2.c part3.c ... " build would.
>> + */
>> +
>> +#include "composite.c"
>> +#include "usbstring.c"
>> +#include "config.c"
>> +#include "epautoconf.c"
>> +#include "u_serial.c"
>> +#include "f_acm.c"
>> +#include "f_mass_storage.c"
>> +
>> +/*-------------------------------------------------------------------------*/
>> +
>> +static struct usb_device_descriptor device_desc = {
>> +	.bLength =		sizeof device_desc,
>> +	.bDescriptorType =	USB_DT_DEVICE,
>> +
>> +	.bcdUSB =		cpu_to_le16(0x0200),
>> +
>> +	.bDeviceClass =		USB_CLASS_COMM,
>> +	.bDeviceSubClass =	0,
>> +	.bDeviceProtocol =	0,

Should bDeviceClass, bDeviceSubClass, and bDeviceProtocol have the same 
value like in the file multi.c?

	.bDeviceClass =		USB_CLASS_MISC /* 0xEF */,
	.bDeviceSubClass =	2,
	.bDeviceProtocol =	1,



>> +	/* .bMaxPacketSize0 = f(hardware) */
>> +
>> +	/* Vendor and product id can be overridden by module parameters.  */
>> +	.idVendor =		cpu_to_le16(CDC_VENDOR_NUM),
>> +	.idProduct =		cpu_to_le16(CDC_PRODUCT_NUM),
>> +	/* .bcdDevice = f(hardware) */
>> +	/* .iManufacturer = DYNAMIC */
>> +	/* .iProduct = DYNAMIC */
>> +	/* NO SERIAL NUMBER */
>> +	.bNumConfigurations =	1,
>
> bNumConfigurations should be dynamic. And I guess composite.c is already
> handling that for free.
>

OK

>> +};
>> +
>> +static struct usb_otg_descriptor otg_descriptor = {
>> +	.bLength =		sizeof otg_descriptor,
>> +	.bDescriptorType =	USB_DT_OTG,
>> +
>> +	/*
>> +	 * REVISIT SRP-only hardware is possible, although
>> +	 * it would not be called "OTG" ...
>> +	 */
>> +	.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
>> +};
>> +
>> +static const struct usb_descriptor_header *otg_desc[] = {
>> +	(struct usb_descriptor_header *)&otg_descriptor,
>> +	NULL,
>> +};
>> +
>> +
>> +/* string IDs are assigned dynamically */
>> +
>> +#define STRING_MANUFACTURER_IDX		0
>> +#define STRING_PRODUCT_IDX		1
>> +
>> +static char manufacturer[50];
>> +
>> +static struct usb_string strings_dev[] = {
>> +	[STRING_MANUFACTURER_IDX].s = manufacturer,
>> +	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
>> +	{  } /* end of list */
>> +};
>> +
>> +static struct usb_gadget_strings stringtab_dev = {
>> +	.language	= 0x0409,	/* en-us */
>> +	.strings	= strings_dev,
>> +};
>> +
>> +static struct usb_gadget_strings *dev_strings[] = {
>> +	&stringtab_dev,
>> +	NULL,
>> +};
>> +
>> +/****************************** Configurations ******************************/
>> +
>> +static struct fsg_module_parameters fsg_mod_data = { .stall = 1 };
>> +FSG_MODULE_PARAMETERS(/* no prefix */, fsg_mod_data);
>> +
>> +static struct fsg_common fsg_common;
>> +
>> +/*-------------------------------------------------------------------------*/
>> +
>> +/*
>> + * We _always_ have both CDC ACM and mass storage functions.
>> + */
>> +static int __init cdc_do_config(struct usb_configuration *c)
>> +{
>> +	int	status;
>> +
>> +	if (gadget_is_otg(c->cdev->gadget)) {
>> +		c->descriptors = otg_desc;
>> +		c->bmAttributes |= USB_CONFIG_ATT_WAKEUP;
>> +	}
>> +
>> +
>> +	status = acm_bind_config(c, 0);
>> +	if (status<  0)
>> +		return status;
>> +
>> +	status = fsg_bind_config(c->cdev, c,&fsg_common);
>> +	if (status<  0)
>> +		return status;
>> +
>> +	return 0;
>> +}
>> +
>> +static struct usb_configuration cdc_config_driver = {
>> +	.label			= DRIVER_DESC,
>> +	.bConfigurationValue	= 1,
>> +	/* .iConfiguration = DYNAMIC */
>> +	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
>> +};
>> +
>> +/*-------------------------------------------------------------------------*/
>> +
>> +static int __init cdc_bind(struct usb_composite_dev *cdev)
>> +{
>> +	int			gcnum;
>> +	struct usb_gadget	*gadget = cdev->gadget;
>> +	int			status;
>> +	void			*retp;
>> +
>> +	/* set up serial link layer */
>> +	status = gserial_setup(cdev->gadget, 1);
>> +	if (status<  0)
>> +		return status;
>> +
>> +	/* set up mass storage function */
>> +	retp = fsg_common_from_params(&fsg_common, cdev,&fsg_mod_data);
>> +	if (IS_ERR(retp)) {
>> +		status = PTR_ERR(retp);
>> +		goto fail0;
>> +	}
>> +
>> +	/* set bcdDevice */
>> +	gcnum = usb_gadget_controller_number(gadget);
>> +	if (gcnum>= 0) {
>> +		device_desc.bcdDevice = cpu_to_le16(0x0300 | gcnum);
>> +	} else {
>> +		WARNING(cdev, "controller '%s' not recognized; trying %s\n",
>> +				gadget->name,
>> +				cdc_config_driver.label);
>> +		device_desc.bcdDevice =
>> +			cpu_to_le16(0x0300 | 0x0099);
>> +	}
>> +
>> +	/*
>> +	 * Allocate string descriptor numbers ... note that string
>> +	 * contents can be overridden by the composite_dev glue.
>> +	 */
>> +
>> +	/* device descriptor strings: manufacturer, product */
>> +	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
>> +		init_utsname()->sysname, init_utsname()->release,
>> +		gadget->name);
>> +	status = usb_string_id(cdev);
>> +	if (status<  0)
>> +		goto fail1;
>> +	strings_dev[STRING_MANUFACTURER_IDX].id = status;
>> +	device_desc.iManufacturer = status;
>> +
>> +	status = usb_string_id(cdev);
>> +	if (status<  0)
>> +		goto fail1;
>> +	strings_dev[STRING_PRODUCT_IDX].id = status;
>> +	device_desc.iProduct = status;
>> +
>> +	/* register our configuration */
>> +	status = usb_add_config(cdev,&cdc_config_driver, cdc_do_config);
>> +	if (status<  0)
>> +		goto fail1;
>> +
>> +	dev_info(&gadget->dev, "%s, version: " DRIVER_VERSION "\n",
>> +			DRIVER_DESC);
>> +	fsg_common_put(&fsg_common);
>> +	return 0;
>> +
>> +	/* error recovery */
>> +fail1:
>> +	fsg_common_put(&fsg_common);
>> +fail0:
>> +	gserial_cleanup();
>> +	return status;
>> +}
>> +
>> +static int __exit cdc_unbind(struct usb_composite_dev *cdev)
>> +{
>> +	gserial_cleanup();
>
> shouldn't you call fsg_common_put() ??
>


This is in the function cdc_bind() before the return statement.

>> +	return 0;
>> +}
>> +
>> +static struct usb_composite_driver cdc_driver = {
>> +	.name		= "g_acm_ms",
>> +	.dev		=&device_desc,
>> +	.strings	= dev_strings,
>> +	.unbind		= __exit_p(cdc_unbind),
>> +};
>> +
>> +MODULE_DESCRIPTION(DRIVER_DESC);
>> +MODULE_AUTHOR("Klaus Schwarzkopf");
>
> add email as well:
>
> MODULE_AUTHOR("Klaus Schwarzkopf<schwarzkopf@sensortherm.de>")
>
>> +MODULE_LICENSE("GPL");
>
> should this be GPL v2 instead ?
>
>> +
>> +static int __init init(void)
>> +{
>> +	return usb_composite_probe(&cdc_driver, cdc_bind);
>
> please run a sed script changing cdc_ to acm_ms_, or something similar,
> at least.
>

OK

Thanks,

Klaus



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3] usb: add new usb gadget for ACM and mass storage
  2011-10-10  8:33     ` Klaus Schwarzkopf
@ 2011-10-10  8:49       ` Felipe Balbi
  2011-10-10 10:30         ` Klaus Schwarzkopf
  2011-10-10 15:14       ` Alan Stern
  2011-10-10 16:00       ` Michal Nazarewicz
  2 siblings, 1 reply; 32+ messages in thread
From: Felipe Balbi @ 2011-10-10  8:49 UTC (permalink / raw)
  To: Klaus Schwarzkopf; +Cc: balbi, gregkh, linux-usb, linux-kernel, sshtylyov

[-- Attachment #1: Type: text/plain, Size: 653 bytes --]

Hi,

On Mon, Oct 10, 2011 at 10:33:01AM +0200, Klaus Schwarzkopf wrote:
> >>+static struct usb_device_descriptor device_desc = {
> >>+	.bLength =		sizeof device_desc,
> >>+	.bDescriptorType =	USB_DT_DEVICE,
> >>+
> >>+	.bcdUSB =		cpu_to_le16(0x0200),
> >>+
> >>+	.bDeviceClass =		USB_CLASS_COMM,
> >>+	.bDeviceSubClass =	0,
> >>+	.bDeviceProtocol =	0,
> 
> Should bDeviceClass, bDeviceSubClass, and bDeviceProtocol have the
> same value like in the file multi.c?
> 
> 	.bDeviceClass =		USB_CLASS_MISC /* 0xEF */,
> 	.bDeviceSubClass =	2,
> 	.bDeviceProtocol =	1,

maybe, you gotta check CDC and MSC specs to find out.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3] usb: add new usb gadget for ACM and mass storage
  2011-10-10  8:49       ` Felipe Balbi
@ 2011-10-10 10:30         ` Klaus Schwarzkopf
  0 siblings, 0 replies; 32+ messages in thread
From: Klaus Schwarzkopf @ 2011-10-10 10:30 UTC (permalink / raw)
  To: balbi; +Cc: gregkh, linux-usb, linux-kernel, sshtylyov

Hi,


>> Should bDeviceClass, bDeviceSubClass, and bDeviceProtocol have the
>> same value like in the file multi.c?
>>
>> 	.bDeviceClass =		USB_CLASS_MISC /* 0xEF */,
>> 	.bDeviceSubClass =	2,
>> 	.bDeviceProtocol =	1,

 From http://www.usb.org/developers/whitepapers/iadclasscode_r10.pdf:

This set of class codes is defined as the Multi-Interface Function
Device Class Codes.

So, this are the right settings.

Regards,

Klaus

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3] usb: add new usb gadget for ACM and mass storage
  2011-10-10  8:33     ` Klaus Schwarzkopf
  2011-10-10  8:49       ` Felipe Balbi
@ 2011-10-10 15:14       ` Alan Stern
  2011-10-10 16:00       ` Michal Nazarewicz
  2 siblings, 0 replies; 32+ messages in thread
From: Alan Stern @ 2011-10-10 15:14 UTC (permalink / raw)
  To: Klaus Schwarzkopf; +Cc: balbi, gregkh, linux-usb, linux-kernel, sshtylyov

On Mon, 10 Oct 2011, Klaus Schwarzkopf wrote:

> >> +	.bDeviceClass =		USB_CLASS_COMM,
> >> +	.bDeviceSubClass =	0,
> >> +	.bDeviceProtocol =	0,
> 
> Should bDeviceClass, bDeviceSubClass, and bDeviceProtocol have the same 
> value like in the file multi.c?
> 
> 	.bDeviceClass =		USB_CLASS_MISC /* 0xEF */,
> 	.bDeviceSubClass =	2,
> 	.bDeviceProtocol =	1,

This may or may not be relevant...  The USB 2.0 spec (Table 9-8) says
that if bDeviceClass is 0 then each interface specifies its own class
information and the interfaces operate independently.  Isn't that what 
you want?

Alan Stern


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3] usb: add new usb gadget for ACM and mass storage
  2011-10-10  8:33     ` Klaus Schwarzkopf
  2011-10-10  8:49       ` Felipe Balbi
  2011-10-10 15:14       ` Alan Stern
@ 2011-10-10 16:00       ` Michal Nazarewicz
  2 siblings, 0 replies; 32+ messages in thread
From: Michal Nazarewicz @ 2011-10-10 16:00 UTC (permalink / raw)
  To: balbi, Klaus Schwarzkopf; +Cc: gregkh, linux-usb, linux-kernel, sshtylyov

On Mon, 10 Oct 2011 10:33:01 +0200, Klaus Schwarzkopf <schwarzkopf@sensortherm.de> wrote:
>> +static struct usb_device_descriptor device_desc = {
>> +	.bLength =		sizeof device_desc,
>> +	.bDescriptorType =	USB_DT_DEVICE,
>> +
>> +	.bcdUSB =		cpu_to_le16(0x0200),
>> +
>> +	.bDeviceClass =		USB_CLASS_COMM,
>> +	.bDeviceSubClass =	0,
>> +	.bDeviceProtocol =	0,

> Should bDeviceClass, bDeviceSubClass, and bDeviceProtocol have the same
> value like in the file multi.c?
>
> 	.bDeviceClass =		USB_CLASS_MISC /* 0xEF */,
> 	.bDeviceSubClass =	2,
> 	.bDeviceProtocol =	1,

IIRC, the reason multi.c has those is because Windows does not recognise
a multi-configuration device with zeros here.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4] usb: add new usb gadget for ACM and mass storage
  2011-10-10  8:32 ` [PATCH v4] " Klaus Schwarzkopf
@ 2011-10-13 17:43   ` Felipe Balbi
  0 siblings, 0 replies; 32+ messages in thread
From: Felipe Balbi @ 2011-10-13 17:43 UTC (permalink / raw)
  To: Klaus Schwarzkopf; +Cc: balbi, gregkh, linux-usb, linux-kernel, sshtylyov

[-- Attachment #1: Type: text/plain, Size: 318 bytes --]

On Mon, Oct 10, 2011 at 10:32:23AM +0200, Klaus Schwarzkopf wrote:
> This driver provides two functions in one configuration:
> a mass storage, and a ACM (serial port) link.
> Heavily based on multi.c and cdc2.c
> 
> Signed-off-by: Klaus Schwarzkopf <schwarzkopf@sensortherm.de>

applied, thanks

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2011-10-13 17:44 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-08 18:24 [PATCH] usb: add new usb gadget for ACM and mass storage Klaus Schwarzkopf
2011-09-08 19:11 ` Greg KH
2011-09-09  8:37   ` Klaus Schwarzkopf
2011-09-09 10:30     ` Michal Nazarewicz
2011-09-09 18:45     ` Greg KH
2011-09-09 10:20   ` Michal Nazarewicz
2011-09-09 18:43     ` Greg KH
2011-09-16 17:18       ` Sebastian Andrzej Siewior
2011-09-16 17:25         ` Greg KH
2011-09-16 17:30         ` Michal Nazarewicz
2011-09-16 18:22         ` Steve Calfee
2011-09-16 18:55           ` Sebastian Andrzej Siewior
2011-09-16 21:22         ` Alan Stern
2011-10-06 12:08 ` Felipe Balbi
2011-10-07  8:23   ` Klaus Schwarzkopf
2011-10-07  8:38     ` Felipe Balbi
2011-10-07 10:07       ` Klaus Schwarzkopf
2011-10-07 10:14         ` Felipe Balbi
2011-10-07 11:13           ` Sergei Shtylyov
2011-10-07 12:55             ` Felipe Balbi
2011-10-07 11:11     ` Sergei Shtylyov
2011-10-07  8:16 ` [PATCH v2] " Klaus Schwarzkopf
2011-10-07  8:39   ` Felipe Balbi
2011-10-08  7:44 ` [PATCH v3] " Klaus Schwarzkopf
2011-10-10  5:50   ` Felipe Balbi
2011-10-10  8:33     ` Klaus Schwarzkopf
2011-10-10  8:49       ` Felipe Balbi
2011-10-10 10:30         ` Klaus Schwarzkopf
2011-10-10 15:14       ` Alan Stern
2011-10-10 16:00       ` Michal Nazarewicz
2011-10-10  8:32 ` [PATCH v4] " Klaus Schwarzkopf
2011-10-13 17:43   ` Felipe Balbi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.