On Thu, 21 Oct 2010 15:37:21 -0700 Ken Lierman <[email protected]> wrote:
> From: Ken Lierman <[email protected]> > > Add 3rd party device driver and associated platform support for > Broadcom BCM4751 GPS chip on customer hardware. > > Please enabled in the default config for mid as well, ie: > CONFIG_BCM4751_GPS=y Again needs the originators sign off really. Also the From is wrong, you didn't write it according to the comments. This stuff is *IMPORTANT*. > +static int bcm4751_gps_setup(struct i2c_client *client) > +{ I think mrst.c urgently needs an gpio_alloc_group() that takes a struct describing the direction etc for each pin ! > + return copy_to_user(buf, tmp, num_read) ? -EFAULT : num_read; Properly x = copy_to_user(...); return x ? x : -EFAULT; But that's pedantic standards compliance I just mention in passing. In this case you've consumed some data and provided the user a subset of their request. > +static int bcm4751_gps_ioctl(struct inode *inode, struct file *file, > + unsigned int cmd, unsigned long arg) > +{ > + struct i2c_client *client = bcm4751_gps_device->client; > + > + dev_dbg(&client->dev, "ioctl: cmd = 0x%02x, arg=0x%02lx\n", > cmd, arg); + > + switch (cmd) { > + case I2C_SLAVE: > + case I2C_SLAVE_FORCE: > + if ((arg > 0x3ff) || > + (((client->flags & I2C_M_TEN) == 0) > && > + arg > 0x7f)) > + return -EINVAL; > + client->addr = arg; So a user can use ioctl to talk to any arbitary i2c device this way. How "convenient" for all the wrong reasons. Also no locking. I don't think these ioctls belong in the kernel somehow. > +static ssize_t bcm4751_gps_set_enable(struct device *dev, > + struct device_attribute *attr, const char *buf, > size_t len) +{ > + struct bcm4751_gps_data *self = dev_get_drvdata(dev); > + int value; > + > + sscanf(buf, "%d", &value); > + dev_dbg(dev, "enable: %d", value); > + > + switch (value) { > + case 0: > + bcm4751_gps_disable(self); > + break; > + > + case 1: > + bcm4751_gps_enable(self); > + break; So what stops disable, enable, read and the workqueue all running at once (oh and all while the irq function is changing the i2c address) ? > > + * "Bottom half" of the interrupt. > + * It gets called only when the "main" handler detects HOST_IRQ=1 > + */ Should be using a threaded IRQ ? > + bcm4751_gps_miscdevice.parent = &client->dev; > + err = misc_register(&bcm4751_gps_miscdevice); Suppose its the second one detected ? Secondly it needs some documentation - it returns some binary packet format, but which of the standards does it follow ? Alan _______________________________________________ Meego-kernel mailing list [email protected] http://lists.meego.com/listinfo/meego-kernel
