Messages in this thread | | | Date | Sun, 21 Sep 2008 15:14:07 +0100 | From | Alan Cox <> | Subject | Re: [PATCH] Winchiphead 340/1: full baud rate and status/control line support |
| |
Some niggles but this is a big improvement to the code.
> static int ch341_set_baudrate(struct usb_device *dev, > + struct tty_struct *tty, > struct ch341_private *priv) > + baud = CH341_BAUDBASE_FACTOR / t1; > + > + if (baud && tty) > + tty_encode_baud_rate(tty, baud, baud);
Can you really get a speed below 1 baud ?
If there is a hangup requested (c_cflag & CBAUD) == B0 then you don't want to overwrite the baud rate but that you handled in set_termios already.
> +static void ch341_close(struct tty_struct *tty, struct usb_serial_port *port, > + struct file *filp) > +{ > + struct ch341_private *priv = usb_get_serial_port_data(port); > + unsigned long flags; > + > + dbg("%s - port %d", __func__, port->number); > + > + /* shutdown our urbs */ > + dbg("%s - shutting down urbs", __func__); > + usb_kill_urb(port->write_urb); > + usb_kill_urb(port->read_urb); > + usb_kill_urb(port->interrupt_in_urb); > + > + if (tty) {
close should never be called with tty == NULL unless there is some kind of internal abuse of it - is the tty check really needed ?
> @@ -242,21 +320,34 @@ static int ch341_open(struct tty_struct *tty, struct usb_serial_port *port, > dbg("ch341_open()"); > > priv->baud_rate = DEFAULT_BAUD_RATE; > - priv->dtr = 1; > - priv->rts = 1; > + > + if (C_CLOCAL(tty))
Open on the other hand is currently called with tty = NULL in the case it is used as a console device (ugly and something I want to fix but it happens for now)
> @@ -270,38 +361,207 @@ static void ch341_set_termios(struct tty_struct *tty, > { > struct ch341_private *priv = usb_get_serial_port_data(port); > unsigned baud_rate; > + unsigned long flags; > > dbg("ch341_set_termios()"); > > + if (!tty) > + return; > +
set_termios should never be called with tty == NULL nowdays so that test should be removable.
> + control = priv->line_control; > + spin_unlock_irqrestore(&priv->lock, flags); > + > + return ch341_set_handshake(port->serial->dev, control); > +}
This could race another set_handshake ? I'm not sure it matters and I'd suggest that stuff gets fixed after this is submitted separately
| |