[PATCH v2 2/2] usb: gadget: Add CDC ACM function

[Pali Rohár]

Hello! I have just few notes, see below.

> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
> index f560068..d5d891b 100644
> --- a/drivers/usb/gadget/Makefile
> +++ b/drivers/usb/gadget/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_USB_FUNCTION_MASS_STORAGE) += f_mass_storage.o
>  obj-$(CONFIG_USB_FUNCTION_FASTBOOT) += f_fastboot.o
>  obj-$(CONFIG_USB_FUNCTION_SDP) += f_sdp.o
>  obj-$(CONFIG_USB_FUNCTION_ROCKUSB) += f_rockusb.o
> +obj-$(CONFIG_USB_FUNCTION_ACM)	+= f_acm.o
>  endif
>  endif
>  ifdef CONFIG_USB_ETHER
> diff --git a/drivers/usb/gadget/f_acm.c b/drivers/usb/gadget/f_acm.c
> new file mode 100644
> index 0000000..3ea3b4a
> --- /dev/null
> +++ b/drivers/usb/gadget/f_acm.c
...
> +static struct usb_cdc_header_desc acm_header_desc = {
> +	.bLength =		sizeof(acm_header_desc),
> +	.bDescriptorType =	USB_DT_CS_INTERFACE,
> +	.bDescriptorSubType =	USB_CDC_HEADER_TYPE,
> +	.bcdCDC =		cpu_to_le16(0x0110),

This is global structure initialization. So should not it use
__constant_cpu_to_le16() macro instead of cpu_to_le16()? I guess that on
armel or x86 is cpu_to_le16() just identity macro, but on big endian
systems it is needed to use constant initialization. Other gadget
drivers are using those __constant_cpu* macros.

> +};
...
> +int drv_usb_acm_stdio_init(void)
> +{
> +	struct stdio_dev stdio;
> +
> +	strcpy(stdio.name, "stdio_acm");

Just suggestion: What about "usbacm" or just "acm"?

> +	stdio.flags = DEV_FLAGS_INPUT | DEV_FLAGS_OUTPUT;
> +	stdio.tstc = acm_stdio_tstc;
> +	stdio.getc = acm_stdio_getc;
> +	stdio.putc = acm_stdio_putc;
> +	stdio.puts = acm_stdio_puts;
> +	stdio.start = acm_stdio_start;
> +	stdio.stop = acm_stdio_stop;
> +	stdio.priv = NULL;
> +	stdio.ext = 0;
> +
> +	return stdio_register(&stdio);
> +}
> diff --git a/include/stdio_dev.h b/include/stdio_dev.h
> index 8fb9a12..d584793 100644
> --- a/include/stdio_dev.h
> +++ b/include/stdio_dev.h
> @@ -103,6 +103,7 @@ int drv_lcd_init(void);
>  int drv_video_init(void);
>  int drv_keyboard_init(void);
>  int drv_usbtty_init(void);
> +int drv_usb_acm_stdio_init(void);

Other functions do not have stdio in name too. So what about shorting
it? E.g. just drv_usbacm_init()?

>  int drv_nc_init(void);
>  int drv_jtag_console_init(void);
>  int cbmemc_init(void);
> -- 
> 2.7.4
>