Hello Markus,
Markus Rechberger wrote:
> +static int tuner_open(struct inode *inode, struct file *file)
> +{
> + struct tuner_info *info = kzalloc(sizeof(struct tuner_info),
> GFP_KERNEL);
you should return -ENOMEM in case kzalloc fails.
> + init_waitqueue_head(&info->waitqueue);
> + mutex_init(&info->lock);
> + INIT_LIST_HEAD(&info->tuner_commands);
> + file->private_data = info;
> + return 0;
> +}
> +
> +static int tuner_close(struct inode *inode, struct file *file)
> +{
> + struct tuner_info *info, *tuner_info;
> + struct list_head *list;
> +
> + if (file->private_data == NULL)
> + return 0;
> +
> + info = file->private_data;
> +
> + if(info == NULL) {
> + printk("BUG: info == NULL\n");
> + return 0;
> + }
I think this check is superflous because of the test above. Also I think
that file->private_data can never be NULL if kzalloc in tuner_open
didn't fail.
> + switch(cmd) {
> + case TUNER_ADD_DAEMON:
> + {
> + if (daemon_client) {
> + printk("a userspace daemon seems to be attached already\n");
Calls to printk should include a message level.
> + /* simple i2c wrapper */
> + case TUNER_SEND_I2C_CMD:
> + {
> + struct tuner_i2c_msg *i2ccmd = (struct tuner_i2c_msg *)arg;
> + struct tuner_i2c_msg icmd;
> + struct i2c_msg msg = { .flags = 0 };
> + struct list_head *list;
> + int retval = 0;
> + struct tuner_dev *devlist,*dev=NULL;
> + if(copy_from_user(&icmd, i2ccmd, sizeof(struct tuner_i2c_msg))) {
> + mutex_unlock(&info->lock);
> + printk("unable to copy from user!\n");
> + goto efault;
> + }
> + list_for_each(list, &tuner_devlist) {
> + devlist = list_entry(list, struct tuner_dev, devlist);
> + if (devlist->clientid == icmd.clientid) {
> + dev = devlist;
> + break;
> + }
> + }
> + if(dev == NULL) {
> + printk("DEV is zero (clientid: %d)!!!\n", icmd.clientid);
> + mutex_unlock(&info->lock);
> + goto einval;
> + }
> +
> + /* took this restriction from i2c-dev.c */
> + if (icmd.len>8192) {
> + printk("len > 8192 error!\n");
> + mutex_unlock(&info->lock);
> + goto einval;
> + }
> +
> + msg.buf = kzalloc(icmd.len, GFP_KERNEL);
> + msg.len = icmd.len;
> + msg.addr = icmd.addr;
> +
> + if (msg.buf == NULL) {
> + mutex_unlock(&info->lock);
> + goto enomem;
> + }
> +
> + if(copy_from_user(msg.buf, i2ccmd->buf,i2ccmd->len)) {
> + kfree(msg.buf);
> + mutex_unlock(&info->lock);
> + printk("unable to copy from user!\n");
> + goto efault;
> + }
> +
> +
> +
> + if(!(retval=i2c_transfer(dev->adap,&msg, 1))) {
> + printk("retval: %d\n",retval);
> + kfree(msg.buf);
> + mutex_unlock(&info->lock);
> + goto einval;
> + }
> + kfree(msg.buf);
> + mutex_unlock(&info->lock);
> + goto e_ok;
> + }
> +
> + case TUNER_READ_I2C_CMD:
> + {
> + struct tuner_i2c_msg *i2ccmd = (struct tuner_i2c_msg *)arg;
> + struct tuner_i2c_msg icmd;
> + struct i2c_msg msg = { .flags = I2C_M_RD };
> + struct tuner_dev *devlist,*dev=NULL;
> + int retval = 0;
> + struct list_head *list;
> + if(copy_from_user(&icmd, i2ccmd, sizeof(struct tuner_i2c_msg))) {
> + mutex_unlock(&info->lock);
> + goto efault;
> + }
> + list_for_each(list, &tuner_devlist) {
> + devlist = list_entry(list, struct tuner_dev, devlist);
> + if (devlist->clientid == icmd.clientid) {
> + dev = devlist;
> + break;
> + }
> + }
> + if(dev == NULL) {
> + mutex_unlock(&info->lock);
> + goto einval;
> + }
> +
> + msg.len = icmd.len;
> + msg.addr = icmd.addr;
> +
> + if (msg.len>8192) {
> + mutex_unlock(&info->lock);
> + goto einval;
> + }
> +
> + msg.buf = kzalloc(icmd.len, GFP_KERNEL);
> +
> + if (msg.buf == NULL) {
> + mutex_unlock(&info->lock);
> + goto enomem;
> + }
> +
> + if(!(retval=i2c_transfer(dev->adap,&msg, 1))) {
> + printk("retval: %d\n",retval);
> + mutex_unlock(&info->lock);
> + goto einval;
> + }
> +
> + if (copy_to_user(i2ccmd->buf, msg.buf, msg.len)) {
> + mutex_unlock(&info->lock);
> + goto efault;
> + }
> + kfree(msg.buf);
> + mutex_unlock(&info->lock);
> + goto e_ok;
> + }
Isn't it better to use i2c-dev instead? You would just have to pass the
ID of the i2c bus to userspace. This would allow sending multiple
messages using a single call i2c_transfer (via the I2C_RDWR ioctl). This
is probably needed when tuners are hidden behind an i2c gate which is
closed automatically when a stop condition occurs.
Regards,
Andreas
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
