[U-Boot] [PATCH v3 1/4] PXA: PXA27x Matrix keypad driver

[Marek Vasut]

> 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.

Then change it to space ... (1 <<[TAB]3) is even weirder. And on my side, it 
still looks like two spaces.

> 
> > > +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(&regs->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.

WATCHDOG_RESET() reloads the WDT ...

> 
> > > +	} 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