Re: [PATCH] Winchiphead 340/1: full baud rate and status/controlline support

[Alan Cox]

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



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/