Add new driver for MicroRead NFC chip connected to i2c bus.
See Documentation/nfc/nfc-microread.txt.
Signed-off-by: Waldemar Rymarkiewicz <[email protected]>
---
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 <[email protected]>
+ *
+ * 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 = µread_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(µread_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(µread_driver);
+ pr_info(MICROREAD_DRIVER_NAME ": Unloaded");
+}
+
+module_init(microread_init);
+module_exit(microread_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Waldemar Rymarkiewicz <[email protected]>");
+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
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
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.
On Thursday 10 March 2011, [email protected] 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
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
On Monday 14 March 2011, [email protected] 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
>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-
On Monday 14 March 2011, [email protected] 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
>> 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
On Monday 14 March 2011, [email protected] 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
>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-
On Tuesday 15 March 2011 09:37:04 [email protected] 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
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
On Thursday 17 March 2011, [email protected] 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
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-
On Thursday 17 March 2011, [email protected] 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
>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-