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.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
