[PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar Rymarkiewicz]

Updated patch after Arnd's comments.

Waldek

Waldemar Rymarkiewicz (1):
  NFC: Driver for Inside Secure MicroRead NFC chip

 Documentation/nfc/nfc-microread.txt |   46 ++++
 drivers/nfc/Kconfig                 |   10 +
 drivers/nfc/Makefile                |    1 +
 drivers/nfc/microread.c             |  465 +++++++++++++++++++++++++++++++++++
 include/linux/nfc/microread.h       |   31 +++
 5 files changed, 553 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/nfc/nfc-microread.txt
 create mode 100644 drivers/nfc/microread.c
 create mode 100644 include/linux/nfc/microread.h

[PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar Rymarkiewicz]

Add new driver for MicroRead NFC chip connected to i2c bus.

See Documentation/nfc/nfc-microread.txt.

Signed-off-by: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>
---
 Documentation/nfc/nfc-microread.txt |   46 ++++
 drivers/nfc/Kconfig                 |   10 +
 drivers/nfc/Makefile                |    1 +
 drivers/nfc/microread.c             |  465 +++++++++++++++++++++++++++++++++++
 include/linux/nfc/microread.h       |   31 +++
 5 files changed, 553 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/nfc/nfc-microread.txt
 create mode 100644 drivers/nfc/microread.c
 create mode 100644 include/linux/nfc/microread.h

diff --git a/Documentation/nfc/nfc-microread.txt b/Documentation/nfc/nfc-microread.txt
new file mode 100644
index 0000000..e15c49c
--- /dev/null
+++ b/Documentation/nfc/nfc-microread.txt
@@ -0,0 +1,46 @@
+Kernel driver for Inside Secure MicroRead Near Field Communication chip
+
+General
+-------
+
+microread is the RF chip that uses Near Field Communication (NFC) for proximity
+transactions and interactions.
+
+Please visit Inside Secure webside for microread specification:
+http://www.insidesecure.com/eng/Products/NFC-Products/MicroRead
+
+
+The Driver
+----------
+
+The microread driver can be found under drivers/nfc/microread.c and it's
+compiled as a module named "microread".
+
+The driver supports i2c interface only.
+
+The userspace application can use /dev/microread device to control the chip by
+HCI messages. In a typical scenario application will open() /dev/microread,
+reset the chip useing ioctl() interface then poll() the device to check if
+writing/reaing is possible.Finally, will read/write data (HCI) from/to the chip.
+
+Note that only one application can use the /dev/microread at the time.
+
+The driver waits for microread chip interrupt which occures if there are some
+data to be read. Then, poll() will indicate that fact to the userspace.
+
+The application can use ioctl(MICROREAD_IOC_RESET)to reset the hardware.
+
+
+Platform Data
+-------------
+
+The below platform data should be set when configuring board.
+
+struct microread_nfc_platform_data {
+	unsigned int rst_gpio;
+	unsigned int irq_gpio;
+	unsigned int ioh_gpio;
+	int (*request_hardware) (struct i2c_client *client);
+	void (*release_hardware) (void);
+};
+
diff --git a/drivers/nfc/Kconfig b/drivers/nfc/Kconfig
index ea15800..a805615 100644
--- a/drivers/nfc/Kconfig
+++ b/drivers/nfc/Kconfig
@@ -26,5 +26,15 @@ config PN544_NFC
 	  To compile this driver as a module, choose m here. The module will
 	  be called pn544.
 
+config MICROREAD_NFC
+	tristate "MICROREAD NFC driver"
+	depends on I2C
+	default n
+	---help---
+	  Say yes if you want Inside Secure Microread Near Field Communication
+	  driver. This is for i2c connected version. If unsure, say N here.
+
+	  To compile this driver as a module, choose m here. The module will
+	  be called microread.
 
 endif # NFC_DEVICES
diff --git a/drivers/nfc/Makefile b/drivers/nfc/Makefile
index a4efb16..974f5cb 100644
--- a/drivers/nfc/Makefile
+++ b/drivers/nfc/Makefile
@@ -3,3 +3,4 @@
 #
 
 obj-$(CONFIG_PN544_NFC)		+= pn544.o
+obj-$(CONFIG_MICROREAD_NFC)	+= microread.o
diff --git a/drivers/nfc/microread.c b/drivers/nfc/microread.c
new file mode 100644
index 0000000..bcbdbad
--- /dev/null
+++ b/drivers/nfc/microread.c
@@ -0,0 +1,465 @@
+/*
+ *  Driver for Microread NFC chip
+ *
+ *  Copyright (C) 2011 Tieto Poland
+ *
+ *  Author: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/interrupt.h>
+#include <linux/i2c.h>
+#include <linux/fs.h>
+#include <linux/slab.h>
+#include <linux/poll.h>
+#include <linux/gpio.h>
+#include <linux/delay.h>
+#include <linux/miscdevice.h>
+#include <linux/nfc/microread.h>
+#include <linux/uaccess.h>
+#include <linux/pm.h>
+
+#define MICROREAD_DEV_NAME	"microread"
+#define MICROREAD_MAX_BUF_SIZE	0x20
+
+#define MICROREAD_STATE_BUSY	0x00
+#define MICROREAD_STATE_READY	0x01
+
+struct microread_info {
+	struct i2c_client *i2c_dev;
+	struct miscdevice mdev;
+
+	u8 state;
+	u8 irq_state;
+	int irq;
+	u16 buflen;
+	u8 *buf;
+	wait_queue_head_t rx_waitq;
+	struct mutex rx_mutex;
+	struct mutex mutex;
+};
+
+static void microread_reset_hw(struct microread_nfc_platform_data *pdata)
+{
+	gpio_set_value(pdata->rst_gpio, 1);
+	usleep_range(1000, 2000);
+	gpio_set_value(pdata->rst_gpio, 0);
+	usleep_range(5000, 10000);
+}
+
+static u8 calc_lrc(u8 *buf, int len)
+{
+	int i;
+	u8 lrc = 0;
+	for (i = 0; i < len; i++)
+		lrc = lrc ^ buf[i];
+	return lrc;
+}
+
+static inline int microread_is_busy(struct microread_info *info)
+{
+	return (info->state == MICROREAD_STATE_BUSY);
+}
+
+static int microread_open(struct inode *inode, struct file *file)
+{
+	struct microread_info *info = container_of(file->private_data,
+					struct microread_info, mdev);
+	struct i2c_client *client = info->i2c_dev;
+	int ret = 0;
+
+	dev_vdbg(&client->dev, "%s: info: %p", __func__, info);
+
+	mutex_lock(&info->mutex);
+
+	if (microread_is_busy(info)) {
+		dev_err(&client->dev, "%s: info %p: device is busy.", __func__,
+				info);
+		ret = -EBUSY;
+		goto done;
+	}
+
+	info->state = MICROREAD_STATE_BUSY;
+done:
+	mutex_unlock(&info->mutex);
+	return ret;
+}
+
+static int microread_release(struct inode *inode, struct file *file)
+{
+	struct microread_info *info = container_of(file->private_data,
+					struct microread_info, mdev);
+	struct i2c_client *client = info->i2c_dev;
+
+	dev_vdbg(&client->dev, "%s: info: %p, client %p\n", __func__, info,
+					client);
+
+	mutex_lock(&info->mutex);
+	info->state = MICROREAD_STATE_READY;
+	mutex_unlock(&info->mutex);
+
+	return 0;
+}
+
+static ssize_t microread_read(struct file *file, char __user *buf, size_t count,
+								loff_t *f_pos)
+{
+	struct microread_info *info = container_of(file->private_data,
+					struct microread_info, mdev);
+	struct i2c_client *client = info->i2c_dev;
+	struct microread_nfc_platform_data *pdata =
+				dev_get_platdata(&client->dev);
+	int ret;
+	u8 len, lrc;
+
+	dev_dbg(&client->dev, "%s: info: %p, size: %d (bytes).", __func__,
+						info, count);
+	mutex_lock(&info->mutex);
+
+	if (!info->irq_state && !gpio_get_value(pdata->irq_gpio)) {
+		if (file->f_flags & O_NONBLOCK) {
+			ret = -EAGAIN;
+			goto done;
+		}
+
+		if (wait_event_interruptible(info->rx_waitq,
+				(info->irq_state == 1))) {
+			ret = -EINTR;
+			goto done;
+		}
+	}
+
+	if (count > info->buflen) {
+		dev_err(&client->dev, "%s: no enough space in read buffer.",
+				__func__);
+		ret = -ENOMEM;
+		goto done;
+	}
+
+	mutex_lock(&info->rx_mutex);
+	ret = i2c_master_recv(client, info->buf, info->buflen);
+	info->irq_state = 0;
+	mutex_unlock(&info->rx_mutex);
+
+	if (ret < 0) {
+		dev_err(&client->dev, "%s: i2c read (data) failed (err %d).",
+				__func__, ret);
+		ret = -EREMOTEIO;
+		goto done;
+	}
+
+#ifdef VERBOSE_DEBUG
+		print_hex_dump(KERN_DEBUG, "rx: ", DUMP_PREFIX_NONE,
+				16, 1, info->buf, ret, false);
+#endif
+	len = info->buf[0];
+
+	lrc = calc_lrc(info->buf, len + 1);
+	if (lrc != info->buf[len + 1]) {
+		dev_err(&client->dev, "%s: incorrect i2c frame.", __func__);
+		ret = -EFAULT;
+		goto done;
+	}
+
+	ret = len + 2;
+
+	if (copy_to_user(buf, info->buf, len + 2)) {
+		dev_err(&client->dev, "%s: error copying to user.", __func__);
+		ret = -EFAULT;
+		goto done;
+	}
+done:
+	mutex_unlock(&info->mutex);
+
+	return ret;
+}
+
+static ssize_t microread_write(struct file *file, const char __user *buf,
+			size_t count, loff_t *f_pos)
+{
+	struct microread_info *info = container_of(file->private_data,
+					struct microread_info, mdev);
+	struct i2c_client *client = info->i2c_dev;
+	int ret;
+	u16 len;
+
+	dev_dbg(&client->dev, "%s: info: %p, size %d (bytes).", __func__,
+					info, count);
+
+	if (count > info->buflen) {
+		dev_err(&client->dev, "%s: no enought space in TX buffer.",
+				__func__);
+		return -EINVAL;
+	}
+
+	len = min_t(u16, count, info->buflen);
+
+	mutex_lock(&info->mutex);
+	if (copy_from_user(info->buf, buf, len)) {
+		dev_err(&client->dev, "%s: error copying from user.",
+				__func__);
+		ret = -EFAULT;
+		goto done;
+	}
+
+#ifdef VERBOSE_DEBUG
+	print_hex_dump(KERN_DEBUG, "tx: ", DUMP_PREFIX_NONE, 16, 1, info->buf,
+			len, false);
+#endif
+	ret = i2c_master_send(client, info->buf, len);
+	if (ret < 0)
+		dev_err(&client->dev, "%s: i2c write failed (err %d).",
+			__func__, ret);
+
+done:
+	mutex_unlock(&info->mutex);
+	return ret;
+
+}
+
+static unsigned int microread_poll(struct file *file, poll_table *wait)
+{
+	struct microread_info *info = container_of(file->private_data,
+					struct microread_info, mdev);
+	struct i2c_client *client = info->i2c_dev;
+	int ret = (POLLOUT | POLLWRNORM);
+
+	dev_vdbg(&client->dev, "%s: info: %p client %p", __func__, info,
+			client);
+
+	mutex_lock(&info->mutex);
+	poll_wait(file, &info->rx_waitq, wait);
+
+	mutex_lock(&info->rx_mutex);
+	if (info->irq_state)
+		ret |= (POLLIN | POLLRDNORM);
+	mutex_unlock(&info->rx_mutex);
+	mutex_unlock(&info->mutex);
+
+	return ret;
+}
+
+static long microread_ioctl(struct file *file, unsigned int cmd,
+							unsigned long arg)
+{
+	struct microread_info *info = container_of(file->private_data,
+					struct microread_info, mdev);
+	struct i2c_client *client = info->i2c_dev;
+	struct microread_nfc_platform_data *pdata =
+				dev_get_platdata(&client->dev);
+	int ret = 0;
+
+	dev_dbg(&client->dev, "%s: info: %p cmd %d", __func__, info, cmd);
+
+	mutex_lock(&info->mutex);
+
+	switch (cmd) {
+	case MICROREAD_IOC_CONFIGURE:
+	case MICROREAD_IOC_CONNECT:
+		goto done;
+
+	case MICROREAD_IOC_RESET:
+		microread_reset_hw(pdata);
+		goto done;
+
+	default:
+		dev_err(&client->dev, "%s; not supported ioctl 0x%x", __func__,
+			cmd);
+		ret = -ENOIOCTLCMD;
+		goto done;
+	}
+
+done:
+	mutex_unlock(&info->mutex);
+	return ret;
+}
+
+const struct file_operations microread_fops = {
+	.owner		= THIS_MODULE,
+	.open		= microread_open,
+	.release	= microread_release,
+	.read		= microread_read,
+	.write		= microread_write,
+	.poll		= microread_poll,
+	.unlocked_ioctl	= microread_ioctl,
+};
+
+static irqreturn_t microread_irq(int irq, void *dev)
+{
+	struct microread_info *info = dev;
+	struct i2c_client *client = info->i2c_dev;
+
+	dev_dbg(&client->dev, "irq: info %p client %p ", info,	client);
+
+	if (irq != client->irq)
+		return IRQ_NONE;
+
+	mutex_lock(&info->rx_mutex);
+	info->irq_state = 1;
+	mutex_unlock(&info->rx_mutex);
+
+	wake_up_interruptible(&info->rx_waitq);
+
+	return IRQ_HANDLED;
+}
+
+static int __devinit microread_probe(struct i2c_client *client,
+				 const struct i2c_device_id *id)
+{
+	struct microread_info *info;
+	struct microread_nfc_platform_data *pdata =
+				dev_get_platdata(&client->dev);
+	int ret = 0;
+
+	dev_dbg(&client->dev, "%s: client %p", __func__, client);
+
+	if (!pdata) {
+		dev_err(&client->dev, "%s: client %p: missing platform data.",
+				__func__, client);
+		return -EINVAL;
+	}
+
+	info = kzalloc(sizeof(struct microread_info), GFP_KERNEL);
+	if (!info) {
+		dev_err(&client->dev, "%s: can't allocate microread_info.",
+				__func__);
+		return -ENOMEM;
+	}
+
+	info->buflen = (u16) MICROREAD_MAX_BUF_SIZE;
+	info->buf = kzalloc(info->buflen, GFP_KERNEL);
+	if (!info->buf) {
+		dev_err(&client->dev, "%s: can't allocate buf. (len %d)",
+			__func__, info->buflen);
+		ret = -ENOMEM;
+		goto free_info;
+	}
+
+	mutex_init(&info->mutex);
+	mutex_init(&info->rx_mutex);
+	init_waitqueue_head(&info->rx_waitq);
+	i2c_set_clientdata(client, info);
+	info->i2c_dev = client;
+	info->irq_state = 0;
+	info->state = MICROREAD_STATE_READY;
+
+	ret = request_threaded_irq(client->irq, NULL, microread_irq,
+		IRQF_SHARED|IRQF_TRIGGER_RISING, MICROREAD_DRIVER_NAME, info);
+	if (ret) {
+		dev_err(&client->dev, "%s: can't request irq. (ret %d, irq %d)",
+			__func__, ret, client->irq);
+		goto free_buf;
+	}
+
+	info->mdev.minor = MISC_DYNAMIC_MINOR;
+	info->mdev.name = MICROREAD_DEV_NAME;
+	info->mdev.fops = &microread_fops;
+	info->mdev.parent = &client->dev;
+
+	ret = misc_register(&info->mdev);
+	if (ret < 0) {
+		dev_err(&client->dev, "%s: register chr dev failed (ret %d)",
+			__func__, ret);
+			goto free_irq;
+	}
+
+	dev_info(&client->dev, "Probed.[/dev/%s]", MICROREAD_DEV_NAME);
+	return 0;
+
+free_irq:
+	free_irq(client->irq, info);
+free_buf:
+	kfree(info->buf);
+free_info:
+	kfree(info);
+
+	dev_info(&client->dev, "Not probed.");
+	return ret;
+}
+
+static __devexit int microread_remove(struct i2c_client *client)
+{
+	struct microread_info *info = i2c_get_clientdata(client);
+
+	dev_dbg(&client->dev, "%s", __func__);
+
+	misc_deregister(&info->mdev);
+	free_irq(client->irq, info);
+	kfree(info->buf);
+	kfree(info);
+
+	dev_info(&client->dev, "%s: Removed.", __func__);
+	return 0;
+}
+
+static int microread_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+	return -ENOSYS;
+}
+
+static int microread_resume(struct i2c_client *client)
+{
+	return -ENOSYS;
+}
+
+static struct i2c_device_id microread_id[] = {
+	{ MICROREAD_DRIVER_NAME, 0},
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, microread_id);
+
+static struct i2c_driver microread_driver = {
+	.driver = {
+		.name = MICROREAD_DRIVER_NAME,
+	},
+	.probe		= microread_probe,
+	.remove		= __devexit_p(microread_remove),
+	.suspend	= microread_suspend,
+	.resume		= microread_resume,
+	.id_table	= microread_id,
+};
+
+static int __init microread_init(void)
+{
+	int ret;
+
+	pr_debug(MICROREAD_DRIVER_NAME ": %s", __func__);
+
+	ret = i2c_add_driver(&microread_driver);
+	if (ret) {
+		pr_err(MICROREAD_DRIVER_NAME ": driver registration failed");
+		return ret;
+	}
+	pr_info(MICROREAD_DRIVER_NAME ": Loaded.");
+	return 0;
+}
+
+static void __exit microread_exit(void)
+{
+	pr_debug(MICROREAD_DRIVER_NAME ": %s", __func__);
+	i2c_del_driver(&microread_driver);
+	pr_info(MICROREAD_DRIVER_NAME ": Unloaded");
+}
+
+module_init(microread_init);
+module_exit(microread_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>");
+MODULE_DESCRIPTION("Driver for Microread NFC chip");
diff --git a/include/linux/nfc/microread.h b/include/linux/nfc/microread.h
new file mode 100644
index 0000000..a0ad68e
--- /dev/null
+++ b/include/linux/nfc/microread.h
@@ -0,0 +1,31 @@
+#ifndef _MICROREAD_H
+#define _MICROREAD_H
+
+#include <linux/i2c.h>
+
+#define MICROREAD_DRIVER_NAME	"microread"
+
+#define MICROREAD_IOC_MAGIC	'o'
+#define MICROREAD_IOC_MAX_CONFIG_LENGTH	16
+
+struct microread_ioc_configure {
+	int length;
+	unsigned char buffer[MICROREAD_IOC_MAX_CONFIG_LENGTH];
+};
+
+/* ioctl cmds */
+#define MICROREAD_IOC_CONFIGURE	_IOW(MICROREAD_IOC_MAGIC, 0, \
+					struct microread_ioc_configure)
+#define MICROREAD_IOC_CONNECT	_IO(MICROREAD_IOC_MAGIC, 1)
+#define MICROREAD_IOC_RESET	_IO(MICROREAD_IOC_MAGIC, 2)
+
+#ifdef __KERNEL__
+/* board config platform data for microread */
+struct microread_nfc_platform_data {
+	unsigned int rst_gpio;
+	unsigned int irq_gpio;
+	unsigned int ioh_gpio;
+};
+#endif /* __KERNEL__ */
+
+#endif /* _MICROREAD_H */
-- 
1.7.1

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Alan Cox]

On Fri, 18 Mar 2011 11:40:24 +0100
Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com> wrote:

> Add new driver for MicroRead NFC chip connected to i2c bus.

Ok we now have two devices and they have differing APIs and own device
names and both from the same company. This has to stop right now and the
existing one wants sorting out accordingly (while you are it fix the fact
any user can blow the kernel log away in that one by issuing bogus ioctls
at it, thats not a good thing)

NAK to this in its current form.

If we are going to have multiple nfc devices (and we are) then we need
a /dev/nfc%d device range to dump them in and we need some API
commonality.

Your API seems fairly sane (except your nfc-microread.txt needs to
document or point properly to the HCI messages in question

> +The application can use ioctl(MICROREAD_IOC_RESET)to reset the hardware.

And a reset is a generic sort of interface so we should probably have
NFC_IOC_RESET to go with /dev/nfc%d naming.

> +static int microread_open(struct inode *inode, struct file *file)
> +{
> +	struct microread_info *info = container_of(file->private_data,
> +					struct microread_info, mdev);
> +	struct i2c_client *client = info->i2c_dev;
> +	int ret = 0;
> +
> +	dev_vdbg(&client->dev, "%s: info: %p", __func__, info);
> +
> +	mutex_lock(&info->mutex);
> +
> +	if (microread_is_busy(info)) {
> +		dev_err(&client->dev, "%s: info %p: device is busy.", __func__,
> +				info);

So a process spinning trying to open it can spew all over the log - looks
bogus to me (similar problems in the existing driver)



> +	if (count > info->buflen) {
> +		dev_err(&client->dev, "%s: no enough space in read buffer.",
> +				__func__);
> +		ret = -ENOMEM;
> +		goto done;

More bogus log spewing and an odd error code for good measure

> +	lrc = calc_lrc(info->buf, len + 1);
> +	if (lrc != info->buf[len + 1]) {
> +		dev_err(&client->dev, "%s: incorrect i2c frame.", __func__);
> +		ret = -EFAULT;
> +		goto done;

So I can also spew all over the log by putting a deliberately busted
sender next to it.

> +	}
> +
> +	ret = len + 2;
> +
> +	if (copy_to_user(buf, info->buf, len + 2)) {
> +		dev_err(&client->dev, "%s: error copying to user.", __func__);
> +		ret = -EFAULT;

And another one.

> +static ssize_t microread_write(struct file *file, const char __user *buf,
> +			size_t count, loff_t *f_pos)
> +{
> +	struct microread_info *info = container_of(file->private_data,
> +					struct microread_info, mdev);
> +	struct i2c_client *client = info->i2c_dev;
> +	int ret;
> +	u16 len;
> +
> +	dev_dbg(&client->dev, "%s: info: %p, size %d (bytes).", __func__,
> +					info, count);
> +
> +	if (count > info->buflen) {
> +		dev_err(&client->dev, "%s: no enought space in TX buffer.",
> +				__func__);
> +		return -EINVAL;
> +	}

And another

> +
> +	len = min_t(u16, count, info->buflen);
> +
> +	mutex_lock(&info->mutex);
> +	if (copy_from_user(info->buf, buf, len)) {
> +		dev_err(&client->dev, "%s: error copying from user.",
> +				__func__);

Etc - these all want cleaning up


> +static unsigned int microread_poll(struct file *file, poll_table *wait)
> +{
> +	struct microread_info *info = container_of(file->private_data,
> +					struct microread_info, mdev);
> +	struct i2c_client *client = info->i2c_dev;
> +	int ret = (POLLOUT | POLLWRNORM);
> +
> +	dev_vdbg(&client->dev, "%s: info: %p client %p", __func__, info,
> +			client);
> +
> +	mutex_lock(&info->mutex);
> +	poll_wait(file, &info->rx_waitq, wait);
> +
> +	mutex_lock(&info->rx_mutex);
> +	if (info->irq_state)
> +		ret |= (POLLIN | POLLRDNORM);
> +	mutex_unlock(&info->rx_mutex);
> +	mutex_unlock(&info->mutex);

What is the intended behaviour on a reset while I am polling ?

> +static long microread_ioctl(struct file *file, unsigned int cmd,
> +							unsigned long arg)
> +{
> +	struct microread_info *info = container_of(file->private_data,
> +					struct microread_info, mdev);
> +	struct i2c_client *client = info->i2c_dev;
> +	struct microread_nfc_platform_data *pdata =
> +				dev_get_platdata(&client->dev);
> +	int ret = 0;
> +
> +	dev_dbg(&client->dev, "%s: info: %p cmd %d", __func__, info, cmd);
> +
> +	mutex_lock(&info->mutex);
> +
> +	switch (cmd) {
> +	case MICROREAD_IOC_CONFIGURE:
> +	case MICROREAD_IOC_CONNECT:
> +		goto done;

Ermm nope.. why do we have do nothing ioctls ?

> +	case MICROREAD_IOC_RESET:
> +		microread_reset_hw(pdata);

> +	default:
> +		dev_err(&client->dev, "%s; not supported ioctl 0x%x", __func__,
> +			cmd);

And more spewage


> +static irqreturn_t microread_irq(int irq, void *dev)
> +{
> +	struct microread_info *info = dev;
> +	struct i2c_client *client = info->i2c_dev;
> +
> +	dev_dbg(&client->dev, "irq: info %p client %p ", info,	client);
> +
> +	if (irq != client->irq)
> +		return IRQ_NONE;

How can this occur - why is this test needed ????

> +
> +	mutex_lock(&info->rx_mutex);
> +	info->irq_state = 1;
> +	mutex_unlock(&info->rx_mutex);

Would it not be lighter to use atomic bit ops ?


> +	info->mdev.minor = MISC_DYNAMIC_MINOR;
> +	info->mdev.name = MICROREAD_DEV_NAME;
> +	info->mdev.fops = &microread_fops;
> +	info->mdev.parent = &client->dev;
> +
> +	ret = misc_register(&info->mdev);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "%s: register chr dev failed (ret %d)",
> +			__func__, ret);
> +			goto free_irq;
> +	}

And at this point you want a thing to hand out nfc%d names not to use
misc device with random per device API. The same app ought to be able to
work with many nfc readers.

> +static int microread_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> +	return -ENOSYS;
> +}
> +
> +static int microread_resume(struct i2c_client *client)
> +{
> +	return -ENOSYS;
> +}

So why provide them ??


Alan

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]

Hi Waldemar,

On Fri, Mar 18, 2011 at 11:40:24AM +0100, Waldemar Rymarkiewicz wrote:
> Add new driver for MicroRead NFC chip connected to i2c bus.
> 
> See Documentation/nfc/nfc-microread.txt.
> 
> Signed-off-by: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>

Besides the stuff Alan mentioned...

> +struct microread_nfc_platform_data {
> +	unsigned int rst_gpio;
> +	unsigned int irq_gpio;
> +	unsigned int ioh_gpio;

... you should request and setup the GPIOs before using them.


> +free_irq:
> +	free_irq(client->irq, info);
> +free_buf:
> +	kfree(info->buf);
> +free_info:
> +	kfree(info);
> +
> +	dev_info(&client->dev, "Not probed.");
> +	return ret;
> +}

When respinning, you could consider using managed devices (devm_*);
this error path could completely go then.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Arnd Bergmann]

On Friday 18 March 2011, Waldemar Rymarkiewicz wrote:
> Add new driver for MicroRead NFC chip connected to i2c bus.
> 
> See Documentation/nfc/nfc-microread.txt.

As I said in my first review and Alan also pointed out now, the
most important change will be to add a common NFC core layer,
before adding more hardware drivers.

Also, regarding the user interface, we need to be really sure
that this is the best way of talking to NFC devices. The interface
you have today is a simple character device read/write kind,
which may be the best thing if the protocol stack on top is
really simple and there is never the need to have multiple
applications talking to different endpoints on the wireless
interface, and if there are no protocol headers being
send over the character device interface.

Otherwise, a better interface is probably to add a new network
socket family and abstract the protocol layers in the kernel.
Can you explain in more depth what the kind of data is on
this interface?

A few more things I noticed when reading through the driver
again:

> +const struct file_operations microread_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= microread_open,
> +	.release	= microread_release,
> +	.read		= microread_read,
> +	.write		= microread_write,
> +	.poll		= microread_poll,
> +	.unlocked_ioctl	= microread_ioctl,
> +};

As I said, all the file operations need to move to the core module.
Also, this is missing a compat_ioctl function.

> +static irqreturn_t microread_irq(int irq, void *dev)
> +{
> +	struct microread_info *info = dev;
> +	struct i2c_client *client = info->i2c_dev;
> +
> +	dev_dbg(&client->dev, "irq: info %p client %p ", info,	client);
> +
> +	if (irq != client->irq)
> +		return IRQ_NONE;
> +
> +	mutex_lock(&info->rx_mutex);
> +	info->irq_state = 1;
> +	mutex_unlock(&info->rx_mutex);
> +
> +	wake_up_interruptible(&info->rx_waitq);
> +
> +	return IRQ_HANDLED;
> +}

You cannot take a mutex from interrupt context, that may
cause deadlocks.


> diff --git a/include/linux/nfc/microread.h b/include/linux/nfc/microread.h
> new file mode 100644
> index 0000000..a0ad68e
> --- /dev/null
> +++ b/include/linux/nfc/microread.h

Please split this header file into an include/linux/nfc.h file
and the file with the platform data in include/linux/platform_data/microread.h

Do not define any ioctls that are not used.

	Arnd

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Mark Brown]

On Fri, Mar 18, 2011 at 01:19:53PM +0100, Arnd Bergmann wrote:
> On Friday 18 March 2011, Waldemar Rymarkiewicz wrote:

> > +
> > +	mutex_lock(&info->rx_mutex);
> > +	info->irq_state = 1;
> > +	mutex_unlock(&info->rx_mutex);
> > +
> > +	wake_up_interruptible(&info->rx_waitq);
> > +
> > +	return IRQ_HANDLED;
> > +}

> You cannot take a mutex from interrupt context, that may
> cause deadlocks.

This is a threaded IRQ handler so mutexes are fine.

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Arnd Bergmann]

On Friday 18 March 2011, Mark Brown wrote:
> On Fri, Mar 18, 2011 at 01:19:53PM +0100, Arnd Bergmann wrote:
> > On Friday 18 March 2011, Waldemar Rymarkiewicz wrote:
> 
> > > +
> > > +   mutex_lock(&info->rx_mutex);
> > > +   info->irq_state = 1;
> > > +   mutex_unlock(&info->rx_mutex);
> > > +
> > > +   wake_up_interruptible(&info->rx_waitq);
> > > +
> > > +   return IRQ_HANDLED;
> > > +}
> 
> > You cannot take a mutex from interrupt context, that may
> > cause deadlocks.
> 
> This is a threaded IRQ handler so mutexes are fine.

Ah, right. I've never seen one of these used in the field,
so I didn't think of this.

Looking at the mutexes though: The read function does
not hold the rx_mutex when reading the irq_state
variable, so that is potentially racy.

The read function seems to have another problem regarding
the user space buffer: it bails out if the provided buffer
is larger than the available data, which is pointless,
but it does not check if the user buffer is too short.

	Arnd

RE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar.Rymarkiewicz]

Alan,

>Ok we now have two devices and they have differing APIs and 
>own device names and both from the same company. This has to 
>stop right now and the existing one wants sorting out 
>accordingly (while you are it fix the fact any user can blow 
>the kernel log away in that one by issuing bogus ioctls at it, 
>thats not a good thing)
>
>NAK to this in its current form.
>
>If we are going to have multiple nfc devices (and we are) then 
>we need a /dev/nfc%d device range to dump them in and we need 
>some API commonality.
>
>Your API seems fairly sane (except your nfc-microread.txt 
>needs to document or point properly to the HCI messages in question
>
>> +The application can use ioctl(MICROREAD_IOC_RESET)to reset 
>the hardware.
>
>And a reset is a generic sort of interface so we should 
>probably have NFC_IOC_RESET to go with /dev/nfc%d naming.


I see your point and agree with your and Arnd's opinion on that concept. Will, then, think how to approach to this and try to propose a resonable skeleton first.



>> +	if (microread_is_busy(info)) {
>> +		dev_err(&client->dev, "%s: info %p: device is 
>busy.", __func__,
>> +				info);
>
>So a process spinning trying to open it can spew all over the 
>log - looks bogus to me (similar problems in the existing driver)


All potential spewing logs have been removed.

>
>What is the intended behaviour on a reset while I am polling ?

Good question :|.  I will answare soon.
 

>
>Ermm nope.. why do we have do nothing ioctls ?
>

onfc stack requires those ones, but they are only valid for a specific test enviroment.
This should not be a case for driver and the stack should care about it if it needs this. Then will remove it.


>> +	if (irq != client->irq)
>> +		return IRQ_NONE;
>
>How can this occur - why is this test needed ????


It's not needed. Removed.


>> +
>> +	mutex_lock(&info->rx_mutex);
>> +	info->irq_state = 1;
>> +	mutex_unlock(&info->rx_mutex);
>
>Would it not be lighter to use atomic bit ops ?

Do you mean  in order to remove rx_mutex? 

mutex_lock(&info->rx_mutex);
atomic_set(info->irq_state ,1);
mutex_unlock(&info->rx_mutex);

looks a bit strange. I still need the rx_mutex to protect irq_state while reading i2c.

	mutex_lock(&info->rx_mutex);
	ret = i2c_master_recv(client, info->buf, info->buflen);
	info->irq_state = 0;
	mutex_unlock(&info->rx_mutex);



>> +	info->mdev.minor = MISC_DYNAMIC_MINOR;
>> +	info->mdev.name = MICROREAD_DEV_NAME;
>> +	info->mdev.fops = &microread_fops;
>> +	info->mdev.parent = &client->dev;
>> +
>> +	ret = misc_register(&info->mdev);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "%s: register chr dev 
>failed (ret %d)",
>> +			__func__, ret);
>> +			goto free_irq;
>> +	}
>
>And at this point you want a thing to hand out nfc%d names not 
>to use misc device with random per device API. The same app 
>ought to be able to work with many nfc readers.

Sure. Will fix this as well.


>> +static int microread_suspend(struct i2c_client *client, 
>pm_message_t 
>> +mesg) {
>> +	return -ENOSYS;
>> +}
>> +
>> +static int microread_resume(struct i2c_client *client) {
>> +	return -ENOSYS;
>> +}
>
>So why provide them ??

I've supposed to implement later on (need more hw specific input on that topic). Anyway... 
I can add this when it's completed at all.  Removed for now. 

>-----Original Message-----
>From: Alan Cox [mailto:alan@lxorguk.ukuu.org.uk] 
>Sent: Friday, March 18, 2011 12:04 PM
>To: Rymarkiewicz Waldemar
>Cc: linux-i2c@vger.kernel.org; arnd@arndb.de; 
>sameo@linux.intel.com; linux-kernel@vger.kernel.org; 
>hthebaud@insidefr.com; matti.j.aaltonen@nokia.com
>Subject: Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip
>
>On Fri, 18 Mar 2011 11:40:24 +0100
>Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com> wrote:
>
>> Add new driver for MicroRead NFC chip connected to i2c bus.
>
>Ok we now have two devices and they have differing APIs and 
>own device names and both from the same company. This has to 
>stop right now and the existing one wants sorting out 
>accordingly (while you are it fix the fact any user can blow 
>the kernel log away in that one by issuing bogus ioctls at it, 
>thats not a good thing)
>
>NAK to this in its current form.
>
>If we are going to have multiple nfc devices (and we are) then 
>we need a /dev/nfc%d device range to dump them in and we need 
>some API commonality.
>
>Your API seems fairly sane (except your nfc-microread.txt 
>needs to document or point properly to the HCI messages in question
>
>> +The application can use ioctl(MICROREAD_IOC_RESET)to reset 
>the hardware.
>
>And a reset is a generic sort of interface so we should 
>probably have NFC_IOC_RESET to go with /dev/nfc%d naming.
>
>> +static int microread_open(struct inode *inode, struct file *file) {
>> +	struct microread_info *info = container_of(file->private_data,
>> +					struct microread_info, mdev);
>> +	struct i2c_client *client = info->i2c_dev;
>> +	int ret = 0;
>> +
>> +	dev_vdbg(&client->dev, "%s: info: %p", __func__, info);
>> +
>> +	mutex_lock(&info->mutex);
>> +
>> +	if (microread_is_busy(info)) {
>> +		dev_err(&client->dev, "%s: info %p: device is 
>busy.", __func__,
>> +				info);
>
>So a process spinning trying to open it can spew all over the 
>log - looks bogus to me (similar problems in the existing driver)
>
>
>
>> +	if (count > info->buflen) {
>> +		dev_err(&client->dev, "%s: no enough space in 
>read buffer.",
>> +				__func__);
>> +		ret = -ENOMEM;
>> +		goto done;
>
>More bogus log spewing and an odd error code for good measure
>
>> +	lrc = calc_lrc(info->buf, len + 1);
>> +	if (lrc != info->buf[len + 1]) {
>> +		dev_err(&client->dev, "%s: incorrect i2c 
>frame.", __func__);
>> +		ret = -EFAULT;
>> +		goto done;
>
>So I can also spew all over the log by putting a deliberately 
>busted sender next to it.
>
>> +	}
>> +
>> +	ret = len + 2;
>> +
>> +	if (copy_to_user(buf, info->buf, len + 2)) {
>> +		dev_err(&client->dev, "%s: error copying to 
>user.", __func__);
>> +		ret = -EFAULT;
>
>And another one.
>
>> +static ssize_t microread_write(struct file *file, const 
>char __user *buf,
>> +			size_t count, loff_t *f_pos)
>> +{
>> +	struct microread_info *info = container_of(file->private_data,
>> +					struct microread_info, mdev);
>> +	struct i2c_client *client = info->i2c_dev;
>> +	int ret;
>> +	u16 len;
>> +
>> +	dev_dbg(&client->dev, "%s: info: %p, size %d (bytes).", 
>__func__,
>> +					info, count);
>> +
>> +	if (count > info->buflen) {
>> +		dev_err(&client->dev, "%s: no enought space in 
>TX buffer.",
>> +				__func__);
>> +		return -EINVAL;
>> +	}
>
>And another
>
>> +
>> +	len = min_t(u16, count, info->buflen);
>> +
>> +	mutex_lock(&info->mutex);
>> +	if (copy_from_user(info->buf, buf, len)) {
>> +		dev_err(&client->dev, "%s: error copying from user.",
>> +				__func__);
>
>Etc - these all want cleaning up
>
>
>> +static unsigned int microread_poll(struct file *file, poll_table 
>> +*wait) {
>> +	struct microread_info *info = container_of(file->private_data,
>> +					struct microread_info, mdev);
>> +	struct i2c_client *client = info->i2c_dev;
>> +	int ret = (POLLOUT | POLLWRNORM);
>> +
>> +	dev_vdbg(&client->dev, "%s: info: %p client %p", __func__, info,
>> +			client);
>> +
>> +	mutex_lock(&info->mutex);
>> +	poll_wait(file, &info->rx_waitq, wait);
>> +
>> +	mutex_lock(&info->rx_mutex);
>> +	if (info->irq_state)
>> +		ret |= (POLLIN | POLLRDNORM);
>> +	mutex_unlock(&info->rx_mutex);
>> +	mutex_unlock(&info->mutex);
>
>What is the intended behaviour on a reset while I am polling ?
>
>> +static long microread_ioctl(struct file *file, unsigned int cmd,
>> +							
>unsigned long arg)
>> +{
>> +	struct microread_info *info = container_of(file->private_data,
>> +					struct microread_info, mdev);
>> +	struct i2c_client *client = info->i2c_dev;
>> +	struct microread_nfc_platform_data *pdata =
>> +				dev_get_platdata(&client->dev);
>> +	int ret = 0;
>> +
>> +	dev_dbg(&client->dev, "%s: info: %p cmd %d", __func__, 
>info, cmd);
>> +
>> +	mutex_lock(&info->mutex);
>> +
>> +	switch (cmd) {
>> +	case MICROREAD_IOC_CONFIGURE:
>> +	case MICROREAD_IOC_CONNECT:
>> +		goto done;
>
>Ermm nope.. why do we have do nothing ioctls ?
>
>> +	case MICROREAD_IOC_RESET:
>> +		microread_reset_hw(pdata);
>
>> +	default:
>> +		dev_err(&client->dev, "%s; not supported ioctl 
>0x%x", __func__,
>> +			cmd);
>
>And more spewage
>
>
>> +static irqreturn_t microread_irq(int irq, void *dev) {
>> +	struct microread_info *info = dev;
>> +	struct i2c_client *client = info->i2c_dev;
>> +
>> +	dev_dbg(&client->dev, "irq: info %p client %p ", info,	client);
>> +
>> +	if (irq != client->irq)
>> +		return IRQ_NONE;
>
>How can this occur - why is this test needed ????
>
>> +
>> +	mutex_lock(&info->rx_mutex);
>> +	info->irq_state = 1;
>> +	mutex_unlock(&info->rx_mutex);
>
>Would it not be lighter to use atomic bit ops ?
>
>
>> +	info->mdev.minor = MISC_DYNAMIC_MINOR;
>> +	info->mdev.name = MICROREAD_DEV_NAME;
>> +	info->mdev.fops = &microread_fops;
>> +	info->mdev.parent = &client->dev;
>> +
>> +	ret = misc_register(&info->mdev);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "%s: register chr dev 
>failed (ret %d)",
>> +			__func__, ret);
>> +			goto free_irq;
>> +	}
>
>And at this point you want a thing to hand out nfc%d names not 
>to use misc device with random per device API. The same app 
>ought to be able to work with many nfc readers.
>
>> +static int microread_suspend(struct i2c_client *client, 
>pm_message_t 
>> +mesg) {
>> +	return -ENOSYS;
>> +}
>> +
>> +static int microread_resume(struct i2c_client *client) {
>> +	return -ENOSYS;
>> +}
>
>So why provide them ??
>
>
>Alan
>

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Arnd Bergmann]

On Friday 18 March 2011, Waldemar.Rymarkiewicz@tieto.com wrote:
> >
> >Ermm nope.. why do we have do nothing ioctls ?
> >
> 
> onfc stack requires those ones, but they are only valid for a specific test enviroment.
> This should not be a case for driver and the stack should care about it if it needs
> this. Then will remove it.

The way this normally works is to figure out the correct
way to implement the kernel driver first and then write
a user space stack around it, not the other way round...
  
> >> +
> >> +    mutex_lock(&info->rx_mutex);
> >> +    info->irq_state = 1;
> >> +    mutex_unlock(&info->rx_mutex);
> >
> >Would it not be lighter to use atomic bit ops ?
> 
> Do you mean  in order to remove rx_mutex? 
> 
> mutex_lock(&info->rx_mutex);
> atomic_set(info->irq_state ,1);
> mutex_unlock(&info->rx_mutex);
> 
> looks a bit strange. I still need the rx_mutex to protect irq_state while reading i2c.
> 
>         mutex_lock(&info->rx_mutex);
>         ret = i2c_master_recv(client, info->buf, info->buflen);
>         info->irq_state = 0;
>         mutex_unlock(&info->rx_mutex);

What Alan meant was set_bit/test_and_clear_bit here, not atomic_*().
As I mentioned in the other mail, the way you check the flag is
probably still racy with the current mutex use.

Note that the interrupt handler doesn't do much at all, so it's probably
a good idea to use a regular (non-threaded) handler here to reduce
the overhead, once you have removed the mutex.

	Arnd

RE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar.Rymarkiewicz]

Wolfram,

>> +	unsigned int irq_gpio;
>> +	unsigned int ioh_gpio;
>
>... you should request and setup the GPIOs before using them.

I assume the board specific code will do so. 


Waldek

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Alan Cox]

> >Would it not be lighter to use atomic bit ops ?
> 
> Do you mean  in order to remove rx_mutex? 
> 
> mutex_lock(&info->rx_mutex);
> atomic_set(info->irq_state ,1);
> mutex_unlock(&info->rx_mutex);
> 
> looks a bit strange. I still need the rx_mutex to protect irq_state while reading i2c.
> 
> 	mutex_lock(&info->rx_mutex);
> 	ret = i2c_master_recv(client, info->buf, info->buflen);
> 	info->irq_state = 0;
> 	mutex_unlock(&info->rx_mutex);
> 

I was thinking clear_bit/test_and_set_bit rather than atomic_t operations.

ie in the IRQ

	clear_bit(0, &info->irq_state);


in the main path

	if (test_and_set_bit(0, &info->state))
		i2c_master_recv(...)

but if the mutex is needed anyway it doesn't help make the code saner.

RE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar.Rymarkiewicz]

>I was thinking clear_bit/test_and_set_bit rather than atomic_t 
>operations.
>

I see, then I will for it.


Waldek

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Mark Brown]

On Fri, Mar 18, 2011 at 05:08:39PM +0200, Waldemar.Rymarkiewicz@tieto.com wrote:
> Wolfram,

> >> +	unsigned int irq_gpio;
> >> +	unsigned int ioh_gpio;

> >... you should request and setup the GPIOs before using them.

> I assume the board specific code will do so. 

The usual pattern is that the code which uses the GPIO requests it,
otherwise all the users just end up having to replicate the setup and
teardown code for them.

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Randy Dunlap]

On Fri, 18 Mar 2011 11:40:24 +0100 Waldemar Rymarkiewicz wrote:

> Add new driver for MicroRead NFC chip connected to i2c bus.
> 
> See Documentation/nfc/nfc-microread.txt.
> 
> Signed-off-by: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>
> ---
>  Documentation/nfc/nfc-microread.txt |   46 ++++
>  drivers/nfc/Kconfig                 |   10 +
>  drivers/nfc/Makefile                |    1 +
>  drivers/nfc/microread.c             |  465 +++++++++++++++++++++++++++++++++++
>  include/linux/nfc/microread.h       |   31 +++
>  5 files changed, 553 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/nfc/nfc-microread.txt
>  create mode 100644 drivers/nfc/microread.c
>  create mode 100644 include/linux/nfc/microread.h
> 
> diff --git a/Documentation/nfc/nfc-microread.txt b/Documentation/nfc/nfc-microread.txt
> new file mode 100644
> index 0000000..e15c49c
> --- /dev/null
> +++ b/Documentation/nfc/nfc-microread.txt
> @@ -0,0 +1,46 @@
> +Kernel driver for Inside Secure MicroRead Near Field Communication chip
> +
> +General
> +-------
> +
> +microread is the RF chip that uses Near Field Communication (NFC) for proximity
> +transactions and interactions.
> +
> +Please visit Inside Secure webside for microread specification:

                              website (or web site)

> +http://www.insidesecure.com/eng/Products/NFC-Products/MicroRead
> +
> +
> +The Driver
> +----------
> +
> +The microread driver can be found under drivers/nfc/microread.c and it's
> +compiled as a module named "microread".
> +
> +The driver supports i2c interface only.
> +
> +The userspace application can use /dev/microread device to control the chip by
> +HCI messages. In a typical scenario application will open() /dev/microread,
> +reset the chip useing ioctl() interface then poll() the device to check if

                  using

> +writing/reaing is possible.Finally, will read/write data (HCI) from/to the chip.

           reading is possible. Finally, it will

> +
> +Note that only one application can use the /dev/microread at the time.
> +
> +The driver waits for microread chip interrupt which occures if there are some

                                                       occurs

> +data to be read. Then, poll() will indicate that fact to the userspace.
> +
> +The application can use ioctl(MICROREAD_IOC_RESET)to reset the hardware.

                                              _RESET) to reset

> +
> +
> +Platform Data
> +-------------
> +
> +The below platform data should be set when configuring board.
> +
> +struct microread_nfc_platform_data {
> +	unsigned int rst_gpio;
> +	unsigned int irq_gpio;
> +	unsigned int ioh_gpio;
> +	int (*request_hardware) (struct i2c_client *client);
> +	void (*release_hardware) (void);
> +};
> +


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

RE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar.Rymarkiewicz]

Randy, 

>> +data to be read. Then, poll() will indicate that fact to 
>the userspace.
>> +
>> +The application can use ioctl(MICROREAD_IOC_RESET)to reset 
>the hardware.
>
>                                              _RESET) to reset
>

Thanks, all are now fixed. 

Waldek

RE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar.Rymarkiewicz]

>> +free_irq:
>> +	free_irq(client->irq, info);
>> +free_buf:
>> +	kfree(info->buf);
>> +free_info:
>> +	kfree(info);
>> +
>> +	dev_info(&client->dev, "Not probed.");
>> +	return ret;
>> +}
>
>When respinning, you could consider using managed devices 
>(devm_*); this error path could completely go then.

Well, to be honest I did't know about that 'feature' :), but now I see it's pretty nice. Will start to use it.

Thanks,
Waldek

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Samuel Ortiz]

On Fri, Mar 18, 2011 at 01:19:53PM +0100, Arnd Bergmann wrote:
> On Friday 18 March 2011, Waldemar Rymarkiewicz wrote:
> > Add new driver for MicroRead NFC chip connected to i2c bus.
> > 
> > See Documentation/nfc/nfc-microread.txt.
> 
> As I said in my first review and Alan also pointed out now, the
> most important change will be to add a common NFC core layer,
> before adding more hardware drivers.
> 
> Also, regarding the user interface, we need to be really sure
> that this is the best way of talking to NFC devices. The interface
> you have today is a simple character device read/write kind,
> which may be the best thing if the protocol stack on top is
> really simple and there is never the need to have multiple
> applications talking to different endpoints on the wireless
> interface, and if there are no protocol headers being
> send over the character device interface.
> 
> Otherwise, a better interface is probably to add a new network
> socket family and abstract the protocol layers in the kernel.

Yes, NFC seems to be a good fit for a new socket family. Especially if we ever
want to have a proper NFC p2p support from the kernel.
Sending HCI commands should be done through a dedicated netlink socket too.

I am currently strting to work on such solution, and I hope to be able to come
up with a basic prototype for it in a few weeks.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

RE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar.Rymarkiewicz]

>Yes, NFC seems to be a good fit for a new socket family. 
>Especially if we ever want to have a proper NFC p2p support 
>from the kernel.
>Sending HCI commands should be done through a dedicated 
>netlink socket too.
>
>I am currently strting to work on such solution, and I hope to 
>be able to come up with a basic prototype for it in a few weeks.

What about common drivers interface in this case.  Should we go for common /dev/nfcX interface as well?

Waldek

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Arnd Bergmann]

On Tuesday 29 March 2011, Waldemar.Rymarkiewicz@tieto.com wrote:
> >Yes, NFC seems to be a good fit for a new socket family. 
> >Especially if we ever want to have a proper NFC p2p support 
> >from the kernel.
> >Sending HCI commands should be done through a dedicated 
> >netlink socket too.
> >
> >I am currently strting to work on such solution, and I hope to 
> >be able to come up with a basic prototype for it in a few weeks.
> 
> What about common drivers interface in this case.
> Should we go for common /dev/nfcX interface as well?

I fear there can only be one. A good implementation of a socket
interface would mean that there is no need for a character device.

The difference between the two is where you keep the common
NFC logic:

If you have a character device, it will be like a serial port
connecting to a modem. Any higher-level protocols live in the
user space and are limited to a single application then, which
is required to have appropriate priviledges to open the device.

In contrast, a socket implementation puts the protocol
stack into the kernel, which requires much more kernel code
but almost no user space library code, aside from perhaps
a small shim layer. It makes it possible to have multiple
applications and/or users concurrently use NFC to make connections
to separate endpoints. Since sockets have no implicit permission
handling, the kernel code then needs to implement a way to enforce
policy.

I still don't understand enough about NFC to judge which of
the two is better suited for the problem, but my feeling is
that a socket based implementation would be better if you expect
a lot of people to use it, while the main advantage of the
character device is its simplicity, so that would be preferred
if you only expect a very small set of possible applications
for this.

	Arnd

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Alan Cox]

> The difference between the two is where you keep the common
> NFC logic:

I think I'd disagree on that
> 
> If you have a character device, it will be like a serial port
> connecting to a modem. Any higher-level protocols live in the
> user space and are limited to a single application then, which
> is required to have appropriate priviledges to open the device.

A socket is just an API just as a file, you can put the stack in either
place in either case.

> character device is its simplicity, so that would be preferred
> if you only expect a very small set of possible applications
> for this.

NFC is not particularly performance dependant so having a lot of the
stack in a daemon isn't really going to hurt anything too much on a
client embedded device/phone.

The bigger question is probably what it needs to look like the other end
- ie the server side embedded devices doing transactions.

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Arnd Bergmann]

On Tuesday 29 March 2011, Alan Cox wrote:
> > The difference between the two is where you keep the common
> > NFC logic:
> 
> I think I'd disagree on that
> > 
> > If you have a character device, it will be like a serial port
> > connecting to a modem. Any higher-level protocols live in the
> > user space and are limited to a single application then, which
> > is required to have appropriate priviledges to open the device.
> 
> A socket is just an API just as a file, you can put the stack in either
> place in either case.

Fair enough. Obviously the character device that we've been talking
about here does not include the stack, but that would be another option.

As you say, using a socket with a dumb interface is also possible,
but that sounds to me like a combination of all the disadvantages.

> > character device is its simplicity, so that would be preferred
> > if you only expect a very small set of possible applications
> > for this.
> 
> NFC is not particularly performance dependant so having a lot of the
> stack in a daemon isn't really going to hurt anything too much on a
> client embedded device/phone.
> 
> The bigger question is probably what it needs to look like the other end
> - ie the server side embedded devices doing transactions.

I was under the impression that NFC was peer-to-peer, so the driver
would already be handling both sides potentially.

	Arnd

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Alan Cox]

> I was under the impression that NFC was peer-to-peer, so the driver
> would already be handling both sides potentially.

Your security requirements each side are going to be very different. A
phone or handheld device is generally single user at a time, the other
end may well be interacting with many devices at once each with their own
security context and potentially handling sensitive data (payment
authorisations for example).

Alan

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Arnd Bergmann]

On Tuesday 29 March 2011, Alan Cox wrote:
> > I was under the impression that NFC was peer-to-peer, so the driver
> > would already be handling both sides potentially.
> 
> Your security requirements each side are going to be very different. A
> phone or handheld device is generally single user at a time, the other
> end may well be interacting with many devices at once each with their own
> security context and potentially handling sensitive data (payment
> authorisations for example).

Yes, good point.

	Arnd

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Samuel Ortiz]

On Tue, Mar 29, 2011 at 01:05:02PM +0200, Arnd Bergmann wrote:
> On Tuesday 29 March 2011, Waldemar.Rymarkiewicz@tieto.com wrote:
> > >Yes, NFC seems to be a good fit for a new socket family. 
> > >Especially if we ever want to have a proper NFC p2p support 
> > >from the kernel.
> > >Sending HCI commands should be done through a dedicated 
> > >netlink socket too.
> > >
> > >I am currently strting to work on such solution, and I hope to 
> > >be able to come up with a basic prototype for it in a few weeks.
> > 
> > What about common drivers interface in this case.
> > Should we go for common /dev/nfcX interface as well?
> 
> I fear there can only be one. A good implementation of a socket
> interface would mean that there is no need for a character device.
My idea of an initial NFC subsystem architecture was actually the following
one:
- A core NFC layer against which NFC drivers would register.
- A netlink socket for handling the HCI commands. That would put a big part of
the NFC HCI layer in kernel land and could potentially simplify the existing
NFC stacks.
- A socket family for the LLCP abstraction, a.k.a. NFC peer to peer mode.

We can start working on the first 2 items and leave the last one as a future
enhancement, since what NFC is currently mostly used for is tag
reading/writing and smartcard emulation.

Basically, we'd replace the current character device option with a netlink
one, allowing for a single kernel entry point for multiple applications
willing to do NFC.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

RE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar.Rymarkiewicz]

>My idea of an initial NFC subsystem architecture was actually 
>the following
>one:
>- A core NFC layer against which NFC drivers would register.
>- A netlink socket for handling the HCI commands. That would 
>put a big part of the NFC HCI layer in kernel land and could 
>potentially simplify the existing NFC stacks.

Shouldn't be better to add a new AF_NFC sock family and then register new LLCP and HCI protocols?

Waldek

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Arnd Bergmann]

On Thursday 31 March 2011, Waldemar.Rymarkiewicz@tieto.com wrote:
> >My idea of an initial NFC subsystem architecture was actually 
> >the following
> >one:
> >- A core NFC layer against which NFC drivers would register.
> >- A netlink socket for handling the HCI commands. That would 
> >put a big part of the NFC HCI layer in kernel land and could 
> >potentially simplify the existing NFC stacks.
> 
> Shouldn't be better to add a new AF_NFC sock family and then
> register new LLCP and HCI protocols?

That depends on what the HCI protocol really looks like, e.g.
is it's related to the LLCP data at all or not.

Another option might be to control the HCI over setsockopts,
but the netlink socket sounds more flexible for this.

	Arnd

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Samuel Ortiz]

Hi Waldemar,

On Thu, Mar 31, 2011 at 05:42:21PM +0300, Waldemar.Rymarkiewicz@tieto.com wrote:
> >My idea of an initial NFC subsystem architecture was actually 
> >the following
> >one:
> >- A core NFC layer against which NFC drivers would register.
> >- A netlink socket for handling the HCI commands. That would 
> >put a big part of the NFC HCI layer in kernel land and could 
> >potentially simplify the existing NFC stacks.
> 
> Shouldn't be better to add a new AF_NFC sock family and then register new LLCP and HCI protocols?
> 
That's the Bluetooth approach, and it works just fine as well.
However, the netlink option for sending commands and receiving answers to/from
a subsystem sounds more appropriate, at least to me.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

RE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar.Rymarkiewicz]

Hi Samuel,

>That's the Bluetooth approach, and it works just fine as well.
>However, the netlink option for sending commands and receiving 
>answers to/from a subsystem sounds more appropriate, at least to me.

I haven't used netlink in practice, so just wondering why netlink. I assume, then, it's more suitable :)

Waldek

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Samuel Ortiz]

On Thu, Mar 31, 2011 at 06:24:50PM +0300, Waldemar.Rymarkiewicz@tieto.com wrote:
> Hi Samuel,
> 
> >That's the Bluetooth approach, and it works just fine as well.
> >However, the netlink option for sending commands and receiving 
> >answers to/from a subsystem sounds more appropriate, at least to me.
> 
> I haven't used netlink in practice, so just wondering why netlink. I assume, then, it's more suitable :)
>
Have a look at the wireless nl80211.c implementation. It's a good example for
that sort of usage.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Aloisio Almeida]

Hi all,

On Thu, Mar 31, 2011 at 12:30 PM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> On Thu, Mar 31, 2011 at 06:24:50PM +0300, Waldemar.Rymarkiewicz@tieto.com wrote:
>> Hi Samuel,
>>
>> >That's the Bluetooth approach, and it works just fine as well.
>> >However, the netlink option for sending commands and receiving
>> >answers to/from a subsystem sounds more appropriate, at least to me.
>>
>> I haven't used netlink in practice, so just wondering why netlink. I assume, then, it's more suitable :)
>>
> Have a look at the wireless nl80211.c implementation. It's a good example for
> that sort of usage.

We are finalizing a NFC stack prototype and we decided to use generic
netlink. But during the implementation we found a disadvantage of this
approach.

While implementing data_exchange through netlink, it was not possible
to avoid memcpy as the skbuff coming from netlink can contain more
than one message.

We didn't find any implementation that uses netlink to exchage data.
As in the nl80211, the netlink is used only for control commands.

We plan to publish for comments this prototype early next week. Then
we plan to implement a version using AF_NFC.

Regards,

Aloisio Almeida Jr
INdT - Instituto Nokia de Tecnologia

> Cheers,
> Samuel.
>
> --
> Intel Open Source Technology Centre
> http://oss.intel.com/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Arnd Bergmann]

On Friday 01 April 2011 20:19:30 Aloisio Almeida wrote:
> We are finalizing a NFC stack prototype and we decided to use generic
> netlink. But during the implementation we found a disadvantage of this
> approach.
> 
> While implementing data_exchange through netlink, it was not possible
> to avoid memcpy as the skbuff coming from netlink can contain more
> than one message.

I don't think anyone in this thread suggested using netlink for user
data.

> We didn't find any implementation that uses netlink to exchage data.
> As in the nl80211, the netlink is used only for control commands.

Yes, that is the normal way to do it.
 
> We plan to publish for comments this prototype early next week. Then
> we plan to implement a version using AF_NFC.

Sounds good.
	
	Arnd

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Pavan Savoy]

On Thu, Mar 31, 2011 at 9:30 AM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> On Thu, Mar 31, 2011 at 06:24:50PM +0300, Waldemar.Rymarkiewicz@tieto.com wrote:
>> Hi Samuel,
>>
>> >That's the Bluetooth approach, and it works just fine as well.
>> >However, the netlink option for sending commands and receiving
>> >answers to/from a subsystem sounds more appropriate, at least to me.
>>
>> I haven't used netlink in practice, so just wondering why netlink. I assume, then, it's more suitable :)
>>
> Have a look at the wireless nl80211.c implementation. It's a good example for
> that sort of usage.

So, why was the network interface 'ala eth0 or the netlink interface dropped?
I saw these patches recently "NFC: add nfc subsystem core" - which
seem to add just the device /dev/nfc - am I correct ?
Or missing something ?

> Cheers,
> Samuel.
>
> --
> Intel Open Source Technology Centre
> http://oss.intel.com/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>
>

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Pavan Savoy]

On Mon, Jun 6, 2011 at 3:22 PM, Pavan Savoy <pavan_savoy@sify.com> wrote:
> On Thu, Mar 31, 2011 at 9:30 AM, Samuel Ortiz <sameo@linux.intel.com> wrote:
>> On Thu, Mar 31, 2011 at 06:24:50PM +0300, Waldemar.Rymarkiewicz@tieto.com wrote:
>>> Hi Samuel,
>>>
>>> >That's the Bluetooth approach, and it works just fine as well.
>>> >However, the netlink option for sending commands and receiving
>>> >answers to/from a subsystem sounds more appropriate, at least to me.
>>>
>>> I haven't used netlink in practice, so just wondering why netlink. I assume, then, it's more suitable :)
>>>
>> Have a look at the wireless nl80211.c implementation. It's a good example for
>> that sort of usage.
>
> So, why was the network interface 'ala eth0 or the netlink interface dropped?
> I saw these patches recently "NFC: add nfc subsystem core" - which
> seem to add just the device /dev/nfc - am I correct ?
> Or missing something ?

Yes apparently I did miss a whole of patches in the series.
So, for anyone who is interested on the list - this is the proposed
architecture,

http://code.openbossa.org/?p=nfc/linux-nfc.git;a=blob;f=Documentation/networking/nfc.txt;h=e17ac191ca958235db99da23786c46c4c60082e9;hb=f8d3349352063626240ad1c1a15e2361dab81ef4

>> Cheers,
>> Samuel.
>>
>> --
>> Intel Open Source Technology Centre
>> http://oss.intel.com/
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>>
>>
>

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Aloisio Almeida]

On Mon, Jun 6, 2011 at 5:30 PM, Pavan Savoy <pavan_savoy@sify.com> wrote:
> Yes apparently I did miss a whole of patches in the series.
> So, for anyone who is interested on the list - this is the proposed
> architecture,
>
> http://code.openbossa.org/?p=nfc/linux-nfc.git;a=blob;f=Documentation/networking/nfc.txt;h=e17ac191ca958235db99da23786c46c4c60082e9;hb=f8d3349352063626240ad1c1a15e2361dab81ef4

Yes, the patch series was submitted to linux-wireless and there is an
on going discussion over there.

Aloisio

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Samuel Ortiz]

Hi Pavan,

On Mon, Jun 06, 2011 at 03:22:01PM -0500, Pavan Savoy wrote:
> On Thu, Mar 31, 2011 at 9:30 AM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> > On Thu, Mar 31, 2011 at 06:24:50PM +0300, Waldemar.Rymarkiewicz@tieto.com wrote:
> >> Hi Samuel,
> >>
> >> >That's the Bluetooth approach, and it works just fine as well.
> >> >However, the netlink option for sending commands and receiving
> >> >answers to/from a subsystem sounds more appropriate, at least to me.
> >>
> >> I haven't used netlink in practice, so just wondering why netlink. I assume, then, it's more suitable :)
> >>
> > Have a look at the wireless nl80211.c implementation. It's a good example for
> > that sort of usage.
> 
> So, why was the network interface 'ala eth0 or the netlink interface dropped?
The netlink interface was not dropped. See:
http://marc.info/?l=linux-wireless&m=130705125228073&w=2

> I saw these patches recently "NFC: add nfc subsystem core" - which
> seem to add just the device /dev/nfc - am I correct ?
No, the /dev/nfc interface is gone with this subsystem. Instead you will talk
to an AF_NFC socket, or to the NFC generic netlink socket for some NFC high
level commands and events. See:
http://marc.info/?l=linux-wireless&m=130705125728090&w=2

for more details.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

RE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar.Rymarkiewicz]

>As I said, it's not important if you do it and it certainly 
>doesn't cause harm to prevent multiple open. It's just that 
>generally we don't care too much about this problem.
>

Ok, I get you.

Thanks,
Waldek

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Arnd Bergmann]

On Thursday 17 March 2011, Waldemar.Rymarkiewicz@tieto.com wrote:
> >This is not very different from opening the file descriptor in 
> >multiple processes, which you prevent using your logic.
> 
> but in the case when two independent applications try to open 
> my device I can't let the second to access. They obviously won't
> synch the access.

My point was that you don't need to worry.
 
> >You can of course argue that you try your best to prevent the 
> >race. Traditionally, e.g. on serial ports and the like, we 
> >don't do this but instead rely on user space synchronizing the 
> >access. What you have to make sure of course is that multiple 
> >threads calling read on the same file can never bring the 
> >kernel driver into an invalid state.
> 
> I assume, if an application shares the file pointer deliberately
> it have to sync the access. In other cases, the driver needs to
> secure it.

As I said, it's not important if you do it and it certainly doesn't
cause harm to prevent multiple open. It's just that generally
we don't care too much about this problem.

	Arnd

RE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar.Rymarkiewicz]

Arnd, 

>An application can open the device, and then do a 
>pthread_create() or a fork(). In either case, you have two 
>concurrent threads that have access to the file pointer and 
>can read from it concurrently, which is inherently racy 
>regarding which of the processes gets the data.

In that case I would relay on the application to synchronise the access.

>This is not very different from opening the file descriptor in 
>multiple processes, which you prevent using your logic.

but in the case when two independent applications try to open my device I can't let the second to access. They obviously won't synch the access.

>You can of course argue that you try your best to prevent the 
>race. Traditionally, e.g. on serial ports and the like, we 
>don't do this but instead rely on user space synchronizing the 
>access. What you have to make sure of course is that multiple 
>threads calling read on the same file can never bring the 
>kernel driver into an invalid state.

I assume, if an application shares the file pointer deliberately it have to sync the access. In other cases, the driver needs to secure it.

Waldek

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Arnd Bergmann]

On Thursday 17 March 2011, Waldemar.Rymarkiewicz@tieto.com wrote:
> >> >Note that the microread_is_busy() logic does not protect you from 
> >> >having multiple concurrent readers, because multiple threads may 
> >> >share a single file descriptor.
> >> 
> >> It's just used to ensure that only one reader can open the device.
> >> It's called only in open callback.
> >> The mutex actually secures concurrent read operations. 
> >
> >So if having multiple readers is safe (though possibly not 
> >meaningful), I guess you don't really need the 
> >microread_is_busy() logic.
> >
> >I suppose it doesn't hurt either, it just seems a bit 
> >pointless when it does the right thing most of the time, but 
> >not always.
> >
> 
> Could you give me an example when the microread_is_busy() logic does
> not do what it's intended to do. I don't really get your point here. 
> 

An application can open the device, and then do a pthread_create()
or a fork(). In either case, you have two concurrent threads that
have access to the file pointer and can read from it concurrently,
which is inherently racy regarding which of the processes gets the
data.

This is not very different from opening the file descriptor in
multiple processes, which you prevent using your logic.

You can of course argue that you try your best to prevent the
race. Traditionally, e.g. on serial ports and the like, we
don't do this but instead rely on user space synchronizing the
access. What you have to make sure of course is that multiple
threads calling read on the same file can never bring the
kernel driver into an invalid state.

	Arnd

RE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar.Rymarkiewicz]

Hi Arnd,

>> >Note that the microread_is_busy() logic does not protect you from 
>> >having multiple concurrent readers, because multiple threads may 
>> >share a single file descriptor.
>> 
>> It's just used to ensure that only one reader can open the device.
>> It's called only in open callback.
>> The mutex actually secures concurrent read operations. 
>
>So if having multiple readers is safe (though possibly not 
>meaningful), I guess you don't really need the 
>microread_is_busy() logic.
>
>I suppose it doesn't hurt either, it just seems a bit 
>pointless when it does the right thing most of the time, but 
>not always.
>

Could you give me an example when the microread_is_busy() logic does not do what it's intended to do.
I don't really get your point here. 


Waldek

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Arnd Bergmann]

On Tuesday 15 March 2011 09:37:04 Waldemar.Rymarkiewicz@tieto.com wrote:
> >It sounds strange that this will be handled in user space, 
> >since it won't have any idea if the hardware is available again.
> 
> >I've tried to find the onfc stack that talks to this device 
> >but couldn't see it. Do you have a URL for this?
> 
> Here you are
> 
> http://sourceforge.net/projects/open-nfc/
> http://www.open-nfc.org

I found these before, but I did not find code that opens your
device to read or write it. There is a .zip file with some
source code, but that seems to have no connection to your
driver and no build instructions.

	Arnd

RE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar.Rymarkiewicz]

>It sounds strange that this will be handled in user space, 
>since it won't have any idea if the hardware is available again.

>I've tried to find the onfc stack that talks to this device 
>but couldn't see it. Do you have a URL for this?

Here you are

http://sourceforge.net/projects/open-nfc/
http://www.open-nfc.org

Waldek

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Arnd Bergmann]

On Monday 14 March 2011, Waldemar.Rymarkiewicz@tieto.com wrote:
> >> No, it's simply there as I have been faceing i2c write error while I 
> >> do two consecutive writes.
> >> The second fails now and then. That's seems to be a chip issue.
> >> I will try to investigate this issue.
> >
> >Ok. It sounds like a bug in the i2c driver, you should also 
> >take a look there. Maybe it has never had to deal with this 
> >much write data at once.
> 
> It's been confirmed now that this is how chip is supposed to work and the
> driver should not care about this scenario. Upper layers (onfc stack) handle this.
> Thus, will simply remove urange_sleep and that's it.

It sounds strange that this will be handled in user space, since
it won't have any idea if the hardware is available again.

I've tried to find the onfc stack that talks to this device but
couldn't see it. Do you have a URL for this?

	Arnd

RE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar.Rymarkiewicz]

>> No, it's simply there as I have been faceing i2c write error while I 
>> do two consecutive writes.
>> The second fails now and then. That's seems to be a chip issue.
>> I will try to investigate this issue.
>
>Ok. It sounds like a bug in the i2c driver, you should also 
>take a look there. Maybe it has never had to deal with this 
>much write data at once.

It's been confirmed now that this is how chip is supposed to work and the driver should not care about this scenario. Upper layers (onfc stack) handle this.
Thus, will simply remove urange_sleep and that's it.

/Waldek

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Arnd Bergmann]

On Monday 14 March 2011, Waldemar.Rymarkiewicz@tieto.com wrote:
> >
> >If the usleep_range is trying to synchronize between the NFC 
> >and the I2C chip, you must wait for a notication from the NFC 
> >hardware that it's done.
> 
> No, it's simply there as I have been faceing i2c write error while
> I do two consecutive writes.
> The second fails now and then. That's seems to be a chip issue.
> I will try to investigate this issue.

Ok. It sounds like a bug in the i2c driver, you should also take
a look there. Maybe it has never had to deal with this much
write data at once.

	Arnd

RE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar.Rymarkiewicz]

>Most serial drivers do this, see drivers/tty/serial for a 
>number of examples, or drivers/serial on older kernels.

Thanks, will check it.

>That would depend on your hardware. The only important part is 
>that you make sure you can send out data at any time. If 
>i2c_master_send() causes accesses to your buffer after 
>returning, there has to be an i2c method of making sure that 
>it has completed.
>
>If the usleep_range is trying to synchronize between the NFC 
>and the I2C chip, you must wait for a notication from the NFC 
>hardware that it's done.

No, it's simply there as I have been faceing i2c write error while I do two consecutive writes.
The second fails now and then. That's seems to be a chip issue. I will try to investigate this issue.

>> What's more, I guess the i2c_master_send  is a synchronous call and 
>> when it returnes we know it flushed data. Right?
>
>If i2c_master_send is synchronous, you might not need the 
>usleep_range() at all. Removing that call would be entirely reasonable.

Will see how to approach that.

/Waldek

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Arnd Bergmann]

On Monday 14 March 2011, Waldemar.Rymarkiewicz@tieto.com wrote:
> >Oh, I see you simply do
> >
> >       ret = i2c_master_send(client, info->buf, len);
> >       usleep_range(1000, 10000);
> >
> >and assume that the buffer can always be written within a 
> >milisecond, so you just slow down output enough to never have 
> >to worry about it, right?
> >
> >A nicer solution would be to have an interrupt driven output 
> >so you know when the i2c buffers have been flushed.
> 
> Well, I get the idea of interrupt driven output, but as I have
> little linux kernel experience I'm not sure how to implement this. 
> Can you extend you thoughts or if you know piont me a driver which
> uses that concept?

Most serial drivers do this, see drivers/tty/serial for a number
of examples, or drivers/serial on older kernels.

> I'm not sure who should rise an interrupt when data has
> been flushed. I2c core or the chip itself?

That would depend on your hardware. The only important
part is that you make sure you can send out data at any
time. If i2c_master_send() causes accesses to your
buffer after returning, there has to be an i2c method
of making sure that it has completed.

If the usleep_range is trying to synchronize between the
NFC and the I2C chip, you must wait for a notication from
the NFC hardware that it's done.

> What's more, I guess the i2c_master_send  is a synchronous
> call and when it returnes we know it flushed data. Right? 

If i2c_master_send is synchronous, you might not
need the usleep_range() at all. Removing that call
would be entirely reasonable.

	Arnd

RE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar.Rymarkiewicz]

Hi Arnd,

>Oh, I see you simply do
>
>       ret = i2c_master_send(client, info->buf, len);
>       usleep_range(1000, 10000);
>
>and assume that the buffer can always be written within a 
>milisecond, so you just slow down output enough to never have 
>to worry about it, right?
>
>A nicer solution would be to have an interrupt driven output 
>so you know when the i2c buffers have been flushed.

Well, I get the idea of interrupt driven output, but as I have little linux kernel experience I'm not sure how to implement this. 
Can you extend you thoughts or if you know piont me a driver which uses that concept?

I'm not sure who should rise an interrupt when data has been flushed. I2c core or the chip itself?
What's more, I guess the i2c_master_send  is a synchronous call and when it returnes we know it flushed data. Right? 

/Waldek

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Arnd Bergmann]

On Thursday 10 March 2011, Waldemar.Rymarkiewicz@tieto.com wrote:

> >What I suggest you do is to work with the maintainers of the existing
> >pn544 driver (Matti and Jari) to create an NFC core library 
> >that takes care of the character device interface and that can 
> >be shared between the two drivers. Instead of each driver 
> >registering a misc device, make it call a 
> >nfc_device_register() function that is implemented in a common module.
> 
> I've been already thinking about that and it's seems like next obvious step.

Ok, cool.

> >mdev, rx_waitq and mutex would go into the common module.
> >I would expect that you also need a tx_waitq. What happens 
> >when the buffer is full?
> 
> Do you mean info->buff ?

Oh, I see you simply do

       ret = i2c_master_send(client, info->buf, len);
       usleep_range(1000, 10000);

and assume that the buffer can always be written within a milisecond,
so you just slow down output enough to never have to worry about it,
right?

A nicer solution would be to have an interrupt driven output
so you know when the i2c buffers have been flushed.

> >Note that the microread_is_busy() logic does not protect you 
> >from having multiple concurrent readers, because multiple 
> >threads may share a single file descriptor.
> 
> It's just used to ensure that only one reader can open the device.
> It's called only in open callback.
> The mutex actually secures concurrent read operations. 

So if having multiple readers is safe (though possibly not
meaningful), I guess you don't really need the microread_is_busy()
logic.

I suppose it doesn't hurt either, it just seems a bit pointless
when it does the right thing most of the time, but not always.

	Arnd

RE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar.Rymarkiewicz]

Hi Arnd, 

>However, this is the second NFC driver (at least), and that 
>means it's time to come up with a generic way of talking to 
>NFC devices from user space. We cannot allow every NFC 
>controller to have another set of arbitrary ioctl numbers.

>What I suggest you do is to work with the maintainers of the existing
>pn544 driver (Matti and Jari) to create an NFC core library 
>that takes care of the character device interface and that can 
>be shared between the two drivers. Instead of each driver 
>registering a misc device, make it call a 
>nfc_device_register() function that is implemented in a common module.

I've been already thinking about that and it's seems like next obvious step.

>Not your problem directly, but I think we need to find a way 
>to encode gpio pins in resources like we do for interrupts.
>
>> +	int (*request_hardware) (struct i2c_client *client);
>> +	void (*release_hardware) (void);
>
>Why do you need these in platform data? Can't you just request 
>the i2c device at the time when you create the platform_device?

I do gpio/irq stuff there but you are right it doesn't make sense. I can do it when create platform_device as well.
Will remove this. 

>mdev, rx_waitq and mutex would go into the common module.
>I would expect that you also need a tx_waitq. What happens 
>when the buffer is full?

Do you mean info->buff ?


>> +static inline int microread_is_busy(struct microread_info *info) {
>> +	return (info->state == MICROREAD_STATE_BUSY); }
>> +
>> +static int microread_open(struct inode *inode, struct file *file) {
>> +	struct microread_info *info = container_of(file->private_data,
>> +					struct microread_info, mdev);
>> +	struct i2c_client *client = info->i2c_dev;
>> +	int ret = 0;
>> +
>> +	dev_vdbg(&client->dev, "%s: info: %p", __func__, info);
>> +
>> +	mutex_lock(&info->mutex);
>> +
>> +	if (microread_is_busy(info)) {
>> +		dev_err(&client->dev, "%s: info %p: device is 
>busy.", __func__,
>> +				info);
>> +		ret = -EBUSY;
>> +		goto done;
>> +	}
>> +
>> +	info->state = MICROREAD_STATE_BUSY;
>> +done:
>> +	mutex_unlock(&info->mutex);
>> +	return ret;
>> +}
>
>Note that the microread_is_busy() logic does not protect you 
>from having multiple concurrent readers, because multiple 
>threads may share a single file descriptor.

It's just used to ensure that only one reader can open the device. It's called only in open callback.
The mutex actually secures concurrent read operations. 


>Some of your identifiers are not 'static'. Please make sure 
>that only symbols that are used across files are in the global 
>namespace.

Sure, I missed that. 
Must be fixed.

[PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Waldemar Rymarkiewicz]

Add new driver for MicroRead NFC chip connected to i2c bus.

See Documentation/nfc/nfc-microread.txt.

Signed-off-by: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>
---
 Documentation/nfc/nfc-microread.txt |   46 ++++
 drivers/nfc/Kconfig                 |   10 +
 drivers/nfc/Makefile                |    1 +
 drivers/nfc/microread.c             |  486 +++++++++++++++++++++++++++++++++++
 include/linux/nfc/microread.h       |   34 +++
 5 files changed, 577 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/nfc/nfc-microread.txt
 create mode 100644 drivers/nfc/microread.c
 create mode 100644 include/linux/nfc/microread.h

diff --git a/Documentation/nfc/nfc-microread.txt b/Documentation/nfc/nfc-microread.txt
new file mode 100644
index 0000000..e15c49c
--- /dev/null
+++ b/Documentation/nfc/nfc-microread.txt
@@ -0,0 +1,46 @@
+Kernel driver for Inside Secure MicroRead Near Field Communication chip
+
+General
+-------
+
+microread is the RF chip that uses Near Field Communication (NFC) for proximity
+transactions and interactions.
+
+Please visit Inside Secure webside for microread specification:
+http://www.insidesecure.com/eng/Products/NFC-Products/MicroRead
+
+
+The Driver
+----------
+
+The microread driver can be found under drivers/nfc/microread.c and it's
+compiled as a module named "microread".
+
+The driver supports i2c interface only.
+
+The userspace application can use /dev/microread device to control the chip by
+HCI messages. In a typical scenario application will open() /dev/microread,
+reset the chip useing ioctl() interface then poll() the device to check if
+writing/reaing is possible.Finally, will read/write data (HCI) from/to the chip.
+
+Note that only one application can use the /dev/microread at the time.
+
+The driver waits for microread chip interrupt which occures if there are some
+data to be read. Then, poll() will indicate that fact to the userspace.
+
+The application can use ioctl(MICROREAD_IOC_RESET)to reset the hardware.
+
+
+Platform Data
+-------------
+
+The below platform data should be set when configuring board.
+
+struct microread_nfc_platform_data {
+	unsigned int rst_gpio;
+	unsigned int irq_gpio;
+	unsigned int ioh_gpio;
+	int (*request_hardware) (struct i2c_client *client);
+	void (*release_hardware) (void);
+};
+
diff --git a/drivers/nfc/Kconfig b/drivers/nfc/Kconfig
index ea15800..a805615 100644
--- a/drivers/nfc/Kconfig
+++ b/drivers/nfc/Kconfig
@@ -26,5 +26,15 @@ config PN544_NFC
 	  To compile this driver as a module, choose m here. The module will
 	  be called pn544.
 
+config MICROREAD_NFC
+	tristate "MICROREAD NFC driver"
+	depends on I2C
+	default n
+	---help---
+	  Say yes if you want Inside Secure Microread Near Field Communication
+	  driver. This is for i2c connected version. If unsure, say N here.
+
+	  To compile this driver as a module, choose m here. The module will
+	  be called microread.
 
 endif # NFC_DEVICES
diff --git a/drivers/nfc/Makefile b/drivers/nfc/Makefile
index a4efb16..974f5cb 100644
--- a/drivers/nfc/Makefile
+++ b/drivers/nfc/Makefile
@@ -3,3 +3,4 @@
 #
 
 obj-$(CONFIG_PN544_NFC)		+= pn544.o
+obj-$(CONFIG_MICROREAD_NFC)	+= microread.o
diff --git a/drivers/nfc/microread.c b/drivers/nfc/microread.c
new file mode 100644
index 0000000..4393033
--- /dev/null
+++ b/drivers/nfc/microread.c
@@ -0,0 +1,486 @@
+/*
+ *  Driver for Microread NFC chip
+ *
+ *  Copyright (C) 2011 Tieto Poland
+ *
+ *  Author: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/interrupt.h>
+#include <linux/i2c.h>
+#include <linux/fs.h>
+#include <linux/slab.h>
+#include <linux/poll.h>
+#include <linux/gpio.h>
+#include <linux/delay.h>
+#include <linux/miscdevice.h>
+#include <linux/nfc/microread.h>
+#include <linux/uaccess.h>
+#include <linux/pm.h>
+
+#define MICROREAD_DEV_NAME	"microread"
+#define MICROREAD_MAX_BUF_SIZE	0x20
+
+#define MICROREAD_STATE_BUSY	0x00
+#define MICROREAD_STATE_READY	0x01
+
+struct microread_info {
+	struct i2c_client *i2c_dev;
+	struct miscdevice mdev;
+
+	u8 state;
+	u8 irq_state;
+	int irq;
+	u16 buflen;
+	u8 *buf;
+	wait_queue_head_t rx_waitq;
+	struct mutex rx_mutex;
+	struct mutex mutex;
+};
+
+static void microread_reset_hw(struct microread_nfc_platform_data *pdata)
+{
+	gpio_set_value(pdata->rst_gpio, 1);
+	usleep_range(1000, 2000);
+	gpio_set_value(pdata->rst_gpio, 0);
+	usleep_range(5000, 10000);
+}
+
+static u8 calc_lrc(u8 *buf, int len)
+{
+	int i;
+	u8 lrc = 0;
+	for (i = 0; i < len; i++)
+		lrc = lrc ^ buf[i];
+	return lrc;
+}
+
+static inline int microread_is_busy(struct microread_info *info)
+{
+	return (info->state == MICROREAD_STATE_BUSY);
+}
+
+static int microread_open(struct inode *inode, struct file *file)
+{
+	struct microread_info *info = container_of(file->private_data,
+					struct microread_info, mdev);
+	struct i2c_client *client = info->i2c_dev;
+	int ret = 0;
+
+	dev_vdbg(&client->dev, "%s: info: %p", __func__, info);
+
+	mutex_lock(&info->mutex);
+
+	if (microread_is_busy(info)) {
+		dev_err(&client->dev, "%s: info %p: device is busy.", __func__,
+				info);
+		ret = -EBUSY;
+		goto done;
+	}
+
+	info->state = MICROREAD_STATE_BUSY;
+done:
+	mutex_unlock(&info->mutex);
+	return ret;
+}
+
+static int microread_release(struct inode *inode, struct file *file)
+{
+	struct microread_info *info = container_of(file->private_data,
+					struct microread_info, mdev);
+	struct i2c_client *client = info->i2c_dev;
+
+	dev_vdbg(&client->dev, "%s: info: %p, client %p\n", __func__, info,
+					client);
+
+	mutex_lock(&info->mutex);
+	info->state = MICROREAD_STATE_READY;
+	mutex_unlock(&info->mutex);
+
+	return 0;
+}
+
+ssize_t microread_read(struct file *file, char __user *buf, size_t count,
+								loff_t *f_pos)
+{
+	struct microread_info *info = container_of(file->private_data,
+					struct microread_info, mdev);
+	struct i2c_client *client = info->i2c_dev;
+	struct microread_nfc_platform_data *pdata =
+				dev_get_platdata(&client->dev);
+	int ret;
+	u8 len, lrc;
+
+	dev_dbg(&client->dev, "%s: info: %p, size: %d (bytes).", __func__,
+						info, count);
+	mutex_lock(&info->mutex);
+
+	if (!info->irq_state && !gpio_get_value(pdata->irq_gpio)) {
+		if (file->f_flags & O_NONBLOCK) {
+			ret = -EAGAIN;
+			goto done;
+		}
+
+		if (wait_event_interruptible(info->rx_waitq,
+				(info->irq_state == 1))) {
+			ret = -EINTR;
+			goto done;
+		}
+	}
+
+	if (count > info->buflen) {
+		dev_err(&client->dev, "%s: no enough space in read buffer.",
+				__func__);
+		ret = -ENOMEM;
+		goto done;
+	}
+
+	mutex_lock(&info->rx_mutex);
+	ret = i2c_master_recv(client, info->buf, info->buflen);
+	info->irq_state = 0;
+	mutex_unlock(&info->rx_mutex);
+
+	if (ret < 0) {
+		dev_err(&client->dev, "%s: i2c read (data) failed (err %d).",
+				__func__, ret);
+		ret = -EREMOTEIO;
+		goto done;
+	}
+
+#ifdef VERBOSE_DEBUG
+		print_hex_dump(KERN_DEBUG, "rx: ", DUMP_PREFIX_NONE,
+				16, 1, info->buf, ret, false);
+#endif
+	len = info->buf[0];
+
+	lrc = calc_lrc(info->buf, len + 1);
+	if (lrc != info->buf[len + 1]) {
+		dev_err(&client->dev, "%s: incorrect i2c frame.", __func__);
+		ret = -EFAULT;
+		goto done;
+	}
+
+	ret = len + 2;
+
+	if (copy_to_user(buf, info->buf, len + 2)) {
+		dev_err(&client->dev, "%s: error copying to user.", __func__);
+		ret = -EFAULT;
+		goto done;
+	}
+done:
+	mutex_unlock(&info->mutex);
+
+	return ret;
+}
+
+ssize_t microread_write(struct file *file, const char __user *buf,
+			size_t count, loff_t *f_pos)
+{
+	struct microread_info *info = container_of(file->private_data,
+					struct microread_info, mdev);
+	struct i2c_client *client = info->i2c_dev;
+	int ret;
+	u16 len;
+
+	dev_dbg(&client->dev, "%s: info: %p, size %d (bytes).", __func__,
+					info, count);
+
+	if (count > info->buflen) {
+		dev_err(&client->dev, "%s: no enought space in TX buffer.",
+				__func__);
+		return -EINVAL;
+	}
+
+	len = min_t(u16, count, info->buflen);
+
+	mutex_lock(&info->mutex);
+	if (copy_from_user(info->buf, buf, len)) {
+		dev_err(&client->dev, "%s: error copying from user.",
+				__func__);
+		ret = -EFAULT;
+		goto done;
+	}
+
+#ifdef VERBOSE_DEBUG
+	print_hex_dump(KERN_DEBUG, "tx: ", DUMP_PREFIX_NONE, 16, 1, info->buf,
+			len, false);
+#endif
+	ret = i2c_master_send(client, info->buf, len);
+	if (ret < 0)
+		dev_err(&client->dev, "%s: i2c write failed (err %d).",
+			__func__, ret);
+	usleep_range(1000, 10000);
+done:
+	mutex_unlock(&info->mutex);
+	return ret;
+
+}
+
+unsigned int microread_poll(struct file *file, poll_table *wait)
+{
+	struct microread_info *info = container_of(file->private_data,
+					struct microread_info, mdev);
+	struct i2c_client *client = info->i2c_dev;
+	int ret = (POLLOUT | POLLWRNORM);
+
+	dev_vdbg(&client->dev, "%s: info: %p client %p", __func__, info,
+			client);
+
+	mutex_lock(&info->mutex);
+	poll_wait(file, &info->rx_waitq, wait);
+
+	mutex_lock(&info->rx_mutex);
+	if (info->irq_state)
+		ret |= (POLLIN | POLLRDNORM);
+	mutex_unlock(&info->rx_mutex);
+	mutex_unlock(&info->mutex);
+
+	return ret;
+}
+
+long microread_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct microread_info *info = container_of(file->private_data,
+					struct microread_info, mdev);
+	struct i2c_client *client = info->i2c_dev;
+	struct microread_nfc_platform_data *pdata =
+				dev_get_platdata(&client->dev);
+	int ret = 0;
+
+	dev_dbg(&client->dev, "%s: info: %p cmd %d", __func__, info, cmd);
+
+	mutex_lock(&info->mutex);
+
+	switch (cmd) {
+	case MICROREAD_IOC_CONFIGURE:
+	case MICROREAD_IOC_CONNECT:
+		goto done;
+
+	case MICROREAD_IOC_RESET:
+		microread_reset_hw(pdata);
+		goto done;
+
+	default:
+		dev_err(&client->dev, "%s; not supported ioctl 0x%x", __func__,
+			cmd);
+		ret = -ENOIOCTLCMD;
+		goto done;
+	}
+
+done:
+	mutex_unlock(&info->mutex);
+	return ret;
+}
+
+const struct file_operations microread_fops = {
+	.owner		= THIS_MODULE,
+	.open		= microread_open,
+	.release	= microread_release,
+	.read		= microread_read,
+	.write		= microread_write,
+	.poll		= microread_poll,
+	.unlocked_ioctl	= microread_ioctl,
+};
+
+static irqreturn_t microread_irq(int irq, void *dev)
+{
+	struct microread_info *info = dev;
+	struct i2c_client *client = info->i2c_dev;
+
+	dev_dbg(&client->dev, "irq: info %p client %p ", info,	client);
+
+	if (irq != client->irq)
+		return IRQ_NONE;
+
+	mutex_lock(&info->rx_mutex);
+	info->irq_state = 1;
+	mutex_unlock(&info->rx_mutex);
+
+	wake_up_interruptible(&info->rx_waitq);
+
+	return IRQ_HANDLED;
+}
+
+static int __devinit microread_probe(struct i2c_client *client,
+				 const struct i2c_device_id *id)
+{
+	struct microread_info *info;
+	struct microread_nfc_platform_data *pdata =
+				dev_get_platdata(&client->dev);
+	int ret = 0;
+
+	dev_dbg(&client->dev, "%s: client %p", __func__, client);
+
+	if (!pdata) {
+		dev_err(&client->dev, "%s: client %p: missing platform data.",
+				__func__, client);
+		return -EINVAL;
+	}
+
+	if (!pdata->request_hardware) {
+		dev_err(&client->dev, "%s: client %p: no request_hardware()",
+				__func__, client);
+		return -EINVAL;
+	}
+
+	info = kzalloc(sizeof(struct microread_info), GFP_KERNEL);
+	if (!info) {
+		dev_err(&client->dev, "%s: can't allocate microread_info.",
+				__func__);
+		return -ENOMEM;
+	}
+
+	info->buflen = (u16) MICROREAD_MAX_BUF_SIZE;
+	info->buf = kzalloc(info->buflen, GFP_KERNEL);
+	if (!info->buf) {
+		dev_err(&client->dev, "%s: can't allocate buf. (len %d)",
+			__func__, info->buflen);
+		ret = -ENOMEM;
+		goto free_info;
+	}
+
+	mutex_init(&info->mutex);
+	mutex_init(&info->rx_mutex);
+	init_waitqueue_head(&info->rx_waitq);
+	i2c_set_clientdata(client, info);
+	info->i2c_dev = client;
+	info->irq_state = 0;
+	info->state = MICROREAD_STATE_READY;
+
+	ret = pdata->request_hardware(client);
+	if (ret) {
+		dev_err(&client->dev, "%s: requesting hardware failed (ret %d)",
+				__func__, ret);
+		goto free_buf;
+	}
+
+	ret = request_threaded_irq(client->irq, NULL, microread_irq,
+		IRQF_SHARED|IRQF_TRIGGER_RISING, MICROREAD_DRIVER_NAME, info);
+	if (ret) {
+		dev_err(&client->dev, "%s: can't request irq. (ret %d, irq %d)",
+			__func__, ret, client->irq);
+		goto free_hardware;
+	}
+
+	info->mdev.minor = MISC_DYNAMIC_MINOR;
+	info->mdev.name = MICROREAD_DEV_NAME;
+	info->mdev.fops = &microread_fops;
+	info->mdev.parent = &client->dev;
+
+	ret = misc_register(&info->mdev);
+	if (ret < 0) {
+		dev_err(&client->dev, "%s: register chr dev failed (ret %d)",
+			__func__, ret);
+			goto free_irq;
+	}
+
+	dev_info(&client->dev, "Probed.[/dev/%s]", MICROREAD_DEV_NAME);
+	return 0;
+
+free_irq:
+	free_irq(client->irq, info);
+free_hardware:
+	pdata->release_hardware();
+free_buf:
+	kfree(info->buf);
+free_info:
+	kfree(info);
+
+	dev_info(&client->dev, "Not probed.");
+	return ret;
+}
+
+static __devexit int microread_remove(struct i2c_client *client)
+{
+	struct microread_info *info = i2c_get_clientdata(client);
+	struct microread_nfc_platform_data *pdata =
+				dev_get_platdata(&client->dev);
+
+	dev_dbg(&client->dev, "%s", __func__);
+
+	misc_deregister(&info->mdev);
+	free_irq(client->irq, info);
+
+	if (pdata->release_hardware)
+		pdata->release_hardware();
+
+	kfree(info->buf);
+	kfree(info);
+
+	dev_info(&client->dev, "%s: Removed.", __func__);
+	return 0;
+}
+
+static int microread_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+	return -ENOSYS;
+}
+
+static int microread_resume(struct i2c_client *client)
+{
+	return -ENOSYS;
+}
+
+static struct i2c_device_id microread_id[] = {
+	{ MICROREAD_DRIVER_NAME, 0},
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, microread_id);
+
+static struct i2c_driver microread_driver = {
+	.driver = {
+		.name = MICROREAD_DRIVER_NAME,
+	},
+	.probe		= microread_probe,
+	.remove		= __devexit_p(microread_remove),
+	.suspend	= microread_suspend,
+	.resume		= microread_resume,
+	.id_table	= microread_id,
+};
+
+static int __init microread_init(void)
+{
+	int ret;
+
+	pr_debug(MICROREAD_DRIVER_NAME ": %s", __func__);
+
+	ret = i2c_add_driver(&microread_driver);
+	if (ret) {
+		pr_err(MICROREAD_DRIVER_NAME ": driver registration failed");
+		return ret;
+	}
+	pr_info(MICROREAD_DRIVER_NAME ": Loaded.");
+	return 0;
+}
+
+static void __exit microread_exit(void)
+{
+	pr_debug(MICROREAD_DRIVER_NAME ": %s", __func__);
+	i2c_del_driver(&microread_driver);
+	pr_info(MICROREAD_DRIVER_NAME ": Unloaded");
+}
+
+module_init(microread_init);
+module_exit(microread_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Waldemar Rymarkiewicz <waldemar.rymarkiewicz@tieto.com>");
+MODULE_DESCRIPTION("Driver for Microread NFC chip");
+
diff --git a/include/linux/nfc/microread.h b/include/linux/nfc/microread.h
new file mode 100644
index 0000000..1adbf93
--- /dev/null
+++ b/include/linux/nfc/microread.h
@@ -0,0 +1,34 @@
+#ifndef _MICROREAD_H
+#define _MICROREAD_H
+
+#include <linux/i2c.h>
+
+#define MICROREAD_DRIVER_NAME	"microread"
+
+#define MICROREAD_IOC_MAGIC	'o'
+#define MICROREAD_IOC_MAX_CONFIG_LENGTH	16
+
+struct microread_ioc_configure {
+	int length;
+	unsigned char buffer[MICROREAD_IOC_MAX_CONFIG_LENGTH];
+};
+
+/* ioctl cmds */
+#define MICROREAD_IOC_CONFIGURE	_IOW(MICROREAD_IOC_MAGIC, 0, \
+					struct microread_ioc_configure)
+#define MICROREAD_IOC_CONNECT	_IO(MICROREAD_IOC_MAGIC, 1)
+#define MICROREAD_IOC_RESET	_IO(MICROREAD_IOC_MAGIC, 2)
+
+#ifdef __KERNEL__
+/* board config platform data for microread */
+struct microread_nfc_platform_data {
+	unsigned int rst_gpio;
+	unsigned int irq_gpio;
+	unsigned int ioh_gpio;
+	int (*request_hardware) (struct i2c_client *client);
+	void (*release_hardware) (void);
+};
+#endif /* __KERNEL__ */
+
+#endif /* _MICROREAD_H */
+
-- 
1.7.1

Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip

[Arnd Bergmann]

On Thursday 10 March 2011, Waldemar Rymarkiewicz wrote:
> Add new driver for MicroRead NFC chip connected to i2c bus.
> 
> See Documentation/nfc/nfc-microread.txt.

The imlpementation looks fairly clean, no objections on that.

However, this is the second NFC driver (at least), and that means
it's time to come up with a generic way of talking to NFC devices
from user space. We cannot allow every NFC controller to have
another set of arbitrary ioctl numbers.

What I suggest you do is to work with the maintainers of the existing
pn544 driver (Matti and Jari) to create an NFC core library that
takes care of the character device interface and that can be
shared between the two drivers. Instead of each driver registering a
misc device, make it call a nfc_device_register() function that
is implemented in a common module.

You will need a structure like

struct nfc_device {
	struct device *dev; /* points to i2c/platform/... hardware device */
	const struct nfc_operations *ops;
	struct mutex mutex;
};

struct nfc_operations {
	/* pin before calling the functions */
	struct module *owner;

	/* called from file operations */
	int (*open)(struct nfc_device *dev);
	void (*close)(struct nfc_device *dev);

	/* called from ioctl */
	int (*configure)(struct nfc_device *dev);
	int (*connect)(struct nfc_device *dev);
	int (*reset)(struct nfc_device *);

	size_t (*read_avail)(struct nfc_device *); /* for poll, read */
	ssize_t (*read)(struct nfc_device *, void __user *buf, size_t len);
	size_t (*write_avail)(struct nfc_device *); /* for write */
	ssize_t (*write)(struct nfc_device *, void __user *buf, size_t len);
};

extern int nfc_device_register(struct nfc_device *dev);
extern void nfc_device_unregister(struct nfc_device *dev);
extern void nfc_wake_up(struct nfc_device *dev); /* on interrupt */

> +The below platform data should be set when configuring board.
> +
> +struct microread_nfc_platform_data {
> +	unsigned int rst_gpio;
> +	unsigned int irq_gpio;
> +	unsigned int ioh_gpio;

Not your problem directly, but I think we need to find a way to encode
gpio pins in resources like we do for interrupts.

> +	int (*request_hardware) (struct i2c_client *client);
> +	void (*release_hardware) (void);

Why do you need these in platform data? Can't you just request
the i2c device at the time when you create the platform_device?

> +struct microread_info {
> +	struct i2c_client *i2c_dev;
> +	struct miscdevice mdev;
> +
> +	u8 state;
> +	u8 irq_state;
> +	int irq;
> +	u16 buflen;
> +	u8 *buf;
> +	wait_queue_head_t rx_waitq;
> +	struct mutex rx_mutex;
> +	struct mutex mutex;
> +};

mdev, rx_waitq and mutex would go into the common module.
I would expect that you also need a tx_waitq. What happens
when the buffer is full?

> +static inline int microread_is_busy(struct microread_info *info)
> +{
> +	return (info->state == MICROREAD_STATE_BUSY);
> +}
> +
> +static int microread_open(struct inode *inode, struct file *file)
> +{
> +	struct microread_info *info = container_of(file->private_data,
> +					struct microread_info, mdev);
> +	struct i2c_client *client = info->i2c_dev;
> +	int ret = 0;
> +
> +	dev_vdbg(&client->dev, "%s: info: %p", __func__, info);
> +
> +	mutex_lock(&info->mutex);
> +
> +	if (microread_is_busy(info)) {
> +		dev_err(&client->dev, "%s: info %p: device is busy.", __func__,
> +				info);
> +		ret = -EBUSY;
> +		goto done;
> +	}
> +
> +	info->state = MICROREAD_STATE_BUSY;
> +done:
> +	mutex_unlock(&info->mutex);
> +	return ret;
> +}

Note that the microread_is_busy() logic does not protect you from having
multiple concurrent readers, because multiple threads may share a single
file descriptor.

> +long microread_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
...
> +const struct file_operations microread_fops = {
> +	.owner		= THIS_MODULE,

Some of your identifiers are not 'static'. Please make sure that only symbols
that are used across files are in the global namespace.

	Arnd