ch341: Add support for HL340 devices
Subsystems (2)
cover ch341: Add support for HL340 devices
A subset of CH341 devices does not support all features, namely the prescaler is limited to a reduced precision and there is no support for sending a RS232 break condition.
These devices can usually be identified by an imprint of "340" on the turquoise-colored plug. They're also sometimes called "HL340", hence the terminology in this series and driver.
This series adds detection of these devices, adjusts the divisor/prescaler setup and implements a simulated break condition.
Michael Hanselmann (4):
ch341: Name more registers
ch341: Detect HL340 variant
ch341: Limit prescaler on HL340 variant
ch341: Simulate break condition on HL340 variant
drivers/usb/serial/ch341.c | 196 +++++++++++++++++++++++++++++++++---- 1 file changed, 176 insertions(+), 20 deletions(-)
Signature · 1 line
2.20.1
PATCH 1/4 ch341: Name more registers
Add more #defines with register names.
--- drivers/usb/serial/ch341.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/usb/serial/ch341.c +9 -2
@@ -59,6 +59,8 @@#define CH341_REQ_MODEM_CTRL 0xA4#define CH341_REG_BREAK 0x05 +#define CH341_REG_PRESCALER 0x12 +#define CH341_REG_DIVISOR 0x13#define CH341_REG_LCR 0x18#define CH341_NBREAK_BITS 0x01
@@ -221,6 +223,7 @@ static int ch341_get_divisor(speed_t speed)static int ch341_set_baudrate_lcr (struct usb_device *dev,struct ch341_private *priv,u8 lcr) { +uint16_t reg;int val;int r;
@@ -237,11 +240,15 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,*/ val |=BIT (7 ); - r =ch341_control_out (dev,CH341_REQ_WRITE_REG ,0x1312 , val); + reg = ((uint16_t )CH341_REG_DIVISOR <<8 ) |CH341_REG_PRESCALER ; + + r =ch341_control_out (dev,CH341_REQ_WRITE_REG , reg, val);if (r)return r; - r =ch341_control_out (dev,CH341_REQ_WRITE_REG ,0x2518 , lcr); + reg = ((uint16_t )0x2500 ) |CH341_REG_LCR ; + + r =ch341_control_out (dev,CH341_REQ_WRITE_REG , reg, lcr);if (r)return r;
Signature · 1 line
2.20.1
Johan Hovold
maintainerRe: [PATCH 1/4] ch341: Name more registers
Quoted 1 line · Michael Hanselmann
Add more #defines with register names.
Please be more specific (e.g. which registers are you naming). Update the patch summary too.
Quoted 19 lines
Signed-off-by Michael Hanselmann <[email protected]>--- drivers/usb/serial/ch341.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)drivers/usb/serial/ch341.c +2
-0@@ -59,6 +59,8 @@#define CH341_REQ_MODEM_CTRL 0xA4#define CH341_REG_BREAK 0x05 +#define CH341_REG_PRESCALER 0x12 +#define CH341_REG_DIVISOR 0x13#define CH341_REG_LCR 0x18#define CH341_NBREAK_BITS 0x01
drivers/usb/serial/ch341.c static int ch341_get_divisor(speed_t speed) · line 226
Quoted 4 lines
drivers/usb/serial/ch341.c +1
-0@@ -221,6 +223,7 @@ static int ch341_get_divisor(speed_t speed)static int ch341_set_baudrate_lcr (struct usb_device *dev,struct ch341_private *priv,u8 lcr) { +uint16_t reg;
Use u16.
drivers/usb/serial/ch341.c static int ch341_set_baudrate_lcr(struct usb_device *dev, · line 243
Quoted 9 lines
drivers/usb/serial/ch341.c +1
-1int val;int r;@@ -237,11 +240,15 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,*/ val |=BIT (7 ); - r =ch341_control_out (dev,CH341_REQ_WRITE_REG ,0x1312 , val); + reg = ((uint16_t )CH341_REG_DIVISOR <<8 ) |CH341_REG_PRESCALER ;
No need to cast.
drivers/usb/serial/ch341.c static int ch341_set_baudrate_lcr(struct usb_device *dev, · line 249
Quoted 7 lines
drivers/usb/serial/ch341.c +3
-1+ + r =ch341_control_out (dev,CH341_REQ_WRITE_REG , reg, val);if (r)return r; - r =ch341_control_out (dev,CH341_REQ_WRITE_REG ,0x2518 , lcr); + reg = ((uint16_t )0x2500 ) |CH341_REG_LCR ;
Ditto, and please add a bit shift for consistency.
Hmm, but this is unrelated to the defines you are adding, and there are other magic constants for registers in this driver. Perhaps drop this bit or break it out in its own patch (rule of thumb: one logical change per patch).
drivers/usb/serial/ch341.c static int ch341_set_baudrate_lcr(struct usb_device *dev, · line 253
Quoted 4 lines
drivers/usb/serial/ch341.c +2
-0+ + r =ch341_control_out (dev,CH341_REQ_WRITE_REG , reg, lcr);if (r)return r;
I'd also move this last in the series as it's more of a clean up or documentation patch.
Johan
Re: [PATCH 1/4] ch341: Name more registers
Quoted 5 lines · Johan Hovold
Quoted 1 line · Michael Hanselmann
Add more #defines with register names.
Please be more specific (e.g. which registers are you naming). Update the patch summary too.
Done. Will be in the next revision.
Quoted 9 lines
drivers/usb/serial/ch341.c static int ch341_set_baudrate_lcr(struct usb_device *dev, · line 249
Quoted 2 lines
drivers/usb/serial/ch341.c +1
-1- r =ch341_control_out (dev,CH341_REQ_WRITE_REG ,0x2518 , lcr); + reg = ((uint16_t )0x2500 ) |CH341_REG_LCR ;Ditto, and please add a bit shift for consistency.
Hmm, but this is unrelated to the defines you are adding, and there are other magic constants for registers in this driver. Perhaps drop this bit or break it out in its own patch (rule of thumb: one logical change per patch).
I figured out what the 0x25 register is, or maybe was, and updated the patch with a description and moved it later in the series, just before the simulated break condition.
Michael
PATCH 2/4 ch341: Detect HL340 variant
In reply to Michael HanselmannA subset of CH341 devices does not support all features, namely the prescaler is limited to a reduced precision and there is no support for sending a RS232 break condition.
These devices can usually be identified by an imprint of "340" on the turquoise-colored plug. They're called "HL340" in this driver.
--- drivers/usb/serial/ch341.c | 42 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)
drivers/usb/serial/ch341.c +42 -0
@@ -89,6 +89,7 @@ struct ch341_private {u8 mcr;u8 msr;u8 lcr; +u8 flags; }; static void ch341_set_termios(struct tty_struct *tty,
@@ -315,6 +316,43 @@ out: kfree(buffer);return r; } +/* +* A subset of CH341 devices, called "HL340" in this driver, does not support +* all features. The prescaler is limited and there is no support for sending +* a RS232 break condition. A read failure when trying to set up the latter is +* used to detect these devices. +*/ +static int ch341_detect_hl340 (struct usb_device *dev) +{ +const unsigned int size =2 ; +char *buffer; +int r; + + buffer =kmalloc (size,GFP_KERNEL ); +if (!buffer) +return -ENOMEM ; + + r =ch341_control_in (dev,CH341_REQ_READ_REG , +CH341_REG_BREAK ,0 , buffer, size); +if (r == -EPIPE ) { +dev_dbg (&dev->dev,"%s - Chip is a HL340 variant\n" , + __func__); + r =1 ; + goto out; + } + +if (r <0 ) { +dev_err (&dev->dev,"%s - USB control read error (%d)\n" , + __func__, r); + goto out; + } + + r =0 ; + +out:kfree (buffer); +return r; +} +static int ch341_port_probe (struct usb_serial_port *port) {struct ch341_private *priv;
@@ -336,6 +374,10 @@ static int ch341_port_probe(struct usb_serial_port *port)if (r <0 ) goto error; + r =ch341_detect_hl340 (port->serial->dev); +if (r <0 ) + goto error; +usb_set_serial_port_data (port, priv);return 0 ;
Signature · 1 line
2.20.1
Johan Hovold
maintainerRe: [PATCH 2/4] ch341: Detect HL340 variant
Quoted 6 lines · Michael Hanselmann
A subset of CH341 devices does not support all features, namely the prescaler is limited to a reduced precision and there is no support for sending a RS232 break condition.
These devices can usually be identified by an imprint of "340" on the turquoise-colored plug. They're called "HL340" in this driver.
As I mentioned in my reply to the cover letter, you need to come up with a different designation as this does not apply to all HL340 devices.
Quoted 18 lines
Signed-off-by Michael Hanselmann <[email protected]>--- drivers/usb/serial/ch341.c | 42 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)drivers/usb/serial/ch341.c +1
-0@@ -89,6 +89,7 @@ struct ch341_private {u8 mcr;u8 msr;u8 lcr; +u8 flags; }; static void ch341_set_termios(struct tty_struct *tty,
drivers/usb/serial/ch341.c out: kfree(buffer); · line 325
Quoted 10 lines
drivers/usb/serial/ch341.c +7
-0@@ -315,6 +316,43 @@ out: kfree(buffer);return r; } +/* +* A subset of CH341 devices, called "HL340" in this driver, does not support +* all features. The prescaler is limited and there is no support for sending +* a RS232 break condition. A read failure when trying to set up the latter is +* used to detect these devices. +*/ +static int ch341_detect_hl340 (struct usb_device *dev)
Return bool? Rename ch341_is_xxx() ?
drivers/usb/serial/ch341.c out: kfree(buffer); · line 336
Quoted 11 lines
drivers/usb/serial/ch341.c +11
-0+{ +const unsigned int size =2 ; +char *buffer; +int r; + + buffer =kmalloc (size,GFP_KERNEL ); +if (!buffer) +return -ENOMEM ; + + r =ch341_control_in (dev,CH341_REQ_READ_REG , +CH341_REG_BREAK ,0 , buffer, size);
This helper would already have logged an error message, which perhaps is ok, but you could consider using usb_control_msg() directly here.
drivers/usb/serial/ch341.c out: kfree(buffer); · line 348
Quoted 12 lines
drivers/usb/serial/ch341.c +12
-0+if (r == -EPIPE ) { +dev_dbg (&dev->dev,"%s - Chip is a HL340 variant\n" , + __func__); + r =1 ; + goto out; + } + +if (r <0 ) { +dev_err (&dev->dev,"%s - USB control read error (%d)\n" , + __func__, r); + goto out; + }
So this is currently redundant.
drivers/usb/serial/ch341.c out: kfree(buffer); · line 350
Quoted 2 lines
drivers/usb/serial/ch341.c +2
-0+ + r =0 ;
Not needed either, right?
drivers/usb/serial/ch341.c out: kfree(buffer); · line 352
Quoted 2 lines
drivers/usb/serial/ch341.c +2
-0+ +out:kfree (buffer);
Line break after the label, please.
drivers/usb/serial/ch341.c static int ch341_port_probe(struct usb_serial_port *port) · line 379
Quoted 13 lines
drivers/usb/serial/ch341.c +6
-0+return r; +} +static int ch341_port_probe (struct usb_serial_port *port) {struct ch341_private *priv;@@ -336,6 +374,10 @@ static int ch341_port_probe(struct usb_serial_port *port)if (r <0 ) goto error; + r =ch341_detect_hl340 (port->serial->dev); +if (r <0 ) + goto error;
You never store the return value (and the "flags" variable you add is unused) which indicates your series needs to be restructured.
At least update flags in this patch. Perhaps consider renaming it "quirks" depending on how it ends up being used.
drivers/usb/serial/ch341.c static int ch341_port_probe(struct usb_serial_port *port) · line 382
Quoted 3 lines
drivers/usb/serial/ch341.c +1
-0+usb_set_serial_port_data (port, priv);return 0 ;
Johan
Re: [PATCH 2/4] ch341: Detect HL340 variant
Quoted 4 lines · Johan Hovold
Quoted 1 line · Michael Hanselmann
+static int
ch341_detect_hl340(struct usb_device *dev)Return bool? Rename
ch341_is_xxx()?
Changed the function to return the quirks flags, its name is now "ch341_detect_quirks".
Quoted 5 lines
drivers/usb/serial/ch341.c out: kfree(buffer); · line 336
Quoted 2 lines
drivers/usb/serial/ch341.c +2
-0+ r =ch341_control_in (dev,CH341_REQ_READ_REG , +CH341_REG_BREAK ,0 , buffer, size);This helper would already have logged an error message, which perhaps is ok, but you could consider using
usb_control_msg()directly here.
Changed to use "usb_control_msg" directly.
Quoted 6 lines
Quoted 6 lines
drivers/usb/serial/ch341.c +6
-0+if (r == -EPIPE ) { +dev_dbg (&dev->dev,"%s - Chip is a HL340 variant\n" , + __func__); + r =1 ; + goto out; + }
Quoted 8 lines
drivers/usb/serial/ch341.c out: kfree(buffer); · line 348
Quoted 6 lines
drivers/usb/serial/ch341.c +6
-0+ +if (r <0 ) { +dev_err (&dev->dev,"%s - USB control read error (%d)\n" , + __func__, r); + goto out; + }So this is currently redundant.
Not anymore after the change above: EPIPE is encountered for the limited chips, 0 for normal chips and other errors should be reported.
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ +out: kfree(buffer);Line break after the label, please.
Tried to keep the style consistent (see "ch341_port_probe" for example). Done.
Quoted 6 lines
drivers/usb/serial/ch341.c static int ch341_port_probe(struct usb_serial_port *port) · line 379
Quoted 3 lines
drivers/usb/serial/ch341.c +3
-0+ r =ch341_detect_hl340 (port->serial->dev); +if (r <0 ) + goto error;You never store the return value (and the "flags" variable you add is unused) which indicates your series needs to be restructured.
That's different now with the renaming to "ch341_detect_quirks". The actual return value is still 0 as of this patch. Quirks are added in later patches.
Quoted 2 lines
At least update flags in this patch. Perhaps consider renaming it "quirks" depending on how it ends up being used.
Done.
PATCH 3/4 ch341: Limit prescaler on HL340 variant
In reply to Michael HanselmannHL340 devices, a subset of all CH340 devices, do not work correctly when the highest prescaler bit (0b100) is set. Limit these to the lower prescaler values at the cost of timing precision.
--- drivers/usb/serial/ch341.c | 53 ++++++++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 13 deletions(-)
drivers/usb/serial/ch341.c +40 -13
@@ -24,6 +24,8 @@#define DEFAULT_BAUD_RATE 9600#define DEFAULT_TIMEOUT 1000 +#define CH341_QUIRK_LIMITED_PRESCALER 0x01 +/* flags for IO-Bits */ #define CH341_BIT_RTS (1 << 6)#define CH341_BIT_DTR (1 << 5)
@@ -143,13 +145,19 @@ static int ch341_control_in(struct usb_device *dev,#define CH341_CLKRATE 48000000#define CH341_CLK_DIV (ps, fact) (1 << (12 - 3 * (ps) - (fact))) -#define CH341_MIN_RATE (ps) (CH341_CLKRATE / (CH341_CLK_DIV((ps), 1) * 512)) +#define CH341_MIN_RATE (ps, fact) \ + (CH341_CLKRATE / (CH341_CLK_DIV((ps), (fact)) * 512))static const speed_t ch341_min_rates[] = { -CH341_MIN_RATE (0 ), -CH341_MIN_RATE (1 ), -CH341_MIN_RATE (2 ), -CH341_MIN_RATE (3 ), +CH341_MIN_RATE (0 ,0 ), +CH341_MIN_RATE (1 ,0 ), +CH341_MIN_RATE (2 ,0 ), +CH341_MIN_RATE (3 ,0 ), + +CH341_MIN_RATE (0 ,1 ), +CH341_MIN_RATE (1 ,1 ), +CH341_MIN_RATE (2 ,1 ), +CH341_MIN_RATE (3 ,1 ), };/*
@@ -162,24 +170,41 @@ static const speed_t ch341_min_rates[] = {* 2 <= div <= 256 if fact = 0, or * 9 <= div <= 256 if fact = 1 */ -static int ch341_get_divisor (speed_t speed) +static int ch341_get_divisor (struct ch341_private *priv) { +const speed_t *min_rates; +speed_t speed;unsigned int fact, div, clk_div;int ps; + speed = priv->baud_rate; +/* * Clamp to supported range, this makes the (ps < 0) and (div < 2) * sanity checks below redundant. */ speed =clamp (speed,46U ,3000000U ); -/* -* Start with highest possible base clock (fact = 1) that will give a -* divisor strictly less than 512. -*/ - fact =1 ; +if (priv->flags &CH341_QUIRK_LIMITED_PRESCALER ) { +/* +* Devices of the "HL340" variant don't work reliably when the +* third bit is set in the prescaler (0b100). Limit these to +* prescaler values in the range 0..3 (fact = 0) at the cost of +* precision. +*/ + min_rates = &ch341_min_rates[4 ]; + fact =0 ; + }else { +/* +* Start with highest possible base clock (fact = 1) that will +* give a divisor strictly less than 512. +*/ + min_rates = ch341_min_rates; + fact =1 ; + } +for (ps =3 ; ps >=0 ; ps--) { -if (speed > ch341_min_rates[ps]) +if (speed > min_rates[ps])break ; }
@@ -231,7 +256,7 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,if (!priv->baud_rate)return -EINVAL ; - val =ch341_get_divisor (priv->baud_rate); + val =ch341_get_divisor (priv);if (val <0 )return -EINVAL ;
@@ -377,6 +402,8 @@ static int ch341_port_probe(struct usb_serial_port *port) r =ch341_detect_hl340 (port->serial->dev);if (r <0 ) goto error; +else if (r !=0 ) + priv->flags |=CH341_QUIRK_LIMITED_PRESCALER ;usb_set_serial_port_data (port, priv);return 0 ;
Signature · 1 line
2.20.1
Johan Hovold
maintainerRe: [PATCH 3/4] ch341: Limit prescaler on HL340 variant
Quoted 3 lines · Michael Hanselmann
HL340 devices, a subset of all CH340 devices, do not work correctly when the highest prescaler bit (0b100) is set. Limit these to the lower prescaler values at the cost of timing precision.
When we discussed this off list, you said that your device could handle the highest prescaler bit being set for some rates. You specifically confirmed that the 576000 and 921600 rates worked, while 110, 134 and 200 did not.
Could you reconfirm which, if any, of the following rates work with the current driver?
1152000
921600
576000
200
134
110Perhaps we can still continue supporting the higher rates, which are way off unless using the factor-2 prescaler.
Johan
Re: [PATCH 3/4] ch341: Limit prescaler on HL340 variant
Quoted 20 lines · Johan Hovold
Quoted 3 lines · Michael Hanselmann
HL340 devices, a subset of all CH340 devices, do not work correctly when the highest prescaler bit (0b100) is set. Limit these to the lower prescaler values at the cost of timing precision.
When we discussed this off list, you said that your device could handle the highest prescaler bit being set for some rates. You specifically confirmed that the 576000 and 921600 rates worked, while 110, 134 and 200 did not.
Could you reconfirm which, if any, of the following rates work with the current driver?
1152000 921600 576000 200 134 110
Quoted 2 lines
Perhaps we can still continue supporting the higher rates, which are way off unless using the factor-2 prescaler.
You're right, 110, 134 and 200 don't work whereas 576000, 921600 and 1152000 do. I totally missed this while working on the patch.
I ended up doing more research and figured out that fact=0 with ps=0..3 works, as does fact=1 with ps=3. It's only fact=1 with ps0..2 which is not working properly.
The next revision of the series will contain additional patches to restructure the prescaler computation before implementing the quirk.
Michael
PATCH 4/4 ch341: Simulate break condition on HL340 variant
In reply to Michael Hanselmannch34x devices of the "HL340" variant don't support a real break condition. This fact is already used in the ch341_detect_hl340 function. With this change a quirk is implemented to simulate a break condition by temporarily lowering the baud rate and sending a NUL byte.
The primary drawback of this approach is that the duration of the break can't be controlled by userland.
--- drivers/usb/serial/ch341.c | 102 +++++++++++++++++++++++++++++++++---- 1 file changed, 91 insertions(+), 11 deletions(-)
drivers/usb/serial/ch341.c +91 -11
@@ -25,6 +25,10 @@#define DEFAULT_TIMEOUT 1000#define CH341_QUIRK_LIMITED_PRESCALER 0x01 +#define CH341_QUIRK_SIMULATE_BREAK 0x02 + +/* Minimum baud rate */ +#define CH341_MIN_BPS 46U/* flags for IO-Bits */ #define CH341_BIT_RTS (1 << 6)
@@ -92,6 +96,7 @@ struct ch341_private {u8 msr;u8 lcr;u8 flags; +unsigned long break_end; }; static void ch341_set_termios(struct tty_struct *tty,
@@ -170,20 +175,17 @@ static const speed_t ch341_min_rates[] = {* 2 <= div <= 256 if fact = 0, or * 9 <= div <= 256 if fact = 1 */ -static int ch341_get_divisor (struct ch341_private *priv) +static int ch341_get_divisor (struct ch341_private *priv,speed_t speed) {const speed_t *min_rates; -speed_t speed;unsigned int fact, div, clk_div;int ps; - speed = priv->baud_rate; -/* * Clamp to supported range, this makes the (ps < 0) and (div < 2) * sanity checks below redundant. */ - speed =clamp (speed,46U ,3000000U ); + speed =clamp (speed,CH341_MIN_BPS ,3000000U );if (priv->flags &CH341_QUIRK_LIMITED_PRESCALER ) {/*
@@ -247,16 +249,17 @@ static int ch341_get_divisor(struct ch341_private *priv) }static int ch341_set_baudrate_lcr (struct usb_device *dev, -struct ch341_private *priv,u8 lcr) +struct ch341_private *priv, +unsigned baud_rate,u8 lcr) {uint16_t reg;int val;int r; -if (!priv->baud_rate) +if (!baud_rate)return -EINVAL ; - val =ch341_get_divisor (priv); + val =ch341_get_divisor (priv, baud_rate);if (val <0 )return -EINVAL ;
@@ -331,7 +334,7 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv)if (r <0 ) goto out; - r =ch341_set_baudrate_lcr (dev, priv, priv->lcr); + r =ch341_set_baudrate_lcr (dev, priv, priv->baud_rate, priv->lcr);if (r <0 ) goto out;
@@ -403,7 +406,9 @@ static int ch341_port_probe(struct usb_serial_port *port)if (r <0 ) goto error;else if (r !=0 ) - priv->flags |=CH341_QUIRK_LIMITED_PRESCALER ; + priv->flags |= +CH341_QUIRK_LIMITED_PRESCALER | +CH341_QUIRK_SIMULATE_BREAK ;usb_set_serial_port_data (port, priv);return 0 ;
@@ -536,7 +541,8 @@ static void ch341_set_termios(struct tty_struct *tty,if (baud_rate) { priv->baud_rate = baud_rate; - r =ch341_set_baudrate_lcr (port->serial->dev, priv, lcr); + r =ch341_set_baudrate_lcr (port->serial->dev, priv, + priv->baud_rate, lcr);if (r <0 && old_termios) { priv->baud_rate =tty_termios_baud_rate (old_termios);tty_termios_copy_hw (&tty->termios, old_termios);
@@ -555,15 +561,89 @@ static void ch341_set_termios(struct tty_struct *tty,ch341_set_handshake (port->serial->dev, priv->mcr); } +/* +* Devices of the "HL340" variant don't support a real break condition and +* reading CH341_REG_BREAK fails (see also ch341_detect_hl340). This function +* simulates a break condition by lowering the baud rate to the minimum +* supported by the hardware upon enabling the break condition and sending +* a NUL byte. +* +* Normally the duration of the break condition can be controlled individually +* by userspace using TIOCSBRK and TIOCCBRK or by passing an argument to +* TCSBRKP. Due to how the simulation is implemented the duration can't be +* controlled. The duration is always 1s / 46bd * 10bit = 217ms. +*/ +static void ch341_simulate_break (struct tty_struct *tty,int break_state) +{ +struct usb_serial_port *port = tty->driver_data; +struct ch341_private *priv =usb_get_serial_port_data (port); +unsigned long delay; +int r; + +if (break_state !=0 ) { +dev_dbg (&port->dev,"%s - Enter break state requested\n" , + __func__); + + r =ch341_set_baudrate_lcr (port->serial->dev, priv, +CH341_MIN_BPS , +CH341_LCR_ENABLE_RX |CH341_LCR_ENABLE_TX | +CH341_LCR_CS8 ); +if (r <0 ) { +dev_err (&port->dev,"%s - baud rate status %d\n" , + __func__, r); + goto restore; + } + + r =tty_put_char (tty,'\0' ); +if (r <0 ) { +dev_err (&port->dev,"%s - write status %d\n" , + __func__, r); + goto restore; + } + + priv->break_end = jiffies + (10 *HZ /CH341_MIN_BPS ); + +return ; + } + +dev_dbg (&port->dev,"%s - Leave break state requested\n" , __func__); + +if (time_before (jiffies, priv->break_end)) { +/* Wait until NUL byte is written */ + delay = min_t(unsigned long, HZ, priv->break_end - jiffies); + +dev_dbg (&port->dev,"%s - sleep for %d ms\n" , __func__, +jiffies_to_msecs (delay)); +schedule_timeout_interruptible (delay); + } + +restore: +/* Restore original baud rate */ + r =ch341_set_baudrate_lcr (port->serial->dev, priv, priv->baud_rate, + priv->lcr); +if (r <0 ) +dev_err (&port->dev,"%s - baud rate status %d\n" , __func__, r); +} +static void ch341_break_ctl (struct tty_struct *tty,int break_state) {const uint16_t ch341_break_reg = ((uint16_t )CH341_REG_LCR <<8 ) |CH341_REG_BREAK ;struct usb_serial_port *port = tty->driver_data; +struct ch341_private *priv =usb_get_serial_port_data (port);int r;uint16_t reg_contents;uint8_t *break_reg; +if (priv->flags &CH341_QUIRK_SIMULATE_BREAK ) { +dev_warn_once (&port->dev, +"%s - hardware doesn't support real break" +" condition, simulating instead\n" , + __func__); +ch341_simulate_break (tty, break_state); +return ; + } + break_reg =kmalloc (2 ,GFP_KERNEL );if (!break_reg)return ;
Signature · 1 line
2.20.1
Johan Hovold
maintainerRe: [PATCH 4/4] ch341: Simulate break condition on HL340 variant
Quoted 7 lines · Michael Hanselmann
ch34x devices of the "HL340" variant don't support a real break condition. This fact is already used in the
ch341_detect_hl340function. With this change a quirk is implemented to simulate a break condition by temporarily lowering the baud rate and sending a NUL byte.The primary drawback of this approach is that the duration of the break can't be controlled by userland.
Hmm. I'll have to get back to you on this. Please continue keeping it last in the series if you end up resubmitting before.
Johan
Johan Hovold
maintainerHi Michael,
Quoted 7 lines · Michael Hanselmann
A subset of CH341 devices does not support all features, namely the prescaler is limited to a reduced precision and there is no support for sending a RS232 break condition.
These devices can usually be identified by an imprint of "340" on the turquoise-colored plug. They're also sometimes called "HL340", hence the terminology in this series and driver.
You need to come up with a different designation. I have a HL340 device here which works just fine.
Quoted 8 lines
This series adds detection of these devices, adjusts the divisor/prescaler setup and implements a simulated break condition.
Michael Hanselmann (4):
ch341: Name more registers
ch341: Detect HL340 variant
ch341: Limit prescaler on HL340 variant
ch341: Simulate break condition on HL340 variant
Nit: please use a "USB: serial: ch341:" subject prefix.
Also, if possible, please move the second and third patches first in the series as these could be considered fixes rather than new features (and considered for backporting).
Quoted 2 lines
drivers/usb/serial/ch341.c | 196 +++++++++++++++++++++++++++++++++---- 1 file changed, 176 insertions(+), 20 deletions(-)
Johan
Quoted 11 lines · Johan Hovold
Quoted 7 lines · Michael Hanselmann
A subset of CH341 devices does not support all features, namely the prescaler is limited to a reduced precision and there is no support for sending a RS232 break condition.
These devices can usually be identified by an imprint of "340" on the turquoise-colored plug. They're also sometimes called "HL340", hence the terminology in this series and driver.
You need to come up with a different designation. I have a HL340 device here which works just fine.
Thank you, I didn't know whether the behaviour I'm seeing affects others too. I removed the "HL340" designation for the next revision.