Hi Russell,
On Sat, Apr 18, 2009 at 03:20:50PM +0100, Russell King wrote:
> This adds support for the keyboard support on the LX platform.
> We aren't directly connected to the keyboard itself, but the
> keyboard is managed by PCON via the system I2C bus.
>
Very nice driver. I prefer input devices having a private copy of keymap
but I suppose there is just such device in the system anyways...
>
> +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.
> +
> + /*
> + * 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.
> + break;
> +
> + case KEY_SUSPEND:
> + input_event(kbd, EV_PWR, key, !up);
> +#if defined(CONFIG_APM_EMULATION) && !defined(CONFIG_INPUT_APMPOWER)
> + if (!up)
> + apm_queue_event(APM_SYS_SUSPEND);
> +#endif
> + break;
> +
> + default:
> + input_report_key(kbd, key, !up);
> + break;
> + }
> +}
> +
> +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.
> +
> + num_lck = 0;
> +}
> +
> +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 ;)
> + pcon_kbd_ack();
> + lx_keyb_process_key(kbd, val);
> +}
> +
> +static struct lx_i2cslave_watcher lx_keyb_watcher = {
> + .start = PHST_KEYBOARD_START,
> + .size = PHST_KEYBOARD_SIZE,
> + .write = lx_keyb_write,
> +};
> +
> +static int lx_keyb_probe(struct platform_device *dev)
> +{
> + struct input_dev *kbd;
> + int i, ret;
> +
> + kbd = input_allocate_device();
> + if (!kbd)
> + return -ENOMEM;
> +
> + 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.
> + for (i = 0; i < ARRAY_SIZE(normal_map); i++) {
> + if (normal_map[i])
> + __set_bit(normal_map[i], kbd->keybit);
> + }
> + for (i = 0; i < ARRAY_SIZE(numlck_map); i++) {
> + if (numlck_map[i])
> + __set_bit(numlck_map[i], kbd->keybit);
> + }
> + for (i = 0; i < ARRAY_SIZE(fn_map); i++) {
> + if (fn_map[i])
> + __set_bit(fn_map[i], kbd->keybit);
> + }
> +
> + ret = input_register_device(kbd);
> + if (ret == 0) {
> + platform_set_drvdata(dev, kbd);
> + lx_i2cslave_addwatcher(&lx_keyb_watcher, kbd);
> + } else {
> + input_free_device(kbd);
> + }
> + return ret;
> +}
> +
> +static int __devexit lx_keyb_remove(struct platform_device *dev)
> +{
> + struct input_dev *kbd = platform_get_drvdata(dev);
> + platform_set_drvdata(dev, NULL);
> + lx_i2cslave_delwatcher(&lx_keyb_watcher, kbd);
> + input_unregister_device(kbd);
> + return 0;
> +}
> +
> +static int lx_keyb_resume(struct platform_device *dev)
> +{
> + struct input_dev *kbd = platform_get_drvdata(dev);
> + lx_keyb_reset(kbd);
> + return 0;
> +}
> +
> +static struct platform_driver lx_kbd = {
> + .driver = {
> + .name = "lx-kbd",
> + .owner = THIS_MODULE,
> + },
> + .probe = lx_keyb_probe,
> + .remove = __devexit_p(lx_keyb_remove),
> + .resume = lx_keyb_resume,
> +};
> +
> +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...
Thanks!
--
Dmitry
--
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
