Hi Dmitry,
Sorry for the slow response.
On Dec 23, 2007 6:29 PM, Dmitry Baryshkov <dbaryshkov@xxxxxxxxx> wrote:
> > From b213bef91e3f98c6104138050ad4eaa0a54e765d Mon Sep 17 00:00:00 2001
> From: Dmitry Baryshkov <dbaryshkov@xxxxxxxxx>
> Date: Mon, 24 Dec 2007 02:15:34 +0300
> Subject: [PATCH] Tosa keyboard support
>
> Support keyboard on tosa (Sharp Zaurus SL-6000x).
> Largely based on patches by Dirk Opfer.
> This is the final version that supports switching between
> full keycode range and range available to the console and
> keyb Kdrive driver.
>
The driver looks very nice, just a couple minor comments:
> +
> +#ifdef CONFIG_PM
> +static int tosakbd_suspend(struct platform_device *dev, pm_message_t state)
> +{
> + /* Nothing yet */
Stop the polling timer here?
> +
> + return 0;
> +}
> +
...
> +
> + tosakbd->input = input_dev;
> +
> + input_dev->private = tosakbd;
Please use input_set_drvdata(). I am about to commit the change
removing private from input_dev structure.
> + input_dev->name = "Tosa Keyboard";
> + input_dev->phys = "tosakbd/input0";
> + input_dev->cdev.dev = &pdev->dev;
Please use input_dev->dev.parent. cdev is going away.
> +
> + input_dev->id.bustype = BUS_HOST;
> + input_dev->id.vendor = 0x0001;
> + input_dev->id.product = 0x0001;
> + input_dev->id.version = 0x0100;
> +
> + input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REP);
> + input_dev->keycode = tosakbd->keycode;
> + input_dev->keycodesize = sizeof(unsigned int);
> + input_dev->keycodemax = ARRAY_SIZE(tosakbd_keycode);
> +
> + memcpy(tosakbd->keycode, tosakbd_keycode, sizeof(tosakbd_keycode));
> +
> + for (i = 0; i < ARRAY_SIZE(tosakbd_keycode); i++)
> + set_bit(tosakbd->keycode[i], input_dev->keybit);
> + clear_bit(0, input_dev->keybit);
Might want to switch to __set_bit to save couple of bytes. We dont
need atomicity here.
> +
> +#define TOSA_KEY_SYNC KEY_102ND /* ??? */
> +
> +
> +#ifndef CONFIG_KEYBOARD_TOSA_USE_EXT_KEYCODES
> +#define TOSA_KEY_RECORD KEY_YEN
> +#define TOSA_KEY_ADDRESSBOOK KEY_KATAKANA
> +#define TOSA_KEY_CANCEL KEY_ESC
> +#define TOSA_KEY_CENTER KEY_HIRAGANA
> +#define TOSA_KEY_OK KEY_HENKAN
> +#define TOSA_KEY_CALENDAR KEY_KATAKANAHIRAGANA
> +#define TOSA_KEY_HOMEPAGE KEY_HANGEUL
> +#define TOSA_KEY_LIGHT KEY_MUHENKAN
> +#define TOSA_KEY_MENU KEY_HANJA
> +#define TOSA_KEY_FN KEY_RIGHTALT
> +#define TOSA_KEY_MAIL KEY_ZENKAKUHANKAKU
> +#else
> +#define TOSA_KEY_RECORD KEY_RECORD
> +#define TOSA_KEY_ADDRESSBOOK KEY_ADDRESSBOOK
> +#define TOSA_KEY_CANCEL KEY_CANCEL
> +#define TOSA_KEY_CENTER KEY_SELECT /* ??? */
> +#define TOSA_KEY_OK KEY_OK
> +#define TOSA_KEY_CALENDAR KEY_CALENDAR
> +#define TOSA_KEY_HOMEPAGE KEY_HOMEPAGE
> +#define TOSA_KEY_LIGHT KEY_SWITCHVIDEOMODE /* ??? */
KEY_ILLUMTOGGLE maybe? SWITCHWIDEOMODE is for cycling between outputs...
Also, I'd put these defines rigth in tosakbd.c.
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
