> -----Original Message-----
> From: Johan Hovold [mailto:jhov...@gmail.com] On Behalf Of Johan Hovold
> Sent: Friday, April 29, 2016 05:09
> To: Valentin Yakovenkov
> Cc: Konstantin Shkolnyy; linux-usb@vger.kernel.org
> Subject: [EXT] Re: [PATCH] Add DCD line support to CP210x driver
> 
> On Thu, Mar 24, 2016 at 10:53:27AM +0300, Valentin Yakovenkov wrote:
> > Sorry, I missed the branch.
> >
> > Here it is.
> 
> Thanks for the patch. Please resubmit it on a format that can be applied
> (i.e. without quoted text, etc). Remember to include a patch
> revision in the Subject when resubmitting patches (this one should be
> [PATCH v3] or so). Putting a changelog below the cut-off line (---) is
> also recommended.
> 
> > This patch adds DCD line support to CP210x USB serial driver.
> >
> > First it enables CP210x events embedding to incoming URB's by calling:
> > cp210x_set_config_single(port, CP210X_EMBED_EVENTS,
> CP210X_ESCCHAR);
> >
> > Then it parses incoming URB's via custom routine:
> > cp210x_process_read_urb(...)
> > searches for event sequences and handles all of DCD changes calling
> > usb_serial_handle_dcd_change(...)
> >
> > Signed-off-by: Valentin Yakovenkov <yakoven...@niistt.ru>
> > ---
> >  drivers/usb/serial/cp210x.c | 118
> +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 117 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> > index c740592..ef0e8b1 100644
> > --- a/drivers/usb/serial/cp210x.c
> > +++ b/drivers/usb/serial/cp210x.c
> > @@ -47,6 +47,7 @@ static void cp210x_break_ctl(struct tty_struct *, int);
> >  static int cp210x_port_probe(struct usb_serial_port *);
> >  static int cp210x_port_remove(struct usb_serial_port *);
> >  static void cp210x_dtr_rts(struct usb_serial_port *p, int on);
> > +static void cp210x_process_read_urb(struct urb *urb);
> >
> >  static const struct usb_device_id id_table[] = {
> >     { USB_DEVICE(0x045B, 0x0053) }, /* Renesas RX610 RX-Stick */
> > @@ -199,9 +200,21 @@ static const struct usb_device_id id_table[] = {
> >
> >  MODULE_DEVICE_TABLE(usb, id_table);
> >
> > +#define CP210X_ESCCHAR             0x1e
> 
> Move this below the PURGE_ALL definition.
> 
> > +enum cp210x_rx_state {
> > +   CP210X_STATE_IDLE = 0,
> > +   CP210X_STATE_ESC,
> > +   CP210X_STATE_LS0,
> > +   CP210X_STATE_LS1,
> > +   CP210X_STATE_LS,
> > +   CP210X_STATE_MS
> > +};
> > +
> > +
> >  struct cp210x_port_private {
> >     __u8                    bInterfaceNumber;
> >     bool                    has_swapped_line_ctl;
> > +   enum cp210x_rx_state    rx_state;
> 
> This shouldn't be needed (see below).
> 
> >  };
> >
> >  static struct usb_serial_driver cp210x_device = {
> > @@ -222,7 +235,8 @@ static struct usb_serial_driver cp210x_device = {
> >     .tiocmset               = cp210x_tiocmset,
> >     .port_probe             = cp210x_port_probe,
> >     .port_remove            = cp210x_port_remove,
> > -   .dtr_rts                = cp210x_dtr_rts
> > +   .dtr_rts                = cp210x_dtr_rts,
> > +   .process_read_urb       = cp210x_process_read_urb
> >  };
> >
> >  static struct usb_serial_driver * const serial_drivers[] = {
> > @@ -588,6 +602,11 @@ static int cp210x_open(struct tty_struct *tty, struct
> usb_serial_port *port)
> >  {
> >     int result;
> >
> > +   struct usb_serial *serial = port->serial;
> > +   struct cp210x_port_private *spriv = usb_get_serial_data(serial);
> > +
> > +   spriv->rx_state = CP210X_STATE_IDLE;
> > +
> >     result = cp210x_write_u16_reg(port, CP210X_IFC_ENABLE,
> UART_ENABLE);
> >     if (result) {
> >             dev_err(&port->dev, "%s - Unable to enable UART\n",
> __func__);
> > @@ -601,6 +620,15 @@ static int cp210x_open(struct tty_struct *tty, struct
> usb_serial_port *port)
> >     if (tty)
> >             cp210x_change_speed(tty, port, NULL);
> >
> > +   /* Enable events embedding to data stream */
> > +   result = cp210x_write_u16_reg(port, CP210X_EMBED_EVENTS,
> > +
>       CP210X_ESCCHAR);
> > +   if (result) {
> > +           dev_err(&port->dev, "%s - Unable to enable event
> embedding on UART\n",
> > +                           __func__);
> > +           return result;
> > +   }
> > +
> >     return usb_serial_generic_open(tty, port);
> >  }
> >
> > @@ -659,6 +687,94 @@ static bool cp210x_tx_empty(struct
> usb_serial_port *port)
> >     return !count;
> >  }
> >
> > +static void cp210x_process_read_urb(struct urb *urb)
> > +{
> > +   struct usb_serial_port *port = urb->context;
> > +   char *ch = (char *)urb->transfer_buffer;
> > +   char *tbuf = (char *)urb->transfer_buffer;
> > +   int i;
> > +   int tcnt = 0;
> > +   struct usb_serial *serial = port->serial;
> > +   struct cp210x_port_private *spriv = usb_get_serial_data(serial);
> > +
> > +   if (!urb->actual_length)
> > +           return;
> > +
> > +   /* Process escape chars */
> > +   for (i = 0; i < urb->actual_length; i++) {
> > +           char c = ch[i];
> > +
> > +           switch (spriv->rx_state) {
> > +           case CP210X_STATE_IDLE:
> > +                   if (c == CP210X_ESCCHAR)
> > +                           spriv->rx_state = CP210X_STATE_ESC;
> > +                   else
> > +                           tbuf[tcnt++] = c;
> > +                   break;
> > +
> > +           case CP210X_STATE_ESC:
> > +                   if (c == 0x01)
> > +                           spriv->rx_state = CP210X_STATE_LS0;
> > +                   else if (c == 0x02)
> > +                           spriv->rx_state = CP210X_STATE_LS;
> > +                   else if (c == 0x03)
> > +                           spriv->rx_state = CP210X_STATE_MS;
> > +                   else {
> > +                           tbuf[tcnt++] = (c == 0x00) ? CP210X_ESCCHAR
> : c;
> > +                           spriv->rx_state = CP210X_STATE_IDLE;
> > +                   }
> > +                   break;
> > +
> > +           case CP210X_STATE_LS0:
> > +                   spriv->rx_state = CP210X_STATE_LS1;
> > +                   break;
> > +
> > +           case CP210X_STATE_LS1:
> > +                   tbuf[tcnt++] = c;
> > +                   spriv->rx_state = CP210X_STATE_IDLE;
> > +                   break;
> > +
> > +           case CP210X_STATE_LS:
> > +                   spriv->rx_state = CP210X_STATE_IDLE;
> > +                   break;
> > +
> > +           case CP210X_STATE_MS:
> > +                   if (c & 0x08) {
> > +                           /* DCD change event */
> > +                           struct tty_struct *tty;
> > +
> > +                           port->icount.dcd++;
> > +                           tty = tty_port_tty_get(&port->port);
> > +                           if (tty)
> > +                                   usb_serial_handle_dcd_change(port,
> tty,
> > +                                                   c & 0x80);
> > +                           tty_kref_put(tty);
> > +
> > +                   }
> > +                   wake_up_interruptible(&port-
> >port.delta_msr_wait);
> > +                   spriv->rx_state = CP210X_STATE_IDLE;
> > +                   break;
> > +           }
> > +   }
> 
> Instead of maintaining this state machine (which should not need to be
> global, or can the events really be split over multiple bulk
> transfers?), just scan the buffer for the escape character and if found,
Do you know for sure that they can't be split over multiple transfers, in 
today's and future chips?

> process the symbols one by one for that buffer. If line errors were
> detected, this should also be reported to user space (e.g. using
> tty_insert_flip_char and a non TTY_NORMAL flag), etc.
> 
> Thanks,
> Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to