drivers/usb/serial/ch341.c | 41 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
This adds support for enabling and disabling
RTS/CTS hardware flow control.
Tested using a CH340E in combination with a CP2102.
Fixes part of the following bug report:
Link: https://bugzilla.kernel.org/show_bug.cgi?id=197109
Signed-off-by: Lode Willems <me@lodewillems.com>
---
drivers/usb/serial/ch341.c | 41 ++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index d10e4c4848a0..62237f657320 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -63,6 +63,7 @@
#define CH341_REG_DIVISOR 0x13
#define CH341_REG_LCR 0x18
#define CH341_REG_LCR2 0x25
+#define CH341_REG_FLOW_CTL 0x27
#define CH341_NBREAK_BITS 0x01
@@ -77,6 +78,9 @@
#define CH341_LCR_CS6 0x01
#define CH341_LCR_CS5 0x00
+#define CH341_FLOW_CTL_NONE 0x000
+#define CH341_FLOW_CTL_RTSCTS 0x101
+
#define CH341_QUIRK_LIMITED_PRESCALER BIT(0)
#define CH341_QUIRK_SIMULATE_BREAK BIT(1)
@@ -478,6 +482,41 @@ static int ch341_open(struct tty_struct *tty, struct usb_serial_port *port)
return r;
}
+static void ch341_set_flow_control(struct tty_struct *tty,
+ struct usb_serial_port *port,
+ const struct ktermios *old_termios)
+{
+ int r;
+
+ if (old_termios &&
+ C_CRTSCTS(tty) == (old_termios->c_cflag & CRTSCTS))
+ return;
+
+ if (C_CRTSCTS(tty)) {
+ r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
+ CH341_REG_FLOW_CTL |
+ ((u16)CH341_REG_FLOW_CTL << 8),
+ CH341_FLOW_CTL_RTSCTS);
+ if (r < 0) {
+ dev_err(&port->dev,
+ "%s - failed to enable flow control: %d\n",
+ __func__, r);
+ tty->termios.c_cflag &= ~CRTSCTS;
+ }
+ } else {
+ r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
+ CH341_REG_FLOW_CTL |
+ ((u16)CH341_REG_FLOW_CTL << 8),
+ CH341_FLOW_CTL_NONE);
+ if (r < 0) {
+ dev_err(&port->dev,
+ "%s - failed to disable flow control: %d\n",
+ __func__, r);
+ tty->termios.c_cflag |= CRTSCTS;
+ }
+ }
+}
+
/* Old_termios contains the original termios settings and
* tty->termios contains the new setting to be used.
*/
@@ -546,6 +585,8 @@ static void ch341_set_termios(struct tty_struct *tty,
spin_unlock_irqrestore(&priv->lock, flags);
ch341_set_handshake(port->serial->dev, priv->mcr);
+
+ ch341_set_flow_control(tty, port, old_termios);
}
/*
--
2.47.0On Wed, Nov 13, 2024 at 07:08:27PM +0100, Lode Willems wrote:
> This adds support for enabling and disabling
> RTS/CTS hardware flow control.
> Tested using a CH340E in combination with a CP2102.
>
> Fixes part of the following bug report:
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=197109
>
> Signed-off-by: Lode Willems <me@lodewillems.com>
Thanks for the patch and sorry about the late feedback on this one. I
wanted to give it a spin with the devices I have here (ch340g and
ch341a).
Appears to the modem control lines are not wired up on the ch341a
evaluation board I have, but the device accepts the request and stops
transmitting with hardware flow control enabled.
With ch340g in loopback, I also see it refuse to transmit unless cts is
asserted. I was not able to get the device to deassert rts when
attempting to fill its receive buffers, however. Perhaps the hardware
does not support this, but is this something you tried?
> ---
> drivers/usb/serial/ch341.c | 41 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index d10e4c4848a0..62237f657320 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -63,6 +63,7 @@
> #define CH341_REG_DIVISOR 0x13
> #define CH341_REG_LCR 0x18
> #define CH341_REG_LCR2 0x25
> +#define CH341_REG_FLOW_CTL 0x27
>
> #define CH341_NBREAK_BITS 0x01
>
> @@ -77,6 +78,9 @@
> #define CH341_LCR_CS6 0x01
> #define CH341_LCR_CS5 0x00
>
> +#define CH341_FLOW_CTL_NONE 0x000
> +#define CH341_FLOW_CTL_RTSCTS 0x101
The registers are eight bits wide, but the driver writes two at a time.
So this should just be 0x00 and 0x01.
> +
> #define CH341_QUIRK_LIMITED_PRESCALER BIT(0)
> #define CH341_QUIRK_SIMULATE_BREAK BIT(1)
>
> @@ -478,6 +482,41 @@ static int ch341_open(struct tty_struct *tty, struct usb_serial_port *port)
> return r;
> }
>
> +static void ch341_set_flow_control(struct tty_struct *tty,
> + struct usb_serial_port *port,
> + const struct ktermios *old_termios)
> +{
> + int r;
> +
> + if (old_termios &&
> + C_CRTSCTS(tty) == (old_termios->c_cflag & CRTSCTS))
> + return;
Just drop this and set the requested setting unconditionally.
> +
> + if (C_CRTSCTS(tty)) {
> + r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
> + CH341_REG_FLOW_CTL |
> + ((u16)CH341_REG_FLOW_CTL << 8),
> + CH341_FLOW_CTL_RTSCTS);
> + if (r < 0) {
> + dev_err(&port->dev,
> + "%s - failed to enable flow control: %d\n",
> + __func__, r);
> + tty->termios.c_cflag &= ~CRTSCTS;
> + }
> + } else {
> + r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
> + CH341_REG_FLOW_CTL |
> + ((u16)CH341_REG_FLOW_CTL << 8),
> + CH341_FLOW_CTL_NONE);
> + if (r < 0) {
> + dev_err(&port->dev,
> + "%s - failed to disable flow control: %d\n",
> + __func__, r);
> + tty->termios.c_cflag |= CRTSCTS;
> + }
> + }
Please rewrite this so that you prepare the value and index parameters
based on the termios settings and then do one control request. If it
fails you can restore the old setting (if old_termios is non-NULL).
And please drop the redundant __func__ from the error message (even if
the driver still uses it in some other functions still).
> +}
> +
Johan
On 14/12/2024 18:21, Johan Hovold wrote:
> On Wed, Nov 13, 2024 at 07:08:27PM +0100, Lode Willems wrote:
>> This adds support for enabling and disabling
>> RTS/CTS hardware flow control.
>> Tested using a CH340E in combination with a CP2102.
>>
>> Fixes part of the following bug report:
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=197109
>>
>> Signed-off-by: Lode Willems <me@lodewillems.com>
>
> Thanks for the patch and sorry about the late feedback on this one. I
> wanted to give it a spin with the devices I have here (ch340g and
> ch341a).
Thanks for the review. Sorry in advance if this e-mail is formatted
incorrectly, this is my first time replaying to a review.
>
> Appears to the modem control lines are not wired up on the ch341a
> evaluation board I have, but the device accepts the request and stops
> transmitting with hardware flow control enabled.
>
Since creating this patch I've acquired a ch341a breakout board and can
confirm that it transmits with hardware flow control enabled and CTS
asserted.
> With ch340g in loopback, I also see it refuse to transmit unless cts is
> asserted. I was not able to get the device to deassert rts when
> attempting to fill its receive buffers, however. Perhaps the hardware
> does not support this, but is this something you tried?
>
I didn't try this before. Just trying a couple of things now I also
couldn't seem to make it deassert RTS, but this may be a failure of
how I'm testing this.
>> ---
>> drivers/usb/serial/ch341.c | 41 ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 41 insertions(+)
>>
>> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
>> index d10e4c4848a0..62237f657320 100644
>> --- a/drivers/usb/serial/ch341.c
>> +++ b/drivers/usb/serial/ch341.c
>> @@ -63,6 +63,7 @@
>> #define CH341_REG_DIVISOR 0x13
>> #define CH341_REG_LCR 0x18
>> #define CH341_REG_LCR2 0x25
>> +#define CH341_REG_FLOW_CTL 0x27
>>
>> #define CH341_NBREAK_BITS 0x01
>>
>> @@ -77,6 +78,9 @@
>> #define CH341_LCR_CS6 0x01
>> #define CH341_LCR_CS5 0x00
>>
>> +#define CH341_FLOW_CTL_NONE 0x000
>> +#define CH341_FLOW_CTL_RTSCTS 0x101
>
> The registers are eight bits wide, but the driver writes two at a time.
> So this should just be 0x00 and 0x01.
Ok. I'm guessing I don't have to send the same request twice and can
just leave the top eight bits of the value and index empty?
It seems to work in the quick testing I've done.
>
>> +
>> #define CH341_QUIRK_LIMITED_PRESCALER BIT(0)
>> #define CH341_QUIRK_SIMULATE_BREAK BIT(1)
>>
>> @@ -478,6 +482,41 @@ static int ch341_open(struct tty_struct *tty, struct usb_serial_port *port)
>> return r;
>> }
>>
>> +static void ch341_set_flow_control(struct tty_struct *tty,
>> + struct usb_serial_port *port,
>> + const struct ktermios *old_termios)
>> +{
>> + int r;
>> +
>> + if (old_termios &&
>> + C_CRTSCTS(tty) == (old_termios->c_cflag & CRTSCTS))
>> + return;
>
> Just drop this and set the requested setting unconditionally.
Ok
>
>> +
>> + if (C_CRTSCTS(tty)) {
>> + r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
>> + CH341_REG_FLOW_CTL |
>> + ((u16)CH341_REG_FLOW_CTL << 8),
>> + CH341_FLOW_CTL_RTSCTS);
>> + if (r < 0) {
>> + dev_err(&port->dev,
>> + "%s - failed to enable flow control: %d\n",
>> + __func__, r);
>> + tty->termios.c_cflag &= ~CRTSCTS;
>> + }
>> + } else {
>> + r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
>> + CH341_REG_FLOW_CTL |
>> + ((u16)CH341_REG_FLOW_CTL << 8),
>> + CH341_FLOW_CTL_NONE);
>> + if (r < 0) {
>> + dev_err(&port->dev,
>> + "%s - failed to disable flow control: %d\n",
>> + __func__, r);
>> + tty->termios.c_cflag |= CRTSCTS;
>> + }
>> + }
>
> Please rewrite this so that you prepare the value and index parameters
> based on the termios settings and then do one control request. If it
> fails you can restore the old setting (if old_termios is non-NULL).
Ok, I haven't seen any other driver restore the old setting on request
failure, now I'm questioning if it's actually necessary?
If it is, I'll change it to the following:
tty->termios.c_cflag = (tty->termios.c_cflag & !CRTSCTS)
| (old_termios->c_cflag & CRTSCTS);
>
> And please drop the redundant __func__ from the error message (even if
> the driver still uses it in some other functions still).
Ok. Looking at the code again, the error already gets logged in
ch341_control_out on failure. Maybe this log line shouldn't be added?
>
>> +}
>> +
>
> Johan
Lode
On Tue, Dec 17, 2024 at 09:08:23PM +0100, Lode Willems wrote:
> On 14/12/2024 18:21, Johan Hovold wrote:
> > On Wed, Nov 13, 2024 at 07:08:27PM +0100, Lode Willems wrote:
> >> This adds support for enabling and disabling
> >> RTS/CTS hardware flow control.
> >> Tested using a CH340E in combination with a CP2102.
> >>
> >> Fixes part of the following bug report:
> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=197109
> >>
> >> Signed-off-by: Lode Willems <me@lodewillems.com>
> >
> > Thanks for the patch and sorry about the late feedback on this one. I
> > wanted to give it a spin with the devices I have here (ch340g and
> > ch341a).
>
> Thanks for the review. Sorry in advance if this e-mail is formatted
> incorrectly, this is my first time replaying to a review.
Looks all good.
> > Appears to the modem control lines are not wired up on the ch341a
> > evaluation board I have, but the device accepts the request and stops
> > transmitting with hardware flow control enabled.
>
> Since creating this patch I've acquired a ch341a breakout board and can
> confirm that it transmits with hardware flow control enabled and CTS
> asserted.
Thanks for checking.
> > With ch340g in loopback, I also see it refuse to transmit unless cts is
> > asserted. I was not able to get the device to deassert rts when
> > attempting to fill its receive buffers, however. Perhaps the hardware
> > does not support this, but is this something you tried?
>
> I didn't try this before. Just trying a couple of things now I also
> couldn't seem to make it deassert RTS, but this may be a failure of
> how I'm testing this.
Or it's a limitation of the hardware (e.g. it really only support
auto-cts). I patched the driver to not submit any read urbs, which
should eventually fill up the receive buffers.
> >> +#define CH341_FLOW_CTL_NONE 0x000
> >> +#define CH341_FLOW_CTL_RTSCTS 0x101
> >
> > The registers are eight bits wide, but the driver writes two at a time.
> > So this should just be 0x00 and 0x01.
>
> Ok. I'm guessing I don't have to send the same request twice and can
> just leave the top eight bits of the value and index empty?
> It seems to work in the quick testing I've done.
Possibly, but writing the same register twice is what the vendor driver
is doing here (e.g. otherwise you may actually write to offset 0 as
well).
> >> +static void ch341_set_flow_control(struct tty_struct *tty,
> >> + struct usb_serial_port *port,
> >> + const struct ktermios *old_termios)
> >> +{
> >> + int r;
> >> +
> >> + if (old_termios &&
> >> + C_CRTSCTS(tty) == (old_termios->c_cflag & CRTSCTS))
> >> + return;
> >
> > Just drop this and set the requested setting unconditionally.
>
> Ok
>
> >
> >> +
> >> + if (C_CRTSCTS(tty)) {
> >> + r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
> >> + CH341_REG_FLOW_CTL |
> >> + ((u16)CH341_REG_FLOW_CTL << 8),
> >> + CH341_FLOW_CTL_RTSCTS);
> >> + if (r < 0) {
> >> + dev_err(&port->dev,
> >> + "%s - failed to enable flow control: %d\n",
> >> + __func__, r);
> >> + tty->termios.c_cflag &= ~CRTSCTS;
> >> + }
> >> + } else {
> >> + r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
> >> + CH341_REG_FLOW_CTL |
> >> + ((u16)CH341_REG_FLOW_CTL << 8),
> >> + CH341_FLOW_CTL_NONE);
> >> + if (r < 0) {
> >> + dev_err(&port->dev,
> >> + "%s - failed to disable flow control: %d\n",
> >> + __func__, r);
> >> + tty->termios.c_cflag |= CRTSCTS;
> >> + }
> >> + }
> >
> > Please rewrite this so that you prepare the value and index parameters
> > based on the termios settings and then do one control request. If it
> > fails you can restore the old setting (if old_termios is non-NULL).
>
> Ok, I haven't seen any other driver restore the old setting on request
> failure, now I'm questioning if it's actually necessary?
Not all drivers do, but it is the right thing to do as that's the only
way we have to report failure to change a setting.
> If it is, I'll change it to the following:
> tty->termios.c_cflag = (tty->termios.c_cflag & !CRTSCTS)
You meant bitwise negation here (~) as you used above.
> | (old_termios->c_cflag & CRTSCTS);
But remember to only do this if old_termios is non-NULL.
Might be cleaner as two separate statements (&= and |=).
> > And please drop the redundant __func__ from the error message (even if
> > the driver still uses it in some other functions still).
>
> Ok. Looking at the code again, the error already gets logged in
> ch341_control_out on failure. Maybe this log line shouldn't be added?
Yeah, I guess that's more in line with what the driver is currently
doing. We could always amend those printk to include index and value to
make it easier to figure out which call is failing if this becomes an
issue.
Johan
© 2016 - 2026 Red Hat, Inc.