Re: [PATCH] USB: serial: add nt124 usb to serial driver

From: Johan Hovold
Date: Mon Dec 15 2014 - 04:42:16 EST


On Fri, Dec 12, 2014 at 09:01:03AM -0600, George McCollister wrote:
> On Wed, Dec 10, 2014 at 7:04 AM, Johan Hovold <johan@xxxxxxxxxx> wrote:
> > On Mon, Dec 08, 2014 at 05:24:17PM -0600, George McCollister wrote:

> >> + switch (termios->c_cflag & CSIZE) {
> >
> > C_CSIZE(tty)
> Okay
> >
> >> + case CS5:
> >> + newline.bDataBits = 5;
> >> + break;
> >> + case CS6:
> >> + newline.bDataBits = 6;
> >> + break;
> I should probably just remove CS5 and CS6 since I don't think they
> will work anyway.

Ok. Remember to update the passed termios with the settings that you
actually end up using (i.e. settings from old_termios).

> >> +static int nt124_open(struct tty_struct *tty,
> >> + struct usb_serial_port *port)
> >> +{
> >> + struct nt124_private *priv = usb_get_serial_port_data(port);
> >> + int result = 0;
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&port->lock, flags);
> >> + port->throttled = 0;
> >> + port->throttle_req = 0;
> >> + spin_unlock_irqrestore(&port->lock, flags);
> >> +
> >> + priv->flowctrl = 0;
> >
> > Drop this and keep the current settings.
> Okay
> >
> >> + nt124_set_termios(tty, port, NULL);
> >> + nt124_set_flowctrl(port, priv->flowctrl);
> >
> > Drop this. This is handled by tty and dtr_rts().
> Okay
> >
> >> +
> >> + if (port->bulk_in_size)
> >> + result = usb_serial_generic_submit_read_urbs(port, GFP_KERNEL);
> >
> > Call usb_serial_generic_open() instead (and skip the throttle-flags bit
> > above).
> Okay, so looks like the only thing I will do in this function is call
> nt124_set_termios() followed by usb_serial_generic_open().

Yes, that should do it.

> >> +static struct usb_serial_driver nt124_device = {
> >> + .driver = {
> >> + .owner = THIS_MODULE,
> >> + .name = "nt124",
> >> + },
> >> + .id_table = id_table,
> >> + .num_ports = 1,
> >> + .bulk_in_size = 32,
> >> + .bulk_out_size = 32,
> >
> > Why do you reduce these buffer sizes? They can be bigger than the
> > endpoint size for increased throughput.
> I'll leave them out like most of the other drivers (and retest) unless
> you have another suggestion.

That's perfectly fine, and means that the buffer sizes will match the
endpoint sizes (they will in fact never be smaller than that).

Johan
--
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/