On Thu, Aug 20, 2020 at 03:52:40PM +0800, Sheng Long Wang wrote:
> From: Wang Sheng Long <shenglong.wang.ext@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
>
> Changes in v3:
> -fixed code style, It mainly adjusts the code style acccording
> to kernel specification.
> ---
> drivers/usb/serial/cp210x.c | 118 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 113 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index e732949f65..c66a0e0fb9 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -380,6 +380,9 @@ static struct usb_serial_driver * const serial_drivers[] = {
> #define CP210X_PARTNUM_CP2102N_QFN20 0x22
> #define CP210X_PARTNUM_UNKNOWN 0xFF
>
> +#define CP210X_VSTART 0x11
> +#define CP210X_VSTOP 0x13
These should come from the termios settings.
> +
> /* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
> struct cp210x_comm_status {
> __le32 ulErrors;
> @@ -391,6 +394,15 @@ struct cp210x_comm_status {
> u8 bReserved;
> } __packed;
>
> +struct cp210x_chars_response {
Please rename cp210x_special_chars.
> + u8 eofchar;
> + u8 errochar;
> + u8 breakchar;
> + u8 eventchar;
> + u8 xonchar;
> + u8 xoffchar;
Please revert to using the field names from the spec for consistency.
> +} __packed;
No need for __packed.
> +
> /*
> * CP210X_PURGE - 16 bits passed in wValue of USB request.
> * SiLabs app note AN571 gives a strange description of the 4 bits:
> @@ -624,6 +636,45 @@ static int cp210x_read_vendor_block(struct usb_serial *serial, u8 type, u16 val,
> return result;
> }
>
> +/*
> + * Read and Write Character Responses operate
> + * Register SET_CHARS/GET_CHATS
This comment makes very little sense to me. Please fix up (including
typos) or drop.
> + */
> +static int cp210x_operate_chars_block(struct usb_serial_port *port,
> + u8 req, u8 type, 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_rcvctrlpipe(serial->dev, 0),
This should be usb_sndctrlpipe() when updating the settings.
> + req, type, 0, port_priv->bInterfaceNumber,
> + dmabuf, bufsize, USB_CTRL_SET_TIMEOUT);
> +
> + if (result == bufsize) {
> + if (type == REQTYPE_DEVICE_TO_HOST)
> + memcpy(buf, dmabuf, bufsize);
> +
> + result = 0;
> + } else {
> + dev_err(&port->dev, "failed get req 0x%x size %d status: %d\n",
> + req, bufsize, result);
> + if (result >= 0)
> + result = -EIO;
> +
Spurious newline.
> + }
> +
> + kfree(dmabuf);
> +
> + return result;
> +}
Replace this with two dedicated helpers cp210x_get_chars() and
cp210x_set_chars().
> +
> /*
> * Writes any 16-bit CP210X_ register (req) whose value is passed
> * entirely in the wValue field of the USB request.
> @@ -1134,11 +1185,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;
> + struct cp210x_chars_response charsres;
> + struct cp210x_flow_ctl flow_ctl;
> + unsigned int cflag, old_cflag, iflag;
> 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);
> @@ -1212,10 +1269,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);
> @@ -1252,6 +1305,61 @@ static void cp210x_set_termios(struct tty_struct *tty,
> sizeof(flow_ctl));
> }
>
> + /*
> + * Set Software Flow Control
> + * Check the IXOFF/IXON status in the iflag component of the
> + * termios structure.
> + *
> + */
Drop this comment.
> + if ((iflag & IXOFF) || (iflag & IXON)) {
> +
> + result = cp210x_operate_chars_block(port,
> + CP210X_GET_CHARS,
> + REQTYPE_DEVICE_TO_HOST,
> + &charsres,
> + sizeof(charsres));
> +
> + if (result < 0) {
> + dev_err(dev, "Read Characrter Responses failed\n");
Please run your patch through a spell checker. And no need for Capital
letters everywhere.
> + return;
You shouldn't just return here (more important with latest driver).
> + }
> + charsres.xonchar = CP210X_VSTART;
> + charsres.xoffchar = CP210X_VSTOP;
These should come from termios (see START_CHAR() and STOP_CHAR()).
> + result = cp210x_operate_chars_block(port,
> + CP210X_SET_CHARS,
> + REQTYPE_HOST_TO_INTERFACE,
> + &charsres,
> + sizeof(charsres));
> + if (result < 0) {
> + memset(&charsres, 0, sizeof(charsres));
Not needed.
> + dev_err(dev, "Write Characrter Responses failed\n");
> + return;
> + }
> +
> + /*Set Rx/Tx Flow Contrl Flag in ulFlowReplace*/
Spaces after and before *. Random spaces mid sentence. Capitalisation...
And generally not useful, please drop it.
> + cp210x_read_reg_block(port,
> + CP210X_GET_FLOW,
> + &flow_ctl,
> + sizeof(flow_ctl));
Error handling.
> +
> + 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);
> + cp210x_write_reg_block(port,
> + CP210X_SET_FLOW,
> + &flow_ctl,
> + sizeof(flow_ctl));
Error handling?
> + }
> }
>
> static int cp210x_tiocmset(struct tty_struct *tty,
Johan
