RE: [PATCH try #3] Blackfin BF54x Input Keypad controller driver

Hi Dmitry,

From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
Sent: Donnerstag, 11. Oktober 2007 21:58
To: Hennerich, Michael
Cc: bryan.wu@analog.com; linux-input@atrey.karlin.mff.cuni.cz; Linux
Kernel; uclinux-dist-devel@blackfin.uclinux.org
Subject: Re: [PATCH try #3] Blackfin BF54x Input Keypad controller

driver

Hi Michael,

On 10/11/07, Hennerich, Michael [off-list ref] wrote:

quoted

quoted

From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
Sent: Donnerstag, 11. Oktober 2007 19:27
To: bryan.wu@analog.com
Cc: Michael Hennerich; linux-input@atrey.karlin.mff.cuni.cz; Linux

Kernel;

quoted

uclinux-dist-devel@blackfin.uclinux.org
Subject: Re: [PATCH try #3] Blackfin BF54x Input Keypad controller

driver

quoted

On Thursday 11 October 2007, Bryan Wu wrote:

quoted

From: Michael Hennerich <michael.hennerich@analog.com>
Date: Fri, 12 Oct 2007 00:20:19 +0800
Subject: [PATCH] Blackfin BF54x Input Keypad controller driver

[try #2] Changelog:
 - Coding style issue fixes
 - using a temp variable for bf54x_kpad->input
 - Other updates according to Dmitry's review

[try #3] Changelog:
 - Coding style cleanups
 - Use local copy of keymap
 - Remove empty PM functions
 - Use input_set_drvdata() since input->private is going away

This looks very good, thank youvery much. I have only 1 more

suggestion

quoted

quoted

(and I can make the changes locally, you don't need to resubmit) -
since it is highly unlikely that we will have a keycode > 65535 do

you

quoted

quoted

think we could change keymap to unsigned short. It would save you a
couple of bytes. I am also going to change "input->cdev.dev =

&pdev->dev;"

quoted

to "input_dev->dev.parent = &pdev->dev;". Please let me know if you

are

quoted

quoted

OK with it.

Hi Dmitry,

Thanks for your excellent feedback and support.

We do a pretty similar thing like the omap keypad driver.
Our keycodes do not exceed KEY_MAX but we encode the ROW and COL into
the keycode matrix, in order simplify things in a "normal use case"?

.

quoted

What we store is u32, 16-bit for the keycode and the upper for

private

quoted

use. My understanding is that we then have to set input->keycodesize
also being u32.

Otherwise the generic input layer might get screwed up?!

Am I wrong here, or do I miss something?
Either this is a valid use case or I was inspired by a bad example.

Thank you very much for alerting me of omap case, I missed it.

Drivers that use complex values in their keymap tables should not use
input->keycode, keycodesize and keycodemax. These fields are only used
by input core if the driver relies on default implementation of
getkeycode() and setkeycode() for manipulations with its keymap. But
this will not work in your case (nor with omap) because your keymap
table does not contain "pure" input keycodes.

Well, I guess the easiest is just not set these fields (and remove the
per-device map allocationand copying) and say that the driver does not
support altering keymaps. After all, like you said, it is an embedded
platform and it is unlikely that users will want to alter keymaps.

However, if you think that this is a nice feature, then you either
need to rethink how you handle your keymap of implement custom
getkeycode and setkeycode methods in your driver. The coice is yours,
I will gladly accept either version of drver.

Well it's a nice feature - 

I changed following:

- Change keymapsize to u16
- Copy privately used LUT behind keycodemax in keycode table.
This is more cache friendly when doing the tablewalk, and goes totally
unnoticed be the input device layer. 
- Change Input_dev->cdev.dev to input_dev->dev.parent

Bryan is going to send you an updated patch soon.

Thanks and best regards,
Michael

--
Dmitry