[PATCH v2 0/1] PN544 NFC driver.
Hello.
And thanks to Andrew for the comments.
include/linux/pn544.h | 99 ++
Is drivers/misc/ the best place for this?
Don't be afraid to create a new drivers/nfc/ even if it has only one
driver in it. If someone later comes in and adds a new NFC driver then
things will all fall into place.
OK. Now I've created directories drivers/nfc, include/linux/nfc and
Documentation/nfc.
+ Say yes if you want PN544 Near Field Communication driver.
OK, I did google it ;) Interesting.
I agree... ;-)
+ pr_info(\n);
You might be able to use print_hex_dump() here, but I wouldn't blame
you if you didn't ;)
I replaced the debug function with calls to print_hex_dump.
+/* sysfs interface */
OK, this is more serious.
You're proposing a permanent addition to the kernel ABI. This is the
most important part of the driver because it is something we can never
change. This interface is the very first thing we'll want to
understand and review, before we even look at the implementation.
And it isn't even described! Not in the changelog, not in a
documentation file, not even in code comments.
Please, provide a description of this proposed interface. Sufficient
for reviewers to understand it and for users to use it. Pobably this
will require some description of the hardware functions as well.
Please also consider updating Documentation/ABI/
I've added a documentation file. But I didn't make changes to the interface
yet.
+GFP_KERNEL);
From my reading, later code will crash the kernel if this allocation failed.
Also, is there a race here between reading info-fw_buf and setting it?
Can two CPUs get into thei function at the same time? If so, there
should be locking. Say, a mutex local to this function.
I moved the data buffer allocation to probe and also changed the locking.
+ return -EBADMSG;
EBADMSG is a unix streams errno. It's quite inappropriate that a
device driver be using it. Imagine the poor user's confution if he
sees this message pop up on his screen.
Changed to EINVAL.
+ if (r == -EREMOTEIO) { /* Retry, chip was in standby */
And what on earth is EREMOTEIO? Is it appropriate for use in a driver?
I think it is, because it comes from i2c_master_send and it's kind of
just forwarded here.
+ return -EOVERFLOW;
EOVERFLOW refers to a floating point overflow!
It's used for buffer overflow in many places in the kernel, but that's
not an excuse so I removed it.
+ return -ENOSPC;
Disk full!
Also removed...
+ mutex_unlock(info-read_mutex);
It usually doesn't make much sense to add locking around a single
atomic read instruction.
Yes, but if there's a sequence of instructions somewhere else in the
code that the single instruction is not allowd to intersect then it
can be valid.
+ size_t count, loff_t *offset)
Again, I really cannot review this code when I haven't been told what
it's reading from, what's in the payload, what it is all to be used
for, etc. It's just C code to me.
I've added a documentation file to explain things but basically the
driver doesn't much care about the data it passes through, apart from
message lengths and check sums.
+ len = min(count, (size_t) PN544_MAX_I2C_TRANSFER);
min_t() is the preferred way of solving this.
Good to know, but because of a code change it wasn't necessary.
+}
So it can poll() somethnig as well! This driver *really* needs
documentation!
Yes, you can wait for something to read.
+ if (info-state == PN544_ST_FW_READY) {
Is some locking needed here? What prevents info-state from
transitioning while this code is executing?
Locking added.
+static long pn544_ioctl(struct file *file, unsigned int cmd, unsigned long
arg)
And an ioctl interface as well! An undocmented one.
ioctls are pretty unpopular for various reasons. To a large extent,
people have been using sysfs interfaces as a repalcement for ioctl()s.
But this driver has both!
Some explanatory text written into the documentation file.
Does this code need compat handling, or it is 32/64-bit clean?
I think it is...
+ pn544_disable(info);
bug. pn544_disable() calls msleep(), but msleep() is very illegal
under spin_lock_irqsave(). This tells me that this code hasn't been
tested with even rudimentary kernel rimtime debugging options enabled.
Documentation/SubmitChecklist section 12 is fairly up-to-date here.
Removed spinlocks and in general revised the locking...
+ pn544_disable(info);
Many more such bugs there.
Well, yes...
+#ifdef DO_RSET_TEST
I'd suggest enabling this via a module parameter if it's at all useful.
Otherwise it should be enabled with a Kconfig variable, not via a code
edit.
Removed the test...
+ flush_scheduled_work();
This seems unneeded? We're trying to get rid
[PATCH v2 1/1] NFC: Driver for NXP Semiconductors PN544 NFC chip.
This is a driver for the pn544 NFC device. The driver transfers
ETSI messages between the device and the user space.
Signed-off-by: Matti J. Aaltonen matti.j.aalto...@nokia.com
---
Documentation/nfc/nfc-pn544.txt | 105 +
drivers/Kconfig |2 +
drivers/Makefile|2 +-
drivers/nfc/Kconfig | 30 ++
drivers/nfc/Makefile|5 +
drivers/nfc/pn544.c | 893 +++
include/linux/nfc/pn544.h | 99 +
7 files changed, 1135 insertions(+), 1 deletions(-)
create mode 100644 Documentation/nfc/nfc-pn544.txt
create mode 100644 drivers/nfc/Kconfig
create mode 100644 drivers/nfc/Makefile
create mode 100644 drivers/nfc/pn544.c
create mode 100644 include/linux/nfc/pn544.h
diff --git a/Documentation/nfc/nfc-pn544.txt b/Documentation/nfc/nfc-pn544.txt
new file mode 100644
index 000..8adcffa
--- /dev/null
+++ b/Documentation/nfc/nfc-pn544.txt
@@ -0,0 +1,105 @@
+Kernel driver for PN544
+
+
+* NXP Semiconductor PN544 Near Field Communication (NFC) chip
+
+Author: Jari Vanhala
+Contact: Matti Aaltonen (matti.j.aaltonen at nokia.com)
+
+Description
+---
+
+The PN544 is an integrated transmission module for contactless
+communication. The driver goes under drives/nfc/ and is compiled as a
+module named pn544. It registers a misc device and creates a device
+file named /dev/pn544.
+
+The driver, is quite simple and it mainly passes data between the
+hardware and the user space. There are two operating modes: The HCI
+(normal) mode and the firmware update mode. It can write and read in
+both modes. The hardware sends an interrupt when data is available
+for reading. Data is read when the read function is called by some
+user space application. Poll() checks the IRQ state. Configuration and
+self testing are done from the user space using read and write.
+
+The chip is powered up when the device is opened and otherwise
+it's turned off. Only one instance can use the device at a time.
+
+Host Interfaces: consisting of I2C, SPI and HSU as physical
+interfaces, this driver support I2C.
+
+The chip is controlled from the user spase by sending SWP/HCI (Single
+Wire Protocol) messages according to the ETSI/SCP standard, see
+http://www.etsi.org/WebSite/Technologies/ProtocolSpecification.aspx
+
+The driver handles two kinds of messages: the normal HCI messages and
+the firmware update messages. HCI messages consist of and eight bit
+header and the message body. The header contains the message length.
+Maximum size for an HCI message is 33. In HCI mode sent packets are
+tested for a correct CRC.
+
+Firmware update messages, which can be read and written in the
+firmware upload mode have the length in the second (MSB) and third
+(LSB) bytes of the message. The maximum FW message size is 1024
+bytes.
+
+The interfaces for pn544 users:
+
+There is a sysfs interface for testing that the hardware is
+operational. Reading from the sysfs file activates the test. The test
+returns one on success and zero on failure.
+
+Example:
+ cat /sys/module/pn544/drivers/i2c\:pn544/3-002b/nfc_test
+1
+
+For mode setting there is an IOCTL interface, which consists of
+two messages:
+
+PN544_GET_FW_MODE returns 1 if the device is in firmware
+update mode and zero in the normal (HCI) mode.
+
+PN544_SET_FW_MODE is for setting the mode. The message parameter
+value defines the mode: one corresponds firmware update and zero the
+HCI mode.
+
+
+Example platform data:
+
+static int rx71_pn544_nfc_request_resources(struct i2c_client *client)
+{
+ /* Get and setup the HW resources for the device */
+}
+
+static void rx71_pn544_nfc_free_resources(void)
+{
+ /* Release the HW resources */
+}
+
+static void rx71_pn544_nfc_enable(int fw)
+{
+ /* Turn the device on */
+}
+
+static int rx71_pn544_nfc_test(void)
+{
+ /*
+* Put the device into the FW update mode
+* and then back to the normal mode.
+* Return one on success and zero on
+ * failure.
+ */
+}
+
+static void rx71_pn544_nfc_disable(void)
+{
+ /* turn the power off */
+}
+
+static struct pn544_nfc_platform_data rx71_nfc_data = {
+ .request_resources = rx71_pn544_nfc_request_resources,
+ .free_resources = rx71_pn544_nfc_free_resources,
+ .enable = rx71_pn544_nfc_enable,
+ .test = rx71_pn544_nfc_test,
+ .disable = rx71_pn544_nfc_disable,
+};
diff --git a/drivers/Kconfig b/drivers/Kconfig
index a2b902f..d7f27d6 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -111,4 +111,6 @@ source drivers/xen/Kconfig
source drivers/staging/Kconfig
source drivers/platform/Kconfig
+
+source drivers/nfc/Kconfig
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index a2aea53..a6705ff 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -39,7 +39,7 @@ obj-$(CONFIG_FB_INTEL) += video/intelfb/
obj-y += serial/
[PATCH -next] i2c: intel-mid depends on PCI
From: Randy Dunlap randy.dun...@oracle.com i2c-intel-mid driver uses PCI data structs and interfaces, so it should depend on PCI. Fixes these build errors: drivers/i2c/busses/i2c-intel-mid.c:977: error: implicit declaration of function 'pci_request_region' drivers/i2c/busses/i2c-intel-mid.c:1077: error: implicit declaration of function 'pci_release_region' Signed-off-by: Randy Dunlap randy.dun...@oracle.com Cc: Ba Zheng zheng...@intel.com Cc: Jean Delvare kh...@linux-fr.org Cc: Ben Dooks ben-li...@fluff.org Cc: linux-i2c@vger.kernel.org --- drivers/i2c/busses/Kconfig |1 + 1 file changed, 1 insertion(+) --- linux-next-20101029.orig/drivers/i2c/busses/Kconfig +++ linux-next-20101029/drivers/i2c/busses/Kconfig @@ -398,6 +398,7 @@ config I2C_IMX config I2C_INTEL_MID tristate Intel Moorestown/Medfield Platform I2C controller + depends on PCI help Say Y here if you have an Intel Moorestown/Medfield platform I2C controller. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/1] PN544 NFC driver.
On Fri, 29 Oct 2010 09:26:08 +0300 Matti J. Aaltonen matti.j.aalto...@nokia.com wrote: Hello. And thanks to Andrew for the comments. include/linux/pn544.h | 99 ++ Is drivers/misc/ the best place for this? Don't be afraid to create a new drivers/nfc/ even if it has only one driver in it. If someone later comes in and adds a new NFC driver then things will all fall into place. OK. Now I've created directories drivers/nfc, include/linux/nfc and Documentation/nfc. I beefed up the changelog a bit, telling people what nfc is. We really should tell more people that we're adding a new subsystem. Can you please resend the patchset, cc'ing linux-kernel? +/* sysfs interface */ OK, this is more serious. You're proposing a permanent addition to the kernel ABI. This is the most important part of the driver because it is something we can never change. This interface is the very first thing we'll want to understand and review, before we even look at the implementation. And it isn't even described! Not in the changelog, not in a documentation file, not even in code comments. Please, provide a description of this proposed interface. Sufficient for reviewers to understand it and for users to use it. Pobably this will require some description of the hardware functions as well. Please also consider updating Documentation/ABI/ I've added a documentation file. But I didn't make changes to the interface yet. So we can expect some updates? And an ioctl interface as well! An undocmented one. ioctls are pretty unpopular for various reasons. To a large extent, people have been using sysfs interfaces as a repalcement for ioctl()s. But this driver has both! Some explanatory text written into the documentation file. So, the sysfs interface is purely for running a device test? And primary communication is via the /dev node, and that node supports an ioctl() which changes the meaning of reads and writes to that node? If so, that's a bit of a peculiar interface. Perhaps there should have been two /dev nodes. Certainly it would be most popular if the ioctl() interface were to simply disappear. Please, do spell all of this out in some detail within the changelog when resending the code for wider review. There are people out there (eg Greg, Alan) who are better at these things than I and we should provide them with all the details up-front without going through another question-and-answer session or expecting them to grovel through code to reverse-engineer the interfaces. Another consideration here is that if we do expect more NFC devices and drivers for them, then we should aim for some standardisation of the interface, from day one. Some discussion of this would also be helpful for reviewers. One other thing: these messages which flow between userspace and the device. Are they documented or sufficiently well understood so that non-Nokia people can use this driver? Because we had a driver come up a couple of weeks ago. It was a simple, clean driver but all it did was to shuffle opaque data between userspace and the device, and the contents of that data was not publicly available. I discussed the driver with Linus and he said no. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] NFC: Driver for NXP Semiconductors PN544 NFC chip.
On Fri, 29 Oct 2010 09:26:09 +0300
Matti J. Aaltonen matti.j.aalto...@nokia.com wrote:
+static void __exit pn544_exit(void)
+{
+ flush_scheduled_work();
You said this had been removed. Did you send the correct version?
+ i2c_del_driver(pn544_driver);
+ pr_info(DRIVER_DESC , Exiting.\n);
+}
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/1] PN544 NFC driver.
On Fri, Oct 29, 2010 at 02:33:33PM -0700, Andrew Morton wrote: Another consideration here is that if we do expect more NFC devices and drivers for them, then we should aim for some standardisation of the interface, from day one. Some discussion of this would also be helpful for reviewers. There's other NFC devices out there being used with Linux. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] bjdooks' i2c tree
The following changes since commit b18cae4224bde7e5a332c19bc99247b2098ea232: Linus Torvalds (1): Merge branch 'for-next' of git://android.git.kernel.org/kernel/tegra are available in the git repository at: git://git.fluff.org/bjdooks/linux.git for-2637/i2c-all Alan Cox (1): i2c-intel-mid: support for Moorestown and Medfield platform Arnaud Patard (1): i2c-s3c2410: Enable i2c clock only when doing some transfert Ben Dooks (2): Merge branch 'for-2637/i2c/i2c-nomadik' into next-i2c Merge branch 'for-2637/i2c/samsung' into next-i2c Linus Walleij (4): i2c-nomadik: documentation fixes i2c-nomadik: dynamic clocking i2c-nomadik: support smbus emulation i2c-nomadik: fixup bus delays Randy Dunlap (1): i2c-intel-mid: Driver depends on PCI drivers/i2c/busses/Kconfig | 10 + drivers/i2c/busses/Makefile|1 + drivers/i2c/busses/i2c-intel-mid.c | 1135 drivers/i2c/busses/i2c-nomadik.c | 36 +- drivers/i2c/busses/i2c-s3c2410.c | 10 +- 5 files changed, 1176 insertions(+), 16 deletions(-) create mode 100644 drivers/i2c/busses/i2c-intel-mid.c -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
