Re: Handling of large keycodes

[Jarod Wilson <jarod@redhat.com>]

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@redhat.com>

Yeah, works for me too.

-- 
Jarod Wilson
jarod@redhat.com