Re: usb : serial : ch341 : set tty baud speed according to tty struct
Johan Hovold jo...@kernel.org wrote: Yes, there shouldn't be a need to store the baud rate in the driver (the tty layer will keep track of that), but you probably still want to store the state of the modem-control signals. Sounds right. The modem control signals I'm having a harder time setting up good tests for, but it's coming together. I've been testing with a cp2104 connected back to back with a ch340 device, and Looks like we could get rid of that ch341_configure at every open too? That's the plan yes. With more of the control requests decoded, the _configure call was really just a badly implemented set_termios. Looking forward to seeing your patches when you're done. Me too! It's slow going in the evenings, but I'm getting more comfortable with it slowly but surely :) Cheers, Karl P
Re: Re: usb : serial : ch341 : set tty baud speed according to tty struct
Johan Hovold jo...@kernel.org wrote: On Wed, Feb 18, 2015 at 11:32:38AM +0700, Johan Hovold wrote: On Tue, Feb 17, 2015 at 10:45:11PM -0500, Nicolas PLANEL wrote: I believe the fix should be implemented slightly differently however. Most usb-serial driver call set_termios from open to handle this issue. It looks like you could simply replace the calls to set baudrate and handshake in open with ch341_set_termios(tty, port, NULL); You currently need to make sure tty is not NULL in case the device is being used as a console by the way. Just skip the set_termios call in that case. I've already got most of this working in my branch overhauling this driver. I agree, the open call shouldn't be doing all the hard resetting of settings that it currently does. I've implemented more of set termios / get termios. It's not ready for submission yet, I've got a lot of debug added that needs to be rebased and squished out, and I still want to do more testing with hardware flow control and the rest of the modem status lines. https://github.com/karlp/linux/commits/ch341-3.18.6 By implementing proper reading of settings from the device, a lot of the private copies can just be dropped out altogether. Sincerely, Karl Palsson
Re: usb : serial : ch341 : set tty baud speed according to tty struct
On 02/18/2015 02:05 PM, Karl Palsson wrote: Johan Hovold jo...@kernel.org wrote: On Wed, Feb 18, 2015 at 11:32:38AM +0700, Johan Hovold wrote: On Tue, Feb 17, 2015 at 10:45:11PM -0500, Nicolas PLANEL wrote: I believe the fix should be implemented slightly differently however. Most usb-serial driver call set_termios from open to handle this issue. It looks like you could simply replace the calls to set baudrate and handshake in open with ch341_set_termios(tty, port, NULL); You currently need to make sure tty is not NULL in case the device is being used as a console by the way. Just skip the set_termios call in that case. I've already got most of this working in my branch overhauling this driver. I agree, the open call shouldn't be doing all the hard resetting of settings that it currently does. I've implemented more of set termios / get termios. It's not ready for submission yet, I've got a lot of debug added that needs to be rebased and squished out, and I still want to do more testing with hardware flow control and the rest of the modem status lines. https://github.com/karlp/linux/commits/ch341-3.18.6 By implementing proper reading of settings from the device, a lot of the private copies can just be dropped out altogether. Sincerely, Karl Palsson I will try your branch and send you my comment. Regards, Nicolas -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: usb : serial : ch341 : set tty baud speed according to tty struct
On Wed, Feb 18, 2015 at 07:05:41PM -, Karl Palsson wrote: I've already got most of this working in my branch overhauling this driver. I agree, the open call shouldn't be doing all the hard resetting of settings that it currently does. I've implemented more of set termios / get termios. It's not ready for submission yet, I've got a lot of debug added that needs to be rebased and squished out, and I still want to do more testing with hardware flow control and the rest of the modem status lines. https://github.com/karlp/linux/commits/ch341-3.18.6 By implementing proper reading of settings from the device, a lot of the private copies can just be dropped out altogether. Yes, there shouldn't be a need to store the baud rate in the driver (the tty layer will keep track of that), but you probably still want to store the state of the modem-control signals. Looks like we could get rid of that ch341_configure at every open too? Looking forward to seeing your patches when you're done. Thanks, Johan -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usb : serial : ch341 : set tty baud speed according to tty struct
From 467794e88dc08f61e1068c510c97baa9a12b841d Mon Sep 17 00:00:00 2001
From: Nicolas PLANEL nicolas.pla...@enovance.com
Date: Tue, 17 Feb 2015 00:59:14 -0500
Subject: [PATCH] USB: ch341: set tty baud speed according to tty struct
The ch341_set_baudrate() function initialize the device baud speed according
to the value on priv-baud_rate. By default the ch341_open() set it to a
hardcoded value (DEFAULT_BAUD_RATE 9600). Unfortunately, the tty_struct is
not initialized with the same default value. (usually 56700)
This means that the tty_struct and the device baud rate generator are not
synchronized after opening the port.
Fixup is elementary simple by calling tty_get_baud_rate() and
tty_encode_baud_rate() helper to sync up the baud rate during the
ch341_open() call.
Signed-off-by: Nicolas PLANEL nicolas.pla...@enovance.com
---
drivers/usb/serial/ch341.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 2d72aa3564a3..2dbbbf6d4187 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -307,9 +307,13 @@ static int ch341_open(struct tty_struct *tty,
struct usb_serial_port *port)
{
struct usb_serial *serial = port-serial;
struct ch341_private *priv = usb_get_serial_port_data(port);
+unsigned baud;
int r;
priv-baud_rate = DEFAULT_BAUD_RATE;
+baud = tty_get_baud_rate(tty);
+if (baud)
+priv-baud_rate = baud;
r = ch341_configure(serial-dev, priv);
if (r)
@@ -323,6 +327,9 @@ static int ch341_open(struct tty_struct *tty, struct
usb_serial_port *port)
if (r)
goto out;
+baud = priv-baud_rate;
+tty_encode_baud_rate(tty, baud, baud);
+
dev_dbg(port-dev, %s - submitting interrupt urb\n, __func__);
r = usb_submit_urb(port-interrupt_in_urb, GFP_KERNEL);
if (r) {
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usb : serial : ch341 : set tty baud speed according to tty struct
On Tue, Feb 17, 2015 at 10:11:55AM -0500, Nicolas PLANEL wrote:
Hi,
According to git log your are currently the two maintainer of the ch341 usb
serial driver.
Thanks to pyserial, that did not initialize the serial port speed like
minicom.
Basically pyserial only set the port speed (calling TIOCSSERIAL ioctl) if
the user request a different value different currently used. (TIOCGSERIAL)
A long sentence in short code :
if ser.baudrate != user_requested_baudrate:
ser.baudrate = user_requested_baudrate
In short, if the python code open a serial port at a speed of 57600 baud,
the hw speed line will be in reality at 9600 baud. (driver default speed)
Regards,
Nicolas PLANEL
===
From 16df0d507a653c48f757635f717f2ba886f8c5ff Mon Sep 17 00:00:00 2001
From: Nicolas PLANEL nicolas.pla...@enovance.com
Date: Tue, 17 Feb 2015 00:59:14 -0500
Subject: [PATCH] USB: ch341: set tty baud speed according to tty struct
The ch341_set_baudrate() function initialize the device baud speed according
to the value on priv-baud_rate. By default the ch341_open() set it to a
hardcoded value (DEFAULT_BAUD_RATE 9600). Unfortunately, the tty_struct is
not initialized with the same default value. (usually 56700)
This means that the tty_struct and the device baud rate generator are not
synchronized after opening the port.
Fixup is elementary simple by calling tty_get_baud_rate() and
tty_encode_baud_rate() helper to sync up the baud rate during the
ch341_open() call.
---
drivers/usb/serial/ch341.c | 7 +++
1 file changed, 7 insertions(+)
Thanks for the patch, but in order to accept it we need a signed-off-by
line as the file Documentation/SubmittingPatches describes.
Also, just send the patch like this, without the text above, as we can't
hand-edit all emails for patches.
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 2d72aa3..2dbbbf6 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -307,9 +307,13 @@ static int ch341_open(struct tty_struct *tty, struct
usb_serial_port *port)
{
struct usb_serial *serial = port-serial;
struct ch341_private *priv = usb_get_serial_port_data(port);
+unsigned baud;
int r;
Also, your email client turned all tabs into spaces, making this patch
unable to be applied. It also line-wrapped the patch :(
Can you fix this up and resend?
thanks,
greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usb : serial : ch341 : set tty baud speed according to tty struct
On Wed, Feb 18, 2015 at 11:32:38AM +0700, Johan Hovold wrote: On Tue, Feb 17, 2015 at 10:45:11PM -0500, Nicolas PLANEL wrote: I believe the fix should be implemented slightly differently however. Most usb-serial driver call set_termios from open to handle this issue. It looks like you could simply replace the calls to set baudrate and handshake in open with ch341_set_termios(tty, port, NULL); You currently need to make sure tty is not NULL in case the device is being used as a console by the way. Just skip the set_termios call in that case. Johan -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usb : serial : ch341 : set tty baud speed according to tty struct
On Tue, Feb 17, 2015 at 10:45:11PM -0500, Nicolas PLANEL wrote: From 467794e88dc08f61e1068c510c97baa9a12b841d Mon Sep 17 00:00:00 2001 From: Nicolas PLANEL nicolas.pla...@enovance.com Date: Tue, 17 Feb 2015 00:59:14 -0500 Subject: [PATCH] USB: ch341: set tty baud speed according to tty struct Your patch is still whitespace damaged. Consider using git send-email for sending. Try sending it to yourself first and run scritps/checkpatch.pl on the received mail to make sure you got it right. The ch341_set_baudrate() function initialize the device baud speed according to the value on priv-baud_rate. By default the ch341_open() set it to a hardcoded value (DEFAULT_BAUD_RATE 9600). Unfortunately, the tty_struct is not initialized with the same default value. (usually 56700) This means that the tty_struct and the device baud rate generator are not synchronized after opening the port. Good catch. Fixup is elementary simple by calling tty_get_baud_rate() and tty_encode_baud_rate() helper to sync up the baud rate during the ch341_open() call. I believe the fix should be implemented slightly differently however. Most usb-serial driver call set_termios from open to handle this issue. It looks like you could simply replace the calls to set baudrate and handshake in open with ch341_set_termios(tty, port, NULL); Care to send an updated v2 of your patch? Remember to include the patch revision in the subject line when resending (i.e. [PATCH v2]). Thanks, Johan -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usb : serial : ch341 : set tty baud speed according to tty struct
On Tue, Feb 17, 2015 at 10:45:11PM -0500, Nicolas PLANEL wrote:
From 467794e88dc08f61e1068c510c97baa9a12b841d Mon Sep 17 00:00:00 2001
From: Nicolas PLANEL nicolas.pla...@enovance.com
Date: Tue, 17 Feb 2015 00:59:14 -0500
Subject: [PATCH] USB: ch341: set tty baud speed according to tty struct
What is all of this here for? And it doesn't match your From: line in
your email, which is a bit odd.
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 2d72aa3564a3..2dbbbf6d4187 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -307,9 +307,13 @@ static int ch341_open(struct tty_struct *tty, struct
usb_serial_port *port)
Linewrapped :(
{
struct usb_serial *serial = port-serial;
struct ch341_private *priv = usb_get_serial_port_data(port);
+unsigned baud;
int r;
tabs converted to spaces :(
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
