Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755338AbbKRKpE (ORCPT ); Wed, 18 Nov 2015 05:45:04 -0500 Received: from mail-yk0-f176.google.com ([209.85.160.176]:36857 "EHLO mail-yk0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751736AbbKRKpA convert rfc822-to-8bit (ORCPT ); Wed, 18 Nov 2015 05:45:00 -0500 MIME-Version: 1.0 In-Reply-To: References: Date: Wed, 18 Nov 2015 18:44:59 +0800 Message-ID: Subject: Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port From: Baolin Wang To: Andy Shevchenko Cc: Felipe Balbi , Greg Kroah-Hartman , r.baldyga@samsung.com, fabio.estevam@freescale.com, Philip Oberstaller , Peter Hurley , scottwood@freescale.com, Mark Brown , USB , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3784 Lines: 122 On 18 November 2015 at 17:32, Andy Shevchenko wrote: > On Wed, Nov 18, 2015 at 4:15 AM, Baolin Wang wrote: >> On 17 November 2015 at 21:34, Andy Shevchenko wrote: >>> On Mon, Nov 16, 2015 at 9:05 AM, Baolin Wang 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/