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

[Johan Hovold <johan@kernel.org>]

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@kernel.org> 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