Am 05.01.2013 16:35, schrieb Mauro Carvalho Chehab: > Em Sat, 05 Jan 2013 14:42:10 +0100 > Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> escreveu: > >> Am 05.01.2013 14:22, schrieb Frank Schäfer: >>> Am 04.01.2013 22:15, schrieb Mauro Carvalho Chehab: >>>> Frank pointed that IR was not working with I2C devices. So, I took some >>>> time to fix them. >>>> >>>> Tested with Hauppauge WinTV USB2. >>>> >>>> Mauro Carvalho Chehab (4): >>>> [media] em28xx: initialize button/I2C IR earlier >>>> [media] em28xx: autoload em28xx-rc if the device has an I2C IR >>>> [media] em28xx: simplify IR names on I2C devices >>>> [media] em28xx: tell ir-kbd-i2c that WinTV uses an RC5 protocol >>>> >>>> drivers/media/usb/em28xx/em28xx-cards.c | 2 +- >>>> drivers/media/usb/em28xx/em28xx-input.c | 29 ++++++++++++++++------------- >>>> 2 files changed, 17 insertions(+), 14 deletions(-) >>>> >>> While these patches make I2C IR remote controls working again, they >>> leave several issues unaddressed which should really be fixed: >>> 1) the i2c client isn't unregistered on module unload. This was the >>> reason for patch 2 in my series. There is also a FIXME comment about >>> this in em28xx_release_resources() (although this is the wrong place to >>> do it). >>> 2) there is no error checking in em28xx_register_i2c_ir(). >>> em28xx_ir_init should really bail out if no i2c device is found. >>> 3) All RC maps should be assigned at the same place, no matter if the >>> receiver/demodulator is built in or external. Spreading them over the >>> code is inconsistent and makes the code bug prone. >>> 4) the list of known i2c devices in em28xx-i2c.c misses client address >>> 0x3e >> 1 = 0x1f. See client list in em28xx_register_i2c_ir(). >>> 5) there should be a warning message for the case that we call >>> ir-kbd-i2c with an unknown rc device. >>> 6) because we use our own key polling functions with ir-kbd-i2c, we >>> should also select the polling interval value manually. That makes >>> things consistent and avoids confusion. >>> >>> The rest is a matter of taste / prefered code layout. I'm fine with it. >>> >>> Regards, >>> Frank >> It seems like already applied them... :( >> >> While I certainly appreciate patches beeing applied as soon as possible, >> I think there should really be a chance to review them before this happens. >> Especially when the changes are non-trivial and someone else has posted >> patches addressing the same issues before (other contributers might feel >> offended ;) ). > All the 4 applied patches are really trivial: > - patch 1: just reorder existing code; > - patch 2: one-line patch adding another condition to an existing if; > - patch 3: pure string rename; > - patch 4: one line patch properly reporting the RC5 protocol on WinTV. Just because a patch "just reorders existing code" or "just changes a single line" it's not automatically trivial. I'm sure you have seen more cases than me in which it were patches like this who caused big trouble. ;) And especially in cases where the changes are under discussion (which I would say is the case when someone else has posted patches addressing the same issues before) there should be a minimum chance to react on them. Isn't that what you would expect from others, too ? ;) Apart from that, there are also lots of other 'trivial' patches rotting at patchwork or bugzilla... > Also, my time is very limited, especially when I need to test a driver, as > I need to allocate a bigger time window. On such cases, I just reorder the > patches to to apply all of them at the same time, to optimize my time. Yeah, I understand your time problems and I really appreciate patches beeing applied as soon as possible (after they have been reviewed). But delaying a patch for a few days really shouldn't cause too much extra work. > Also, both Devin and you are working right now at the same driver, and you > both have pending work. Merging the patches quicker helps to avoid merge > conflicts. 100% agreement, although I don't think these patches are causing any problems here. Regards, Frank > Regards, > Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
