Em 02-08-2010 05:02, Dmitry Torokhov escreveu: > On Sat, Jul 31, 2010 at 10:23:45AM -0300, Mauro Carvalho Chehab wrote: >> Hi Dmitry, >> >> Em 31-07-2010 06:19, Dmitry Torokhov escreveu: >>> Hi Mauro, >>> >>> I finally got a chance to review the patches adding handling of large >>> scancodes to input core and there are a few things with this approach >>> that I do not like. >> >> Thanks for the review! >> >>> First of all I do not think that we should be working with scancode via >>> a pointer as it requires additional compat handling when running 32-bit >>> userspace on 64-bit kernel. We can use a static buffer of sufficient >>> size (lets say 32 bytes) to move scancode around and simply increase its >>> size if we come upon device that uses even bigger scancodes. As long as >>> buffer is at the end we can handle this in a compatible way. >> >> Yes, this is the downside of using a pointer. I'm not aware of a Remote >> Controller protocol using more than 256 bits for scancode, so 32 bits >> should be ok. >> >>> The other issue is that interface is notsymmetrical, setting is done by >>> scancode but retrieval is done by index. I think we should be able to >>> use both scancode and index in both operations. >> >> Yes, this also bothered me. I was thinking to do something similar to your >> approach of having a bool to select between them. This change is welcome. >> >>> The usefulnes of reserved data elements in the structure is doubtful, >>> since we do not seem to require them being set to a particular value and >>> so we'll be unable to distinguish betwee legacy and newer users. >> >> David proposed some parameters that we rejected on our discussions. As we >> might need to add something similar, I decided to keep it on my approach, >> since a set of reserved fields wouldn't hurt (and removing it on our discussions >> would be easy), but I'm ok on removing them. >> >>> I also concerned about the code very messy with regard to using old/new >>> style interfaces instea dof converting old ones to use new >>> insfrastructure, >> >> Good cleanup at the code! >> >>> I below is something that addresses these issues and seems to be working >>> for me. It is on top of your patches and it also depends on a few >>> changes in my tree that I have not publushed yet but plan on doing that >>> tomorrow. I am also attaching patches converting sparse keymap and hid >>> to the new style of getkeycode and setkeycode as examples. >>> >>> Please take a look and let me know if I missed something important. >> >> It seems to work for me. After you add the patches on your git tree, I'll >> work on porting the RC core to the new approach, and change the ir-keycode >> userspace program to work with, in order to be 100% sure that it will work, >> but I can't foresee any missing part on it. >> >> Currently, I'm not using my input patches, as I was waiting for your >> review. I just patched the userspace application, in order to test the legacy >> mode. >> > > OK, great. > > I want to fold all the patches from your tree plus this one into one and > apply in one shpt (mentioning Jarod and Dan as authors of fixup patches > in the changelog) - I do not believe we shoudl expose intermediate > versions in the code that will go to Linus. Are you OK with this? I'm OK. If you want, you can add my ack on your patch: Acked-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> 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
