Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
On Tue, Sep 06, 2005 at 09:15:10AM -0700, Roland Dreier wrote: > Harald> Obviously, if HZ would ever go below 100, the code above > Harald> would provide some problems. I'm not sure what the future > Harald> plans with HZ are, but I'll add an #error statement in > Harald> case HZ goes smaller than that. > > It might be simpler just to define it to msecs_to_jiffies(10). That's what I did in the last version that was posted to lkml ;) -- - Harald Welte <[EMAIL PROTECTED]> http://gnumonks.org/ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6) pgp7m8zaAY3Vq.pgp Description: PGP signature
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
Harald> Obviously, if HZ would ever go below 100, the code above Harald> would provide some problems. I'm not sure what the future Harald> plans with HZ are, but I'll add an #error statement in Harald> case HZ goes smaller than that. It might be simpler just to define it to msecs_to_jiffies(10). - R. - 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/
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
Harald Obviously, if HZ would ever go below 100, the code above Harald would provide some problems. I'm not sure what the future Harald plans with HZ are, but I'll add an #error statement in Harald case HZ goes smaller than that. It might be simpler just to define it to msecs_to_jiffies(10). - R. - 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/
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
On Tue, Sep 06, 2005 at 09:15:10AM -0700, Roland Dreier wrote: Harald Obviously, if HZ would ever go below 100, the code above Harald would provide some problems. I'm not sure what the future Harald plans with HZ are, but I'll add an #error statement in Harald case HZ goes smaller than that. It might be simpler just to define it to msecs_to_jiffies(10). That's what I did in the last version that was posted to lkml ;) -- - Harald Welte [EMAIL PROTECTED] http://gnumonks.org/ Privacy in residential applications is a desirable marketing option. (ETSI EN 300 175-7 Ch. A6) pgp7m8zaAY3Vq.pgp Description: PGP signature
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
On Sat, Sep 03, 2005 at 04:27:00PM -0500, Chase Venters wrote: > > Below you can find a driver for the Omnikey CardMan 4040 PCMCIA > > Smartcard Reader. > > Someone correct me if I'm wrong, but wouldn't these #defines be a problem > with > the new HZ flexibility: > > #define CCID_DRIVER_BULK_DEFAULT_TIMEOUT(150*HZ) > #define CCID_DRIVER_ASYNC_POWERUP_TIMEOUT (35*HZ) > #define CCID_DRIVER_MINIMUM_TIMEOUT (3*HZ) The defines above certainly have no problems. They want to wait for multiples of seconds. > /* how often to poll for fifo status change */ > #define POLL_PERIOD (HZ/100) > > In particular, 2.6.13 allows a HZ of 100, which would define POLL_PERIOD to > 0. > Your later calls to mod_timer would be setting cmx_poll_timer to the current > value of jiffies. 100/100 == 1. As far as my limited math skills go, only 0 divided by something can give a result of zero ;) So yes, the code would poll every 1/100th of a second, even with HZ=100. Obviously, if HZ would ever go below 100, the code above would provide some problems. I'm not sure what the future plans with HZ are, but I'll add an #error statement in case HZ goes smaller than that. > Also, you've got a typo in the comments: thanks. -- - Harald Welte <[EMAIL PROTECTED]> http://gnumonks.org/ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6) pgpb5i3Zcc8TV.pgp Description: PGP signature
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
On Sun, Sep 04, 2005 at 12:27:20AM +0200, Jesper Juhl wrote:
> 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.
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 = _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 <[EMAIL PROTECTED]> http://gnumonks.org/
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
pgp0GtfIPIiSQ.pgp
Description: PGP signature
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
On Sun, Sep 04, 2005 at 02:58:27PM +0200, Ingo Oeser wrote: > just use > return nonseekable_open(inode, filp); > > to really disable any file pointer positioning (e.g. pwrite/pread too). > > Addtionally cmx_llseek() is implement already as "no_llseek()" > by the VFS, so you delete it from the driver an use no_llseek() from > the VFS instead. great, thanks, I've merged your suggested changes into my local tree. Stay tuned for a re-submit later today. -- - Harald Welte <[EMAIL PROTECTED]> http://gnumonks.org/ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6) pgpD9aDqrpuRh.pgp Description: PGP signature
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
On Sun, Sep 04, 2005 at 02:58:27PM +0200, Ingo Oeser wrote: just use return nonseekable_open(inode, filp); to really disable any file pointer positioning (e.g. pwrite/pread too). Addtionally cmx_llseek() is implement already as no_llseek() by the VFS, so you delete it from the driver an use no_llseek() from the VFS instead. great, thanks, I've merged your suggested changes into my local tree. Stay tuned for a re-submit later today. -- - Harald Welte [EMAIL PROTECTED] http://gnumonks.org/ Privacy in residential applications is a desirable marketing option. (ETSI EN 300 175-7 Ch. A6) pgpD9aDqrpuRh.pgp Description: PGP signature
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
On Sun, Sep 04, 2005 at 12:27:20AM +0200, Jesper Juhl wrote:
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.
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 [EMAIL PROTECTED] http://gnumonks.org/
Privacy in residential applications is a desirable marketing option.
(ETSI EN 300 175-7 Ch. A6)
pgp0GtfIPIiSQ.pgp
Description: PGP signature
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
On Sat, Sep 03, 2005 at 04:27:00PM -0500, Chase Venters wrote: Below you can find a driver for the Omnikey CardMan 4040 PCMCIA Smartcard Reader. Someone correct me if I'm wrong, but wouldn't these #defines be a problem with the new HZ flexibility: #define CCID_DRIVER_BULK_DEFAULT_TIMEOUT(150*HZ) #define CCID_DRIVER_ASYNC_POWERUP_TIMEOUT (35*HZ) #define CCID_DRIVER_MINIMUM_TIMEOUT (3*HZ) The defines above certainly have no problems. They want to wait for multiples of seconds. /* how often to poll for fifo status change */ #define POLL_PERIOD (HZ/100) In particular, 2.6.13 allows a HZ of 100, which would define POLL_PERIOD to 0. Your later calls to mod_timer would be setting cmx_poll_timer to the current value of jiffies. 100/100 == 1. As far as my limited math skills go, only 0 divided by something can give a result of zero ;) So yes, the code would poll every 1/100th of a second, even with HZ=100. Obviously, if HZ would ever go below 100, the code above would provide some problems. I'm not sure what the future plans with HZ are, but I'll add an #error statement in case HZ goes smaller than that. Also, you've got a typo in the comments: thanks. -- - Harald Welte [EMAIL PROTECTED] http://gnumonks.org/ Privacy in residential applications is a desirable marketing option. (ETSI EN 300 175-7 Ch. A6) pgpb5i3Zcc8TV.pgp Description: PGP signature
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
On 9/4/05, Horst von Brand <[EMAIL PROTECTED]> wrote: > Jesper Juhl <[EMAIL PROTECTED]> wrote: > > 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 : > > [...] > > > + unsigned long ulBytesToRead; > > > > > > lowercase prefered also for variables. > > Also, "encoding" the type (ul) into the variable name is nonsense. > Agreed, and it's even mentioned in CodingStyle (ok, it talks about functions, but the same goes for variables): ... Encoding the type of a function into the name (so-called Hungarian notation) is brain damaged - the compiler knows the types anyway and can check those, and it only confuses the programmer. No wonder MicroSoft makes buggy programs. LOCAL variable names should be short, and to the point. If you have some random integer loop counter, it should probably be called "i". Calling it "loop_counter" is non-productive, if there is no chance of it being mis-understood. Similarly, "tmp" can be just about any type of variable that is used to hold a temporary value. ... -- 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/
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
Jesper Juhl <[EMAIL PROTECTED]> wrote: > 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 : [...] > + unsigned long ulBytesToRead; > > > lowercase prefered also for variables. Also, "encoding" the type (ul) into the variable name is nonsense. [...] > + ulMin = (count < (ulBytesToRead+5))?count:(ulBytesToRead+5); Again. -- Dr. Horst H. von Brand User #22616 counter.li.org Departamento de Informatica Fono: +56 32 654431 Universidad Tecnica Federico Santa Maria +56 32 654239 Casilla 110-V, Valparaiso, ChileFax: +56 32 797513 - 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/
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
On Sunday 04 September 2005 12:12, Harald Welte wrote: > cmx_llseek just use return nonseekable_open(inode, filp); as your last statement in cmx_open() instead of return 0; to really disable any file pointer positioning (e.g. pwrite/pread too). Addtionally cmx_llseek() is implement already as "no_llseek()" by the VFS, so you delete it from the driver an use no_llseek() from the VFS instead. Regards Ingo Oeser pgpfDmvKLYTKl.pgp Description: PGP signature
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
Hi Nish, thanks for your comments. On Sat, Sep 03, 2005 at 03:13:43PM -0700, Nish Aravamudan wrote: > On 9/3/05, Chase Venters <[EMAIL PROTECTED]> wrote: > > > Below you can find a driver for the Omnikey CardMan 4040 PCMCIA > > > Smartcard Reader. > > > > #define CCID_DRIVER_BULK_DEFAULT_TIMEOUT(150*HZ) > > These are all fine. Although I am a bit suspicious of 150 second > timeouts; but if that is the hardware... That's a definition from the original vendor-supplied driver. Unfortunately there's no hardware documentation, so I can't verify it. But generally speaking, serial smart cards can really be slow, so I think it could make sense. > > /* how often to poll for fifo status change */ > > #define POLL_PERIOD (HZ/100) > > This needs to be msecs_to_jiffies(10), please. thanks, changed in my local tree now. > Of bigger concern to me is the use of the sleep_on() family of > functions, all of which are deprecated. Ok, I'm working on replacing the respective code with wait_event_interruptible_timeout(). -- - Harald Welte <[EMAIL PROTECTED]> http://gnumonks.org/ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6) pgpjkUFyNclXx.pgp Description: PGP signature
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
Thanks for your comments, Alexey. I've now incorprorated all of the requested changes and am testing the driver. If everything is still fine, I'll repost later today. -- - Harald Welte <[EMAIL PROTECTED]> http://gnumonks.org/ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6) pgpCUESmjveeW.pgp Description: PGP signature
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
Thanks for your comments, Alexey. I've now incorprorated all of the requested changes and am testing the driver. If everything is still fine, I'll repost later today. -- - Harald Welte [EMAIL PROTECTED] http://gnumonks.org/ Privacy in residential applications is a desirable marketing option. (ETSI EN 300 175-7 Ch. A6) pgpCUESmjveeW.pgp Description: PGP signature
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
Hi Nish, thanks for your comments. On Sat, Sep 03, 2005 at 03:13:43PM -0700, Nish Aravamudan wrote: On 9/3/05, Chase Venters [EMAIL PROTECTED] wrote: Below you can find a driver for the Omnikey CardMan 4040 PCMCIA Smartcard Reader. #define CCID_DRIVER_BULK_DEFAULT_TIMEOUT(150*HZ) These are all fine. Although I am a bit suspicious of 150 second timeouts; but if that is the hardware... That's a definition from the original vendor-supplied driver. Unfortunately there's no hardware documentation, so I can't verify it. But generally speaking, serial smart cards can really be slow, so I think it could make sense. /* how often to poll for fifo status change */ #define POLL_PERIOD (HZ/100) This needs to be msecs_to_jiffies(10), please. thanks, changed in my local tree now. Of bigger concern to me is the use of the sleep_on() family of functions, all of which are deprecated. Ok, I'm working on replacing the respective code with wait_event_interruptible_timeout(). -- - Harald Welte [EMAIL PROTECTED] http://gnumonks.org/ Privacy in residential applications is a desirable marketing option. (ETSI EN 300 175-7 Ch. A6) pgpjkUFyNclXx.pgp Description: PGP signature
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
On Sunday 04 September 2005 12:12, Harald Welte wrote: cmx_llseek just use return nonseekable_open(inode, filp); as your last statement in cmx_open() instead of return 0; to really disable any file pointer positioning (e.g. pwrite/pread too). Addtionally cmx_llseek() is implement already as no_llseek() by the VFS, so you delete it from the driver an use no_llseek() from the VFS instead. Regards Ingo Oeser pgpfDmvKLYTKl.pgp Description: PGP signature
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
Jesper Juhl [EMAIL PROTECTED] wrote: 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 : [...] + unsigned long ulBytesToRead; lowercase prefered also for variables. Also, encoding the type (ul) into the variable name is nonsense. [...] + ulMin = (count (ulBytesToRead+5))?count:(ulBytesToRead+5); Again. -- Dr. Horst H. von Brand User #22616 counter.li.org Departamento de Informatica Fono: +56 32 654431 Universidad Tecnica Federico Santa Maria +56 32 654239 Casilla 110-V, Valparaiso, ChileFax: +56 32 797513 - 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/
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
On 9/4/05, Horst von Brand [EMAIL PROTECTED] wrote: Jesper Juhl [EMAIL PROTECTED] wrote: 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 : [...] + unsigned long ulBytesToRead; lowercase prefered also for variables. Also, encoding the type (ul) into the variable name is nonsense. Agreed, and it's even mentioned in CodingStyle (ok, it talks about functions, but the same goes for variables): ... Encoding the type of a function into the name (so-called Hungarian notation) is brain damaged - the compiler knows the types anyway and can check those, and it only confuses the programmer. No wonder MicroSoft makes buggy programs. LOCAL variable names should be short, and to the point. If you have some random integer loop counter, it should probably be called i. Calling it loop_counter is non-productive, if there is no chance of it being mis-understood. Similarly, tmp can be just about any type of variable that is used to hold a temporary value. ... -- 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/
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
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, );
+rc == CS_SUCCESS;
+rc = pcmcia_get_next_tuple(handle, )) {
...
+ 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, );
rc == CS_SUCCESS;
rc = pcmcia_get_next_tuple(handle, )) {
...
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 = 0002;
more spaces used for indentation. Not going to point out any more of these.
+ cmx_poll_timer.function = _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/
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
> Um, 100/100 = 1, not 0? Oh my... it's been a long day. Regards, Chase Venters - 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/
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
On 9/3/05, Chase Venters <[EMAIL PROTECTED]> wrote: > > Below you can find a driver for the Omnikey CardMan 4040 PCMCIA > > Smartcard Reader. > > Someone correct me if I'm wrong, but wouldn't these #defines be a problem > with the new HZ flexibility: > > #define CCID_DRIVER_BULK_DEFAULT_TIMEOUT(150*HZ) > #define CCID_DRIVER_ASYNC_POWERUP_TIMEOUT (35*HZ) > #define CCID_DRIVER_MINIMUM_TIMEOUT (3*HZ) > #define READ_WRITE_BUFFER_SIZE 512 > #define POLL_LOOP_COUNT 1000 These are all fine. Although I am a bit suspicious of 150 second timeouts; but if that is the hardware... > /* how often to poll for fifo status change */ > #define POLL_PERIOD (HZ/100) This needs to be msecs_to_jiffies(10), please. > In particular, 2.6.13 allows a HZ of 100, which would define POLL_PERIOD to 0. Um, 100/100 = 1, not 0? > Your later calls to mod_timer would be setting cmx_poll_timer to the current > value of jiffies. Which is technically ok, because HZ=100, a jiffies + 0 or jiffies + 1 timeout request will both result in the soft-timer being expired at the *next* timer interrupt. Regardless, you're right, and msecs_to_jiffies() will cover it. > Also, you've got a typo in the comments: > > * - adhere to linux kenrel coding style and policies > > Forgive me if I'm way off - I'm just now getting my feet wet in kernel > development. Just making comments based on what I (think) I know at this > point. Of bigger concern to me is the use of the sleep_on() family of functions, all of which are deprecated. Thanks, Nish - 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/
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
On Sun, Sep 04, 2005 at 12:12:18PM +0200, Harald Welte wrote:
> Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
> Smartcard Reader.
> --- /dev/null
> +++ b/drivers/char/pcmcia/cm4040_cs.c
> +#include
Not needed.
> +static volatile char *version =
Can we lose all volatile and register keywords?
> +typedef struct reader_dev_t {
> + dev_link_t link;
> + dev_node_t node;
> + wait_queue_head_t devq;
> +
> + wait_queue_head_t poll_wait;
> + wait_queue_head_t read_wait;
> + wait_queue_head_t write_wait;
> +
> + unsigned intbuffer_status;
> +
> + unsigned intfTimerExpired;
> + struct timer_list timer;
> + unsigned long timeout;
> + unsigned char sBuf[READ_WRITE_BUFFER_SIZE];
> + unsigned char rBuf[READ_WRITE_BUFFER_SIZE];
> + struct task_struct *owner;
> +} reader_dev_t;
And typedefs too.
struct reader_dev {
};
> +static ssize_t cmx_read(struct file *filp,char *buf,size_t count,loff_t
> *ppos)
char __user *buf
> + ulBytesToRead = 5 +
> + (0x00FF&((char)dev->rBuf[1])) +
> + (0xFF00&((char)dev->rBuf[2] << 8)) +
> + (0x00FF&((char)dev->rBuf[3] << 16)) +
> + (0xFF00&((char)dev->rBuf[4] << 24));
ulBytesToRead = 5 + le32_to_cpu(*(__le32 *)>rBuf[1]);
> + ulMin = (count < (ulBytesToRead+5))?count:(ulBytesToRead+5);
ulMin = min(count, ulBytesToRead + 5);
> + copy_to_user(buf, dev->rBuf, ulMin);
Can fail.
> +static ssize_t cmx_write(struct file *filp,const char *buf,size_t count,
const char __user *buf
> + loff_t *ppos)
> + copy_from_user(dev->sBuf, buf, uiBytesToWrite);
Can fail.
> +static int cmx_ioctl(struct inode *inode,struct file *filp,unsigned int cmd,
> + unsigned long arg)
> +{
> + dev_link_t *link;
> + int rc, size;
> +
> + link=dev_table[MINOR(inode->i_rdev)];
> + if (!(DEV_OK(link))) {
> + DEBUG(4, "DEV_OK false\n");
> + return -ENODEV;
> + }
> + if (_IOC_TYPE(cmd)!=CM_IOC_MAGIC) {
> + DEBUG(4,"ioctype mismatch\n");
> + return -EINVAL;
> + }
> + if (_IOC_NR(cmd)>CM_IOC_MAXNR) {
> + DEBUG(4,"iocnr mismatch\n");
> + return -EINVAL;
> + }
> + size = _IOC_SIZE(cmd);
> + rc = 0;
> + DEBUG(4,"iocdir=%.4x iocr=%.4x iocw=%.4x iocsize=%d cmd=%.4x\n",
> + _IOC_DIR(cmd),_IOC_READ,_IOC_WRITE,size,cmd);
> +
> + if (_IOC_DIR(cmd)&_IOC_READ) {
> + if (!access_ok(VERIFY_WRITE, (void *)arg, size))
> + return -EFAULT;
> + }
> + if (_IOC_DIR(cmd)&_IOC_WRITE) {
> + if (!access_ok(VERIFY_READ, (void *)arg, size))
> + return -EFAULT;
> + }
> +
> + return rc;
> +}
Whoo, empty ioctl handler.
> +static void reader_release(u_long arg)
> + link = (dev_link_t *)arg;
You do
reader_release((unsigned long)link);
somewhere above and below.
> +static void reader_detach_by_devno(int devno,dev_link_t *link)
> + reader_release((u_long)link);
Like this.
-
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/
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
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.
--
- Harald Welte <[EMAIL PROTECTED]> http://gnumonks.org/
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
Add Omnikey CardMan 4040 Driver
Signed-off-by: Harald Welte <[EMAIL PROTECTED]>
---
commit 0e760a5785ebb83b932d104679cc2b2b4070b1c1
tree 46d71d0c6b41b441a1a8d725b741450f1334296e
parent c4ab879b6ef599bf88d19b9b145878ef73400ce7
author Harald Welte <[EMAIL PROTECTED]> So, 04 Sep 2005 13:05:44 +0200
committer Harald Welte <[EMAIL PROTECTED]> So, 04 Sep 2005 13:05:44 +0200
MAINTAINERS |5
drivers/char/pcmcia/Kconfig | 13 +
drivers/char/pcmcia/Makefile|1
drivers/char/pcmcia/cm4040_cs.c | 900 +++
drivers/char/pcmcia/cm4040_cs.h | 52 ++
5 files changed, 971 insertions(+), 0 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1737,6 +1737,11 @@ L: [EMAIL PROTECTED]
W: http://www.linuxtr.net
S: Maintained
+OMNIKEY CARDMAN 4040 DRIVER
+P: Harald Welte
+M: [EMAIL PROTECTED]
+S: Maintained
+
ONSTREAM SCSI TAPE DRIVER
P: Willem Riede
M: [EMAIL PROTECTED]
diff --git a/drivers/char/pcmcia/Kconfig b/drivers/char/pcmcia/Kconfig
--- a/drivers/char/pcmcia/Kconfig
+++ b/drivers/char/pcmcia/Kconfig
@@ -18,5 +18,18 @@ config SYNCLINK_CS
The module will be called synclinkmp. If you want to do that, say M
here.
+config CARDMAN_4040
+ tristate "Omnikey CardMan 4040 support"
+ depends on PCMCIA
+ help
+ Enable support for the Omnikey CardMan 4040 PCMCIA Smartcard
+ reader.
+
+ This card is basically a USB CCID device connected to a FIFO
+ in I/O space. To use the kernel driver, you will need either the
+ PC/SC ifdhandler provided from the Omnikey homepage
+ (http://www.omnikey.com/), or a current development version of OpenCT
+ (http://www.opensc.org/).
+
endmenu
diff --git a/drivers/char/pcmcia/Makefile b/drivers/char/pcmcia/Makefile
--- a/drivers/char/pcmcia/Makefile
+++ b/drivers/char/pcmcia/Makefile
@@ -5,3 +5,4 @@
#
obj-$(CONFIG_SYNCLINK_CS) += synclink_cs.o
+obj-$(CONFIG_CARDMAN_4040) += cm4040_cs.o
diff --git a/drivers/char/pcmcia/cm4040_cs.c b/drivers/char/pcmcia/cm4040_cs.c
new file mode 100644
--- /dev/null
+++ b/drivers/char/pcmcia/cm4040_cs.c
@@ -0,0 +1,900 @@
+ /*
+ * A driver for the Omnikey PCMCIA smartcard reader CardMan 4040
+ *
+ * (c) 2000-2004 Omnikey AG (http://www.omnikey.com/)
+ *
+ * (C) 2005 Harald Welte <[EMAIL PROTECTED]>
+ * - add support for poll()
+ * - driver cleanup
+ * - add waitqueues
+ * - adhere to linux kenrel coding style and policies
+ * - support 2.6.13 "new style" pcmcia interface
+ *
+ * All rights reserved, Dual BSD/GPL Licensed.
+ */
+
+/* #define PCMCIA_DEBUG 6 */
+
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "cm4040_cs.h"
+
+static atomic_t cmx_num_devices_open;
+
+#ifdef PCMCIA_DEBUG
+static int pc_debug = PCMCIA_DEBUG;
+module_param(pc_debug, int, 0600);
+#define DEBUG(n, x, args...) do { if (pc_debug >= (n))
\
+ printk(KERN_DEBUG "%s:%s:" x, MODULE_NAME, \
+__FUNCTION__, ##args); } while (0)
+#else
+#define DEBUG(n, args...)
+#endif
+
+static volatile char *version =
+"OMNIKEY CardMan 4040 v1.1.0gm3 - All bugs added by Harald Welte";
+
+#defineCCID_DRIVER_BULK_DEFAULT_TIMEOUT(150*HZ)
+#defineCCID_DRIVER_ASYNC_POWERUP_TIMEOUT (35*HZ)
+#defineCCID_DRIVER_MINIMUM_TIMEOUT (3*HZ)
+#define READ_WRITE_BUFFER_SIZE 512
+#define POLL_LOOP_COUNT1000
+
+/* how often to poll for fifo status change */
+#define POLL_PERIOD(HZ/100)
+
+static void reader_release(u_long arg);
+static void reader_detach(dev_link_t *link);
+
+static int major;
+
+#defineBS_READABLE 0x01
+#defineBS_WRITABLE 0x02
+
+typedef struct reader_dev_t {
+ dev_link_t link;
+ dev_node_t node;
+ wait_queue_head_t devq;
+
+ wait_queue_head_t poll_wait;
+ wait_queue_head_t read_wait;
+ wait_queue_head_t write_wait;
+
+ unsigned intbuffer_status;
+
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
> Below you can find a driver for the Omnikey CardMan 4040 PCMCIA > Smartcard Reader. Someone correct me if I'm wrong, but wouldn't these #defines be a problem with the new HZ flexibility: #define CCID_DRIVER_BULK_DEFAULT_TIMEOUT(150*HZ) #define CCID_DRIVER_ASYNC_POWERUP_TIMEOUT (35*HZ) #define CCID_DRIVER_MINIMUM_TIMEOUT (3*HZ) #define READ_WRITE_BUFFER_SIZE 512 #define POLL_LOOP_COUNT 1000 /* how often to poll for fifo status change */ #define POLL_PERIOD (HZ/100) In particular, 2.6.13 allows a HZ of 100, which would define POLL_PERIOD to 0. Your later calls to mod_timer would be setting cmx_poll_timer to the current value of jiffies. Also, you've got a typo in the comments: * - adhere to linux kenrel coding style and policies Forgive me if I'm way off - I'm just now getting my feet wet in kernel development. Just making comments based on what I (think) I know at this point. Best Regards, Chase Venters - 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/
[PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
Hi!
Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
Smartcard Reader.
It's based on some source code originally made available by the vendor
(as BSD/GPL dual licensed code), but has undergone significant changes
to make it more compliant with the general kernel community coding
practise.
As this is the first PCMCIA driver that I'm involved in, please let me
know if I missed something.
If there are no objections, I'd like to see it included in mainline.
Thanks!
--
- Harald Welte <[EMAIL PROTECTED]> http://gnumonks.org/
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
Omnikey CardMan 4040 PCMCIA Smartcard reader driver
Signed-off-by: Harald Welte <[EMAIL PROTECTED]>
---
commit 04ec3ca3e3c6fd6d88c508b6ebe32726ef109367
tree e27a5ccafbcebda6ebfe90036016f0e76dd93137
parent c4ab879b6ef599bf88d19b9b145878ef73400ce7
author Harald Welte <[EMAIL PROTECTED]> So, 04 Sep 2005 11:59:37 +0200
committer Harald Welte <[EMAIL PROTECTED]> So, 04 Sep 2005 11:59:37 +0200
MAINTAINERS |5
drivers/char/pcmcia/Kconfig | 13 +
drivers/char/pcmcia/Makefile|1
drivers/char/pcmcia/cm4040_cs.c | 900 +++
4 files changed, 919 insertions(+), 0 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1737,6 +1737,11 @@ L: [EMAIL PROTECTED]
W: http://www.linuxtr.net
S: Maintained
+OMNIKEY CARDMAN 4040 DRIVER
+P: Harald Welte
+M: [EMAIL PROTECTED]
+S: Maintained
+
ONSTREAM SCSI TAPE DRIVER
P: Willem Riede
M: [EMAIL PROTECTED]
diff --git a/drivers/char/pcmcia/Kconfig b/drivers/char/pcmcia/Kconfig
--- a/drivers/char/pcmcia/Kconfig
+++ b/drivers/char/pcmcia/Kconfig
@@ -18,5 +18,18 @@ config SYNCLINK_CS
The module will be called synclinkmp. If you want to do that, say M
here.
+config CARDMAN_4040
+ tristate "Omnikey CardMan 4040 support"
+ depends on PCMCIA
+ help
+ Enable support for the Omnikey CardMan 4040 PCMCIA Smartcard
+ reader.
+
+ This card is basically a USB CCID device connected to a FIFO
+ in I/O space. To use the kernel driver, you will need either the
+ PC/SC ifdhandler provided from the Omnikey homepage
+ (http://www.omnikey.com/), or a current development version of OpenCT
+ (http://www.opensc.org/).
+
endmenu
diff --git a/drivers/char/pcmcia/Makefile b/drivers/char/pcmcia/Makefile
--- a/drivers/char/pcmcia/Makefile
+++ b/drivers/char/pcmcia/Makefile
@@ -5,3 +5,4 @@
#
obj-$(CONFIG_SYNCLINK_CS) += synclink_cs.o
+obj-$(CONFIG_CARDMAN_4040) += cm4040_cs.o
diff --git a/drivers/char/pcmcia/cm4040_cs.c b/drivers/char/pcmcia/cm4040_cs.c
new file mode 100644
--- /dev/null
+++ b/drivers/char/pcmcia/cm4040_cs.c
@@ -0,0 +1,900 @@
+ /*
+ * A driver for the Omnikey PCMCIA smartcard reader CardMan 4040
+ *
+ * (c) 2000-2004 Omnikey AG (http://www.omnikey.com/)
+ *
+ * (C) 2005 Harald Welte <[EMAIL PROTECTED]>
+ * - add support for poll()
+ * - driver cleanup
+ * - add waitqueues
+ * - adhere to linux kenrel coding style and policies
+ * - support 2.6.13 "new style" pcmcia interface
+ *
+ * All rights reserved, Dual BSD/GPL Licensed.
+ */
+
+/* #define PCMCIA_DEBUG 6 */
+
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "cm4040_cs.h"
+
+static atomic_t cmx_num_devices_open;
+
+#ifdef PCMCIA_DEBUG
+static int pc_debug = PCMCIA_DEBUG;
+module_param(pc_debug, int, 0600);
+#define DEBUG(n, x, args...) do { if (pc_debug >= (n))
\
+ printk(KERN_DEBUG "%s:%s:" x, MODULE_NAME, \
+__FUNCTION__, ##args); } while (0)
+#else
+#define DEBUG(n, args...)
+#endif
+
+static volatile char *version =
+"OMNIKEY CardMan 4040 v1.1.0gm3 - All bugs added by Harald Welte";
+
+#defineCCID_DRIVER_BULK_DEFAULT_TIMEOUT(150*HZ)
+#defineCCID_DRIVER_ASYNC_POWERUP_TIMEOUT (35*HZ)
+#defineCCID_DRIVER_MINIMUM_TIMEOUT (3*HZ)
+#define READ_WRITE_BUFFER_SIZE 512
+#define POLL_LOOP_COUNT1000
+
+/* how often to poll for fifo status change */
+#define POLL_PERIOD(HZ/100)
+
+static void reader_release(u_long arg);
+static void reader_detach(dev_link_t *link);
+
+static int major;
+
+#defineBS_READABLE 0x01
+#defineBS_WRITABLE 0x02
+
+typedef struct reader_dev_t {
+ dev_link_t link;
+ dev_node_t node;
+
[PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
Hi!
Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
Smartcard Reader.
It's based on some source code originally made available by the vendor
(as BSD/GPL dual licensed code), but has undergone significant changes
to make it more compliant with the general kernel community coding
practise.
As this is the first PCMCIA driver that I'm involved in, please let me
know if I missed something.
If there are no objections, I'd like to see it included in mainline.
Thanks!
--
- Harald Welte [EMAIL PROTECTED] http://gnumonks.org/
Privacy in residential applications is a desirable marketing option.
(ETSI EN 300 175-7 Ch. A6)
Omnikey CardMan 4040 PCMCIA Smartcard reader driver
Signed-off-by: Harald Welte [EMAIL PROTECTED]
---
commit 04ec3ca3e3c6fd6d88c508b6ebe32726ef109367
tree e27a5ccafbcebda6ebfe90036016f0e76dd93137
parent c4ab879b6ef599bf88d19b9b145878ef73400ce7
author Harald Welte [EMAIL PROTECTED] So, 04 Sep 2005 11:59:37 +0200
committer Harald Welte [EMAIL PROTECTED] So, 04 Sep 2005 11:59:37 +0200
MAINTAINERS |5
drivers/char/pcmcia/Kconfig | 13 +
drivers/char/pcmcia/Makefile|1
drivers/char/pcmcia/cm4040_cs.c | 900 +++
4 files changed, 919 insertions(+), 0 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1737,6 +1737,11 @@ L: [EMAIL PROTECTED]
W: http://www.linuxtr.net
S: Maintained
+OMNIKEY CARDMAN 4040 DRIVER
+P: Harald Welte
+M: [EMAIL PROTECTED]
+S: Maintained
+
ONSTREAM SCSI TAPE DRIVER
P: Willem Riede
M: [EMAIL PROTECTED]
diff --git a/drivers/char/pcmcia/Kconfig b/drivers/char/pcmcia/Kconfig
--- a/drivers/char/pcmcia/Kconfig
+++ b/drivers/char/pcmcia/Kconfig
@@ -18,5 +18,18 @@ config SYNCLINK_CS
The module will be called synclinkmp. If you want to do that, say M
here.
+config CARDMAN_4040
+ tristate Omnikey CardMan 4040 support
+ depends on PCMCIA
+ help
+ Enable support for the Omnikey CardMan 4040 PCMCIA Smartcard
+ reader.
+
+ This card is basically a USB CCID device connected to a FIFO
+ in I/O space. To use the kernel driver, you will need either the
+ PC/SC ifdhandler provided from the Omnikey homepage
+ (http://www.omnikey.com/), or a current development version of OpenCT
+ (http://www.opensc.org/).
+
endmenu
diff --git a/drivers/char/pcmcia/Makefile b/drivers/char/pcmcia/Makefile
--- a/drivers/char/pcmcia/Makefile
+++ b/drivers/char/pcmcia/Makefile
@@ -5,3 +5,4 @@
#
obj-$(CONFIG_SYNCLINK_CS) += synclink_cs.o
+obj-$(CONFIG_CARDMAN_4040) += cm4040_cs.o
diff --git a/drivers/char/pcmcia/cm4040_cs.c b/drivers/char/pcmcia/cm4040_cs.c
new file mode 100644
--- /dev/null
+++ b/drivers/char/pcmcia/cm4040_cs.c
@@ -0,0 +1,900 @@
+ /*
+ * A driver for the Omnikey PCMCIA smartcard reader CardMan 4040
+ *
+ * (c) 2000-2004 Omnikey AG (http://www.omnikey.com/)
+ *
+ * (C) 2005 Harald Welte [EMAIL PROTECTED]
+ * - add support for poll()
+ * - driver cleanup
+ * - add waitqueues
+ * - adhere to linux kenrel coding style and policies
+ * - support 2.6.13 new style pcmcia interface
+ *
+ * All rights reserved, Dual BSD/GPL Licensed.
+ */
+
+/* #define PCMCIA_DEBUG 6 */
+
+#include linux/config.h
+#include linux/version.h
+
+#include linux/kernel.h
+#include linux/module.h
+#include linux/slab.h
+#include linux/init.h
+#include linux/fs.h
+#include linux/delay.h
+#include linux/poll.h
+#include asm/uaccess.h
+#include asm/io.h
+
+#include pcmcia/version.h
+#include pcmcia/cs_types.h
+#include pcmcia/cs.h
+#include pcmcia/cistpl.h
+#include pcmcia/cisreg.h
+#include pcmcia/ciscode.h
+#include pcmcia/ds.h
+
+#include cm4040_cs.h
+
+static atomic_t cmx_num_devices_open;
+
+#ifdef PCMCIA_DEBUG
+static int pc_debug = PCMCIA_DEBUG;
+module_param(pc_debug, int, 0600);
+#define DEBUG(n, x, args...) do { if (pc_debug = (n))
\
+ printk(KERN_DEBUG %s:%s: x, MODULE_NAME, \
+__FUNCTION__, ##args); } while (0)
+#else
+#define DEBUG(n, args...)
+#endif
+
+static volatile char *version =
+OMNIKEY CardMan 4040 v1.1.0gm3 - All bugs added by Harald Welte;
+
+#defineCCID_DRIVER_BULK_DEFAULT_TIMEOUT(150*HZ)
+#defineCCID_DRIVER_ASYNC_POWERUP_TIMEOUT (35*HZ)
+#defineCCID_DRIVER_MINIMUM_TIMEOUT (3*HZ)
+#define READ_WRITE_BUFFER_SIZE 512
+#define POLL_LOOP_COUNT1000
+
+/* how often to poll for fifo status change */
+#define POLL_PERIOD(HZ/100)
+
+static void reader_release(u_long arg);
+static void reader_detach(dev_link_t *link);
+
+static int major;
+
+#define
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
Below you can find a driver for the Omnikey CardMan 4040 PCMCIA Smartcard Reader. Someone correct me if I'm wrong, but wouldn't these #defines be a problem with the new HZ flexibility: #define CCID_DRIVER_BULK_DEFAULT_TIMEOUT(150*HZ) #define CCID_DRIVER_ASYNC_POWERUP_TIMEOUT (35*HZ) #define CCID_DRIVER_MINIMUM_TIMEOUT (3*HZ) #define READ_WRITE_BUFFER_SIZE 512 #define POLL_LOOP_COUNT 1000 /* how often to poll for fifo status change */ #define POLL_PERIOD (HZ/100) In particular, 2.6.13 allows a HZ of 100, which would define POLL_PERIOD to 0. Your later calls to mod_timer would be setting cmx_poll_timer to the current value of jiffies. Also, you've got a typo in the comments: * - adhere to linux kenrel coding style and policies Forgive me if I'm way off - I'm just now getting my feet wet in kernel development. Just making comments based on what I (think) I know at this point. Best Regards, Chase Venters - 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/
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
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.
--
- Harald Welte [EMAIL PROTECTED] http://gnumonks.org/
Privacy in residential applications is a desirable marketing option.
(ETSI EN 300 175-7 Ch. A6)
Add Omnikey CardMan 4040 Driver
Signed-off-by: Harald Welte [EMAIL PROTECTED]
---
commit 0e760a5785ebb83b932d104679cc2b2b4070b1c1
tree 46d71d0c6b41b441a1a8d725b741450f1334296e
parent c4ab879b6ef599bf88d19b9b145878ef73400ce7
author Harald Welte [EMAIL PROTECTED] So, 04 Sep 2005 13:05:44 +0200
committer Harald Welte [EMAIL PROTECTED] So, 04 Sep 2005 13:05:44 +0200
MAINTAINERS |5
drivers/char/pcmcia/Kconfig | 13 +
drivers/char/pcmcia/Makefile|1
drivers/char/pcmcia/cm4040_cs.c | 900 +++
drivers/char/pcmcia/cm4040_cs.h | 52 ++
5 files changed, 971 insertions(+), 0 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1737,6 +1737,11 @@ L: [EMAIL PROTECTED]
W: http://www.linuxtr.net
S: Maintained
+OMNIKEY CARDMAN 4040 DRIVER
+P: Harald Welte
+M: [EMAIL PROTECTED]
+S: Maintained
+
ONSTREAM SCSI TAPE DRIVER
P: Willem Riede
M: [EMAIL PROTECTED]
diff --git a/drivers/char/pcmcia/Kconfig b/drivers/char/pcmcia/Kconfig
--- a/drivers/char/pcmcia/Kconfig
+++ b/drivers/char/pcmcia/Kconfig
@@ -18,5 +18,18 @@ config SYNCLINK_CS
The module will be called synclinkmp. If you want to do that, say M
here.
+config CARDMAN_4040
+ tristate Omnikey CardMan 4040 support
+ depends on PCMCIA
+ help
+ Enable support for the Omnikey CardMan 4040 PCMCIA Smartcard
+ reader.
+
+ This card is basically a USB CCID device connected to a FIFO
+ in I/O space. To use the kernel driver, you will need either the
+ PC/SC ifdhandler provided from the Omnikey homepage
+ (http://www.omnikey.com/), or a current development version of OpenCT
+ (http://www.opensc.org/).
+
endmenu
diff --git a/drivers/char/pcmcia/Makefile b/drivers/char/pcmcia/Makefile
--- a/drivers/char/pcmcia/Makefile
+++ b/drivers/char/pcmcia/Makefile
@@ -5,3 +5,4 @@
#
obj-$(CONFIG_SYNCLINK_CS) += synclink_cs.o
+obj-$(CONFIG_CARDMAN_4040) += cm4040_cs.o
diff --git a/drivers/char/pcmcia/cm4040_cs.c b/drivers/char/pcmcia/cm4040_cs.c
new file mode 100644
--- /dev/null
+++ b/drivers/char/pcmcia/cm4040_cs.c
@@ -0,0 +1,900 @@
+ /*
+ * A driver for the Omnikey PCMCIA smartcard reader CardMan 4040
+ *
+ * (c) 2000-2004 Omnikey AG (http://www.omnikey.com/)
+ *
+ * (C) 2005 Harald Welte [EMAIL PROTECTED]
+ * - add support for poll()
+ * - driver cleanup
+ * - add waitqueues
+ * - adhere to linux kenrel coding style and policies
+ * - support 2.6.13 new style pcmcia interface
+ *
+ * All rights reserved, Dual BSD/GPL Licensed.
+ */
+
+/* #define PCMCIA_DEBUG 6 */
+
+#include linux/config.h
+#include linux/version.h
+
+#include linux/kernel.h
+#include linux/module.h
+#include linux/slab.h
+#include linux/init.h
+#include linux/fs.h
+#include linux/delay.h
+#include linux/poll.h
+#include asm/uaccess.h
+#include asm/io.h
+
+#include pcmcia/version.h
+#include pcmcia/cs_types.h
+#include pcmcia/cs.h
+#include pcmcia/cistpl.h
+#include pcmcia/cisreg.h
+#include pcmcia/ciscode.h
+#include pcmcia/ds.h
+
+#include cm4040_cs.h
+
+static atomic_t cmx_num_devices_open;
+
+#ifdef PCMCIA_DEBUG
+static int pc_debug = PCMCIA_DEBUG;
+module_param(pc_debug, int, 0600);
+#define DEBUG(n, x, args...) do { if (pc_debug = (n))
\
+ printk(KERN_DEBUG %s:%s: x, MODULE_NAME, \
+__FUNCTION__, ##args); } while (0)
+#else
+#define DEBUG(n, args...)
+#endif
+
+static volatile char *version =
+OMNIKEY CardMan 4040 v1.1.0gm3 - All bugs added by Harald Welte;
+
+#defineCCID_DRIVER_BULK_DEFAULT_TIMEOUT(150*HZ)
+#defineCCID_DRIVER_ASYNC_POWERUP_TIMEOUT (35*HZ)
+#defineCCID_DRIVER_MINIMUM_TIMEOUT (3*HZ)
+#define READ_WRITE_BUFFER_SIZE 512
+#define POLL_LOOP_COUNT1000
+
+/* how often to poll for fifo status change */
+#define POLL_PERIOD(HZ/100)
+
+static void reader_release(u_long arg);
+static void reader_detach(dev_link_t *link);
+
+static int major;
+
+#defineBS_READABLE 0x01
+#defineBS_WRITABLE 0x02
+
+typedef struct reader_dev_t {
+ dev_link_t link;
+ dev_node_t node;
+
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
On Sun, Sep 04, 2005 at 12:12:18PM +0200, Harald Welte wrote:
Below you can find a driver for the Omnikey CardMan 4040 PCMCIA
Smartcard Reader.
--- /dev/null
+++ b/drivers/char/pcmcia/cm4040_cs.c
+#include linux/config.h
Not needed.
+static volatile char *version =
Can we lose all volatile and register keywords?
+typedef struct reader_dev_t {
+ dev_link_t link;
+ dev_node_t node;
+ wait_queue_head_t devq;
+
+ wait_queue_head_t poll_wait;
+ wait_queue_head_t read_wait;
+ wait_queue_head_t write_wait;
+
+ unsigned intbuffer_status;
+
+ unsigned intfTimerExpired;
+ struct timer_list timer;
+ unsigned long timeout;
+ unsigned char sBuf[READ_WRITE_BUFFER_SIZE];
+ unsigned char rBuf[READ_WRITE_BUFFER_SIZE];
+ struct task_struct *owner;
+} reader_dev_t;
And typedefs too.
struct reader_dev {
};
+static ssize_t cmx_read(struct file *filp,char *buf,size_t count,loff_t
*ppos)
char __user *buf
+ ulBytesToRead = 5 +
+ (0x00FF((char)dev-rBuf[1])) +
+ (0xFF00((char)dev-rBuf[2] 8)) +
+ (0x00FF((char)dev-rBuf[3] 16)) +
+ (0xFF00((char)dev-rBuf[4] 24));
ulBytesToRead = 5 + le32_to_cpu(*(__le32 *)dev-rBuf[1]);
+ ulMin = (count (ulBytesToRead+5))?count:(ulBytesToRead+5);
ulMin = min(count, ulBytesToRead + 5);
+ copy_to_user(buf, dev-rBuf, ulMin);
Can fail.
+static ssize_t cmx_write(struct file *filp,const char *buf,size_t count,
const char __user *buf
+ loff_t *ppos)
+ copy_from_user(dev-sBuf, buf, uiBytesToWrite);
Can fail.
+static int cmx_ioctl(struct inode *inode,struct file *filp,unsigned int cmd,
+ unsigned long arg)
+{
+ dev_link_t *link;
+ int rc, size;
+
+ link=dev_table[MINOR(inode-i_rdev)];
+ if (!(DEV_OK(link))) {
+ DEBUG(4, DEV_OK false\n);
+ return -ENODEV;
+ }
+ if (_IOC_TYPE(cmd)!=CM_IOC_MAGIC) {
+ DEBUG(4,ioctype mismatch\n);
+ return -EINVAL;
+ }
+ if (_IOC_NR(cmd)CM_IOC_MAXNR) {
+ DEBUG(4,iocnr mismatch\n);
+ return -EINVAL;
+ }
+ size = _IOC_SIZE(cmd);
+ rc = 0;
+ DEBUG(4,iocdir=%.4x iocr=%.4x iocw=%.4x iocsize=%d cmd=%.4x\n,
+ _IOC_DIR(cmd),_IOC_READ,_IOC_WRITE,size,cmd);
+
+ if (_IOC_DIR(cmd)_IOC_READ) {
+ if (!access_ok(VERIFY_WRITE, (void *)arg, size))
+ return -EFAULT;
+ }
+ if (_IOC_DIR(cmd)_IOC_WRITE) {
+ if (!access_ok(VERIFY_READ, (void *)arg, size))
+ return -EFAULT;
+ }
+
+ return rc;
+}
Whoo, empty ioctl handler.
+static void reader_release(u_long arg)
+ link = (dev_link_t *)arg;
You do
reader_release((unsigned long)link);
somewhere above and below.
+static void reader_detach_by_devno(int devno,dev_link_t *link)
+ reader_release((u_long)link);
Like this.
-
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/
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
On 9/3/05, Chase Venters [EMAIL PROTECTED] wrote: Below you can find a driver for the Omnikey CardMan 4040 PCMCIA Smartcard Reader. Someone correct me if I'm wrong, but wouldn't these #defines be a problem with the new HZ flexibility: #define CCID_DRIVER_BULK_DEFAULT_TIMEOUT(150*HZ) #define CCID_DRIVER_ASYNC_POWERUP_TIMEOUT (35*HZ) #define CCID_DRIVER_MINIMUM_TIMEOUT (3*HZ) #define READ_WRITE_BUFFER_SIZE 512 #define POLL_LOOP_COUNT 1000 These are all fine. Although I am a bit suspicious of 150 second timeouts; but if that is the hardware... /* how often to poll for fifo status change */ #define POLL_PERIOD (HZ/100) This needs to be msecs_to_jiffies(10), please. In particular, 2.6.13 allows a HZ of 100, which would define POLL_PERIOD to 0. Um, 100/100 = 1, not 0? Your later calls to mod_timer would be setting cmx_poll_timer to the current value of jiffies. Which is technically ok, because HZ=100, a jiffies + 0 or jiffies + 1 timeout request will both result in the soft-timer being expired at the *next* timer interrupt. Regardless, you're right, and msecs_to_jiffies() will cover it. Also, you've got a typo in the comments: * - adhere to linux kenrel coding style and policies Forgive me if I'm way off - I'm just now getting my feet wet in kernel development. Just making comments based on what I (think) I know at this point. Of bigger concern to me is the use of the sleep_on() family of functions, all of which are deprecated. Thanks, Nish - 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/
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
Um, 100/100 = 1, not 0? Oh my... it's been a long day. Regards, Chase Venters - 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/
Re: [PATCH] New: Omnikey CardMan 4040 PCMCIA Driver
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; i5; 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 = 0002;
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/
