Hi Andreas,
On 8/14/07, Andreas Oberritter <obi@xxxxxxxxxxx> wrote:
> 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.
>
Ok.
> > + 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.
>
Ok.
> > + 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.
>
Ok.
I added your proposed changes to:
http://mcentral.de/~mrec/patches/v4l_dvb_userspace_tuner_r1.diff
> > + /* 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?
I don't think so since i2c is only meant for i2c in the end and it doesn't implement the poll mechanism to notify the userland application that something is required from userspace.
> 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.
>
it's also possible to do this with that API, the userland application is also able to embed an ioctl like message within an ioctl message to pass it forward to the driver (for example for toggling a register which is non i2c), this would be equivalent to the callback feature in tuner-core.
I'm aware of this because the qt1010 needs to toggle a non i2c register too, although that's beeing done in the demodulator driver before it tunes to a channel, the tuner-stub would be able to do that too.
thanks,
Markus
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
