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