On Thu, Nov 07, 2013 at 03:39:16PM +0100, Andrew Lunn wrote:
> 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.
I'm aware of that. It's a bit of a mess, although it's partly due to the
fact that how to implement it depends on hardware capabilities. And not
all drivers get it right.
Hopefully my comments will suffice as guidance for mxuport, but we could
iterate the flow-control handling separate from the other changes if you
want.
The semantics is fairly clear: Both signals should be cleared when B0 is
requested and raised when transitioning from B0 unless flow is stopped,
in which case only DTR should be raised.
However, if hardware-assisted flow control is enabled this may require
disabling and re-enabling flow control in hardware (simply to be able to
control the RTS signal).
> 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?
And refactoring this is on my TODO list. :)
> > > +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.
Ok, fair enough. Perhaps someone from Moxa that are CC:ed could
enlighten us?
Thanks,
Johan
--
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
