On Fri, 2012-01-13 at 13:41 +0100, Marek Vasut wrote:
+#define KPDK_DK1 (0x1 << 1) +#define KPDK_DK0 (0x1 << 0)
Drop those two spaces here
Two spaces? It's tabs! Btw, it's for good looking formatting and it was taken from pxa-regs.h. I'd like to preserve it like it is.
+static unsigned char queue[64] = {0}; +static int queue_len; +static struct pxa_keypad_regs *regs;
You can as well assign it here.
Ok
+static int scan_keys(int scan_modif, uint32_t kpasmkp[4]) +{
- uint32_t reg = 0;
- int col, row;
- static int mod = MOD_NONE;
Are you sure you know what you're doing using all those static vars all around?
I'm sure I know what I'm doing, right here it preserves modifier between two function calls, i.e. scan_keys(1, ...); scan_keys(0, ...);
if ((reg >> 16) & (1 << row)) {if (scan_modif) {mod = kbd_get_mdf(row, col + 1);if (mod != MOD_NONE)return mod;} else {key = kbd_lookup(row, col + 1, mod);if (key != 0xff)return key;}Can you do some elegant solution of this code duplication?
Ok
}}- }
Newline
Ok
- /* wait for scan to finish */
- while (timeout--) {
if (!(readl(®s->kpc) & KPC_AS))break;udelay(10);Use WATCHDOG_RESET()
Why the heck you want to reset here? It's not fatal. We can handle this timeout gracefully, without resetting and messing up all the stuff.
- } else {
key_counter = 0;last_key_row = last_key_col = 0xff;scan_keys(0, kpasmkp_diff);serial_printf("New key\n");What's this dammit? And above too! Use debug() if you need it.
Sorry, left it occasionally.
- kbddev.getc = kbd_getc ;
- kbddev.tstc = kbd_testc ;
Drop spaces before semicolon
Ok
Thanks for review!
Regards, Vasily