On 9/4/05, Harald Welte <[EMAIL PROTECTED]> wrote:
> On Sun, Sep 04, 2005 at 12:12:18PM +0200, Harald Welte wrote:
> > Hi!
> >
> > Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
> > Smartcard Reader.
>
> Sorry, the patch was missing a "cg-add" of the header file. Please use
> the patch below.
It would be so much nicer if the patch actually was "below" - that is
"inline in the email as opposed to as an attachment". Having to first
save an attachment and then cut'n'paste from it is a pain.
Anyway, a few comments below :
+#define DEBUG(n, x, args...) do { if (pc_debug >= (n))
\
line longer than 80 chars. Please adhere to CodingStyle and keep lines
<80 chars.
There's more than one occourance of this.
+static inline int cmx_waitForBulkOutReady(reader_dev_t *dev)
Why TheStudlyCaps ? Please keep function names lowercase. There are
more instances of this, only pointing out one.
+ register int i;
+ register int iobase = dev->link.io.BasePort1;
Please use only tabs for indentation (line 1 of the above is indented
with spaces).
+ for (i=0; i < POLL_LOOP_COUNT; i++) {
for (i = 0; i < POLL_LOOP_COUNT; i++) {
+ if (rc != 1)
Again spaces used for indentation, please fix all that up to use tabs.
+ unsigned long ulBytesToRead;
lowercase prefered also for variables.
+ for (i=0; i<5; i++) {
for (i = 0; i < 5; i++) {
+ DEBUG(5,"cmx_waitForBulkInReady rc=%.2x\n",rc);
Space after ","s please : DEBUG(5, "cmx_waitForBulkInReady rc=%.2x\n", rc);
+ ulMin = (count < (ulBytesToRead+5))?count:(ulBytesToRead+5);
needs spaces :
ulMin = (count < (ulBytesToRead + 5)) ? count : (ulBytesToRead + 5);
+ reader_dev_t *dev=(reader_dev_t *)filp->private_data;
reader_dev_t *dev = (reader_dev_t *)filp->private_data;
+static int cmx_open (struct inode *inode, struct file *filp)
get rid of the space before the opening paren :
static int cmx_open(struct inode *inode, struct file *filp)
+ for (rc = pcmcia_get_first_tuple(handle, &tuple);
+ rc == CS_SUCCESS;
+ rc = pcmcia_get_next_tuple(handle, &tuple)) {
...
+ if (parse.cftable_entry.io.nwin) {
+ link->io.BasePort1 = parse.cftable_entry.io.win[0].base;
+ link->io.NumPorts1 = parse.cftable_entry.io.win[0].len;
+ link->io.Attributes1 = IO_DATA_PATH_WIDTH_AUTO;
+ if(!(parse.cftable_entry.io.flags & CISTPL_IO_8BIT))
+ link->io.Attributes1 = IO_DATA_PATH_WIDTH_16;
...
+ }
+ }
How about not having to indent so deep by rewriting that as
for (rc = pcmcia_get_first_tuple(handle, &tuple);
rc == CS_SUCCESS;
rc = pcmcia_get_next_tuple(handle, &tuple)) {
...
if (!parse.cftable_entry.io.nwin)
continue;
link->io.BasePort1 = parse.cftable_entry.io.win[0].base;
link->io.NumPorts1 = parse.cftable_entry.io.win[0].len;
link->io.Attributes1 = IO_DATA_PATH_WIDTH_AUTO;
if(!(parse.cftable_entry.io.flags & CISTPL_IO_8BIT))
link->io.Attributes1 = IO_DATA_PATH_WIDTH_16;
...
}
+ link->conf.IntType = 00000002;
more spaces used for indentation. Not going to point out any more of these.
+ cmx_poll_timer.function = &cmx_do_poll;
shouldn't this be
cmx_poll_timer.function = cmx_do_poll;
???
+ int i;
+ DEBUG(3, "-> reader_detach(link=%p\n", link);
please have a blank line between variable declarations and other statements.
--
Jesper Juhl <[EMAIL PROTECTED]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
