> Date: Wed, 14 May 2014 11:04:56 +0200
> From: Martin Pieuchot <[email protected]>
>
> 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.
And really, the old code had magic numbers as well.
> > > 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;
> > >
> >
>
>
