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