Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753335AbcD2NkO (ORCPT ); Fri, 29 Apr 2016 09:40:14 -0400 Received: from mail-lf0-f66.google.com ([209.85.215.66]:34937 "EHLO mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752625AbcD2NkL (ORCPT ); Fri, 29 Apr 2016 09:40:11 -0400 Date: Fri, 29 Apr 2016 15:40:12 +0200 From: Johan Hovold To: Grigori Goronzy Cc: Johan Hovold , Greg Kroah-Hartman , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4 10/13] USB: ch341: clean up messages Message-ID: <20160429134012.GN22229@localhost> References: <1460754856-27908-1-git-send-email-greg@chown.ath.cx> <1460754856-27908-11-git-send-email-greg@chown.ath.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1460754856-27908-11-git-send-email-greg@chown.ath.cx> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4683 Lines: 132 On Fri, Apr 15, 2016 at 11:14:13PM +0200, Grigori Goronzy wrote: > No functional change. Remove explicit function name printing, it's > easy to use dynamic debug to print it every time, if required. While that is true, we currently use __func__ in a lot of debug messages as a compact form for a self-contained message (e.g. "%s - x=y\n", __func__) and that should be ok. > Fix capitalization and phrasing in some cases. No need to capitalise either. I'd even prefer sticking to lower-case consistently. > Drop useless > information like a USB buffer pointer, which is not helpful. > > Signed-off-by: Grigori Goronzy > --- > drivers/usb/serial/ch341.c | 48 +++++++++++++++++++++------------------------- > 1 file changed, 22 insertions(+), 26 deletions(-) > > diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c > index f524aa9..22cfd88 100644 > --- a/drivers/usb/serial/ch341.c > +++ b/drivers/usb/serial/ch341.c > @@ -106,7 +106,7 @@ static int ch341_control_out(struct usb_device *dev, u8 request, > { > int r; > > - dev_dbg(&dev->dev, "ch341_control_out(%02x,%02x,%04x,%04x)\n", > + dev_dbg(&dev->dev, "control_out(%02x,%02x,%04x,%04x)\n", Drop this one or do use __func__ here. > USB_DIR_OUT|0x40, (int)request, (int)value, (int)index); > > r = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request, > @@ -122,8 +122,8 @@ static int ch341_control_in(struct usb_device *dev, > { > int r; > > - dev_dbg(&dev->dev, "ch341_control_in(%02x,%02x,%04x,%04x,%p,%u)\n", > - USB_DIR_IN|0x40, (int)request, (int)value, (int)index, buf, > + dev_dbg(&dev->dev, "control_in(%02x,%02x,%04x,%04x,%u)\n", > + USB_DIR_IN|0x40, (int)request, (int)value, (int)index, Ditto. > (int)bufsize); > > r = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), request, > @@ -327,11 +327,11 @@ static int ch341_open(struct tty_struct *tty, struct usb_serial_port *port) > if (tty) > ch341_set_termios(tty, port, NULL); > > - dev_dbg(&port->dev, "%s - submitting interrupt urb\n", __func__); > + dev_dbg(&port->dev, "Submitting interrupt URB\n"); Drop. > r = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL); > if (r) { > - dev_err(&port->dev, "%s - failed to submit interrupt urb: %d\n", > - __func__, r); > + dev_err(&port->dev, > + "Failed to submit interrupt URB: %d\n", r); Error message, so I agree that this should be spelled out, but not need to capitalise "failed". > goto out; > } > > @@ -409,8 +409,7 @@ static void ch341_set_termios(struct tty_struct *tty, > CH341_REG_RTSCTS | ((uint16_t)CH341_REG_RTSCTS << 8), > 0x0101); > if (r < 0) { > - dev_err(&port->dev, "%s - USB control write error (%d)\n", > - __func__, r); > + dev_err(&port->dev, "USB control write error: %d\n", r); Please say what went wrong instead of what function failed. (I think I already commented on this one when you added it, fix it at the source). > tty->termios.c_cflag &= ~CRTSCTS; > } > } > @@ -432,29 +431,27 @@ static void ch341_break_ctl(struct tty_struct *tty, int break_state) > r = ch341_control_in(port->serial->dev, CH341_REQ_READ_REG, > ch341_break_reg, 0, break_reg, 2); > if (r < 0) { > - dev_err(&port->dev, "%s - USB control read error (%d)\n", > - __func__, r); > + dev_err(&port->dev, "USB control read error: %d\n", r); "failed to read break status: %d\n", or similar. > goto out; > } > - dev_dbg(&port->dev, "%s - initial ch341 break register contents - reg1: %x, reg2: %x\n", > - __func__, break_reg[0], break_reg[1]); > + dev_dbg(&port->dev, "Initial break register contents - reg1: %x, reg2: %x\n", > + break_reg[0], break_reg[1]); __func__ is fine for compact debug messages, but you can remove the verbose text if you want. I'd prefer the form "%s - x = a, y = b\n" > if (break_state != 0) { > - dev_dbg(&port->dev, "%s - Enter break state requested\n", __func__); > + dev_dbg(&port->dev, "Enter break state requested\n"); > break_reg[0] &= ~CH341_NBREAK_BITS_REG1; > break_reg[1] &= ~CH341_LCR_ENABLE_TX; > } else { > - dev_dbg(&port->dev, "%s - Leave break state requested\n", __func__); > + dev_dbg(&port->dev, "Leave break state requested\n"); > break_reg[0] |= CH341_NBREAK_BITS_REG1; > break_reg[1] |= CH341_LCR_ENABLE_TX; > } A common "%s - break = %d\n" would do instead of the two above. > - dev_dbg(&port->dev, "%s - New ch341 break register contents - reg1: %x, reg2: %x\n", > - __func__, break_reg[0], break_reg[1]); > + dev_dbg(&port->dev, "New break register contents - reg1: %x, reg2: %x\n", > + break_reg[0], break_reg[1]); I think you get the idea, so won't comment on the rest. Thanks, Johan