From: Vadim Tsozik <[email protected]>
Added mct_u232_ioctl function and implemented TIOCMIWAIT and TIOCGICOUNT commands. MCT u232 p9 is one of a few usb to serail adapters which converts USB +/-5v voltage levels to COM +/-15 voltages. So it can also power COM interfaced devices. This makes it very usable for legacy COM interfaced data-acquisition hardware. I tested new implementation with AWARE Electronics RM-60 radiation meter, which sends pulse via RNG COM line whenever new particle is registered.
Patch below is based on linux-2.6.35.10-72.fc14.x86_64.
Signed-off-by: Vadim Tsozik <[email protected]>
---
--- original/mct_u232.c 2010-12-25 21:31:13.744174626 -0500
+++ mct_u232.c 2010-12-25 21:44:57.714640343 -0500
@@ -24,6 +24,12 @@
* Basic tests have been performed with minicom/zmodem transfers and
* modem dialing under Linux 2.4.0-test10 (for me it works fine).
*
+ * 24-Apr-2010 Vadim Tsozik <[email protected]>
+ * - Added implementation of 'TIOCMIWAIT' and 'TIOCGICOUNT' ioctls.
+ * This routines are necessary if you use mct u232 p9 as data
+ * acquisition interface. These routines were tested with RM-60 AWARE
+ * Electronics Radiation Monitor.
+ *
* 04-Nov-2003 Bill Marr <marr at flex dot com>
* - Mimic Windows driver by sending 2 USB 'device request' messages
* following normal 'baud rate change' message. This allows data to be
@@ -78,6 +84,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 +112,8 @@ 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 void mct_u232_throttle(struct tty_struct *tty);
static void mct_u232_unthrottle(struct tty_struct *tty);
@@ -150,6 +160,7 @@ static struct usb_serial_driver mct_u232
.tiocmset = mct_u232_tiocmset,
.attach = mct_u232_startup,
.release = mct_u232_release,
+ .ioctl = mct_u232_ioctl,
};
@@ -160,6 +171,8 @@ 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,27 +399,41 @@ static int mct_u232_get_modem_stat(struc
return rc;
} /* mct_u232_get_modem_stat */
-static void mct_u232_msr_to_state(unsigned int *control_state,
- unsigned char msr)
+static void mct_u232_msr_to_state(struct mct_u232_private *priv)
{
- /* Translate Control Line states */
- if (msr & MCT_U232_MSR_DSR)
- *control_state |= TIOCM_DSR;
- else
- *control_state &= ~TIOCM_DSR;
- if (msr & MCT_U232_MSR_CTS)
- *control_state |= TIOCM_CTS;
- else
- *control_state &= ~TIOCM_CTS;
- if (msr & MCT_U232_MSR_RI)
- *control_state |= TIOCM_RI;
- else
- *control_state &= ~TIOCM_RI;
- if (msr & MCT_U232_MSR_CD)
- *control_state |= TIOCM_CD;
- else
- *control_state &= ~TIOCM_CD;
- dbg("msr_to_state: msr=0x%x ==> state=0x%x", msr, *control_state);
+ unsigned char msr = priv->last_msr;
+ unsigned int *control_state = &priv->control_state;
+ struct async_icount *icount = &priv->icount;
+
+ /* Translate Control Line states */
+ if (msr & MCT_U232_MSR_DSR) {
+ *control_state |= TIOCM_DSR;
+ icount->dsr++;
+ } else {
+ *control_state &= ~TIOCM_DSR;
+ }
+ if (msr & MCT_U232_MSR_CTS) {
+ *control_state |= TIOCM_CTS;
+ icount->cts++;
+ } else {
+ *control_state &= ~TIOCM_CTS;
+ }
+ if (msr & MCT_U232_MSR_RI) {
+ *control_state |= TIOCM_RI;
+ icount->rng++;
+ } else {
+ *control_state &= ~TIOCM_RI;
+ }
+ if (msr & MCT_U232_MSR_CD) {
+ *control_state |= TIOCM_CD;
+ icount->dcd++;
+ } else {
+ *control_state &= ~TIOCM_CD;
+ }
+
+ dbg("msr_to_state: msr=0x%x ==> state=0x%x", msr, *control_state);
+
+ wake_up_interruptible(&priv->msr_wait);
} /* mct_u232_msr_to_state */
/*
@@ -422,6 +449,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);
@@ -498,7 +526,7 @@ static int mct_u232_open(struct tty_str
mct_u232_get_modem_stat(serial, &last_msr);
spin_lock_irqsave(&priv->lock, flags);
priv->last_msr = last_msr;
- mct_u232_msr_to_state(&priv->control_state, priv->last_msr);
+ mct_u232_msr_to_state(priv);
spin_unlock_irqrestore(&priv->lock, flags);
port->read_urb->dev = port->serial->dev;
@@ -616,7 +644,7 @@ static void mct_u232_read_int_callback(s
priv->last_msr = data[MCT_U232_MSR_INDEX];
/* Record Control Line states */
- mct_u232_msr_to_state(&priv->control_state, priv->last_msr);
+ mct_u232_msr_to_state(priv);
#if 0
/* Not yet handled. See belkin_sa.c for further information */
@@ -823,7 +851,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;
@@ -844,6 +871,57 @@ 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;
+
+ dbg("%s - port %d, cmd = 0x%x", __func__, port->number, cmd);
+
+ switch (cmd) {
+
+ case TIOCMIWAIT:
+
+ dbg("%s (%d) TIOCMIWAIT", __func__, port->number);
+
+ cprev = mct_u232_port->icount;
+ 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;
+ cnow = mct_u232_port->icount;
+ 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;
+ }
+ /* NOTREACHED */
+ break;
+
+ case TIOCGICOUNT:
+ dbg("%s - (%d) TIOCGICOUNT RX=%d, TX=%d", __func__,
+ port->number, mct_u232_port->icount.rx, mct_u232_port->icount.tx);
+ if (copy_to_user((void __user *)arg, &mct_u232_port->icount,
+ sizeof(mct_u232_port->icount)))
+ return -EFAULT;
+ return 0;
+ }
+ return -ENOIOCTLCMD;
+}
+
static int __init mct_u232_init(void)
{
int retval;
On Sat, 25 Dec 2010 21:39:39 -0800 (PST)
Tsozik <[email protected]> wrote:
> +++ mct_u232.c 2010-12-25 21:44:57.714640343 -0500
> +static int mct_u232_ioctl(struct tty_struct *tty, struct file *file,
> + unsigned int cmd, unsigned long arg)
> +{
> + case TIOCGICOUNT:
> + dbg("%s - (%d) TIOCGICOUNT RX=%d, TX=%d", __func__,
> + port->number, mct_u232_port->icount.rx, mct_u232_port->icount.tx);
> + if (copy_to_user((void __user *)arg, &mct_u232_port->icount,
> + sizeof(mct_u232_port->icount)))
> + return -EFAULT;
This looks suspicious. Didn't we relocate the machinery for TIOCGICOUNT
into a generic place? Please examine how ->get_icount works before
hand-rolling the ioctl.
-- Pete
On Sat, Dec 25, 2010 at 09:39:39PM -0800, Tsozik wrote:
> From: Vadim Tsozik <[email protected]>
>
> Added mct_u232_ioctl function and implemented TIOCMIWAIT and TIOCGICOUNT commands. MCT u232 p9 is one of a few usb to serail adapters which converts USB +/-5v voltage levels to COM +/-15 voltages. So it can also power COM interfaced devices. This makes it very usable for legacy COM interfaced data-acquisition hardware. I tested new implementation with AWARE Electronics RM-60 radiation meter, which sends pulse via RNG COM line whenever new particle is registered.
>
> Patch below is based on linux-2.6.35.10-72.fc14.x86_64.
Care to make sure this applies on a kernel that we can actually apply
patches to (i.e 2.6.37-rc6?) The .35 kernel was a long time ago.
>
> Signed-off-by: Vadim Tsozik <[email protected]>
>
> ---
> --- original/mct_u232.c 2010-12-25 21:31:13.744174626 -0500
> +++ mct_u232.c 2010-12-25 21:44:57.714640343 -0500
Care to read over Documentation/SubmittingPatches which shows you the
proper patch level to make a patch against?
> @@ -24,6 +24,12 @@
> * Basic tests have been performed with minicom/zmodem transfers and
> * modem dialing under Linux 2.4.0-test10 (for me it works fine).
> *
> + * 24-Apr-2010 Vadim Tsozik <[email protected]>
> + * - Added implementation of 'TIOCMIWAIT' and 'TIOCGICOUNT' ioctls.
> + * This routines are necessary if you use mct u232 p9 as data
> + * acquisition interface. These routines were tested with RM-60 AWARE
> + * Electronics Radiation Monitor.
> + *
No need to ever add an entry here, your changelog information is now
stored in git, not in the files themselves.
> * 04-Nov-2003 Bill Marr <marr at flex dot com>
> * - Mimic Windows driver by sending 2 USB 'device request' messages
> * following normal 'baud rate change' message. This allows data to be
> @@ -78,6 +84,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 +112,8 @@ 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 void mct_u232_throttle(struct tty_struct *tty);
> static void mct_u232_unthrottle(struct tty_struct *tty);
>
> @@ -150,6 +160,7 @@ static struct usb_serial_driver mct_u232
> .tiocmset = mct_u232_tiocmset,
> .attach = mct_u232_startup,
> .release = mct_u232_release,
> + .ioctl = mct_u232_ioctl,
Your email client converted the tabs to spaces, making this patch
unable to apply. Please fix your email client and also run your patch
through the scripts/checkpatch.pl script before sending it to fix up the
formatting errors that are in it.
Care to try again after doing this, and addressing Pete's concerns?
thanks,
greg k-h
Pete,
Many thanks for your comment/concern. I borrowed an TIOCGICOUNT implementation from usb/serial/io_ti.c:
case TIOCGICOUNT:
dbg("%s - (%d) TIOCGICOUNT RX=%d, TX=%d", __func__,
port->number, edge_port->icount.rx, edge_port->icount.tx);
if (copy_to_user((void __user *)arg, &edge_port->icount,
sizeof(edge_port->icount)))
return -EFAULT;
return 0;
}
There are 2 similar TIOCGICOUNT implementations listed in
ark3116.c
io_edgeport.c
io_ti.c
mos7720.c
mos7840.c
ti_usb_3410_5052.c
files under usb/serial/ directory. One based on io_ti.c and another based on io_edgeport.c. I borrowed one from io_ti.c, since it looked more effecient to me. I searched for any mention of get_icount function under linux-2.6.35 and didn't find any file which declared or called this function:
[vtsozik@SR71 linux-2.6.35]$ find . -type f -name '*.[c,h]' | xargs grep get_icount
[vtsozik@SR71 linux-2.6.35]$
I'm wondering if you could give me a bit more information on this. Otherwise I would really prefer to proceed with something that already exists and tested. If by some reason you believe that alternative implementation from io_edgeport.c (please see code snippet below) should be used please let me know. Again I didn't see any reason for extra copy.
case TIOCGICOUNT:
cnow = edge_port->icount;
memset(&icount, 0, sizeof(icount));
icount.cts = cnow.cts;
icount.dsr = cnow.dsr;
icount.rng = cnow.rng;
icount.dcd = cnow.dcd;
icount.rx = cnow.rx;
icount.tx = cnow.tx;
icount.frame = cnow.frame;
icount.overrun = cnow.overrun;
icount.parity = cnow.parity;
icount.brk = cnow.brk;
icount.buf_overrun = cnow.buf_overrun;
dbg("%s (%d) TIOCGICOUNT RX=%d, TX=%d",
__func__, port->number, icount.rx, icount.tx);
if (copy_to_user((void __user *)arg, &icount, sizeof(icount)))
return -EFAULT;
return 0;
}
Thank you in advance,
Vadim.
--- On Sun, 12/26/10, Pete Zaitcev <[email protected]> wrote:
> From: Pete Zaitcev <[email protected]>
> Subject: Re: [PATCH 1/1] mct_u232: IOCTL implementation
> To: "Tsozik" <[email protected]>
> Cc: "Greg Kroah-Hartman" <[email protected]>, [email protected], [email protected]
> Date: Sunday, December 26, 2010, 12:49 PM
> On Sat, 25 Dec 2010 21:39:39 -0800
> (PST)
> Tsozik <[email protected]>
> wrote:
>
> > +++ mct_u232.c? 2010-12-25 21:44:57.714640343
> -0500
> > +static int? mct_u232_ioctl(struct tty_struct
> *tty, struct file *file,
> > +? ? ? ? ? ? ?
> ? ? ? ? ? unsigned int cmd,
> unsigned long arg)
> > +{
> > +? ? ???case TIOCGICOUNT:
> > +? ? ? ? ? ? ?
> ? dbg("%s - (%d) TIOCGICOUNT RX=%d, TX=%d", __func__,
> > +? ? ? ? ? ? ?
> ? ? ? port->number,
> mct_u232_port->icount.rx, mct_u232_port->icount.tx);
> > +? ? ? ? ? ? ?
> ? if (copy_to_user((void __user *)arg,
> &mct_u232_port->icount,
> > +? ? ? ? ? ? ?
> ? ? ? sizeof(mct_u232_port->icount)))
> > +? ? ? ? ? ? ?
> ? ? ? ? ? return -EFAULT;
>
> This looks suspicious. Didn't we relocate the machinery for
> TIOCGICOUNT
> into a generic place? Please examine how ->get_icount
> works before
> hand-rolling the ioctl.
>
> -- Pete
>
Greg,
Many thanks for your reply. I will try to install git and download the latest release candidate. I see that 2.6.37.rc7 is already available. I think I will also hope to find get_icount there to address Pete's concern,
Thank you again,
Vadim.
--- On Sun, 12/26/10, Greg KH <[email protected]> wrote:
> From: Greg KH <[email protected]>
> Subject: Re: [PATCH 1/1] mct_u232: IOCTL implementation
> To: "Tsozik" <[email protected]>
> Cc: [email protected], [email protected]
> Date: Sunday, December 26, 2010, 1:41 PM
> On Sat, Dec 25, 2010 at 09:39:39PM
> -0800, Tsozik wrote:
> > From: Vadim Tsozik <[email protected]>
> >
> > Added mct_u232_ioctl function and implemented
> TIOCMIWAIT and TIOCGICOUNT commands. MCT u232 p9 is one of a
> few usb to serail adapters which converts USB +/-5v voltage
> levels to COM +/-15 voltages. So it can also power COM
> interfaced devices. This makes it very usable for legacy COM
> interfaced data-acquisition hardware. I tested new
> implementation with AWARE Electronics RM-60 radiation meter,
> which sends pulse via RNG COM line whenever new particle is
> registered.
> >
> > Patch below is based on
> linux-2.6.35.10-72.fc14.x86_64.
>
> Care to make sure this applies on a kernel that we can
> actually apply
> patches to (i.e 2.6.37-rc6?)? The .35 kernel was a
> long time ago.
>
> >
> > Signed-off-by: Vadim Tsozik <[email protected]>
> >
> > ---
> > --- original/mct_u232.c 2010-12-25 21:31:13.744174626
> -0500
> > +++ mct_u232.c? 2010-12-25 21:44:57.714640343
> -0500
>
> Care to read over Documentation/SubmittingPatches which
> shows you the
> proper patch level to make a patch against?
>
> > @@ -24,6 +24,12 @@
> >???*???Basic tests have
> been performed with minicom/zmodem transfers and
> >???*???modem dialing
> under Linux 2.4.0-test10 (for me it works fine).
> >???*
> > + * 24-Apr-2010 Vadim Tsozik <[email protected]>
> > + *???- Added implementation of
> 'TIOCMIWAIT' and 'TIOCGICOUNT' ioctls.
> > + *? ???This routines are
> necessary if you use mct u232 p9 as data
> > + *? ???acquisition interface.
> These routines were tested with RM-60 AWARE
> > + *? ???Electronics Radiation
> Monitor.
> > + *
>
> No need to ever add an entry here, your changelog
> information is now
> stored in git,? not in the files themselves.
>
> >???* 04-Nov-2003 Bill Marr <marr at
> flex dot com>
> >???*???- Mimic Windows
> driver by sending 2 USB 'device request' messages
> >???*? ???following
> normal 'baud rate change' message.? This allows data to
> be
> > @@ -78,6 +84,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 +112,8 @@ 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 void mct_u232_throttle(struct tty_struct
> *tty);
> >? static void mct_u232_unthrottle(struct
> tty_struct *tty);
> >
> > @@ -150,6 +160,7 @@ static struct usb_serial_driver
> mct_u232
> >? ? ? ???.tiocmset
> =? ? ? ? ? mct_u232_tiocmset,
> >? ? ? ???.attach =?
> ? ? ? ? ? mct_u232_startup,
> >? ? ? ???.release =?
> ? ? ? ???mct_u232_release,
> > +? ? ???.ioctl =? ?
> ? ? ? ???mct_u232_ioctl,
>
> Your email client converted the tabs to spaces, making this
> patch
> unable to apply.? Please fix your email client and
> also run your patch
> through the scripts/checkpatch.pl script before sending it
> to fix up the
> formatting errors that are in it.
>
> Care to try again after doing this, and addressing Pete's
> concerns?
>
> thanks,
>
> greg k-h
>
On Sun, 26 Dec 2010 11:51:28 -0800 (PST) Tsozik wrote:
> Greg,
>
> Many thanks for your reply. I will try to install git and download the latest release candidate. I see that 2.6.37.rc7 is already available. I think I will also hope to find get_icount there to address Pete's concern,
You can use git if you want to, but it certainly is not required for this.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
desserts: http://www.xenotime.net/linux/recipes/
Randy,
Greg referred to git as to place where change log comments are now checked in. I googled it and found that one can also check out latest kernel source code as opposed to tarball download. However, You're right, looking at http://kernelnewbies.org/UpstreamMerge/SubmittingPatches documentation I don't see how one can use git to check in comments or code directly to git repository,
Thank you,
Vadim.
--- On Sun, 12/26/10, Randy Dunlap <[email protected]> wrote:
> From: Randy Dunlap <[email protected]>
> Subject: Re: [PATCH 1/1] mct_u232: IOCTL implementation
> To: "Tsozik" <[email protected]>
> Cc: "Greg KH" <[email protected]>, [email protected], [email protected]
> Date: Sunday, December 26, 2010, 4:30 PM
> On Sun, 26 Dec 2010 11:51:28 -0800
> (PST) Tsozik wrote:
>
> > Greg,
> >
> > Many thanks for your reply. I will try to install git
> and download the latest release candidate. I see that
> 2.6.37.rc7 is already available. I think I will also hope to
> find get_icount there to address Pete's concern,
>
>
> You can use git if you want to, but it certainly is not
> required for this.
>
> ---
> ~Randy
> *** Remember to use Documentation/SubmitChecklist when
> testing your code ***
> desserts:? http://www.xenotime.net/linux/recipes/
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-usb" in
> the body of a message to [email protected]
> More majordomo info at? http://vger.kernel.org/majordomo-info.html
>
On Sun, 26 Dec 2010, Tsozik wrote:
> Randy,
>
> Greg referred to git as to place where change log comments are now
> checked in. I googled it and found that one can also check out latest
> kernel source code as opposed to tarball download. However, You're
> right, looking at
> http://kernelnewbies.org/UpstreamMerge/SubmittingPatches documentation I
> don't see how one can use git to check in comments or code directly to
> git repository,
>
Only Linus can check code into the final official tree. But, you can use
git to get a copy of the latest Linus tree (and many other trees run by
various maintainers and others). Git is also useful for maintaining your
own local branch with your changes.
The way it usually works is that you create a patch and send it off to
some sub-system maintainer via email patch ('git diff' generated usually)
or via email pull request for your personal repository. The sub-system
maintainer then merges your patch with his tree and, at a later date,
submits his tree (including your patch) to Linus.
Changelog comments are tracked with git. Once your patch is accepted it
will eventually make its way to Linus' tree and your changelog comments in
the patch mail (or git commit message - if you send a pull request) will
eventually turn into a git commit message in the official tree. So there's
no need to add changelog info in the files themselves.
Check http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=shortlog
(or git log after cloning the tree) to see commit messages.
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.
Pete,
My apology, reading Greg's post I realized that 2.6.35 (distributed with Fedora 14) is indeed outdated. I just checked out 2.6.37-rc7 tree and saw references to get_icount function. I will definetely address your concern,
Thank you again,
Vadim.
--- On Sun, 12/26/10, Tsozik <[email protected]> wrote:
> From: Tsozik <[email protected]>
> Subject: Re: [PATCH 1/1] mct_u232: IOCTL implementation
> To: "Pete Zaitcev" <[email protected]>
> Cc: "Greg Kroah-Hartman" <[email protected]>, [email protected], [email protected]
> Date: Sunday, December 26, 2010, 2:41 PM
> Pete,
>
> Many thanks for your comment/concern. I borrowed an
> TIOCGICOUNT implementation from usb/serial/io_ti.c:
>
> ? ? ? ? case TIOCGICOUNT:
> ? ? ? ? ? ? ? ?
> dbg("%s - (%d) TIOCGICOUNT RX=%d, TX=%d", __func__,
> ? ? ? ? ? ? ? ?
> ? ???port->number,
> edge_port->icount.rx, edge_port->icount.tx);
> ? ? ? ? ? ? ? ? if
> (copy_to_user((void __user *)arg,
> &edge_port->icount,
> ? ? ? ? ? ? ? ?
> ? ? ? ? ? ? ? ?
> sizeof(edge_port->icount)))
> ? ? ? ? ? ? ? ?
> ? ? ? ? return -EFAULT;
> ? ? ? ? ? ? ? ?
> return 0;
> ? ? ? ? }
>
> There are 2 similar TIOCGICOUNT implementations listed in
>
> ark3116.c
> io_edgeport.c
> io_ti.c
> mos7720.c
> mos7840.c
> ti_usb_3410_5052.c
>
> files under usb/serial/ directory. One based on io_ti.c and
> another based on io_edgeport.c. I borrowed one from io_ti.c,
> since it looked more effecient to me. I searched for any
> mention of get_icount function under linux-2.6.35 and didn't
> find any file which declared or called this function:
>
> [vtsozik@SR71 linux-2.6.35]$ find . -type f -name '*.[c,h]'
> | xargs grep get_icount
> [vtsozik@SR71 linux-2.6.35]$
>
> I'm wondering if you could give me a bit more information
> on this. Otherwise I would really prefer to proceed with
> something that already exists and tested. If by some reason
> you believe that alternative implementation from
> io_edgeport.c (please see code snippet below) should be used
> please let me know. Again I didn't see any reason for extra
> copy.
>
> ? ? ? ? case TIOCGICOUNT:
> ? ? ? ? ? ? ? ?
> cnow = edge_port->icount;
> ? ? ? ? ? ? ? ?
> memset(&icount, 0, sizeof(icount));
> ? ? ? ? ? ? ? ?
> icount.cts = cnow.cts;
> ? ? ? ? ? ? ? ?
> icount.dsr = cnow.dsr;
> ? ? ? ? ? ? ? ?
> icount.rng = cnow.rng;
> ? ? ? ? ? ? ? ?
> icount.dcd = cnow.dcd;
> ? ? ? ? ? ? ? ?
> icount.rx = cnow.rx;
> ? ? ? ? ? ? ? ?
> icount.tx = cnow.tx;
> ? ? ? ? ? ? ? ?
> icount.frame = cnow.frame;
> ? ? ? ? ? ? ? ?
> icount.overrun = cnow.overrun;
> ? ? ? ? ? ? ? ?
> icount.parity = cnow.parity;
> ? ? ? ? ? ? ? ?
> icount.brk = cnow.brk;
> ? ? ? ? ? ? ? ?
> icount.buf_overrun = cnow.buf_overrun;
>
> ? ? ? ? ? ? ? ?
> dbg("%s (%d) TIOCGICOUNT RX=%d, TX=%d",
> ? ? ? ? ? ? ? ?
> ? ? ? ? ? ? ? ?
> __func__,? port->number, icount.rx, icount.tx);
> ? ? ? ? ? ? ? ? if
> (copy_to_user((void __user *)arg, &icount,
> sizeof(icount)))
> ? ? ? ? ? ? ? ?
> ? ? ? ? return -EFAULT;
> ? ? ? ? ? ? ? ?
> return 0;
> ? ? ? ? }
>
> Thank you in advance,
> Vadim.
>
> --- On Sun, 12/26/10, Pete Zaitcev <[email protected]>
> wrote:
>
> > From: Pete Zaitcev <[email protected]>
> > Subject: Re: [PATCH 1/1] mct_u232: IOCTL
> implementation
> > To: "Tsozik" <[email protected]>
> > Cc: "Greg Kroah-Hartman" <[email protected]>,
> [email protected],
> [email protected]
> > Date: Sunday, December 26, 2010, 12:49 PM
> > On Sat, 25 Dec 2010 21:39:39 -0800
> > (PST)
> > Tsozik <[email protected]>
> > wrote:
> >
> > > +++ mct_u232.c? 2010-12-25 21:44:57.714640343
> > -0500
> > > +static int? mct_u232_ioctl(struct tty_struct
> > *tty, struct file *file,
> > > +? ? ? ? ? ? ?
> > ? ? ? ? ? unsigned int cmd,
> > unsigned long arg)
> > > +{
> > > +? ? ???case TIOCGICOUNT:
> > > +? ? ? ? ? ? ?
> > ? dbg("%s - (%d) TIOCGICOUNT RX=%d, TX=%d",
> __func__,
> > > +? ? ? ? ? ? ?
> > ? ? ? port->number,
> > mct_u232_port->icount.rx,
> mct_u232_port->icount.tx);
> > > +? ? ? ? ? ? ?
> > ? if (copy_to_user((void __user *)arg,
> > &mct_u232_port->icount,
> > > +? ? ? ? ? ? ?
> > ? ? ? sizeof(mct_u232_port->icount)))
> > > +? ? ? ? ? ? ?
> > ? ? ? ? ? return -EFAULT;
> >
> > This looks suspicious. Didn't we relocate the
> machinery for
> > TIOCGICOUNT
> > into a generic place? Please examine how
> ->get_icount
> > works before
> > hand-rolling the ioctl.
> >
> > -- Pete
> >
>
>
>
>
Jesper,
Many thanks for the info. After reading your post I realized that Greg meant that change log comments will be stored in git repository comment section like in any other source control system (e.g. cvs, rcs, ...). So no need to duplicate them in actual file. I will only provide comments in description section of canonical patch format. I'm also wondering if it's OK to provide "git diffs" instead of diffs as a part of patch email?
Thank you in advance,
Vadim.
--- On Sun, 12/26/10, Jesper Juhl <[email protected]> wrote:
> From: Jesper Juhl <[email protected]>
> Subject: Re: [PATCH 1/1] mct_u232: IOCTL implementation
> To: "Tsozik" <[email protected]>
> Cc: "Randy Dunlap" <[email protected]>, "Greg KH" <[email protected]>, [email protected], [email protected]
> Date: Sunday, December 26, 2010, 5:03 PM
> On Sun, 26 Dec 2010, Tsozik wrote:
>
> > Randy,
> >
> > Greg referred to git as to place where change log
> comments are now
> > checked in. I googled it and found that one can also
> check out latest
> > kernel source code as opposed to tarball download.
> However, You're
> > right, looking at
> > http://kernelnewbies.org/UpstreamMerge/SubmittingPatches
> documentation I
> > don't see how one can use git to check in comments or
> code directly to
> > git repository,
> >
>
> Only Linus can check code into the final official tree.
> But, you can use
> git to get a copy of the latest Linus tree (and many other
> trees run by
> various maintainers and others). Git is also useful for
> maintaining your
> own local branch with your changes.
>
> The way it usually works is that you create a patch and
> send it off to
> some sub-system maintainer via email patch ('git diff'
> generated usually)
> or via email pull request for your personal repository. The
> sub-system
> maintainer then merges your patch with his tree and, at a
> later date,
> submits his tree (including your patch) to Linus.
>
> Changelog comments are tracked with git. Once your patch is
> accepted it
> will eventually make its way to Linus' tree and your
> changelog comments in
> the patch mail (or git commit message - if you send a pull
> request) will
> eventually turn into a git commit message in the official
> tree. So there's
> no need to add changelog info in the files themselves.
> Check http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=shortlog
>
> (or git log after cloning the tree) to see commit
> messages.
>
>
> --
> Jesper Juhl <[email protected]>?
> ? ? ? ? ? http://www.chaosbits.net/
> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
> Plain text mails only, please.
>
>
On Sun, 26 Dec 2010 14:14:44 -0800 (PST)
Tsozik <[email protected]> wrote:
> My apology, reading Greg's post I realized that 2.6.35 (distributed with
> Fedora 14) is indeed outdated. I just checked out 2.6.37-rc7 tree and saw
> references to get_icount function. I will definetely address your concern,
Thanks. I figured there was a reason for us to do this (if nothing else,
to get rid of casts with __user). So, you might as well align the code
with the current practice.
Please try to be more careful with spaces and tabs. I didn't want to
focus on such small things right from beginning, but they are considered
important.
Yours,
-- Pete