Re: [PATCH 1/1] mct_u232: added _ioctl, _msr_to_icount and _get_icount functions

[Tsozik <tsozik@yahoo.com>]

[-- Attachment #1: Type: text/plain, Size: 3813 bytes --]

Pete,

You're absolutely right. My apology again, I re-checked values of MSR masks used to update icount counters and they all belong to upper MSR nibble, so they indeed track changes (not states). I took out counter increments from mct_u232_msr_to_state function and encapsulated them in the newly defined mct_u232_msr_to_icount function, effectively leaving the previous implementation of mct_u232_msr_to_state function intact. mct_u232_msr_to_icount uses delta nibble to track state changes. RM-60 testing showed the same levels as measured by PDM-2 in close proximity to RM-60. Please let me know if anything else needs to be corrected to roll this out,

Thank you again for your expertise,
 Vadim.

--- On Mon, 12/27/10, Pete Zaitcev <zaitcev@redhat.com> wrote:

> From: Pete Zaitcev <zaitcev@redhat.com>
> Subject: Re: [PATCH 1/1] mct_u232: added _ioctl and _get_icount functions
> To: "Tsozik" <tsozik@yahoo.com>
> Cc: "Greg Kroah-Hartman" <gregkh@suse.de>, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
> Date: Monday, December 27, 2010, 12:08 PM
> On Sun, 26 Dec 2010 22:34:23 -0800
> (PST)
> Tsozik <tsozik@yahoo.com>
> wrote:
> 
> > Usually MSR contains information about both states
> which are raised and
> > states which are changed. It looks like we use upper
> MSR nibble which
> > contains information about 4 states which were raised,
> not lower MSR
> > nibble which contains information about 4 states which
> were changed.
> 
> Sure, I know how typical UART works. Does the MCT device
> deliver the
> change states? If it does and you want to use those, go
> ahead.
> 
> > I also researched implementations under usb/serial and
> saw that all of
> > them are rely on the current states (not change
> states) to increment
> > the respective counter variables.
> 
> Here's drivers/usb/serial/io_edgeport.c:
> 
> static void handle_new_msr(struct edgeport_port *edge_port,
> __u8 newMsr)
> {
>     struct  async_icount *icount;
> 
>     dbg("%s %02x", __func__, newMsr);
> 
>     if (newMsr & (EDGEPORT_MSR_DELTA_CTS
> | EDGEPORT_MSR_DELTA_DSR |
>            
> EDGEPORT_MSR_DELTA_RI | EDGEPORT_MSR_DELTA_CD)) {
>         icount =
> &edge_port->icount;
> 
>         /* update input line
> counters */
>         if (newMsr &
> EDGEPORT_MSR_DELTA_CTS)
>            
> icount->cts++;
>         if (newMsr &
> EDGEPORT_MSR_DELTA_DSR)
>            
> icount->dsr++;
>         if (newMsr &
> EDGEPORT_MSR_DELTA_CD)
>            
> icount->dcd++;
>         if (newMsr &
> EDGEPORT_MSR_DELTA_RI)
>            
> icount->rng++;
>        
> wake_up_interruptible(&edge_port->delta_msr_wait);
>     }
> 
> Looks like it counts changes to me. And in any case, the
> gold standard
> is in drivers/serial/8250.c:
> 
> static unsigned int check_modem_status(struct
> uart_8250_port *up)
> {
>     unsigned int status = serial_in(up,
> UART_MSR);
> 
>     status |= up->msr_saved_flags;
>     up->msr_saved_flags = 0;
>     if (status & UART_MSR_ANY_DELTA
> && up->ier & UART_IER_MSI &&
>         up->port.state != NULL)
> {
>         if (status &
> UART_MSR_TERI)
>            
> up->port.icount.rng++;
>         if (status &
> UART_MSR_DDSR)
>            
> up->port.icount.dsr++;
>         if (status &
> UART_MSR_DDCD)
>            
> uart_handle_dcd_change(&up->port, status &
> UART_MSR_DCD);
>         if (status &
> UART_MSR_DCTS)
>            
> uart_handle_cts_change(&up->port, status &
> UART_MSR_CTS);
> 
>        
> wake_up_interruptible(&up->port.state->port.delta_msr_wait);
>     }
> 
> I only proposed accomplishing the same result by comparing
> states
> in place of relying on hardware change reports like both of
> the
> above do.
> 
> -- Pete
>


      

[-- Attachment #2: mct_u232.c.patch_v3 --]
[-- Type: application/octet-stream, Size: 5699 bytes --]

--- usb/serial/mct_u232.c.orig	2010-12-26 16:54:16.660294924 -0500
+++ usb/serial/mct_u232.c	2010-12-27 15:29:30.895141755 -0500
@@ -78,6 +78,8 @@
 #include <asm/unaligned.h>
 #include <linux/usb.h>
 #include <linux/usb/serial.h>
+#include <linux/serial.h>
+#include <linux/ioctl.h>
 #include "mct_u232.h"
 
 /*
@@ -104,6 +106,10 @@ static void mct_u232_break_ctl(struct tt
 static int  mct_u232_tiocmget(struct tty_struct *tty, struct file *file);
 static int  mct_u232_tiocmset(struct tty_struct *tty, struct file *file,
 			unsigned int set, unsigned int clear);
+static int  mct_u232_ioctl(struct tty_struct *tty, struct file *file,
+			unsigned int cmd, unsigned long arg);
+static int  mct_u232_get_icount(struct tty_struct *tty,
+			struct serial_icounter_struct *icount);
 static void mct_u232_throttle(struct tty_struct *tty);
 static void mct_u232_unthrottle(struct tty_struct *tty);
 
@@ -150,9 +156,10 @@ static struct usb_serial_driver mct_u232
 	.tiocmset =	     mct_u232_tiocmset,
 	.attach =	     mct_u232_startup,
 	.release =	     mct_u232_release,
+	.ioctl =             mct_u232_ioctl,
+	.get_icount =        mct_u232_get_icount,
 };
 
-
 struct mct_u232_private {
 	spinlock_t lock;
 	unsigned int	     control_state; /* Modem Line Setting (TIOCM) */
@@ -160,6 +167,9 @@ struct mct_u232_private {
 	unsigned char	     last_lsr;      /* Line Status Register */
 	unsigned char	     last_msr;      /* Modem Status Register */
 	unsigned int	     rx_flags;      /* Throttling flags */
+	struct async_icount  icount;
+	wait_queue_head_t    msr_wait;	/* for handling sleeping while waiting
+						for msr change to happen */
 };
 
 #define THROTTLED		0x01
@@ -386,6 +396,20 @@ static int mct_u232_get_modem_stat(struc
 	return rc;
 } /* mct_u232_get_modem_stat */
 
+static void mct_u232_msr_to_icount(struct async_icount *icount,
+						unsigned char msr)
+{
+	/* Translate Control Line states */
+	if (msr & MCT_U232_MSR_DDSR)
+		icount->dsr++;
+	if (msr & MCT_U232_MSR_DCTS)
+		icount->cts++;
+	if (msr & MCT_U232_MSR_DRI)
+		icount->rng++;
+	if (msr & MCT_U232_MSR_DCD)
+		icount->dcd++;
+} /* mct_u232_msr_to_icount */
+
 static void mct_u232_msr_to_state(unsigned int *control_state,
 						unsigned char msr)
 {
@@ -422,6 +446,7 @@ static int mct_u232_startup(struct usb_s
 	if (!priv)
 		return -ENOMEM;
 	spin_lock_init(&priv->lock);
+	init_waitqueue_head(&priv->msr_wait);
 	usb_set_serial_port_data(serial->port[0], priv);
 
 	init_waitqueue_head(&serial->port[0]->write_wait);
@@ -621,6 +646,8 @@ static void mct_u232_read_int_callback(s
 	/* Record Control Line states */
 	mct_u232_msr_to_state(&priv->control_state, priv->last_msr);
 
+	mct_u232_msr_to_icount(&priv->icount, priv->last_msr);
+
 #if 0
 	/* Not yet handled. See belkin_sa.c for further information */
 	/* Now to report any errors */
@@ -647,6 +674,7 @@ static void mct_u232_read_int_callback(s
 		tty_kref_put(tty);
 	}
 #endif
+	wake_up_interruptible(&priv->msr_wait);
 	spin_unlock_irqrestore(&priv->lock, flags);
 exit:
 	retval = usb_submit_urb(urb, GFP_ATOMIC);
@@ -826,7 +854,6 @@ static void mct_u232_throttle(struct tty
 	}
 }
 
-
 static void mct_u232_unthrottle(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
@@ -847,6 +874,82 @@ static void mct_u232_unthrottle(struct t
 	}
 }
 
+static int  mct_u232_ioctl(struct tty_struct *tty, struct file *file,
+			unsigned int cmd, unsigned long arg)
+{
+	DEFINE_WAIT(wait);
+	struct usb_serial_port *port = tty->driver_data;
+	struct mct_u232_private *mct_u232_port = usb_get_serial_port_data(port);
+	struct async_icount cnow, cprev;
+	unsigned long flags;
+
+	dbg("%s - port %d, cmd = 0x%x", __func__, port->number, cmd);
+
+	switch (cmd) {
+
+	case TIOCMIWAIT:
+
+		dbg("%s (%d) TIOCMIWAIT", __func__,  port->number);
+
+		spin_lock_irqsave(&mct_u232_port->lock, flags);
+		cprev = mct_u232_port->icount;
+		spin_unlock_irqrestore(&mct_u232_port->lock, flags);
+		for ( ; ; ) {
+			prepare_to_wait(&mct_u232_port->msr_wait,
+					&wait, TASK_INTERRUPTIBLE);
+			schedule();
+			finish_wait(&mct_u232_port->msr_wait, &wait);
+			/* see if a signal did it */
+			if (signal_pending(current))
+				return -ERESTARTSYS;
+			spin_lock_irqsave(&mct_u232_port->lock, flags);
+			cnow = mct_u232_port->icount;
+			spin_unlock_irqrestore(&mct_u232_port->lock, flags);
+			if (cnow.rng == cprev.rng && cnow.dsr == cprev.dsr &&
+			    cnow.dcd == cprev.dcd && cnow.cts == cprev.cts)
+				return -EIO; /* no change => error */
+			if (((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
+			    ((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
+			    ((arg & TIOCM_CD)  && (cnow.dcd != cprev.dcd)) ||
+			    ((arg & TIOCM_CTS) && (cnow.cts != cprev.cts))) {
+				return 0;
+			}
+			cprev = cnow;
+		}
+
+	}
+	return -ENOIOCTLCMD;
+}
+
+static int  mct_u232_get_icount(struct tty_struct *tty,
+			struct serial_icounter_struct *icount)
+{
+	struct usb_serial_port *port = tty->driver_data;
+	struct mct_u232_private *mct_u232_port = usb_get_serial_port_data(port);
+	struct async_icount *ic = &mct_u232_port->icount;
+	unsigned long flags;
+
+	spin_lock_irqsave(&mct_u232_port->lock, flags);
+
+	icount->cts = ic->cts;
+	icount->dsr = ic->dsr;
+	icount->rng = ic->rng;
+	icount->dcd = ic->dcd;
+	icount->rx = ic->rx;
+	icount->tx = ic->tx;
+	icount->frame = ic->frame;
+	icount->overrun = ic->overrun;
+	icount->parity = ic->parity;
+	icount->brk = ic->brk;
+	icount->buf_overrun = ic->buf_overrun;
+
+	spin_unlock_irqrestore(&mct_u232_port->lock, flags);
+
+	dbg("%s (%d) TIOCGICOUNT RX=%d, TX=%d",
+		__func__,  port->number, icount->rx, icount->tx);
+	return 0;
+}
+
 static int __init mct_u232_init(void)
 {
 	int retval;