Re: fix uchcom(4) handling of parity and character size config

2021-10-29 Thread Patrick Wildt 
Am Wed, Oct 27, 2021 at 06:46:38AM +1000 schrieb David Gwynne:
> i bought some random usb to rs485 serial adapters so i can talk
> modbus to a thing, but then discovered i can't talk to the modbus
> thing because uchcom doesn't support configuring parity.
> 
> this ports the functionality to support configuring parity and char size
> masks from netbsd src/sys/dev/usb/uchcom.c r1.26. part of that change
> including tweaks to uchcom_reset_chip, which was then changed in r1.28
> back to what we already have, so i left that chunk out.
> 
> ive tested this talking to a device at 19200 with cs8 and even
> parity. more tests would be appreciated to make sure i haven't
> broken existing use functionality.

ok patrick@

> Index: uchcom.c
> ===
> RCS file: /cvs/src/sys/dev/usb/uchcom.c,v
> retrieving revision 1.28
> diff -u -p -r1.28 uchcom.c
> --- uchcom.c  31 Jul 2020 10:49:33 -  1.28
> +++ uchcom.c  26 Oct 2021 20:33:36 -
> @@ -72,9 +72,8 @@ int uchcomdebug = 0;
>  #define UCHCOM_REG_BPS_DIV   0x13
>  #define UCHCOM_REG_BPS_MOD   0x14
>  #define UCHCOM_REG_BPS_PAD   0x0F
> -#define UCHCOM_REG_BREAK10x05
> -#define UCHCOM_REG_BREAK20x18
> -#define UCHCOM_REG_LCR1  0x18
> +#define UCHCOM_REG_BREAK 0x05
> +#define UCHCOM_REG_LCR   0x18
>  #define UCHCOM_REG_LCR2  0x25
>  
>  #define UCHCOM_VER_200x20
> @@ -83,11 +82,25 @@ int   uchcomdebug = 0;
>  #define UCHCOM_BPS_MOD_BASE  2000
>  #define UCHCOM_BPS_MOD_BASE_OFS  1100
>  
> +#define UCHCOM_BPS_PRE_IMM   0x80/* CH341: immediate RX forwarding */
> +
>  #define UCHCOM_DTR_MASK  0x20
>  #define UCHCOM_RTS_MASK  0x40
>  
> -#define UCHCOM_BRK1_MASK 0x01
> -#define UCHCOM_BRK2_MASK 0x40
> +#define UCHCOM_BREAK_MASK0x01
> +
> +#define UCHCOM_LCR_CS5   0x00
> +#define UCHCOM_LCR_CS6   0x01
> +#define UCHCOM_LCR_CS7   0x02
> +#define UCHCOM_LCR_CS8   0x03
> +#define UCHCOM_LCR_STOPB 0x04
> +#define UCHCOM_LCR_PARENB0x08
> +#define UCHCOM_LCR_PARODD0x00
> +#define UCHCOM_LCR_PAREVEN   0x10
> +#define UCHCOM_LCR_PARMARK   0x20
> +#define UCHCOM_LCR_PARSPACE  0x30
> +#define UCHCOM_LCR_TXE   0x40
> +#define UCHCOM_LCR_RXE   0x80
>  
>  #define UCHCOM_INTR_STAT10x02
>  #define UCHCOM_INTR_STAT20x03
> @@ -577,23 +590,21 @@ int
>  uchcom_set_break(struct uchcom_softc *sc, int onoff)
>  {
>   usbd_status err;
> - uint8_t brk1, brk2;
> + uint8_t brk, lcr;
>  
> - err = uchcom_read_reg(sc, UCHCOM_REG_BREAK1, , UCHCOM_REG_BREAK2,
> - );
> + err = uchcom_read_reg(sc, UCHCOM_REG_BREAK, , UCHCOM_REG_LCR, );
>   if (err)
>   return EIO;
>   if (onoff) {
>   /* on - clear bits */
> - brk1 &= ~UCHCOM_BRK1_MASK;
> - brk2 &= ~UCHCOM_BRK2_MASK;
> + brk &= ~UCHCOM_BREAK_MASK;
> + lcr &= ~UCHCOM_LCR_TXE;
>   } else {
>   /* off - set bits */
> - brk1 |= UCHCOM_BRK1_MASK;
> - brk2 |= UCHCOM_BRK2_MASK;
> + brk |= UCHCOM_BREAK_MASK;
> + lcr |= UCHCOM_LCR_TXE;
>   }
> - err = uchcom_write_reg(sc, UCHCOM_REG_BREAK1, brk1, UCHCOM_REG_BREAK2,
> - brk2);
> + err = uchcom_write_reg(sc, UCHCOM_REG_BREAK, brk, UCHCOM_REG_LCR, lcr);
>   if (err)
>   return EIO;
>  
> @@ -665,23 +676,50 @@ uchcom_set_dte_rate(struct uchcom_softc 
>  int
>  uchcom_set_line_control(struct uchcom_softc *sc, tcflag_t cflag)
>  {
> - /*
> -  * XXX: it is difficult to handle the line control appropriately:
> -  *   work as chip default - CS8, no parity, !CSTOPB
> -  *   other modes are not supported.
> -  */
> + usbd_status err;
> + uint8_t lcr = 0, lcr2 = 0;
> +
> + err = uchcom_read_reg(sc, UCHCOM_REG_LCR, ,
> + UCHCOM_REG_LCR2, );
> + if (err) {
> + printf("%s: cannot get LCR: %s\n",
> + sc->sc_dev.dv_xname, usbd_errstr(err));
> + return EIO;
> + }
> +
> + lcr = UCHCOM_LCR_RXE | UCHCOM_LCR_TXE;
>  
>   switch (ISSET(cflag, CSIZE)) {
>   case CS5:
> + lcr |= UCHCOM_LCR_CS5;
> + break;
>   case CS6:
> + lcr |= UCHCOM_LCR_CS6;
> + break;
>   case CS7:
> - return EINVAL;
> + lcr |= UCHCOM_LCR_CS7;
> + break;
>   case CS8:
> + lcr |= UCHCOM_LCR_CS8;
>   break;
>   }
>  
> - if (ISSET(cflag, PARENB) || ISSET(cflag, CSTOPB))
> - return EINVAL;
> + if (ISSET(cflag, PARENB)) {
> + lcr |= UCHCOM_LCR_PARENB;
> + if (!ISSET(cflag, PARODD))
> + lcr |= UCHCOM_LCR_PAREVEN;
> + }
> +
> + if (ISSET(cflag, CSTOPB)) {
> + lcr |= UCHCOM_LCR_STOPB;
> + }
>

Re: fix uchcom(4) handling of parity and character size config

2021-10-26 Thread Felix Kronlage-Dammers 
On Wed, Oct 27, 2021 at 06:46:38AM +1000, David Gwynne wrote:

Hi David,

> this ports the functionality to support configuring parity and char size
> masks from netbsd src/sys/dev/usb/uchcom.c r1.26. part of that change
> including tweaks to uchcom_reset_chip, which was then changed in r1.28
> back to what we already have, so i left that chunk out.
> 
> ive tested this talking to a device at 19200 with cs8 and even
> parity. more tests would be appreciated to make sure i haven't
> broken existing use functionality.

I use uchcom(4) to connect with 115200 to the pinebook pro. I see no
regressions with your diff, works fine.

felix

fix uchcom(4) handling of parity and character size config

2021-10-26 Thread David Gwynne 
i bought some random usb to rs485 serial adapters so i can talk
modbus to a thing, but then discovered i can't talk to the modbus
thing because uchcom doesn't support configuring parity.

this ports the functionality to support configuring parity and char size
masks from netbsd src/sys/dev/usb/uchcom.c r1.26. part of that change
including tweaks to uchcom_reset_chip, which was then changed in r1.28
back to what we already have, so i left that chunk out.

ive tested this talking to a device at 19200 with cs8 and even
parity. more tests would be appreciated to make sure i haven't
broken existing use functionality.

Index: uchcom.c
===
RCS file: /cvs/src/sys/dev/usb/uchcom.c,v
retrieving revision 1.28
diff -u -p -r1.28 uchcom.c
--- uchcom.c31 Jul 2020 10:49:33 -  1.28
+++ uchcom.c26 Oct 2021 20:33:36 -
@@ -72,9 +72,8 @@ int   uchcomdebug = 0;
 #define UCHCOM_REG_BPS_DIV 0x13
 #define UCHCOM_REG_BPS_MOD 0x14
 #define UCHCOM_REG_BPS_PAD 0x0F
-#define UCHCOM_REG_BREAK1  0x05
-#define UCHCOM_REG_BREAK2  0x18
-#define UCHCOM_REG_LCR10x18
+#define UCHCOM_REG_BREAK   0x05
+#define UCHCOM_REG_LCR 0x18
 #define UCHCOM_REG_LCR20x25
 
 #define UCHCOM_VER_20  0x20
@@ -83,11 +82,25 @@ int uchcomdebug = 0;
 #define UCHCOM_BPS_MOD_BASE2000
 #define UCHCOM_BPS_MOD_BASE_OFS1100
 
+#define UCHCOM_BPS_PRE_IMM 0x80/* CH341: immediate RX forwarding */
+
 #define UCHCOM_DTR_MASK0x20
 #define UCHCOM_RTS_MASK0x40
 
-#define UCHCOM_BRK1_MASK   0x01
-#define UCHCOM_BRK2_MASK   0x40
+#define UCHCOM_BREAK_MASK  0x01
+
+#define UCHCOM_LCR_CS5 0x00
+#define UCHCOM_LCR_CS6 0x01
+#define UCHCOM_LCR_CS7 0x02
+#define UCHCOM_LCR_CS8 0x03
+#define UCHCOM_LCR_STOPB   0x04
+#define UCHCOM_LCR_PARENB  0x08
+#define UCHCOM_LCR_PARODD  0x00
+#define UCHCOM_LCR_PAREVEN 0x10
+#define UCHCOM_LCR_PARMARK 0x20
+#define UCHCOM_LCR_PARSPACE0x30
+#define UCHCOM_LCR_TXE 0x40
+#define UCHCOM_LCR_RXE 0x80
 
 #define UCHCOM_INTR_STAT1  0x02
 #define UCHCOM_INTR_STAT2  0x03
@@ -577,23 +590,21 @@ int
 uchcom_set_break(struct uchcom_softc *sc, int onoff)
 {
usbd_status err;
-   uint8_t brk1, brk2;
+   uint8_t brk, lcr;
 
-   err = uchcom_read_reg(sc, UCHCOM_REG_BREAK1, , UCHCOM_REG_BREAK2,
-   );
+   err = uchcom_read_reg(sc, UCHCOM_REG_BREAK, , UCHCOM_REG_LCR, );
if (err)
return EIO;
if (onoff) {
/* on - clear bits */
-   brk1 &= ~UCHCOM_BRK1_MASK;
-   brk2 &= ~UCHCOM_BRK2_MASK;
+   brk &= ~UCHCOM_BREAK_MASK;
+   lcr &= ~UCHCOM_LCR_TXE;
} else {
/* off - set bits */
-   brk1 |= UCHCOM_BRK1_MASK;
-   brk2 |= UCHCOM_BRK2_MASK;
+   brk |= UCHCOM_BREAK_MASK;
+   lcr |= UCHCOM_LCR_TXE;
}
-   err = uchcom_write_reg(sc, UCHCOM_REG_BREAK1, brk1, UCHCOM_REG_BREAK2,
-   brk2);
+   err = uchcom_write_reg(sc, UCHCOM_REG_BREAK, brk, UCHCOM_REG_LCR, lcr);
if (err)
return EIO;
 
@@ -665,23 +676,50 @@ uchcom_set_dte_rate(struct uchcom_softc 
 int
 uchcom_set_line_control(struct uchcom_softc *sc, tcflag_t cflag)
 {
-   /*
-* XXX: it is difficult to handle the line control appropriately:
-*   work as chip default - CS8, no parity, !CSTOPB
-*   other modes are not supported.
-*/
+   usbd_status err;
+   uint8_t lcr = 0, lcr2 = 0;
+
+   err = uchcom_read_reg(sc, UCHCOM_REG_LCR, ,
+   UCHCOM_REG_LCR2, );
+   if (err) {
+   printf("%s: cannot get LCR: %s\n",
+   sc->sc_dev.dv_xname, usbd_errstr(err));
+   return EIO;
+   }
+
+   lcr = UCHCOM_LCR_RXE | UCHCOM_LCR_TXE;
 
switch (ISSET(cflag, CSIZE)) {
case CS5:
+   lcr |= UCHCOM_LCR_CS5;
+   break;
case CS6:
+   lcr |= UCHCOM_LCR_CS6;
+   break;
case CS7:
-   return EINVAL;
+   lcr |= UCHCOM_LCR_CS7;
+   break;
case CS8:
+   lcr |= UCHCOM_LCR_CS8;
break;
}
 
-   if (ISSET(cflag, PARENB) || ISSET(cflag, CSTOPB))
-   return EINVAL;
+   if (ISSET(cflag, PARENB)) {
+   lcr |= UCHCOM_LCR_PARENB;
+   if (!ISSET(cflag, PARODD))
+   lcr |= UCHCOM_LCR_PAREVEN;
+   }
+
+   if (ISSET(cflag, CSTOPB)) {
+   lcr |= UCHCOM_LCR_STOPB;
+   }
+
+   err = uchcom_write_reg(sc, UCHCOM_REG_LCR, lcr, UCHCOM_REG_LCR2, lcr2);
+   if (err) {
+   printf("%s: cannot set LCR: %s\n",
+   sc->sc_dev.dv_xname, usbd_errstr(err));
+