[gpsd-dev] Fw: [EXT] Re: [PATCH] Add DCD line support to CP210x driver

[gem]

Yo All!

Interesting message from Linux-USB.  We have had reports of PPS on DCD
not working for some USB/Serial adapters.  It seems that in at least
one chip driver (CP210x) this function is broken, and here is the patch
to  fix it.  Let's hope it gets in the kernel soon.

RGDS
GARY

--------------------------------------

Begin forwarded message:

Date: Fri, 29 Apr 2016 16:37:49 +0000
From: Konstantin Shkolnyy <address@hidden>
To: Johan Hovold <address@hidden>,        Valentin Yakovenkov
<address@hidden> Cc: "address@hidden"
<address@hidden> Subject: RE: [EXT] Re: [PATCH] Add DCD line
support to CP210x driver


> -----Original Message-----
> From: Johan Hovold [mailto:address@hidden On Behalf Of Johan
> Hovold Sent: Friday, April 29, 2016 05:09
> To: Valentin Yakovenkov
> Cc: Konstantin Shkolnyy; address@hidden
> 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 <address@hidden>
> > ---
> >  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 address@hidden
More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
RGDS
GARY
---------------------------------------------------------------------------
Gary E. Miller Rellim 109 NW Wilmington Ave., Suite E, Bend, OR 97701
        address@hidden  Tel:+1(541)382-8588