| Message ID | 20200707061957.17425-1-johan@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: serial: ch341: fix simulated-break comment | expand |
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; }
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
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);
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(-)