On Wed, Apr 16, 2014 at 10:42:41AM +0000, 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_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? 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
