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@xxxxxxxxxx> wrote: > From: Pete Zaitcev <zaitcev@xxxxxxxxxx> > Subject: Re: [PATCH 1/1] mct_u232: added _ioctl and _get_icount functions > To: "Tsozik" <tsozik@xxxxxxxxx> > Cc: "Greg Kroah-Hartman" <gregkh@xxxxxxx>, linux-usb@xxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx > Date: Monday, December 27, 2010, 12:08 PM > On Sun, 26 Dec 2010 22:34:23 -0800 > (PST) > Tsozik <tsozik@xxxxxxxxx> > 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:
mct_u232.c.patch_v3
Description: Binary data