Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
On Thu, Nov 11, 2010 at 06:40:42PM -0500, Jarod Wilson wrote:
On Thu, Nov 11, 2010 at 3:35 PM, David Härdeman da...@hardeman.nu wrote:
On Thu, Nov 11, 2010 at 02:00:38PM -0200, Mauro Carvalho Chehab wrote:
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? :)
As I said before, that won't work with multiple input devices per rc
dev. And it's a poorly designed API (IMHO) which expects you to set a
few properties in a struct and then add a few more via a function call.
Notes wrt. a future multi-input support:
First, realize that the name/phys/id of an input device is primarily
used to distinguish them from each other in user-space. Which means
having a shared name/phys/id triplet for all input devices belonging to
one rc device only makes them pointless.
So, I think we'll want something similar to name/phys/id, but for the rc
device (can be exported via sysfs). Input name/phys/id can then be
derived from the rc device for each input subdev (or name could perhaps
be set to some user-friendly description of the actual remote control by
whichever user-space tool loads the corresponding keymap).
Example:
rc_dev-name = FooBar IR-Masta 2000
rc_dev-phys = PNP0BAR/rc0
(would be available from /sys/class/rc/rc0/{name|phys})
/* Not set by the driver, available with lsinput */
rc_dev-input_devs[0]-name = FooBar IR-Masta 2000 Remote 1
rc_dev-input_devs[0]-phys = PNP0BAR/rc0/input0
rc_dev-input_devs[0]-id.bustype = BUS_RC;
rc_dev-input_devs[1]-name = FooBar IR-Masta 2000 Remote 2
rc_dev-input_devs[1]-phys = PNP0BAR/rc0/input1
rc_dev-input_devs[1]-id.bustype = BUS_RC;
(Just an example, don't overanalyse the details)
As you see, the rc_dev-input_name/phys/id is just a stopgap measure for
now, and I certainly don't think future development will be helped by
moving any input related fields up to the rc_register_device() level
when they should instead go further down.
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.
Agreed.
--
David Härdeman
--
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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
On Fri, Nov 12, 2010 at 02:00:55AM -0200, Mauro Carvalho Chehab wrote: Em 11-11-2010 21:40, Jarod Wilson escreveu: On Thu, Nov 11, 2010 at 3:35 PM, David Härdeman da...@hardeman.nu wrote: On Thu, Nov 11, 2010 at 02:00:38PM -0200, Mauro Carvalho Chehab wrote: 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. Yes, but if lirc_zilog doesn't use ir-kbd-i2c, there might not be a need for the large number of rc-specific members in platform_data? Maybe one solution would be to pass rc_dev via platform_data. Shouldn't platform_data be const? And you'll break the refcounting done in rc_allocate_device() and rc_free_device() / rc_unregister_device(). Not to mention the silent bugs that may be introduced if anyone modifies rc_allocate_device() without noticing that one driver isn't using it. 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. See my reply to Jarod. And also, rc_register_device() is anyway going to check other mandatory fields so having it check all of them in one go is just good consistency IMHO. 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. Meaning that you'll my patch with the rc_dev API the way it is basically and then we can revisit the IR_i2c debate later if necessary? If that's what you mean I'm all for it. -- David Härdeman -- 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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
Em 12-11-2010 10:12, David Härdeman escreveu: On Fri, Nov 12, 2010 at 02:00:55AM -0200, Mauro Carvalho Chehab wrote: Em 11-11-2010 21:40, Jarod Wilson escreveu: On Thu, Nov 11, 2010 at 3:35 PM, David Härdeman da...@hardeman.nu wrote: On Thu, Nov 11, 2010 at 02:00:38PM -0200, Mauro Carvalho Chehab wrote: 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. Yes, but if lirc_zilog doesn't use ir-kbd-i2c, there might not be a need for the large number of rc-specific members in platform_data? This device is more complex than the current I2C devices, so, this will basically depend on the diversity of devices that are/will be supported. Yet, things like open/close callbacks are interesting, as they allow to turn off RC support when a table is empty, turning it on when the table is filled and some userspace app opens the input device. Maybe one solution would be to pass rc_dev via platform_data. Shouldn't platform_data be const? And you'll break the refcounting done in rc_allocate_device() and rc_free_device() / rc_unregister_device(). Not to mention the silent bugs that may be introduced if anyone modifies rc_allocate_device() without noticing that one driver isn't using it. It will still be const. platform_data will pass a pointer to some struct. The value of the pointer won't change. I don't see why this would break refcounting, as what will happen is that the caller driver will call rc_allocate_device() and fill some fields there, instead of ir_kbd_i2c. I'm working on a patch for it right now. Basically, drivers that need to use other fields will do: dev-init_data-rc = rc_allocate_device(); if (!dev-init_data.rc) return -ENOMEM; dev-init_data.rc-driver_name = foo_driver; ... i2c_new_device(i2c_adap, info) 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. See my reply to Jarod. And also, rc_register_device() is anyway going to check other mandatory fields so having it check all of them in one go is just good consistency IMHO. 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. Meaning that you'll my patch with the rc_dev API the way it is basically and then we can revisit the IR_i2c debate later if necessary? If that's what you mean I'm all for it. Ok. I'll do some tests here with the changes, and see if the i2c remotes will keep working after your and my patch. If they're ok, I'll post the patches to the ML and commit on my main tree. Cheers, Mauro -- 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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
On Fri, Nov 12, 2010 at 10:56:34AM -0200, Mauro Carvalho Chehab wrote: Em 12-11-2010 10:12, David Härdeman escreveu: Shouldn't platform_data be const? And you'll break the refcounting done in rc_allocate_device() and rc_free_device() / rc_unregister_device(). Not to mention the silent bugs that may be introduced if anyone modifies rc_allocate_device() without noticing that one driver isn't using it. It will still be const. platform_data will pass a pointer to some struct. The value of the pointer won't change. I don't see why this would break refcounting, as what will happen is that the caller driver will call rc_allocate_device() and fill some fields there, instead of ir_kbd_i2c. I think I've misunderstood what you've been proposing for ir_kbd_i2c. That sounds like a good solution. I'm working on a patch for it right now. Good, I'll just wait until the patches are available to comment :) -- David Härdeman -- 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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
Em 12-11-2010 12:08, David Härdeman escreveu: On Fri, Nov 12, 2010 at 10:56:34AM -0200, Mauro Carvalho Chehab wrote: Em 12-11-2010 10:12, David Härdeman escreveu: Shouldn't platform_data be const? And you'll break the refcounting done in rc_allocate_device() and rc_free_device() / rc_unregister_device(). Not to mention the silent bugs that may be introduced if anyone modifies rc_allocate_device() without noticing that one driver isn't using it. It will still be const. platform_data will pass a pointer to some struct. The value of the pointer won't change. I don't see why this would break refcounting, as what will happen is that the caller driver will call rc_allocate_device() and fill some fields there, instead of ir_kbd_i2c. I think I've misunderstood what you've been proposing for ir_kbd_i2c. That sounds like a good solution. I'm working on a patch for it right now. Good, I'll just wait until the patches are available to comment :) They are there already ;) http://www.spinics.net/lists/linux-media/msg24987.html http://www.spinics.net/lists/linux-media/msg24986.html Tested with a i2c-based cx231xx device (Pixelview Hybrid). Cheers, Mauro -- 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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
Em 10-11-2010 20:01, David Härdeman escreveu: On Wed, Nov 10, 2010 at 02:24:16PM -0200, Mauro Carvalho Chehab wrote: Em 10-11-2010 11:06, David Härdeman escreveu: On Wed, 10 Nov 2010 10:49:10 -0200, Mauro Carvalho Chehab mche...@infradead.org wrote: So, I'll try to merge the pending patches from your tree. I'll let you know if I have any problems. Sounds good. Thanks. David/Jarod, I pushed the merged patches at the tmp_rc tree: http://git.linuxtv.org/mchehab/tmp_rc.git Please test and give me some feedback. It ended that the rc function renaming patch (6/7) broke both mceusb (due to TX changes) and cx231xx-input (a new driver from me, for some devices that uses a crappy i2c uP, instead of the excellent in-cx231xx IR support). I did no tests at all, except for compilation. So, I need your feedback if the patches didn't actually break anything. So far I've noticed that this patch: [media] rc-core: convert winbond-cir removed the old winbond-cir.c file but doesn't add one in the drivers/media/rc/ directory. Weird... it seems that i forgot to add the new file on my tree. I think I know why... there are two files from kernel tree that got deleted during the build time. So, I had to manually revert and re-do the commit. I suspect that there's something bad with those two files at -rc1... Anyway, my rebase to the patch that created the rc_register_device() functions were not ok, as it broke cx231xx-input (a new IR driver added before your series). While debugging the reason, I discovered some problems at the original patch. I just sent an update for it. 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, and then at struct input_dev. It is too much data duplication for no good reason. So, I think we should re-think about your patch 6/7. Comments? Mauro -- 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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
Em 10-11-2010 20:01, David Härdeman escreveu: On Wed, Nov 10, 2010 at 02:24:16PM -0200, Mauro Carvalho Chehab wrote: Em 10-11-2010 11:06, David Härdeman escreveu: On Wed, 10 Nov 2010 10:49:10 -0200, Mauro Carvalho Chehab mche...@infradead.org wrote: So, I'll try to merge the pending patches from your tree. I'll let you know if I have any problems. Sounds good. Thanks. David/Jarod, I pushed the merged patches at the tmp_rc tree: http://git.linuxtv.org/mchehab/tmp_rc.git Please test and give me some feedback. It ended that the rc function renaming patch (6/7) broke both mceusb (due to TX changes) and cx231xx-input (a new driver from me, for some devices that uses a crappy i2c uP, instead of the excellent in-cx231xx IR support). I did no tests at all, except for compilation. So, I need your feedback if the patches didn't actually break anything. So far I've noticed that this patch: [media] rc-core: convert winbond-cir removed the old winbond-cir.c file but doesn't add one in the drivers/media/rc/ directory. Fixed: http://git.linuxtv.org/mchehab/tmp_rc.git?a=commit;h=b84d81386621b2c56b65f80f9428a516a330b095 (note that I rebased the temporary branch, and that the latest patch on it is incomplete and broken) -- 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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
On Thu, 11 Nov 2010 11:54:30 -0200, Mauro Carvalho Chehab mche...@infradead.org 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
Em 11-11-2010 13:19, David Härdeman escreveu: On Thu, 11 Nov 2010 11:54:30 -0200, Mauro Carvalho Chehab mche...@infradead.org 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). 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? 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). 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? Duplicating dozens of fields is not a good idea. 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. It were added as optional, to avoid many changes at the drivers, as a way to speedup drivers porting, but I think that all drivers should properly initialize the fields there. 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). 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. So, I think we should re-think about your patch 6/7. 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. Also, instead of duplicating input_dev fields, directly initialize them inside the drivers, and not at rc-core. 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. Comments? Unsurprisingly, I disagree on the whole re-think part :) :) Cheers, Mauro. -- 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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
On Thu, Nov 11, 2010 at 02:00:38PM -0200, Mauro Carvalho Chehab wrote: Em 11-11-2010 13:19, David Härdeman escreveu: On Thu, 11 Nov 2010 11:54:30 -0200, Mauro Carvalho Chehab mche...@infradead.org 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). 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). 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? Duplicating dozens of fields is not a good idea. Duplicating dozens of fields is a perfectly good idea if the other ideas are worse. The reason why code duplication is usually bad is that there are several implementations of the same function, each slightly different and some likely to be missed if new features are added to one of them. Here the duplication only exists in headers, not in actual code, and only comes into play if e.g. a callback function signature is changed. Say that the signature of a callback function in struct ir_dev_props/rc_dev was changed in a patch. With the struct ir_dev_props scenario, you'd need to change one line in ir-core.h (change the function signature in struct ir_dev_props) and one line in each and every one of 30 or so different drivers (31 lines changed). After my patch, you'd need to change one line in ir-core.h (change the function signature in struct ir_dev_props) and one line in each and every one of 30 or so different drivers + a one line change to files like ir-kbd-i2c.h which have a duplicate definition of the callback function (say...34 lines changed?). Not a big deal. And in both scenarios, the compiler will warn you about each and every necessary change. (Talking of duplication, having every driver do an identical kzalloc(), check if kzalloc succeeds, additional error path, and kfree() in the unregister phase for the ir_dev_props struct is also duplication. *Some* duplication is unavoidable). 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. It were added as optional, to avoid many changes at the drivers, as a way to speedup drivers porting, but I think that all drivers should properly initialize
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
On Thu, Nov 11, 2010 at 3:35 PM, David Härdeman da...@hardeman.nu 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. 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? :) 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... -- Jarod Wilson ja...@wilsonet.com -- 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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
Em 11-11-2010 21:40, Jarod Wilson escreveu: On Thu, Nov 11, 2010 at 3:35 PM, David Härdeman da...@hardeman.nu 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,
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
On Tue, 09 Nov 2010 23:50:17 -0200, Mauro Carvalho Chehab mche...@infradead.org wrote: Hi David, Em 02-11-2010 18:26, Jarod Wilson escreveu: On Tue, Nov 2, 2010 at 4:17 PM, David Härdeman da...@hardeman.nu wrote: This is my current patch queue, the main change is to make struct rc_dev the primary interface for rc drivers and to abstract away the fact that there's an input device lurking in there somewhere. In addition, the cx88 and winbond-cir drivers are converted to use rc-core. The patchset is now based on current linux-2.6 upstream git tree since it carries both the v4l patches from the staging/for_v2.6.37-rc1 branch, large scancode support and bugfixes. Given the changes, these patches touch every single driver. Obviously I haven't tested them all due to a lack of hardware (I have made sure that all drivers compile without any warnings and I have tested the end result on mceusb and winbond-cir hardware, Jarod Wilson has tested nuvoton-cir, imon and several mceusb devices). And streamzap! :) Mauro's at the kernel summit, but I had a brief moment to talk to him earlier today. He had a few issues he wanted to give feedback on, but I didn't get any specifics yet, other than him not liking the rc-map.c bits merged into rc-main.c, mainly because part of the plan is to remove in-kernel maps entirely in 2.6.38. It doesn't make a big difference to me either way, and rc-main.c is still only 1300-ish lines, and would be even less once rc-map.c bits are ripped out... Sorry for giving you a late feedback about those patches. I was busy the last two weeks, due to my trip to US for KS/LPC. I've applied patches 1 to 3 (in fact, I got the patches from the previous version - unfortunately, patchwork do a very bad job when someone sends a new series that superseeds the previous patches). Kinda makes it pointless to refresh patchsets, doesn't it? I didn't like patch 4 for some reasons: instead of just doing rename, it is a all-in-one patch, doing several things at the same time. It is hard to analyse it by just looking at the diffs, as it is not a pure rename patch. It was an almost pure merge + rename (it added only the things that follow from the merger...forward declarations and making functions static). The real problem is that, since it includes the removal of files, those files need to be identical. Also, it doesn't rename /drivers/media/IR into something else. No, of course not, that would have made the patch even larger for little gain. I see that you've included a renaming patch in your patchset sent to the list, but wouldn't it be easier to apply it after all the other patches rather than before? Btw, the patch is currently broken: $ quilt push Applying patch patches/lmml_298052_4_6_ir_core_merge_and_rename_to_rc_core.patch patching file drivers/media/IR/Makefile patching file drivers/media/IR/ir-core-priv.h patching file drivers/media/IR/ir-keytable.c Hunk #1 FAILED at 1. File drivers/media/IR/ir-keytable.c is not empty after patch, as expected 1 out of 1 hunk FAILED -- rejects in file drivers/media/IR/ir-keytable.c Not sure if you used the most recent version of patch 4/6 or not. If you used the most recent, it's based on 2.6.37-rc1 upstream which has both the large-input-scancodes patches as well as two important bugfixes to ir-keytable.c, so since your staging/for_v2.6.38 is based on 2.6.36 plus the staging/for_v2.6.37-rc1 branch, it won't apply. If you used to second most recent, then it's based on the input-large-scancodes being merged but not the two upstream bugfixes which followed it. Whichever way you choose, those two bugfixes should not get lost in the noise. patching file drivers/media/IR/ir-raw-event.c patching file drivers/media/IR/ir-sysfs.c patching file drivers/media/IR/rc-main.c patching file drivers/media/IR/rc-map.c patching file drivers/media/IR/rc-raw.c patching file include/media/ir-core.h Patch patches/lmml_298052_4_6_ir_core_merge_and_rename_to_rc_core.patch does not apply (enforce with -f) I think that the better is if I write a few patches doing the basic rename stuff, based on my current tip, and then we can discuss about merging things into a fewer number of files, as you're proposing, and apply patch 5/6 and 6/6. Not sure why, but patchwork didn't seem to catch patch 6/6. I suspect that it is because your name is not encoded with UTF-8 inside the driver. I've picked it manually here, and fixed the naming stuff, but it needs patch 5/6, in order to work. My name used to be UTF-8 encoded in winbond-cir, and it was changed upstream (not by me), so I'm not going to revert it. I'll be pushing the renaming stuff soon at ML. I'll try to use your naming convention and, if I do it well, maybe I can apply patches 5/6 and 6/6 on it without rebasing. Well, let's see. I'll wait for more feedback then? -- David Härdeman -- To unsubscribe from this list: send the line unsubscribe
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
On Tue, 9 Nov 2010 22:25:56 -0500, Jarod Wilson ja...@wilsonet.com wrote: On Tue, Nov 9, 2010 at 8:50 PM, Mauro Carvalho Chehab mche...@infradead.org wrote: ... Sorry for giving you a late feedback about those patches. I was busy the last two weeks, due to my trip to US for KS/LPC. I've applied patches 1 to 3 (in fact, I got the patches from the previous version - unfortunately, patchwork do a very bad job when someone sends a new series that superseeds the previous patches). I didn't like patch 4 for some reasons: instead of just doing rename, it is a all-in-one patch, doing several things at the same time. It is hard to analyse it by just looking at the diffs, as it is not a pure rename patch. Also, it doesn't rename /drivers/media/IR into something else. Btw, the patch is currently broken: Hm, the series applied cleanly against 2.6.37-rc1 a bit ago: http://git.kernel.org/?p=linux/kernel/git/jarod/linux-2.6-ir.git;a=shortlog;h=refs/heads/staging Still does, and it's no wonder it doesn't apply to staging/for_v2.6.38 since it's a franken-branch of 2.6.36 + staging/for_v2.6.37 + some more patches. -- David Härdeman -- 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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
Em 10-11-2010 07:24, David Härdeman escreveu: On Tue, 09 Nov 2010 23:50:17 -0200, Mauro Carvalho Chehab mche...@infradead.org wrote: Hi David, Em 02-11-2010 18:26, Jarod Wilson escreveu: On Tue, Nov 2, 2010 at 4:17 PM, David Härdeman da...@hardeman.nu wrote: This is my current patch queue, the main change is to make struct rc_dev the primary interface for rc drivers and to abstract away the fact that there's an input device lurking in there somewhere. In addition, the cx88 and winbond-cir drivers are converted to use rc-core. The patchset is now based on current linux-2.6 upstream git tree since it carries both the v4l patches from the staging/for_v2.6.37-rc1 branch, large scancode support and bugfixes. Given the changes, these patches touch every single driver. Obviously I haven't tested them all due to a lack of hardware (I have made sure that all drivers compile without any warnings and I have tested the end result on mceusb and winbond-cir hardware, Jarod Wilson has tested nuvoton-cir, imon and several mceusb devices). And streamzap! :) Mauro's at the kernel summit, but I had a brief moment to talk to him earlier today. He had a few issues he wanted to give feedback on, but I didn't get any specifics yet, other than him not liking the rc-map.c bits merged into rc-main.c, mainly because part of the plan is to remove in-kernel maps entirely in 2.6.38. It doesn't make a big difference to me either way, and rc-main.c is still only 1300-ish lines, and would be even less once rc-map.c bits are ripped out... Sorry for giving you a late feedback about those patches. I was busy the last two weeks, due to my trip to US for KS/LPC. I've applied patches 1 to 3 (in fact, I got the patches from the previous version - unfortunately, patchwork do a very bad job when someone sends a new series that superseeds the previous patches). Kinda makes it pointless to refresh patchsets, doesn't it? Yes. I didn't like patch 4 for some reasons: instead of just doing rename, it is a all-in-one patch, doing several things at the same time. It is hard to analyse it by just looking at the diffs, as it is not a pure rename patch. It was an almost pure merge + rename (it added only the things that follow from the merger...forward declarations and making functions static). The real problem is that, since it includes the removal of files, those files need to be identical. Also, it doesn't rename /drivers/media/IR into something else. No, of course not, that would have made the patch even larger for little gain. I see that you've included a renaming patch in your patchset sent to the list, but wouldn't it be easier to apply it after all the other patches rather than before? When such change is done as I did, git is smart enough to do the right thing. So, even if the file suffered changes, it is still a rename operation there, even if the file had changes. Btw, the patch is currently broken: $ quilt push Applying patch patches/lmml_298052_4_6_ir_core_merge_and_rename_to_rc_core.patch patching file drivers/media/IR/Makefile patching file drivers/media/IR/ir-core-priv.h patching file drivers/media/IR/ir-keytable.c Hunk #1 FAILED at 1. File drivers/media/IR/ir-keytable.c is not empty after patch, as expected 1 out of 1 hunk FAILED -- rejects in file drivers/media/IR/ir-keytable.c Not sure if you used the most recent version of patch 4/6 or not. If you used the most recent, it's based on 2.6.37-rc1 upstream which has both the large-input-scancodes patches as well as two important bugfixes to ir-keytable.c, so since your staging/for_v2.6.38 is based on 2.6.36 plus the staging/for_v2.6.37-rc1 branch, it won't apply. Gah! Yeah, my tree is based on 2.6.36, but we need to be based on .37-rc1. I'll merge .37-rc1. If you used to second most recent, then it's based on the input-large-scancodes being merged but not the two upstream bugfixes which followed it. Whichever way you choose, those two bugfixes should not get lost in the noise. Yeah, sure. The git renaming patches will probably do the right thing. I'll double check. patching file drivers/media/IR/ir-raw-event.c patching file drivers/media/IR/ir-sysfs.c patching file drivers/media/IR/rc-main.c patching file drivers/media/IR/rc-map.c patching file drivers/media/IR/rc-raw.c patching file include/media/ir-core.h Patch patches/lmml_298052_4_6_ir_core_merge_and_rename_to_rc_core.patch does not apply (enforce with -f) I think that the better is if I write a few patches doing the basic rename stuff, based on my current tip, and then we can discuss about merging things into a fewer number of files, as you're proposing, and apply patch 5/6 and 6/6. Not sure why, but patchwork didn't seem to catch patch 6/6. I suspect that it is because your name is not encoded with UTF-8 inside the driver. I've picked it manually here, and fixed the naming stuff, but it
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
On Wed, 10 Nov 2010 10:49:10 -0200, Mauro Carvalho Chehab mche...@infradead.org wrote: Em 10-11-2010 07:24, David Härdeman escreveu: Not sure if you used the most recent version of patch 4/6 or not. If you used the most recent, it's based on 2.6.37-rc1 upstream which has both the large-input-scancodes patches as well as two important bugfixes to ir-keytable.c, so since your staging/for_v2.6.38 is based on 2.6.36 plus the staging/for_v2.6.37-rc1 branch, it won't apply. Gah! Yeah, my tree is based on 2.6.36, but we need to be based on .37-rc1. I'll merge .37-rc1. I think/hope my patches will apply with little to no fuzz once you've done that. My name used to be UTF-8 encoded in winbond-cir, and it was changed upstream (not by me), so I'm not going to revert it. Patchwork handles very badly charset encodings, due to Python. I sent several patches for it to fix several problems I noticed there. Basically, Python kills any script if the an invalid character is inserted on a string. E. g., if your emailer says that the email is encoded as UTF-8, and a non-UTF-8 character is found on any part of the email, the script will die, as it will try to write the email contents on some vars. Due to that, a patch/email with an invalid character on his charset will be silently discarded by patchwork, as the script will die. Not much I can do there :) I suspect that your emailer might be doing some bad things also, as I need to manually fix your author's name every time. In general the SOB on your emails have one encoding, while the From: has another encoding. I didn't know that, I'll have a look. I'm guessing that both iso88591-1 and utf-8 encodings are used. So, I'll try to merge the pending patches from your tree. I'll let you know if I have any problems. Sounds good. Thanks. -- David Härdeman -- 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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
Em 10-11-2010 11:06, David Härdeman escreveu: On Wed, 10 Nov 2010 10:49:10 -0200, Mauro Carvalho Chehab mche...@infradead.org wrote: So, I'll try to merge the pending patches from your tree. I'll let you know if I have any problems. Sounds good. Thanks. David/Jarod, I pushed the merged patches at the tmp_rc tree: http://git.linuxtv.org/mchehab/tmp_rc.git Please test and give me some feedback. It ended that the rc function renaming patch (6/7) broke both mceusb (due to TX changes) and cx231xx-input (a new driver from me, for some devices that uses a crappy i2c uP, instead of the excellent in-cx231xx IR support). I did no tests at all, except for compilation. So, I need your feedback if the patches didn't actually break anything. I'll do some tests with cx231xx-input/mceusb probably later today. Cheers, Mauro. -- 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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
On Wed, Nov 10, 2010 at 02:24:16PM -0200, Mauro Carvalho Chehab wrote: Em 10-11-2010 11:06, David Härdeman escreveu: On Wed, 10 Nov 2010 10:49:10 -0200, Mauro Carvalho Chehab mche...@infradead.org wrote: So, I'll try to merge the pending patches from your tree. I'll let you know if I have any problems. Sounds good. Thanks. David/Jarod, I pushed the merged patches at the tmp_rc tree: http://git.linuxtv.org/mchehab/tmp_rc.git Please test and give me some feedback. It ended that the rc function renaming patch (6/7) broke both mceusb (due to TX changes) and cx231xx-input (a new driver from me, for some devices that uses a crappy i2c uP, instead of the excellent in-cx231xx IR support). I did no tests at all, except for compilation. So, I need your feedback if the patches didn't actually break anything. So far I've noticed that this patch: [media] rc-core: convert winbond-cir removed the old winbond-cir.c file but doesn't add one in the drivers/media/rc/ directory. -- David Härdeman -- 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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
On Tue, 02 Nov 2010 21:17:38 +0100, David Härdeman da...@hardeman.nu wrote: This is my current patch queue, the main change is to make struct rc_dev the primary interface for rc drivers and to abstract away the fact that there's an input device lurking in there somewhere. Mauro, you have neither commented on the patches nor committed them. At the same time you've created a for_v2.6.38 branch where you've already committed other IR related patches. Could you please provide some feedback on what the plan is? -- David Härdeman -- 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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
Hi David, Em 09-11-2010 08:27, David Härdeman escreveu: On Tue, 02 Nov 2010 21:17:38 +0100, David Härdeman da...@hardeman.nu wrote: This is my current patch queue, the main change is to make struct rc_dev the primary interface for rc drivers and to abstract away the fact that there's an input device lurking in there somewhere. Mauro, you have neither commented on the patches nor committed them. At the same time you've created a for_v2.6.38 branch where you've already committed other IR related patches. Could you please provide some feedback on what the plan is? I've returned from LPC at Sunday. In the last two weeks, I received about 140+ patches at patchwork, plus 8 pull requests, plus tons of emails that I received at the last week. So, I have lots of backlog to handle. My intention is to handle the pending stuff during this week. Cheers, Mauro -- 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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
On Tue, Nov 9, 2010 at 8:50 PM, Mauro Carvalho Chehab mche...@infradead.org wrote: ... Sorry for giving you a late feedback about those patches. I was busy the last two weeks, due to my trip to US for KS/LPC. I've applied patches 1 to 3 (in fact, I got the patches from the previous version - unfortunately, patchwork do a very bad job when someone sends a new series that superseeds the previous patches). I didn't like patch 4 for some reasons: instead of just doing rename, it is a all-in-one patch, doing several things at the same time. It is hard to analyse it by just looking at the diffs, as it is not a pure rename patch. Also, it doesn't rename /drivers/media/IR into something else. Btw, the patch is currently broken: Hm, the series applied cleanly against 2.6.37-rc1 a bit ago: http://git.kernel.org/?p=linux/kernel/git/jarod/linux-2.6-ir.git;a=shortlog;h=refs/heads/staging -- Jarod Wilson ja...@wilsonet.com -- 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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
Em 10-11-2010 01:25, Jarod Wilson escreveu: On Tue, Nov 9, 2010 at 8:50 PM, Mauro Carvalho Chehab mche...@infradead.org wrote: ... Sorry for giving you a late feedback about those patches. I was busy the last two weeks, due to my trip to US for KS/LPC. I've applied patches 1 to 3 (in fact, I got the patches from the previous version - unfortunately, patchwork do a very bad job when someone sends a new series that superseeds the previous patches). I didn't like patch 4 for some reasons: instead of just doing rename, it is a all-in-one patch, doing several things at the same time. It is hard to analyse it by just looking at the diffs, as it is not a pure rename patch. Also, it doesn't rename /drivers/media/IR into something else. Btw, the patch is currently broken: Hm, the series applied cleanly against 2.6.37-rc1 a bit ago: http://git.kernel.org/?p=linux/kernel/git/jarod/linux-2.6-ir.git;a=shortlog;h=refs/heads/staging Probably, some other RC patch broke it. It doesn't apply cleanly against staging/for_v2.6.38. Cheers, Mauro -- 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
Re: [PATCH 0/6] rc-core: ir-core to rc-core conversion
On Tue, Nov 2, 2010 at 4:17 PM, David Härdeman da...@hardeman.nu wrote: This is my current patch queue, the main change is to make struct rc_dev the primary interface for rc drivers and to abstract away the fact that there's an input device lurking in there somewhere. In addition, the cx88 and winbond-cir drivers are converted to use rc-core. The patchset is now based on current linux-2.6 upstream git tree since it carries both the v4l patches from the staging/for_v2.6.37-rc1 branch, large scancode support and bugfixes. Given the changes, these patches touch every single driver. Obviously I haven't tested them all due to a lack of hardware (I have made sure that all drivers compile without any warnings and I have tested the end result on mceusb and winbond-cir hardware, Jarod Wilson has tested nuvoton-cir, imon and several mceusb devices). And streamzap! :) Mauro's at the kernel summit, but I had a brief moment to talk to him earlier today. He had a few issues he wanted to give feedback on, but I didn't get any specifics yet, other than him not liking the rc-map.c bits merged into rc-main.c, mainly because part of the plan is to remove in-kernel maps entirely in 2.6.38. It doesn't make a big difference to me either way, and rc-main.c is still only 1300-ish lines, and would be even less once rc-map.c bits are ripped out... -- Jarod Wilson ja...@wilsonet.com -- 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
