> -----Original Message----- > From: linux-usb-owner@xxxxxxxxxxxxxxx [mailto:linux-usb- > owner@xxxxxxxxxxxxxxx] On Behalf Of Konstantin Shkolnyy > Sent: Thursday, January 14, 2016 12:16 > To: Martyn Welch; Johan Hovold > Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Subject: RE: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register access > functions for large registers > > > -----Original Message----- > > From: Martyn Welch [mailto:martyn.welch@xxxxxxxxxxxxxxx] > > Sent: Thursday, January 14, 2016 11:52 > > To: Konstantin Shkolnyy; Johan Hovold > > Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > > Subject: Re: [PATCH v3 4/4] USB: serial: cp210x: Switch to new register > access > > functions for large registers > ... > > > > @@ -1038,27 +941,35 @@ static void cp210x_set_termios(struct > tty_struct > > *tty, > > > } > > > > > > if ((cflag & CRTSCTS) != (old_cflag & CRTSCTS)) { > > > - cp210x_get_config(port, CP210X_GET_FLOW, modem_ctl, > > 16); > > > - dev_dbg(dev, "%s - read modem controls = 0x%.4x 0x%.4x > > 0x%.4x 0x%.4x\n", > > > - __func__, modem_ctl[0], modem_ctl[1], > > > - modem_ctl[2], modem_ctl[3]); > > > + > > > + /* Only bytes 0, 4 and 7 out of first 8 have functional bits */ > > > + > > > + cp210x_read_reg_block(port, CP210X_GET_FLOW, > > modem_ctl, > > > + sizeof(modem_ctl)); > > > + dev_dbg(dev, "%s - read modem controls = %02x .. .. .. %02x > > .. .. %02x\n", > > > + __func__, modem_ctl[0], modem_ctl[4], > > modem_ctl[7]); > > > > > > if (cflag & CRTSCTS) { > > > modem_ctl[0] &= ~0x7B; > > > modem_ctl[0] |= 0x09; > > > - modem_ctl[1] = 0x80; > > > + modem_ctl[4] = 0x80; > > > + /* FIXME - why clear reserved bits just read? */ > > > + modem_ctl[5] = 0; > > > + modem_ctl[6] = 0; > > > + modem_ctl[7] = 0; > > > dev_dbg(dev, "%s - flow control = CRTSCTS\n", > > __func__); > > > } else { > > > modem_ctl[0] &= ~0x7B; > > > modem_ctl[0] |= 0x01; > > > - modem_ctl[1] |= 0x40; > > > + /* FIXME - OR here istead of assignment looks wrong > > */ > > > > s/istead/instead/ > > > > I'm a little unsure about FIXME comments being added rather than the > > issue being addressed. If I'm reading this right, then this is the > > control for the RTS/CTS lines, could the operation without these bits > > being cleared/ORed be checked by using a serial cable (connected to > > another serial port) and writing data with and without flow control > > enabled through a terminal emulator? > > The patching procedure enforced by maintainers dictates that each separate > patch addresses 1 issue. > It's much easier to review changes this way. So this particular patch just > converts from one register access function to another. > The bugfix patch will come later. > > While doing this cleanup, I also noticed another bug - this function will always > set the low 2 bits of byte 0 to 01b: "modem_ctl[0] |= 0x01". > This field is called SERIAL_DTR_MASK. It's 0 by default. ("DTR is held > inactive"). The function will only write it when CRTSCTS changes. > So the device will start with 0, then, if CRTSCTS ever changes, it'll become 1 > and stay 1 forever. Looks wrong to me. > I'm still researching the subject when and how it should be set. > > * Wikipedia: "DTR and DSR are usually on all the time and, per the > * RS-232 standard and its successors, are used to signal from each > * end that the other equipment is actually present and powered- > up." > * So perhaps DTR should be turned ON in open() and OFF in close()? And the same problem with SERIAL_RTS_MASK. Here is what this piece of code turned it into after fixing the FIXME you mentioned and assigning names to all the bits. if ((cflag & CRTSCTS) != (old_cflag & CRTSCTS)) { struct cp210x_flow_ctl flow_ctl; /* Only bytes 0, 4 and 7 out of first 8 have functional bits */ cp210x_read_reg_block(port, CP210X_GET_FLOW, &flow_ctl, sizeof(flow_ctl)); if (cflag & CRTSCTS) { /* "DTR is held active" */ flow_ctl.SERIAL_DTR_MASK = 1; /* "CTS is a handshake line" */ flow_ctl.SERIAL_CTS_HANDSHAKE = 1; flow_ctl.SERIAL_DSR_HANDSHAKE = 0; flow_ctl.SERIAL_DCD_HANDSHAKE = 0; flow_ctl.SERIAL_DSR_SENSITIVITY = 0; flow_ctl.SERIAL_AUTO_TRANSMIT = 0; flow_ctl.SERIAL_AUTO_RECEIVE = 0; flow_ctl.SERIAL_ERROR_CHAR = 0; flow_ctl.SERIAL_NULL_STRIPPING = 0; flow_ctl.SERIAL_BREAK_CHAR = 0; /* "RTS is used for receive flow control" */ flow_ctl.SERIAL_RTS_MASK = 2; dev_dbg(dev, "%s - flow control = CRTSCTS\n", __func__); } else { /* "DTR is held active" */ flow_ctl.SERIAL_DTR_MASK = 1; /* "CTS is simply a status input" */ flow_ctl.SERIAL_CTS_HANDSHAKE = 0; flow_ctl.SERIAL_DSR_HANDSHAKE = 0; flow_ctl.SERIAL_DCD_HANDSHAKE = 0; flow_ctl.SERIAL_DSR_SENSITIVITY = 0; flow_ctl.SERIAL_AUTO_TRANSMIT = 0; flow_ctl.SERIAL_AUTO_RECEIVE = 0; flow_ctl.SERIAL_ERROR_CHAR = 0; flow_ctl.SERIAL_NULL_STRIPPING = 0; flow_ctl.SERIAL_BREAK_CHAR = 0; /* "RTS is statically active" */ flow_ctl.SERIAL_RTS_MASK = 1; dev_dbg(dev, "%s - flow control = NONE\n", __func__); } flow_ctl.SERIAL_XOFF_CONTINUE = 0; cp210x_write_reg_block(port, CP210X_SET_FLOW, flow_ctl, sizeof(flow_ctl)); } -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
