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 <[email protected]> > --- > 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
