From: Wang Sheng Long <[email protected]>
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 <[email protected]>
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.
---
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)
+{
+ 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.
--
2.17.1
On Wed, Sep 09, 2020 at 10:39:30AM +0800, Sheng Long Wang wrote:
> From: Wang Sheng Long <[email protected]>
>
> 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 <[email protected]>
>
> 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