On Tue, Apr 15, 2014 at 11:26:52PM +0000, 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 > >> > >> 0xc0 parity mode (2 bits) > >> 0x08 parity enable > >> > >> but your patch now also sets bits 0x83 and clears bit 0x10. > > > > That should have been: > > > > 0x30 parity mode (2 bits) > > 0x08 parity 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 > 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_SBC 0x40 /* 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). Could you also see if everything appears to be working even if you leave DLAB and SBC unset? Do you have access to both ch340 and ch341 devices in order to verify that both types keep working? Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
