Re: [PATCH] usb/serial/ch341: Add parity support
On Tue, Apr 15, 2014 at 11:26:52PM +, Karl P wrote: On 04/15/2014 05:34 PM, Johan Hovold wrote: On Tue, Apr 15, 2014 at 04:06:11PM +0200, Johan Hovold wrote: Specifically, I asked if you are able to make sense of the values of register 0x2518. The reason I ask is that your patch changes the value of that register from 0x50 (set in ch341_configure) to 0xc3 (when no parity is used) and I want to make sure that you're not inadvertently changing some other setting. You're right, I realised I'd forgotten some of your other comments earlier, but wasn't back to a computer to update, my apologies. No problem. Do you know what those other bits do? Do they encode the number of data and stop bits? From a quick glance it seems like 0xc0parity mode (2 bits) 0x08parity enable but your patch now also sets bits 0x83 and clears bit 0x10. That should have been: 0x30parity mode (2 bits) 0x08parity enable and your patch now always sets bits 0x83. No, I have no idea what these bits mean. We can go and look at FreeBSD's code and make assumptions about whether their decode is any more correct or not. (They break things into parts and declare 8bit registers, that must be set two at a time) As I mentioned, this was based on wireshark tracing of windows programs opening the serial port, and without any better documentation, it's all I've got to go on. Without better documentation, how do we even know that what _was_ being done was any more or less correct than what it will be doing now? All I can say is that 8-n-1 works as it did before, and 8-e-1, 8-o-1 and 8-m-1/8-s-1 now actually work at all. The register appears to be a standard 8-bit (8250, 16450, 16550) UART Line Control Register (LCR): * Bit 7: Divisor Latch Access Bit (DLAB). When set, access to the data * transmit/receive register (THR/RBR) and the Interrupt Enable Register * (IER) is disabled. Any access to these ports is now redirected to the * Divisor Latch Registers. Setting this bit, loading the Divisor * Registers, and clearing DLAB should be done with interrupts disabled. * Bit 6: Set Break. When set to 1, the transmitter begins to transmit * continuous Spacing until this bit is set to 0. This overrides any * bits of characters that are being transmitted. * Bit 5: Stick Parity. When parity is enabled, setting this bit causes parity * to always be 1 or 0, based on the value of Bit 4. * Bit 4: Even Parity Select (EPS). When parity is enabled and Bit 5 is 0, * setting this bit causes even parity to be transmitted and expected. * Otherwise, odd parity is used. * Bit 3: Parity Enable (PEN). When set to 1, a parity bit is inserted * between the last bit of the data and the Stop Bit. The UART will also * expect parity to be present in the received data. * Bit 2: Number of Stop Bits (STB). If set to 1 and using 5-bit data words, * 1.5 Stop Bits are transmitted and expected in each data word. For * 6, 7 and 8-bit data words, 2 Stop Bits are transmitted and expected. * When this bit is set to 0, one Stop Bit is used on each data word. * Bit 1: Word Length Select Bit #1 (WLSB1) * Bit 0: Word Length Select Bit #0 (WLSB0) * Together these bits specify the number of bits in each data word. * 1 0 Word Length * 0 0 5 Data Bits * 0 1 6 Data Bits * 1 0 7 Data Bits * 1 1 8 Data Bits This is an excerpt from drivers/usb/serial/mct_u232.h but the definition is standard. Maybe it's setting data bits to 8? (the ch340 doesn't support variable data bits, the ch341 does) Data bit support / stop bit support is still not supported by this driver anyway, I believe that's a project for another day. Yes, that guess seems correct. But this would mean that the driver is currently unusable with ch341 devices (unless you actually want 5 data bits)? And then your patch isn't just adding parity support. So, the only things that looks odd about your patch is that it sets bit 7 (which is possibly not even used) and that the driver has always been setting bit 6... Your other comment was about using the #define for 0x9a labelled write register I can if you would like, but that would make some of the code use the define others not. Unless you want me to redo the _rest_ of existing code to use this define. Further, while write register _seems_ like a believable interpretation of the magic number, unless someone has some better documentation, it's just an educated guess._Only_ the break support code, which came from FreeBSD even attempts to make up/assign names to some of these magic numbers. I'm happy to go and do this, (replacing magic numbers with the recently added #defines) but I had endeavoured to follow the style of the
Re: [PATCH] usb/serial/ch341: Add parity support
On Wed, Apr 16, 2014 at 10:17:29AM +0200, Johan Hovold wrote: This is an excerpt from drivers/usb/serial/mct_u232.h but the definition is standard. Maybe it's setting data bits to 8? (the ch340 doesn't support variable data bits, the ch341 does) Data bit support / stop bit support is still not supported by this driver anyway, I believe that's a project for another day. Yes, that guess seems correct. But this would mean that the driver is currently unusable with ch341 devices (unless you actually want 5 data bits)? And then your patch isn't just adding parity support. So, the only things that looks odd about your patch is that it sets bit 7 (which is possibly not even used) and that the driver has always been setting bit 6... Thanks for the interesting LCR info, I've never looked at those old devices anywhere closely enough, so it didn't have any meaning to me. (And hopefully it's not just coincidental for some pieces) Your other comment was about using the #define for 0x9a labelled write register I can if you would like, but that would make some of the code use the define others not. Unless you want me to redo the _rest_ of existing code to use this define. Further, while write register _seems_ like a believable interpretation of the magic number, unless someone has some better documentation, it's just an educated guess._Only_ the break support code, which came from FreeBSD even attempts to make up/assign names to some of these magic numbers. I'm happy to go and do this, (replacing magic numbers with the recently added #defines) but I had endeavoured to follow the style of the existing code. Fair enough. I don't mind keeping some of those magic constants for now, but I think we should at least assume that we're dealing with a fairly standard LCR register and use appropriate names and defines for that bit that you patch is changing. That is, something like: u8 lcr; and #define UART_LCR_DLAB 0x80 /* Divisor latch access bit (?) */ #define UART_LCR_SBC0x40 /* Set break control (?) */ #define UART_LCR_SPAR 0x20 /* Stick parity */ #define UART_LCR_EPAR 0x10 /* Even parity select */ #define UART_LCR_PARITY 0x08 /* Parity Enable */ #define UART_LCR_STOP 0x04 /* Stop bits: 0=1 bit, 1=2 bits */ #define UART_LCR_WLEN5 0x00 /* Wordlength: 5 bits */ #define UART_LCR_WLEN6 0x01 /* Wordlength: 6 bits */ #define UART_LCR_WLEN7 0x02 /* Wordlength: 7 bits */ #define UART_LCR_WLEN8 0x03 /* Wordlength: 8 bits */ Could you redo your patch using those defines? It's up to you if you want to implement stop and data bit support at once or do that as a follow-up patch (but please still use UART_LCR_WLEN8 if you choose to keep the data bits hard-coded). Yes, I can do that, but I don't have any good devices handy to test variable databits. I can maybe test out variable stop bit, I think I have some hardware for that, but parity is my primary (really, only) concern. Are those already defined somewhere else, or are you proposing (re) defining them again in this driver? Could you also see if everything appears to be working even if you leave DLAB and SBC unset? Yeah, I kinda took that as required testing :) Do you have access to both ch340 and ch341 devices in order to verify that both types keep working? This is also why I don't want to go too far with trying to test stop bits and data bits. I have a CH340, which doesn't support variable stop/data bits, and another device with the chip labels scratched off, that could be either. This is going to take a while longer to redo unfortunately. Sincerely, Karl Palsson -- 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: [PATCH] usb/serial/ch341: Add parity support
On Wed, Apr 16, 2014 at 10:42:41AM +, Karl Palsson wrote: On Wed, Apr 16, 2014 at 10:17:29AM +0200, Johan Hovold wrote: Fair enough. I don't mind keeping some of those magic constants for now, but I think we should at least assume that we're dealing with a fairly standard LCR register and use appropriate names and defines for that bit that you patch is changing. That is, something like: u8 lcr; and #define UART_LCR_DLAB 0x80 /* Divisor latch access bit (?) */ #define UART_LCR_SBC0x40 /* Set break control (?) */ #define UART_LCR_SPAR 0x20 /* Stick parity */ #define UART_LCR_EPAR 0x10 /* Even parity select */ #define UART_LCR_PARITY 0x08 /* Parity Enable */ #define UART_LCR_STOP 0x04 /* Stop bits: 0=1 bit, 1=2 bits */ #define UART_LCR_WLEN5 0x00 /* Wordlength: 5 bits */ #define UART_LCR_WLEN6 0x01 /* Wordlength: 6 bits */ #define UART_LCR_WLEN7 0x02 /* Wordlength: 7 bits */ #define UART_LCR_WLEN8 0x03 /* Wordlength: 8 bits */ Could you redo your patch using those defines? It's up to you if you want to implement stop and data bit support at once or do that as a follow-up patch (but please still use UART_LCR_WLEN8 if you choose to keep the data bits hard-coded). Yes, I can do that, but I don't have any good devices handy to test variable databits. I can maybe test out variable stop bit, I think I have some hardware for that, but parity is my primary (really, only) concern. Are those already defined somewhere else, or are you proposing (re) defining them again in this driver? Good question. They can be made available by #include linux/serial_reg.h But until we know (or are reasonably sure) what the bits are for, perhaps it is better to redefine them in the driver with a CH341_-prefix (instead of UART_) and comment on those that are left to be verified? Could you also see if everything appears to be working even if you leave DLAB and SBC unset? Yeah, I kinda took that as required testing :) Good. :) Do you have access to both ch340 and ch341 devices in order to verify that both types keep working? This is also why I don't want to go too far with trying to test stop bits and data bits. I have a CH340, which doesn't support variable stop/data bits, and another device with the chip labels scratched off, that could be either. Well, if you really want to make a minimal implementation of only parity, then only modify bits 0x38 and keep bit 0x40 (SBC) set use UART_LCR_WLEN5 as the driver used to (it set 0x50). If the device with scratched of label still works then it should also be a ch340? I am a little worried that the magic happening in ch341_break_ctl also messes with this very same register (CH341_NBREAK_BITS_REG2 is 0x40, that is, UART_LCR_SBC)... I guess such things could be verified by reading back LCR after setting and clearing break (as appears to be done in ch341_configure()). This is going to take a while longer to redo unfortunately. Yeah, reverse engineering can be a PITA, and especially as we also try to avoid regressions. 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: [PATCH] usb/serial/ch341: Add parity support
On Mon, Apr 14, 2014 at 07:54:17PM +, Karl Palsson wrote:
Based on wireshark packet traces from a windows machine.
ch340 and ch341 both seem to support all parity modes, but only the ch341
appears to support variable data bits and variable stop bits, so those are
left
unimplemented, as before.
Tested on a generic usb-rs485 dongle with the chip label scratched off, and
some Modbus/RTU devices that required various parity settings.
Signed-off-by: Karl Palsson ka...@tweak.net.au
---
drivers/usb/serial/ch341.c | 27 ++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 82371f6..b870f6f 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -346,6 +346,7 @@ 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 int par_flags;
baud_rate = tty_get_baud_rate(tty);
@@ -366,9 +367,33 @@ static void ch341_set_termios(struct tty_struct *tty,
/* Unimplemented:
* (cflag CSIZE) : data bits [5, 8]
- * (cflag PARENB) : parity {NONE, EVEN, ODD}
* (cflag CSTOPB) : stop bits [1, 2]
*/
+ /* CH340 doesn't appear to support variable stop bits or data bits */
+ if (C_PARENB(tty)) {
+ if (C_PARODD(tty)) {
+ if (C_CMSPAR(tty)) {
Thanks for fixing the C_CMSPAR macro, but you didn't address my other
comments on v1.
+ dev_dbg(port-dev, parity = mark\n);
+ par_flags = 0xeb;
+ } else {
+ dev_dbg(port-dev, parity = odd\n);
+ par_flags = 0xcb;
+ }
+ } else {
+ if (C_CMSPAR(tty)) {
+ dev_dbg(port-dev, parity = space\n);
+ par_flags = 0xfb;
+ } else {
+ dev_dbg(port-dev, parity = even\n);
+ par_flags = 0xdb;
+ }
+ }
+ } else {
+ dev_dbg(port-dev, parity = none\n);
+ par_flags = 0xc3;
+ }
+ ch341_control_out(port-serial-dev, 0x9a, 0x2518, par_flags);
Specifically, I asked if you are able to make sense of the values of
register 0x2518. The reason I ask is that your patch changes the value
of that register from 0x50 (set in ch341_configure) to 0xc3 (when no
parity is used) and I want to make sure that you're not inadvertently
changing some other setting.
Do you know what those other bits do? Do they encode the number of data
and stop bits?
From a quick glance it seems like
0xc0parity mode (2 bits)
0x08parity enable
but your patch now also sets bits 0x83 and clears bit 0x10.
+
Again, you should remove this newline that your patch is adding.
}
static void ch341_break_ctl(struct tty_struct *tty, int break_state)
--
1.9.0
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: [PATCH] usb/serial/ch341: Add parity support
On Tue, Apr 15, 2014 at 04:06:11PM +0200, Johan Hovold wrote:
On Mon, Apr 14, 2014 at 07:54:17PM +, Karl Palsson wrote:
+ if (C_PARENB(tty)) {
+ if (C_PARODD(tty)) {
+ if (C_CMSPAR(tty)) {
Thanks for fixing the C_CMSPAR macro, but you didn't address my other
comments on v1.
+ dev_dbg(port-dev, parity = mark\n);
+ par_flags = 0xeb;
+ } else {
+ dev_dbg(port-dev, parity = odd\n);
+ par_flags = 0xcb;
+ }
+ } else {
+ if (C_CMSPAR(tty)) {
+ dev_dbg(port-dev, parity = space\n);
+ par_flags = 0xfb;
+ } else {
+ dev_dbg(port-dev, parity = even\n);
+ par_flags = 0xdb;
+ }
+ }
+ } else {
+ dev_dbg(port-dev, parity = none\n);
+ par_flags = 0xc3;
+ }
+ ch341_control_out(port-serial-dev, 0x9a, 0x2518, par_flags);
Specifically, I asked if you are able to make sense of the values of
register 0x2518. The reason I ask is that your patch changes the value
of that register from 0x50 (set in ch341_configure) to 0xc3 (when no
parity is used) and I want to make sure that you're not inadvertently
changing some other setting.
Do you know what those other bits do? Do they encode the number of data
and stop bits?
From a quick glance it seems like
0xc0parity mode (2 bits)
0x08parity enable
but your patch now also sets bits 0x83 and clears bit 0x10.
That should have been:
0x30parity mode (2 bits)
0x08parity enable
and your patch now always sets bits 0x83.
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: [PATCH] usb/serial/ch341: Add parity support
On 04/15/2014 05:34 PM, Johan Hovold wrote: On Tue, Apr 15, 2014 at 04:06:11PM +0200, Johan Hovold wrote: Specifically, I asked if you are able to make sense of the values of register 0x2518. The reason I ask is that your patch changes the value of that register from 0x50 (set in ch341_configure) to 0xc3 (when no parity is used) and I want to make sure that you're not inadvertently changing some other setting. You're right, I realised I'd forgotten some of your other comments earlier, but wasn't back to a computer to update, my apologies. Do you know what those other bits do? Do they encode the number of data and stop bits? From a quick glance it seems like 0xc0parity mode (2 bits) 0x08parity enable but your patch now also sets bits 0x83 and clears bit 0x10. That should have been: 0x30parity mode (2 bits) 0x08parity enable and your patch now always sets bits 0x83. No, I have no idea what these bits mean. We can go and look at FreeBSD's code and make assumptions about whether their decode is any more correct or not. (They break things into parts and declare 8bit registers, that must be set two at a time) As I mentioned, this was based on wireshark tracing of windows programs opening the serial port, and without any better documentation, it's all I've got to go on. Without better documentation, how do we even know that what _was_ being done was any more or less correct than what it will be doing now? All I can say is that 8-n-1 works as it did before, and 8-e-1, 8-o-1 and 8-m-1/8-s-1 now actually work at all. Maybe it's setting data bits to 8? (the ch340 doesn't support variable data bits, the ch341 does) Data bit support / stop bit support is still not supported by this driver anyway, I believe that's a project for another day. Your other comment was about using the #define for 0x9a labelled write register I can if you would like, but that would make some of the code use the define others not. Unless you want me to redo the _rest_ of existing code to use this define. Further, while write register _seems_ like a believable interpretation of the magic number, unless someone has some better documentation, it's just an educated guess. _Only_ the break support code, which came from FreeBSD even attempts to make up/assign names to some of these magic numbers. I'm happy to go and do this, (replacing magic numbers with the recently added #defines) but I had endeavoured to follow the style of the existing code. Sincerely, Karl Palsson -- 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
[PATCH] usb/serial/ch341: Add parity support
Based on wireshark packet traces from a windows machine.
ch340 and ch341 both seem to support all parity modes, but only the ch341
appears to support variable data bits and variable stop bits, so those are left
unimplemented, as before.
Tested on a generic usb-rs485 dongle with the chip label scratched off, and
some Modbus/RTU devices that required various parity settings.
Signed-off-by: Karl Palsson ka...@tweak.net.au
---
drivers/usb/serial/ch341.c | 27 ++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 82371f6..b870f6f 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -346,6 +346,7 @@ 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 int par_flags;
baud_rate = tty_get_baud_rate(tty);
@@ -366,9 +367,33 @@ static void ch341_set_termios(struct tty_struct *tty,
/* Unimplemented:
* (cflag CSIZE) : data bits [5, 8]
-* (cflag PARENB) : parity {NONE, EVEN, ODD}
* (cflag CSTOPB) : stop bits [1, 2]
*/
+ /* CH340 doesn't appear to support variable stop bits or data bits */
+ if (C_PARENB(tty)) {
+ if (C_PARODD(tty)) {
+ if (C_CMSPAR(tty)) {
+ dev_dbg(port-dev, parity = mark\n);
+ par_flags = 0xeb;
+ } else {
+ dev_dbg(port-dev, parity = odd\n);
+ par_flags = 0xcb;
+ }
+ } else {
+ if (C_CMSPAR(tty)) {
+ dev_dbg(port-dev, parity = space\n);
+ par_flags = 0xfb;
+ } else {
+ dev_dbg(port-dev, parity = even\n);
+ par_flags = 0xdb;
+ }
+ }
+ } else {
+ dev_dbg(port-dev, parity = none\n);
+ par_flags = 0xc3;
+ }
+ ch341_control_out(port-serial-dev, 0x9a, 0x2518, par_flags);
+
}
static void ch341_break_ctl(struct tty_struct *tty, int break_state)
--
1.9.0
--
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: [PATCH] usb/serial/ch341: Add parity support
Apply this patch because I have a device that uses even parity. It is
a tax machine, it worked perfect in that case. I want to thank you
again, though I did in my thread, I write only to support you and
implement this PATCH soon.
Based on wireshark packet traces from a windows machine.
ch340 and ch341 both seem to support all parity modes, but only the ch341
appears to support variable data bits and variable stop bits, so those are
left
unimplemented, as before.
Tested on a generic usb-rs485 dongle with the chip label scratched off, and
some Modbus/RTU devices that required various parity settings.
Signed-off-by: Karl Palsson ka...@tweak.net.au
---
drivers/usb/serial/ch341.c | 27 ++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 82371f6..b870f6f 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -346,6 +346,7 @@ 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 int par_flags;
baud_rate = tty_get_baud_rate(tty);
@@ -366,9 +367,33 @@ static void ch341_set_termios(struct tty_struct *tty,
/* Unimplemented:
* (cflag CSIZE) : data bits [5, 8]
-* (cflag PARENB) : parity {NONE, EVEN, ODD}
* (cflag CSTOPB) : stop bits [1, 2]
*/
+ /* CH340 doesn't appear to support variable stop bits or data bits */
+ if (C_PARENB(tty)) {
+ if (C_PARODD(tty)) {
+ if (C_CMSPAR(tty)) {
+ dev_dbg(port-dev, parity = mark\n);
+ par_flags = 0xeb;
+ } else {
+ dev_dbg(port-dev, parity = odd\n);
+ par_flags = 0xcb;
+ }
+ } else {
+ if (C_CMSPAR(tty)) {
+ dev_dbg(port-dev, parity = space\n);
+ par_flags = 0xfb;
+ } else {
+ dev_dbg(port-dev, parity = even\n);
+ par_flags = 0xdb;
+ }
+ }
+ } else {
+ dev_dbg(port-dev, parity = none\n);
+ par_flags = 0xc3;
+ }
+ ch341_control_out(port-serial-dev, 0x9a, 0x2518, par_flags);
+
}
static void ch341_break_ctl(struct tty_struct *tty, int break_state)
--
1.9.0
--
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: [PATCH] usb/serial/ch341: Add parity support
On Mon, Mar 31, 2014 at 11:38:43PM +, Karl Palsson wrote:
Based on wireshark packet traces from a windows machine.
ch340 and ch341 both seem to support all parity modes, but only the ch341
appears to support variable data bits and variable stop bits, so those are
left
unimplemented, as before.
Tested on a generic usb-rs485 dongle with the chip label scratched off, and
some Modbus/RTU devices that required various parity settings.
Signed-off-by: Karl Palsson ka...@tweak.net.au
---
drivers/usb/serial/ch341.c | 27 ++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 82371f6..afd2ec4 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -346,6 +346,7 @@ 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 int par_flags;
baud_rate = tty_get_baud_rate(tty);
@@ -366,9 +367,33 @@ static void ch341_set_termios(struct tty_struct *tty,
/* Unimplemented:
* (cflag CSIZE) : data bits [5, 8]
- * (cflag PARENB) : parity {NONE, EVEN, ODD}
* (cflag CSTOPB) : stop bits [1, 2]
*/
+ /* CH340 doesn't appear to support variable stop bits or data bits */
+ if (C_PARENB(tty)) {
+ if (C_PARODD(tty)) {
+ if (tty-termios.c_cflag CMSPAR) {
The kernel recently gained a C_CMSPAR(tty) macro. Could you use that for
consistency?
+ dev_dbg(port-dev, parity = mark\n);
+ par_flags = 0xeb;
+ } else {
+ dev_dbg(port-dev, parity = odd\n);
+ par_flags = 0xcb;
+ }
+ } else {
+ if (tty-termios.c_cflag CMSPAR) {
+ dev_dbg(port-dev, parity = space\n);
+ par_flags = 0xfb;
+ } else {
+ dev_dbg(port-dev, parity = even\n);
+ par_flags = 0xdb;
+ }
+ }
+ } else {
+ dev_dbg(port-dev, parity = none\n);
+ par_flags = 0xc3;
+ }
+ ch341_control_out(port-serial-dev, 0x9a, 0x2518, par_flags);
I noticed that 0x2518 is the same register that is being read from and
written to in ch341_configure(). Does the value written there (0x50)
make any sense to you?
Also, a patch adding break control introduced
#define CH341_REQ_WRITE_REG0x9A
#define CH341_REQ_READ_REG 0x95
Perhaps we should start using those?
+
There's a stray new line here.
}
static void ch341_break_ctl(struct tty_struct *tty, int break_state)
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
Patch: usb/serial/ch341: Add parity support
I originally sent this to fr...@kingswood-consulting.co.uk who is listed as the maintainer for this driver, but I haven't heard a reply, so I'm posting to the list. This is my first patch for a driver, so I've tried to follow the existing style, but if there are any changes that should be made, please let me know. (There is almost no debugging in this driver, so possibly I should remove the debug I added?) This was developed against 3.13.6 and 3.13.7, but has been rebased against 3.14. -- 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
[PATCH] usb/serial/ch341: Add parity support
Based on wireshark packet traces from a windows machine.
ch340 and ch341 both seem to support all parity modes, but only the ch341
appears to support variable data bits and variable stop bits, so those are left
unimplemented, as before.
Tested on a generic usb-rs485 dongle with the chip label scratched off, and
some Modbus/RTU devices that required various parity settings.
Signed-off-by: Karl Palsson ka...@tweak.net.au
---
drivers/usb/serial/ch341.c | 27 ++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 82371f6..afd2ec4 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -346,6 +346,7 @@ 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 int par_flags;
baud_rate = tty_get_baud_rate(tty);
@@ -366,9 +367,33 @@ static void ch341_set_termios(struct tty_struct *tty,
/* Unimplemented:
* (cflag CSIZE) : data bits [5, 8]
-* (cflag PARENB) : parity {NONE, EVEN, ODD}
* (cflag CSTOPB) : stop bits [1, 2]
*/
+ /* CH340 doesn't appear to support variable stop bits or data bits */
+ if (C_PARENB(tty)) {
+ if (C_PARODD(tty)) {
+ if (tty-termios.c_cflag CMSPAR) {
+ dev_dbg(port-dev, parity = mark\n);
+ par_flags = 0xeb;
+ } else {
+ dev_dbg(port-dev, parity = odd\n);
+ par_flags = 0xcb;
+ }
+ } else {
+ if (tty-termios.c_cflag CMSPAR) {
+ dev_dbg(port-dev, parity = space\n);
+ par_flags = 0xfb;
+ } else {
+ dev_dbg(port-dev, parity = even\n);
+ par_flags = 0xdb;
+ }
+ }
+ } else {
+ dev_dbg(port-dev, parity = none\n);
+ par_flags = 0xc3;
+ }
+ ch341_control_out(port-serial-dev, 0x9a, 0x2518, par_flags);
+
}
static void ch341_break_ctl(struct tty_struct *tty, int break_state)
--
1.8.3.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: Patch: usb/serial/ch341: Add parity support
On Mon, Mar 31, 2014 at 11:38:42PM +, Karl Palsson wrote: I originally sent this to fr...@kingswood-consulting.co.uk who is listed as the maintainer for this driver, but I haven't heard a reply, so I'm posting to the list. That's good, in the future, you can use the kernel script, scripts/get_maintainer.pl to determine who to send a patch to, including the mailing list. This is my first patch for a driver, so I've tried to follow the existing style, but if there are any changes that should be made, please let me know. (There is almost no debugging in this driver, so possibly I should remove the debug I added?) This was developed against 3.13.6 and 3.13.7, but has been rebased against 3.14. I'll look at it after 3.15-rc1 is out, thanks for the patch. 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
