On 18 November 2015 at 17:32, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > On Wed, Nov 18, 2015 at 4:15 AM, Baolin Wang <baolin.wang@xxxxxxxxxx> wrote: >> On 17 November 2015 at 21:34, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: >>> On Mon, Nov 16, 2015 at 9:05 AM, Baolin Wang <baolin.wang@xxxxxxxxxx> wrote: >>>> It dose not work when we want to use the usb-to-serial port based >>>> on one usb gadget as a console. Thus this patch adds the console >>>> initialization to support this request. >>> > >>>> +#define GS_BUFFER_SIZE (4096) >>> Redundant parens >> OK. I'll remove it. >> >>>> +#define GS_CONSOLE_BUF_SIZE (2 * GS_BUFFER_SIZE) >>>> + >>>> +struct gscons_info { >>>> + struct gs_port *port; >>>> + struct tty_driver *tty_driver; >>>> + struct work_struct work; >>>> + int buf_tail; >>>> + char buf[GS_CONSOLE_BUF_SIZE]; >>> >>> Can't be malloced once? >> The 'gscons_info' structure is malloced once. > > In state of high fragmentation is quite hard to find big memory chunks. > I would split it to two allocations, though if maintainers are okay > with your code, then I'm also okay. > Make sense. But I think the major memory of the 'struct gscons_info' is for the 'buf' member, so I still think no need to allocate it 2 times. >>>> +static struct usb_request *gs_request_new(struct usb_ep *ep, int buffer_size) >>>> +{ >>>> + struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC); >>>> + >>>> + if (!req) >>> >>> For sake of readability it's better to have assignment explicitly before 'if'. >> >> But I think it is very easy to understand the assignment here with >> saving code lines. > > It's not a function of couple of lines, so, for me makes sense to > explicitly put the assignment here. Especially that one that does > allocations (for pointer arithmetic I could agree to place the > assignment in the definition block). > OK. Sounds reasonable. >>>> +static void gs_complete_out(struct usb_ep *ep, struct usb_request *req) >>>> +{ >>>> + if (req->status != 0 && req->status != -ECONNRESET) >>>> + return; >>> >>> Something missed here. Currently it's no-op. >>> >> >> Yeah. I didn't realize what need to do in the callback here, so just >> leave a callback without anything. But maybe something will be added >> if there are some requirements in future. > > if () > .. > > will be optimized away, why not to remove it? OK. I'll remove it. > >>>> + port = ports[port_num].port; >>>> + if (!port) { >>>> + pr_err("%s: serial line [%d] not allocated.\n", >>>> + __func__, port_num); >>>> + return -ENODEV; >>>> + } >>>> + >>>> + if (!port->port_usb) { >>>> + pr_err("%s: no port usb.\n", __func__); >>> >>> Starting from here could it be dev_err and so on? >> >> There are no dev_err things and device things in this file, so pr_xxx >> is more reasonable. > > This is understandable, but if in case you have device in place why > not to use its name? Yes, that's right. > >>>> + pr_debug("%s: port[%d] console connect!\n", __func__, port_num); >>> >>> Dynamic debug will add function name if asked. >> >> Sorry, I didn't get your point, you mean print the function name is >> redundant here? > > Right. > > Just pr_debug("port[%d] …", …); > OK. Very thanks for your suggestions. > -- > With Best Regards, > Andy Shevchenko -- Baolin.wang Best Regards -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html