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 <g...@chown.ath.cx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
