Date:	Mon, 10 Oct 2011 10:33:01 +0200
From:	Klaus Schwarzkopf <schwarzkopf@...sortherm.de>
To:	balbi@...com
CC:	gregkh@...e.de, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, sshtylyov@...mvista.com
Subject: Re: [PATCH v3] usb: add new usb gadget for ACM and mass storage

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@...sortherm.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@...sortherm.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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists