Em Sat, 05 Jan 2013 13:58:20 +0100 Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> escreveu: > Am 04.01.2013 22:12, schrieb Mauro Carvalho Chehab: > > Em Fri, 28 Dec 2012 00:02:45 +0100 > > Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> escreveu: > > > >> Tested with device "Terratec Cinergy 200 USB". > > Sorry, but this patch is completely wrong ;) > > Completely wrong ? That's not helpful... > Please elaborate a bit more on this so that I can do things right next > time. ;) Sorry, I was too busy yesterday with the tests to elaborate it more. In general, big patches like that to fix bug fixes are generally wrong: they touch on a lot of the code and it is hard to be sure that it doesn't come with regressions on it. In this particular case, it was: - mixing bug fixes with some other random stuff; - moving only one part of the IR needed data elsewhere (it were moving the IR tables, to the board descriptions, keeping them on a separate part of the code, obfuscating the code); - putting a large amount of the code inside an if, increasing the driver's complexity with no need; - initializing some data for IR that are never used, at em28xx_ir_init; - not fixing the snapshot button. The bug fix was as simple as: 1) move snapshot button register to happen before IR; 2) move I2C init to happen before the em2860/2874 IR init. See: http://git.linuxtv.org/media_tree.git/commitdiff/8303dc9952758ab3060a3ee9a19ecb6fec83c600 That's simple to review, and the produced patch can be accepted later at stable, because it is not a code rewrite. Of course, if we backport this to -stable, this one is also recommended: http://git.linuxtv.org/media_tree.git/commitdiff/728f9778e273a11a65926ac21574e6ca8d911ebf in order to autoload the RC extension automatically for I2C IR's, but this is a separate bug. Btw, I really prefer to have the RC tables for the I2C devices inside em28xx-input, as: 1) there are other board-specific platform_data that needed to be filled for the IR to work there; 2) we want to keep all those platform_data initialization together, to make the code simpler to maintain; 3) moving all those data to em28xx cards struct is a bad idea, as newer em28xx won't use I2C IR's, as the new chipsets have already its own IR decoder. Moving those 4-5 fields to the board struct would increase its size for every board. So, it would be a waste of space. 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
