Em 02-08-2010 05:02, Dmitry Torokhov escreveu:

On Sat, Jul 31, 2010 at 10:23:45AM -0300, Mauro Carvalho Chehab wrote:

quoted

Hi Dmitry,

Em 31-07-2010 06:19, Dmitry Torokhov escreveu:

quoted

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!

quoted

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.

quoted

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.

quoted

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.

quoted

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!

quoted

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 <redacted>

Cheers,
Mauro