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

[Harald Welte <laforge@gnumonks.org>]

On Sun, Sep 04, 2005 at 12:27:20AM +0200, Jesper Juhl wrote:
> On 9/4/05, Harald Welte <laforge@gnumonks.org> 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.

This is a neverending discussion.  A number of kernel develpoers I know
prefer attachements these days.  It's a matter of your email client, and
why it doesn't just append a "plaintext" attachment at the bottom of
your mail and rather display you the "save as" icon/button/whatever.

> 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.

The line is _not_ longer than 80 chars, at least not if you remove the
"+" from the beginning of the patch and you hve 8 chars wide

> There's more than one occourance of this.

there was one occurrence in the file which I have missed, thanks.

> +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.

Yes, there are.  My initial idea was to diverge as little as possible
from the original vendor driver, making it easy to pull in changes from
their tree in the future, should it be neccessarry.

However, it has diverged that much now, it doesn't really matter whether
I also rename the functions, too.  Please stay tuned for the next
revision of the patch.

> Please use only tabs for indentation (line 1 of the above is indented
> with spaces).

thanks, should have catched all of them now.

> lowercase prefered also for variables.

done

> Space after ","s please : DEBUG(5, "cmx_waitForBulkInReady rc=%.2x\n", rc);

done

> get rid of the space before the opening paren : 
> static int cmx_open(struct inode *inode, struct file *filp)

done

> How about not having to indent so deep by rewriting that as

done

> +	cmx_poll_timer.function = &cmx_do_poll;
> 
> shouldn't this be 
> 	 cmx_poll_timer.function = cmx_do_poll;
> ???

I don't really know what difference it would make.  My understanding of
'c' is that both cases would take the address of the function.

My personal taste is rather to explicitly indicate this with an '&' than
let the compiler implicitly take the address.

-- 
- Harald Welte <laforge@gnumonks.org>          	        http://gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
                                                  (ETSI EN 300 175-7 Ch. A6)

Attachment: pgpaU9bRnWkWI.pgp
Description: PGP signature