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.
Changes in v5:
- Fixes:
* According to the cp210x specification, use usb_control_msg()
requesttype 'REQTYPE_DEVICE_TO_HOST' is modified to
'REQTYPE_INTERFACE_TO_HOST' in cp210x_get_chars().
* If modify IXOFF/IXON has been changed, we can call set software
flow control code.
* If the setting software flow control wrong, do not continue
processing proceed with updating software flow control.
Changes in v6:
- Fix 'result' variable not uninitialized warning in cp210x_set_termios().
---
drivers/usb/serial/cp210x.c | 128 ++++++++++++++++++++++++++++++++++--
1 file changed, 123 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index d0c05aa8a0d6..2d5e31282599 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,70 @@ 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)
+{
+ struct usb_serial *serial = port->serial;
+ struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+ struct cp210x_special_chars *special_chars;
+ void *dmabuf;
+ int result;
+
+ dmabuf = kmemdup(buf, sizeof(*special_chars), GFP_KERNEL);
+ if (!dmabuf)
+ return -ENOMEM;
+
+ result = usb_control_msg(serial->dev,
+ usb_rcvctrlpipe(serial->dev, 0),
+ CP210X_GET_CHARS, REQTYPE_INTERFACE_TO_HOST, 0,
+ port_priv->bInterfaceNumber,
+ dmabuf, sizeof(*special_chars),
+ USB_CTRL_GET_TIMEOUT);
+
+ if (result == sizeof(*special_chars)) {
+ memcpy(buf, dmabuf, sizeof(*special_chars));
+ result = 0;
+ } else {
+ dev_err(&port->dev, "failed to get special chars: %d\n", result);
+ if (result >= 0)
+ result = -EIO;
+ }
+
+ kfree(dmabuf);
+
+ return result;
+}
+
+static int cp210x_set_chars(struct usb_serial_port *port, void *buf)
+{
+ struct usb_serial *serial = port->serial;
+ struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+ struct cp210x_special_chars *special_chars;
+ void *dmabuf;
+ int result;
+
+ dmabuf = kmemdup(buf, sizeof(*special_chars), 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, sizeof(*special_chars), USB_CTRL_SET_TIMEOUT);
+
+ if (result == sizeof(*special_chars)) {
+ result = 0;
+ } else {
+ dev_err(&port->dev, "failed to set special chars: %d\n", 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 +1429,18 @@ 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, old_iflag;
+ struct cp210x_special_chars special_chars;
+ struct cp210x_flow_ctl flow_ctl;
u16 bits;
+ int result = 0;
+ u32 ctl_hs;
+ u32 flow_repl;
cflag = tty->termios.c_cflag;
+ iflag = tty->termios.c_iflag;
old_cflag = old_termios->c_cflag;
+ old_iflag = old_termios->c_iflag;
if (tty->termios.c_ospeed != old_termios->c_ospeed)
cp210x_change_speed(tty, port, old_termios);
@@ -1434,10 +1514,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 +1550,48 @@ static void cp210x_set_termios(struct tty_struct *tty,
sizeof(flow_ctl));
}
+ if (((iflag & IXOFF) != (old_iflag & IXOFF)) ||
+ ((iflag & IXON) != (old_iflag & IXON))) {
+ result = cp210x_get_chars(port, &special_chars);
+ if (result < 0)
+ goto out;
+
+ special_chars.bXonChar = START_CHAR(tty);
+ special_chars.bXoffChar = STOP_CHAR(tty);
+
+ result = cp210x_set_chars(port, &special_chars);
+ if (result < 0)
+ goto out;
+
+ result = cp210x_read_reg_block(port,
+ CP210X_GET_FLOW,
+ &flow_ctl,
+ sizeof(flow_ctl));
+ if (result < 0)
+ goto out;
+
+ 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));
+ }
+out:
+ if (result < 0)
+ dev_err(dev, "failed to set software flow control: %d\n", result);
+
/*
* Enable event-insertion mode only if input parity checking is
* enabled for now.
--
2.17.1
On Fri, Oct 16, 2020 at 10:24:28AM +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.
Again, you did a whole lot more than just rebase here based on the
review feedback you got.
> Changes in v5:
> - Fixes:
> * According to the cp210x specification, use usb_control_msg()
> requesttype 'REQTYPE_DEVICE_TO_HOST' is modified to
> 'REQTYPE_INTERFACE_TO_HOST' in cp210x_get_chars().
>
> * If modify IXOFF/IXON has been changed, we can call set software
> flow control code.
>
> * If the setting software flow control wrong, do not continue
> processing proceed with updating software flow control.
>
> Changes in v6:
> - Fix 'result' variable not uninitialized warning in cp210x_set_termios().
> ---
> drivers/usb/serial/cp210x.c | 128 ++++++++++++++++++++++++++++++++++--
> 1 file changed, 123 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index d0c05aa8a0d6..2d5e31282599 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,70 @@ 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)
As I said earlier, these functions should take a pointer to a struct
special_chars (not void *).
> +{
> + struct usb_serial *serial = port->serial;
> + struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> + struct cp210x_special_chars *special_chars;
> + void *dmabuf;
> + int result;
> +
> + dmabuf = kmemdup(buf, sizeof(*special_chars), GFP_KERNEL);
> + if (!dmabuf)
> + return -ENOMEM;
> +
> + result = usb_control_msg(serial->dev,
> + usb_rcvctrlpipe(serial->dev, 0),
> + CP210X_GET_CHARS, REQTYPE_INTERFACE_TO_HOST, 0,
> + port_priv->bInterfaceNumber,
> + dmabuf, sizeof(*special_chars),
> + USB_CTRL_GET_TIMEOUT);
> +
> + if (result == sizeof(*special_chars)) {
> + memcpy(buf, dmabuf, sizeof(*special_chars));
> + result = 0;
> + } else {
> + dev_err(&port->dev, "failed to get special chars: %d\n", result);
> + if (result >= 0)
> + result = -EIO;
> + }
> +
> + kfree(dmabuf);
> +
> + return result;
> +}
> +
> +static int cp210x_set_chars(struct usb_serial_port *port, void *buf)
> +{
> + struct usb_serial *serial = port->serial;
> + struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> + struct cp210x_special_chars *special_chars;
> + void *dmabuf;
> + int result;
> +
> + dmabuf = kmemdup(buf, sizeof(*special_chars), 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, sizeof(*special_chars), USB_CTRL_SET_TIMEOUT);
> +
> + if (result == sizeof(*special_chars)) {
> + result = 0;
> + } else {
> + dev_err(&port->dev, "failed to set special chars: %d\n", 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 +1429,18 @@ 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, old_iflag;
> + struct cp210x_special_chars special_chars;
> + struct cp210x_flow_ctl flow_ctl;
> u16 bits;
> + int result = 0;
> + u32 ctl_hs;
> + u32 flow_repl;
>
> cflag = tty->termios.c_cflag;
> + iflag = tty->termios.c_iflag;
> old_cflag = old_termios->c_cflag;
> + old_iflag = old_termios->c_iflag;
>
> if (tty->termios.c_ospeed != old_termios->c_ospeed)
> cp210x_change_speed(tty, port, old_termios);
> @@ -1434,10 +1514,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 +1550,48 @@ static void cp210x_set_termios(struct tty_struct *tty,
> sizeof(flow_ctl));
> }
>
> + if (((iflag & IXOFF) != (old_iflag & IXOFF)) ||
> + ((iflag & IXON) != (old_iflag & IXON))) {
You need to check if START_CHAR or STOP_CHAR has changed too.
> + result = cp210x_get_chars(port, &special_chars);
> + if (result < 0)
> + goto out;
> +
> + special_chars.bXonChar = START_CHAR(tty);
> + special_chars.bXoffChar = STOP_CHAR(tty);
> +
> + result = cp210x_set_chars(port, &special_chars);
> + if (result < 0)
> + goto out;
> +
> + result = cp210x_read_reg_block(port,
> + CP210X_GET_FLOW,
> + &flow_ctl,
> + sizeof(flow_ctl));
> + if (result < 0)
> + goto out;
> +
> + 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));
> + }
> +out:
> + if (result < 0)
> + dev_err(dev, "failed to set software flow control: %d\n", result);
> +
Ok, this works, but it's weird with a label named "out" in the middle of
a function. Please handle software flow control in a helper function as
I suggested.
> /*
> * Enable event-insertion mode only if input parity checking is
> * enabled for now.
Also, you didn't address my final comment:
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.
Without that bit, output flow control (IXON, which is set in termios by
default) will not be enabled in the device until it's disabled and
re-enabled.
I'll try to find some time to rip that bit out of the driver, but for
now you need to handle also IXON/IXOFF in cp210x_get_termios().
Johan
Hi, Johan
Thank you very much for your reply!
You mean if we call cp210x_open() When opening the device, because IXON is set by default, the
cp210x_get_termios() does not process IXON, So it is invalid IXON at this time.
As you said, It is very strange in cp210x_get_termios() In the "get" function to "set" IXON.
In addition, the best way is to disable the IXON bit as you said. If the user needs IXON, call set_ termios function,
So I'm now in cp210x_get_termios() Is it a temporary solution for terminos to handle IXON ?
I'm afraid it will need to be adjusted.
BR/ShengLong
-----Original Message-----
From: Johan Hovold <[email protected]>
Sent: Friday, November 13, 2020 11:28 PM
To: Sheng Long Wang <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Wang, Sheng Long (EXT) (RC-CN DI FA BL IPC&C PRC4) <[email protected]>
Subject: Re: [PATCH v6] usb-serial:cp210x: add support to software flow control
On Fri, Oct 16, 2020 at 10:24:28AM +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.
Again, you did a whole lot more than just rebase here based on the review feedback you got.
> Changes in v5:
> - Fixes:
> * According to the cp210x specification, use usb_control_msg()
> requesttype 'REQTYPE_DEVICE_TO_HOST' is modified to
> 'REQTYPE_INTERFACE_TO_HOST' in cp210x_get_chars().
>
> * If modify IXOFF/IXON has been changed, we can call set software
> flow control code.
>
> * If the setting software flow control wrong, do not continue
> processing proceed with updating software flow control.
>
> Changes in v6:
> - Fix 'result' variable not uninitialized warning in cp210x_set_termios().
> ---
> drivers/usb/serial/cp210x.c | 128
> ++++++++++++++++++++++++++++++++++--
> 1 file changed, 123 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index d0c05aa8a0d6..2d5e31282599 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,70 @@ 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)
As I said earlier, these functions should take a pointer to a struct special_chars (not void *).
> +{
> + struct usb_serial *serial = port->serial;
> + struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> + struct cp210x_special_chars *special_chars;
> + void *dmabuf;
> + int result;
> +
> + dmabuf = kmemdup(buf, sizeof(*special_chars), GFP_KERNEL);
> + if (!dmabuf)
> + return -ENOMEM;
> +
> + result = usb_control_msg(serial->dev,
> + usb_rcvctrlpipe(serial->dev, 0),
> + CP210X_GET_CHARS, REQTYPE_INTERFACE_TO_HOST, 0,
> + port_priv->bInterfaceNumber,
> + dmabuf, sizeof(*special_chars),
> + USB_CTRL_GET_TIMEOUT);
> +
> + if (result == sizeof(*special_chars)) {
> + memcpy(buf, dmabuf, sizeof(*special_chars));
> + result = 0;
> + } else {
> + dev_err(&port->dev, "failed to get special chars: %d\n", result);
> + if (result >= 0)
> + result = -EIO;
> + }
> +
> + kfree(dmabuf);
> +
> + return result;
> +}
> +
> +static int cp210x_set_chars(struct usb_serial_port *port, void *buf)
> +{
> + struct usb_serial *serial = port->serial;
> + struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
> + struct cp210x_special_chars *special_chars;
> + void *dmabuf;
> + int result;
> +
> + dmabuf = kmemdup(buf, sizeof(*special_chars), 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, sizeof(*special_chars), USB_CTRL_SET_TIMEOUT);
> +
> + if (result == sizeof(*special_chars)) {
> + result = 0;
> + } else {
> + dev_err(&port->dev, "failed to set special chars: %d\n", 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 +1429,18 @@ 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, old_iflag;
> + struct cp210x_special_chars special_chars;
> + struct cp210x_flow_ctl flow_ctl;
> u16 bits;
> + int result = 0;
> + u32 ctl_hs;
> + u32 flow_repl;
>
> cflag = tty->termios.c_cflag;
> + iflag = tty->termios.c_iflag;
> old_cflag = old_termios->c_cflag;
> + old_iflag = old_termios->c_iflag;
>
> if (tty->termios.c_ospeed != old_termios->c_ospeed)
> cp210x_change_speed(tty, port, old_termios); @@ -1434,10 +1514,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 +1550,48 @@ static void cp210x_set_termios(struct tty_struct *tty,
> sizeof(flow_ctl));
> }
>
> + if (((iflag & IXOFF) != (old_iflag & IXOFF)) ||
> + ((iflag & IXON) != (old_iflag & IXON))) {
You need to check if START_CHAR or STOP_CHAR has changed too.
> + result = cp210x_get_chars(port, &special_chars);
> + if (result < 0)
> + goto out;
> +
> + special_chars.bXonChar = START_CHAR(tty);
> + special_chars.bXoffChar = STOP_CHAR(tty);
> +
> + result = cp210x_set_chars(port, &special_chars);
> + if (result < 0)
> + goto out;
> +
> + result = cp210x_read_reg_block(port,
> + CP210X_GET_FLOW,
> + &flow_ctl,
> + sizeof(flow_ctl));
> + if (result < 0)
> + goto out;
> +
> + 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));
> + }
> +out:
> + if (result < 0)
> + dev_err(dev, "failed to set software flow control: %d\n", result);
> +
Ok, this works, but it's weird with a label named "out" in the middle of a function. Please handle software flow control in a helper function as I suggested.
> /*
> * Enable event-insertion mode only if input parity checking is
> * enabled for now.
Also, you didn't address my final comment:
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.
Without that bit, output flow control (IXON, which is set in termios by
default) will not be enabled in the device until it's disabled and re-enabled.
I'll try to find some time to rip that bit out of the driver, but for now you need to handle also IXON/IXOFF in cp210x_get_termios().
Johan
[ Please avoid top posting. ]
On Mon, Nov 16, 2020 at 07:56:10AM +0000, Wang, Sheng Long wrote:
> Hi, Johan
>
> Thank you very much for your reply!
>
> You mean if we call cp210x_open() When opening the device, because
> IXON is set by default, the cp210x_get_termios() does not process
> IXON, So it is invalid IXON at this time.
Right, with the current implementation you need to make sure that
termios reflects the device state on open or your changes will never
actually enable software flow control in the device.
> As you said, It is very strange in cp210x_get_termios() In the "get"
> function to "set" IXON. In addition, the best way is to disable the
> IXON bit as you said. If the user needs IXON, call set_ termios
> function, So I'm now in cp210x_get_termios() Is it a temporary
> solution for terminos to handle IXON ? I'm afraid it will need to be
> adjusted.
No, I didn't mean that IXON should be disabled by default. I meant that
the driver should make sure that the device settings matches termios on
open, not the other way round.
This unusual implementation has caused a number of issues in the past
and it's been on my list fix up for some time. I finally got around to
that today and I just CCed you on the result. That should simplify
adding software flow control and allow more code to be shared with the
hardware flow-control handling.
I've pushed a branch for you here:
https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=cp210x-termios
Johan
Hi, Johan
Do I add my software flow control patch directly to the branch you gave me now ?
https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=cp210x-termios
Then, I also need the cp210x_ get_ terminus() add ixoff / iXon handling?
Thanks!
With best regards,
Wang Sheng Long
Siemens Ltd., China
RC-CN DI FA R&D SW
Tianyuan road No.99
611731 CHENGDU, China
Mobil: +86 15281074996
mailto:[email protected]
http://www.siemens.com/ingenuityforlife
-----Original Message-----
From: Johan Hovold <[email protected]>
Sent: Tuesday, November 17, 2020 12:38 AM
To: Wang, Sheng Long (EXT) (RC-CN DI FA BL IPC&C PRC4) <[email protected]>
Cc: Johan Hovold <[email protected]>; Sheng Long Wang <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v6] usb-serial:cp210x: add support to software flow control
[ Please avoid top posting. ]
On Mon, Nov 16, 2020 at 07:56:10AM +0000, Wang, Sheng Long wrote:
> Hi, Johan
>
> Thank you very much for your reply!
>
> You mean if we call cp210x_open() When opening the device, because
> IXON is set by default, the cp210x_get_termios() does not process
> IXON, So it is invalid IXON at this time.
Right, with the current implementation you need to make sure that termios reflects the device state on open or your changes will never actually enable software flow control in the device.
> As you said, It is very strange in cp210x_get_termios() In the "get"
> function to "set" IXON. In addition, the best way is to disable the
> IXON bit as you said. If the user needs IXON, call set_ termios
> function, So I'm now in cp210x_get_termios() Is it a temporary
> solution for terminos to handle IXON ? I'm afraid it will need to be
> adjusted.
No, I didn't mean that IXON should be disabled by default. I meant that the driver should make sure that the device settings matches termios on open, not the other way round.
This unusual implementation has caused a number of issues in the past and it's been on my list fix up for some time. I finally got around to that today and I just CCed you on the result. That should simplify adding software flow control and allow more code to be shared with the hardware flow-control handling.
I've pushed a branch for you here:
https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=cp210x-termios
Johan
[ Again, please do not top-post. Quoting Greg:
A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
A: No.
Q: Should I include quotations after my reply?
http://daringfireball.net/2007/07/on_top
]
On Mon, Nov 23, 2020 at 01:11:24AM +0000, Wang, Sheng Long wrote:
> Hi, Johan
>
> Do I add my software flow control patch directly to the branch you
> gave me now ?
> https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=cp210x-termios
Yes, please rebase your work on top of that branch. I'll merge it to my
usb-next branch before applying your patch.
> Then, I also need the cp210x_ get_ terminus() add ixoff / iXon handling?
No, if you rebase your work on top of the above branch, you won't have
to deal with cp210x_get_termios() which has now been removed.
Just add support for software flow control to the new
cp210x_set_flow_control() function (and cp210x_termios_change()).
I think you can even drop get_special_chars() and just leave the rest
set as NUL, which appears to be the default.
Johan