Re: [PATCH] Omnikey Cardman 4040 driver (UPDATE)
Hi hch, thanks for your feedback.
An updated version of the patch is at the bottom of this mail.
On Mon, Sep 05, 2005 at 08:06:35PM +0100, Christoph Hellwig wrote:
> > +#include
>
> I don't think you need this one.
done
> > +#include
>
> you shouldn't need this one.
done
> > +static atomic_t cm4040_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
>
> What about just using pr_debug (or dev_dbg where you have a struct device
> handy)
I really appreciate the multiple debug levels of the driver, sorry.
However, I have converted the DEBUG macro to use dev_printk as a
compromise.
> Please make the poll timer per device. We generally try to avoid
> global state, and this allows to get rid of the opencount tracking aswell.
done.
> > +static ssize_t cm4040_read(struct file *filp, char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct reader_dev *dev = (struct reader_dev *) filp->private_data;
>
> no need to case a void pointer.
well, but good style in my opioion anyway. Anway, I don't really mind,
so it has been removed.
> > + if (count < 10)
> > + return -EFAULT;
> > +
> > + if (filp->f_flags & O_NONBLOCK) {
> > + DEBUG(4, "filep->f_flags O_NONBLOCK set\n");
> > + DEBUG(2, "<- cm4040_read (failure)\n");
> > + return -EAGAIN;
> > + }
>
> this sounds rather pointless. letting an O_NONBLOCK open fail all
> the time doesn't sound like a good idea.
what about fcntl enabling NONBLOCK after the open?
Other character drivers (such as rtc) do the same, btw.
Is there any way how I can tell the VFS to make any nonblocking
operation fail?
> please use iminor.
done.
> given that you fail O_NONLOCK in open already the code above makes even
> less sense.
see my comment above.
> > + dev->owner = current;
>
> this doesn't make a lot of sense and seems to be only used in
> debug code, I'd suggest killing it.
done.
> you should be able to use file->private_data here.
done
> I think these events became methods of their own recently, not sure
> if it hit -mm or mainline yet.
I cannot find anything like that in mainline, thus my code remains
unchanged for now.
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,835 @@
+/*
+ * 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 kernel coding style and policies
+ * - support 2.6.13 "new style" pcmcia interface
+ *
+ * The device basically is a USB CCID compliant device that has been
+ * attached to an I/O-Mapped FIFO.
+ *
+ * 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 "cm4040_cs.h"
+
+
+#ifdef PCMCIA_DEBUG
+#define reader_to_dev(x)
Re: [PATCH] Omnikey Cardman 4040 driver (UPDATE)
Hi hch, thanks for your feedback.
An updated version of the patch is at the bottom of this mail.
On Mon, Sep 05, 2005 at 08:06:35PM +0100, Christoph Hellwig wrote:
+#include linux/version.h
I don't think you need this one.
done
+#include pcmcia/version.h
you shouldn't need this one.
done
+static atomic_t cm4040_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
What about just using pr_debug (or dev_dbg where you have a struct device
handy)
I really appreciate the multiple debug levels of the driver, sorry.
However, I have converted the DEBUG macro to use dev_printk as a
compromise.
Please make the poll timer per device. We generally try to avoid
global state, and this allows to get rid of the opencount tracking aswell.
done.
+static ssize_t cm4040_read(struct file *filp, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct reader_dev *dev = (struct reader_dev *) filp-private_data;
no need to case a void pointer.
well, but good style in my opioion anyway. Anway, I don't really mind,
so it has been removed.
+ if (count 10)
+ return -EFAULT;
+
+ if (filp-f_flags O_NONBLOCK) {
+ DEBUG(4, filep-f_flags O_NONBLOCK set\n);
+ DEBUG(2, - cm4040_read (failure)\n);
+ return -EAGAIN;
+ }
this sounds rather pointless. letting an O_NONBLOCK open fail all
the time doesn't sound like a good idea.
what about fcntl enabling NONBLOCK after the open?
Other character drivers (such as rtc) do the same, btw.
Is there any way how I can tell the VFS to make any nonblocking
operation fail?
please use iminor.
done.
given that you fail O_NONLOCK in open already the code above makes even
less sense.
see my comment above.
+ dev-owner = current;
this doesn't make a lot of sense and seems to be only used in
debug code, I'd suggest killing it.
done.
you should be able to use file-private_data here.
done
I think these events became methods of their own recently, not sure
if it hit -mm or mainline yet.
I cannot find anything like that in mainline, thus my code remains
unchanged for now.
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,835 @@
+/*
+ * 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 kernel coding style and policies
+ * - support 2.6.13 new style pcmcia interface
+ *
+ * The device basically is a USB CCID compliant device that has been
+ * attached to an I/O-Mapped FIFO.
+ *
+ * All rights reserved, Dual BSD/GPL Licensed.
+ */
+
+/* #define PCMCIA_DEBUG 6 */
+
+#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 linux/wait.h
+#include asm/uaccess.h
+#include asm/io.h
+
+#include pcmcia/cs_types.h
+#include pcmcia/cs.h
+#include pcmcia/cistpl.h
+#include
