Re: [PATCH 3/5] [ARM] pxa: lx: add keyboard driver

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Sat, Apr 18, 2009 at 04:10:28PM -0700, Dmitry Torokhov wrote:
> > +config KEYBOARD_LX
> > +	tristate "LX Keyboard support"
> > +	depends on MACH_NETBOOKPRO && INPUT && INPUT_KEYBOARD
> 
> Dependancy on INPUT_KEYBOARD is not needed, strictly speaking.
> INPUT_KEYBOARD is only [menu]config syntactic sugar, there is no meat
> behind the option.

Gone.

> > +
> > +	/*
> > +	 * Some keys have additional operations after we have
> > +	 * decoded them.  We don't report these "special" keys
> > +	 * to the input layer.
> > +	 */
> > +	switch (key) {
> > +	case KEY_BRIGHTNESSUP:
> > +	case KEY_BRIGHTNESSDOWN:
> > +		input_event(kbd, EV_PWR, key, !up);
> 
> Umm... some userspace apps might still want to get these. Strictly
> speaking KEY_* codes belong to EV_KEY namespace.

Maybe, if someone wants it I guess they can modify the driver.

> > +static void lx_keyb_reset(struct input_dev *kbd)
> > +{
> > +	unsigned char *map;
> > +	int key;
> > +
> > +	for (key = 0; key < LX_NUMKEYCODES; key++)
> > +		if (keystate[key] & KP_DOWN) {
> > +			map = normal_map;
> > +			if (keystate[key] & KP_FN)
> > +				map = fn_map;
> > +			else if (keystate[key] & KP_NUMLCK)
> > +				map = numlck_map;
> > +			keystate[key] = KP_NONE;
> > +			input_report_key(kbd, map[key], 0);
> > +		}
> 
> Why don't you just send 'up' event for all known keys? Input core will
> filter out the ones that have not been 'down'ed... Better yet, don't do
> anything here, I should finish adding forced release at suspend/resume
> to the input core shortly.

I'll leave it as is until you've decided what to do with the input core.

> > +static void lx_keyb_write(void *data, unsigned int addr, unsigned char val)
> > +{
> > +	struct input_dev *kbd = data;
> 
> Blank line between variables and the code is always appreciated ;)

Added.

> > +	kbd->name        = "LX Keyboard";
> > +	kbd->phys        = "pcon/input0";
> > +	kbd->dev.parent  = &dev->dev;
> > +	kbd->evbit[0]    = BIT_MASK(EV_KEY) | BIT_MASK(EV_REP) |
> > +			   BIT_MASK(EV_PWR);
> > +	kbd->keycode     = normal_map;
> > +	kbd->keycodesize = sizeof(unsigned char);
> > +	kbd->keycodemax  = ARRAY_SIZE(normal_map);
> > +
> 
> So we have 3 keymaps but only one can be changed form userspace... I
> suppose it is OK given how special the other 2 keymaps are. Or we could
> provide an alternative {get,set}keycode methods.

Yes, not sure what to do about those.  It is a rather specialised
keyboard in itself, so the standard maps wouldn't work with it.

The other issue here is that changing what's published may break the
existing userspace (and since I didn't take a copy of the userspace
build for this platform before the server was taken down, that could
present something of a problem resulting in my use of the device being
crippled.)  Let's leave it as is and if someone does want to change it
later...

> > +static int __init lx_keyb_init(void)
> > +{
> > +	printk(KERN_INFO "LX Keyboard Driver, (c) 2004 Simtec Electronics\n");
> 
> Is this needed? Input core will already add printk when input device is
> added and our boot is already chatty enough...

As mentioned in the other mail, now removed.