On Thu, 11 Nov 2010 11:54:30 -0200, Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxx> wrote: > The bad news is that ir-kbd-i2c also needs the stuff that are inside > ir.props (e. g., the IR configuration logic). I wrote and just sent 2 > patches to the ML with the fix patches, against my media-tree.git, > branch staging/for_v2.6.38. For now, only one field > of props is used, but other fields there are likely needed for the other > places where this driver is used, like the open/close callbacks, > allowed_protocols, etc. > > I don't like the idea of just copying all those config stuff into struct > IR_i2c, and then at struct rc_dev, Which is the way it is with or without my patch, either a struct ir_dev_props or a subset of the former members of that struct are in IR_i2c, same data. And right now you would need to have a mix between ir_dev_props data and additional ir_dev related data in struct IR_i2c (the rc map name is outside of ir_dev_props). Note that the patch *removes* > 200 lines of code (without any loss of functionality) - and that's because the interface is simplified. Simple is good. > and then at struct input_dev. struct input_dev only gets input_name, input_phys and input_id from struct rc_dev, and I did it that way because I didn't want to remove all that information from all drivers (and fill input_dev with generic information instead). We'll have to readdress that issue once multi-input-devs-per-rc-dev functionality is implemented. > It is too much data duplication for no good reason. And you believe it's an important feature that props used to be a struct and that you could pass a pointer (and take care of initializing rc_map) instead of initializing a couple of members in rc_dev directly? The use of struct ir_dev_props was a truely bizarre interface. For example: setting the props member was optional, and a ir_input_dev struct without a props member lacks a driver_type submember. So all codepaths need to know what the default driver_type is when props is not set. Not to mention the number of oops reports that linux-media has already seen, caused by code which didn't check ->props before dereferencing it. That's of course a bug in code, but it's a bug caused by a non-intuitive interface. The new struct is much more straightforward, and your worries about additional pain caused by not having a struct ir_dev_props did not materialize in any of the changes I did (see for example drivers/media/dvb/dvb-usb/dib0700_devices.c which had similar requirements to struct IR_i2c). > So, I think we should re-think about your patch 6/7. What's your suggestion? > Comments? Unsurprisingly, I disagree on the whole re-think part :) -- David HÃrdeman -- 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
