Re: ch341

[Karl Palsson]

Can you actually trust that pl2303 module? They're not exactly
known for being trouble free, and have often been cloned in the
past as well, with varyings levels of success. Do you have a
cp210x or ft23x at all?

This is one of the great pitfalls of loopback testing, it doesn't
test that anything really does what it says, just that it does
whatever it does consistently.

Sincerely,
Karl P


Russell Senior  wrote:
> > Also, when the above change makes usb-linus works, does changing the
> > word size appear to work after probe?
> 
> I'm not sure what changing the word size is supposed to do. The
> usb-linus + previous patch in loopback seems to work with
> minicom regardless of wordsize. More so even than I would guess
> at 5N1: a -> a, A -> A. Trying to talk with a pl2303, 8N1 on
> both sides works. 7E1 works sending from ch341 to pl2303, but
> not pl2303 to ch341 (text is garbled on reception). 6N1 is
> garbled in both directions, but differently.
> 
> 
> Russell

signature.asc
Description: OpenPGP Digital Signature

Re: [PATCH 2/4] USB: ch341: reinitialize chip on reconfiguration

[Karl Palsson]

Johan Hovold <jo...@kernel.org> wrote:
> On Sun, Oct 16, 2016 at 05:09:24PM +0100, Aidan Thornton wrote:
> > On Mon, Oct 10, 2016 at 8:56 PM, Grigori Goronzy <g...@chown.ath.cx> wrote:
> > > On 2016-10-08 23:07, Aidan Thornton wrote:
> > >>
> > >> On Fri, Oct 7, 2016 at 12:30 PM, Johan Hovold <jo...@kernel.org> wrote:
> > >>>
> > >>> Why is this change needed? I see no write to this register in the
> > >>> vendor driver.
> > >>
> > >>
> > >> In principle, it might not be because the value written to register
> > >> 0x18 is probably overwritten by ch341_init_set_baudrate anyway. It's
> > >> there because it's in Grigori's original patch, it looks superficially
> > >> reasonable (corresponds to ENABLE_RX|ENABLE_TX|CS8, as opposed to the
> > >> rather implausible ENABLE_TX|PAR_EVEN|CS5), and given that some
> > >> hardware revisions are apparently a little temperamental I was
> > >> reluctant to remove it without fully understanding why it was added in
> > >> the first place. As the vendor driver does without it might make sense
> > >> to delete the write altogether, but it does do quite a few things
> > >> differently from this driver. Depends what Grigori says and the
> > >> results of actual testing.
> 
> Ok, thanks for the explanation. I think we should try to get
> rid of anything apparently redundant eventually, moving towards
> how the vendor driver does things. It would also be good to
> replace more of the magic constants in there to make the code
> more self-describing.
> 
> As long this is done in steps that can be (more easily)
> rolled-back in case it turns out we do actually break support
> for some device in the process.

Perhaps it needs to be repeated more, but the current driver is
so badly broken for so many devices that people simply don't use
them. This insistence that any patch trying to fix this driver
somehow preserves all the existing undocumented, unexplained and
_non-functioning_ bizarre lines of code is exhausting.
There's been probably ~6 people now submit patches to this
driver, all of which make marked useful improvements to a driver
that _is_broken_ and they're all in limbo because "compatibility
with unknown magic number XYZ" that cant' be explained anyway.

Is there any chance of any improvements to this driver ever
landing? The requirements have been so high, when the existing
was s poor.

Sincerely,
Karl Palsson

signature.asc
Description: OpenPGP Digital Signature

Re: [PATCH v3 06/13] USB: ch341: add support for parity, frame length, stop bits

[Karl Palsson]

Sorry if you get this twice, I was having some client problems,
but wanted to make sure you got this one...


Grigori Goronzy  wrote:
> With the new reinitialization method, configuring parity,
> different frame lengths and different stop bit settings work as
> expected on both CH340G and CH341A. This has been extensively
> tested with a logic analyzer.
> 
> v2: only set mark/space when parity is enabled,
> simplifications, patch termios HW flags.
> 
> Tested-by: Ryan Barber 
> Signed-off-by: Grigori Goronzy 
> ---
>  drivers/usb/serial/ch341.c | 40 ++--
>  1 file changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c
> b/drivers/usb/serial/ch341.c index 6181616..99b4621 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -341,7 +341,6 @@ static void ch341_set_termios(struct tty_struct *tty,
>   struct usb_serial_port *port, struct ktermios *old_termios)
>  {
>   struct ch341_private *priv = usb_get_serial_port_data(port);
> - unsigned baud_rate;
>   unsigned long flags;
>   unsigned char ctrl;
>   int r;
> @@ -350,13 +349,39 @@ static void ch341_set_termios(struct tty_struct *tty,
>   if (old_termios && !tty_termios_hw_change(>termios, old_termios))
>   return;
>  
> - baud_rate = tty_get_baud_rate(tty);
> + priv->baud_rate = tty_get_baud_rate(tty);
>  
> - priv->baud_rate = baud_rate;
> + ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX;
>  
> - ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX | CH341_LCR_CS8;
> + switch (C_CSIZE(tty)) {
> + case CS5:
> + ctrl |= CH341_LCR_CS5;
> + break;
> + case CS6:
> + ctrl |= CH341_LCR_CS6;
> + break;
> + case CS7:
> + ctrl |= CH341_LCR_CS7;
> + break;
> + default:
> + tty->termios.c_cflag |= CS8;
> + case CS8:
> + ctrl |= CH341_LCR_CS8;
> + break;
> + }
> +
> + if (C_PARENB(tty)) {
> + ctrl |= CH341_LCR_ENABLE_PAR;
> + if (C_PARODD(tty))
> + ctrl |= CH341_LCR_PAR_EVEN;

Are you sure this does the right thing now? this is, as best as I
can tell, the inverse of what you had earlier, and doesn't read
right, if this is working, then I suggest renaming _LCR_PAR_EVEN
to LCR_PAR_ODD?

Cheers,
Karl P


> + if (C_CMSPAR(tty))
> + ctrl |= CH341_LCR_MARK_SPACE;
> + }
> +
> + if (C_CSTOPB(tty))
> + ctrl |= CH341_LCR_STOP_BITS_2;
>  
> - if (baud_rate) {
> + if (priv->baud_rate) {
>   spin_lock_irqsave(>lock, flags);
>   priv->line_control |= (CH341_BIT_DTR | CH341_BIT_RTS);
>   spin_unlock_irqrestore(>lock, flags);
> @@ -373,11 +398,6 @@ static void ch341_set_termios(struct tty_struct *tty,
>  
>   ch341_set_handshake(port->serial->dev, priv->line_control);
>  
> - /* Unimplemented:
> -  * (cflag & CSIZE) : data bits [5, 8]
> -  * (cflag & PARENB) : parity {NONE, EVEN, ODD}
> -  * (cflag & CSTOPB) : stop bits [1, 2]
> -  */
>  }
>  
>  static void ch341_break_ctl(struct tty_struct *tty, int break_state)
> -- 
> 1.9.1
> 
> --
> 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

signature.asc
Description: OpenPGP Digital Signature

Re: [PATCH v2 14/14] USB: ch341: implement tx_empty callback

[Karl Palsson]

Grigori Goronzy  wrote:
> The status bit was found with USB captures of the Windows
> driver and some luck. Tested on CH340G and CH341A.

By my reversing, bit 0x4, is "multiple status changes since last
interrupt"

> 
> Signed-off-by: Grigori Goronzy 
> ---
>  drivers/usb/serial/ch341.c | 19 ++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/ch341.c
> b/drivers/usb/serial/ch341.c index 6981e2ad..adf7d79 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -82,6 +82,8 @@
>  #define CH341_LCR_CS6  0x01
>  #define CH341_LCR_CS5  0x00
>  
> +#define CH341_STATUS_TXBUSY0x01
> +
>  static const struct usb_device_id id_table[] = {
>   { USB_DEVICE(0x4348, 0x5523) },
>   { USB_DEVICE(0x1a86, 0x7523) },
> @@ -95,6 +97,7 @@ struct ch341_private {
>   unsigned baud_rate; /* set baud rate */
>   u8 line_control; /* set line control value RTS/DTR */
>   u8 line_status; /* active status of modem control inputs */
> + u8 uart_status; /* generic UART status bits */
>  };
>  
>  static void ch341_set_termios(struct tty_struct *tty,
> @@ -187,7 +190,8 @@ static int ch341_get_status(struct usb_device *dev, 
> struct ch341_private *priv)
>   if (r == 2) {
>   r = 0;
>   spin_lock_irqsave(>lock, flags);
> - priv->line_status = (~(*buffer)) & CH341_BITS_MODEM_STAT;
> + priv->line_status = (~buffer[0]) & CH341_BITS_MODEM_STAT;
> + priv->uart_status = buffer[1];
>   spin_unlock_irqrestore(>lock, flags);
>   } else {
>   r = -EPROTO;
> @@ -198,6 +202,18 @@ out:
>   return r;
>  }
>  
> +static bool ch341_tx_empty(struct usb_serial_port *port)
> +{
> + int r;
> + struct ch341_private *priv = usb_get_serial_port_data(port);
> +
> + r = ch341_get_status(port->serial->dev, priv);
> + if (r < 0)
> + return true;
> +
> + return !(priv->uart_status & CH341_STATUS_TXBUSY);
> +}
> +
>  /* 
> -- */
>  
>  static int ch341_configure(struct usb_device *dev, struct ch341_private 
> *priv)
> @@ -606,6 +622,7 @@ static struct usb_serial_driver ch341_device = {
>   .carrier_raised= ch341_carrier_raised,
>   .close = ch341_close,
>   .set_termios   = ch341_set_termios,
> + .tx_empty  = ch341_tx_empty,
>   .break_ctl = ch341_break_ctl,
>   .tiocmget  = ch341_tiocmget,
>   .tiocmset  = ch341_tiocmset,
> -- 
> 1.9.1
> 
> --
> 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

Re: [PATCH v2 07/14] USB: ch341: add support for parity, frame length, stop bits

[Karl Palsson]

Grigori Goronzy  wrote:
> With the new reinitialization method, configuring parity,
> different frame lengths and different stop bit settings work as
> expected on both CH340G and CH341A. This has been extensively
> tested with a logic analyzer.
> 
> Tested-by: Ryan Barber 
> Signed-off-by: Grigori Goronzy 
> ---
>  drivers/usb/serial/ch341.c | 36 ++--
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c
> b/drivers/usb/serial/ch341.c index c001773..4d66f0f 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,
>   unsigned baud_rate;
>   unsigned long flags;
>   unsigned char ctrl;
> + unsigned cflag = tty->termios.c_cflag;
>   int r;
>  
>   /* redundant changes may cause the chip to lose bytes */
> @@ -356,7 +357,35 @@ static void ch341_set_termios(struct tty_struct *tty,
>  
>   priv->baud_rate = baud_rate;
>  
> - ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX | CH341_LCR_CS8;
> + ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX;
> +
> + switch (cflag & CSIZE) {
> + case CS5:
> + ctrl |= CH341_LCR_CS5;
> + break;
> + case CS6:
> + ctrl |= CH341_LCR_CS6;
> + break;
> + case CS7:
> + ctrl |= CH341_LCR_CS7;
> + break;
> + case CS8:
> + default:
> + ctrl |= CH341_LCR_CS8;
> + break;
> + }
> +
> + if (cflag & PARENB) {
> + ctrl |= CH341_LCR_ENABLE_PAR;
> + if ((cflag & PARODD) == 0)
> + ctrl |= CH341_LCR_PAR_EVEN;
> + }
> +
> + if (cflag & CMSPAR)
> + ctrl |= CH341_LCR_MARK_SPACE;
> +
> + if (cflag & CSTOPB)
> + ctrl |= CH341_LCR_STOP_BITS_2;
>  

I think this should be moved up to the PARENB check, at least,
when I was working on this. Also there's macros for some of the
flag checks: (From some code I was working on, but you can see
the mark/space is differently handled, this matched the windows
driver I was reversing usb captures from.)

if (C_PARENB(tty)) {
*lcr |= CH341_LCR_PARITY;
if (C_CMSPAR(tty)) {
*lcr |= CH341_LCR_SPAR;
if (C_PARODD(tty)) {
dev_dbg(>dev, "parity = mark\n");
*lcr &= ~CH341_LCR_EPAR;
} else {
dev_dbg(>dev, "parity = space\n");
*lcr |= CH341_LCR_EPAR;
}
} else {
*lcr &= ~CH341_LCR_SPAR;
if (C_PARODD(tty)) {
dev_dbg(>dev, "parity = odd\n");
*lcr &= ~CH341_LCR_EPAR;
} else {
dev_dbg(>dev, "parity = even\n");
*lcr |= CH341_LCR_EPAR;
}
}
} else {
*lcr &= ~(CH341_LCR_PARITY | CH341_LCR_SPAR | CH341_LCR_EPAR);
dev_dbg(>dev, "parity = none\n");
}



>   if (baud_rate) {
>   spin_lock_irqsave(>lock, flags);
> @@ -373,11 +402,6 @@ static void ch341_set_termios(struct tty_struct *tty,
>  
>   ch341_set_handshake(port->serial->dev, priv->line_control);
>  
> - /* Unimplemented:
> -  * (cflag & CSIZE) : data bits [5, 8]
> -  * (cflag & PARENB) : parity {NONE, EVEN, ODD}
> -  * (cflag & CSTOPB) : stop bits [1, 2]
> -  */
>  }
>  
>  static void ch341_break_ctl(struct tty_struct *tty, int break_state)
> -- 
> 1.9.1
> 
> --
> 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

Re: [PATCH v7 1/5] leds: core: add generic support for RGB LED's

[Karl Palsson]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

> > I admit I hadn't seen this earlier, and I didn't spend all day
> > looking at previous attempts, but what's the motivation for
> > putting all this overloaded syntax into the "brightness" file? I
> > thought the point was to have a single value in each file, one of
> > the references I did find was
> > http://www.spinics.net/lists/linux-leds/msg02995.html Is there
> > some thread where this was decided as advantageous? Surely 0-255
> > for _brightness_ is what the brightness entry should do?
> > 
> The referenced mail thread refers to a proposed sysfs attribute
> holding a list of space-separated entries. Here it's still
> about one numeric value. Advantage of the approach used now is
> the full backwards compatibilty. You can still set brightness
> to 0..255 and only the brightness will change (as expected). Or
> in other words: So far only V was supported, now we support HSV
> as a superset.
> 
> > I'd like to set the rgb colour of a led (or hsv, that can be it's
> > own file too) and separately ramp the brightness up and down. I
> > also think it's substantially simpler and easier to use from the
> > user's point of view, which is surely the place you want easy
> > right?
> > 
> What you describe is perfectly possible with the new approach.
> 

Only by using magically different formats for what I write to the
file labelled "brightness" I'm really not a fan of a knob called
"brightness" that does _other things_ if you write something
different to it.

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBAgAGBQJW3LXbAAoJEBmotQ/U1cr2fdUQAKONfj98SS2b4zcHheXG4+ts
PfiyS3nhh3VxlNykJSF13ou6ZBgYs4vuTVFuXO2Vya07TFWGrRM4ZNM0d+3JU7BY
QkDq8nhqYkxBmIvuz3dgmr+HrMShT/ECQoOYSoIq9bAHpXj0v+eCvYnlLseVLBGx
inwgyBme0jSiXvITRDBc0YM0wvpnh492UlruOHGsGkoTSaPaBXU8E+Uv4sv+y448
muYI9M3l+4Lg+B5k9UyXEvwCi2pyLJ2pSGbXyk6flSWYxI92Mny0decOQ+myJqEr
sfIqOczGbXFXiWXo8oX2i/Dm9+b+ChrMEXAtcfMcuB+9+469p/2eoqcCCtMWnAUJ
qmM8h37mNoohwDIv9c4blG2Y2854n6L2R7ZCXGMZj1Thidmn7ANNhnIFp+ojyXsz
O/JSongYWxX4b9pMQ8QhZFRCXfi/V7c/0RDREN5IcjB5nm+1W4Wr/u0jqDGsD7hW
kYgWHLbRzBtjOk7ruyDRBNlUyV+xCwxmYgmHAo3Ko3ZPs0MAPQoD+u6mtG5BPDkY
ek8ek7+Zw1V8MQSKp1LEfVr6GX5rTkaTD13odbC8PcMYACAC6NKFDqS1NNvSclKv
nUyccdaxw0NU4WZtG1dalvJGMwMj6z5MT9BjlE9JvSs4vROYx7RGOQvGvxw9syJT
j5AL5VA86J8n30tDXFuy
=hUju
-END PGP SIGNATURE-

Re: [PATCH v7 1/5] leds: core: add generic support for RGB LED's

[Karl Palsson]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Heiner Kallweit <hkallwe...@gmail.com> wrote:
> Add generic support for RGB LED's.
> 
> Basic idea is to use enum led_brightness also for the hue and
> saturation color components.This allows to implement the color
> extension w/o changes to struct led_classdev.
> 
> Select LEDS_RGB to enable building drivers using the RGB
> extension.
> 
> Flag LED_SET_HUE_SAT allows to specify that hue / saturation
> should be overridden even if the provided values are zero.
> 
> Some examples for writing values to
> /sys/class/leds//brightness: (now also hex notation can be
> used)
> 
> 255 -> set full brightness and keep existing color if set 0 ->
> switch LED off but keep existing color so that it can be
> restored
>  if the LED is switched on again later
> 0x100 -> switch LED off and set also hue and saturation to
> 0 0x00 -> set full brightness, full saturation and set hue
> to 0 (red)
> 


I admit I hadn't seen this earlier, and I didn't spend all day
looking at previous attempts, but what's the motivation for
putting all this overloaded syntax into the "brightness" file? I
thought the point was to have a single value in each file, one of
the references I did find was
http://www.spinics.net/lists/linux-leds/msg02995.html Is there
some thread where this was decided as advantageous? Surely 0-255
for _brightness_ is what the brightness entry should do?

I'd like to set the rgb colour of a led (or hsv, that can be it's
own file too) and separately ramp the brightness up and down. I
also think it's substantially simpler and easier to use from the
user's point of view, which is surely the place you want easy
right?

Sincerely,
Karl Palsson

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBAgAGBQJW3Jk5AAoJEBmotQ/U1cr2fxYP/AsuQ/x8ky86S9xf9Y8UdRrk
IC7eouBpf07RsDTv2KobRwEH69tk12zxGKmOpNZ5SY8ozVT/VDXMA8Iw/cwKL2t4
EBWTdBhORrOlfxw0sykp4SXYSYBm9n2Z+xZGK9b/fN+2g+XCv4B+W2iDejyvAsIt
c/eH6dGR0PvYdovEh0Tq7qAflpXRAhU0ykRR0Ydq/HrF8Xfxi+MDHC99zTRrHIsV
rPTbPh26cxZ3zyOoUxwgPLNmm4O1BvMsghxuQXV49A95gOlRet+ewDQxBgwWabEp
AUh3fuOl53R/ODJSqjX/JjlO4ynXWgv/9kdCF8QwPUAl13gyhilPvIdI5O3gm3Nr
beiW/rUnvHej3ZxbRUe/Q8ZlQ099WTVH4cEgSxLclC5hiWm4dCjsjskJA1acbnZV
4w5WSqrAqSyNP81Rhy7WV6k8kazDUrASSAl4JFnNJVRC4WNdHQJA4pKkH08mtYyo
5ls3ydMzU2eiTNKCFEze4/cH3MgUWM+L29rLRzev6rT7s32rPzR0JKaKv460pocd
rjpKanbt+zgUVySprVzX4t4GsmDZtKjQkTGooz9BabZP5+WeVvDtEMK3kciZ1d/x
ubtvcMXGbDpZ0FMcQkTQj44Sq3wMdr3P0CoMiDspDGk7XY67gSXsmUgSSh0JTLRL
X4K67h/OUpH0A00XGZCO
=86mG
-END PGP SIGNATURE-

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Karl Palsson]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Douglas Anderson  wrote:
>  
> + /*
> +  * Sleep for 10-15 ms after the reset to let it finish.
> +  *
> +  * It's been confirmed on at least one version of the controller
> +  * that this is a requirement that this is a requirement in order for

^^ duplicate wording here.


Cheers,
Karl P

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBAgAGBQJW2h5AAAoJEBmotQ/U1cr2izQQAJ26Rnz1arSPnccUXyOilD9g
vkf6wEEZNHsQUsnrzLbY9+x7rUAWgK6Wq+LYFv7vsOL63ogKpf6O52bZVE0/KQkT
IABKZB312AecotpbSdsZBXaK1SYFUXSRd0CDHqnL9o7SBE2sNRVVd+e/h2z45hUg
H2KkHZbzJA5btZveR+kkYX8PzV3QTBAmgqZ4YjI3uFllQtcRyJflYg9lJNm/GzpG
+ckG/JDlfSfv8Y4C7CrvCot0iTktLXFgzYO8ftI2z8ZAbV2IP3kOf2sc+8b+TX34
yI3odnpf+N0lNQhJESQaYAbOLF45SLGbFK5cqi1zi0AuHJA2eTISOOZWo5Zl/nhE
vneDG6zkS2q0YQRJlBIq23KxTdT9WJjW6qNs+OhVFo5k1900GWtuibfKr43g7JeF
l2d5uVeL9trwDUNmMvyGelSRXL12DhJ/k3IX1TgVMPsfACbGFWS74nzWfdHYjTUS
48ou5a9QED632Na1ZsxhSa1Ce4IOn7Uhaa13WIjKqo8IZM5TXEWwTAczF/9lLpBM
kz4Gb1tII5lQt0KpTOMHs/rXs0/k9iq0x0zuSUVNQEYJrAPhcJ/r+SBRJMqQb5Zy
jzsMzWiuYrL7hpAjQv9s9vyJdQT+/IlIhgM3g+MiQat+LO3uUO10xIspK1+hIglJ
A9VTP1o6Il8hRlQGrqLl
=p21r
-END PGP SIGNATURE-

Re: [PATCH v4] USB: serial: cp210x: Adding GPIO support for CP2105

[Karl Palsson]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Martyn Welch  wrote:
> 
> 
> 
> The cp2105 is the only one of these devices that muxes GPIO
> with serial control signals. The cp2102, cp2103 and cp2108
> provide both GPIO and serial control signals separately, so the
> logic to configure the GPIO for those devices will be much
> simpler. (Hence why I called this cp210x_shared_gpio_init, as I
> figured the best approach for supporting the other devices
> would be through a separate init routine).
> 
> > What about the other configurations supported for these pins (e.g.
> > traffic indication, rs485, ...). Would you also end up with non-zero
> > values for those?
> >
> 
> Will investigate.
> 

CP2105 is configured by OTP rom, so should those options even be
configurable? It's _very_ easy to blow through your settings
trying to configure these. Especially given that cp2102 and
cp2104 and cp2108 all have proper EEPROM for these settings. (And
the "dual" ports aren't remotely equal, the cp2105 is a kinda
ugly stepchild really)

Cheers,
Karl P

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBAgAGBQJWsIfxAAoJEBmotQ/U1cr2MZIQAKG+N/qiXbv0VYB3nglB/xss
d9Jhx9BuCHFcMdxH/zKipqxn43v7/QRJvCdXNRYyDWWjbNvgi7VjqaGo+ywcgE9P
KZxF3DgvUD0ybrrkEV2p9p7Gu2y2tGQazQLAKXAuRTmHRJnASasOVTEBXI8n7IR1
oTO0WctaTITHl7cA7BcZe3oIN9E5XU1GLTqaOP5pWqwiz55xDbFUVHkfhq+psHPR
X8P5Vn9yKWYWa4qw/Xg8p6SdW83sdlAIIN+XXrM8kHH1S3YHT0oi2KcDuIY4wsxD
WkqnPnzsJG7yFTXcXqyEaV4s54ALsKTcFIIMLfP+O6H7OYD/BIxCbWa7RfBBB0ts
tfml5cx7gfy7SUwmaZVs9hCbwQskx8jU2mnl685dbGQH6zAV4DfAPYQb3aYQxkP8
B+klSwBPTptYvZLMiuvb9cUF6XpdQqDmdC820APAaEv1cl+uuqjtn2YkLlEv/JqV
sZmkpiNH4yVwvTRcqOYtaMZbLndnXJaz667dkYGfEOO6njWFmYHy0MJBY8O1OKzO
ZZtpoa6oAT6kmbF5DR0wUpl9hu7O423qogLngDYYP5jUZUwxQtvsSsdYRWfF+wCX
FAcwzYVZ/EiyAU/jeyOZhxoMZ5+x6Ikze1Kb4KJJKvKnLJu57bHHGPsby0ZJPa6b
tWxRfTEBA5limAc0NXSF
=ZPkX
-END PGP SIGNATURE-

Re: ch341: support new device types (Was: Re: PROBLEM: ch341 module fails with EPROTO when I plug-in Arduino board)

[Karl Palsson]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Johan Hovold jo...@kernel.org wrote:
 On Mon, Jul 06, 2015 at 03:24:17PM +0200, Peter Stuge wrote:
 
  Unrelated, but I also have some ch341 hardware, and mine does not
  work with the mainline driver, it does however work reliably with the
  vendor driver.
  
  The two drivers initialize the hardware fairly differently,
  especially the baudrate generator, and the mainline driver doesn't
  support all control signals and modes.
  
  A few things are obvious and easy for me to fix and post, but I'm not
  sure how to proceed with the more complicated things; the baudrate
  seems to have two settings, one clock divisor set on init and another
  set depending on the selected baudrate. Mainline uses a different
  algorithm to set these than the vendor driver.
  
  I notice that my hardware version is more recent than the mainline
  driver was written for. I also notice that my hardware does not
  respond according to the expectations documented in mainline
  comments.
  
  
  Your thoughts? I know that another person was also working on this.
 
 It should be possible to detect what version of the hardware is being
 used and switch baudrate-configuration scheme accordingly if necessary.
 Does anyone know how the vendor driver works with older devices?
 
 I've added Karl on CC who I think have been working on getting these new
 devices supported as well.

working on it is perhaps a very generous way of describing it.  It's one of 
many projects, and it's not very often on the top of the pile.  My last work is 
here  https://github.com/karlp/linux/tree/ch341-3.18.6  and included some work 
on reading out the baud rate.  It also supports the reopening problems but it's 
not ready for submission.

I can test out some fixes, but it's simply not a priority for me right
now unfortunately.

one day, one day...

Sincerely,
Karl Palsson

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBAgAGBQJVm7KIAAoJEBmotQ/U1cr2OH8P/A8/tWWNCRz6fgUtPxbmM73/
D272u1kTBC26PwOEN0Eb9gCAXEDk6xb7+1GcKTSzZ7v2LyAV9EbC/rfUw04+omLB
F/hzNEB8h6xDIZN1Pe/3jOeZlYJxhflz/Uh1+TN7In5mr73ePhe8mrGX8UF5fTMF
1LkxExLUnVeQwGYQ9ud6EbI/mtrykz+HjsAFGpX7sDBqnlUasLU2+jDHM1nUYjp2
HXsWFlCt4/tIRUduxZiiLT0BabDCDcdlmWtu3Yd6O07GrZmfB87bvA6tH9iIMMQL
Ib3AxTQwb9MB+uu7zT4OBIr0c+nkhIDsMkGb30MxPcP/1CJ4ROwPHWDfJrh9vs0J
+bJW3M+pgPlS2oCXY8lZCcRxf0tcDGgHp/MDzmDavPA07jPlin7NyTNvFjcfDHJw
MO86fDImwjmokC3BesRDNUkdbX8+uh1aYTxmrLhDJxMXJ9RbF954QS1Ol6BebDqD
ky0MUJTepY95mb8p760erwZUWmNIJID8/ypvxKOUB2h/uvVhrMITWgr/K5dS9guj
i5AeJySNrs3GAM5a/RXRV6cgSQeF3cjSqD4gH9/uASz1jaAYlQLIdQyZVwlbrC6d
4TnQu+k0uZAcOJNUmKDOfr3zrlMeU2y6OMJE8NDrSx5OBzp/FJmrX9F7OHx0Yc8r
fZiL3bdtS6AIdMs1ZfLo
=iQyG
-END PGP SIGNATURE-

Re: Re: excessive usb traffic with FT230x

[Karl Palsson]

Johan Hovold jo...@kernel.org wrote:
  
  The usbmon traces seem to be a continual reporting of the 0x0160 shown
  in the ftdi_get_modem_status lines.  Is this actually _meant_ to be
  happening?  Can I turn it off?  Any other suggestions?
 
 This is the device reporting the modem and line status every
 millisecond. You can increase the interval by clearing the low_latency
 flag and setting a even higher value than the otherwise default 16ms
 period through sysfs.
 
 Note that this also means that any data received will not necessarily be
 forwarded every millisecond either anymore.
 

Ok, I've now seen that this is actually happening with my ftdi ft232
devices on desktop linux too [1], but not for my cp210x, and not for a
ch341 device

If I'm not interested in modem status, can I disable this entirely?  I
don't even have modem lines connected.  

Sincerely,
Karl Palsson

[1] I had been capturing on the wrong usb interface before, my bad :(

excessive usb traffic with FT230x

[Karl Palsson]

Hi,

I've been using OpenWrt's usb activity led trigger to display
usb-serial traffic over a cp210x dongle quite successfully, but after
replacing the cp210x with a FT230x, I find that I get constant USB
traffic (every millisecond) as soon as the port is opened.

I've put a usbmon capture available here:
https://github.com/karlp/ftx-linux-usb/blob/master/ftx-3.18.12.openport.idle.pcap
 (Captured with tcpdump -i usbmon1 -w , then opening the port with picocom, 
waiting, then exiting)

This is from opening /dev/ttyUSB0 with picocom.  I've added ftdio_sio
module debugging, which produces the following output

[ 5829.43] ftdi_sio ttyUSB0: Setting CS8
[ 5829.43] ftdi_sio ttyUSB0: get_ftdi_divisor - tty_get_baud_rate reports 
speed 19200
[ 5829.43] ftdi_sio ttyUSB0: get_ftdi_divisor - Baud rate set to 19200 
(divisor 0x809C) on chip FT-X
[ 5829.43] ftdi_sio ttyUSB0: ftdi_set_termios Turning off hardware flow 
control
[ 5829.43] ftdi_sio ttyUSB0: update_mctrl - DTR HIGH, RTS HIGH
[ 5829.43] ftdi_sio ttyUSB0: ftdi_get_modem_status - 0x0160
[ 5829.43] ftdi_sio ttyUSB0: get_ftdi_divisor - tty_get_baud_rate reports 
speed 19200
[ 5829.43] ftdi_sio ttyUSB0: get_ftdi_divisor - Baud rate set to 19200 
(divisor 0x809C) on chip FT-X
[ 5829.43] ftdi_sio ttyUSB0: ftdi_set_termios Turning off hardware flow 
control
 exit from picocom here 
[ 5856.46] ftdi_sio ttyUSB0: ftdi_get_modem_status - 0x0160
[ 5856.46] ftdi_sio ttyUSB0: get_ftdi_divisor - tty_get_baud_rate reports 
speed 19200
[ 5856.46] ftdi_sio ttyUSB0: get_ftdi_divisor - Baud rate set to 19200 
(divisor 0x809C) on chip FT-X
[ 5856.46] ftdi_sio ttyUSB0: ftdi_set_termios Turning off hardware flow 
control
[ 5857.52] ftdi_sio ttyUSB0: ftdi_get_modem_status - 0x0160

It's important to note that /dev/ttyUSB0 still _works_ I just get
substantially more usb traffic than I expect.  I do _not_ see this level
of traffic with any FT232RL devices or cp210x devices, though,
unfortunately, I can't test those with the same hardware.  The FT230x is
permanently connected to a SMSC USB2640 hub on a custom board.  I also
don't have a FT230x I can try on my desktop linux either at this stage.
:(

This is seen with kernel 3.18.12, but was originally seen with 3.10.49. 
I can't see anything in the ftdio_sio driver other than new VID/PIDs
since 3.18, so I haven't tried anything newer.  

The usbmon traces seem to be a continual reporting of the 0x0160 shown
in the ftdi_get_modem_status lines.  Is this actually _meant_ to be
happening?  Can I turn it off?  Any other suggestions?

Sincerely,
Karl Palsson

Re: Re: Re: excessive usb traffic with FT230x

[Karl Palsson]

Greg KH gre...@linuxfoundation.org wrote:
 
  If I'm not interested in modem status, can I disable this entirely?  I
  don't even have modem lines connected.  
 
 Is this data packet somehow causing a problem with your system?  If so,
 I suggest using a different type of usb to serial device, but it might
 be hard to find a device that doesn't do some kind of communication when
 the port is open, that's how data is received from the device :)
 

Yeah, I get that, I just was't expecting so much :)  The device works
just fine, I only even noticed because we'd been using a usb activity
led trigger before, which gave us a nice easy way of blinking lights for
serial traffic.  With the constant traffic of the ftdi devices, it just
means the light was blinking continuously, regardless of traffic.  I'll
probably look at having one of the applications that uses the serial
port to kick of a transient flicker instead.

Thanks kindly for the prompt and useful responses. 

Sincerely,
Karl Palsson

Re: usb : serial : ch341 : set tty baud speed according to tty struct

[Karl Palsson]

Johan Hovold jo...@kernel.org wrote:
 
 Yes, there shouldn't be a need to store the baud rate in the driver (the
 tty layer will keep track of that), but you probably still want to store
 the state of the modem-control signals.

Sounds right.  The modem control signals I'm having a harder time
setting up good tests for, but it's coming together.  I've been testing
with a cp2104 connected back to back with a ch340 device, and 

 Looks like we could get rid of that ch341_configure at every open too?

That's the plan yes.  With more of the control requests decoded, the
_configure call was really just a badly implemented set_termios.  
 
 Looking forward to seeing your patches when you're done. 

Me too!  It's slow going in the evenings, but I'm getting more
comfortable with it slowly but surely :)

Cheers,
Karl P

Re: Re: usb : serial : ch341 : set tty baud speed according to tty struct

[Karl Palsson]

Johan Hovold jo...@kernel.org wrote:
 On Wed, Feb 18, 2015 at 11:32:38AM +0700, Johan Hovold wrote:
  On Tue, Feb 17, 2015 at 10:45:11PM -0500, Nicolas PLANEL wrote:
 
  I believe the fix should be implemented slightly differently however.
  Most usb-serial driver call set_termios from open to handle this issue.
  It looks like you could simply replace the calls to set baudrate and
  handshake in open with
  
  ch341_set_termios(tty, port, NULL);
 
 You currently need to make sure tty is not NULL in case the device is
 being used as a console by the way. Just skip the set_termios call in
 that case.

I've already got most of this working in my branch overhauling this
driver.  I agree, the open call shouldn't be doing all the hard
resetting of settings that it currently does.  I've implemented more of
set termios / get termios.

It's not ready for submission yet, I've got a lot of debug added that
needs to be rebased and squished out, and I still want to do more
testing with hardware flow control and the rest of the modem status
lines.

https://github.com/karlp/linux/commits/ch341-3.18.6

By implementing proper reading of settings from the device, a lot of the
private copies can just be dropped out altogether.

Sincerely,
Karl Palsson

Re: Re: [possible fix] HL-340 USB don't work correctly (ch340 based usb-rs232 adapter)

[Karl Palsson]

Eddi De Pieri e...@depieri.net wrote:
 Hi Karl,
 
 I don't know but as documented by usbsnoop log the value written from
 kernel driver make my device to fail.
 
 Windows driver after some assumption  write back the original value
 (0xc3)
 

Ok,

I'm still progressing on more of that init code, but what I've got
pushed to https://github.com/karlp/linux/tree/ch341-3.18.6 at the moment
is _substantially_ more reliable than the original code.  It might
already fix  your problems.

With the original code, many software packages I tried wouldn't actually
work unless something else had setup the tty explicitly ahead of time.  

I'm still continuing to add the parity and stopbit configurations, and
then need to review break/clear and hardware flow control.

Cheers

Re: [possible fix] HL-340 USB don't work correctly (ch340 based usb-rs232 adapter)

[Karl Palsson]

My work in 


Eddi De Pieri e...@depieri.net wrote:
 Bus 003 Device 014: ID 1a86:7523 QinHeng Electronics HL-340 USB-Serial
 adapter
 
 ch431 kernel driver don't work correctly with this device,
 when URB_FUNCTION_BULK_OR_INTERRUPT_TRANSFER answer/send wrong character
 
 After some experiment  by passing command retrieved from usbsnoop log
 it seems that ch341.c  starts working if comment out following line
 
 // r = ch341_control_out(dev, 0x9a, 0x2518, 0x0050);
 
 my windows driver usbsnoop  | grep 18 25
 07: 03 ms 000398 ms c0 95 18 25 00 00 02 00   c3 00
 36: 04 ms 000620 ms 40 9a 18 25 c2 00 00 00 
 75: 03 ms 011673 ms 40 9a 18 25 c3 00 00 00 
 
 as you can see I get c3 while who first logged windows driver sniff get
 56.

My work in progress updates to this driver had already marked that line
as suspicious.
My draft comment is this sets break + 5 bit mode, why?!  On a ch340,
this _should_ have just been ignored, as it doesn't support 5 bit mode. 
Also, break on this device is actually by disabling the rx and tx
sides, there's a bit in that register for both directions to be enabled
individually.  

It's a little sad that your hl340 device fails, as those are the only
ones that have been working for me so far.  ch341 devices don't work
reliably at all.

 please note that my device after following command
 r = ch341_control_in(dev, 0x5f, 0, 0, buffer, size); (
 answer 0x3000, so it should be a more recent hardware version.
 
 ch34x linux driver from the oem confirmed that 0x5f means hardware
 version)
 
 I need someone that have same device or other devices that needs
 ch341.ko to check that it works properly even if you comment out
 before sending a patch back to the ML.
 

It needs a lot more than just commenting out that line unfortunately.
Good news, I'm working on this again, bad news, I said that 6 months ago
too :(

My current work is at https://github.com/karlp/linux/tree/ch341-3.18.6
but note that this is only the initial cleanup, no fixes at this stage.

Sincerely,
Karl Palsson

Re: Re: ch341.c does not work with new ch34x devices

[Karl Palsson]

I'm seeing this as well, I've got some work in progress towards this,
but not finding the time I'd hoped.  CH340 devices work fine with the
current linux driver, but CH341, very very very flaky, if at all.

Re: Re: [PATCH] USB: serial: add nt124 usb to serial driver

[Karl Palsson]

Johan Hovold jo...@kernel.org wrote:
 On Mon, Dec 08, 2014 at 05:24:17PM -0600, George McCollister wrote:

  +   newline.bParityType = termios-c_cflag  PARENB ?
  +   (termios-c_cflag  PARODD ? 1 : 2) +
  +   (termios-c_cflag  CMSPAR ? 2 : 0) : 0;
 
 This hardly readable. Don't use ?:
 

There's also C_PARENB(tty),  C_PARODD(tty), and C_CMSPAR(tty) macros
available, in addition to the others that Johan pointed out.

Sincerely,
Karl P

Re: [PATCH] cdc-acm: Drop the warning for unusual capabilities

[Karl Palsson]

On Tue, Oct 28, 2014 at 10:20:29AM +0100, Oliver Neukum wrote:
 Let's drop the warning for modems with unusual capabilities,
 the associated quirk and blacklist. They made little sense.
 
[snip]
 @@ -1796,11 +1794,6 @@ static const struct usb_device_id acm_ids[] = {
  
   /* NOTE: non-Nokia COMM/ACM/0xff is likely MSFT RNDIS... NOT a modem! */
  
 - /* Support Lego NXT using pbLua firmware */
 - { USB_DEVICE(0x0694, 0xff00),
 - .driver_info = NOT_A_MODEM,
 - },
 -

Isn't this dropping the device too, not just the quirk?

Sincerely,
Karl Palsson
--
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

cdc-acm with incorrect bInterfaceSubClass

[Karl Palsson]

Hi,

I've got a GW Instek function generator (AFG-2225) that _appears_ like it 
should be a
CDC-ACM device, but it seems to have a bug in it's descriptor.  The same bug as 
Steven
Ackerman had in this thread: 
http://comments.gmane.org/gmane.linux.usb.general/71225
(lsusb -v output at the bottom of this mail)

However, unlike Steven, I can't fix the descriptor. (I guess GW Instek devs 
copied the same
bad example code from renesas?)  I tried modprobe cdc-acm  but other than a 
warning that
the vid/pid args were unused, nothing happened.  I don't have /dev/ttyACM* or
/dev/serial/by-id/*  and dmesg just shows...


Oct 27 14:26:30 pojak kernel: cdc_acm: unknown parameter 'vendor' ignored
Oct 27 14:26:30 pojak kernel: cdc_acm: unknown parameter 'product' ignored
Oct 27 14:26:30 pojak kernel: usbcore: registered new interface driver cdc_acm
Oct 27 14:26:30 pojak kernel: cdc_acm: USB Abstract Control Model driver for 
USB modems and ISDN adapters
 replug device here 
Oct 27 14:35:41 pojak kernel: usb 3-4.1: USB disconnect, device number 12
Oct 27 14:35:43 pojak kernel: usb 3-4.1: new full-speed USB device number 13 
using xhci_hcd
Oct 27 14:35:44 pojak kernel: usb 3-4.1: New USB device found, idVendor=2184, 
idProduct=001c
Oct 27 14:35:44 pojak kernel: usb 3-4.1: New USB device strings: Mfr=1, 
Product=2, SerialNumber=3
Oct 27 14:35:44 pojak kernel: usb 3-4.1: Product: AFG-2225
Oct 27 14:35:44 pojak kernel: usb 3-4.1: Manufacturer: GW IISTEK
Oct 27 14:35:44 pojak kernel: usb 3-4.1: SerialNumber: GEN850317
Oct 27 14:35:44 pojak kernel: usb 3-4.1: ep 0x83 - rounding interval to 1024 
microframes, ep desc...rames
Oct 27 14:35:44 pojak mtp-probe[12215]: checking bus 3, device 13: 
/sys/devices/pci:00/:0...4.1
Oct 27 14:35:44 pojak mtp-probe[12215]: bus: 3, device: 13 was not an MTP device


The device appears as a regular serial port in windows, after installing the
gw instek drivers.  Those drivers are an .INF file only, with the following
lines indicating that at least as far as they're concerned, the new and
old devices are the same. (Old device listed as working here:
http://www.tablix.org/~avian/blog/archives/2013/05/gw_instek_afg_2005/

%DESCRIPTION1%=DriverInstall, USB\VID_2184PID_0011
%DESCRIPTION1%=DriverInstall, USB\VID_2184PID_001C

Is there anything I can do to get this to turn up as an ACM device in linux?

Sincerely,
Karl Palsson


karlp@pojak:~$ sudo lsusb -v -d 2184:

Bus 003 Device 012: ID 2184:001c GW Instek 
Device Descriptor:
  bLength18
  bDescriptorType 1
  bcdUSB   2.00
  bDeviceClass2 Communications
  bDeviceSubClass 0 
  bDeviceProtocol 0 
  bMaxPacketSize064
  idVendor   0x2184 GW Instek
  idProduct  0x001c 
  bcdDevice1.00
  iManufacturer   1 GW IISTEK
  iProduct2 AFG-2225
  iSerial 3 GEN850317
  bNumConfigurations  1
  Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength   67
bNumInterfaces  2
bConfigurationValue 1
iConfiguration  0 
bmAttributes 0xc0
  Self Powered
MaxPower  100mA
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber0
  bAlternateSetting   0
  bNumEndpoints   1
  bInterfaceClass 2 Communications
vvv
  bInterfaceSubClass 32 
^^^
  bInterfaceProtocol  0 
  iInterface  0 
  CDC Header:
bcdCDC   1.10
  CDC ACM:
bmCapabilities   0x02
  line coding and serial state
  CDC Union:
bMasterInterface0
bSlaveInterface 1 
  CDC Call Management:
bmCapabilities   0x00
bDataInterface  1
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83  EP 3 IN
bmAttributes3
  Transfer TypeInterrupt
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0008  1x 8 bytes
bInterval 255
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber1
  bAlternateSetting   0
  bNumEndpoints   2
  bInterfaceClass10 CDC Data
  bInterfaceSubClass  0 Unused
  bInterfaceProtocol  0 
  iInterface  0 
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x01  EP 1 OUT
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0040  1x 64 bytes
bInterval   0
  Endpoint

Re: cdc-acm with incorrect bInterfaceSubClass

[Karl Palsson]

On Mon, Oct 27, 2014 at 04:38:48PM +0100, Johan Hovold wrote:
 On Mon, Oct 27, 2014 at 02:52:59PM +, Karl Palsson wrote:
  Hi,
  
  I've got a GW Instek function generator (AFG-2225) that _appears_ like
  it should be a
  CDC-ACM device, but it seems to have a bug in it's descriptor.  The
  same bug as Steven
  Ackerman had in this thread:
  http://comments.gmane.org/gmane.linux.usb.general/71225
  (lsusb -v output at the bottom of this mail)
 
  Is there anything I can do to get this to turn up as an ACM device in linux?
 
 What happens if you add the device id through sysfs?
 
   echo 2184 001c /sys/bus/usb/drivers/cdc_acm/new_id
 

That works fine, shows up perfectly as /dev/ttyACMx and
/dev/serial/by-id/usb-GW_IISTEK_AFG-2225_GEN850317-if00 and I use picocom
to talk to it as expected.

Sincerely,
Karl Palsson
--
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

Re: [PATCH] USB: cdc-acm: add device id for GW Instek AFG-2225

[Karl Palsson]

On Mon, Oct 27, 2014 at 06:34:33PM +0100, Johan Hovold wrote:
 Add device-id entry for GW Instek AFG-2225, which has a byte swapped
 bInterfaceSubClass (0x20).
 
 Reported-by: Karl Palsson ka...@tweak.net.au
 Cc: stable sta...@vger.kernel.org
 Signed-off-by: Johan Hovold jo...@kernel.org
 ---
 
 Care to give this patch a try as well, Karl? You can respond with a
 Tested-by tag if you want.
 

Not really setup for redoing kernels in the day job, but will see what I can
 do tomorrow, as I'm getting a bit better at doing it at home now :)

One thing, I see in the table of ids, that there's a NOT_A_MODEM define.
Is that something that should be set here too? Would it stop modem-manager
from insisting on trying to probe the device?  I've gotten kind of used to
black listing modem manager from generic serial ports provied via usb, but
this is pretty clearly never going to be a modem. 

Sincerely,
Karl Palsson
--
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

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

[Karl Palsson]

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

Re: Driver CH341 USB Adapter Serial Port not Works in Linux

[Karl Palsson]

On Sun, Apr 13, 2014 at 08:51:56PM -0430, Kijam López wrote:
 The following code works for me correctly in Windows, but Linux does
 not work. I am using the same PC, both operating systems are installed
 native. I do not use virtual machine. I need to work on Linux. I have
 tried in different linux distributions and does not work anywhere.
[snip]
   if(!serial.setParity(QSerialPort::EvenParity))
 qCritical()Error in setParity;

the ch34x driver does not (yet) support parities other than None.  It doesn't 
report a
failure though, it just simply ignores it altogether.

If you're actually working with an even parity device, you're going to have all 
sorts of
not-fun.  I've got a patch here: 
http://www.spinics.net/lists/linux-usb/msg105238.html
but I've not (yet) resubmitted it after Johan Hovold's comments

Cheers,
Karl P
--
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

patch: usb/serial/ch341: v2: Add parity support

[Karl Palsson]

Changes since v1:
* Use the C_CMSPAR macros from 3.14 as requested by Johan Hovold

v1 was here: http://comments.gmane.org/gmane.linux.usb.general/105940

Sincerely,
Karl Palsson

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

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

[Karl Palsson]

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)) {
+   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);
+
 }
 
 static void ch341_break_ctl(struct tty_struct *tty, int break_state)
-- 
1.9.0

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

Patch: usb/serial/ch341: Add parity support

[Karl Palsson]

I originally sent this to fr...@kingswood-consulting.co.uk who is listed as the
maintainer for this driver, but I haven't heard a reply, so I'm posting to the
list.  This is my first patch for a driver, so I've tried to follow the
existing style, but if there are any changes that should be made, please let me
know. (There is almost no debugging in this driver, so possibly I should remove
the debug I added?)

This was developed against 3.13.6 and 3.13.7, but has been rebased against
3.14.

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

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

[Karl Palsson]

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..afd2ec4 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 (tty-termios.c_cflag  CMSPAR) {
+   dev_dbg(port-dev, parity = mark\n);
+   par_flags = 0xeb;
+   } else {
+   dev_dbg(port-dev, parity = odd\n);
+   par_flags = 0xcb;
+   }
+   } else {
+   if (tty-termios.c_cflag  CMSPAR) {
+   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);
+
 }
 
 static void ch341_break_ctl(struct tty_struct *tty, int break_state)
-- 
1.8.3.1

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