Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver

[Jesper Juhl]

On 9/4/05, Harald Welte <laforge@xxxxxxxxxxxx> 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 <jesper.juhl@xxxxxxxxx>
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/