Em 11-11-2010 21:40, Jarod Wilson escreveu: > On Thu, Nov 11, 2010 at 3:35 PM, David Härdeman <david@xxxxxxxxxxx> wrote: >> On Thu, Nov 11, 2010 at 02:00:38PM -0200, Mauro Carvalho Chehab wrote: > ... >>>> 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). >>> >>> input_dev fields need to be properly filled, but just duplicating the same >>> info on both structs and copying from one to the other doesn't seem the better >>> way. Why not just initialize rc_dev->input_dev fields directly, not duplicating >>> the data (or having a helper in-lined routine) to initialize those fields? >> >> The data is not duplicated, input_name and input_phys are passed around >> as pointers. >> >> And the reason it looks the way it does is, again, that >> multiple-input-devices-per-rc-device will be broken if drivers fiddle >> with the input_dev directly. Not to mention it's a layering violation to >> expect rc drivers to also know about the underlying input devices. >> >> (Yes, you could say that input_name, input_phys and input_id are >> layering violations as well, but they are not an equally problematic >> violation and they're a stopgap measure). >> >>> Ok, but the point is that a driver like ir-kbd-i2c (and other I2C drivers like >>> lirc-zilog - after ported to rc-core) will require several additional fields >>> that are added at rc_dev (basically, all fields that are, currently, at >>> ir_dev_props structs may be needed by an i2c driver). >> >> I don't think it's a problem. Making rc-core ir-kbd-i2c friendly is >> putting the cart before the horse, ir-kbd-i2c is but one user of rc-core >> (and I also doubt that you'll actually need to duplicate all members, >> i2c hardware is too basic to need all bells and whistles of rc-core). > > And just for the record, lirc_zilog probably needs a fairly massive > overhaul, so I'd definitely not worry about it *too* much... > >>>> 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). >>> >>> dvb-usb uses a large struct to device dvb devices, and, due to the >>> way it were done, every single field at RC should be inititialized, >>> per device. I don't like the way it is, but I didn't want to delay >>> the rc_core port on it, due to some discussions about how to re-structure >>> it to avoid the large amount of data duplication there. So, I just >>> added the absolute minimum fields there. IMO, we should do later >>> a large cleanup on it. Yet, it is different than ir-kdb-i2c, since >>> since the beginning, the complete IR code is exported on DVB drivers, >>> while V4L drivers use to export just a few bits of the IR code (up >>> to seven bits). So, it is not a good example. >>> >>> A good exercise would be to port lirc-zilog and see what happens. >> >> I had a quick look at lirc-zilog and I doubt it would be a good >> candidate to integrate with ir-kbd-i2c.c (I assume that's what you were >> implying?). Which code from ir-kbd-i2c would it actually be using? > > On the receive side, lirc_zilog was pretty similar to lirc_i2c, which > we dropped entirely, as ir-kbd-i2c handles receive just fine for all > the relevant rx-only devices lirc_i2c worked with. So in theory, > ir-kbd-i2c might want to just grow tx support, but I think I'm more > inclined to make it a new stand-alone rx and tx capable driver. It doesn't matter much if we'll grow ir-kbd-i2c or convert lirc_zilog. The point is that rc_register_device() should be called inside the i2c driver, but several parameters should be passed to it via platform_data, in a way that is similar to ir-kbd-i2c. Maybe one solution would be to pass rc_dev via platform_data. >>>> What's your suggestion? >>> >>> One idea could be to initialize rc_dev at the caller drivers, passing >>> it via platform_data for the I2C drivers. >> >> Having a subsystem mucking around in a struct embedded as part of the >> platform data of a higher level driver sounds iffy. You'll never (for >> example) be able to const'ify platform_data... >> >>> Also, instead of duplicating input_dev fields, directly initialize them >>> inside the drivers, and not at rc-core. >> >> Won't work for the reasons explained above. >> >>> I like the idea of having an inlined function (like >>> usb_fill_control_urb), to be sure that all mandatory fields are >>> initialized by the drivers. >> >> I like the idea of having a function, let's call it >> rc_register_device(), which makes sure that all mandatory fields are >> initialized by the drivers :) > > rc_register_device(rc, name, phys, id); to further prevent duplicate > struct members? :) Seems a good idea to me. It is easier and more direct to pass those info as parameter, than to have some code inside rc_register_device to check for the mandatory data. > I still really like this interface change, even if its going to cause > short-term issues for i2c devices. I think we just extend this as > needed to handle the i2c bits. That said, I haven't really looked all > that closely at how much that entails... > I think I'll apply the cx231xx fixes and then rebase the rc_register_device patch on the top of it, doing a minimal change at IR_i2c. Currently, we just need to pass one extra parameter. After this, we can work to improve it. Cheers, 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
