[PATCH 0/4] ch341: Add support for HL340 devices

[PATCH 0/4] ch341: Add support for HL340 devices

[Michael Hanselmann]

A subset of CH341 devices does not support all features, namely the
prescaler is limited to a reduced precision and there is no support for
sending a RS232 break condition.

These devices can usually be identified by an imprint of "340" on the
turquoise-colored plug. They're also sometimes called "HL340", hence the
terminology in this series and driver.

This series adds detection of these devices, adjusts the
divisor/prescaler setup and implements a simulated break condition.

Michael Hanselmann (4):
  ch341: Name more registers
  ch341: Detect HL340 variant
  ch341: Limit prescaler on HL340 variant
  ch341: Simulate break condition on HL340 variant

 drivers/usb/serial/ch341.c | 196 +++++++++++++++++++++++++++++++++----
 1 file changed, 176 insertions(+), 20 deletions(-)

-- 
2.20.1

[PATCH 1/4] ch341: Name more registers

[Michael Hanselmann]

Add more #defines with register names.

Signed-off-by: Michael Hanselmann <public@hansmi.ch>
---
 drivers/usb/serial/ch341.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index c5ecdcd51ffc..518209072c50 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -59,6 +59,8 @@
 #define CH341_REQ_MODEM_CTRL   0xA4
 
 #define CH341_REG_BREAK        0x05
+#define CH341_REG_PRESCALER    0x12
+#define CH341_REG_DIVISOR      0x13
 #define CH341_REG_LCR          0x18
 #define CH341_NBREAK_BITS      0x01
 
@@ -221,6 +223,7 @@ static int ch341_get_divisor(speed_t speed)
 static int ch341_set_baudrate_lcr(struct usb_device *dev,
 				  struct ch341_private *priv, u8 lcr)
 {
+	uint16_t reg;
 	int val;
 	int r;
 
@@ -237,11 +240,15 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
 	 */
 	val |= BIT(7);
 
-	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x1312, val);
+	reg = ((uint16_t)CH341_REG_DIVISOR << 8) | CH341_REG_PRESCALER;
+
+	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, reg, val);
 	if (r)
 		return r;
 
-	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x2518, lcr);
+	reg = ((uint16_t)0x2500) | CH341_REG_LCR;
+
+	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, reg, lcr);
 	if (r)
 		return r;
 
-- 
2.20.1

[PATCH 2/4] ch341: Detect HL340 variant

[Michael Hanselmann]

A subset of CH341 devices does not support all features, namely the
prescaler is limited to a reduced precision and there is no support for
sending a RS232 break condition.

These devices can usually be identified by an imprint of "340" on the
turquoise-colored plug. They're called "HL340" in this driver.

Signed-off-by: Michael Hanselmann <public@hansmi.ch>
---
 drivers/usb/serial/ch341.c | 42 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 518209072c50..0523f46f53c7 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -89,6 +89,7 @@ struct ch341_private {
 	u8 mcr;
 	u8 msr;
 	u8 lcr;
+	u8 flags;
 };
 
 static void ch341_set_termios(struct tty_struct *tty,
@@ -315,6 +316,43 @@ out:	kfree(buffer);
 	return r;
 }
 
+/*
+ * A subset of CH341 devices, called "HL340" in this driver, does not support
+ * all features. The prescaler is limited and there is no support for sending
+ * a RS232 break condition. A read failure when trying to set up the latter is
+ * used to detect these devices.
+ */
+static int ch341_detect_hl340(struct usb_device *dev)
+{
+	const unsigned int size = 2;
+	char *buffer;
+	int r;
+
+	buffer = kmalloc(size, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	r = ch341_control_in(dev, CH341_REQ_READ_REG,
+			     CH341_REG_BREAK, 0, buffer, size);
+	if (r == -EPIPE) {
+		dev_dbg(&dev->dev, "%s - Chip is a HL340 variant\n",
+			__func__);
+		r = 1;
+		goto out;
+	}
+
+	if (r < 0) {
+		dev_err(&dev->dev, "%s - USB control read error (%d)\n",
+			__func__, r);
+		goto out;
+	}
+
+	r = 0;
+
+out:	kfree(buffer);
+	return r;
+}
+
 static int ch341_port_probe(struct usb_serial_port *port)
 {
 	struct ch341_private *priv;
@@ -336,6 +374,10 @@ static int ch341_port_probe(struct usb_serial_port *port)
 	if (r < 0)
 		goto error;
 
+	r = ch341_detect_hl340(port->serial->dev);
+	if (r < 0)
+		goto error;
+
 	usb_set_serial_port_data(port, priv);
 	return 0;
 
-- 
2.20.1

[PATCH 3/4] ch341: Limit prescaler on HL340 variant

[Michael Hanselmann]

HL340 devices, a subset of all CH340 devices, do not work correctly when
the highest prescaler bit (0b100) is set. Limit these to the lower
prescaler values at the cost of timing precision.

Signed-off-by: Michael Hanselmann <public@hansmi.ch>
---
 drivers/usb/serial/ch341.c | 53 ++++++++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 0523f46f53c7..48a704174aec 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -24,6 +24,8 @@
 #define DEFAULT_BAUD_RATE 9600
 #define DEFAULT_TIMEOUT   1000
 
+#define CH341_QUIRK_LIMITED_PRESCALER 0x01
+
 /* flags for IO-Bits */
 #define CH341_BIT_RTS (1 << 6)
 #define CH341_BIT_DTR (1 << 5)
@@ -143,13 +145,19 @@ static int ch341_control_in(struct usb_device *dev,
 
 #define CH341_CLKRATE		48000000
 #define CH341_CLK_DIV(ps, fact)	(1 << (12 - 3 * (ps) - (fact)))
-#define CH341_MIN_RATE(ps)	(CH341_CLKRATE / (CH341_CLK_DIV((ps), 1) * 512))
+#define CH341_MIN_RATE(ps, fact) \
+	(CH341_CLKRATE / (CH341_CLK_DIV((ps), (fact)) * 512))
 
 static const speed_t ch341_min_rates[] = {
-	CH341_MIN_RATE(0),
-	CH341_MIN_RATE(1),
-	CH341_MIN_RATE(2),
-	CH341_MIN_RATE(3),
+	CH341_MIN_RATE(0, 0),
+	CH341_MIN_RATE(1, 0),
+	CH341_MIN_RATE(2, 0),
+	CH341_MIN_RATE(3, 0),
+
+	CH341_MIN_RATE(0, 1),
+	CH341_MIN_RATE(1, 1),
+	CH341_MIN_RATE(2, 1),
+	CH341_MIN_RATE(3, 1),
 };
 
 /*
@@ -162,24 +170,41 @@ static const speed_t ch341_min_rates[] = {
  *		2 <= div <= 256 if fact = 0, or
  *		9 <= div <= 256 if fact = 1
  */
-static int ch341_get_divisor(speed_t speed)
+static int ch341_get_divisor(struct ch341_private *priv)
 {
+	const speed_t *min_rates;
+	speed_t speed;
 	unsigned int fact, div, clk_div;
 	int ps;
 
+	speed = priv->baud_rate;
+
 	/*
 	 * Clamp to supported range, this makes the (ps < 0) and (div < 2)
 	 * sanity checks below redundant.
 	 */
 	speed = clamp(speed, 46U, 3000000U);
 
-	/*
-	 * Start with highest possible base clock (fact = 1) that will give a
-	 * divisor strictly less than 512.
-	 */
-	fact = 1;
+	if (priv->flags & CH341_QUIRK_LIMITED_PRESCALER) {
+		/*
+		 * Devices of the "HL340" variant don't work reliably when the
+		 * third bit is set in the prescaler (0b100). Limit these to
+		 * prescaler values in the range 0..3 (fact = 0) at the cost of
+		 * precision.
+		 */
+		min_rates = &ch341_min_rates[4];
+		fact = 0;
+	} else {
+		/*
+		 * Start with highest possible base clock (fact = 1) that will
+		 * give a divisor strictly less than 512.
+		 */
+		min_rates = ch341_min_rates;
+		fact = 1;
+	}
+
 	for (ps = 3; ps >= 0; ps--) {
-		if (speed > ch341_min_rates[ps])
+		if (speed > min_rates[ps])
 			break;
 	}
 
@@ -231,7 +256,7 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
 	if (!priv->baud_rate)
 		return -EINVAL;
 
-	val = ch341_get_divisor(priv->baud_rate);
+	val = ch341_get_divisor(priv);
 	if (val < 0)
 		return -EINVAL;
 
@@ -377,6 +402,8 @@ static int ch341_port_probe(struct usb_serial_port *port)
 	r = ch341_detect_hl340(port->serial->dev);
 	if (r < 0)
 		goto error;
+	else if (r != 0)
+		priv->flags |= CH341_QUIRK_LIMITED_PRESCALER;
 
 	usb_set_serial_port_data(port, priv);
 	return 0;
-- 
2.20.1

[PATCH 4/4] ch341: Simulate break condition on HL340 variant

[Michael Hanselmann]

ch34x devices of the "HL340" variant don't support a real break
condition. This fact is already used in the ch341_detect_hl340 function.
With this change a quirk is implemented to simulate a break condition by
temporarily lowering the baud rate and sending a NUL byte.

The primary drawback of this approach is that the duration of the break
can't be controlled by userland.

Signed-off-by: Michael Hanselmann <public@hansmi.ch>
---
 drivers/usb/serial/ch341.c | 102 +++++++++++++++++++++++++++++++++----
 1 file changed, 91 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 48a704174aec..459a27a6ebcc 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -25,6 +25,10 @@
 #define DEFAULT_TIMEOUT   1000
 
 #define CH341_QUIRK_LIMITED_PRESCALER 0x01
+#define CH341_QUIRK_SIMULATE_BREAK    0x02
+
+/* Minimum baud rate */
+#define CH341_MIN_BPS 46U
 
 /* flags for IO-Bits */
 #define CH341_BIT_RTS (1 << 6)
@@ -92,6 +96,7 @@ struct ch341_private {
 	u8 msr;
 	u8 lcr;
 	u8 flags;
+	unsigned long break_end;
 };
 
 static void ch341_set_termios(struct tty_struct *tty,
@@ -170,20 +175,17 @@ static const speed_t ch341_min_rates[] = {
  *		2 <= div <= 256 if fact = 0, or
  *		9 <= div <= 256 if fact = 1
  */
-static int ch341_get_divisor(struct ch341_private *priv)
+static int ch341_get_divisor(struct ch341_private *priv, speed_t speed)
 {
 	const speed_t *min_rates;
-	speed_t speed;
 	unsigned int fact, div, clk_div;
 	int ps;
 
-	speed = priv->baud_rate;
-
 	/*
 	 * Clamp to supported range, this makes the (ps < 0) and (div < 2)
 	 * sanity checks below redundant.
 	 */
-	speed = clamp(speed, 46U, 3000000U);
+	speed = clamp(speed, CH341_MIN_BPS, 3000000U);
 
 	if (priv->flags & CH341_QUIRK_LIMITED_PRESCALER) {
 		/*
@@ -247,16 +249,17 @@ static int ch341_get_divisor(struct ch341_private *priv)
 }
 
 static int ch341_set_baudrate_lcr(struct usb_device *dev,
-				  struct ch341_private *priv, u8 lcr)
+				  struct ch341_private *priv,
+				  unsigned baud_rate, u8 lcr)
 {
 	uint16_t reg;
 	int val;
 	int r;
 
-	if (!priv->baud_rate)
+	if (!baud_rate)
 		return -EINVAL;
 
-	val = ch341_get_divisor(priv);
+	val = ch341_get_divisor(priv, baud_rate);
 	if (val < 0)
 		return -EINVAL;
 
@@ -331,7 +334,7 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)
 	if (r < 0)
 		goto out;
 
-	r = ch341_set_baudrate_lcr(dev, priv, priv->lcr);
+	r = ch341_set_baudrate_lcr(dev, priv, priv->baud_rate, priv->lcr);
 	if (r < 0)
 		goto out;
 
@@ -403,7 +406,9 @@ static int ch341_port_probe(struct usb_serial_port *port)
 	if (r < 0)
 		goto error;
 	else if (r != 0)
-		priv->flags |= CH341_QUIRK_LIMITED_PRESCALER;
+		priv->flags |=
+			CH341_QUIRK_LIMITED_PRESCALER |
+			CH341_QUIRK_SIMULATE_BREAK;
 
 	usb_set_serial_port_data(port, priv);
 	return 0;
@@ -536,7 +541,8 @@ static void ch341_set_termios(struct tty_struct *tty,
 	if (baud_rate) {
 		priv->baud_rate = baud_rate;
 
-		r = ch341_set_baudrate_lcr(port->serial->dev, priv, lcr);
+		r = ch341_set_baudrate_lcr(port->serial->dev, priv,
+					   priv->baud_rate, lcr);
 		if (r < 0 && old_termios) {
 			priv->baud_rate = tty_termios_baud_rate(old_termios);
 			tty_termios_copy_hw(&tty->termios, old_termios);
@@ -555,15 +561,89 @@ static void ch341_set_termios(struct tty_struct *tty,
 	ch341_set_handshake(port->serial->dev, priv->mcr);
 }
 
+/*
+ * Devices of the "HL340" variant don't support a real break condition and
+ * reading CH341_REG_BREAK fails (see also ch341_detect_hl340). This function
+ * simulates a break condition by lowering the baud rate to the minimum
+ * supported by the hardware upon enabling the break condition and sending
+ * a NUL byte.
+ *
+ * Normally the duration of the break condition can be controlled individually
+ * by userspace using TIOCSBRK and TIOCCBRK or by passing an argument to
+ * TCSBRKP. Due to how the simulation is implemented the duration can't be
+ * controlled. The duration is always 1s / 46bd * 10bit = 217ms.
+ */
+static void ch341_simulate_break(struct tty_struct *tty, int break_state)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	struct ch341_private *priv = usb_get_serial_port_data(port);
+	unsigned long delay;
+	int r;
+
+	if (break_state != 0) {
+		dev_dbg(&port->dev, "%s - Enter break state requested\n",
+			__func__);
+
+		r = ch341_set_baudrate_lcr(port->serial->dev, priv,
+			CH341_MIN_BPS,
+			CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX |
+			CH341_LCR_CS8);
+		if (r < 0) {
+			dev_err(&port->dev, "%s - baud rate status %d\n",
+				__func__, r);
+			goto restore;
+		}
+
+		r = tty_put_char(tty, '\0');
+		if (r < 0) {
+			dev_err(&port->dev, "%s - write status %d\n",
+				__func__, r);
+			goto restore;
+		}
+
+		priv->break_end = jiffies + (10 * HZ / CH341_MIN_BPS);
+
+		return;
+	}
+
+	dev_dbg(&port->dev, "%s - Leave break state requested\n", __func__);
+
+	if (time_before(jiffies, priv->break_end)) {
+		/* Wait until NUL byte is written */
+		delay = min_t(unsigned long, HZ, priv->break_end - jiffies);
+
+		dev_dbg(&port->dev, "%s - sleep for %d ms\n", __func__,
+			jiffies_to_msecs(delay));
+		schedule_timeout_interruptible(delay);
+	}
+
+restore:
+	/* Restore original baud rate */
+	r = ch341_set_baudrate_lcr(port->serial->dev, priv, priv->baud_rate,
+				   priv->lcr);
+	if (r < 0)
+		dev_err(&port->dev, "%s - baud rate status %d\n", __func__, r);
+}
+
 static void ch341_break_ctl(struct tty_struct *tty, int break_state)
 {
 	const uint16_t ch341_break_reg =
 			((uint16_t) CH341_REG_LCR << 8) | CH341_REG_BREAK;
 	struct usb_serial_port *port = tty->driver_data;
+	struct ch341_private *priv = usb_get_serial_port_data(port);
 	int r;
 	uint16_t reg_contents;
 	uint8_t *break_reg;
 
+	if (priv->flags & CH341_QUIRK_SIMULATE_BREAK) {
+		dev_warn_once(&port->dev,
+			      "%s - hardware doesn't support real break"
+			      " condition, simulating instead\n",
+			      __func__);
+		ch341_simulate_break(tty, break_state);
+		return;
+	}
+
 	break_reg = kmalloc(2, GFP_KERNEL);
 	if (!break_reg)
 		return;
-- 
2.20.1

Re: [PATCH 0/4] ch341: Add support for HL340 devices

[Johan Hovold]

Hi Michael,

On Fri, Mar 06, 2020 at 07:00:41PM +0000, Michael Hanselmann wrote:
> A subset of CH341 devices does not support all features, namely the
> prescaler is limited to a reduced precision and there is no support for
> sending a RS232 break condition.
> 
> These devices can usually be identified by an imprint of "340" on the
> turquoise-colored plug. They're also sometimes called "HL340", hence the
> terminology in this series and driver.

You need to come up with a different designation. I have a HL340 device
here which works just fine.

> This series adds detection of these devices, adjusts the
> divisor/prescaler setup and implements a simulated break condition.
> 
> Michael Hanselmann (4):
>   ch341: Name more registers
>   ch341: Detect HL340 variant
>   ch341: Limit prescaler on HL340 variant
>   ch341: Simulate break condition on HL340 variant

Nit: please use a "USB: serial: ch341:" subject prefix.

Also, if possible, please move the second and third patches first in the
series as these could be considered fixes rather than new features (and
considered for backporting).

>  drivers/usb/serial/ch341.c | 196 +++++++++++++++++++++++++++++++++----
>  1 file changed, 176 insertions(+), 20 deletions(-)

Johan

Re: [PATCH 1/4] ch341: Name more registers

[Johan Hovold]

On Fri, Mar 06, 2020 at 07:00:42PM +0000, Michael Hanselmann wrote:
> Add more #defines with register names.

Please be more specific (e.g. which registers are you naming). Update
the patch summary too.

> Signed-off-by: Michael Hanselmann <public@hansmi.ch>
> ---
>  drivers/usb/serial/ch341.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index c5ecdcd51ffc..518209072c50 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -59,6 +59,8 @@
>  #define CH341_REQ_MODEM_CTRL   0xA4
>  
>  #define CH341_REG_BREAK        0x05
> +#define CH341_REG_PRESCALER    0x12
> +#define CH341_REG_DIVISOR      0x13
>  #define CH341_REG_LCR          0x18
>  #define CH341_NBREAK_BITS      0x01
>  
> @@ -221,6 +223,7 @@ static int ch341_get_divisor(speed_t speed)
>  static int ch341_set_baudrate_lcr(struct usb_device *dev,
>  				  struct ch341_private *priv, u8 lcr)
>  {
> +	uint16_t reg;

Use u16.

>  	int val;
>  	int r;
>  
> @@ -237,11 +240,15 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
>  	 */
>  	val |= BIT(7);
>  
> -	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x1312, val);
> +	reg = ((uint16_t)CH341_REG_DIVISOR << 8) | CH341_REG_PRESCALER;

No need to cast.

> +
> +	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, reg, val);
>  	if (r)
>  		return r;
>  
> -	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x2518, lcr);
> +	reg = ((uint16_t)0x2500) | CH341_REG_LCR;

Ditto, and please add a bit shift for consistency.

Hmm, but this is unrelated to the defines you are adding, and there are
other magic constants for registers in this driver. Perhaps drop this
bit or break it out in its own patch (rule of thumb: one logical change
per patch).

> +
> +	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, reg, lcr);
>  	if (r)
>  		return r;

I'd also move this last in the series as it's more of a clean up or
documentation patch.

Johan

Re: [PATCH 2/4] ch341: Detect HL340 variant

[Johan Hovold]

On Fri, Mar 06, 2020 at 07:00:43PM +0000, Michael Hanselmann wrote:
> A subset of CH341 devices does not support all features, namely the
> prescaler is limited to a reduced precision and there is no support for
> sending a RS232 break condition.
> 
> These devices can usually be identified by an imprint of "340" on the
> turquoise-colored plug. They're called "HL340" in this driver.

As I mentioned in my reply to the cover letter, you need to come up with
a different designation as this does not apply to all HL340 devices.
 
> Signed-off-by: Michael Hanselmann <public@hansmi.ch>
> ---
>  drivers/usb/serial/ch341.c | 42 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 518209072c50..0523f46f53c7 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -89,6 +89,7 @@ struct ch341_private {
>  	u8 mcr;
>  	u8 msr;
>  	u8 lcr;
> +	u8 flags;
>  };
>  
>  static void ch341_set_termios(struct tty_struct *tty,
> @@ -315,6 +316,43 @@ out:	kfree(buffer);
>  	return r;
>  }
>  
> +/*
> + * A subset of CH341 devices, called "HL340" in this driver, does not support
> + * all features. The prescaler is limited and there is no support for sending
> + * a RS232 break condition. A read failure when trying to set up the latter is
> + * used to detect these devices.
> + */
> +static int ch341_detect_hl340(struct usb_device *dev)

Return bool? Rename ch341_is_xxx() ?

> +{
> +	const unsigned int size = 2;
> +	char *buffer;
> +	int r;
> +
> +	buffer = kmalloc(size, GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	r = ch341_control_in(dev, CH341_REQ_READ_REG,
> +			     CH341_REG_BREAK, 0, buffer, size);

This helper would already have logged an error message, which perhaps is
ok, but you could consider using usb_control_msg() directly here.

> +	if (r == -EPIPE) {
> +		dev_dbg(&dev->dev, "%s - Chip is a HL340 variant\n",
> +			__func__);
> +		r = 1;
> +		goto out;
> +	}
> +
> +	if (r < 0) {
> +		dev_err(&dev->dev, "%s - USB control read error (%d)\n",
> +			__func__, r);
> +		goto out;
> +	}

So this is currently redundant.

> +
> +	r = 0;

Not needed either, right?

> +
> +out:	kfree(buffer);

Line break after the label, please.

> +	return r;
> +}
> +
>  static int ch341_port_probe(struct usb_serial_port *port)
>  {
>  	struct ch341_private *priv;
> @@ -336,6 +374,10 @@ static int ch341_port_probe(struct usb_serial_port *port)
>  	if (r < 0)
>  		goto error;
>  
> +	r = ch341_detect_hl340(port->serial->dev);
> +	if (r < 0)
> +		goto error;

You never store the return value (and the "flags" variable you add is
unused) which indicates your series needs to be restructured.

At least update flags in this patch. Perhaps consider renaming it
"quirks" depending on how it ends up being used.

> +
>  	usb_set_serial_port_data(port, priv);
>  	return 0;

Johan

Re: [PATCH 3/4] ch341: Limit prescaler on HL340 variant

[Johan Hovold]

On Fri, Mar 06, 2020 at 07:00:44PM +0000, Michael Hanselmann wrote:
> HL340 devices, a subset of all CH340 devices, do not work correctly when
> the highest prescaler bit (0b100) is set. Limit these to the lower
> prescaler values at the cost of timing precision.

When we discussed this off list, you said that your device could handle
the highest prescaler bit being set for some rates. You specifically
confirmed that the 576000 and 921600 rates worked, while 110, 134 and
200 did not.

Could you reconfirm which, if any, of the following rates work with the
current driver?

	    1152000
             921600
             576000
                200
                134
                110

Perhaps we can still continue supporting the higher rates, which are way
off unless using the factor-2 prescaler.

Johan

Re: [PATCH 4/4] ch341: Simulate break condition on HL340 variant

[Johan Hovold]

On Fri, Mar 06, 2020 at 07:00:45PM +0000, Michael Hanselmann wrote:
> ch34x devices of the "HL340" variant don't support a real break
> condition. This fact is already used in the ch341_detect_hl340 function.
> With this change a quirk is implemented to simulate a break condition by
> temporarily lowering the baud rate and sending a NUL byte.
> 
> The primary drawback of this approach is that the duration of the break
> can't be controlled by userland.

Hmm. I'll have to get back to you on this. Please continue keeping it
last in the series if you end up resubmitting before.

Johan

Re: [PATCH 1/4] ch341: Name more registers

[Michael Hanselmann]

On 24.03.20 11:20, Johan Hovold wrote:
> On Fri, Mar 06, 2020 at 07:00:42PM +0000, Michael Hanselmann wrote:
>> Add more #defines with register names.
> 
> Please be more specific (e.g. which registers are you naming). Update
> the patch summary too.

Done. Will be in the next revision.

>> -	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x2518, lcr);
>> +	reg = ((uint16_t)0x2500) | CH341_REG_LCR;
> 
> Ditto, and please add a bit shift for consistency.
> 
> Hmm, but this is unrelated to the defines you are adding, and there are
> other magic constants for registers in this driver. Perhaps drop this
> bit or break it out in its own patch (rule of thumb: one logical change
> per patch).

I figured out what the 0x25 register is, or maybe was, and updated the
patch with a description and moved it later in the series, just before
the simulated break condition.

Michael

Re: [PATCH 0/4] ch341: Add support for HL340 devices

[Michael Hanselmann]

On 24.03.20 11:05, Johan Hovold wrote:
> On Fri, Mar 06, 2020 at 07:00:41PM +0000, Michael Hanselmann wrote:
>> A subset of CH341 devices does not support all features, namely the
>> prescaler is limited to a reduced precision and there is no support for
>> sending a RS232 break condition.
>>
>> These devices can usually be identified by an imprint of "340" on the
>> turquoise-colored plug. They're also sometimes called "HL340", hence the
>> terminology in this series and driver.
> 
> You need to come up with a different designation. I have a HL340 device
> here which works just fine.

Thank you, I didn't know whether the behaviour I'm seeing affects others
too. I removed the "HL340" designation for the next revision.

Re: [PATCH 2/4] ch341: Detect HL340 variant

[Michael Hanselmann]

On 24.03.20 11:31, Johan Hovold wrote:
> On Fri, Mar 06, 2020 at 07:00:43PM +0000, Michael Hanselmann wrote:
>> +static int ch341_detect_hl340(struct usb_device *dev)
> 
> Return bool? Rename ch341_is_xxx() ?

Changed the function to return the quirks flags, its name is now
"ch341_detect_quirks".

>> +	r = ch341_control_in(dev, CH341_REQ_READ_REG,
>> +			     CH341_REG_BREAK, 0, buffer, size);
> 
> This helper would already have logged an error message, which perhaps is
> ok, but you could consider using usb_control_msg() directly here.

Changed to use "usb_control_msg" directly.

>> +	if (r == -EPIPE) {
>> +		dev_dbg(&dev->dev, "%s - Chip is a HL340 variant\n",
>> +			__func__);
>> +		r = 1;
>> +		goto out;
>> +	}
>> +
>> +	if (r < 0) {
>> +		dev_err(&dev->dev, "%s - USB control read error (%d)\n",
>> +			__func__, r);
>> +		goto out;
>> +	}
> 
> So this is currently redundant.

Not anymore after the change above: EPIPE is encountered for the limited
chips, 0 for normal chips and other errors should be reported.

>> +
>> +out:	kfree(buffer);
> 
> Line break after the label, please.

Tried to keep the style consistent (see "ch341_port_probe" for example).
Done.

>> +	r = ch341_detect_hl340(port->serial->dev);
>> +	if (r < 0)
>> +		goto error;
> 
> You never store the return value (and the "flags" variable you add is
> unused) which indicates your series needs to be restructured.

That's different now with the renaming to "ch341_detect_quirks". The
actual return value is still 0 as of this patch. Quirks are added in
later patches.

> At least update flags in this patch. Perhaps consider renaming it
> "quirks" depending on how it ends up being used.

Done.

Re: [PATCH 3/4] ch341: Limit prescaler on HL340 variant

[Michael Hanselmann]

On 24.03.20 11:41, Johan Hovold wrote:
> On Fri, Mar 06, 2020 at 07:00:44PM +0000, Michael Hanselmann wrote:
>> HL340 devices, a subset of all CH340 devices, do not work correctly when
>> the highest prescaler bit (0b100) is set. Limit these to the lower
>> prescaler values at the cost of timing precision.
> 
> When we discussed this off list, you said that your device could handle
> the highest prescaler bit being set for some rates. You specifically
> confirmed that the 576000 and 921600 rates worked, while 110, 134 and
> 200 did not.
> 
> Could you reconfirm which, if any, of the following rates work with the
> current driver?
> 
> 	    1152000
>              921600
>              576000
>                 200
>                 134
>                 110
> 
> Perhaps we can still continue supporting the higher rates, which are way
> off unless using the factor-2 prescaler.

You're right, 110, 134 and 200 don't work whereas 576000, 921600 and
1152000 do. I totally missed this while working on the patch.

I ended up doing more research and figured out that fact=0 with ps=0..3
works, as does fact=1 with ps=3. It's only fact=1 with ps0..2 which is
not working properly.

The next revision of the series will contain additional patches to
restructure the prescaler computation before implementing the quirk.

Michael