On Thu, 21 Oct 2010 15:37:40 -0700
Ken Lierman <[email protected]> wrote:
> Add 3rd party device driver and associated platform support
> for NFC (NXP PN544 controller chip)
>
> Please enabled in the default config for mid as well, ie:
> CONFIG_PN544_NFC=y
>
> Change-Id: Idf6a2b4a0283d6ae1dd7fdf0455b9d6529cd1670
>
It should also have a Nokia sign off, and the non kernel/mrst.c bit
really should go upstream first.
> +++ b/drivers/misc/pn544.c
> + case TCGETS:
> + return -ENOIOCTLCMD;
> +
> + default:
> + dev_err(&info->i2c_dev->dev, "Unknown ioctl 0x%x\n",
> cmd);
> + return -ENOIOCTLCMD;
This should never be returned to user space, nor should an ioctl
handler allow users to cause the log to be filled with crap. If you are
trying to pretend to be a tty device it's nowdays easier just to use
the tty_port interface and be one as you need very little extra code.
> +static int pn544_open(struct inode *inode, struct file *file)
> +{
> + struct pn544_info *info;
> + struct i2c_client *client;
> + int r = 0;
> +
> + info = pn544; /* ugly, somehow we should get this via inode
> dev */
If you are using a cdev you can and with the current misc device code
you can (this got fixed a few releases ago)
> + /*
> + * Only 1 at a time.
> + * XXX: maybe user (counter) would work better
> + */
> + if (info->state != PN544_ST_COLD)
> + return -EBUSY;
> +
> + file->private_data = info;
> + file->f_pos = info->read_offset;
> + r = pn544_enable(info, HCI_MODE);
So what stops two parallel opens or closes ?
And if you are using a misc device what if someone has two ?
> +/* Optional test to see if the chip actually answers */
> +#ifdef DO_RSET_TEST
> +static int pn544_rset_test(struct pn544_info *info)
> +{
> + struct i2c_client *client = info->i2c_dev;
> + struct pn544_llc_packet rset = { /* U RSET frame without
> baudrate */
> + .length = 0x05,
> + .header = 0xF9,
> + .data = { 0x04, 0x00, 0xC3, 0xE5 }
> + };
> + struct pn544_llc_packet answer = {
> + .length = 0x03,
> + .header = 0xe6,
> + .data = { 0x17, 0xa7 }
> + };
static const if they don't change - otherwise the compiler has to build
the object on the stack
> +static int pn544_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct pn544_info *info;
> +
> + dev_info(&client->dev, "***\n%s: client %p\n***\n",
> __func__, client); +
> + info = i2c_get_clientdata(client);
> + dev_info(&client->dev, "%s: info: %p, client %p\n", __func__,
> + info, client);
> +
> + switch (info->state) {
> + case PN544_ST_FW_READY:
> + /* Do not suspend while upgrading FW, please! */
If that is important what locking do you have to ensure this suspend
test doesn't race a FW update starting ?
> + return -EPERM;
> +
> + case PN544_ST_READY:
> + /*
> + * CHECK: Device should be in standby-mode. No way
> to check?
> + * Allowing low power mode for the regulator is
> potentially
> + * dangerous if pn544 does not go to suspension.
> + */
> + break;
> +
> + case PN544_ST_COLD:
So its "potentially dangerous" and you want to submit it to a public
tree ? What sort of dangerous are we talking here ?
> + pn544_miscdevice.parent = &client->dev;
> + r = misc_register(&pn544_miscdevice);
What if this is the second one you detected ?
> +static void __exit pn544_exit(void)
> +{
> + flush_scheduled_work();
Is being removed from the upstream kernel very soon.
_______________________________________________
Meego-kernel mailing list
[email protected]
http://lists.meego.com/listinfo/meego-kernel
