On 13/05/14(Tue) 21:24, Mike Larkin wrote:
> 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.
Unfortunately a lot of USB drivers are written without spec. and some
values are taken by analysing the traffic generated by drivers on other
OS.
> 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.
I also really don't like magic values, but I don't see how we could do
it differently. Maybe somebody can send an email to the author of the
linux driver and ask him what these values are. But I'd bet he doesn't
no neither.
> > ----
> > SASANO Takayoshi <u...@mx5.nisiq.net>
> >
> > 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;
> >
>
