On Sun, 26 Dec 2010 18:58:46 -0800 (PST)
Tsozik <tsozik@xxxxxxxxx> wrote:
> Please let me know if there's anything else I need to correct before
> it can be rolled out,
I am hardly an expert here, but I looked it up as much as I could,
and this looks like a wrong way to do it:
> @@ -386,27 +396,41 @@ static int mct_u232_get_modem_stat(struc
> /* Translate Control Line states */
> + if (msr & MCT_U232_MSR_DSR) {
> *control_state |= TIOCM_DSR;
> + icount->dsr++;
> + } else {
> *control_state &= ~TIOCM_DSR;
> + }
Unfortunately, Kerrisk's manpages do not seem to offer an authoritative
definition, but the expectations across the code seems that the counter
actually counts the changes. I know that some people talked about
counting "interrupts", but the way it's implemented in trustworthy
drivers suggests something like this:
unsigned int old_ctl_state;
old_ctl_state = *control_state;
if (msr & MCT_U232_MSR_DSR)
*control_state |= TIOCM_DSR;
else
*control_state &= ~TIOCM_DSR;
if (old_ctl_state != *control_state)
icount->dsr++;
...... repeat for all bits
E.g. for DSR going down too, although perhaps I'm not suggesting the
best way to do it. Could you re-implement it like the above and
check that your radiation counter works the same with it and
when driven by a built-in serial port?
Another thing, you should take priv->lock around fetching of
mct_u232_port->icount in mct_u232_ioctl. It seems superfluous,
I know, because the API itself is racy, but just to be nice
and doing the expected thing...
-- Pete
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
