Hi Pali, On Mon, 22 Nov 2021 at 16:48, Pali Rohár <p...@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