Hi,
here's a revised v3 of my ch341 patchset sent earlier this month.
Thanks to everyone who provided feedback. Changes compared to the
last submission have been indicated in the commit messages.
Please review.
Best regards
Grigori
Use the correct types and sizes.
v2: use u8 shorthand for unsigned char.
Signed-off-by: Grigori Goronzy <[email protected]>
---
drivers/usb/serial/ch341.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index db4b561..95c8a40 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -115,7 +115,7 @@ static int ch341_control_out(struct usb_device *dev, u8 request,
static int ch341_control_in(struct usb_device *dev,
u8 request, u16 value, u16 index,
- char *buf, unsigned bufsize)
+ u8 *buf, unsigned bufsize)
{
int r;
@@ -168,9 +168,9 @@ static int ch341_set_handshake(struct usb_device *dev, u8 control)
static int ch341_get_status(struct usb_device *dev, struct ch341_private *priv)
{
- char *buffer;
+ unsigned char *buffer;
int r;
- const unsigned size = 8;
+ const unsigned size = 2;
unsigned long flags;
buffer = kmalloc(size, GFP_KERNEL);
@@ -198,9 +198,9 @@ out: kfree(buffer);
static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
{
- char *buffer;
+ unsigned char *buffer;
int r;
- const unsigned size = 8;
+ const unsigned size = 2;
buffer = kmalloc(size, GFP_KERNEL);
if (!buffer)
--
1.9.1
BREAK2 seems to be a misnomer, the register configures various aspects
of the UART configuration.
Signed-off-by: Grigori Goronzy <[email protected]>
---
drivers/usb/serial/ch341.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 63df8ce..1ab4384 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -64,10 +64,19 @@
#define CH341_REQ_WRITE_REG 0x9A
#define CH341_REQ_READ_REG 0x95
#define CH341_REG_BREAK1 0x05
-#define CH341_REG_BREAK2 0x18
+#define CH341_REG_LCR 0x18
#define CH341_NBREAK_BITS_REG1 0x01
-#define CH341_NBREAK_BITS_REG2 0x40
+#define CH341_LCR_ENABLE_RX 0x80
+#define CH341_LCR_ENABLE_TX 0x40
+#define CH341_LCR_MARK_SPACE 0x20
+#define CH341_LCR_PAR_EVEN 0x10
+#define CH341_LCR_ENABLE_PAR 0x08
+#define CH341_LCR_STOP_BITS_2 0x04
+#define CH341_LCR_CS8 0x03
+#define CH341_LCR_CS7 0x02
+#define CH341_LCR_CS6 0x01
+#define CH341_LCR_CS5 0x00
static const struct usb_device_id id_table[] = {
{ USB_DEVICE(0x4348, 0x5523) },
@@ -370,7 +379,7 @@ static void ch341_set_termios(struct tty_struct *tty,
static void ch341_break_ctl(struct tty_struct *tty, int break_state)
{
const uint16_t ch341_break_reg =
- CH341_REG_BREAK1 | ((uint16_t) CH341_REG_BREAK2 << 8);
+ CH341_REG_BREAK1 | ((uint16_t) CH341_REG_LCR << 8);
struct usb_serial_port *port = tty->driver_data;
int r;
uint16_t reg_contents;
@@ -392,11 +401,11 @@ static void ch341_break_ctl(struct tty_struct *tty, int break_state)
if (break_state != 0) {
dev_dbg(&port->dev, "%s - Enter break state requested\n", __func__);
break_reg[0] &= ~CH341_NBREAK_BITS_REG1;
- break_reg[1] &= ~CH341_NBREAK_BITS_REG2;
+ break_reg[1] &= ~CH341_LCR_ENABLE_TX;
} else {
dev_dbg(&port->dev, "%s - Leave break state requested\n", __func__);
break_reg[0] |= CH341_NBREAK_BITS_REG1;
- break_reg[1] |= CH341_NBREAK_BITS_REG2;
+ break_reg[1] |= CH341_LCR_ENABLE_TX;
}
dev_dbg(&port->dev, "%s - New ch341 break register contents - reg1: %x, reg2: %x\n",
__func__, break_reg[0], break_reg[1]);
--
1.9.1
No functional change. The following adjustments were made to be more in
line with official coding style and to be more consistent.
Stop mixing tabs and spaces for alignment. Stop putting labels and
statements into the same line. Use braces consistently for a single
statement.
v2: drop most changes, particularly indentation changes.
Signed-off-by: Grigori Goronzy <[email protected]>
---
drivers/usb/serial/ch341.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 135370b..2d28556 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -133,7 +133,7 @@ static int ch341_control_in(struct usb_device *dev,
}
static int ch341_init_set_baudrate(struct usb_device *dev,
- struct ch341_private *priv, unsigned ctrl)
+ struct ch341_private *priv, unsigned ctrl)
{
short a;
int r;
@@ -187,10 +187,12 @@ static int ch341_get_status(struct usb_device *dev, struct ch341_private *priv)
spin_lock_irqsave(&priv->lock, flags);
priv->line_status = (~(*buffer)) & CH341_BITS_MODEM_STAT;
spin_unlock_irqrestore(&priv->lock, flags);
- } else
+ } else {
r = -EPROTO;
+ }
-out: kfree(buffer);
+out:
+ kfree(buffer);
return r;
}
@@ -241,7 +243,8 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
/* expect 0x9f 0xee */
r = ch341_get_status(dev, priv);
-out: kfree(buffer);
+out:
+ kfree(buffer);
return r;
}
@@ -265,7 +268,8 @@ static int ch341_port_probe(struct usb_serial_port *port)
usb_set_serial_port_data(port, priv);
return 0;
-error: kfree(priv);
+error:
+ kfree(priv);
return r;
}
@@ -479,7 +483,7 @@ static int ch341_tiocmset(struct tty_struct *tty,
}
static void ch341_update_line_status(struct usb_serial_port *port,
- unsigned char *data, size_t len)
+ unsigned char *data, size_t len)
{
struct ch341_private *priv = usb_get_serial_port_data(port);
struct tty_struct *tty;
@@ -600,7 +604,7 @@ static struct usb_serial_driver ch341_device = {
.id_table = id_table,
.num_ports = 1,
.open = ch341_open,
- .dtr_rts = ch341_dtr_rts,
+ .dtr_rts = ch341_dtr_rts,
.carrier_raised = ch341_carrier_raised,
.close = ch341_close,
.set_termios = ch341_set_termios,
--
1.9.1
Changing the LCR register after initialization does not seem to be
reliable on all chips (particularly not on CH341A). Restructure
initialization and configuration to always reinit the chip on
configuration changes instead and pass the LCR register value directly
to the initialization command.
v2: get rid of unused variable, improve error handling.
Tested-by: Ryan Barber <[email protected]>
Signed-off-by: Grigori Goronzy <[email protected]>
---
drivers/usb/serial/ch341.c | 47 ++++++++++++++++++++++++----------------------
1 file changed, 25 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 95c8a40..6181616 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -61,6 +61,8 @@
* the Net/FreeBSD uchcom.c driver by Takanori Watanabe. Domo arigato.
*/
+#define CH341_SERIAL_INIT 0xA1
+#define CH341_VERSION 0x5F
#define CH341_MODEM_CTRL 0xA4
#define CH341_REQ_WRITE_REG 0x9A
#define CH341_REQ_READ_REG 0x95
@@ -129,10 +131,10 @@ static int ch341_control_in(struct usb_device *dev,
return r;
}
-static int ch341_set_baudrate(struct usb_device *dev,
- struct ch341_private *priv)
+static int ch341_init_set_baudrate(struct usb_device *dev,
+ struct ch341_private *priv, unsigned ctrl)
{
- short a, b;
+ short a;
int r;
unsigned long factor;
short divisor;
@@ -152,11 +154,8 @@ static int ch341_set_baudrate(struct usb_device *dev,
factor = 0x10000 - factor;
a = (factor & 0xff00) | divisor;
- b = factor & 0xff;
- r = ch341_control_out(dev, 0x9a, 0x1312, a);
- if (!r)
- r = ch341_control_out(dev, 0x9a, 0x0f2c, b);
+ r = ch341_control_out(dev, CH341_SERIAL_INIT, 0x9c | (ctrl << 8), a | 0x80);
return r;
}
@@ -177,7 +176,7 @@ static int ch341_get_status(struct usb_device *dev, struct ch341_private *priv)
if (!buffer)
return -ENOMEM;
- r = ch341_control_in(dev, 0x95, 0x0706, 0, buffer, size);
+ r = ch341_control_in(dev, CH341_REQ_READ_REG, 0x0706, 0, buffer, size);
if (r < 0)
goto out;
@@ -207,24 +206,20 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
return -ENOMEM;
/* expect two bytes 0x27 0x00 */
- r = ch341_control_in(dev, 0x5f, 0, 0, buffer, size);
+ r = ch341_control_in(dev, CH341_VERSION, 0, 0, buffer, size);
if (r < 0)
goto out;
- r = ch341_control_out(dev, 0xa1, 0, 0);
- if (r < 0)
- goto out;
-
- r = ch341_set_baudrate(dev, priv);
+ r = ch341_control_out(dev, CH341_SERIAL_INIT, 0, 0);
if (r < 0)
goto out;
/* expect two bytes 0x56 0x00 */
- r = ch341_control_in(dev, 0x95, 0x2518, 0, buffer, size);
+ r = ch341_control_in(dev, CH341_REQ_READ_REG, 0x2518, 0, buffer, size);
if (r < 0)
goto out;
- r = ch341_control_out(dev, 0x9a, 0x2518, 0x0050);
+ r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x2518, 0x00c3);
if (r < 0)
goto out;
@@ -233,11 +228,7 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
if (r < 0)
goto out;
- r = ch341_control_out(dev, 0xa1, 0x501f, 0xd90a);
- if (r < 0)
- goto out;
-
- r = ch341_set_baudrate(dev, priv);
+ r = ch341_init_set_baudrate(dev, priv, 0);
if (r < 0)
goto out;
@@ -352,16 +343,28 @@ static void ch341_set_termios(struct tty_struct *tty,
struct ch341_private *priv = usb_get_serial_port_data(port);
unsigned baud_rate;
unsigned long flags;
+ unsigned char ctrl;
+ int r;
+
+ /* redundant changes may cause the chip to lose bytes */
+ if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
+ return;
baud_rate = tty_get_baud_rate(tty);
priv->baud_rate = baud_rate;
+ ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX | CH341_LCR_CS8;
+
if (baud_rate) {
spin_lock_irqsave(&priv->lock, flags);
priv->line_control |= (CH341_BIT_DTR | CH341_BIT_RTS);
spin_unlock_irqrestore(&priv->lock, flags);
- ch341_set_baudrate(port->serial->dev, priv);
+ r = ch341_init_set_baudrate(port->serial->dev, priv, ctrl);
+ if (r < 0 && old_termios) {
+ priv->baud_rate = tty_termios_baud_rate(old_termios);
+ tty_termios_copy_hw(&tty->termios, old_termios);
+ }
} else {
spin_lock_irqsave(&priv->lock, flags);
priv->line_control &= ~(CH341_BIT_DTR | CH341_BIT_RTS);
--
1.9.1
No functional change. Remove explicit function name printing, it's
easy to use dynamic debug to print it every time, if required.
Fix capitalization and phrasing in some cases. Drop useless
information like a USB buffer pointer, which is not helpful.
Signed-off-by: Grigori Goronzy <[email protected]>
---
drivers/usb/serial/ch341.c | 48 +++++++++++++++++++++-------------------------
1 file changed, 22 insertions(+), 26 deletions(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 2d28556..bf256eb 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -106,7 +106,7 @@ static int ch341_control_out(struct usb_device *dev, u8 request,
{
int r;
- dev_dbg(&dev->dev, "ch341_control_out(%02x,%02x,%04x,%04x)\n",
+ dev_dbg(&dev->dev, "control_out(%02x,%02x,%04x,%04x)\n",
USB_DIR_OUT|0x40, (int)request, (int)value, (int)index);
r = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request,
@@ -122,8 +122,8 @@ static int ch341_control_in(struct usb_device *dev,
{
int r;
- dev_dbg(&dev->dev, "ch341_control_in(%02x,%02x,%04x,%04x,%p,%u)\n",
- USB_DIR_IN|0x40, (int)request, (int)value, (int)index, buf,
+ dev_dbg(&dev->dev, "control_in(%02x,%02x,%04x,%04x,%u)\n",
+ USB_DIR_IN|0x40, (int)request, (int)value, (int)index,
(int)bufsize);
r = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), request,
@@ -327,11 +327,11 @@ static int ch341_open(struct tty_struct *tty, struct usb_serial_port *port)
if (tty)
ch341_set_termios(tty, port, NULL);
- dev_dbg(&port->dev, "%s - submitting interrupt urb\n", __func__);
+ dev_dbg(&port->dev, "Submitting interrupt URB\n");
r = usb_submit_urb(port->interrupt_in_urb, GFP_KERNEL);
if (r) {
- dev_err(&port->dev, "%s - failed to submit interrupt urb: %d\n",
- __func__, r);
+ dev_err(&port->dev,
+ "Failed to submit interrupt URB: %d\n", r);
goto out;
}
@@ -409,8 +409,7 @@ static void ch341_set_termios(struct tty_struct *tty,
CH341_REG_RTSCTS | ((uint16_t)CH341_REG_RTSCTS << 8),
0x0101);
if (r < 0) {
- dev_err(&port->dev, "%s - USB control write error (%d)\n",
- __func__, r);
+ dev_err(&port->dev, "USB control write error: %d\n", r);
tty->termios.c_cflag &= ~CRTSCTS;
}
}
@@ -432,29 +431,27 @@ static void ch341_break_ctl(struct tty_struct *tty, int break_state)
r = ch341_control_in(port->serial->dev, CH341_REQ_READ_REG,
ch341_break_reg, 0, break_reg, 2);
if (r < 0) {
- dev_err(&port->dev, "%s - USB control read error (%d)\n",
- __func__, r);
+ dev_err(&port->dev, "USB control read error: %d\n", r);
goto out;
}
- dev_dbg(&port->dev, "%s - initial ch341 break register contents - reg1: %x, reg2: %x\n",
- __func__, break_reg[0], break_reg[1]);
+ dev_dbg(&port->dev, "Initial break register contents - reg1: %x, reg2: %x\n",
+ break_reg[0], break_reg[1]);
if (break_state != 0) {
- dev_dbg(&port->dev, "%s - Enter break state requested\n", __func__);
+ dev_dbg(&port->dev, "Enter break state requested\n");
break_reg[0] &= ~CH341_NBREAK_BITS_REG1;
break_reg[1] &= ~CH341_LCR_ENABLE_TX;
} else {
- dev_dbg(&port->dev, "%s - Leave break state requested\n", __func__);
+ dev_dbg(&port->dev, "Leave break state requested\n");
break_reg[0] |= CH341_NBREAK_BITS_REG1;
break_reg[1] |= CH341_LCR_ENABLE_TX;
}
- dev_dbg(&port->dev, "%s - New ch341 break register contents - reg1: %x, reg2: %x\n",
- __func__, break_reg[0], break_reg[1]);
+ dev_dbg(&port->dev, "New break register contents - reg1: %x, reg2: %x\n",
+ break_reg[0], break_reg[1]);
reg_contents = get_unaligned_le16(break_reg);
r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
ch341_break_reg, reg_contents);
if (r < 0)
- dev_err(&port->dev, "%s - USB control write error (%d)\n",
- __func__, r);
+ dev_err(&port->dev, "USB control write error: %d\n", r);
out:
kfree(break_reg);
}
@@ -502,7 +499,7 @@ static void ch341_update_line_status(struct usb_serial_port *port,
spin_unlock_irqrestore(&priv->lock, flags);
if (data[1] & CH341_MULT_STAT)
- dev_dbg(&port->dev, "%s - multiple status change\n", __func__);
+ dev_dbg(&port->dev, "Multiple status change\n");
if (!delta)
return;
@@ -541,12 +538,12 @@ static void ch341_read_int_callback(struct urb *urb)
case -ENOENT:
case -ESHUTDOWN:
/* this urb is terminated, clean up */
- dev_dbg(&urb->dev->dev, "%s - urb shutting down: %d\n",
- __func__, urb->status);
+ dev_dbg(&urb->dev->dev, "URB shutting down: %d\n",
+ urb->status);
return;
default:
- dev_dbg(&urb->dev->dev, "%s - nonzero urb status: %d\n",
- __func__, urb->status);
+ dev_dbg(&urb->dev->dev, "Nonzero URB status: %d\n",
+ urb->status);
goto exit;
}
@@ -555,8 +552,7 @@ static void ch341_read_int_callback(struct urb *urb)
exit:
status = usb_submit_urb(urb, GFP_ATOMIC);
if (status) {
- dev_err(&urb->dev->dev, "%s - usb_submit_urb failed: %d\n",
- __func__, status);
+ dev_err(&urb->dev->dev, "URB submit failed: %d\n", status);
}
}
@@ -581,7 +577,7 @@ static int ch341_tiocmget(struct tty_struct *tty)
| ((status & CH341_BIT_RI) ? TIOCM_RI : 0)
| ((status & CH341_BIT_DCD) ? TIOCM_CD : 0);
- dev_dbg(&port->dev, "%s - result = %x\n", __func__, result);
+ dev_dbg(&port->dev, "Result = %x\n", result);
return result;
}
--
1.9.1
The status bit was found with USB captures of the Windows driver and
some luck. Tested on CH340G and CH341A.
v2: unify general status definitions
Signed-off-by: Grigori Goronzy <[email protected]>
---
drivers/usb/serial/ch341.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 451fa64..000d391 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -39,9 +39,6 @@
/* third irq byte base 0x94 + below */
/* fourth irq byte normally 0xee */
-/* second interrupt byte */
-#define CH341_MULT_STAT 0x04 /* multiple status since last interrupt event */
-
/* status returned in third interrupt answer byte, inverted in data
from irq */
#define CH341_BIT_CTS 0x01
@@ -81,6 +78,10 @@
#define CH341_LCR_CS6 0x01
#define CH341_LCR_CS5 0x00
+/* General status from register 0x07 and second interrupt byte */
+#define CH341_STATUS_TXBUSY 0x01
+#define CH341_STATUS_MULTI 0x04
+
static const struct usb_device_id id_table[] = {
{ USB_DEVICE(0x4348, 0x5523) },
{ USB_DEVICE(0x1a86, 0x7523) },
@@ -94,6 +95,7 @@ struct ch341_private {
unsigned baud_rate; /* set baud rate */
u8 line_control; /* set line control value RTS/DTR */
u8 line_status; /* active status of modem control inputs */
+ u8 uart_status; /* generic UART status bits */
};
static void ch341_set_termios(struct tty_struct *tty,
@@ -184,7 +186,8 @@ static int ch341_get_status(struct usb_device *dev, struct ch341_private *priv)
if (r == 2) {
r = 0;
spin_lock_irqsave(&priv->lock, flags);
- priv->line_status = (~(*buffer)) & CH341_BITS_MODEM_STAT;
+ priv->line_status = (~buffer[0]) & CH341_BITS_MODEM_STAT;
+ priv->uart_status = buffer[1];
spin_unlock_irqrestore(&priv->lock, flags);
} else {
r = -EPROTO;
@@ -195,6 +198,18 @@ out:
return r;
}
+static bool ch341_tx_empty(struct usb_serial_port *port)
+{
+ int r;
+ struct ch341_private *priv = usb_get_serial_port_data(port);
+
+ r = ch341_get_status(port->serial->dev, priv);
+ if (r < 0)
+ return true;
+
+ return !(priv->uart_status & CH341_STATUS_TXBUSY);
+}
+
/* -------------------------------------------------------------------------- */
static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
@@ -493,7 +508,7 @@ static void ch341_update_line_status(struct usb_serial_port *port,
priv->line_status = status;
spin_unlock_irqrestore(&priv->lock, flags);
- if (data[1] & CH341_MULT_STAT)
+ if (data[1] & CH341_STATUS_MULTI)
dev_dbg(&port->dev, "Multiple status change\n");
if (!delta)
@@ -599,6 +614,7 @@ static struct usb_serial_driver ch341_device = {
.carrier_raised = ch341_carrier_raised,
.close = ch341_close,
.set_termios = ch341_set_termios,
+ .tx_empty = ch341_tx_empty,
.break_ctl = ch341_break_ctl,
.tiocmget = ch341_tiocmget,
.tiocmset = ch341_tiocmset,
--
1.9.1
Check for B0 in a more idiomatic way and make sure to not enable
RTS/CTS hardware flow control in B0 as it may override the control
lines. Also make sure to only enable RTS/DTR if there's a transition
from B0.
v2: use c_cflag macros.
Signed-off-by: Grigori Goronzy <[email protected]>
---
drivers/usb/serial/ch341.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index bf256eb..78590b7 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -387,10 +387,12 @@ static void ch341_set_termios(struct tty_struct *tty,
if (C_CSTOPB(tty))
ctrl |= CH341_LCR_STOP_BITS_2;
- if (priv->baud_rate) {
- spin_lock_irqsave(&priv->lock, flags);
- priv->line_control |= (CH341_BIT_DTR | CH341_BIT_RTS);
- spin_unlock_irqrestore(&priv->lock, flags);
+ if (C_BAUD(tty) != B0) {
+ if (old_termios && (old_termios->c_cflag & CBAUD) == B0) {
+ spin_lock_irqsave(&priv->lock, flags);
+ priv->line_control |= (CH341_BIT_DTR | CH341_BIT_RTS);
+ spin_unlock_irqrestore(&priv->lock, flags);
+ }
r = ch341_init_set_baudrate(port->serial->dev, priv, ctrl);
if (r < 0 && old_termios) {
priv->baud_rate = tty_termios_baud_rate(old_termios);
@@ -404,7 +406,7 @@ static void ch341_set_termios(struct tty_struct *tty,
ch341_set_handshake(port->serial->dev, priv->line_control);
- if (cflag & CRTSCTS) {
+ if (C_CRTSCTS(tty) && C_BAUD(tty) != B0) {
r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
CH341_REG_RTSCTS | ((uint16_t)CH341_REG_RTSCTS << 8),
0x0101);
--
1.9.1
v2: use correct flag variable.
Signed-off-by: Grigori Goronzy <[email protected]>
---
drivers/usb/serial/ch341.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 94e6016..135370b 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -68,6 +68,7 @@
#define CH341_REQ_READ_REG 0x95
#define CH341_REG_BREAK1 0x05
#define CH341_REG_LCR 0x18
+#define CH341_REG_RTSCTS 0x27
#define CH341_NBREAK_BITS_REG1 0x01
#define CH341_LCR_ENABLE_RX 0x80
@@ -399,6 +400,16 @@ static void ch341_set_termios(struct tty_struct *tty,
ch341_set_handshake(port->serial->dev, priv->line_control);
+ if (cflag & CRTSCTS) {
+ r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
+ CH341_REG_RTSCTS | ((uint16_t)CH341_REG_RTSCTS << 8),
+ 0x0101);
+ if (r < 0) {
+ dev_err(&port->dev, "%s - USB control write error (%d)\n",
+ __func__, r);
+ tty->termios.c_cflag &= ~CRTSCTS;
+ }
+ }
}
static void ch341_break_ctl(struct tty_struct *tty, int break_state)
--
1.9.1
There are at least two hardware revisions, this may be helpful in
case compatibility issues need to be debugged.
Signed-off-by: Grigori Goronzy <[email protected]>
---
drivers/usb/serial/ch341.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 99b4621..94e6016 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -209,6 +209,7 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
r = ch341_control_in(dev, CH341_VERSION, 0, 0, buffer, size);
if (r < 0)
goto out;
+ dev_dbg(&dev->dev, "Chip version: %d\n", buffer[0]);
r = ch341_control_out(dev, CH341_SERIAL_INIT, 0, 0);
if (r < 0)
--
1.9.1
If the serial port hasn't been opened yet, no baud rate should be
set and RTS/DTR need to be deasserted.
Signed-off-by: Grigori Goronzy <[email protected]>
---
drivers/usb/serial/ch341.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 78590b7..451fa64 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -24,7 +24,6 @@
#include <linux/serial.h>
#include <asm/unaligned.h>
-#define DEFAULT_BAUD_RATE 9600
#define DEFAULT_TIMEOUT 1000
/* flags for IO-Bits */
@@ -232,10 +231,6 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
if (r < 0)
goto out;
- r = ch341_init_set_baudrate(dev, priv, 0);
- if (r < 0)
- goto out;
-
r = ch341_set_handshake(dev, priv->line_control);
if (r < 0)
goto out;
@@ -258,8 +253,6 @@ static int ch341_port_probe(struct usb_serial_port *port)
return -ENOMEM;
spin_lock_init(&priv->lock);
- priv->baud_rate = DEFAULT_BAUD_RATE;
- priv->line_control = CH341_BIT_RTS | CH341_BIT_DTR;
r = ch341_configure(port->serial->dev, priv);
if (r < 0)
--
1.9.1
With the new reinitialization method, configuring parity, different
frame lengths and different stop bit settings work as expected on
both CH340G and CH341A. This has been extensively tested with a
logic analyzer.
v2: only set mark/space when parity is enabled, simplifications,
patch termios HW flags.
Tested-by: Ryan Barber <[email protected]>
Signed-off-by: Grigori Goronzy <[email protected]>
---
drivers/usb/serial/ch341.c | 40 ++++++++++++++++++++++++++++++----------
1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 6181616..99b4621 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -341,7 +341,6 @@ static void ch341_set_termios(struct tty_struct *tty,
struct usb_serial_port *port, struct ktermios *old_termios)
{
struct ch341_private *priv = usb_get_serial_port_data(port);
- unsigned baud_rate;
unsigned long flags;
unsigned char ctrl;
int r;
@@ -350,13 +349,39 @@ static void ch341_set_termios(struct tty_struct *tty,
if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
return;
- baud_rate = tty_get_baud_rate(tty);
+ priv->baud_rate = tty_get_baud_rate(tty);
- priv->baud_rate = baud_rate;
+ ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX;
- ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX | CH341_LCR_CS8;
+ switch (C_CSIZE(tty)) {
+ case CS5:
+ ctrl |= CH341_LCR_CS5;
+ break;
+ case CS6:
+ ctrl |= CH341_LCR_CS6;
+ break;
+ case CS7:
+ ctrl |= CH341_LCR_CS7;
+ break;
+ default:
+ tty->termios.c_cflag |= CS8;
+ case CS8:
+ ctrl |= CH341_LCR_CS8;
+ break;
+ }
+
+ if (C_PARENB(tty)) {
+ ctrl |= CH341_LCR_ENABLE_PAR;
+ if (C_PARODD(tty))
+ ctrl |= CH341_LCR_PAR_EVEN;
+ if (C_CMSPAR(tty))
+ ctrl |= CH341_LCR_MARK_SPACE;
+ }
+
+ if (C_CSTOPB(tty))
+ ctrl |= CH341_LCR_STOP_BITS_2;
- if (baud_rate) {
+ if (priv->baud_rate) {
spin_lock_irqsave(&priv->lock, flags);
priv->line_control |= (CH341_BIT_DTR | CH341_BIT_RTS);
spin_unlock_irqrestore(&priv->lock, flags);
@@ -373,11 +398,6 @@ static void ch341_set_termios(struct tty_struct *tty,
ch341_set_handshake(port->serial->dev, priv->line_control);
- /* Unimplemented:
- * (cflag & CSIZE) : data bits [5, 8]
- * (cflag & PARENB) : parity {NONE, EVEN, ODD}
- * (cflag & CSTOPB) : stop bits [1, 2]
- */
}
static void ch341_break_ctl(struct tty_struct *tty, int break_state)
--
1.9.1
This may fail, do not assume it always works.
Signed-off-by: Grigori Goronzy <[email protected]>
---
drivers/usb/serial/ch341.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index c73808f..63df8ce 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -544,9 +544,7 @@ static int ch341_reset_resume(struct usb_serial *serial)
priv = usb_get_serial_port_data(serial->port[0]);
/* reconfigure ch341 serial port after bus-reset */
- ch341_configure(serial->dev, priv);
-
- return 0;
+ return ch341_configure(serial->dev, priv);
}
static struct usb_serial_driver ch341_device = {
--
1.9.1
Signed-off-by: Grigori Goronzy <[email protected]>
---
drivers/usb/serial/ch341.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 1ab4384..db4b561 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -61,6 +61,7 @@
* the Net/FreeBSD uchcom.c driver by Takanori Watanabe. Domo arigato.
*/
+#define CH341_MODEM_CTRL 0xA4
#define CH341_REQ_WRITE_REG 0x9A
#define CH341_REQ_READ_REG 0x95
#define CH341_REG_BREAK1 0x05
@@ -162,7 +163,7 @@ static int ch341_set_baudrate(struct usb_device *dev,
static int ch341_set_handshake(struct usb_device *dev, u8 control)
{
- return ch341_control_out(dev, 0xa4, ~control, 0);
+ return ch341_control_out(dev, CH341_MODEM_CTRL, ~control, 0);
}
static int ch341_get_status(struct usb_device *dev, struct ch341_private *priv)
--
1.9.1
Hi Grigori,
[auto build test ERROR on v4.5-rc7]
[cannot apply to usb/usb-testing usb-serial/usb-next v4.6-rc2 v4.6-rc1 next-20160408]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Grigori-Goronzy/USB-ch341-fix-error-handling-on-resume/20160411-075235
config: x86_64-allmodconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
Note: the linux-review/Grigori-Goronzy/USB-ch341-fix-error-handling-on-resume/20160411-075235 HEAD a1faa3e66e1410087d7b4711181282df9b094024 builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
drivers/usb/serial/ch341.c: In function 'ch341_set_termios':
>> drivers/usb/serial/ch341.c:403:6: error: 'cflag' undeclared (first use in this function)
if (cflag & CRTSCTS) {
^
drivers/usb/serial/ch341.c:403:6: note: each undeclared identifier is reported only once for each function it appears in
vim +/cflag +403 drivers/usb/serial/ch341.c
397 priv->line_control &= ~(CH341_BIT_DTR | CH341_BIT_RTS);
398 spin_unlock_irqrestore(&priv->lock, flags);
399 }
400
401 ch341_set_handshake(port->serial->dev, priv->line_control);
402
> 403 if (cflag & CRTSCTS) {
404 r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
405 CH341_REG_RTSCTS | ((uint16_t)CH341_REG_RTSCTS << 8),
406 0x0101);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Sorry if you get this twice, I was having some client problems,
but wanted to make sure you got this one...
Grigori Goronzy <[email protected]> wrote:
> With the new reinitialization method, configuring parity,
> different frame lengths and different stop bit settings work as
> expected on both CH340G and CH341A. This has been extensively
> tested with a logic analyzer.
>
> v2: only set mark/space when parity is enabled,
> simplifications, patch termios HW flags.
>
> Tested-by: Ryan Barber <[email protected]>
> Signed-off-by: Grigori Goronzy <[email protected]>
> ---
> drivers/usb/serial/ch341.c | 40 ++++++++++++++++++++++++++++++----------
> 1 file changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/serial/ch341.c
> b/drivers/usb/serial/ch341.c index 6181616..99b4621 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -341,7 +341,6 @@ static void ch341_set_termios(struct tty_struct *tty,
> struct usb_serial_port *port, struct ktermios *old_termios)
> {
> struct ch341_private *priv = usb_get_serial_port_data(port);
> - unsigned baud_rate;
> unsigned long flags;
> unsigned char ctrl;
> int r;
> @@ -350,13 +349,39 @@ static void ch341_set_termios(struct tty_struct *tty,
> if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
> return;
>
> - baud_rate = tty_get_baud_rate(tty);
> + priv->baud_rate = tty_get_baud_rate(tty);
>
> - priv->baud_rate = baud_rate;
> + ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX;
>
> - ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX | CH341_LCR_CS8;
> + switch (C_CSIZE(tty)) {
> + case CS5:
> + ctrl |= CH341_LCR_CS5;
> + break;
> + case CS6:
> + ctrl |= CH341_LCR_CS6;
> + break;
> + case CS7:
> + ctrl |= CH341_LCR_CS7;
> + break;
> + default:
> + tty->termios.c_cflag |= CS8;
> + case CS8:
> + ctrl |= CH341_LCR_CS8;
> + break;
> + }
> +
> + if (C_PARENB(tty)) {
> + ctrl |= CH341_LCR_ENABLE_PAR;
> + if (C_PARODD(tty))
> + ctrl |= CH341_LCR_PAR_EVEN;
Are you sure this does the right thing now? this is, as best as I
can tell, the inverse of what you had earlier, and doesn't read
right, if this is working, then I suggest renaming _LCR_PAR_EVEN
to LCR_PAR_ODD?
Cheers,
Karl P
> + if (C_CMSPAR(tty))
> + ctrl |= CH341_LCR_MARK_SPACE;
> + }
> +
> + if (C_CSTOPB(tty))
> + ctrl |= CH341_LCR_STOP_BITS_2;
>
> - if (baud_rate) {
> + if (priv->baud_rate) {
> spin_lock_irqsave(&priv->lock, flags);
> priv->line_control |= (CH341_BIT_DTR | CH341_BIT_RTS);
> spin_unlock_irqrestore(&priv->lock, flags);
> @@ -373,11 +398,6 @@ static void ch341_set_termios(struct tty_struct *tty,
>
> ch341_set_handshake(port->serial->dev, priv->line_control);
>
> - /* Unimplemented:
> - * (cflag & CSIZE) : data bits [5, 8]
> - * (cflag & PARENB) : parity {NONE, EVEN, ODD}
> - * (cflag & CSTOPB) : stop bits [1, 2]
> - */
> }
>
> static void ch341_break_ctl(struct tty_struct *tty, int break_state)
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-usb" in the body of a message to
> [email protected] More majordomo info at
> http://vger.kernel.org/majordomo-info.html
On 2016-04-11 19:25, Karl Palsson wrote:
> Sorry if you get this twice, I was having some client problems,
> but wanted to make sure you got this one...
>
>
> Grigori Goronzy <[email protected]> wrote:
>> With the new reinitialization method, configuring parity,
>> different frame lengths and different stop bit settings work as
>> expected on both CH340G and CH341A. This has been extensively
>> tested with a logic analyzer.
>>
>> v2: only set mark/space when parity is enabled,
>> simplifications, patch termios HW flags.
>>
>> Tested-by: Ryan Barber <[email protected]>
>> Signed-off-by: Grigori Goronzy <[email protected]>
>> ---
>> drivers/usb/serial/ch341.c | 40
>> ++++++++++++++++++++++++++++++----------
>> 1 file changed, 30 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/usb/serial/ch341.c
>> b/drivers/usb/serial/ch341.c index 6181616..99b4621 100644
>> --- a/drivers/usb/serial/ch341.c
>> +++ b/drivers/usb/serial/ch341.c
>> @@ -341,7 +341,6 @@ static void ch341_set_termios(struct tty_struct
>> *tty,
>> struct usb_serial_port *port, struct ktermios *old_termios)
>> {
>> struct ch341_private *priv = usb_get_serial_port_data(port);
>> - unsigned baud_rate;
>> unsigned long flags;
>> unsigned char ctrl;
>> int r;
>> @@ -350,13 +349,39 @@ static void ch341_set_termios(struct tty_struct
>> *tty,
>> if (old_termios && !tty_termios_hw_change(&tty->termios,
>> old_termios))
>> return;
>>
>> - baud_rate = tty_get_baud_rate(tty);
>> + priv->baud_rate = tty_get_baud_rate(tty);
>>
>> - priv->baud_rate = baud_rate;
>> + ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX;
>>
>> - ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX | CH341_LCR_CS8;
>> + switch (C_CSIZE(tty)) {
>> + case CS5:
>> + ctrl |= CH341_LCR_CS5;
>> + break;
>> + case CS6:
>> + ctrl |= CH341_LCR_CS6;
>> + break;
>> + case CS7:
>> + ctrl |= CH341_LCR_CS7;
>> + break;
>> + default:
>> + tty->termios.c_cflag |= CS8;
>> + case CS8:
>> + ctrl |= CH341_LCR_CS8;
>> + break;
>> + }
>> +
>> + if (C_PARENB(tty)) {
>> + ctrl |= CH341_LCR_ENABLE_PAR;
>> + if (C_PARODD(tty))
>> + ctrl |= CH341_LCR_PAR_EVEN;
>
> Are you sure this does the right thing now? this is, as best as I
> can tell, the inverse of what you had earlier, and doesn't read
> right, if this is working, then I suggest renaming _LCR_PAR_EVEN
> to LCR_PAR_ODD?
>
No, this is absolutely wrong, of course. I only did some sporadic
testing because my refactoring wasn't supposed to change functionality.
Thanks for pointing it out!
Grigori