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

[Loic Poulain <loic.poulain@linaro.org>]

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