On Mon, Apr 14, 2014 at 07:54:17PM +0000, Karl Palsson wrote:
> Based on wireshark packet traces from a windows machine.
> 
> ch340 and ch341 both seem to support all parity modes, but only the ch341
> appears to support variable data bits and variable stop bits, so those are 
> left
> unimplemented, as before.
> 
> Tested on a generic usb-rs485 dongle with the chip label scratched off, and
> some Modbus/RTU devices that required various parity settings.
> 
> Signed-off-by: Karl Palsson <ka...@tweak.net.au>
> ---
>  drivers/usb/serial/ch341.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 82371f6..b870f6f 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -346,6 +346,7 @@ static void ch341_set_termios(struct tty_struct *tty,
>       struct ch341_private *priv = usb_get_serial_port_data(port);
>       unsigned baud_rate;
>       unsigned long flags;
> +     unsigned int par_flags;
>  
>       baud_rate = tty_get_baud_rate(tty);
>  
> @@ -366,9 +367,33 @@ static void ch341_set_termios(struct tty_struct *tty,
>  
>       /* Unimplemented:
>        * (cflag & CSIZE) : data bits [5, 8]
> -      * (cflag & PARENB) : parity {NONE, EVEN, ODD}
>        * (cflag & CSTOPB) : stop bits [1, 2]
>        */
> +     /* CH340 doesn't appear to support variable stop bits or data bits */
> +     if (C_PARENB(tty)) {
> +             if (C_PARODD(tty)) {
> +                     if (C_CMSPAR(tty)) {

Thanks for fixing the C_CMSPAR macro, but you didn't address my other
comments on v1.

> +                             dev_dbg(&port->dev, "parity = mark\n");
> +                             par_flags = 0xeb;
> +                     } else {
> +                             dev_dbg(&port->dev, "parity = odd\n");
> +                             par_flags = 0xcb;
> +                     }
> +             } else {
> +                     if (C_CMSPAR(tty)) {
> +                             dev_dbg(&port->dev, "parity = space\n");
> +                             par_flags = 0xfb;
> +                     } else {
> +                             dev_dbg(&port->dev, "parity = even\n");
> +                             par_flags = 0xdb;
> +                     }
> +             }
> +     } else {
> +             dev_dbg(&port->dev, "parity = none\n");
> +             par_flags = 0xc3;
> +     }
> +     ch341_control_out(port->serial->dev, 0x9a, 0x2518, par_flags);

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.

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.

> +

Again, you should remove this newline that your patch is adding.

>  }
>  
>  static void ch341_break_ctl(struct tty_struct *tty, int break_state)
> -- 
> 1.9.0

Thanks,
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

Reply via email to