Re: [PULL] http://linuxtv.org/hg/~stoth/tda10048
Nice catch. Thanks. -Mike On Tue, 5 May 2009, Steven Toth wrote: > In addition to this: > > Please pull from http://www.linuxtv.org/hg/~stoth/tda10048 > >- tda10048: Added option to block i2c gate control from other drivers. >- pvrusb2: Ensure the PVRUSB2 disabled the i2c gate on the tda10048. > > dvb/frontends/tda10048.c| 222 +++- > dvb/frontends/tda10048.h| 17 + > video/cx23885/cx23885-dvb.c |4 > video/pvrusb2/pvrusb2-devattr.c |3 > 4 files changed, 244 insertions(+), 2 deletions(-) > > This fixes a bug in the current PVRUSB2 tree where DVB-T is not working due > to multiple repeaters upsteam of the tuner on the same i2c bus. > > - Steve > > Steven Toth wrote: > > Mauro, > > > > Please pull from http://www.linuxtv.org/hg/~stoth/tda10048 > > > >- tda10048: Add ability to select I/F at attach time. > >- cx23885: For tda10048 boards ensure we specify the I/F > >- pvrusb2: Ensure we specify the I/F at attach time. > > > > dvb/frontends/tda10048.c| 219 +++- > > dvb/frontends/tda10048.h| 14 + > > video/cx23885/cx23885-dvb.c |4 > > video/pvrusb2/pvrusb2-devattr.c |2 > > 4 files changed, 237 insertions(+), 2 deletions(-) > > > > The TDA10048 used to have a hard-coded I/F, I've improved this to support > > different I/F's and ensured that all current bridge drivers specify their > > needs. > > > > Regards, > > > > - Steve > > > > -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pvrusb2: Don't use the internal i2c client list
On Thu, 30 Apr 2009, Mike Isely wrote:
> On Thu, 30 Apr 2009, Jean Delvare wrote:
>
> > The i2c core used to maintain a list of client for each adapter. This
> > is a duplication of what the driver core already does, so this list
> > will be removed as part of a future cleanup. Anyone using this list
> > must stop doing so.
> >
> > For pvrusb2, I propose the following change, which should lead to an
> > equally informative output. The only difference is that i2c clients
> > which are not a v4l2 subdev won't show up, but I guess this case is
> > not supposed to happen anyway.
>
> It will happen for anything i2c used by v4l which itself is not really a
> part of v4l. That would include, uh, lirc.
>
> I will review and test this first chance I get which should be tomorrow.
>
I've merged and tested this patch. It behaves as expected.
I'm putting together a bunch of pvrusb2 changesets right now anyway.
I've pulled this one into the collection, with appropriate attributions
of course.
-Mike
>
>
> >
> > Signed-off-by: Jean Delvare
> > Cc: Mike Isely
> > ---
> > Mike, can you please review and test this patch? Thanks.
> >
> > linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c | 56
> > +--
> > 1 file changed, 13 insertions(+), 43 deletions(-)
> >
> > --- v4l-dvb.orig/linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> > 2009-04-30 16:52:32.0 +0200
> > +++ v4l-dvb/linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c 2009-04-30
> > 17:20:37.0 +0200
> > @@ -4920,65 +4920,35 @@ static unsigned int pvr2_hdw_report_clie
> > unsigned int tcnt = 0;
> > unsigned int ccnt;
> > struct i2c_client *client;
> > - struct list_head *item;
> > - void *cd;
> > const char *p;
> > unsigned int id;
> >
> > - ccnt = scnprintf(buf, acnt, "Associated v4l2-subdev drivers:");
> > + ccnt = scnprintf(buf, acnt, "Associated v4l2-subdev drivers and I2C
> > clients:\n");
> > tcnt += ccnt;
> > v4l2_device_for_each_subdev(sd, &hdw->v4l2_dev) {
> > id = sd->grp_id;
> > p = NULL;
> > if (id < ARRAY_SIZE(module_names)) p = module_names[id];
> > if (p) {
> > - ccnt = scnprintf(buf + tcnt, acnt - tcnt, " %s", p);
> > + ccnt = scnprintf(buf + tcnt, acnt - tcnt, " %s:", p);
> > tcnt += ccnt;
> > } else {
> > ccnt = scnprintf(buf + tcnt, acnt - tcnt,
> > -" (unknown id=%u)", id);
> > +" (unknown id=%u):", id);
> > tcnt += ccnt;
> > }
> > - }
> > - ccnt = scnprintf(buf + tcnt, acnt - tcnt, "\n");
> > - tcnt += ccnt;
> > -
> > - ccnt = scnprintf(buf + tcnt, acnt - tcnt, "I2C clients:\n");
> > - tcnt += ccnt;
> > -
> > - mutex_lock(&hdw->i2c_adap.clist_lock);
> > - list_for_each(item, &hdw->i2c_adap.clients) {
> > - client = list_entry(item, struct i2c_client, list);
> > - ccnt = scnprintf(buf + tcnt, acnt - tcnt,
> > -" %s: i2c=%02x", client->name, client->addr);
> > - tcnt += ccnt;
> > - cd = i2c_get_clientdata(client);
> > - v4l2_device_for_each_subdev(sd, &hdw->v4l2_dev) {
> > - if (cd == sd) {
> > - id = sd->grp_id;
> > - p = NULL;
> > - if (id < ARRAY_SIZE(module_names)) {
> > - p = module_names[id];
> > - }
> > - if (p) {
> > - ccnt = scnprintf(buf + tcnt,
> > -acnt - tcnt,
> > -" subdev=%s", p);
> > - tcnt += ccnt;
> > - } else {
> > - ccnt = scnprintf(buf + tcnt,
> > -acnt - tcnt,
> > -" subdev= id %u)",
> > -id);
> > - tcnt += ccnt;
> &
Re: [PATCH] media: remove driver_data direct access of struct device
Acked-By: Mike Isely
Note #1: I am just acking the pvrusb2 part of this.
Note #2: I am immediately pulling the pvrusb2 part of these changes into
that driver.
-Mike
On Thu, 30 Apr 2009, Greg Kroah-Hartman wrote:
> From: Greg Kroah-Hartman
>
> In the near future, the driver core is going to not allow direct access
> to the driver_data pointer in struct device. Instead, the functions
> dev_get_drvdata() and dev_set_drvdata() should be used. These functions
> have been around since the beginning, so are backwards compatible with
> all older kernel versions.
>
>
> Cc: Mauro Carvalho Chehab
> Cc: linux-media@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman
>
> ---
> drivers/media/dvb/firewire/firedtv-1394.c |4 ++--
> drivers/media/dvb/firewire/firedtv-dvb.c|2 +-
> drivers/media/video/pvrusb2/pvrusb2-sysfs.c | 22 +++---
> 3 files changed, 14 insertions(+), 14 deletions(-)
>
> --- a/drivers/media/dvb/firewire/firedtv-1394.c
> +++ b/drivers/media/dvb/firewire/firedtv-1394.c
> @@ -225,7 +225,7 @@ fail_free:
>
> static int node_remove(struct device *dev)
> {
> - struct firedtv *fdtv = dev->driver_data;
> + struct firedtv *fdtv = dev_get_drvdata(dev);
>
> fdtv_dvb_unregister(fdtv);
>
> @@ -242,7 +242,7 @@ static int node_remove(struct device *de
>
> static int node_update(struct unit_directory *ud)
> {
> - struct firedtv *fdtv = ud->device.driver_data;
> + struct firedtv *fdtv = dev_get_drvdata(&ud->device);
>
> if (fdtv->isochannel >= 0)
> cmp_establish_pp_connection(fdtv, fdtv->subunit,
> --- a/drivers/media/dvb/firewire/firedtv-dvb.c
> +++ b/drivers/media/dvb/firewire/firedtv-dvb.c
> @@ -268,7 +268,7 @@ struct firedtv *fdtv_alloc(struct device
> if (!fdtv)
> return NULL;
>
> - dev->driver_data= fdtv;
> + dev_set_drvdata(dev, fdtv);
> fdtv->device= dev;
> fdtv->isochannel= -1;
> fdtv->voltage = 0xff;
> --- a/drivers/media/video/pvrusb2/pvrusb2-sysfs.c
> +++ b/drivers/media/video/pvrusb2/pvrusb2-sysfs.c
> @@ -539,7 +539,7 @@ static void class_dev_destroy(struct pvr
>&sfp->attr_unit_number);
> }
> pvr2_sysfs_trace("Destroying class_dev id=%p",sfp->class_dev);
> - sfp->class_dev->driver_data = NULL;
> + dev_set_drvdata(sfp->class_dev, NULL);
> device_unregister(sfp->class_dev);
> sfp->class_dev = NULL;
> }
> @@ -549,7 +549,7 @@ static ssize_t v4l_minor_number_show(str
>struct device_attribute *attr, char *buf)
> {
> struct pvr2_sysfs *sfp;
> - sfp = (struct pvr2_sysfs *)class_dev->driver_data;
> + sfp = dev_get_drvdata(class_dev);
> if (!sfp) return -EINVAL;
> return scnprintf(buf,PAGE_SIZE,"%d\n",
>pvr2_hdw_v4l_get_minor_number(sfp->channel.hdw,
> @@ -561,7 +561,7 @@ static ssize_t bus_info_show(struct devi
>struct device_attribute *attr, char *buf)
> {
> struct pvr2_sysfs *sfp;
> - sfp = (struct pvr2_sysfs *)class_dev->driver_data;
> + sfp = dev_get_drvdata(class_dev);
> if (!sfp) return -EINVAL;
> return scnprintf(buf,PAGE_SIZE,"%s\n",
>pvr2_hdw_get_bus_info(sfp->channel.hdw));
> @@ -572,7 +572,7 @@ static ssize_t hdw_name_show(struct devi
>struct device_attribute *attr, char *buf)
> {
> struct pvr2_sysfs *sfp;
> - sfp = (struct pvr2_sysfs *)class_dev->driver_data;
> + sfp = dev_get_drvdata(class_dev);
> if (!sfp) return -EINVAL;
> return scnprintf(buf,PAGE_SIZE,"%s\n",
>pvr2_hdw_get_type(sfp->channel.hdw));
> @@ -583,7 +583,7 @@ static ssize_t hdw_desc_show(struct devi
>struct device_attribute *attr, char *buf)
> {
> struct pvr2_sysfs *sfp;
> - sfp = (struct pvr2_sysfs *)class_dev->driver_data;
> + sfp = dev_get_drvdata(class_dev);
> if (!sfp) return -EINVAL;
> return scnprintf(buf,PAGE_SIZE,"%s\n",
>pvr2_hdw_get_desc(sfp->channel.hdw));
> @@ -595,7 +595,7 @@ static ssize_t v4l_radio_minor_number_sh
> char *buf)
> {
> struct pvr2_sysfs *sfp;
> - sfp = (struct pvr2_sysfs *)class_dev->driver_data;
> + sfp = dev_get_drvdata(class_dev);
> if (!sfp) return -EINVAL;
> return scnprin
Re: [PATCH 2/6] ir-kbd-i2c: Switch to the new-style device binding model
Jean: I have another idea that I think you'll like. I'm putting the finishing touches on the patch right now. What I have implements correct ir_video loading for the pvrusb2 driver. It also includes a lookup table (though with only 1 entry right now) to determine the proper I2C address and I use i2c_new_device() now instead of i2c_new_probed_device(). I've also renamed things to be "ir_video" everywhere instead of ir_kbd_i2c as was discussed. In particular the disable option is there like before, but now it's called disable_autoload_ir_video. So far this is exactly what I was saying before. But I'm also making one more change: the disable_autoload_ir_video option default value will - for now - be 1, i.e. everything above I just described starts off disabled. This means that the entire patch can be pulled _right_ _now_ without breaking anything at all, because the outward behavior is still unchanged. Then, when you're ready to bring your stuff in, all you need to do is include a 1-line change to pvrusb2-i2c-core.c to switch the default value of disable_autoload_ir_video back to 0, which now enables all the corresponding pvrusb2 changes that you need to avoid any breakage inside my driver. Just to be absolutely crystal clear, here's the change you will be able to do once these changes are in: diff -r 8d2e1361520c linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c --- a/linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c Fri May 01 20:23:39 2009 -0500 +++ b/linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c Fri May 01 20:24:23 2009 -0500 @@ -43,7 +43,7 @@ module_param_array(ir_mode, int, NULL, 0444); MODULE_PARM_DESC(ir_mode,"specify: 0=disable IR reception, 1=normal IR"); +static int pvr2_disable_ir_video; -static int pvr2_disable_ir_video = 1; module_param_named(disable_autoload_ir_video, pvr2_disable_ir_video, int, S_IRUGO|S_IWUSR); MODULE_PARM_DESC(disable_autoload_ir_video, So with all this, what I am setting up right now will cause no harm to the existing situation, requires no actual messing with module options, and once you're reading, just include the 1-line change above and you're set. There's no "race here", no gap in IR handling. -Mike On Thu, 23 Apr 2009, Mike Isely wrote: > > Hi Jean, > > I had actually written out a longer, detailed, point-by-point reply > earlier today, but before I could finish it I got interrupted with a > crisis. And then another. And that's kind of how my day went. Now I'm > finally back to this, but I have another e-mail debacle to immediately > deal with (thank you pobox.com, not!) so I'm tossing that unfinished > lengthy reply. I think I can sum this whole thing up very simply. > Here's what I think should be happening: > > 1a. Your existing IR changeset, minus the pvrusb2 changes, should be > merged. > > 1b. In parallel with (1a) I modify my pvrusb2 changeset to use the right > module name and I adjust the driver option name to match. > > 2. My pvrusb2 changeset, with changes from (1b) is merged. > > 3. Andy's proposed change for ir_kbd_i2c to support additional IR > devices is investigated and probably merged. > > 4. I test the changed ir_kbd_i2c against additional pvrusb2 devices > known not to work previously with ir_kbd_i2c. If they work, then I will > create a pvrusb2 patch to load ir_video in those cases as well as the > cases already set up (which still won't cover all possible > pvrusb2-driven devices). > > I think this satisfies the remaining issues, except that from between > steps 1 and 2 ir_kbd_i2c won't work with the pvrusb2 driver. But you > know, I'm OK with that. I expect (2) to happen within a few days after > (1) so the impact should be minimal. It certainly won't escape into the > kernel tree that way. In addition, my impression from the community of > pvrusb2 users is that the preferred IR strategy is lirc anyway, and it's > a foregone conclusion that they are all going to be, uh, impacted once > your compatibility parts are gone from i2c. From where I'm sitting the > "gap" from (1) to (2) is trivial compared to that impending mess - > realize I'm not complaining since after all (a) it really falls to the > lirc developers to fix that, (b) you do need to get your changes in, and > (c) there's little I can do for lirc there except to keep it working as > long as possible with the pvrusb2 driver. I'm just pointing out that > I'm OK with the step 1 -> 2 gap for the pvrusb2 driver because it's > small and will be nothing compared to what's about to happen with lirc. > > If you still don't like any of tha
Re: [cron job] v4l-dvb daily build 2.6.22 and up: ERRORS, 2.6.16-2.6.21: ERRORS
On Fri, 1 May 2009, Alexey Klimov wrote: > Hello, > > On Mon, Apr 20, 2009 at 3:59 AM, Mike Isely wrote: [...] > > > > So the kernel already has this; it just needs to be pulled back into > > v4l-dvb. It's an obvious trivial thing for now and I've acked it there. > > Obviously we're getting had here because you're compiling against a > > kernel snapshot that's been changed but v4l-dvb doesn't have the > > corresponding change in its local copy of the pvrusb2 driver. Part of > > the fun of synchronizing changes from different trees :-( > > Well, good to know that this thing is already fixed. > I'm very sorry for the mess. No apology needed. Really - this "mess" wasn't caused by you. If anything I should have just immediately pulled that patch into hg and not waited for it to trickle back to Mauro. That would have avoided the error. So, all I can say is that I'm sorry you had to hit this! -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
Re: [PATCH] pvrusb2: Don't use the internal i2c client list
On Thu, 30 Apr 2009, Jean Delvare wrote:
> The i2c core used to maintain a list of client for each adapter. This
> is a duplication of what the driver core already does, so this list
> will be removed as part of a future cleanup. Anyone using this list
> must stop doing so.
>
> For pvrusb2, I propose the following change, which should lead to an
> equally informative output. The only difference is that i2c clients
> which are not a v4l2 subdev won't show up, but I guess this case is
> not supposed to happen anyway.
It will happen for anything i2c used by v4l which itself is not really a
part of v4l. That would include, uh, lirc.
I will review and test this first chance I get which should be tomorrow.
-Mike
>
> Signed-off-by: Jean Delvare
> Cc: Mike Isely
> ---
> Mike, can you please review and test this patch? Thanks.
>
> linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c | 56
> +--
> 1 file changed, 13 insertions(+), 43 deletions(-)
>
> --- v4l-dvb.orig/linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> 2009-04-30 16:52:32.0 +0200
> +++ v4l-dvb/linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c 2009-04-30
> 17:20:37.0 +0200
> @@ -4920,65 +4920,35 @@ static unsigned int pvr2_hdw_report_clie
> unsigned int tcnt = 0;
> unsigned int ccnt;
> struct i2c_client *client;
> - struct list_head *item;
> - void *cd;
> const char *p;
> unsigned int id;
>
> - ccnt = scnprintf(buf, acnt, "Associated v4l2-subdev drivers:");
> + ccnt = scnprintf(buf, acnt, "Associated v4l2-subdev drivers and I2C
> clients:\n");
> tcnt += ccnt;
> v4l2_device_for_each_subdev(sd, &hdw->v4l2_dev) {
> id = sd->grp_id;
> p = NULL;
> if (id < ARRAY_SIZE(module_names)) p = module_names[id];
> if (p) {
> - ccnt = scnprintf(buf + tcnt, acnt - tcnt, " %s", p);
> + ccnt = scnprintf(buf + tcnt, acnt - tcnt, " %s:", p);
> tcnt += ccnt;
> } else {
> ccnt = scnprintf(buf + tcnt, acnt - tcnt,
> - " (unknown id=%u)", id);
> + " (unknown id=%u):", id);
> tcnt += ccnt;
> }
> - }
> - ccnt = scnprintf(buf + tcnt, acnt - tcnt, "\n");
> - tcnt += ccnt;
> -
> - ccnt = scnprintf(buf + tcnt, acnt - tcnt, "I2C clients:\n");
> - tcnt += ccnt;
> -
> - mutex_lock(&hdw->i2c_adap.clist_lock);
> - list_for_each(item, &hdw->i2c_adap.clients) {
> - client = list_entry(item, struct i2c_client, list);
> - ccnt = scnprintf(buf + tcnt, acnt - tcnt,
> - " %s: i2c=%02x", client->name, client->addr);
> - tcnt += ccnt;
> - cd = i2c_get_clientdata(client);
> - v4l2_device_for_each_subdev(sd, &hdw->v4l2_dev) {
> - if (cd == sd) {
> - id = sd->grp_id;
> - p = NULL;
> - if (id < ARRAY_SIZE(module_names)) {
> - p = module_names[id];
> - }
> - if (p) {
> - ccnt = scnprintf(buf + tcnt,
> - acnt - tcnt,
> - " subdev=%s", p);
> - tcnt += ccnt;
> - } else {
> - ccnt = scnprintf(buf + tcnt,
> - acnt - tcnt,
> - " subdev= id %u)",
> - id);
> - tcnt += ccnt;
> - }
> - break;
> - }
> + client = v4l2_get_subdevdata(sd);
> + if (client) {
> + ccnt = scnprintf(buf + tcnt, acnt - tcnt,
> + " %s @ %02x\n", client->name,
> + client->addr);
> + tcnt += ccnt;
> + } else {
> + ccnt = scnprintf(buf + tcnt, acnt - tcnt,
> + " no i2c client\n");
> +
Re: [PATCH 2/6] ir-kbd-i2c: Switch to the new-style device binding model
Hi Jean, I had actually written out a longer, detailed, point-by-point reply earlier today, but before I could finish it I got interrupted with a crisis. And then another. And that's kind of how my day went. Now I'm finally back to this, but I have another e-mail debacle to immediately deal with (thank you pobox.com, not!) so I'm tossing that unfinished lengthy reply. I think I can sum this whole thing up very simply. Here's what I think should be happening: 1a. Your existing IR changeset, minus the pvrusb2 changes, should be merged. 1b. In parallel with (1a) I modify my pvrusb2 changeset to use the right module name and I adjust the driver option name to match. 2. My pvrusb2 changeset, with changes from (1b) is merged. 3. Andy's proposed change for ir_kbd_i2c to support additional IR devices is investigated and probably merged. 4. I test the changed ir_kbd_i2c against additional pvrusb2 devices known not to work previously with ir_kbd_i2c. If they work, then I will create a pvrusb2 patch to load ir_video in those cases as well as the cases already set up (which still won't cover all possible pvrusb2-driven devices). I think this satisfies the remaining issues, except that from between steps 1 and 2 ir_kbd_i2c won't work with the pvrusb2 driver. But you know, I'm OK with that. I expect (2) to happen within a few days after (1) so the impact should be minimal. It certainly won't escape into the kernel tree that way. In addition, my impression from the community of pvrusb2 users is that the preferred IR strategy is lirc anyway, and it's a foregone conclusion that they are all going to be, uh, impacted once your compatibility parts are gone from i2c. From where I'm sitting the "gap" from (1) to (2) is trivial compared to that impending mess - realize I'm not complaining since after all (a) it really falls to the lirc developers to fix that, (b) you do need to get your changes in, and (c) there's little I can do for lirc there except to keep it working as long as possible with the pvrusb2 driver. I'm just pointing out that I'm OK with the step 1 -> 2 gap for the pvrusb2 driver because it's small and will be nothing compared to what's about to happen with lirc. If you still don't like any of that, well, then I give up. In that case, then merge your changes with the pvrusb2 changes included. I won't ack them, but I'll just deal with it later. Because while your changes might keep ir_kbd_i2c going, they will also immediately break the more-useful and apparently more-used lirc (by always binding ir_kbd_i2c even in cases in the pvrusb2 driver where it's known that it can't even work but lirc would have) which as far as I'm concerned is far worse than the step 1 - 2 gap above. But it's just another gap and I'll push another pvrusb2 changeset on top to clean it up. So just do whatever you think is best right now, if you disagree with the sequence above. Now I will go off and deal with the steamer that pobox.com has just handed me :-( -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [cron job] v4l-dvb daily build 2.6.22 and up: ERRORS, 2.6.16-2.6.21: ERRORS
On Mon, 20 Apr 2009, Alexey Klimov wrote: [...] > When trying to compile v4l-dvb tree under 2.6.30-rc2 (up-to-date) i > have such error with pvr2 module: > > CC [M] /w/new/v4l-dvb/v4l/pvrusb2-hdw.o > /w/new/v4l-dvb/v4l/pvrusb2-hdw.c: In function 'pvr2_upload_firmware1': > /w/new/v4l-dvb/v4l/pvrusb2-hdw.c:1474: error: implicit declaration of > function 'usb_settoggle' > /w/new/v4l-dvb/v4l/pvrusb2-hdw.c: In function 'pvr2_hdw_load_modules': > /w/new/v4l-dvb/v4l/pvrusb2-hdw.c:2133: warning: format not a string > literal and no format arguments > make[3]: *** [/w/new/v4l-dvb/v4l/pvrusb2-hdw.o] Error 1 > make[2]: *** [_module_/w/new/v4l-dvb/v4l] Error 2 > > It's probably due to this git commit: > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=3444b26afa145148951112534f298bdc554ec789 > > I don't have idea how to fix it fast and correctly. This might explain things a bit. The following thread took place on linux-usb on 7-April: On Tue, 7 Apr 2009, Greg KH wrote: > On Tue, Apr 07, 2009 at 05:31:55PM +, David Vrabel wrote: > > Wireless USB endpoint state has a sequence number and a current > > window and not just a single toggle bit. So allow HCDs to provide a > > endpoint_reset method and call this or clear the software toggles as > > required (after a clear halt). > > > > usb_settoggle() and friends are then HCD internal and are moved into > > core/hcd.h. > > You remove this api, yet the pvrusb2 driver used it, and you don't seem > to have resolved the issue where it was calling it: > > > diff --git a/drivers/media/video/pvrusb2/pvrusb2-hdw.c > > b/drivers/media/video/pvrusb2/pvrusb2-hdw.c > > index fa304e5..b86682d 100644 > > --- a/drivers/media/video/pvrusb2/pvrusb2-hdw.c > > +++ b/drivers/media/video/pvrusb2/pvrusb2-hdw.c > > @@ -1418,7 +1418,6 @@ static int pvr2_upload_firmware1(struct pvr2_hdw *hdw) > > return ret; > > } > > > > - usb_settoggle(hdw->usb_dev, 0 & 0xf, !(0 & USB_DIR_IN), 0); > > usb_clear_halt(hdw->usb_dev, usb_sndbulkpipe(hdw->usb_dev, 0 & 0x7f)); > > > > pipe = usb_sndctrlpipe(hdw->usb_dev, 0); > > Should usb_reset_endpoint() be called here instead? > Speaking as the maintainer of that driver, I'm OK with accepting this as-is for now. This is a sequence that should not interfere with normal driver operation. I will look at this further a little later (not likely before the merge window closes) and if this change turns out to cause a problem I'll make a follow-up fix upstream. Acked-By: Mike Isely -Mike So the kernel already has this; it just needs to be pulled back into v4l-dvb. It's an obvious trivial thing for now and I've acked it there. Obviously we're getting had here because you're compiling against a kernel snapshot that's been changed but v4l-dvb doesn't have the corresponding change in its local copy of the pvrusb2 driver. Part of the fun of synchronizing changes from different trees :-( Mauro: If you just want to take this as-is (or find the git commit and pull it down), I'm fine. Otherwise I'll set up a repo that you can pull from with this single-line change. The above changeset also has the following attributions: From: David Vrabel Signed-off-by: David Vrabel -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] ir-kbd-i2c: Switch to the new-style device binding model
On Sat, 18 Apr 2009, Jean Delvare wrote: > Hi again Mike, > > On Sat, 18 Apr 2009 11:25:19 +0200, Jean Delvare wrote: > > On Fri, 17 Apr 2009 18:35:55 -0500 (CDT), Mike Isely wrote: > > > I thought we were going to leave the pvrusb2 driver out of this since > > > I've already got a change ready that also includes additional logic to > > > take into account the properties of the hardware device (i.e. only > > > activate ir-kbd-i2c when we know it has a chance of working). > > > > Hmm, I thought that our latest discussions had (at least partly) > > obsoleted your patches. Remember that we want to always instantiate > > ir_video I2C devices even when ir-kbd-i2c can't driver them, otherwise > > lirc won't be able to bind to the devices in question as soon as the > > legacy binding model is gone. So the conditionals in your second patch > > (which is all that makes it differ from mine) are no longer desirable. Jean: The differences between my patch(es) and yours are: 1. My patch only attempts to bind the driver if the hardware actually supports it. 2. My patch selects the right I2C address for the case(s) where it makes sense to bind. 3. My patch provides a module option to completely disable binding. You are probably thinking about (3) but you forgot that I had also taken care of (1). Difference (1) is why I don't want your patch. If your patch gets merged I will have to partially redo my patch to make (1) work again. When I had issued my pull request to Mauro (which he didn't pull), there were actually 2 patches. The first one dealt with (1) and the second dealt with (2) and (3), while taking advantage of (1). Had Mauro pulled those patches at that time then you could have made further changes on top without losing (1). But since he didn't, it's best just to leave the pvrusb2 driver alone and I'll make the needed additional change(s) there after your stuff is merged. > > > > I'll work on lirc patches today or tomorrow, so that lirc doesn't break > > when my patches hit mainline. > > Speaking of this: do you know all the I2C addresses that can host IR > devices on pvrusb2 cards? I understand that the only address supported > by ir-kbd-i2c is 0x18, but I also need to know the addresses supported > by lirc_i2c and possibly lirc_zilog, if you happen to know this. Right now I only care about 0x18 (for 29xxx and early 24xxx devices). I noticed the thread where Andy got IR reception to work with ir-kbd-i2c using 0x71 (lirc_zilog type) and I suspect that same set of ir-kbd-i2c changes will probably work with the pvrusb2 driver for MCE 24xxx and HVR-1900/HVR-1950 devices. But I figured once Andy's stuff gets into ir-kbd-i2c I'd simply test for this and if it worked I would make the appropriate adjustments in the pvrusb2 driver to enable ir-kbd-i2c binding in those cases as well (an easy change). However even with that change, there are other pvrusb2-driven devices that cannot support ir-kbd-i2c. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6] ir-kbd-i2c: Switch to the new-style device binding model
I thought we were going to leave the pvrusb2 driver out of this since I've already got a change ready that also includes additional logic to take into account the properties of the hardware device (i.e. only activate ir-kbd-i2c when we know it has a chance of working). -Mike On Fri, 17 Apr 2009, Jean Delvare wrote: > Let card drivers probe for IR receiver devices and instantiate them if > found. Ultimately it would be better if we could stop probing > completely, but I suspect this won't be possible for all card types. > > There's certainly room for cleanups. For example, some drivers are > sharing I2C adapter IDs, so they also had to share the list of I2C > addresses being probed for an IR receiver. Now that each driver > explicitly says which addresses should be probed, maybe some addresses > can be dropped from some drivers. > > Also, the special cases in saa7134-i2c should probably be handled on a > per-board basis. This would be more efficient and less risky than always > probing extra addresses on all boards. I'll give it a try later. > > Signed-off-by: Jean Delvare > Cc: Mauro Carvalho Chehab > Cc: Hans Verkuil > Cc: Andy Walls > Cc: Mike Isely > --- > linux/drivers/media/video/bt8xx/bttv-i2c.c | 21 + > linux/drivers/media/video/cx231xx/cx231xx-cards.c| 11 > linux/drivers/media/video/cx231xx/cx231xx-i2c.c |3 > linux/drivers/media/video/cx231xx/cx231xx.h |1 > linux/drivers/media/video/cx23885/cx23885-i2c.c | 12 + > linux/drivers/media/video/cx88/cx88-i2c.c| 13 + > linux/drivers/media/video/em28xx/em28xx-cards.c | 20 + > linux/drivers/media/video/em28xx/em28xx-i2c.c|3 > linux/drivers/media/video/em28xx/em28xx-input.c |6 > linux/drivers/media/video/em28xx/em28xx.h|1 > linux/drivers/media/video/ir-kbd-i2c.c | 200 > +++--- > linux/drivers/media/video/ivtv/ivtv-i2c.c| 31 ++ > linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c | 24 ++ > linux/drivers/media/video/saa7134/saa7134-i2c.c |3 > linux/drivers/media/video/saa7134/saa7134-input.c| 86 ++- > linux/drivers/media/video/saa7134/saa7134.h |1 > linux/include/media/ir-kbd-i2c.h |2 > 17 files changed, 244 insertions(+), 194 deletions(-) [...] -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Anticipating lirc breakage
On Tue, 7 Apr 2009, Jean Delvare wrote: > I'll rework my patch set to implement strategy #1 and post it when I'm > done. As far as I can see this should be very similar to my original > attempt, with just "ir_video" devices instead or "ir-kbd" devices, and > also fixes for the minor issues that have been reported. > > Do you want me to include pvrusb2 in my new patch set, or should I still > leave it to you? If you were to include pvrusb2, you would also need the changeset which expands the "IR scheme" handling to make it possible to correctly select when to bind the driver. But Mauro hasn't pulled that so you can't easily build on it right now. And as I understand it, the only real impact in the second changeset in the pending series is just the name of the module (i.e. change "ir-kbd" to "ir_video"). It's extra work for you to do this. So just let me deal with it. If my understanding above is correct, I'll just fix the second patch and the pvrusb2 driver should be ready to go for this. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Anticipating lirc breakage
On Mon, 6 Apr 2009, Jean Delvare wrote: > Hi all, > > In the light of recent discussions and planed changes to the i2c > subsystem and the ir-kbd-i2c driver, I will try to summarize the > situation and make some proposals. Note that I am really not sure what > we want to do, so this is a true request for opinions. > > First of all, the original reason for these changes is that I want to > get rid of the legacy i2c binding model. As far as IR support is > concerned, this means two things: > * The ir-kbd-i2c driver must be converted to the new i2c binding model. > I have been working on this already. > * The removal of the legacy model will break lirc_i2c and possibly > other lirc drivers. These will have to be converted to the new i2c > binding model as well. > > While discussing my changes to ir-kbd-i2c, I received objections that > these would adversely affect lirc users, and proposals were made to > work around this problem, either by the means of module parameters, or > by adding per-card logic in the bridge drivers. While this temporarily > addresses the conflict with lirc, I feel like it is wasted energy > because the second point is much more critical for lirc users. I'm > going to remove the legacy i2c model pretty soon now and lirc_i2c and > friends will have to be updated. This means two things: It wasn't really wasting that much energy for me. The change was simple and now that you made me look at this issue more closely, I realize I need to do something more serious in the pvrusb2 driver anyway to enable better control over which IR receiver(s) are to be enabled when the user has multiple devices. So in the end for me at least, it wasn't a waste. > * There is no point in refining the ir-kbd-i2c conversion for users of > the current lirc drivers, because the removal of the legacy i2c model > will break these drivers a lot more anyway. > * We need to come up with a strategy that makes it possible for lirc > modules to still work afterwards. This is not trivial because the new > i2c binding model makes life much harder for rogue/out-of-tree i2c > drivers (note, this is just a side effect, the new model was not > designed with this in mind.) > > The main technical problems I see as far as converting lirc to the new > i2c binding model is concerned are: > * Going the .detect() route is not as flexible as it was beforehand. In > particular, having per-board probed address lists is no longer > possible. It is possible to filter the addresses on a per-board basis > after the fact, but the probes will still be issued for all addresses. > I seem to remember that probing random addresses did cause trouble in > the past on some boards, so I doubt we want to go that route. This is > the reason why I decided to NOT go the .detect() route for ir-kbd-i2c > conversion. > * If we don't use .detect(), the bridge drivers must instantiate the > devices themselves. That's what my ir-kbd-i2c patches do. One > requirement is then that the bridge drivers and the chip drivers agree > on the i2c device name(s). This was true for ir-kbd-i2c, it is true for > lirc as well. Where it becomes difficult is that lirc lives outside of > the kernel, while bridge drivers live inside the kernel. This will make > the changes more difficult to synchronize. Probably a good incentive > for lirc developers to merge their drivers into the kernel tree. > > While I think we all agree that lirc drivers should be merged in the > kernel tree, it is pretty clear that it won't happen tomorrow. And, > more importantly, it won't happen before I get rid of the legacy i2c > binding model. So we need to come up with a design which makes it > possible to keep using out-of-tree lirc drivers. It doesn't need to be > perfect, but it has to somewhat work for now. Yup. Totally agree. > > My initial proposal made bridge drivers create "ir-kbd" [1] I2C devices > for the ir-kbd-i2c driver to use. Mike Isely changed this in the pvrusb2 > bridge driver to only instantiate the devices for boards on which > ir-kbd-i2c is known to work. While this makes sense for the current > situation (lirc_i2c is a legacy i2c driver) it will break as soon as > lirc_i2c is converted to a new-style i2c driver: the converted lirc_i2c > driver will need I2C devices to bind to, and the pvrusb2 driver won't > create them. Right, because we haven't addressed the question of still making possible the choice of which actual driver to load. The change I proposed and implemented (within the pvrusb2 driver) was just a simple low-risk attempt to allow for the status quo to still be possible within the pvrusb2 driver. That will work for _right this moment_, bu
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2
On Mon, 6 Apr 2009, Jean Delvare wrote: > Hi Mike, > > Please note: I think the long post I just sent makes part of this > discussion obsolete from a technical perspective. But still interesting > from a functional perspective, which is why I am following up. I plan a reply to your RFC as well, probably later on tonight. > > On Mon, 6 Apr 2009 10:03:00 -0500 (CDT), Mike Isely wrote: > > On Mon, 6 Apr 2009, Jean Delvare wrote: > > > Again, ir-kbd-i2c does _not_ auto-load. What my code (and now yours) > > > does is instantiating an i2c device named "ir-kbd". _If_ the ir-kbd-i2c > > > driver is later loaded, it will bind to that device. We might let udev > > > load the ir-kbd-i2c driver at some point in the future, but clearly we > > > need to sort out the lirc case beforehand, otherwise some users will > > > hit the problems you have anticipated, and we don't want this to happen. > > > > > > So, your module parameter is improperly named. Setting it to 1 does > > > prevent the pvrusb2 driver from instantiating the "ir-kbd" device, and > > > that's all. > > > > The module parameter is named "disabled_autoload_ir_kbd", how is that > > wrong? The name is based on the internal receiver name "ir-kbd". > > There's no "[-_]i2c" in the name. The parameter's description also does > > not reference ir-kbd-i2c by name either. I did it that way specifically > > for the reason you point out here. > > I was perfectly fine with the "ir_kbd" part. The part I complain about > is "autoload", because the original mechanism doesn't involve any > autoloading. But from the viewpoint of a user of the pvrusb2 driver, that is in fact what will appear to happen. 1. User plugs in device. 2. Driver (pvrusb2) instantiates self. 3. Driver automatically attempts to load and bind ir-kbd-i2c to the IR receiver, hence "autoload". Yes I know ir-kbd-i2c no longer autoloads itself; that is a major goal of the conversion. But with the pvrusb2 change to explicitly bind it, the behavior from the view of the user is still that of an autoload operation. I think we're just arguing semantics, but it's why I put "autoload" in the name. However given the realization below about multiple devices, I think I need to step back and look at this from a larger scope (e.g. make it an array, merge with the fairly clunky ir_mode switch already in the driver and clean that up, etc). [...] > > This morning I actually realized another use-case that has been missed. > > It was likely an issue before anyway, but it got me thinking about this: > > If a user has multiple devices attached to one system, he probably won't > > want all of the corresponding IR receivers enabled - that would just > > trigger redundant input events. With a PCI-hosted ivtv device this is > > not really an issue - because there one can just decline to plug in the > > actual IR sensor. But with USB-hosted devices, the IR sensor is an > > integral part of the device and can't be unplugged. OK, well such a > > user could instead just choose to put a piece of tape over the window or > > stick all but one device in a box (and causing potential heat problems > > if it isn't ventilated), but that approach is well, inelegant. I think > > this argues for a better solution in the bridge driver to selectively > > disable IR on a per-device instance basis. There's already some logic > > in the pvrusb2 driver to do this, but it's clumsy and wasn't intended to > > solve that particular use-case. I need to consider this further and do > > some cleanup. This use-case may actually suggest the disable option > > should be expanded and possibly made permanent. > > I agree. I presume that this is one of the reasons why some bridge > drivers have a disable_ir parameter (the other reason being lirc). It > would probably make sense to not only keep these module parameters even > after lirc is merged into the kernel tree, but turn them into arrays, > so that IR receivers can be disabled selectively. This would address > the problem you just raised. > > But all this can be done after the conversion work it finished. I believe I can solve this for the pvrusb2 driver without entanglement with the conversion work underway. Pieces of the solution are already in the driver; I just need to clean this up. In any case, I'm not going to worry about it immediately. I've been neglecting some non-Linux tasks, like filing bills and finishing my tax return :-) -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2
On Mon, 6 Apr 2009, Jean Delvare wrote: > Hi Mike, > > I'll answer all your questions and express my concerns in this reply, to > avoid spreading the info all around the discussion thread. > > On Mon, 6 Apr 2009 00:19:23 -0500 (CDT), Mike Isely wrote: > > Please pull from http://linuxtv.org/hg/~mcisely/pvrusb2 for two pvrusb2 > > changesets. One sets up the ability to track what kind of IR receiver > > is expected, > > Looks very good. The more we know about each board, the less probing we > have to do, the better. > > > the other optionally autoloads ir-kbd-i2c based on that > > result and a module option that can be used to disable that autoloading. > > Again, ir-kbd-i2c does _not_ auto-load. What my code (and now yours) > does is instantiating an i2c device named "ir-kbd". _If_ the ir-kbd-i2c > driver is later loaded, it will bind to that device. We might let udev > load the ir-kbd-i2c driver at some point in the future, but clearly we > need to sort out the lirc case beforehand, otherwise some users will > hit the problems you have anticipated, and we don't want this to happen. > > So, your module parameter is improperly named. Setting it to 1 does > prevent the pvrusb2 driver from instantiating the "ir-kbd" device, and > that's all. The module parameter is named "disabled_autoload_ir_kbd", how is that wrong? The name is based on the internal receiver name "ir-kbd". There's no "[-_]i2c" in the name. The parameter's description also does not reference ir-kbd-i2c by name either. I did it that way specifically for the reason you point out here. I was originally going to use the name that Hans had suggested but then decided otherwise because (1) I decided to follow your desire and make it a disable option, (2) Hans' suggested name did in fact encode the name ir-kbd-i2c in the module option. > > Speaking of this module parameter, I was a little worried at first that > you wanted an inverted logic for it in pvrusb2 compared to what some > other bridge drivers (saa7134, em28xx, cx231xx), but thinking a bit > more about I came to the conclusion that it was OK because it matched > the history of the pvrusb2 driver. Now I see that you followed their > logic (enabled is the default) but you use a different module parameter > name (disable_autoload_ir_kbd vs. disable_ir). I think there would be > some value in sticking to a common name in all bridge drivers (like we > have the standard video_nr module parameter.) > > That being said, I will not insist on this. My feeling is that this is > all temporary anyway, because the removal of the legacy i2c model will > break lirc and the main point is to decide how we will fix it. I'll > post a separate summary with proposals. Depending on what we do, the > module parameter you added is likely to become obsolete. I did want it to be an enable parameter, in order to match previous driver behavior. But whether it is an enable or a disable option, in one use-case somebody has to set it. So I relented and made it a disable option. > > > This is the result of what I had posted about an hour ago. It looks > > like a lot of lines, but it was all basically trivial stuff. > > > > Note that these changes are safe; nothing is regressed here and this > > does not depend on anyone else's changes. Even though that second > > changeset won't really do much until Jean's ir-kbd-i2c stuff is merged, > > the fact is that it won't cause any harm either. Since the pvrusb2 > > driver wasn't previously autoloading ir-kbd-i2c anyway, this change > > therefore breaks nothing. > > For completeness, your second patch actually breaks the ir-kbd-i2c use > case. Your code will instantiate a new-style "ir-kbd" device which the > legacy ir-kbd-i2c can't use. As the instantiated device makes the > address busy, the probing logic of legacy ir-kbd-i2c driver will skip > it. Without my changes, all users will have to pass > disable_autoload_ir_kbd=1, whether they want to use lirc or ir-kbd-i2c, > otherwise they lose IR support. Well yes. I was saying "no harm" in the sense that everything that was possible before is still possible now, though perhaps with the module option set. > > But honestly that doesn't matter much, I think, because the idea is to > merge my changes quickly. Yes, exactly. [...] This morning I actually realized another use-case that has been missed. It was likely an issue before anyway, but it got me thinking about this: If a user has multiple devices attached to one system, he probably won't want all of the corresponding IR receivers enabled - that would j
[PULL] http://linuxtv.org/hg/~mcisely/pvrusb2
Mauro: Please pull from http://linuxtv.org/hg/~mcisely/pvrusb2 for two pvrusb2 changesets. One sets up the ability to track what kind of IR receiver is expected, the other optionally autoloads ir-kbd-i2c based on that result and a module option that can be used to disable that autoloading. This is the result of what I had posted about an hour ago. It looks like a lot of lines, but it was all basically trivial stuff. Note that these changes are safe; nothing is regressed here and this does not depend on anyone else's changes. Even though that second changeset won't really do much until Jean's ir-kbd-i2c stuff is merged, the fact is that it won't cause any harm either. Since the pvrusb2 driver wasn't previously autoloading ir-kbd-i2c anyway, this change therefore breaks nothing. But it does have the desirable effect in that it should take me out of the IR debate for now and I can stop being a pain to Jean :-) Jean: I hope I didn't break any etiquette here. Though that second change is "from" me, the majority of the changeset really is from your patch to the pvrusb2 driver. I made changes to it so I didn't feel right in still trying to blame you for it ;-) Plus, if I were to hand it back to you for inclusion in your patch series then you would be dependant upon the pvrusb2 change I made to drive the decision about loading based on the device hardware. Since this change as a whole is mergeable without the rest of your changeset I felt it most expedient just to push this up now. -Mike - pvrusb2: Select, track, and report IR scheme in use with the device - pvrusb2: autoload ir-kbd when appropriate pvrusb2-devattr.c |1 + pvrusb2-devattr.h | 22 +++--- pvrusb2-hdw-internal.h |3 +++ pvrusb2-hdw.c | 18 +- pvrusb2-i2c-core.c | 40 ++-- 5 files changed, 66 insertions(+), 18 deletions(-) -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
pvrusb2 IR changes coming [was: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model]
Jean: Here's a description of what I've got on the front burner right now: 1. The pvrusb2 driver now can unambiguously know if it is dealing with a device that is known to have ir-kbd-i2c compatible IR capabilities. 2. There is a new module option, "disable_autoload_ir_kbd", which if present and set to 1 will indicate that ir-kbd should not be loaded. 2. Based upon (1) and (2), the driver will optionally attempt to load ir-kbd using the code from your patch. 3. In the pvrusb2 case, the only i2c address that currently matters is 0x18 (though I have some suspicions about another case but that can be dealt with later), so I trimmed the probe list in the register function you had added. Since calling i2c_new_probed_device() for a non-existent target driver doesn't cause any harm, then merging the above now should not result in any kind of regression. So it can go in even before the rest of your changes. That I believe also removes the objection Mauro had - this way there's no issues / dependencies. I've tested this enough to know that it at least doesn't do any further harm. I will put this up in a changeset shortly. With all that said, we should not ignore lirc and instead do whatever is reasonable to help ensure it continues to work. Though admittedly there's been plenty of opportunity to update and this whole transition has been going on for a long time. The stuff I describe above should at least keep the pvrusb2 driver out of the fray for now. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model
hat's really not right at all. The IR receiver used does not automatically mean which remote is used. What if the vendor switches remotes? That's happened with the PVR-USB2 hardware in the past (based on photos I've seen). Who's to say the next remote to be supplied is compatible? 2. Your example above is opening the video device endpoint and issuing ioctl()s that are not part of V4L. That is supposed to work?!? 3. A given IR remote may be described by much more than what 'scan codes' it produces. I don't know a lot about IR, but looking at the typical lirc definition for a remote, there's obvious timing and protocol parameters as well. Just being able to swap scan codes around is not always going to be enough. 4. I imagine that the input event framework in the kernel has a means for programmatic mapping of scan codes to key codes, but looking at ir-kbd-i2c.c, it appears to only be selecting from among a very small set of kernel-compiled translation tables. I must be missing something here. In an earlier post (from Andy?) some history was dug up about ir-kbd-i2c.c. From what I understand, the only reason ir-kbd-i2c.c came into existence was because lirc was late in supporting 2.6.x series kernels and Gerd needed *something* to allow IR to work. So he created this module, knowing full well that it didn't cover all possible cases. Rather it covered the common cases he cared about. That was a while ago. And we need to do all the cases - or at least not mess up what already exists elsewhere that does handle the "uncommon" cases. The lirc drivers do work in 2.6. And apparently they were on the scene before ir-kbd-i2c.c, just unfortunately not in-tree. The lirc drivers really need to get into the kernel. From where I'm sitting the long term goal should be to get lirc into the kernel. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model
On Sun, 5 Apr 2009, Hans Verkuil wrote: [...] > > Let's keep it simple: add a 'load_ir_kbd_i2c' module option for those > drivers that did not autoload this module. The driver author can refine > things later (I'll definitely will do that for ivtv). Yes, and I will do the same for pvrusb2. Simple is good. > > It will be interesting if someone can find out whether lirc will work at all > once autoprobing is removed from i2c. If it isn't, then perhaps that will > wake them up to the realization that they really need to move to the > kernel. It's probably going to break, and that will wake them up. There's no choice otherwise. > > The new mechanism is the right way to do it: the adapter driver has all the > information if, where and what IR is used and so should be the one to tell > the kernel what to do. Attempting to autodetect and magically figure out > what IR might be there is awkward and probably impossible to get right > 100%. > > Hell, it's wrong already: if you have another board that already loads > ir-kbd-i2c then if you load ivtv or pvrusb2 afterwards you get ir-kbd-i2c > whether you like it or not, because ir-kbd-i2c will connect to your i2c > adapter like a leech. So with the addition of a module option you at least > give back control of this to the user. Yes, I know this is a possibility. It sucks and long term the new mechanism is the solution. I don't want anyone to think I am against the new mechanism. I'm for it. But I'd like to minimize the damage potential on the way there. > > When this initial conversion is done I'm pretty sure we can improve > ir-kbd-i2c to make it easier to let the adapter driver tell it what to do. > So we don't need those horrible adapter ID tests and other magic that's > going on in that driver. But that's phase two. My impression (at least for pvrusb2-driven devices) is that the later IR receivers require a completely different driver to work properly; one can't just bolt additional features into ir-kbd-i2c for this. In lirc's case, a different driver is in fact used. But you know this already. I haven't looked at ir-kbd-i2c in a while, but one other thing I seriously did not like about it was that the mapping of IR codes to key events was effectively hardcoded in the driver. That makes it difficult to make the same driver work with different kinds of remotes. Even if the bridge driver (e.g. pvrusb2) were to supply such a map, that's still wrong because it's within the realm of possibility that the user might actually want to use a different remote transmitter (provided it is compatible enough to work with the receiver hardware). The lirc architecture solves this easily via a conf file in userspace. In fact, lirc can map multiple mappings to a single receiver, permitting it to work concurrently with more than one remote. But is such a thing even possible with ir-kbd-i2c? I know this is one reason people tend to choose lirc. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] ir-kbd-i2c conversion to the new i2c binding model
On Sun, 5 Apr 2009, Jean Delvare wrote: > Hi Mauro, > > On Sun, 5 Apr 2009 07:01:16 -0300, Mauro Carvalho Chehab wrote: > > From the discussions we already have, I noticed some points to take care of: > > > > 1) about the lirc support, I don't think we should change a kernel driver > > due > > to an out-of-tree kernel driver. As I've commented on PATCH 3/6 discussion, > > we > > need to better address this with lirc developers; > > Well, the new binding model makes it harder for "rogue" drivers such as > lirc_i2c. They will need _some_ form of cooperation from us, which will > most likely come when they get merged into the kernel tree. > > > 2) the way Mike is proposing to solve the issue with pvrusb2 will break > > userspace usage for people that have those devices whose IR work with the > > in-kernel IR i2c driver. This means that we'll cause a kernel regression > > due to > > an out-of-tree driver; It's an either/or. If nothing is done, the ir-kbd-i2c become unusable for pvrusb2 but lirc (for now) continues to work. If Jean's change is accepted as-is, then ir-kbd-i2c will be ok but now lirc is toast. If I implement what I am suggesting, then it becomes possible at least for both cases to still work, but with a module option. Not perfect, but it is the only way I see to allow this situation to retain some sanity. In the longer term, the lirc folks are going to have to change what they are doing. Fine, that's a problem they have to solve. It's nothing I can do anyting about. But I am not going to be the instigator that breaks lirc as used by the pvrusb2 driver. In the short term, implementing the module option breaks the deadlock here. Jean can continue getting rid of the old i2c model and I won't be a pain about it. > > > > 3) So far, nobody gave us any positive return that the new IR model is > > working with > > any of the touched drivers. So, more tests are needed. I'm expecting to > > have a > > positive reply for each of the touched drivers. People, please test! > > Yes, please! :) > > > Since the merge window is almost finished, IMO, we should postpone those > > changes to 2.6.31, (...) > > The legacy i2c model will be gone in 2.6.30. Really. Hans and myself > have put enough energy into this to not let it slip for just a > miserable infrared support module which I understand is hardly used by > anyone. > > So it's really up to you, either you accept my ir-kbd-i2c conversion > "now" (that is, when it has received the minimum testing and reviewing > it deserves) and ir-kbd-i2c has a chance to work in 2.6.30, or you > don't and I'll just have to mark ir-kbd-i2c as BROKEN to prevent build > failures. Accept his ir-kbd-i2c conversion now, minus the pvrusb2 changes. I will deal with the pvrusb2 driver appropriately (and immediately). That should resolve the issue for the short term. > > > to better address the lirc issue and to give people more > > time for testing, applying the changesets after the end of the merge window > > at > > the v4l/dvb development tree. This will help people to test, review and > > propose > > changes if needed. > > These changes are on-going for over a year now. If the lirc people > didn't hear about it so far, I doubt they will pay more attention just > because we delay the deadline by 2 months. The only thing that will get > their attention is when lirc_i2c break. So let's just do that ;) > > Don't get me wrong. I don't want to be (too) rude to lirc developers. > If they need help to port their code to the new i2c binding model, I'll > help them. If they need help to merge lirc_i2c into the kernel, I'll > help as well. But I don't see any point in delaying important, long > awaited kernel changes just for them. As long as they are out-of-tree, > they can fix things after the fact easily. They aren't affected by the > merge window. They'll have several weeks before kernel 2.6.30 is > actually released, which they can use to fix anything that broke. > I agree. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model
> drivers. *I* didn't break ir-kbd-i2c. And now because of what has effectively "broken" (viewed in just the wrong light) you are asking me to accept changes in the pvrusb2 driver to make up the difference? Normally I wouldn't object, except those proposed changes in turn will break another driver (lirc). That is why I nacked the change. I don't care if the driver is in-tree or out of tree, I am not going to accept a change that breaks without recourse a known use-case that people are using. I consider Mauro's reasoning flawed here. What I am suggesting for (1) does not "break" ir-kbd-i2c. Yes, it impacts its use but only within the pvrusb2 driver, and only until I get (2) implemented, which unless something comes up I plan on dealing with today. So (1) should be acceptable. > > > 2. Once ir-kbd-i2c has been updated, I will push another set of changes > > into the pvrusb2 driver that will make it usable there. Basically what > > I have in mind is similar to what you tried but I'm going to integrate > > it with the device profiles so that it can be appropriately loaded based > > on device model, with the correct I2C address in each case. And most > > importantly, I will add a module option to enable/disable loading or > > ir-kbd-i2c. This will fix my main objection, since then it will still > > allow lirc to be usable, for now... > > This sounds like a good idea. > > > 3. I'd like to fix the "abuse" you mention regarding I2C_HW_B_BT848. > > I'm unclear on what the cleanest solution is there, but if you like to > > (a) point at some documentation, (b) describe what I should do, or (c) > > suggest a patch, I will work to deal with the problem. > > Ideally these adapter IDs will no longer be needed within a couple > weeks, so it's probably better to leave this alone and just let it die > in silence. > OK, no problem. I'm happy with letting it die :-) -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pvrusb2: Drop client_register/unregister stubs
Acked-by: Mike Isely
On Sat, 4 Apr 2009, Jean Delvare wrote:
> The client_register and client_unregister methods are optional so
> there is no point in defining stub ones. Especially when these methods
> are likely to be removed soon.
>
> Signed-off-by: Jean Delvare
> ---
> linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c | 12
> 1 file changed, 12 deletions(-)
>
> --- v4l-dvb.orig/linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
> 2009-04-04 13:58:40.0 +0200
> +++ v4l-dvb/linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
> 2009-04-04 22:12:21.0 +0200
> @@ -595,16 +595,6 @@ static u32 pvr2_i2c_functionality(struct
> return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_I2C;
> }
>
> -static int pvr2_i2c_attach_inform(struct i2c_client *client)
> -{
> - return 0;
> -}
> -
> -static int pvr2_i2c_detach_inform(struct i2c_client *client)
> -{
> - return 0;
> -}
> -
> static struct i2c_algorithm pvr2_i2c_algo_template = {
> .master_xfer = pvr2_i2c_xfer,
> .functionality = pvr2_i2c_functionality,
> @@ -617,8 +607,6 @@ static struct i2c_adapter pvr2_i2c_adap_
> .owner = THIS_MODULE,
> .class = 0,
> .id= I2C_HW_B_BT848,
> - .client_register = pvr2_i2c_attach_inform,
> - .client_unregister = pvr2_i2c_detach_inform,
> };
>
>
>
>
--
Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model
On Sun, 5 Apr 2009, Jean Delvare wrote: > Hi Mike, > > On Sat, 4 Apr 2009 10:51:01 -0500 (CDT), Mike Isely wrote: > > > > Nacked-by: Mike Isely > > > > This will interfere with the alternative use of LIRC drivers (which work > > in more cases that ir-kbd). > > Why then is ir-kbd in the kernel tree and not LIRC drivers? > How should I know? I don't maintain either. But I know they are both used and it's not my place to force usage of one over the other. > > It will thus break some peoples' use of the driver. > > Do you think it will, or did you test and it actually does? If it > indeed breaks, please explain why, so that a solution can be found. As I interpret this, your patch will cause the pvrusb2 to probe for the IR receiver's I2C address and bind ir-kbd-i2c to what it finds. That will break anyone's usage of the driver where another IR driver (e.g. something from LIRC) is used instead. > > > Also we have better information on what i2c addresses needed to > > be probed based on the model of the device > > This is excellent news. As I said in the header comment of the patch, > avoiding probing when we know what the IR receiver is and at which > address it sits is the way to go. Please send me all the information > you have and I'll be happy to add a patch to the series, that skips > probing whenever possible. Or write that patch yourself if you prefer. I have samples of most of the devices in question and can code proper I2C addresses for each of those for each resident I2C client. The pvrusb2 driver's device attribute structure already has allowance for specification of the correct I2C addresses to use for chips specific to each device (part of the v4l2-subdev rework I recently did). Right now the driver has built in defaults, and if a particular model needs something else, it can be overridden as part of the device's profile in pvrusb2-devattr.c. > > > - and some devices supported > > by this device are not from Hauppauge so you are making a too-strong > > assumption that IR should be probed this way in all cases. > > I didn't make any assumption, sorry. I simply copied the code from > ir-kbd-i2c. If my code does the wrong thing for some devices, that was > already the case before. And this will certainly be easier to fix after > my changes than before. No, I think the point you are missing is that by moving that logic from ir-kbd-i2c into each driver (e.g. pvrusb2) you are moving logic that *might* be executed into a spot where it *will* be executed. Remember that the pvrusb2 driver did not autoload ir-kbd-i2c before this patch. Before this change, a user could elect not to use ir-i2c-kbd simply by not loading it. The pvrusb2 driver didn't request it, because the intent all along there is that the user makes the choice by loading the desired driver. Now with this change, the pvrusb2 driver is going to attempt to load ir-kbd-i2c where asked for or not. And if not, the resulting binding will prevent lirc_i2c from later on loading. If the user had been loading lirc_i2c previously, this will cause his/her usage of IR to break. No, it's not perfect, but it worked. I'm all for improving things, but not by removing an ability that people are using. > > On top of that, the "Hauppauge trick" is really only the order in which > the addresses are probed. Just because a specific order is better for > Hauppauge boards, doesn't mean it won't work for non-Hauppauge boards. "Hauppauge trick"? > > > Also, unless > > ir-kbd has suddenly improved, this will not work at all for HVR-1950 > > class devices nor MCE type PVR-24xxx devices (different incompatible IR > > receiver). > > I'm sorry but you can't blame me for ir-kbd-i2c not supporting some > devices. I updated the driver to make use of the new binding model, but > that's about all I did. Yes, but I can point out that this change now will cause ir-kbd-i2c to be loaded in cases where the user didn't want it and will risk interference with the driver that the user *did* want. > > > This is why the pvrusb2 driver has never directly attempted to load > > ir-kbd. > > The pvrusb2 driver however abuses the bttv driver's I2C adapter ID > (I2C_HW_B_BT848) and was thus affected when ir-kbd-i2c is loaded. This > is the only reason why my patch touches the pvrusb2 driver. If you tell > me you want the ir-kbd-i2c driver to leave pvrusb2 alone, I can drop > all the related changes from my patch, that's very easy. That "abuse" is a separate issue. The pvrusb2 driver started a long time ago out-of-tree and at that time it was the only way to get IR to wo
Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model
On Sat, 4 Apr 2009, Andy Walls wrote: [...] > > I have an I2C related question. If the cx18 or ivtv driver autoloads > "ir-kbd-i2c" and registers an I2C client on the bus, does that preclude > lirc_i2c, lirc_pvr150 or lirc_zilog from using the device? LIRC users > may notice, if it does. > > If that is the case, then we probably shouldn't autoload the ir-kbd > module after the CX23418 i2c adapters are initialized. > > I'm not sure what's the best solution: > > 1. A module option to the cx18 driver to tell it to call > init_cx18_i2c_ir() from cx18_probe() or not? (Easiest solution) > > 2. Some involved programmatic way for IR device modules to query bridge > drivers about what IR devices they may have, and on which I2C bus, and > at what addresses to probe, and whether a driver/module has already > claimed that device? (Gold plated solution) > > Regards, > Andy Ah, glad to see I'm not the only one concerned about this. I suppose I could instead add a module option to the pvrusb2 driver to control autoloading of ir-kbd (option 1). It also should probably be a per-device attribute, since AFAIK, ir-kbd only even works when using older Hauppauge IR receivers (i.e. lirc_i2c - cases that would otherwise use lirc_pvr150 or lirc_zilog I believe do not work with ir-kbd). Some devices handled by the pvrusb2 driver are not from Hauppauge. Too bad if this is the case, it was easier to let the user decide just by choosing which actual module to load. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] ir-kbd-i2c conversion to the new i2c binding model
Jean: I understand what you're trying to do but how is LIRC expected to still work if all drivers now force the user over to ir-kbd? -Mike On Sat, 4 Apr 2009, Jean Delvare wrote: > Hi all, > > Here finally comes my conversion of ir-kbd-i2c to the new i2c binding > model. I've split it into 6 pieces for easier review. Firstly there are > 2 preliminary patches: > > media-video-01-cx18-fix-i2c-error-handling.patch > media-video-02-ir-kbd-i2c-dont-abuse-client-name.patch > > Then 2 patches doing the actual conversion: > > media-video-03-ir-kbd-i2c-convert-to-new-style.patch > media-video-04-configure-ir-receiver.patch > > And lastly 2 patches cleaning up saa7134-input thanks to the new > possibilities offered by the conversion: > > media-video-05-saa7134-input-cleanup-msi-ir.patch > media-video-06-saa7134-input-cleanup-avermedia-cardbus.patch > > This patch set is against the v4l-dvb repository, but I didn't pay > attention to the compatibility issues. I simply build-tested it on > 2.6.27 and 2.6.29. > > This patch set touches many different drivers and I can't test any of > them. My only TV card with an IR receiver doesn't make use of > ir-kbd-i2c. So I would warmly welcome testers. The more testing my > changes can get, the better. > > And of course I welcome reviews and comments as well. I had to touch > many drivers I don't know anything about so it is possible that I > missed something. > > I'll post all 6 patches as replies to this post. They can also be > temporarily downloaded from: > http://jdelvare.pck.nerim.net/linux/ir-kbd-i2c/ > Additionally I've put a combined patch there, to make testing easier: > > http://jdelvare.pck.nerim.net/linux/ir-kbd-i2c/ir-kbd-i2c-conversion-ALL-IN-ONE.patch > But for review the individual patches are much better. > > Thanks, > -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/6] ir-kbd-i2c: Switch to the new-style device binding model
Nacked-by: Mike Isely
This will interfere with the alternative use of LIRC drivers (which work
in more cases that ir-kbd). It will thus break some peoples' use of the
driver. Also we have better information on what i2c addresses needed to
be probed based on the model of the device - and some devices supported
by this device are not from Hauppauge so you are making a too-strong
assumption that IR should be probed this way in all cases. Also, unless
ir-kbd has suddenly improved, this will not work at all for HVR-1950
class devices nor MCE type PVR-24xxx devices (different incompatible IR
receiver).
This is why the pvrusb2 driver has never directly attempted to load
ir-kbd.
-Mike
On Sat, 4 Apr 2009, Jean Delvare wrote:
> --- v4l-dvb.orig/linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
> 2009-04-04 10:53:08.0 +0200
> +++ v4l-dvb/linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c
> 2009-04-04 10:58:36.0 +0200
> @@ -649,6 +649,27 @@ static void do_i2c_scan(struct pvr2_hdw
> printk(KERN_INFO "%s: i2c scan done.\n", hdw->name);
> }
>
> +static void pvr2_i2c_register_ir(struct i2c_adapter *i2c_adap)
> +{
> + struct i2c_board_info info;
> + /* The external IR receiver is at i2c address 0x34 (0x35 for
> +reads). Future Hauppauge cards will have an internal
> +receiver at 0x30 (0x31 for reads). In theory, both can be
> +fitted, and Hauppauge suggest an external overrides an
> +internal.
> +
> +That's why we probe 0x1a (~0x34) first. CB
> + */
> + const unsigned short addr_list[] = {
> + 0x1a, 0x18, 0x4b, 0x64, 0x30,
> + I2C_CLIENT_END
> + };
> +
> + memset(&info, 0, sizeof(struct i2c_board_info));
> + strlcpy(info.type, "ir-kbd", I2C_NAME_SIZE);
> + i2c_new_probed_device(i2c_adap, &info, addr_list);
> +}
> +
> void pvr2_i2c_core_init(struct pvr2_hdw *hdw)
> {
> unsigned int idx;
> @@ -696,6 +717,9 @@ void pvr2_i2c_core_init(struct pvr2_hdw
> }
> }
> if (i2c_scan) do_i2c_scan(hdw);
> +
> + /* Instantiate the IR receiver device, if present */
> + pvr2_i2c_register_ir(&hdw->i2c_adap);
> }
>
> void pvr2_i2c_core_done(struct pvr2_hdw *hdw)
--
Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://linuxtv.org/hg/~mcisely/pvrusb2
Mauro: Please pull from http://linuxtv.org/hg/~mcisely/pvrusb2 for a few pvrusb2 changesets. All should find their way into 2.6.30; two in particular are important bug fixes. -Mike - pvrusb2: Fix incorrect reporting of default value for non-integer controls - pvrusb2: Report def_val items in sysfs symbolically, consistent with cur_val - pvrusb2: Fix uninitialized tuner_setup field(s) pvrusb2-ctrl.c | 12 +--- pvrusb2-hdw.c |1 + pvrusb2-sysfs.c | 14 -- 3 files changed, 14 insertions(+), 13 deletions(-) -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5 of 8] pvrusb2: use usb_interface.dev for v4l2_device_register
This patch will not at all impact the operation of the pvrusb2 driver,
and if associating with the USB interface's device node is preferred
then I'm fine with it.
Acked-by: Mike Isely
Mauro: Is this series going to be pulled into v4l-dvb or shall I just
bring this one specific change into my pvrusb2 repo? Is there any
reason not to pull it?
-Mike
On Sun, 29 Mar 2009, Janne Grunau wrote:
> # HG changeset patch
> # User Janne Grunau
> # Date 1238338428 -7200
> # Node ID 2d52ac089920f9ac36960c0245442fd89a06bb75
> # Parent 01af508490af3bc9c939c36001d6989e2c147aa0
> pvrusb2: use usb_interface.dev for v4l2_device_register
>
> Priority: normal
>
> Signed-off-by: Janne Grunau
>
> diff -r 01af508490af -r 2d52ac089920
> linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> --- a/linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c Sun Mar 29 16:53:48
> 2009 +0200
> +++ b/linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c Sun Mar 29 16:53:48
> 2009 +0200
> @@ -2591,7 +2591,7 @@
> hdw->ctl_read_urb = usb_alloc_urb(0,GFP_KERNEL);
> if (!hdw->ctl_read_urb) goto fail;
>
> - if (v4l2_device_register(&usb_dev->dev, &hdw->v4l2_dev) != 0) {
> + if (v4l2_device_register(&intf->dev, &hdw->v4l2_dev) != 0) {
> pvr2_trace(PVR2_TRACE_ERROR_LEGS,
> "Error registering with v4l core, giving up");
> goto fail;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2
On Thu, 26 Mar 2009, Mauro Carvalho Chehab wrote: > On Tue, 24 Mar 2009 23:07:02 -0500 (CDT) > Mike Isely wrote: > > > > > Mauro: > > > > Please pull from http://linuxtv.org/hg/~mcisely/pvrusb2 for a large > > collection of pvrusb2 changesets (see below). > > You forgot to add pvrusb2-cs53l32a.o on your Makefile. > > I'll add it and merge with the correspond patch that added this patch, to > avoid > bisect breakage. Damn. Sorry about that. Usually I *do* catch things like that :-( Thanks for spotting and fixing it. There might be another problem - I did test compile and run all this from within v4l-dvb. With that file missing it should not have loaded correctly, which means I must have missed something else too. I will double check this. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://linuxtv.org/hg/~mcisely/pvrusb2
ck.h | 102 - a/linux/drivers/media/video/pvrusb2/pvrusb2-tuner.c | 122 - a/linux/drivers/media/video/pvrusb2/pvrusb2-tuner.h | 37 b/linux/drivers/media/video/pvrusb2/pvrusb2-cs53l32a.c | 96 + b/linux/drivers/media/video/pvrusb2/pvrusb2-cs53l32a.h | 48 b/linux/drivers/media/video/pvrusb2/pvrusb2-i2c-track.c | 481 ++ b/linux/drivers/media/video/pvrusb2/pvrusb2-i2c-track.h | 97 + linux/drivers/media/video/pvrusb2/Makefile |8 linux/drivers/media/video/pvrusb2/pvrusb2-audio.c| 178 -- linux/drivers/media/video/pvrusb2/pvrusb2-audio.h|8 linux/drivers/media/video/pvrusb2/pvrusb2-cx2584x-v4l.c | 279 --- linux/drivers/media/video/pvrusb2/pvrusb2-cx2584x-v4l.h | 12 linux/drivers/media/video/pvrusb2/pvrusb2-debugifc.c |9 linux/drivers/media/video/pvrusb2/pvrusb2-debugifc.h | 12 linux/drivers/media/video/pvrusb2/pvrusb2-devattr.c | 102 - linux/drivers/media/video/pvrusb2/pvrusb2-devattr.h | 36 linux/drivers/media/video/pvrusb2/pvrusb2-dvb.c |2 linux/drivers/media/video/pvrusb2/pvrusb2-hdw-internal.h | 38 linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c | 866 --- linux/drivers/media/video/pvrusb2/pvrusb2-i2c-chips-v4l2.c |4 linux/drivers/media/video/pvrusb2/pvrusb2-i2c-cmd-v4l2.c |4 linux/drivers/media/video/pvrusb2/pvrusb2-i2c-cmd-v4l2.h |4 linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.c | 425 - linux/drivers/media/video/pvrusb2/pvrusb2-i2c-core.h | 57 linux/drivers/media/video/pvrusb2/pvrusb2-i2c-track.c| 87 - linux/drivers/media/video/pvrusb2/pvrusb2-i2c-track.h|5 linux/drivers/media/video/pvrusb2/pvrusb2-tuner.c|1 linux/drivers/media/video/pvrusb2/pvrusb2-tuner.h|2 linux/drivers/media/video/pvrusb2/pvrusb2-v4l2.c |4 linux/drivers/media/video/pvrusb2/pvrusb2-video-v4l.c| 284 --- linux/drivers/media/video/pvrusb2/pvrusb2-video-v4l.h|9 linux/drivers/media/video/pvrusb2/pvrusb2-wm8775.c | 154 - linux/drivers/media/video/pvrusb2/pvrusb2-wm8775.h | 10 37 files changed, 1749 insertions(+), 2849 deletions(-) -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb
On Sat, 14 Mar 2009, Hans Verkuil wrote: > On Saturday 14 March 2009 17:13:27 Mike Isely wrote: > > On Sat, 14 Mar 2009, Hans Verkuil wrote: > > > Hi Mauro, > > > > > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb for the > > > following: > > > > > > > > > - v4l2-device: add v4l2_device_disconnect > > > > Any chance this is going to get into 2.6.29? I need to know. > > No, this won't go to 2.6.29. None of the drivers in 2.6.29 using this > framework are USB drivers, so it's not needed there. I was going to configure the standalone version of the pvrusb2 driver to use the new framework for 2.6.29. That's why I needed to know. So I'll either configure the driver to not do this until it is built under 2.6.30 or just continue poking the structure directly for 2.6.29. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://www.linuxtv.org/hg/~hverkuil/v4l-dvb
On Sat, 14 Mar 2009, Hans Verkuil wrote: > Hi Mauro, > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb for the > following: > > - v4l2-device: add v4l2_device_disconnect Any chance this is going to get into 2.6.29? I need to know. -Mike > - v4l2: call v4l2_device_disconnect in USB drivers. > - tvaudio: add tda9875 support. > - bttv: convert to v4l2_device. > - cx88: convert to v4l2_device. > > These patches prepare the bttv and cx88 drivers for the v4l2_subdev > conversion. Note that this is not the actual conversion, it just prepares > these drivers by introducing v4l2_device. > > The v4l2_device_disconnect was added thanks to Mike Isely who pointed out > to me that the parent device can disappear after a disconnect. > > The tda9875 support in tvaudio is also needed before the bttv driver can > be converted to v4l2_subdev as it is otherwise impossible to correctly > probe for a tda9875 or tda9874. > > Mauro, I don't think I'll merge the tda7432 support into tvaudio. It is a > nice-to-have, but not a requirement since there are no probing conflicts > with that device. > > Thanks, > > Hans > > diffstat: > Documentation/video4linux/v4l2-framework.txt| 11 + > drivers/media/dvb/bt8xx/dvb-bt8xx.c |2 > drivers/media/video/bt8xx/bttv-driver.c | 47 +++-- > drivers/media/video/bt8xx/bttv-i2c.c| 10 - > drivers/media/video/bt8xx/bttv.h|3 > drivers/media/video/bt8xx/bttvp.h |5 > drivers/media/video/cx88/cx88-cards.c |8 > drivers/media/video/cx88/cx88-core.c|4 > drivers/media/video/cx88/cx88-i2c.c |8 > drivers/media/video/cx88/cx88.h |8 > drivers/media/video/tvaudio.c | 202 > +--- > drivers/media/video/usbvision/usbvision-video.c |2 > drivers/media/video/v4l2-device.c | 15 + > drivers/media/video/w9968cf.c |4 > include/media/v4l2-device.h |6 > 15 files changed, 279 insertions(+), 56 deletions(-) > > -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: POLL: for/against dropping support for kernels < 2.6.22
On Sun, 22 Feb 2009, Hans Verkuil wrote: > Hi all, > > There are lot's of discussions, but it can be hard sometimes to actually > determine someone's opinion. > > So here is a quick poll, please reply either to the list or directly to me > with your yes/no answer and (optional but welcome) a short explanation to > your standpoint. It doesn't matter if you are a user or developer, I'd like > to see your opinion regardless. > > Please DO NOT reply to the replies, I'll summarize the results in a week's > time and then we can discuss it further. > > Should we drop support for kernels <2.6.22 in our v4l-dvb repository? > > _: Yes > _: No Yes (see below) > > Optional question: > > Why: > I'm always for backwards compatibility in general. I have an out-of-tree "standalone" pvrusb2 driver which includes extra stuff that at least compiles correctly all the way back to 2.6.12 (extra - but old - i2c modules are also included with the driver for kernels of that vintage). However, that's just my one driver and I think trying to maintain that sort of (in)sanity over the entire v4l-dvb tree is going to be a major morale-sucking headache. I'm working right now on v4l2-subdev support and it's my intention that I will be ripping out all the old I2C adaptation stuff as part of this effort. (I am actually going to at least try to make the old stuff still work as a compile-time switch in the standalone pvrusb2 driver but I don't realistically expect that to remain practical with the driver as it currently resides in v4l-dvb.) So even if the decision is made to keep v4l-dvb as a whole compatible all the way back to 2.6.16, the pvrusb2 driver will still in the end have to be excluded in v4l-dvb builds for anything older than 2.6.22. I really can't vote "no" above with a straight face while doing this v4l2-subdev related work in the driver. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://linuxtv.org/hg/~mcisely/pvrusb2-pull
Mauro: Please pull from http://linuxtv.org/hg/~mcisely/pvrusb2-pull for the following: - pvrusb2: Use usb_make_path() to determine device bus location pvrusb2-hdw.c |5 + 1 file changed, 1 insertion(+), 4 deletions(-) This is the usb_make_path() change that's been talked about. Hopefully you'll see this as a real live actual pull request in spite of the subject line having been thoroughly spoiled / thrashed over the past week due to all the subsequent discussion from the last pull request :-) Note also that the pull location is slightly different than what I usually use. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2
On Tue, 20 Jan 2009, Thierry Merle wrote: > Mauro Carvalho Chehab a écrit : > > On Sat, 17 Jan 2009 19:09:51 +0100 > > Carsten Meier wrote: > > > >> Am Fri, 16 Jan 2009 02:47:50 -0200 > >> schrieb Mauro Carvalho Chehab : > >> > >>> For usb devices, usb_make_path() provide a canonical name for the > >>> device. For PCI ones, we have pci_name() for the same function. in > >>> the case of pci devices, I suspect that all use pci_name(). We just > >>> need to use usb_make_path() at the usb ones. > >>> > >> I looked at the sources for what string gets generated for bus_info by > >> usb_make_path(). If it gets used by pvrusb2, my problems are solved, > >> because it is constant across standby-wake-up-cycles. The pvrusb2's > >> implementation currently delivers "usb 7-2 address 6" here. "address > >> 6" corresponds to devnum which gets constantly increased, which results > >> in always changing strings here. Sorry for my unneccessary complaints. > > > > Mike, Thierry, Jean-Francois, Laurent and others: > > > > IMO, we should patch all usb drivers to use usb_make_path(). It will be more > > transparent to userspace, if all drivers provide the bus_info using the same > > notation. Comments? > > > In fact the usbvision code was reporting already a portable information using > dev_name. > strlcpy(vc->bus_info, dev_name(&usbvision->dev->dev), > sizeof(vc->bus_info)); > changed to: > usb_make_path(usbvision->dev, vc->bus_info, sizeof(vc->bus_info)); > shows the same information on my system. > It is simpler so I am OK with this change. > Do you want us to make separate patch or did you start a global patch? > Cheers, > Thierry I will update the pvrusb2 driver. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
Re: [linux-dvb] Cross-posting linux-media, linux-dvb etc
On Tue, 20 Jan 2009, Devin Heitmueller wrote: > I spent the morning giving some consideration to the comments people > made regarding the merging of the mailing lists. As with most > attempts at an optimization, there are cases that get more efficient > and cases that get less efficient. If done properly, the important > cases improve in efficiency while the cases that are less critical end > up a little less efficient. > > Clearly, there are two classes of users on the mailing lists: those > who read it and those who read it *and* actively contribute to it. > One of the key goals behind merging the lists was to make it more > efficient for those who have to reply to emails to not have to deal > with duplicated content, since in reality a large portion of the > emails come from people who want their device to work, and don't even > know the differences between acronyms like ATSC, QAM, DVB-T, DVB-C, > analog, etc. > > Looking at the people who have responded to this thread, and the > number of threads they have actually contributed on in the last year, > the disparity is obvious: > > People "in favor" of the lists being merged > 118 Patrick Boettcher > 205 Hans Verkuil > 38 Mike Isely I've contributed to 38 different threads in the past year? Wow, I thought I had been staying mostly in the background... > 196 Devin Heitmueller > "hundreds" Mauro Carvalho Chehab > > People "against" of the lists being merged > 2 Lars Hanisch > 17 user.vdr > 16 Klaus Schmidinger > 2 Bob Cunningham > 10 Tomas Drajsajtl > 17 Ales Jurik > > Yup, it's the developers who are posting on a regular basis who feel > the pain of the two different lists. It's the people who are actively > replying to issues, dealing with problems, and trying to keep track of > it all who want the lists merged. That said, I personally don't feel > any guilt in inconveniencing a few users who are not contributing if > it makes it easier for the people who contribute to the list on a > daily basis. > > I would love to hear more from people who have contributed to more > than 20 threads who think having the two lists are a good idea. I > doubt there will be many of them. [...] I don't have a strong preference about a -users and -dev split vs a single list. It might be worth at least trying - one can always go back to a single list if the experiment fails. Some have posted that they don't want to be bothered about all the "V4L noise" if they only care about DVB. But look at this from a driver's viewpoint. Some drivers aren't just V4L or just DVB - the pvrusb2 driver, being that it handles a few hybrid devices, plays both sides of the fence, and some issues that may arise are not clearly obvious whether V4L or DVB is the correct topic. So to which list does one expect to post? (OK, maybe in my case it's the pvrusb2 list, but the question is still valid in the general sense and is only going to get more commonplace over time.) -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2
On Sun, 18 Jan 2009, Mauro Carvalho Chehab wrote: > On Sat, 17 Jan 2009 19:09:51 +0100 > Carsten Meier wrote: > > > Am Fri, 16 Jan 2009 02:47:50 -0200 > > schrieb Mauro Carvalho Chehab : > > > > > For usb devices, usb_make_path() provide a canonical name for the > > > device. For PCI ones, we have pci_name() for the same function. in > > > the case of pci devices, I suspect that all use pci_name(). We just > > > need to use usb_make_path() at the usb ones. > > > > > > > I looked at the sources for what string gets generated for bus_info by > > usb_make_path(). If it gets used by pvrusb2, my problems are solved, > > because it is constant across standby-wake-up-cycles. The pvrusb2's > > implementation currently delivers "usb 7-2 address 6" here. "address > > 6" corresponds to devnum which gets constantly increased, which results > > in always changing strings here. Sorry for my unneccessary complaints. > > Mike, Thierry, Jean-Francois, Laurent and others: > > IMO, we should patch all usb drivers to use usb_make_path(). It will be more > transparent to userspace, if all drivers provide the bus_info using the same > notation. Comments? On the surface this sounds agreeable. As soon as I am done cleaning up after my plumbing near-disaster here, I plan on investigating and making the appropriate change(s) to the pvrusb2 driver for this. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-dvb] Cross-posting linux-media, linux-dvb etc
On Fri, 16 Jan 2009, user.vdr wrote: > I think it's a lame idea to clump all media related stuff into one > mailing list from separate ml's because 1) it's too general of a topic > and 2) those ml's already had a lot of activity on their own. The > idea of sifting through tons of posts of no interest is quite a hassle > to say the least. This "solution" just doesn't seem very well thought > out imo but it is what it is I guess. That's still better than sifting through MULTIPLE COPIES of the same post from different lists, which frequently is the case right now. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2
On Fri, 16 Jan 2009, Janne Grunau wrote:
> On Friday 16 January 2009 15:39:33 Mike Isely wrote:
> > In any case, right now the serial number in the pvrusb2 is not available
> > through that means because I haven't done anything to make it available
> > to udev. I'd like to do something, but so far I have found no
> > information on how to make that happen.
>
> You shouldn't need to do anything special. The serial number is available
> through the parent USB device. It can be used for udev rules through
> ATTRS{serial} and in sysfs entry of the video device through device/serial.
>
Ah yes!
What I said before was right in its own context, but now I see something
else.
The serial number that the pvrusb2 driver itself knows about is parsed
out of Hauppauge private data by the tveeprom module from the device's
internal I2C ROM. This data is formatted in a packetized way that is
specific to Hauppauge. What I was refering to was *that* serial number,
and since it's in the private ROM I saw no means for udev to know about
it.
However I just tested with two 24xxx devices using usbview, and the same
serial number is in fact visible in the USB configuration data.
There's simply no way for the USB core in Linux to be able to directly
know about, get at, or even understand that internal ROM. Yet there it
is. I'm thinking now that perhaps the FX2 microcontroller is either
parsing out the data itself during initialization and then writing out
the USB configuration accordingly, or the serial number is in fact
written in two places within the device! Up until now, I had not seen
any evidence to suggest that the FX2 firmware ever actually read the
nearby ROM on its own. But that could be what is happening here.
Thanks for pointing that out. These devices still surprise me.
For anyone looking at this, the serial number in the USB configuration
data for the device is just a hex-formatted version of the same value
that you can see via the pvrusb2 driver's sysfs interface.
-Mike
--
Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
Re: [linux-dvb] Cross-posting linux-media, linux-dvb etc
On Fri, 16 Jan 2009, Hans Verkuil wrote: > On Friday 16 January 2009 15:48:45 Patrick Boettcher wrote: > > Hi Mauro, > > > > Since the creation of linux-media@vger.kernel.org I'm seeing lots of > > cross-postings between linux-dvb, linux-media and video4linux. This > > is a little bit annoying if you are subscribed to all of those lists. > > > > Worse is, that some people only send requests to linux-media. Like > > that linux-dvb-only subscribers can't help... > > > > Why not closing linux-dvb (and video4linux) and transferring the > > currently subscribed users to linux-media automatically? > > I agree with Patrick. I suggest a daily automatic posting to linux-dvb > and video4linux telling people that on February 1st these lists > disappear and that they should use linux-media instead. Then they can > be closed down at the end of the month. I definitely wouldn't wait any > longer since it is rather messy right now. One month transition period > seems reasonable to me. > Amen to that. I've been telling people to go over to linux-media, but old habits are hard to break. It's time to actually make a clean break from the old lists. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2
On Thu, 15 Jan 2009, Mauro Carvalho Chehab wrote: > > OK. Well, the usage he wants is something that is better fitted by using sysfs > info. There, you should have all information to uniquely identify a device: > driver, bus location (on PCI this can be relevant), MAC (for dvb devices), > etc. > IMO, the proper way is to add the serial number at sysfs, and let the bus_info > point to the proper bus location. Having the bus location, an userspace app > can > seek the sysfs and look for the udev info. > > For example, on an em28xx analog device I have here, bus_info returns > "1-3". Ok, this is a very bad way to specify the bus. IMO, we should use > usb_make_path() to generate the canonical name for USB buses. I will review what the pvrusb2 driver is doing and change it to use usb_make_path() if needed. However given all the other information about the device that querycap returns, a serial number in that batch would be right at home there. Requiring an app to go through everything you described is a pretty complex process for what should be very simple datum. In any case, right now the serial number in the pvrusb2 is not available through that means because I haven't done anything to make it available to udev. I'd like to do something, but so far I have found no information on how to make that happen. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://linuxtv.org/hg/~mcisely/pvrusb2
Mauro: Please pull from http://linuxtv.org/hg/~mcisely/pvrusb2 for: - pvrusb2: Issue VIDIOC_INT_INIT to v4l2 modules when they first attach - pvrusb2: Code module name directly in printk pvrusb2-i2c-chips-v4l2.c | 23 +-- pvrusb2-i2c-cmd-v4l2.c | 14 ++ pvrusb2-i2c-cmd-v4l2.h |1 + pvrusb2-i2c-core.c |2 +- pvrusb2-main.c |4 ++-- 5 files changed, 31 insertions(+), 13 deletions(-) Thanks. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
The pvrusb2 stuff you just pulled
Mauro: Do you not find it strange that you show up as the credited source for my recent changesets on the summary page http://linuxtv.org/hg/v4l-dvb? (See 10236 -> 10240.) I suspect it's because you cherry picked them, but that doesn't make it right. I could have sworn in the past that I've been able to pull in changes / contributions into hg from other pvrusb2 users and successfully preserved the credit in the change list summary. What's the problem here? -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2
On Thu, 15 Jan 2009, Mauro Carvalho Chehab wrote: > On Wed, 14 Jan 2009 10:44:38 -0600 (CST) > Mike Isely wrote: [...] > > > And I'd appreciate some suggestions from anyone about a means via the > > V4L interface to make available a device-specific identifier, like a > > serial number. Yes I know such a thing is not always available with all > > devices, but it is available for Hauppauge devices in general and users > > want to have access to that information for purposes of mapping behavior > > in userspace. > > Sorry, but I didn't understand the need. What do you want to map in userspace? > Except for debugging purposes, or when the user has more than one device > connected, I can't see any other usage for such identifier. > > For debug, you can just print the serial number. For device unique > identification, there are the sysfs stuff, used by udev. > > Am I missing something? > Rather than paraphrase it all here, I invite you to look at the discussion thread on the pvrusb2 list where the topic came up: http://www.isely.net/pipermail/pvrusb2/2009-January/002091.html After looking over that discussion, let's talk about it. I think I know where you are heading with this, but I have other questions (if I'm right about where you're heading) and I'd like you to see that thread first. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2
On Wed, 14 Jan 2009, Mike Isely wrote: > On Wed, 14 Jan 2009, Mauro Carvalho Chehab wrote: > >[...] > > > > > I can see some troubles here: > > > > 1) The bus info helps to identify the place where you'll find the > > device info at sysfs; > > > > 2) This is a V4L2 API non-compliance. All drivers should be compliant > > with the API; > > > > 3) If we all agree that bus_info doesn't matter at all and decide to > > change V4L2 API, we'll still have a big trouble: most devices don't > > have a serial number. > > > > The other patches are ok. > > I was asked to make this change, because otherwise there's no means via > the V4L interface to uniquely REPEATABLY be able to identify the same > device each time it is plugged in. I have gotten complaints about this. > I actually pointed out that bus_info was about the "where" not the > "what" of the device, but I was convinced to change this - after being > surprised that the V4L spec allowed for this. Look at the online v4l > spec; the following description exists about bus_info (as part of the > VIDIOC_QUERYCAP description): > [...] Mauro: If you still don't like this specific changeset, then OK, but I'd still appreciate it if you could pull the other changes (especially the v4l2_subdev fix). And I'd appreciate some suggestions from anyone about a means via the V4L interface to make available a device-specific identifier, like a serial number. Yes I know such a thing is not always available with all devices, but it is available for Hauppauge devices in general and users want to have access to that information for purposes of mapping behavior in userspace. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PULL] http://linuxtv.org/hg/~mcisely/pvrusb2
On Wed, 14 Jan 2009, Mauro Carvalho Chehab wrote: [...] > > I can see some troubles here: > > 1) The bus info helps to identify the place where you'll find the > device info at sysfs; > > 2) This is a V4L2 API non-compliance. All drivers should be compliant > with the API; > > 3) If we all agree that bus_info doesn't matter at all and decide to > change V4L2 API, we'll still have a big trouble: most devices don't > have a serial number. > > The other patches are ok. I was asked to make this change, because otherwise there's no means via the V4L interface to uniquely REPEATABLY be able to identify the same device each time it is plugged in. I have gotten complaints about this. I actually pointed out that bus_info was about the "where" not the "what" of the device, but I was convinced to change this - after being surprised that the V4L spec allowed for this. Look at the online v4l spec; the following description exists about bus_info (as part of the VIDIOC_QUERYCAP description): "Location of the device in the system, a NUL-terminated ASCII string. For example: "PCI Slot 4". This information is intended for users, to distinguish multiple identical devices. If no such information is available the field may simply count the devices controlled by the driver, or contain the empty string (bus_info[0] = 0)." Based on that description, the actual format of the string depends a lot on the type of the device and that the only real requirement is that it distinguish among multiple devices. The changeset I posted above still fulfills those requirements. How can my change be a non-compliance given the above actual specification in the V4L2 spec? There's absolutely nothing there stating that the information must be usable to map a sysfs path. Surely the published V4L2 spec is the last word on this question... -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PULL] http://linuxtv.org/hg/~mcisely/pvrusb2
Mauro: Please pull from http://linuxtv.org/hg/~mcisely/pvrusb2 for: - pvrusb2: Stop advertising VBI capability - it isn't there - pvrusb2: Generate a device-unique identifier - pvrusb2: Change sysfs serial number handling - pvrusb2: Report device serial number in bus-info field - pvrusb2: Fix misleading comment caused by earlier commit - Fix obvious swapped names in v4l2_subdev logic pvrusb2/pvrusb2-hdw-internal.h | 12 pvrusb2/pvrusb2-hdw.c | 19 +++ pvrusb2/pvrusb2-hdw.h |6 -- pvrusb2/pvrusb2-sysfs.c| 12 ++-- pvrusb2/pvrusb2-v4l2.c |4 ++-- v4l2-subdev.c |4 ++-- 6 files changed, 41 insertions(+), 16 deletions(-) IMPORTANT NOTE: The v4l2-subdev.c is a fix for a regression that breaks most use of the pvrusb2 driver (or probably any other driver which relies on a v4l2_subdev-converted module and attempts a queryctrl or querymenu operation). The problem is generic to the v4l2_subdev support, and is a trivially obvious fix. That one should be fast-tracked into 2.6.29. -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fw: [PATCH] v4l/dvb: remove err macro from few usb devices
On Thu, 8 Jan 2009, Mauro Carvalho Chehab wrote:
> Alexey,
>
> You should get the driver maintainer's ack or at least let them know that
> you're touching on their drivers.
>
> Mike, Thierry an Dean,
>
> Could you please review this patch?
>
> Cheers,
> Mauro.
Alexey:
Once I understood why this was happening, I have no issue with it. The
pvrusb2 portion is fine.
Acked-By: Mike Isely
-Mike
>
> Forwarded message:
>
> Date: Thu, 01 Jan 2009 11:06:08 +0300
> From: Alexey Klimov
> To: Mauro Carvalho Chehab
> Cc: video4linux-l...@redhat.com, Greg KH
> Subject: [PATCH] v4l/dvb: remove err macro from few usb devices
>
>
> Hello all
> I re-send this patch. Previous time i sent i get no response.
> Please nack, apply or criticize :)
>
> --
>
> Patch removes err() macros from few usb devices.
> It places pr_err in pvrusb2-v4l2.c, dev_err in dabusb and in usbvision
> drivers. Beside placing dev_err, patch defines new s2255_dev_err macro
> with S2255_DRIVER_NAME in s2255 module.
>
> Signed-off-by: Alexey Klimov
>
> ---
> diff -r 6a189bc8f115 linux/drivers/media/video/dabusb.c
> --- a/linux/drivers/media/video/dabusb.c Wed Dec 31 15:26:57 2008 -0200
> +++ b/linux/drivers/media/video/dabusb.c Thu Jan 01 10:59:06 2009 +0300
> @@ -199,17 +199,20 @@
> dst += len;
> }
> else
> - err("dabusb_iso_complete: invalid len
> %d", len);
> + dev_err(&purb->dev->dev,
> + "dabusb_iso_complete: invalid
> len %d\n", len);
> }
> else
> dev_warn(&purb->dev->dev, "dabusb_iso_complete:
> corrupted packet status: %d\n", purb->iso_frame_desc[i].status);
> if (dst != purb->actual_length)
> - err("dst!=purb->actual_length:%d!=%d", dst,
> purb->actual_length);
> + dev_err(&purb->dev->dev,
> + "dst!=purb->actual_length:%d!=%d\n",
> + dst, purb->actual_length);
> }
>
> if (atomic_dec_and_test (&s->pending_io) && !s->remove_pending &&
> s->state != _stopped) {
> s->overruns++;
> - err("overrun (%d)", s->overruns);
> + dev_err(&purb->dev->dev, "overrun (%d)\n", s->overruns);
> }
> wake_up (&s->wait);
> }
> @@ -230,13 +233,14 @@
> while (transfer_len < (s->total_buffer_size << 10)) {
> b = kzalloc(sizeof (buff_t), GFP_KERNEL);
> if (!b) {
> - err("kzalloc(sizeof(buff_t))==NULL");
> + dev_err(&s->usbdev->dev,
> + "kzalloc(sizeof(buff_t))==NULL\n");
> goto err;
> }
> b->s = s;
> b->purb = usb_alloc_urb(packets, GFP_KERNEL);
> if (!b->purb) {
> - err("usb_alloc_urb == NULL");
> + dev_err(&s->usbdev->dev, "usb_alloc_urb == NULL\n");
> kfree (b);
> goto err;
> }
> @@ -245,7 +249,8 @@
> if (!b->purb->transfer_buffer) {
> kfree (b->purb);
> kfree (b);
> - err("kmalloc(%d)==NULL", transfer_buffer_length);
> + dev_err(&s->usbdev->dev,
> + "kmalloc(%d)==NULL\n", transfer_buffer_length);
> goto err;
> }
>
> @@ -289,10 +294,11 @@
>
> ret=usb_bulk_msg(s->usbdev, pipe, pb->data, pb->size, &actual_length,
> 100);
> if(ret<0) {
> - err("dabusb: usb_bulk_msg failed(%d)",ret);
> + dev_err(&s->usbdev->dev,
> + "usb_bulk_msg failed(%d)\n", ret);
>
> if (usb_set_interface (s->usbdev, _DABUSB_IF, 1) < 0) {
> - err("set_interface failed");
> + dev_err(&s->usbdev->dev, "set_interface failed\n");
> return -EINVAL;
> }
>
> @@ -301,7 +307,7 @@
> if( ret == -EPIPE )
Re: USB: change interface to usb_lock_device_for_reset()
On Fri, 9 Jan 2009, Mauro Carvalho Chehab wrote:
> On Thu, 8 Jan 2009 21:56:15 -0800 (PST)
> Trent Piepho wrote:
>
> > On Thu, 8 Jan 2009, Mike Isely wrote:
> > > > Yes... Anyway, this is the real patch. I've added a small comment about
> > > > this
> > > > change... I'll commit this tomorrow, if you don't have a better
> > > > suggestion.
> > >
> > > Looks good.
> >
> > Or maybe like this?
> >
> > diff -r f01b3897d141 linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> > --- a/linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c Fri Jan 09
> > 00:27:32 2009 -0200
> > +++ b/linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c Fri Jan 09
> > 02:45:48 2009 -0200
> > @@ -3747,7 +3747,12 @@
> > int ret;
> > pvr2_trace(PVR2_TRACE_INIT,"Performing a device reset...");
> > ret = usb_lock_device_for_reset(hdw->usb_dev,NULL);
> > - if (ret == 1) {
> > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 29)
> > + /* Due to the API changes, the ret value for success changed */
> > + ret = ret != 1;
> > +#endif
> > + if (ret == 0) {
> > ret = usb_reset_device(hdw->usb_dev);
> > usb_unlock_device(hdw->usb_dev);
> > } else {
> >
>
> Seems better! Could you please provide your SOB? I'll apply just the
> backport, then your patch.
Just for the record...
Acked-By: Mike Isely
-Mike
--
Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB: change interface to usb_lock_device_for_reset()
Well that simplifies the ifdef and allows text editors to still see the
braces as being balanced, at the expense of uglier code for all current
kernel versions. On the other hand this clearly documents the backwards
compatible change for all those kernels and won't affect anything going
forward.
I don't have a strong opinion about either. If I were to choose
however, I'd probably pick Trent's version.
-Mike
On Thu, 8 Jan 2009, Trent Piepho wrote:
> On Thu, 8 Jan 2009, Mike Isely wrote:
> > > Yes... Anyway, this is the real patch. I've added a small comment about
> > > this
> > > change... I'll commit this tomorrow, if you don't have a better
> > > suggestion.
> >
> > Looks good.
>
> Or maybe like this?
>
> diff -r f01b3897d141 linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> --- a/linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c Fri Jan 09 00:27:32
> 2009 -0200
> +++ b/linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c Fri Jan 09 02:45:48
> 2009 -0200
> @@ -3747,7 +3747,12 @@
> int ret;
> pvr2_trace(PVR2_TRACE_INIT,"Performing a device reset...");
> ret = usb_lock_device_for_reset(hdw->usb_dev,NULL);
> - if (ret == 1) {
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 29)
> + /* Due to the API changes, the ret value for success changed */
> + ret = ret != 1;
> +#endif
> + if (ret == 0) {
> ret = usb_reset_device(hdw->usb_dev);
> usb_unlock_device(hdw->usb_dev);
> } else {
>
> > > diff -r f01b3897d141 linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> > > --- a/linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c Fri Jan 09
> > > 00:27:32 2009 -0200
> > > +++ b/linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c Fri Jan 09
> > > 02:45:48 2009 -0200
> > > @@ -3747,7 +3747,12 @@
> > > int ret;
> > > pvr2_trace(PVR2_TRACE_INIT,"Performing a device reset...");
> > > ret = usb_lock_device_for_reset(hdw->usb_dev,NULL);
> > > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,29)
> > > + if (ret == 0) {
> > > +#else
> > > + /* Due to the API changes, the ret value for success changed */
> > > if (ret == 1) {
> > > +#endif
> > > ret = usb_reset_device(hdw->usb_dev);
> > > usb_unlock_device(hdw->usb_dev);
> > > } else {
>
--
Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB: change interface to usb_lock_device_for_reset()
On Fri, 9 Jan 2009, Mauro Carvalho Chehab wrote:
> On Thu, 8 Jan 2009 22:42:41 -0600 (CST)
> Mike Isely wrote:
>
> > On Fri, 9 Jan 2009, Mauro Carvalho Chehab wrote:
> >
> > > On Thu, 8 Jan 2009 22:28:18 -0600 (CST)
> > > Mike Isely wrote:
> > >
> > > > On Thu, 8 Jan 2009, Mike Isely wrote:
> > > >
> > > > >
> > > > >
> > > > > On Thu, 8 Jan 2009, Mauro Carvalho Chehab wrote:
> > > > >
> > > > > > Hi Mike,
> > > > > >
> > > > > > There were an upstream change usb_lock_device_for_reset() that
> > > > > > touched on
> > > > > > pvrusb2 driver. I didn't backport it yet, since I'm not sure if the
> > > > > > change is
> > > > > > ok. Could you please check?
> > > > > >
> > > > > > Thanks,
> > > > > > Mauro.
> > > > >
> > > > > Yes, the pvrusb2 part of this change looks fine (just change the
> > > > > treatment of the return code).
> > > >
> > > > Before I cause any confusion, the above sentence has a critical typo.
> > > > I was just pointing out that the pvrusb2 change in the patch below only
> > > > adjusts the treatment of the return code, which makes perfect sense
> > > > given the upstream change. I wasn't asking you to change anything :-)
> > > > The change is fine.
> > >
> > > Ok. The backport of this patch would be something interesting...
> > >
> > > It will be something like this:
> > >
> > > #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,29)
> > > if (ret == 0)
> > > #else
> > > if (ret == 1)
> > > #endif
> > >
> >
> > Yeah, I know. It's part of the fun of staying in sync with multiple
> > kernels and their ever-changing internal interfaces. I have to support
> > this in the standalone pvrusb2 driver too. One more task this
> > weekend...
>
> Yes... Anyway, this is the real patch. I've added a small comment about this
> change... I'll commit this tomorrow, if you don't have a better suggestion.
Looks good.
Acked-By: Mike Isely
-Mike
>
> Now, I need to rest for a while. it is almost 3am here...
>
> Cheers,
> Mauro.
>
> diff -r f01b3897d141 linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c
> --- a/linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c Fri Jan 09 00:27:32
> 2009 -0200
> +++ b/linux/drivers/media/video/pvrusb2/pvrusb2-hdw.c Fri Jan 09 02:45:48
> 2009 -0200
> @@ -3747,7 +3747,12 @@
> int ret;
> pvr2_trace(PVR2_TRACE_INIT,"Performing a device reset...");
> ret = usb_lock_device_for_reset(hdw->usb_dev,NULL);
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,29)
> + if (ret == 0) {
> +#else
> + /* Due to the API changes, the ret value for success changed */
> if (ret == 1) {
> +#endif
> ret = usb_reset_device(hdw->usb_dev);
> usb_unlock_device(hdw->usb_dev);
> } else {
>
> Cheers,
> Mauro
>
--
Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB: change interface to usb_lock_device_for_reset()
On Fri, 9 Jan 2009, Mauro Carvalho Chehab wrote: > On Thu, 8 Jan 2009 22:28:18 -0600 (CST) > Mike Isely wrote: > > > On Thu, 8 Jan 2009, Mike Isely wrote: > > > > > > > > > > > On Thu, 8 Jan 2009, Mauro Carvalho Chehab wrote: > > > > > > > Hi Mike, > > > > > > > > There were an upstream change usb_lock_device_for_reset() that touched > > > > on > > > > pvrusb2 driver. I didn't backport it yet, since I'm not sure if the > > > > change is > > > > ok. Could you please check? > > > > > > > > Thanks, > > > > Mauro. > > > > > > Yes, the pvrusb2 part of this change looks fine (just change the > > > treatment of the return code). > > > > Before I cause any confusion, the above sentence has a critical typo. > > I was just pointing out that the pvrusb2 change in the patch below only > > adjusts the treatment of the return code, which makes perfect sense > > given the upstream change. I wasn't asking you to change anything :-) > > The change is fine. > > Ok. The backport of this patch would be something interesting... > > It will be something like this: > > #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,29) > if (ret == 0) > #else > if (ret == 1) > #endif > Yeah, I know. It's part of the fun of staying in sync with multiple kernels and their ever-changing internal interfaces. I have to support this in the standalone pvrusb2 driver too. One more task this weekend... -Mike -- Mike Isely isely @ pobox (dot) com PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB: change interface to usb_lock_device_for_reset()
On Thu, 8 Jan 2009, Mike Isely wrote:
>
>
> On Thu, 8 Jan 2009, Mauro Carvalho Chehab wrote:
>
> > Hi Mike,
> >
> > There were an upstream change usb_lock_device_for_reset() that touched on
> > pvrusb2 driver. I didn't backport it yet, since I'm not sure if the change
> > is
> > ok. Could you please check?
> >
> > Thanks,
> > Mauro.
>
> Yes, the pvrusb2 part of this change looks fine (just change the
> treatment of the return code).
Before I cause any confusion, the above sentence has a critical typo.
I was just pointing out that the pvrusb2 change in the patch below only
adjusts the treatment of the return code, which makes perfect sense
given the upstream change. I wasn't asking you to change anything :-)
The change is fine.
-Mike
>
> Acked-By: Mike Isely
>
> I expect this weekend to be working through a backlog of pvrusb2 issues
> so you might hear more from me soon :-)
>
> -Mike
>
>
> >
> > commit 011b15df465745474e3ec85482633685933ed5a7
> > Author: Alan Stern
> > Date: Tue Nov 4 11:29:27 2008 -0500
> >
> > USB: change interface to usb_lock_device_for_reset()
> >
> > This patch (as1161) changes the interface to
> > usb_lock_device_for_reset(). The existing interface is apparently not
> > very clear, judging from the fact that several of its callers don't
> > use it correctly. The new interface always returns 0 for success and
> > it always requires the caller to unlock the device afterward.
> >
> > The new routine will not return immediately if it is called while the
> > driver's probe method is running. Instead it will wait until the
> > probe is over and the device has been unlocked. This shouldn't cause
> > any problems; I don't know of any cases where drivers call
> > usb_lock_device_for_reset() during probe.
> >
> > Signed-off-by: Alan Stern
> > Cc: Pete Zaitcev
> > Signed-off-by: Greg Kroah-Hartman
> >
> > diff --git a/drivers/block/ub.c b/drivers/block/ub.c
> > index 048d71d..12fb816 100644
> > --- a/drivers/block/ub.c
> > +++ b/drivers/block/ub.c
> > @@ -1579,7 +1579,7 @@ static void ub_reset_task(struct work_struct *work)
> > struct ub_dev *sc = container_of(work, struct ub_dev, reset_work);
> > unsigned long flags;
> > struct ub_lun *lun;
> > - int lkr, rc;
> > + int rc;
> >
> > if (!sc->reset) {
> > printk(KERN_WARNING "%s: Running reset unrequested\n",
> > @@ -1597,10 +1597,11 @@ static void ub_reset_task(struct work_struct *work)
> > } else if (sc->dev->actconfig->desc.bNumInterfaces != 1) {
> > ;
> > } else {
> > - if ((lkr = usb_lock_device_for_reset(sc->dev, sc->intf)) < 0) {
> > + rc = usb_lock_device_for_reset(sc->dev, sc->intf);
> > + if (rc < 0) {
> > printk(KERN_NOTICE
> > "%s: usb_lock_device_for_reset failed (%d)\n",
> > - sc->name, lkr);
> > + sc->name, rc);
> > } else {
> > rc = usb_reset_device(sc->dev);
> > if (rc < 0) {
> > @@ -1608,9 +1609,7 @@ static void ub_reset_task(struct work_struct *work)
> > "usb_lock_device_for_reset failed (%d)\n",
> > sc->name, rc);
> > }
> > -
> > - if (lkr)
> > - usb_unlock_device(sc->dev);
> > + usb_unlock_device(sc->dev);
> > }
> > }
> >
> > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> > index 03cb494..f0a0f72 100644
> > --- a/drivers/hid/usbhid/hid-core.c
> > +++ b/drivers/hid/usbhid/hid-core.c
> > @@ -102,7 +102,7 @@ static void hid_reset(struct work_struct *work)
> > struct usbhid_device *usbhid =
> > container_of(work, struct usbhid_device, reset_work);
> > struct hid_device *hid = usbhid->hid;
> > - int rc_lock, rc = 0;
> > + int rc = 0;
> >
> > if (test_bit(HID_CLEAR_HALT, &usbhid->iofl)) {
> > dev_dbg(&usbhid->intf->dev, "clear halt\n");
> > @@ -113,11 +113,10 @@ static void hid_reset(struct work_struct *work)
> >
> > else if (test_bit(HID_RESET_PENDING, &usbhi
Re: USB: change interface to usb_lock_device_for_reset()
On Thu, 8 Jan 2009, Mauro Carvalho Chehab wrote:
> Hi Mike,
>
> There were an upstream change usb_lock_device_for_reset() that touched on
> pvrusb2 driver. I didn't backport it yet, since I'm not sure if the change is
> ok. Could you please check?
>
> Thanks,
> Mauro.
Yes, the pvrusb2 part of this change looks fine (just change the
treatment of the return code).
Acked-By: Mike Isely
I expect this weekend to be working through a backlog of pvrusb2 issues
so you might hear more from me soon :-)
-Mike
>
> commit 011b15df465745474e3ec85482633685933ed5a7
> Author: Alan Stern
> Date: Tue Nov 4 11:29:27 2008 -0500
>
> USB: change interface to usb_lock_device_for_reset()
>
> This patch (as1161) changes the interface to
> usb_lock_device_for_reset(). The existing interface is apparently not
> very clear, judging from the fact that several of its callers don't
> use it correctly. The new interface always returns 0 for success and
> it always requires the caller to unlock the device afterward.
>
> The new routine will not return immediately if it is called while the
> driver's probe method is running. Instead it will wait until the
> probe is over and the device has been unlocked. This shouldn't cause
> any problems; I don't know of any cases where drivers call
> usb_lock_device_for_reset() during probe.
>
> Signed-off-by: Alan Stern
> Cc: Pete Zaitcev
> Signed-off-by: Greg Kroah-Hartman
>
> diff --git a/drivers/block/ub.c b/drivers/block/ub.c
> index 048d71d..12fb816 100644
> --- a/drivers/block/ub.c
> +++ b/drivers/block/ub.c
> @@ -1579,7 +1579,7 @@ static void ub_reset_task(struct work_struct *work)
> struct ub_dev *sc = container_of(work, struct ub_dev, reset_work);
> unsigned long flags;
> struct ub_lun *lun;
> - int lkr, rc;
> + int rc;
>
> if (!sc->reset) {
> printk(KERN_WARNING "%s: Running reset unrequested\n",
> @@ -1597,10 +1597,11 @@ static void ub_reset_task(struct work_struct *work)
> } else if (sc->dev->actconfig->desc.bNumInterfaces != 1) {
> ;
> } else {
> - if ((lkr = usb_lock_device_for_reset(sc->dev, sc->intf)) < 0) {
> + rc = usb_lock_device_for_reset(sc->dev, sc->intf);
> + if (rc < 0) {
> printk(KERN_NOTICE
> "%s: usb_lock_device_for_reset failed (%d)\n",
> - sc->name, lkr);
> + sc->name, rc);
> } else {
> rc = usb_reset_device(sc->dev);
> if (rc < 0) {
> @@ -1608,9 +1609,7 @@ static void ub_reset_task(struct work_struct *work)
> "usb_lock_device_for_reset failed (%d)\n",
> sc->name, rc);
> }
> -
> - if (lkr)
> - usb_unlock_device(sc->dev);
> + usb_unlock_device(sc->dev);
> }
> }
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 03cb494..f0a0f72 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -102,7 +102,7 @@ static void hid_reset(struct work_struct *work)
> struct usbhid_device *usbhid =
> container_of(work, struct usbhid_device, reset_work);
> struct hid_device *hid = usbhid->hid;
> - int rc_lock, rc = 0;
> + int rc = 0;
>
> if (test_bit(HID_CLEAR_HALT, &usbhid->iofl)) {
> dev_dbg(&usbhid->intf->dev, "clear halt\n");
> @@ -113,11 +113,10 @@ static void hid_reset(struct work_struct *work)
>
> else if (test_bit(HID_RESET_PENDING, &usbhid->iofl)) {
> dev_dbg(&usbhid->intf->dev, "resetting device\n");
> - rc = rc_lock = usb_lock_device_for_reset(hid_to_usb_dev(hid),
> usbhid->intf);
> - if (rc_lock >= 0) {
> + rc = usb_lock_device_for_reset(hid_to_usb_dev(hid),
> usbhid->intf);
> + if (rc == 0) {
> rc = usb_reset_device(hid_to_usb_dev(hid));
> - if (rc_lock)
> - usb_unlock_device(hid_to_usb_dev(hid));
> + usb_unlock_device(hid_to_usb_dev(hid));
> }
> clear_bit(HID_RESET_PENDING, &usbhid->iofl);
> }
> diff --git a/drivers/media/video/pvrusb2/pvrusb2-hdw.c
&g
Re: Fw: [PATCH] v4l/dvb: remove err macro from few usb devices
t; }
> usbvision->curwidth = usbvision->stretch_width * UsbWidth;
> @@ -2245,7 +2251,7 @@
>(__u16) USBVISION_DRM_PRM1, value, 8, HZ);
>
> if (rc < 0) {
> - err("%sERROR=%d", __func__, rc);
> + dev_err(&usbvision->dev->dev, "%sERROR=%d\n", __func__, rc);
> return rc;
> }
>
> @@ -2453,8 +2459,9 @@
> PDEBUG(DBG_FUNC,"setting alternate %d with wMaxPacketSize=%u",
> dev->ifaceAlt,dev->isocPacketSize);
> errCode = usb_set_interface(dev->dev, dev->iface,
> dev->ifaceAlt);
> if (errCode < 0) {
> - err ("cannot change alternate number to %d (error=%i)",
> - dev->ifaceAlt, errCode);
> + dev_err(&dev->dev->dev,
> + "cannot change alternate number to %d
> (error=%i)\n",
> + dev->ifaceAlt, errCode);
> return errCode;
> }
> }
> @@ -2505,7 +2512,8 @@
>
> urb = usb_alloc_urb(USBVISION_URB_FRAMES, GFP_KERNEL);
> if (urb == NULL) {
> - err("%s: usb_alloc_urb() failed", __func__);
> + dev_err(&usbvision->dev->dev,
> + "%s: usb_alloc_urb() failed\n", __func__);
> return -ENOMEM;
> }
> usbvision->sbuf[bufIdx].urb = urb;
> @@ -2537,8 +2545,9 @@
> errCode = usb_submit_urb(usbvision->sbuf[bufIdx].urb,
>GFP_KERNEL);
> if (errCode) {
> - err("%s: usb_submit_urb(%d) failed: error %d",
> - __func__, bufIdx, errCode);
> + dev_err(&usbvision->dev->dev,
> + "%s: usb_submit_urb(%d) failed: error %d\n",
> + __func__, bufIdx, errCode);
> }
> }
>
> @@ -2587,8 +2596,9 @@
> errCode = usb_set_interface(usbvision->dev, usbvision->iface,
> usbvision->ifaceAlt);
> if (errCode < 0) {
> - err("%s: usb_set_interface() failed: error %d",
> - __func__, errCode);
> + dev_err(&usbvision->dev->dev,
> + "%s: usb_set_interface() failed: error %d\n",
> + __func__, errCode);
> usbvision->last_error = errCode;
> }
> regValue = (16-usbvision_read_reg(usbvision,
> USBVISION_ALTER_REG)) & 0x0F;
> diff -r 6a189bc8f115 linux/drivers/media/video/usbvision/usbvision-i2c.c
> --- a/linux/drivers/media/video/usbvision/usbvision-i2c.c Wed Dec 31
> 15:26:57 2008 -0200
> +++ b/linux/drivers/media/video/usbvision/usbvision-i2c.c Thu Jan 01
> 10:59:06 2009 +0300
> @@ -120,7 +120,8 @@
> /* try extended address code... */
> ret = try_write_address(i2c_adap, addr, retries);
> if (ret != 1) {
> - err("died at extended address code, while writing");
> + dev_err(&i2c_adap->dev,
> + "died at extended address code, while
> writing\n");
> return -EREMOTEIO;
> }
> add[0] = addr;
> @@ -129,7 +130,8 @@
> addr |= 0x01;
> ret = try_read_address(i2c_adap, addr, retries);
> if (ret != 1) {
> - err("died at extended address code, while
> reading");
> + dev_err(&i2c_adap->dev,
> + "died at extended address code, while
> reading\n");
> return -EREMOTEIO;
> }
> }
> diff -r 6a189bc8f115 linux/drivers/media/video/usbvision/usbvision-video.c
> --- a/linux/drivers/media/video/usbvision/usbvision-video.c Wed Dec 31
> 15:26:57 2008 -0200
> +++ b/linux/drivers/media/video/usbvision/usbvision-video.c Thu Jan 01
> 10:59:06 2009 +0300
> @@ -329,7 +329,7 @@
> return;
> } while (0);
>
> - err("%s error: %d\n", __func__, res);
> + dev_err(&vdev->dev, "%
