On Mon, Aug 02, 2010 at 08:35:37AM -0300, Mauro Carvalho Chehab wrote: > 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> Yeah, works for me too. -- Jarod Wilson jarod@xxxxxxxxxx -- 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
