Major improvements to the ch341 driver

Major improvements to the ch341 driver

[Grigori Goronzy]

Hi,

this patchset consists of several improvements and cleanups to the
ch341 driver, which has been mostly unmaintained for the last few
years, despite major shortcomings.  For instance, there is no support
at all for parity, which is an often used feature.  Other settings
are missing too, as is hardware flow control.

Here's a summary of changes:

- Restructured initialization and configuration, which makes CH341A hardware
  work for the first time and is the basis for some following additions.
- Support for the different parity modes, including mark/space
- Support for two stop bits
- Support for 5, 6 and 7 bit transfers
- Support for RTS/CTS hardware flow control
- Improved handling of B0 and DTR/RTS lines
- Added tx_empty callback
- Extracted magic numbers into definitions
- Cleaned up code style and debug/error messages

This has been tested on several different CH340G dongles and a
CH341A adapter which is designed for EEPROM programming (but still
supports UART).  Functionality of the different configurations has
been verified with a logic analyzer. In addition I did some quick
interoperability tests with a CP2102 UART.  My original motivation
for this work was parity support which I needed for stcgal [1],
and it works fine with that software too, of course.

Please review.  I would also appreciate to get some more testing
done.  In particular, I would like to make the sure the restructured
initialization does not break anything.

If you wonder, this is v2 because I initially sent it to the wrong list.

Best regards
Grigori

[1] https://github.com/grigorig/stcgal

[PATCH v2 01/14] USB: ch341: improve documentation

[Grigori Goronzy]

Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
---
 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 c73808f..43e4594 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -3,7 +3,8 @@
  * Copyright 2007, Werner Cornelius <werner@cornelius-consult.de>
  * Copyright 2009, Boris Hajduk <boris@hajduk.org>
  *
- * ch341.c implements a serial port driver for the Winchiphead CH341.
+ * ch341.c implements a serial port driver for the Winchiphead CH341,
+ * the second-worst USB serial chip in the world.
  *
  * The CH341 device can be used to implement an RS232 asynchronous
  * serial port, an IEEE-1284 parallel printer port or a memory-like
-- 
1.9.1

Re: [PATCH v2 01/14] USB: ch341: improve documentation

[One Thousand Gnomes]

On Sat,  2 Apr 2016 19:07:10 +0200
Grigori Goronzy <greg@chown.ath.cx> wrote:

> Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
> ---
>  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 c73808f..43e4594 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -3,7 +3,8 @@
>   * Copyright 2007, Werner Cornelius <werner@cornelius-consult.de>
>   * Copyright 2009, Boris Hajduk <boris@hajduk.org>
>   *
> - * ch341.c implements a serial port driver for the Winchiphead CH341.
> + * ch341.c implements a serial port driver for the Winchiphead CH341,
> + * the second-worst USB serial chip in the world.

Tempting as it may be it probably doesn't belong in the kernel.

[PATCH v2 02/14] USB: ch341: fix error handling on resume

[Grigori Goronzy]

This may fail, do not assume it always works.

Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
---
 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 43e4594..9fb9089 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -545,9 +545,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

[PATCH v2 03/14] USB: ch341: add LCR register definitions

[Grigori Goronzy]

BREAK2 seems to be a misnomer, the register configures various aspects
of the UART configuration.

Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
---
 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 9fb9089..788c75a 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -65,10 +65,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) },
@@ -371,7 +380,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;
@@ -393,11 +402,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

[PATCH v2 04/14] USB: ch341: add definitions for modem control

[Grigori Goronzy]

Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
---
 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 788c75a..25c5d8d 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -62,6 +62,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
@@ -163,7 +164,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

[PATCH v2 05/14] USB: ch341: fix USB buffer allocations

[Grigori Goronzy]

Use the correct types and sizes.

Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
---
 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 25c5d8d..6781911 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -116,7 +116,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)
+			    unsigned char *buf, unsigned bufsize)
 {
 	int r;
 
@@ -169,9 +169,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);
@@ -199,9 +199,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

Re: [PATCH v2 05/14] USB: ch341: fix USB buffer allocations

[Oliver Neukum]

On Sat, 2016-04-02 at 19:07 +0200, Grigori Goronzy wrote:
> Use the correct types and sizes.
> 
> Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
> ---
>  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 25c5d8d..6781911 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -116,7 +116,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)
> +			    unsigned char *buf, unsigned bufsize)

If you do that, you can just use u8 *

>  {
>  	int r;
>  
> @@ -169,9 +169,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);
> @@ -199,9 +199,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;

Are you sure only 2 are used?
For the amount of space needed it makes no difference.

	Regards
		Oliver

[PATCH v2 06/14] USB: ch341: reinitialize chip on reconfiguration

[Grigori Goronzy]

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.

Tested-by: Ryan Barber <rfb@skyscraper.nu>
Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
---
 drivers/usb/serial/ch341.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 6781911..c001773 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -62,6 +62,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
@@ -130,8 +132,8 @@ 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;
 	int r;
@@ -155,9 +157,7 @@ static int ch341_set_baudrate(struct usb_device *dev,
 	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;
 }
@@ -178,7 +178,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;
 
@@ -208,24 +208,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;
 
@@ -234,11 +230,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;
 
@@ -353,16 +345,26 @@ 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)
+			priv->baud_rate = tty_termios_baud_rate(old_termios);
 	} else {
 		spin_lock_irqsave(&priv->lock, flags);
 		priv->line_control &= ~(CH341_BIT_DTR | CH341_BIT_RTS);
-- 
1.9.1

Re: [PATCH v2 06/14] USB: ch341: reinitialize chip on reconfiguration

[Clemens Ladisch]

Grigori Goronzy wrote:
> 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.

> +++ b/drivers/usb/serial/ch341.c
> @@ -155,9 +157,7 @@ static int ch341_set_baudrate(struct usb_device *dev,
>  	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);

The variable b is no longer used, so it is no longer necessary to
compute the lower eight bits of the factor.


Regards,
Clemens

[PATCH v2 07/14] USB: ch341: add support for parity, frame length, stop bits

[Grigori Goronzy]

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.

Tested-by: Ryan Barber <rfb@skyscraper.nu>
Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
---
 drivers/usb/serial/ch341.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index c001773..4d66f0f 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -346,6 +346,7 @@ static void ch341_set_termios(struct tty_struct *tty,
 	unsigned baud_rate;
 	unsigned long flags;
 	unsigned char ctrl;
+	unsigned cflag = tty->termios.c_cflag;
 	int r;
 
 	/* redundant changes may cause the chip to lose bytes */
@@ -356,7 +357,35 @@ static void ch341_set_termios(struct tty_struct *tty,
 
 	priv->baud_rate = baud_rate;
 
-	ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX | CH341_LCR_CS8;
+	ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX;
+
+	switch (cflag & CSIZE) {
+	case CS5:
+		ctrl |= CH341_LCR_CS5;
+		break;
+	case CS6:
+		ctrl |= CH341_LCR_CS6;
+		break;
+	case CS7:
+		ctrl |= CH341_LCR_CS7;
+		break;
+	case CS8:
+	default:
+		ctrl |= CH341_LCR_CS8;
+		break;
+	}
+
+	if (cflag & PARENB) {
+		ctrl |= CH341_LCR_ENABLE_PAR;
+		if ((cflag & PARODD) == 0)
+			ctrl |= CH341_LCR_PAR_EVEN;
+	}
+
+	if (cflag & CMSPAR)
+		ctrl |= CH341_LCR_MARK_SPACE;
+
+	if (cflag & CSTOPB)
+		ctrl |= CH341_LCR_STOP_BITS_2;
 
 	if (baud_rate) {
 		spin_lock_irqsave(&priv->lock, flags);
@@ -373,11 +402,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

Re: [PATCH v2 07/14] USB: ch341: add support for parity, frame length, stop bits

[Karl Palsson]

[-- Attachment #1: Type: text/plain, Size: 3334 bytes --]


Grigori Goronzy <greg@chown.ath.cx> 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.
> 
> Tested-by: Ryan Barber <rfb@skyscraper.nu>
> Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
> ---
>  drivers/usb/serial/ch341.c | 36 ++++++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c
> b/drivers/usb/serial/ch341.c index c001773..4d66f0f 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -346,6 +346,7 @@ static void ch341_set_termios(struct tty_struct *tty,
>  	unsigned baud_rate;
>  	unsigned long flags;
>  	unsigned char ctrl;
> +	unsigned cflag = tty->termios.c_cflag;
>  	int r;
>  
>  	/* redundant changes may cause the chip to lose bytes */
> @@ -356,7 +357,35 @@ static void ch341_set_termios(struct tty_struct *tty,
>  
>  	priv->baud_rate = baud_rate;
>  
> -	ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX | CH341_LCR_CS8;
> +	ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX;
> +
> +	switch (cflag & CSIZE) {
> +	case CS5:
> +		ctrl |= CH341_LCR_CS5;
> +		break;
> +	case CS6:
> +		ctrl |= CH341_LCR_CS6;
> +		break;
> +	case CS7:
> +		ctrl |= CH341_LCR_CS7;
> +		break;
> +	case CS8:
> +	default:
> +		ctrl |= CH341_LCR_CS8;
> +		break;
> +	}
> +
> +	if (cflag & PARENB) {
> +		ctrl |= CH341_LCR_ENABLE_PAR;
> +		if ((cflag & PARODD) == 0)
> +			ctrl |= CH341_LCR_PAR_EVEN;
> +	}
> +
> +	if (cflag & CMSPAR)
> +		ctrl |= CH341_LCR_MARK_SPACE;
> +
> +	if (cflag & CSTOPB)
> +		ctrl |= CH341_LCR_STOP_BITS_2;
>  

I think this should be moved up to the PARENB check, at least,
when I was working on this. Also there's macros for some of the
flag checks: (From some code I was working on, but you can see
the mark/space is differently handled, this matched the windows
driver I was reversing usb captures from.)

if (C_PARENB(tty)) {
		*lcr |= CH341_LCR_PARITY;
		if (C_CMSPAR(tty)) {
			*lcr |= CH341_LCR_SPAR;
			if (C_PARODD(tty)) {
				dev_dbg(&port->dev, "parity = mark\n");
				*lcr &= ~CH341_LCR_EPAR;
			} else {
				dev_dbg(&port->dev, "parity = space\n");
				*lcr |= CH341_LCR_EPAR;
			}
		} else {
			*lcr &= ~CH341_LCR_SPAR;
			if (C_PARODD(tty)) {
				dev_dbg(&port->dev, "parity = odd\n");
				*lcr &= ~CH341_LCR_EPAR;
			} else {
				dev_dbg(&port->dev, "parity = even\n");
				*lcr |= CH341_LCR_EPAR;
			}
		}
	} else {
		*lcr &= ~(CH341_LCR_PARITY | CH341_LCR_SPAR | CH341_LCR_EPAR);
		dev_dbg(&port->dev, "parity = none\n");
	}



>  	if (baud_rate) {
>  		spin_lock_irqsave(&priv->lock, flags);
> @@ -373,11 +402,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
> majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 07/14] USB: ch341: add support for parity, frame length, stop bits

[One Thousand Gnomes]

On Sat,  2 Apr 2016 19:07:16 +0200
Grigori Goronzy <greg@chown.ath.cx> 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.
> 
> Tested-by: Ryan Barber <rfb@skyscraper.nu>
> Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
> ---
>  drivers/usb/serial/ch341.c | 36 ++++++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index c001773..4d66f0f 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -346,6 +346,7 @@ static void ch341_set_termios(struct tty_struct *tty,
>  	unsigned baud_rate;
>  	unsigned long flags;
>  	unsigned char ctrl;
> +	unsigned cflag = tty->termios.c_cflag;
>  	int r;
>  
>  	/* redundant changes may cause the chip to lose bytes */
> @@ -356,7 +357,35 @@ static void ch341_set_termios(struct tty_struct *tty,
>  
>  	priv->baud_rate = baud_rate;
>  
> -	ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX | CH341_LCR_CS8;
> +	ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX;
> +
> +	switch (cflag & CSIZE) {
> +	case CS5:
> +		ctrl |= CH341_LCR_CS5;
> +		break;
> +	case CS6:
> +		ctrl |= CH341_LCR_CS6;
> +		break;
> +	case CS7:
> +		ctrl |= CH341_LCR_CS7;
> +		break;
> +	case CS8:
> +	default:
> +		ctrl |= CH341_LCR_CS8;

In the default case tty-.termios.c_cflag should also be updated to show
CS8

Alan

[PATCH v2 08/14] USB: ch341: add debug output for chip version

[Grigori Goronzy]

There are at least two hardware revisions, this may be helpful in
case compatibility issues need to be debugged.

Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
---
 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 4d66f0f..58309d1 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -211,6 +211,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

[PATCH v2 09/14] USB: ch341: add support for RTS/CTS flow control

[Grigori Goronzy]

v2: use correct flag variable

Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
---
 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 58309d1..d956f75 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -69,6 +69,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
@@ -403,6 +404,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

[PATCH v2 10/14] USB: ch341: fix coding style

[Grigori Goronzy]

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. Align continuations in
function prototypes correctly.  Be more consistent with indentation of
statements broken into multiple lines.  Break some long lines properly.
Stop putting labels and statements into the same line.  Use braces
consistently for a single statement.

v2: minor additions

Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
---
 drivers/usb/serial/ch341.c | 53 ++++++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index d956f75..980dcea 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -108,11 +108,11 @@ 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",
-		USB_DIR_OUT|0x40, (int)request, (int)value, (int)index);
+			USB_DIR_OUT|0x40, (int)request, (int)value, (int)index);
 
 	r = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request,
-			    USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
-			    value, index, NULL, 0, DEFAULT_TIMEOUT);
+			USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
+			value, index, NULL, 0, DEFAULT_TIMEOUT);
 
 	return r;
 }
@@ -124,17 +124,17 @@ 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,
-		(int)bufsize);
+			USB_DIR_IN|0x40, (int)request, (int)value, (int)index,
+			buf, (int)bufsize);
 
 	r = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), request,
-			    USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
-			    value, index, buf, bufsize, DEFAULT_TIMEOUT);
+			USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
+			value, index, buf, bufsize, DEFAULT_TIMEOUT);
 	return r;
 }
 
 static int ch341_init_set_baudrate(struct usb_device *dev,
-			      struct ch341_private *priv, unsigned ctrl)
+				   struct ch341_private *priv, unsigned ctrl)
 {
 	short a, b;
 	int r;
@@ -158,7 +158,8 @@ static int ch341_init_set_baudrate(struct usb_device *dev,
 	a = (factor & 0xff00) | divisor;
 	b = factor & 0xff;
 
-	r = ch341_control_out(dev, CH341_SERIAL_INIT, 0x9c | (ctrl << 8) , a | 0x80);
+	r = ch341_control_out(dev, CH341_SERIAL_INIT, 0x9c | (ctrl << 8),
+			a | 0x80);
 
 	return r;
 }
@@ -189,10 +190,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;
 }
 
@@ -243,7 +246,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;
 }
 
@@ -267,7 +271,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;
 }
 
@@ -329,20 +334,22 @@ static int ch341_open(struct tty_struct *tty, struct usb_serial_port *port)
 	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);
+				__func__, r);
 		goto out;
 	}
 
 	r = usb_serial_generic_open(tty, port);
 
-out:	return r;
+out:
+	return r;
 }
 
 /* Old_termios contains the original termios settings and
  * tty->termios contains the new setting to be used.
  */
 static void ch341_set_termios(struct tty_struct *tty,
-		struct usb_serial_port *port, struct ktermios *old_termios)
+			      struct usb_serial_port *port,
+			      struct ktermios *old_termios)
 {
 	struct ch341_private *priv = usb_get_serial_port_data(port);
 	unsigned baud_rate;
@@ -437,7 +444,7 @@ static void ch341_break_ctl(struct tty_struct *tty, int break_state)
 		goto out;
 	}
 	dev_dbg(&port->dev, "%s - initial ch341 break register contents - reg1: %x, reg2: %x\n",
-		__func__, break_reg[0], break_reg[1]);
+			__func__, break_reg[0], break_reg[1]);
 	if (break_state != 0) {
 		dev_dbg(&port->dev, "%s - Enter break state requested\n", __func__);
 		break_reg[0] &= ~CH341_NBREAK_BITS_REG1;
@@ -448,7 +455,7 @@ static void ch341_break_ctl(struct tty_struct *tty, int break_state)
 		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]);
+			__func__, 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);
@@ -483,7 +490,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;
@@ -542,11 +549,11 @@ static void ch341_read_int_callback(struct urb *urb)
 	case -ESHUTDOWN:
 		/* this urb is terminated, clean up */
 		dev_dbg(&urb->dev->dev, "%s - urb shutting down: %d\n",
-			__func__, urb->status);
+				__func__, urb->status);
 		return;
 	default:
 		dev_dbg(&urb->dev->dev, "%s - nonzero urb status: %d\n",
-			__func__, urb->status);
+				__func__, urb->status);
 		goto exit;
 	}
 
@@ -556,7 +563,7 @@ exit:
 	status = usb_submit_urb(urb, GFP_ATOMIC);
 	if (status) {
 		dev_err(&urb->dev->dev, "%s - usb_submit_urb failed: %d\n",
-			__func__, status);
+				__func__, status);
 	}
 }
 
@@ -604,7 +611,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

Re: [PATCH v2 10/14] USB: ch341: fix coding style

[Joe Perches]

On Sat, 2016-04-02 at 19:07 +0200, Grigori Goronzy wrote:
> 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. Align continuations in
> function prototypes correctly.  Be more consistent with indentation of
> statements broken into multiple lines.  Break some long lines properly.
> Stop putting labels and statements into the same line.  Use braces
> consistently for a single statement.

Most of the whitespace only changes are undesired.

Multi-line statements here are using alignment to
open parenthesis which for some is the preferred
style.
> > diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
[]
> @@ -108,11 +108,11 @@ 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",
> -		USB_DIR_OUT|0x40, (int)request, (int)value, (int)index);
> +			USB_DIR_OUT|0x40, (int)request, (int)value, (int)index);
>  
>  	r = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request,
> -			    USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
> -			    value, index, NULL, 0, DEFAULT_TIMEOUT);
> +			USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
> +			value, index, NULL, 0, DEFAULT_TIMEOUT);

The original code is fine.

etc.

Re: [PATCH v2 10/14] USB: ch341: fix coding style

[Grigori Goronzy]

[-- Attachment #1.1: Type: text/plain, Size: 722 bytes --]

On 04/02/2016 07:29 PM, Joe Perches wrote:
> Most of the whitespace only changes are undesired.
> 

Well, the style wasn't very consistent.  I think consistency is
important.  So I took the liberty of deciding for one style and stuck to it.

> Multi-line statements here are using alignment to
> open parenthesis which for some is the preferred
> style.

I didn't use alignment to open parentheses because that is often
reducing the usable space per line too much.  So you have to break lines
a lot and code becomes less readable.

Of course, I'm open to arguments if and why a particular style should be
preferred.  Maybe we should try to mostly avoid these bikeshed
discussions though. :)

Grigori


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 10/14] USB: ch341: fix coding style

[Johan Hovold]

On Wed, Apr 06, 2016 at 07:58:36PM +0200, Grigori Goronzy wrote:
> On 04/02/2016 07:29 PM, Joe Perches wrote:
> > Most of the whitespace only changes are undesired.
>
> Well, the style wasn't very consistent.  I think consistency is
> important.  So I took the liberty of deciding for one style and stuck
> to it.
> 
> > Multi-line statements here are using alignment to
> > open parenthesis which for some is the preferred
> > style.
> 
> I didn't use alignment to open parentheses because that is often
> reducing the usable space per line too much.  So you have to break lines
> a lot and code becomes less readable.
> 
> Of course, I'm open to arguments if and why a particular style should be
> preferred.  Maybe we should try to mostly avoid these bikeshed
> discussions though. :)

As Joe already said, we generally don't want indentation-only changes to
existing code. Just try to stick to the style of the driver (even if
it's inconsistent at times).

Thanks,
Johan

Re: [PATCH v2 10/14] USB: ch341: fix coding style

[Grigori Goronzy]

[-- Attachment #1.1: Type: text/plain, Size: 971 bytes --]

On 04/06/2016 08:10 PM, Johan Hovold wrote:
> As Joe already said, we generally don't want indentation-only changes to
> existing code. Just try to stick to the style of the driver (even if
> it's inconsistent at times).
> 

Hm, I don't get it.  I understand that white-space-only changes are
discouraged if they are freestanding and contributors don't follow up
with any change to functionality (as outlined in
development-process/4.Coding), but this is not the case here.  IMHO, if
the style of a module is inconsistent, it should be fixed at some point.
 The kind of policy you are presenting here will in the long run lead to
messy code, and can't be found in any of the official documents (e.g.
CodingStyle, SubmitChecklist, development-process/) either.  It also
encourages mixing white-space changes with patches that change
functionality, which is a bad practice.

I'll just drop the indentation changes. The rest is fine, I guess?

Grigori


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2 11/14] USB: ch341: clean up messages

[Grigori Goronzy]

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 <greg@chown.ath.cx>
---
 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 980dcea..ba654e1 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -107,7 +107,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,
@@ -123,9 +123,9 @@ 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",
+	dev_dbg(&dev->dev, "control_in(%02x,%02x,%04x,%04x,%u)\n",
 			USB_DIR_IN|0x40, (int)request, (int)value, (int)index,
-			buf, (int)bufsize);
+			(int)bufsize);
 
 	r = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), request,
 			USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
@@ -330,11 +330,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;
 	}
 
@@ -416,8 +416,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;
 		}
 	}
@@ -439,29 +438,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);
 }
@@ -509,7 +506,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;
@@ -548,12 +545,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;
 	}
 
@@ -562,8 +559,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);
 	}
 }
 
@@ -588,7 +584,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

[PATCH v2 12/14] USB: ch341: improve B0 handling

[Grigori Goronzy]

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.

Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
---
 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 ba654e1..3b9a43d 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -396,10 +396,12 @@ static void ch341_set_termios(struct tty_struct *tty,
 	if (cflag & CSTOPB)
 		ctrl |= CH341_LCR_STOP_BITS_2;
 
-	if (baud_rate) {
-		spin_lock_irqsave(&priv->lock, flags);
-		priv->line_control |= (CH341_BIT_DTR | CH341_BIT_RTS);
-		spin_unlock_irqrestore(&priv->lock, flags);
+	if ((cflag & CBAUD) != 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)
 			priv->baud_rate = tty_termios_baud_rate(old_termios);
@@ -411,7 +413,7 @@ static void ch341_set_termios(struct tty_struct *tty,
 
 	ch341_set_handshake(port->serial->dev, priv->line_control);
 
-	if (cflag & CRTSCTS) {
+	if ((cflag & CRTSCTS) && ((cflag & CBAUD) != 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

[PATCH v2 13/14] USB: ch341: get rid of default configuration

[Grigori Goronzy]

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 <greg@chown.ath.cx>
---
 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 3b9a43d..6981e2ad 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -25,7 +25,6 @@
 #include <linux/serial.h>
 #include <asm/unaligned.h>
 
-#define DEFAULT_BAUD_RATE 9600
 #define DEFAULT_TIMEOUT   1000
 
 /* flags for IO-Bits */
@@ -235,10 +234,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;
@@ -261,8 +256,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

[PATCH v2 14/14] USB: ch341: implement tx_empty callback

[Grigori Goronzy]

The status bit was found with USB captures of the Windows driver and
some luck.  Tested on CH340G and CH341A.

Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
---
 drivers/usb/serial/ch341.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 6981e2ad..adf7d79 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -82,6 +82,8 @@
 #define CH341_LCR_CS6          0x01
 #define CH341_LCR_CS5          0x00
 
+#define CH341_STATUS_TXBUSY    0x01
+
 static const struct usb_device_id id_table[] = {
 	{ USB_DEVICE(0x4348, 0x5523) },
 	{ USB_DEVICE(0x1a86, 0x7523) },
@@ -95,6 +97,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,
@@ -187,7 +190,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;
@@ -198,6 +202,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)
@@ -606,6 +622,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

Re: [PATCH v2 14/14] USB: ch341: implement tx_empty callback

[Karl Palsson]

[-- Attachment #1: Type: text/plain, Size: 2695 bytes --]


Grigori Goronzy <greg@chown.ath.cx> wrote:
> The status bit was found with USB captures of the Windows
> driver and some luck. Tested on CH340G and CH341A.

By my reversing, bit 0x4, is "multiple status changes since last
interrupt"

> 
> Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
> ---
>  drivers/usb/serial/ch341.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/ch341.c
> b/drivers/usb/serial/ch341.c index 6981e2ad..adf7d79 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -82,6 +82,8 @@
>  #define CH341_LCR_CS6          0x01
>  #define CH341_LCR_CS5          0x00
>  
> +#define CH341_STATUS_TXBUSY    0x01
> +
>  static const struct usb_device_id id_table[] = {
>  	{ USB_DEVICE(0x4348, 0x5523) },
>  	{ USB_DEVICE(0x1a86, 0x7523) },
> @@ -95,6 +97,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,
> @@ -187,7 +190,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;
> @@ -198,6 +202,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)
> @@ -606,6 +622,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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-usb" in the body of a message to
> majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 14/14] USB: ch341: implement tx_empty callback

[Grigori Goronzy]

[-- Attachment #1.1: Type: text/plain, Size: 521 bytes --]

On 04/03/2016 06:03 PM, Karl Palsson wrote:
> 
> Grigori Goronzy <greg@chown.ath.cx> wrote:
>> The status bit was found with USB captures of the Windows
>> driver and some luck. Tested on CH340G and CH341A.
> 
> By my reversing, bit 0x4, is "multiple status changes since last
> interrupt"
>

Thanks, I can add the definition.  However, I wonder what this is
actually good for.  We don't actually need this to see that there are
multiple status changes.  We can just look at the different bits.

Grigori


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]