Hi Johan
Thanks for the review.
I have a few questions, points to raise.
> > + /* Submit the vendor request */
> > + buf[0] = data_bits;
> > + buf[1] = parity;
> > + buf[2] = stop_bits;
> > + buf[3] = 0;
> > +
> > + err = mxuport_send_ctrl_data_urb(serial, RQ_VENDOR_SET_LINE,
> > + 0, port->port_number, buf, 4);
> > +
> > + if (err != 4)
> > + goto out;
> > +
> > + if (C_BAUD(tty)) {
> > + if (old_termios && (old_termios->c_cflag & CBAUD) == B0) {
> > + /* Raise DTR/RTS */
> > + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_DTR,
> > + 1, port->port_number);
> > + if (err)
> > + goto out;
> > +
> > + mxport->mcr_state |= (UART_MCR_DTR | UART_MCR_RTS);
> > + } else {
> > + /* Drop DTR/RTS */
> > + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_DTR,
> > + 0, port->port_number);
> > + if (err)
> > + goto out;
> > + mxport->mcr_state &= ~(UART_MCR_DTR | UART_MCR_RTS);
> > + }
> > + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_MCR,
> > + mxport->mcr_state,
> > + port->port_number);
> > + if (err)
> > + goto out;
> > + }
>
> This isn't quite right. You want to raise DTR and (possibly) RTS when
> transitioning from B0 (or when !old_termios), and drop DTR and RTS when
> B0 is requested. That is, something like:
>
> if (C_BAUD(tty)) {
> if (old_termios && old_termios->c_cflag & CBAUD == B0)
> /* raise DTR/RTS */
> } else {
> /* drop DTR/RTS */
> }
Which driver is a good example to copy. I looked at a few, and they
all seem to do this differently.
Maybe something you can put onto you long term TODO list if move most
of the logic for this into the generic code, and call the dtr_rts()
function of the driver?
> > +static int mxuport_open(struct tty_struct *tty, struct usb_serial_port *port)
> > +{
> > + struct mxuport_port *mxport = usb_get_serial_port_data(port);
> > + struct usb_serial *serial = port->serial;
> > + int err;
> > +
> > + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_OPEN,
> > + 1, port->port_number);
> > + if (err)
> > + return err;
> > +
> > + /* Send vendor request - set receive host (enable) */
> > + err = mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_RX_HOST_EN,
> > + 1, port->port_number);
>
> Do you want to enable reception before opening?
>
> > +
> > + if (err) {
> > + mxuport_send_ctrl_urb(serial, RQ_VENDOR_SET_OPEN, 0,
> > + port->port_number);
> > +
> > + return err;
> > + }
> > +
> > + /* Initial port termios */
> > + mxuport_set_termios(tty, port, NULL);
> > +
> > + /* Initial private parameters of port */
> > + mxport->msr_state = 0;
>
> Again, why not use RQ_VENDOR_GET_MSR here?
Sorry, i should of put a note in the change log. I looked at this.
The problem is, i've no idea what RQ_VENDOR_GET_MSR returns. The Moxa
vendor driver never uses it. So setting it to 0 seems safer than
wrongly decoding what i get back from the hardware.
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
