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.

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.

> 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.

But honestly that doesn't matter much, I think, because the idea is to
merge my changes quickly.

> 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 :-)

I am totally fine removing the pvrusb2 parts from my patch set, and
letting you deal with it. As you know, my main concern here is to get
rid of the legacy i2c model. So I am virtually happy with any solution
that leads there, and I am gladly accepting all the help I can get to
go there.

> 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.

That's alright. In this specific case I really don't care who gets the
fame and flames, as longs as things get done quickly. Thank you for
jumping in and helping me sorting it all out, this is very appreciated.

-- 
Jean Delvare
--
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

Reply via email to