Hi Pali,
On Mon, 22 Nov 2021 at 16:48, Pali Rohár pali@kernel.org wrote:
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.
Indeed, going to use it.
+};
...
+int drv_usb_acm_stdio_init(void) +{
struct stdio_dev stdio;strcpy(stdio.name, "stdio_acm");Just suggestion: What about "usbacm" or just "acm"?
usbacm sounds good.
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()?
Yes, I've named it _stdio because it's a subpart of the driver. There is the core cdc_acm function implementation and one client of this function which is stdio. But I'm fine with shortening that.
Regards, Loic