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_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). 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
