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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html