From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754439AbcDYKQi (ORCPT ); Mon, 25 Apr 2016 06:16:38 -0400 Received: from mail-lf0-f67.google.com ([209.85.215.67]:33316 "EHLO mail-lf0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753875AbcDYKQg (ORCPT ); Mon, 25 Apr 2016 06:16:36 -0400 Date: Mon, 25 Apr 2016 12:16:35 +0200 From: Johan Hovold To: Konstantin Shkolnyy Cc: johan@kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH RESEND 3/5] USB: serial: cp210x: Added comments to CRTSCT flag code. Message-ID: <20160425101635.GI18866@localhost> References: <1461517761-6240-1-git-send-email-konstantin.shkolnyy@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1461517761-6240-1-git-send-email-konstantin.shkolnyy@gmail.com> 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 On Sun, Apr 24, 2016 at 12:09:21PM -0500, Konstantin Shkolnyy wrote: > Documented "magic numbers" used in the CRTSCT flag code in terms of > register bit names from the chip specification. Documenting these is long overdue. I even started adding defines just to be able to review the first patch in the series. > Signed-off-by: Konstantin Shkolnyy > --- > drivers/usb/serial/cp210x.c | 38 ++++++++++++++++++++++++++++++++++---- > 1 file changed, 34 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c > index ede5c52..b2321a7 100644 > --- a/drivers/usb/serial/cp210x.c > +++ b/drivers/usb/serial/cp210x.c > @@ -790,7 +790,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port, > > cp210x_read_reg_block(port, CP210X_GET_FLOW, modem_ctl, > sizeof(modem_ctl)); > - if (modem_ctl[0] & 0x08) { > + if (modem_ctl[0] & 0x08) { /* if SERIAL_CTS_HANDSHAKE */ But instead of adding comments like this one and below, please add proper defines for the various flags and masks. That will make the code mostly self-explanatory. > dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__); > cflag |= CRTSCTS; > } else { > @@ -952,17 +952,47 @@ static void cp210x_set_termios(struct tty_struct *tty, > __func__, modem_ctl[0], modem_ctl[4], modem_ctl[7]); > > if (cflag & CRTSCTS) { > - modem_ctl[0] &= ~0x7B; > + modem_ctl[0] &= ~0x7B; /* keep reserved D2 and D7 */ > + /* > + * D1-D0 SERIAL_DTR_MASK =01b: DTR is held active > + * D3 SERIAL_CTS_HANDSHAKE =1: CTS is a handshake line > + * D4 SERIAL_DSR_HANDSHAKE =0 > + * D5 SERIAL_DCD_HANDSHAKE =0 > + * D6 SERIAL_DSR_SENSITIVITY =0 > + */ > - modem_ctl[7] = 0; > + modem_ctl[7] = 0; /* SERIAL_XOFF_CONTINUE = 0 */ And this would be better as a bit operation as well (as already mentioned). Thanks, Johan