| Message ID | 20200909023931.19530-1-china_shenglong@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] usb-serial:cp210x: add support to software flow control | expand |
On Wed, Sep 09, 2020 at 10:39:30AM +0800, Sheng Long Wang wrote: > From: Wang Sheng Long <shenglong.wang.ext@siemens.com> > > When data is transmitted between two serial ports, > the phenomenon of data loss often occurs. The two kinds > of flow control commonly used in serial communication > are hardware flow control and software flow control. > > In serial communication, If you only use RX/TX/GND Pins, you > can't do hardware flow. So we often used software flow control > and prevent data loss. The user sets the software flow control > through the application program, and the application program > sets the software flow control mode for the serial port > chip through the driver. > > For the cp210 serial port chip, its driver lacks the > software flow control setting code, so the user cannot set > the software flow control function through the application > program. This adds the missing software flow control. > > Signed-off-by: Wang Sheng Long <shenglong.wang.ext@siemens.com> > > Changes in v3: > - fixed code style, It mainly adjusts the code style acccording > to kernel specification. > > Changes in v4: > - It mainly adjusts the patch based on the last usb-next branch > of the usb-serial and optimized the relevant code. "optimize code" is too hand-wavy, please be more specific on what you've changed. > --- > drivers/usb/serial/cp210x.c | 125 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 120 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c > index d0c05aa8a0d6..bcbf8da99ebb 100644 > --- a/drivers/usb/serial/cp210x.c > +++ b/drivers/usb/serial/cp210x.c > @@ -412,6 +412,15 @@ struct cp210x_comm_status { > u8 bReserved; > } __packed; > > +struct cp210x_special_chars { > + u8 bEofChar; > + u8 bErrorChar; > + u8 bBreakChar; > + u8 bEventChar; > + u8 bXonChar; > + u8 bXoffChar; > +}; > + > /* > * CP210X_PURGE - 16 bits passed in wValue of USB request. > * SiLabs app note AN571 gives a strange description of the 4 bits: > @@ -675,6 +684,69 @@ static int cp210x_read_vendor_block(struct usb_serial *serial, u8 type, u16 val, > return result; > } > > +static int cp210x_get_chars(struct usb_serial_port *port, void *buf, int bufsize) No need to pass in a length here. Just use a pointer to struct cp210x_special_chars. > +{ > + struct usb_serial *serial = port->serial; > + struct cp210x_port_private *port_priv = usb_get_serial_port_data(port); > + void *dmabuf; > + int result; > + > + dmabuf = kmemdup(buf, bufsize, GFP_KERNEL); > + if (!dmabuf) > + return -ENOMEM; > + > + result = usb_control_msg(serial->dev, > + usb_sndctrlpipe(serial->dev, 0), usb_rcvctrlpipe() > + CP210X_GET_CHARS, REQTYPE_DEVICE_TO_HOST, 0, > + port_priv->bInterfaceNumber, > + dmabuf, bufsize, USB_CTRL_SET_TIMEOUT); USB_CTRL_GET_TIMEOUT > + > + if (result == bufsize) { > + memcpy(buf, dmabuf, bufsize); > + result = 0; > + } else { > + dev_err(&port->dev, "failed get req 0x%x size %d status: %d\n", > + CP210X_GET_CHARS, bufsize, result); Just spell out "failed to get special chars: %d\n" > + if (result >= 0) > + result = -EIO; > + } > + > + kfree(dmabuf); > + > + return result; > +} > + > +static int cp210x_set_chars(struct usb_serial_port *port, void *buf, int bufsize) > +{ Same as above. > + struct usb_serial *serial = port->serial; > + struct cp210x_port_private *port_priv = usb_get_serial_port_data(port); > + void *dmabuf; > + int result; > + > + dmabuf = kmemdup(buf, bufsize, GFP_KERNEL); > + if (!dmabuf) > + return -ENOMEM; > + > + result = usb_control_msg(serial->dev, > + usb_sndctrlpipe(serial->dev, 0), > + CP210X_SET_CHARS, REQTYPE_HOST_TO_INTERFACE, 0, > + port_priv->bInterfaceNumber, > + dmabuf, bufsize, USB_CTRL_SET_TIMEOUT); > + > + if (result == bufsize) { > + result = 0; > + } else { > + dev_err(&port->dev, "failed get req 0x%x size %d status: %d\n", > + CP210X_SET_CHARS, bufsize, result); "failed to set special chars: %d\n" (and not "get"). > + if (result >= 0) > + result = -EIO; > + } > + > + kfree(dmabuf); > + > + return result; > +} > + > /* > * Writes any 16-bit CP210X_ register (req) whose value is passed > * entirely in the wValue field of the USB request. > @@ -1356,11 +1428,17 @@ static void cp210x_set_termios(struct tty_struct *tty, > struct usb_serial_port *port, struct ktermios *old_termios) > { > struct device *dev = &port->dev; > - unsigned int cflag, old_cflag; > + unsigned int cflag, old_cflag, iflag; > + struct cp210x_special_chars charsres; s/charsres/special_chars/ > + struct cp210x_flow_ctl flow_ctl; > u16 bits; > + int result; > + u32 ctl_hs; > + u32 flow_repl; > > cflag = tty->termios.c_cflag; > old_cflag = old_termios->c_cflag; > + iflag = tty->termios.c_iflag; > > if (tty->termios.c_ospeed != old_termios->c_ospeed) > cp210x_change_speed(tty, port, old_termios); > @@ -1434,10 +1512,6 @@ static void cp210x_set_termios(struct tty_struct *tty, > } > > if ((cflag & CRTSCTS) != (old_cflag & CRTSCTS)) { > - struct cp210x_flow_ctl flow_ctl; > - u32 ctl_hs; > - u32 flow_repl; > - > cp210x_read_reg_block(port, CP210X_GET_FLOW, &flow_ctl, > sizeof(flow_ctl)); > ctl_hs = le32_to_cpu(flow_ctl.ulControlHandshake); > @@ -1474,6 +1548,47 @@ static void cp210x_set_termios(struct tty_struct *tty, > sizeof(flow_ctl)); > } > > + if ((iflag & IXOFF) || (iflag & IXON)) { Try to only do this on changes (of IXON/IXOFF/START_CHAR/STOP_CHAR). > + Stray newline. > + result = cp210x_get_chars(port, &charsres, sizeof(charsres)); > + if (result < 0) > + dev_err(dev, "failed to get character: %d\n", result); Error already logged, you shouldn't proceed with set_chars if this fails. Perhaps use a helper function for settings software flow control? > + > + charsres.bXonChar = START_CHAR(tty); > + charsres.bXoffChar = STOP_CHAR(tty); > + > + result = cp210x_set_chars(port, &charsres, sizeof(charsres)); > + if (result < 0) > + dev_err(dev, "failed to set character: %d\n", result); Same here. > + > + result = cp210x_read_reg_block(port, > + CP210X_GET_FLOW, > + &flow_ctl, > + sizeof(flow_ctl)); > + if (result) > + dev_err(dev, "failed to read flow_ctl reg: %d\n", result); Don't continue updating flow control on errors. > + > + flow_repl = le32_to_cpu(flow_ctl.ulFlowReplace); > + > + if (iflag & IXOFF) > + flow_repl |= CP210X_SERIAL_AUTO_RECEIVE; > + else > + flow_repl &= ~CP210X_SERIAL_AUTO_RECEIVE; > + > + if (iflag & IXON) > + flow_repl |= CP210X_SERIAL_AUTO_TRANSMIT; > + else > + flow_repl &= ~CP210X_SERIAL_AUTO_TRANSMIT; > + > + flow_ctl.ulFlowReplace = cpu_to_le32(flow_repl); > + result = cp210x_write_reg_block(port, > + CP210X_SET_FLOW, > + &flow_ctl, > + sizeof(flow_ctl)); > + if (result) > + dev_err(dev, "failed to write flow_ctl reg: %d\n", result); > + } > + > /* > * Enable event-insertion mode only if input parity checking is > * enabled for now. Finally, this driver is a bit weird in that it retrieves the termios settings from the device on open. You need to handle IXON/IXOFF there as well for now I'm afraid. Johan
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c index d0c05aa8a0d6..bcbf8da99ebb 100644 --- a/drivers/usb/serial/cp210x.c +++ b/drivers/usb/serial/cp210x.c @@ -412,6 +412,15 @@ struct cp210x_comm_status { u8 bReserved; } __packed; +struct cp210x_special_chars { + u8 bEofChar; + u8 bErrorChar; + u8 bBreakChar; + u8 bEventChar; + u8 bXonChar; + u8 bXoffChar; +}; + /* * CP210X_PURGE - 16 bits passed in wValue of USB request. * SiLabs app note AN571 gives a strange description of the 4 bits: @@ -675,6 +684,69 @@ static int cp210x_read_vendor_block(struct usb_serial *serial, u8 type, u16 val, return result; } +static int cp210x_get_chars(struct usb_serial_port *port, void *buf, int bufsize) +{ + struct usb_serial *serial = port->serial; + struct cp210x_port_private *port_priv = usb_get_serial_port_data(port); + void *dmabuf; + int result; + + dmabuf = kmemdup(buf, bufsize, GFP_KERNEL); + if (!dmabuf) + return -ENOMEM; + + result = usb_control_msg(serial->dev, + usb_sndctrlpipe(serial->dev, 0), + CP210X_GET_CHARS, REQTYPE_DEVICE_TO_HOST, 0, + port_priv->bInterfaceNumber, + dmabuf, bufsize, USB_CTRL_SET_TIMEOUT); + + if (result == bufsize) { + memcpy(buf, dmabuf, bufsize); + result = 0; + } else { + dev_err(&port->dev, "failed get req 0x%x size %d status: %d\n", + CP210X_GET_CHARS, bufsize, result); + if (result >= 0) + result = -EIO; + } + + kfree(dmabuf); + + return result; +} + +static int cp210x_set_chars(struct usb_serial_port *port, void *buf, int bufsize) +{ + struct usb_serial *serial = port->serial; + struct cp210x_port_private *port_priv = usb_get_serial_port_data(port); + void *dmabuf; + int result; + + dmabuf = kmemdup(buf, bufsize, GFP_KERNEL); + if (!dmabuf) + return -ENOMEM; + + result = usb_control_msg(serial->dev, + usb_sndctrlpipe(serial->dev, 0), + CP210X_SET_CHARS, REQTYPE_HOST_TO_INTERFACE, 0, + port_priv->bInterfaceNumber, + dmabuf, bufsize, USB_CTRL_SET_TIMEOUT); + + if (result == bufsize) { + result = 0; + } else { + dev_err(&port->dev, "failed get req 0x%x size %d status: %d\n", + CP210X_SET_CHARS, bufsize, result); + if (result >= 0) + result = -EIO; + } + + kfree(dmabuf); + + return result; +} + /* * Writes any 16-bit CP210X_ register (req) whose value is passed * entirely in the wValue field of the USB request. @@ -1356,11 +1428,17 @@ static void cp210x_set_termios(struct tty_struct *tty, struct usb_serial_port *port, struct ktermios *old_termios) { struct device *dev = &port->dev; - unsigned int cflag, old_cflag; + unsigned int cflag, old_cflag, iflag; + struct cp210x_special_chars charsres; + struct cp210x_flow_ctl flow_ctl; u16 bits; + int result; + u32 ctl_hs; + u32 flow_repl; cflag = tty->termios.c_cflag; old_cflag = old_termios->c_cflag; + iflag = tty->termios.c_iflag; if (tty->termios.c_ospeed != old_termios->c_ospeed) cp210x_change_speed(tty, port, old_termios); @@ -1434,10 +1512,6 @@ static void cp210x_set_termios(struct tty_struct *tty, } if ((cflag & CRTSCTS) != (old_cflag & CRTSCTS)) { - struct cp210x_flow_ctl flow_ctl; - u32 ctl_hs; - u32 flow_repl; - cp210x_read_reg_block(port, CP210X_GET_FLOW, &flow_ctl, sizeof(flow_ctl)); ctl_hs = le32_to_cpu(flow_ctl.ulControlHandshake); @@ -1474,6 +1548,47 @@ static void cp210x_set_termios(struct tty_struct *tty, sizeof(flow_ctl)); } + if ((iflag & IXOFF) || (iflag & IXON)) { + + result = cp210x_get_chars(port, &charsres, sizeof(charsres)); + if (result < 0) + dev_err(dev, "failed to get character: %d\n", result); + + charsres.bXonChar = START_CHAR(tty); + charsres.bXoffChar = STOP_CHAR(tty); + + result = cp210x_set_chars(port, &charsres, sizeof(charsres)); + if (result < 0) + dev_err(dev, "failed to set character: %d\n", result); + + result = cp210x_read_reg_block(port, + CP210X_GET_FLOW, + &flow_ctl, + sizeof(flow_ctl)); + if (result) + dev_err(dev, "failed to read flow_ctl reg: %d\n", result); + + flow_repl = le32_to_cpu(flow_ctl.ulFlowReplace); + + if (iflag & IXOFF) + flow_repl |= CP210X_SERIAL_AUTO_RECEIVE; + else + flow_repl &= ~CP210X_SERIAL_AUTO_RECEIVE; + + if (iflag & IXON) + flow_repl |= CP210X_SERIAL_AUTO_TRANSMIT; + else + flow_repl &= ~CP210X_SERIAL_AUTO_TRANSMIT; + + flow_ctl.ulFlowReplace = cpu_to_le32(flow_repl); + result = cp210x_write_reg_block(port, + CP210X_SET_FLOW, + &flow_ctl, + sizeof(flow_ctl)); + if (result) + dev_err(dev, "failed to write flow_ctl reg: %d\n", result); + } + /* * Enable event-insertion mode only if input parity checking is * enabled for now.