Re: [PATCH] usb/serial/ch341: Add parity support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux