[PATCH] USB: serial: ch341: fix simulated-break comment

[PATCH] USB: serial: ch341: fix simulated-break comment

[Johan Hovold]

On devices which do not support break signalling a break condition is
simulated by sending a NUL byte at the lowest possible speed. The break
condition will be 9 bit periods long (start bit and eight data bits),
but the transmission itself also includes the stop bit.

Signed-off-by: Johan Hovold <johan@kernel.org>
---

Hi Michael,

I reread the break-end comment and found it a bit confusing still. The
below seems more correct to me. I'm assuming you did not intend to add
an additional bit period as margin?

Johan



 drivers/usb/serial/ch341.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 011d7953f087..27a2a62777c9 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -604,9 +604,8 @@ static void ch341_simulate_break(struct tty_struct *tty, int break_state)
 		}
 
 		/*
-		 * Compute expected transmission duration and add a single bit
-		 * of safety margin (the actual NUL byte transmission is 8 bits
-		 * plus one stop bit).
+		 * Compute expected transmission duration (including stop
+		 * bit).
 		 */
 		priv->break_end = jiffies + (10 * HZ / CH341_MIN_BPS);
 
-- 
2.26.2

Re: [PATCH] USB: serial: ch341: fix simulated-break comment

[Michael Hanselmann]

On devices which do not support break signalling a break condition is
simulated by sending a NUL byte at the lowest possible speed. The break
condition will be 9 bit periods long (start bit and eight data bits),
but the transmission itself also includes the stop bit. The safety
margin of one bit is kept to account for timing differences.

Signed-off-by: Michael Hanselmann <public@hansmi.ch>
---

Hi Johan

On 07.07.20 08:19, Johan Hovold wrote:
> I reread the break-end comment and found it a bit confusing still. The
> below seems more correct to me. I'm assuming you did not intend to add
> an additional bit period as margin?

The additional bit was intentional, but I missed the start bit and was
off by one. As such your fix indeed addresses the inconsistency between
the comment and code. Considering the general quality of the ch341
chips needing the simulation and to account for timing differences I'd
instead prefer to increase the delay from 10 to 11 bits (1 start, 8 data,
1 stop, 1 margin).

Michael

 drivers/usb/serial/ch341.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 0cb02d1bde02..580aa5833091 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -603,11 +603,13 @@ static void ch341_simulate_break(struct tty_struct *tty, int break_state)
 		}
 
 		/*
-		 * Compute expected transmission duration and add a single bit
-		 * of safety margin (the actual NUL byte transmission is 8 bits
-		 * plus one stop bit).
+		 * Compute expected transmission duration including safety
+		 * margin. The original baud rate is only restored after the
+		 * computed point in time.
+		 *
+		 * 11 bits = 1 start, 8 data, 1 stop, 1 margin
 		 */
-		priv->break_end = jiffies + (10 * HZ / CH341_MIN_BPS);
+		priv->break_end = jiffies + (11 * HZ / CH341_MIN_BPS);
 
 		return;
 	}
-- 
2.20.1

Re: [PATCH] USB: serial: ch341: fix simulated-break comment

[Johan Hovold]

On Tue, Jul 07, 2020 at 04:53:22PM +0200, Michael Hanselmann wrote:
> On devices which do not support break signalling a break condition is
> simulated by sending a NUL byte at the lowest possible speed. The break
> condition will be 9 bit periods long (start bit and eight data bits),
> but the transmission itself also includes the stop bit. The safety
> margin of one bit is kept to account for timing differences.
> 
> Signed-off-by: Michael Hanselmann <public@hansmi.ch>
> ---
> 
> Hi Johan
> 
> On 07.07.20 08:19, Johan Hovold wrote:
> > I reread the break-end comment and found it a bit confusing still. The
> > below seems more correct to me. I'm assuming you did not intend to add
> > an additional bit period as margin?
> 
> The additional bit was intentional, but I missed the start bit and was
> off by one. As such your fix indeed addresses the inconsistency between
> the comment and code. Considering the general quality of the ch341
> chips needing the simulation and to account for timing differences I'd
> instead prefer to increase the delay from 10 to 11 bits (1 start, 8 data,
> 1 stop, 1 margin).

Thanks for confirming. I've applied this patch with a slightly modified
commit message:

	https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-next

Johan