On Wed, May 14, 2014 at 11:02:49AM +0900, SASANO Takayoshi wrote:
> Hi,
> 
> Simply magic values are rewrited with #define.
> If these values need to be disassembled, please take a while...
> 

I think we need to understand what those values mean. When I mentioned
#defines, I meant something like:

#define UCHCOM_SOME_FLAG 0x1234
#define UCHCOM_SOME_OTHER_FLAG 0x5678
...
...
#define UCHCOM_RESET_VALUE (UCHCOM_SOME_FLAG | UCHCOM_SOME_OTHER_FLAG)

That way we know what the values do. If the value we're setting is not
a flag, we should understand what 0x501F and 0xD90A actually mean.

If we don't do it this way, the next person to come along and try to
work on this code won't have any idea what to do.

-ml

> ----
> SASANO Takayoshi <[email protected]>
> 
> Index: uchcom.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uchcom.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 uchcom.c
> --- uchcom.c  15 Nov 2013 10:17:39 -0000      1.19
> +++ uchcom.c  14 May 2014 01:43:34 -0000
> @@ -91,18 +91,14 @@ int       uchcomdebug = 0;
>  #define UCHCOM_BRK1_MASK     0x01
>  #define UCHCOM_BRK2_MASK     0x40
>  
> -#define UCHCOM_LCR1_MASK     0xAF
> -#define UCHCOM_LCR2_MASK     0x07
> -#define UCHCOM_LCR1_PARENB   0x80
> -#define UCHCOM_LCR2_PAREVEN  0x07
> -#define UCHCOM_LCR2_PARODD   0x06
> -#define UCHCOM_LCR2_PARMARK  0x05
> -#define UCHCOM_LCR2_PARSPACE 0x04
> -
>  #define UCHCOM_INTR_STAT1    0x02
>  #define UCHCOM_INTR_STAT2    0x03
>  #define UCHCOM_INTR_LEAST    4
>  
> +/* these values come from Linux (drivers/usb/serial/ch341.c) */
> +#define UCHCOM_RESET_VALUE   0x501F  /* XXX default line mode? */
> +#define UCHCOM_RESET_INDEX   0xD90A  /* XXX default baud rate? */
> +
>  #define UCHCOMIBUFSIZE 256
>  #define UCHCOMOBUFSIZE 256
>  
> @@ -707,27 +703,10 @@ uchcom_set_dte_rate(struct uchcom_softc 
>  int
>  uchcom_set_line_control(struct uchcom_softc *sc, tcflag_t cflag)
>  {
> -     usbd_status err;
> -     uint8_t lcr1 = 0, lcr2 = 0;
> -
> -     err = uchcom_read_reg(sc, UCHCOM_REG_LCR1, &lcr1, UCHCOM_REG_LCR2,
> -         &lcr2);
> -     if (err) {
> -             printf("%s: cannot get LCR: %s\n",
> -                    sc->sc_dev.dv_xname, usbd_errstr(err));
> -             return EIO;
> -     }
> -
> -     lcr1 &= ~UCHCOM_LCR1_MASK;
> -     lcr2 &= ~UCHCOM_LCR2_MASK;
> -
>       /*
>        * XXX: it is difficult to handle the line control appropriately:
> -      *   - CS8, !CSTOPB and any parity mode seems ok, but
> -      *   - the chip doesn't have the function to calculate parity
> -      *     in !CS8 mode.
> -      *   - it is unclear that the chip supports CS5,6 mode.
> -      *   - it is unclear how to handle stop bits.
> +      *   work as chip default - CS8, no parity, !CSTOPB
> +      *   other modes are not supported.
>        */
>  
>       switch (ISSET(cflag, CSIZE)) {
> @@ -739,21 +718,8 @@ uchcom_set_line_control(struct uchcom_so
>               break;
>       }
>  
> -     if (ISSET(cflag, PARENB)) {
> -             lcr1 |= UCHCOM_LCR1_PARENB;
> -             if (ISSET(cflag, PARODD))
> -                     lcr2 |= UCHCOM_LCR2_PARODD;
> -             else
> -                     lcr2 |= UCHCOM_LCR2_PAREVEN;
> -     }
> -
> -     err = uchcom_write_reg(sc, UCHCOM_REG_LCR1, lcr1, UCHCOM_REG_LCR2,
> -         lcr2);
> -     if (err) {
> -             printf("%s: cannot set LCR: %s\n",
> -                    sc->sc_dev.dv_xname, usbd_errstr(err));
> -             return EIO;
> -     }
> +     if (ISSET(cflag, PARENB) || ISSET(cflag, CSTOPB))
> +             return EINVAL;
>  
>       return 0;
>  }
> @@ -778,38 +744,12 @@ int
>  uchcom_reset_chip(struct uchcom_softc *sc)
>  {
>       usbd_status err;
> -     uint8_t lcr1, lcr2, pre, div, mod;
> -     uint16_t val=0, idx=0;
> -
> -     err = uchcom_read_reg(sc, UCHCOM_REG_LCR1, &lcr1, UCHCOM_REG_LCR2, 
> &lcr2);
> -     if (err)
> -             goto failed;
> -
> -     err = uchcom_read_reg(sc, UCHCOM_REG_BPS_PRE, &pre, UCHCOM_REG_BPS_DIV,
> -         &div);
> -     if (err)
> -             goto failed;
> -
> -     err = uchcom_read_reg(sc, UCHCOM_REG_BPS_MOD, &mod, UCHCOM_REG_BPS_PAD,
> -         NULL);
> -     if (err)
> -             goto failed;
> -
> -     val |= (uint16_t)(lcr1&0xF0) << 8;
> -     val |= 0x01;
> -     val |= (uint16_t)(lcr2&0x0F) << 8;
> -     val |= 0x02;
> -     idx |= pre & 0x07;
> -     val |= 0x04;
> -     idx |= (uint16_t)div << 8;
> -     val |= 0x08;
> -     idx |= mod & 0xF8;
> -     val |= 0x10;
>  
> -     DPRINTF(("%s: reset v=0x%04X, i=0x%04X\n",
> -              sc->sc_dev.dv_xname, val, idx));
> +     DPRINTF(("%s: reset\n", sc->sc_dev.dv_xname));
>  
> -     err = uchcom_generic_control_out(sc, UCHCOM_REQ_RESET, val, idx);
> +     err = uchcom_generic_control_out(sc, UCHCOM_REQ_RESET,
> +                                      UCHCOM_RESET_VALUE,
> +                                      UCHCOM_RESET_INDEX);
>       if (err)
>               goto failed;
> 

Reply via email to