lkml.org 
[lkml]   [2011]   [Mar]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRE: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip
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.


\
 
 \ /
  Last update: 2011-03-10 15:49    [from the cache]
©2003-2011 Jasper Spaans