All of lore.kernel.org
 help / color / mirror / Atom feed
* Major improvements to the ch341 driver v4
@ 2016-04-15 21:14 Grigori Goronzy
  2016-04-15 21:14 ` [PATCH v4 01/13] USB: ch341: fix error handling on resume Grigori Goronzy
                   ` (13 more replies)
  0 siblings, 14 replies; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-15 21:14 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel

Hi,

here's a hopefully final v4 of my ch341 patchset.  Changelog below
this time, because it's cleary better this way.  Please review.

v4:
- Fix parity even/odd mixup introduced in v3.
- Fix compilation errors of intermediate commits introduced in v3.

v3:
- Use u8 shorthand for unsigned char.
- Get rid of an unused variable.
- Improve error handling in set_termios.
- Only set mark/space when parity is enabled.
- Use C_* macros and some other simplifications.
- Patch termios HW flags for default CS8 case.
- Drop most style fixes.
- Unify definitions for the "general status" register bits.

v2:
- Improve/fix B0 handling.
- Fix initial/default configuration.
- Add tx_empty callback.
- Split up one patch:
  - Reinitialize chip on reconfiguration.
  - Add support for parity, frame length, stop bits.

v1:
- Initial version

Best regards
Grigori

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v4 01/13] USB: ch341: fix error handling on resume
  2016-04-15 21:14 Major improvements to the ch341 driver v4 Grigori Goronzy
@ 2016-04-15 21:14 ` Grigori Goronzy
  2016-04-29 12:16   ` Johan Hovold
  2016-04-15 21:14 ` [PATCH v4 02/13] USB: ch341: add LCR register definitions Grigori Goronzy
                   ` (12 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-15 21:14 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, 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 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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v4 02/13] USB: ch341: add LCR register definitions
  2016-04-15 21:14 Major improvements to the ch341 driver v4 Grigori Goronzy
  2016-04-15 21:14 ` [PATCH v4 01/13] USB: ch341: fix error handling on resume Grigori Goronzy
@ 2016-04-15 21:14 ` Grigori Goronzy
  2016-04-29 12:18   ` Johan Hovold
  2016-04-15 21:14 ` [PATCH v4 03/13] USB: ch341: add definitions for modem control Grigori Goronzy
                   ` (11 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-15 21:14 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, 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 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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v4 03/13] USB: ch341: add definitions for modem control
  2016-04-15 21:14 Major improvements to the ch341 driver v4 Grigori Goronzy
  2016-04-15 21:14 ` [PATCH v4 01/13] USB: ch341: fix error handling on resume Grigori Goronzy
  2016-04-15 21:14 ` [PATCH v4 02/13] USB: ch341: add LCR register definitions Grigori Goronzy
@ 2016-04-15 21:14 ` Grigori Goronzy
  2016-04-29 12:22   ` Johan Hovold
  2016-04-15 21:14 ` [PATCH v4 04/13] USB: ch341: fix USB buffer allocations Grigori Goronzy
                   ` (10 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-15 21:14 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, 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 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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v4 04/13] USB: ch341: fix USB buffer allocations
  2016-04-15 21:14 Major improvements to the ch341 driver v4 Grigori Goronzy
                   ` (2 preceding siblings ...)
  2016-04-15 21:14 ` [PATCH v4 03/13] USB: ch341: add definitions for modem control Grigori Goronzy
@ 2016-04-15 21:14 ` Grigori Goronzy
  2016-04-29 12:52   ` Johan Hovold
  2016-04-15 21:14 ` [PATCH v4 05/13] USB: ch341: reinitialize chip on reconfiguration Grigori Goronzy
                   ` (9 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-15 21:14 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Grigori Goronzy

Use the correct types and sizes.

v2: use u8 shorthand for unsigned char.

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 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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v4 05/13] USB: ch341: reinitialize chip on reconfiguration
  2016-04-15 21:14 Major improvements to the ch341 driver v4 Grigori Goronzy
                   ` (3 preceding siblings ...)
  2016-04-15 21:14 ` [PATCH v4 04/13] USB: ch341: fix USB buffer allocations Grigori Goronzy
@ 2016-04-15 21:14 ` Grigori Goronzy
  2016-04-29 13:03   ` Johan Hovold
  2016-04-15 21:14 ` [PATCH v4 06/13] USB: ch341: add support for parity, frame length, stop bits Grigori Goronzy
                   ` (8 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-15 21:14 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, 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.

v2: get rid of unused variable, improve error handling.

Tested-by: Ryan Barber <rfb@skyscraper.nu>
Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
---
 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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v4 06/13] USB: ch341: add support for parity, frame length, stop bits
  2016-04-15 21:14 Major improvements to the ch341 driver v4 Grigori Goronzy
                   ` (4 preceding siblings ...)
  2016-04-15 21:14 ` [PATCH v4 05/13] USB: ch341: reinitialize chip on reconfiguration Grigori Goronzy
@ 2016-04-15 21:14 ` Grigori Goronzy
  2016-04-29 13:11   ` Johan Hovold
  2016-04-15 21:14 ` [PATCH v4 07/13] USB: ch341: add debug output for chip version Grigori Goronzy
                   ` (7 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-15 21:14 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, 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.

v2: only set mark/space when parity is enabled, simplifications,
patch termios HW flags.
v3: fix parity odd/even regression.

Tested-by: Ryan Barber <rfb@skyscraper.nu>
Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
---
 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..2fbec4a 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) == 0)
+			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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v4 07/13] USB: ch341: add debug output for chip version
  2016-04-15 21:14 Major improvements to the ch341 driver v4 Grigori Goronzy
                   ` (5 preceding siblings ...)
  2016-04-15 21:14 ` [PATCH v4 06/13] USB: ch341: add support for parity, frame length, stop bits Grigori Goronzy
@ 2016-04-15 21:14 ` Grigori Goronzy
  2016-04-29 13:13   ` Johan Hovold
  2016-04-15 21:14 ` [PATCH v4 08/13] USB: ch341: add support for RTS/CTS flow control Grigori Goronzy
                   ` (6 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-15 21:14 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, 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 2fbec4a..e475677 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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v4 08/13] USB: ch341: add support for RTS/CTS flow control
  2016-04-15 21:14 Major improvements to the ch341 driver v4 Grigori Goronzy
                   ` (6 preceding siblings ...)
  2016-04-15 21:14 ` [PATCH v4 07/13] USB: ch341: add debug output for chip version Grigori Goronzy
@ 2016-04-15 21:14 ` Grigori Goronzy
  2016-04-29 13:23   ` Johan Hovold
  2016-04-15 21:14 ` [PATCH v4 09/13] USB: ch341: fix coding style Grigori Goronzy
                   ` (5 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-15 21:14 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Grigori Goronzy

v2: use correct flag variable.
v3: fix compilation

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 e475677..7ca21a1 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 (C_CRTSCTS(tty)) {
+		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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v4 09/13] USB: ch341: fix coding style
  2016-04-15 21:14 Major improvements to the ch341 driver v4 Grigori Goronzy
                   ` (7 preceding siblings ...)
  2016-04-15 21:14 ` [PATCH v4 08/13] USB: ch341: add support for RTS/CTS flow control Grigori Goronzy
@ 2016-04-15 21:14 ` Grigori Goronzy
  2016-04-29 13:29   ` Johan Hovold
  2016-04-15 21:14 ` [PATCH v4 10/13] USB: ch341: clean up messages Grigori Goronzy
                   ` (4 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-15 21:14 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, 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.  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 <greg@chown.ath.cx>
---
 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 7ca21a1..f524aa9 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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v4 10/13] USB: ch341: clean up messages
  2016-04-15 21:14 Major improvements to the ch341 driver v4 Grigori Goronzy
                   ` (8 preceding siblings ...)
  2016-04-15 21:14 ` [PATCH v4 09/13] USB: ch341: fix coding style Grigori Goronzy
@ 2016-04-15 21:14 ` Grigori Goronzy
  2016-04-29 13:40   ` Johan Hovold
  2016-04-15 21:14 ` [PATCH v4 11/13] USB: ch341: improve B0 handling Grigori Goronzy
                   ` (3 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-15 21:14 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, 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 f524aa9..22cfd88 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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v4 11/13] USB: ch341: improve B0 handling
  2016-04-15 21:14 Major improvements to the ch341 driver v4 Grigori Goronzy
                   ` (9 preceding siblings ...)
  2016-04-15 21:14 ` [PATCH v4 10/13] USB: ch341: clean up messages Grigori Goronzy
@ 2016-04-15 21:14 ` Grigori Goronzy
  2016-04-29 13:41   ` Johan Hovold
  2016-04-15 21:14 ` [PATCH v4 12/13] USB: ch341: get rid of default configuration Grigori Goronzy
                   ` (2 subsequent siblings)
  13 siblings, 1 reply; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-15 21:14 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, 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.

v2: use c_cflag macros.
v3: rebase.

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 22cfd88..3ce2041 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 (C_CRTSCTS(tty)) {
+	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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v4 12/13] USB: ch341: get rid of default configuration
  2016-04-15 21:14 Major improvements to the ch341 driver v4 Grigori Goronzy
                   ` (10 preceding siblings ...)
  2016-04-15 21:14 ` [PATCH v4 11/13] USB: ch341: improve B0 handling Grigori Goronzy
@ 2016-04-15 21:14 ` Grigori Goronzy
  2016-04-29 13:43   ` Johan Hovold
  2016-04-15 21:14 ` [PATCH v4 13/13] USB: ch341: implement tx_empty callback Grigori Goronzy
  2016-04-28 23:24 ` Major improvements to the ch341 driver v4 Grigori Goronzy
  13 siblings, 1 reply; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-15 21:14 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, 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 3ce2041..78adce7 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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v4 13/13] USB: ch341: implement tx_empty callback
  2016-04-15 21:14 Major improvements to the ch341 driver v4 Grigori Goronzy
                   ` (11 preceding siblings ...)
  2016-04-15 21:14 ` [PATCH v4 12/13] USB: ch341: get rid of default configuration Grigori Goronzy
@ 2016-04-15 21:14 ` Grigori Goronzy
  2016-04-29 13:47   ` Johan Hovold
  2016-04-28 23:24 ` Major improvements to the ch341 driver v4 Grigori Goronzy
  13 siblings, 1 reply; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-15 21:14 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Grigori Goronzy

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 <greg@chown.ath.cx>
---
 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 78adce7..12a430c 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

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: Major improvements to the ch341 driver v4
  2016-04-15 21:14 Major improvements to the ch341 driver v4 Grigori Goronzy
                   ` (12 preceding siblings ...)
  2016-04-15 21:14 ` [PATCH v4 13/13] USB: ch341: implement tx_empty callback Grigori Goronzy
@ 2016-04-28 23:24 ` Grigori Goronzy
  13 siblings, 0 replies; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-28 23:24 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, linux-usb-owner

On 2016-04-15 23:14, Grigori Goronzy wrote:
> Hi,
> 
> here's a hopefully final v4 of my ch341 patchset.  Changelog below
> this time, because it's cleary better this way.  Please review.
> 

Ping?

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 01/13] USB: ch341: fix error handling on resume
  2016-04-15 21:14 ` [PATCH v4 01/13] USB: ch341: fix error handling on resume Grigori Goronzy
@ 2016-04-29 12:16   ` Johan Hovold
  2016-04-29 15:11     ` Grigori Goronzy
  0 siblings, 1 reply; 31+ messages in thread
From: Johan Hovold @ 2016-04-29 12:16 UTC (permalink / raw)
  To: Grigori Goronzy; +Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel

On Fri, Apr 15, 2016 at 11:14:04PM +0200, Grigori Goronzy wrote:
> 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 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);

This is correct, but have noticed that resume is currently broken in
that the interrupt urb is never resubmitted on resume in case the port is
already open?

Also ch341_configure must not use GFP_KERNEL either if called from a
resume path (use GFP_NOIO).

Care to fix this up as well?

>  }
>  
>  static struct usb_serial_driver ch341_device = {

Thanks,
Johan

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 02/13] USB: ch341: add LCR register definitions
  2016-04-15 21:14 ` [PATCH v4 02/13] USB: ch341: add LCR register definitions Grigori Goronzy
@ 2016-04-29 12:18   ` Johan Hovold
  0 siblings, 0 replies; 31+ messages in thread
From: Johan Hovold @ 2016-04-29 12:18 UTC (permalink / raw)
  To: Grigori Goronzy; +Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel

On Fri, Apr 15, 2016 at 11:14:05PM +0200, Grigori Goronzy wrote:
> BREAK2 seems to be a misnomer, the register configures various aspects
> of the UART configuration.
> 
> Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>

Finally. Thanks for fixing this. :)

Johan

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 03/13] USB: ch341: add definitions for modem control
  2016-04-15 21:14 ` [PATCH v4 03/13] USB: ch341: add definitions for modem control Grigori Goronzy
@ 2016-04-29 12:22   ` Johan Hovold
  0 siblings, 0 replies; 31+ messages in thread
From: Johan Hovold @ 2016-04-29 12:22 UTC (permalink / raw)
  To: Grigori Goronzy; +Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel

On Fri, Apr 15, 2016 at 11:14:06PM +0200, Grigori Goronzy wrote:

No commit message?

> 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 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

Please use the common CH341_REQ prefix and keep the requests sorted
numerically.

>  #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)

Thanks,
Johan

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 04/13] USB: ch341: fix USB buffer allocations
  2016-04-15 21:14 ` [PATCH v4 04/13] USB: ch341: fix USB buffer allocations Grigori Goronzy
@ 2016-04-29 12:52   ` Johan Hovold
  2016-04-29 15:12     ` Grigori Goronzy
  0 siblings, 1 reply; 31+ messages in thread
From: Johan Hovold @ 2016-04-29 12:52 UTC (permalink / raw)
  To: Grigori Goronzy; +Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel

On Fri, Apr 15, 2016 at 11:14:07PM +0200, Grigori Goronzy wrote:
> Use the correct types and sizes.
> 
> v2: use u8 shorthand for unsigned char.

Pleas place commit logs below the cut-off line (---).

> 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 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)

Just use void * for the buffer parameter.

>  {
>  	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;

Did you reply to Oliver's comment about this change? Are you sure this
won't break some old device expecting to be asked for eight bytes even
if only two are returned?

I suggest breaking this out into a separate patch either way.

Thanks,
Johan

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 05/13] USB: ch341: reinitialize chip on reconfiguration
  2016-04-15 21:14 ` [PATCH v4 05/13] USB: ch341: reinitialize chip on reconfiguration Grigori Goronzy
@ 2016-04-29 13:03   ` Johan Hovold
  0 siblings, 0 replies; 31+ messages in thread
From: Johan Hovold @ 2016-04-29 13:03 UTC (permalink / raw)
  To: Grigori Goronzy; +Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel

On Fri, Apr 15, 2016 at 11:14:08PM +0200, 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.
> 
> v2: get rid of unused variable, improve error handling.
> 
> Tested-by: Ryan Barber <rfb@skyscraper.nu>
> Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
> ---
>  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

Use the CH341_REQ prefix keep the entries sorted.

>  #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);

This looks like an unrelated change.

>  	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);

Ditto.

>  	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);

So does this.

Please fix up all the magic constants in a preparatory patch, which
should make it easier to see what's really going on here.

Thanks,
Johan

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 06/13] USB: ch341: add support for parity, frame length, stop bits
  2016-04-15 21:14 ` [PATCH v4 06/13] USB: ch341: add support for parity, frame length, stop bits Grigori Goronzy
@ 2016-04-29 13:11   ` Johan Hovold
  0 siblings, 0 replies; 31+ messages in thread
From: Johan Hovold @ 2016-04-29 13:11 UTC (permalink / raw)
  To: Grigori Goronzy; +Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel

On Fri, Apr 15, 2016 at 11:14:09PM +0200, Grigori Goronzy 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.
> v3: fix parity odd/even regression.
> 
> Tested-by: Ryan Barber <rfb@skyscraper.nu>
> Signed-off-by: Grigori Goronzy <greg@chown.ath.cx>
> ---
>  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..2fbec4a 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;

This is an unrelated change.

> +	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;

CSIZE is 2-bits wide, so this shouldn't be needed. (If it were, you'd
need to clear the bits first.)

> +	case CS8:
> +		ctrl |= CH341_LCR_CS8;
> +		break;
> +	}
> +
> +	if (C_PARENB(tty)) {
> +		ctrl |= CH341_LCR_ENABLE_PAR;
> +		if (C_PARODD(tty) == 0)
> +			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)

Looks good otherwise.

Thanks,
Johan

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 07/13] USB: ch341: add debug output for chip version
  2016-04-15 21:14 ` [PATCH v4 07/13] USB: ch341: add debug output for chip version Grigori Goronzy
@ 2016-04-29 13:13   ` Johan Hovold
  0 siblings, 0 replies; 31+ messages in thread
From: Johan Hovold @ 2016-04-29 13:13 UTC (permalink / raw)
  To: Grigori Goronzy; +Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel

On Fri, Apr 15, 2016 at 11:14:10PM +0200, Grigori Goronzy wrote:
> 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 2fbec4a..e475677 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]);

0x%02x?

>  
>  	r = ch341_control_out(dev, CH341_SERIAL_INIT, 0, 0);
>  	if (r < 0)

Thanks,
Johan

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 08/13] USB: ch341: add support for RTS/CTS flow control
  2016-04-15 21:14 ` [PATCH v4 08/13] USB: ch341: add support for RTS/CTS flow control Grigori Goronzy
@ 2016-04-29 13:23   ` Johan Hovold
  0 siblings, 0 replies; 31+ messages in thread
From: Johan Hovold @ 2016-04-29 13:23 UTC (permalink / raw)
  To: Grigori Goronzy; +Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel

On Fri, Apr 15, 2016 at 11:14:11PM +0200, Grigori Goronzy wrote:

No commit message?

> v2: use correct flag variable.
> v3: fix compilation
> 
> 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 e475677..7ca21a1 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 (C_CRTSCTS(tty)) {
> +		r = ch341_control_out(port->serial->dev, CH341_REQ_WRITE_REG,
> +				CH341_REG_RTSCTS | ((uint16_t)CH341_REG_RTSCTS << 8),

(u16)

> +				0x0101);

You should also coordinate this with B0 handling (e.g. disable
hard-flow control and make sure that RTS is deasserted on ->B0
transitions).

> +		if (r < 0) {
> +			dev_err(&port->dev, "%s - USB control write error (%d)\n",
> +					__func__, r);

Please spell out what went wrong

	"failed to enable flow control: %d\n"

> +			tty->termios.c_cflag &= ~CRTSCTS;
> +		}
> +	}

What about disabling flow control?

>  }
>  
>  static void ch341_break_ctl(struct tty_struct *tty, int break_state)

Thanks,
Johan

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 09/13] USB: ch341: fix coding style
  2016-04-15 21:14 ` [PATCH v4 09/13] USB: ch341: fix coding style Grigori Goronzy
@ 2016-04-29 13:29   ` Johan Hovold
  0 siblings, 0 replies; 31+ messages in thread
From: Johan Hovold @ 2016-04-29 13:29 UTC (permalink / raw)
  To: Grigori Goronzy; +Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel

On Fri, Apr 15, 2016 at 11:14:12PM +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.  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 <greg@chown.ath.cx>
> ---
>  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 7ca21a1..f524aa9 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)

This could go into the patch that renamed the function.

>  {
>  	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)

This chunk isn't really needed (and is inconsistent with your commit
message).

>  {
>  	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,

How about fixing all the entries to use only tabs to align the RHS
instead?

>  	.carrier_raised	   = ch341_carrier_raised,
>  	.close             = ch341_close,
>  	.set_termios       = ch341_set_termios,

Thanks,
Johan

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 10/13] USB: ch341: clean up messages
  2016-04-15 21:14 ` [PATCH v4 10/13] USB: ch341: clean up messages Grigori Goronzy
@ 2016-04-29 13:40   ` Johan Hovold
  0 siblings, 0 replies; 31+ messages in thread
From: Johan Hovold @ 2016-04-29 13:40 UTC (permalink / raw)
  To: Grigori Goronzy; +Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel

On Fri, Apr 15, 2016 at 11:14:13PM +0200, Grigori Goronzy wrote:
> No functional change.  Remove explicit function name printing, it's
> easy to use dynamic debug to print it every time, if required.

While that is true, we currently use __func__ in a lot of debug messages
as a compact form for a self-contained message (e.g. "%s - x=y\n",
__func__) and that should be ok.

> Fix capitalization and phrasing in some cases.

No need to capitalise either. I'd even prefer sticking to lower-case
consistently.

> 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 f524aa9..22cfd88 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",

Drop this one or do use __func__ here.

>  		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,

Ditto.

>  		(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");

Drop.

>  	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);

Error message, so I agree that this should be spelled out, but not need
to capitalise "failed".

>  		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);

Please say what went wrong instead of what function failed. (I think I
already commented on this one when you added it, fix it at the source).

>  			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);

"failed to read break status: %d\n", or similar.

>  		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]);

__func__ is fine for compact debug messages, but you can remove the
verbose text if you want. I'd prefer the form

	"%s - x = a, y = b\n"

>  	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;
>  	}

A common "%s - break = %d\n" would do instead of the two above.

> -	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]);

I think you get the idea, so won't comment on the rest.

Thanks,
Johan

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 11/13] USB: ch341: improve B0 handling
  2016-04-15 21:14 ` [PATCH v4 11/13] USB: ch341: improve B0 handling Grigori Goronzy
@ 2016-04-29 13:41   ` Johan Hovold
  0 siblings, 0 replies; 31+ messages in thread
From: Johan Hovold @ 2016-04-29 13:41 UTC (permalink / raw)
  To: Grigori Goronzy; +Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel

On Fri, Apr 15, 2016 at 11:14:14PM +0200, Grigori Goronzy wrote:
> 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.

Ah, here it is. :) 

Please see if you can incorporate this before and when adding CRSTCTS
support.

Thanks,
Johan

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 12/13] USB: ch341: get rid of default configuration
  2016-04-15 21:14 ` [PATCH v4 12/13] USB: ch341: get rid of default configuration Grigori Goronzy
@ 2016-04-29 13:43   ` Johan Hovold
  0 siblings, 0 replies; 31+ messages in thread
From: Johan Hovold @ 2016-04-29 13:43 UTC (permalink / raw)
  To: Grigori Goronzy; +Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel

On Fri, Apr 15, 2016 at 11:14:15PM +0200, Grigori Goronzy wrote:
> If the serial port hasn't been opened yet, no baud rate should be
> set and RTS/DTR need to be deasserted.

But what about reset_resume?

Thanks,
Johan

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 13/13] USB: ch341: implement tx_empty callback
  2016-04-15 21:14 ` [PATCH v4 13/13] USB: ch341: implement tx_empty callback Grigori Goronzy
@ 2016-04-29 13:47   ` Johan Hovold
  0 siblings, 0 replies; 31+ messages in thread
From: Johan Hovold @ 2016-04-29 13:47 UTC (permalink / raw)
  To: Grigori Goronzy; +Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel

On Fri, Apr 15, 2016 at 11:14:16PM +0200, Grigori Goronzy wrote:
> 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 <greg@chown.ath.cx>

Looks good too.

Thanks for doing all this work, much appreciated. And sorry for the late
review, next round will be faster.

Thanks,
Johan

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 01/13] USB: ch341: fix error handling on resume
  2016-04-29 12:16   ` Johan Hovold
@ 2016-04-29 15:11     ` Grigori Goronzy
  2016-05-02 13:45       ` Johan Hovold
  0 siblings, 1 reply; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-29 15:11 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Johan Hovold

On 2016-04-29 14:16, Johan Hovold wrote:
> On Fri, Apr 15, 2016 at 11:14:04PM +0200, Grigori Goronzy wrote:
>> 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 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);
> 
> This is correct, but have noticed that resume is currently broken in
> that the interrupt urb is never resubmitted on resume in case the port 
> is
> already open?
> 
> Also ch341_configure must not use GFP_KERNEL either if called from a
> resume path (use GFP_NOIO).
> 
> Care to fix this up as well?
> 

Sure. How can I trigger a reset properly? AFAIR, I tried USBDEVFS_RESET 
and it didn't really do what I wanted, at least the reset_resume 
callback wasn't invoked.

Grigori

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 04/13] USB: ch341: fix USB buffer allocations
  2016-04-29 12:52   ` Johan Hovold
@ 2016-04-29 15:12     ` Grigori Goronzy
  0 siblings, 0 replies; 31+ messages in thread
From: Grigori Goronzy @ 2016-04-29 15:12 UTC (permalink / raw)
  To: Johan Hovold; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Johan Hovold

On 2016-04-29 14:52, Johan Hovold wrote:
>> @@ -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;
> 
> Did you reply to Oliver's comment about this change? Are you sure this
> won't break some old device expecting to be asked for eight bytes even
> if only two are returned?
> 

I'll just drop this change.

Grigori

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v4 01/13] USB: ch341: fix error handling on resume
  2016-04-29 15:11     ` Grigori Goronzy
@ 2016-05-02 13:45       ` Johan Hovold
  0 siblings, 0 replies; 31+ messages in thread
From: Johan Hovold @ 2016-05-02 13:45 UTC (permalink / raw)
  To: Grigori Goronzy
  Cc: Johan Hovold, Greg Kroah-Hartman, linux-usb, linux-kernel, Johan Hovold

On Fri, Apr 29, 2016 at 05:11:13PM +0200, Grigori Goronzy wrote:
> On 2016-04-29 14:16, Johan Hovold wrote:
> > On Fri, Apr 15, 2016 at 11:14:04PM +0200, Grigori Goronzy wrote:
> >> 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 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);
> > 
> > This is correct, but have noticed that resume is currently broken in
> > that the interrupt urb is never resubmitted on resume in case the port 
> > is
> > already open?
> > 
> > Also ch341_configure must not use GFP_KERNEL either if called from a
> > resume path (use GFP_NOIO).
> > 
> > Care to fix this up as well?
> > 
> 
> Sure. How can I trigger a reset properly? AFAIR, I tried USBDEVFS_RESET 
> and it didn't really do what I wanted, at least the reset_resume 
> callback wasn't invoked.

You should be able to use the USB persistent-device functionality or
temporarily enable the USB_QUIRK_RESET_RESUME quirk for your device to
have reset_resume be called on system resume, but note that a resume
callback also needs to be implemented for normal (system and runtime)
resume.

Also note that reset_resume for usb-serial is currently broken in
mainline (since 4.5).

Thanks,
Johan

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2016-05-02 13:45 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-15 21:14 Major improvements to the ch341 driver v4 Grigori Goronzy
2016-04-15 21:14 ` [PATCH v4 01/13] USB: ch341: fix error handling on resume Grigori Goronzy
2016-04-29 12:16   ` Johan Hovold
2016-04-29 15:11     ` Grigori Goronzy
2016-05-02 13:45       ` Johan Hovold
2016-04-15 21:14 ` [PATCH v4 02/13] USB: ch341: add LCR register definitions Grigori Goronzy
2016-04-29 12:18   ` Johan Hovold
2016-04-15 21:14 ` [PATCH v4 03/13] USB: ch341: add definitions for modem control Grigori Goronzy
2016-04-29 12:22   ` Johan Hovold
2016-04-15 21:14 ` [PATCH v4 04/13] USB: ch341: fix USB buffer allocations Grigori Goronzy
2016-04-29 12:52   ` Johan Hovold
2016-04-29 15:12     ` Grigori Goronzy
2016-04-15 21:14 ` [PATCH v4 05/13] USB: ch341: reinitialize chip on reconfiguration Grigori Goronzy
2016-04-29 13:03   ` Johan Hovold
2016-04-15 21:14 ` [PATCH v4 06/13] USB: ch341: add support for parity, frame length, stop bits Grigori Goronzy
2016-04-29 13:11   ` Johan Hovold
2016-04-15 21:14 ` [PATCH v4 07/13] USB: ch341: add debug output for chip version Grigori Goronzy
2016-04-29 13:13   ` Johan Hovold
2016-04-15 21:14 ` [PATCH v4 08/13] USB: ch341: add support for RTS/CTS flow control Grigori Goronzy
2016-04-29 13:23   ` Johan Hovold
2016-04-15 21:14 ` [PATCH v4 09/13] USB: ch341: fix coding style Grigori Goronzy
2016-04-29 13:29   ` Johan Hovold
2016-04-15 21:14 ` [PATCH v4 10/13] USB: ch341: clean up messages Grigori Goronzy
2016-04-29 13:40   ` Johan Hovold
2016-04-15 21:14 ` [PATCH v4 11/13] USB: ch341: improve B0 handling Grigori Goronzy
2016-04-29 13:41   ` Johan Hovold
2016-04-15 21:14 ` [PATCH v4 12/13] USB: ch341: get rid of default configuration Grigori Goronzy
2016-04-29 13:43   ` Johan Hovold
2016-04-15 21:14 ` [PATCH v4 13/13] USB: ch341: implement tx_empty callback Grigori Goronzy
2016-04-29 13:47   ` Johan Hovold
2016-04-28 23:24 ` Major improvements to the ch341 driver v4 Grigori Goronzy

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.