[U-Boot] [PATCH] Add support for i.MX6SL EVK board keypad

[Wolfgang Denk]

Dear nitin.garg at freescale.com,

In message <1409520692-30412-2-git-send-email-nitin.garg at freescale.com> you wrote:

> +static void mxc_kpp_dump_regs()
> +{
> +	unsigned short t1, t2, t3;
> +
> +	t1 = __raw_readw(KPCR);
> +	t2 = __raw_readw(KPSR);
> +	t3 = __raw_readw(KDDR);
> +	/*
> +	KPP_PRINTF("KPCR=0x%04x, KPSR=0x%04x, KDDR=0x%04x\n",
> +		t1, t2, t3);
> +		*/

Please do not add dead code.


> +	/*
> +	 * wmb() linux kernel function which guarantees orderings in write
> +	 * operations
> +	 */
> +	/* wmb(); */

Please remove dead code.  Please fix globally.[

> +	for (col = 0; col < kpp_dev.kpp_cols; col++) {	/* Col */
> +		/* 2. Write 1.s to KPDR[15:8] setting column data to 1.s */
> +		reg_val = __raw_readw(KPDR);
> +		reg_val |= 0xff00;
> +		__raw_writew(reg_val, KPDR);

setbits?

Is there any specific reason for performing all these device accesses
using the __raw_*() functions, i. e. without memory barriers?

That looks awfully suspicious to me...

> +		reg_val = __raw_readw(KPCR);
> +		reg_val &= 0x00ff;
> +		__raw_writew(reg_val, KPCR);

clrbits?

> +		udelay(2);

Why exactly is this udelay() needed?  Maybe just because you are
lacking memory barriers, i. e. because you use the wrong I/O
accessors?

> +		reg_val = __raw_readw(KPCR);
> +		reg_val |= ((1 << kpp_dev.kpp_cols) - 1) << 8;
> +		__raw_writew(reg_val, KPCR);

setbits() ... etc.  Please fix globally.

> +		/* Delay added to avoid propagating the 0 from column to row
> +		 * when scanning. */

Incorrect multiline comment style.  Is this delay really needed when
using non-__raw_ I/O accessors?

> +		udelay(5);
> +

> +					KPress = 1;
> +					kpp_dev.iKeyState = KStateUp;

Please fix variable names globally - they are all lower case.

Please fix everywhere, including the header file, sturct names, etc.

> +
> +					KPP_PRINTF("Press   (%d, %d) scan=%d "
> +						 "Kpress=%d\n",
> +						 row, col, scancode, KPress);

Any specific reason for not using standard debug() facility?


> +	/*
> +	* This switch case statement is the
> +	* implementation of state machine of debounc
> +	* logic for key press/release.
> +	* The explaination of state machine is as
> +	* follows:

There is no switch anywhere in the rest of this function?


> +	/* Enable number of rows in keypad (KPCR[7:0])
> +	 * Configure keypad columns as open-drain (KPCR[15:8])
> +	 *
> +	 * Configure the rows/cols in KPP
> +	 * LSB nibble in KPP is for 8 rows
> +	 * MSB nibble in KPP is for 8 cols
> +	 */

Incorrect multiline comment style. Please fix globally.


> +	press_scancode   = (short **)malloc(kpp_dev.kpp_rows * sizeof(press_scancode[0]));
> +	release_scancode = (short **)malloc(kpp_dev.kpp_rows * sizeof(release_scancode[0]));

Line too long.  Please fix globally.

Also, make sure to run your patches through checkpatch and fix such
warnings!!


> +/*
> + * _reg_KPP_KPCR   _reg_KPP_KPSR _reg_KPP_KDDR _reg_KPP_KPDR
> + * Keypad Control Register Address
> + */
> +#define KPCR    (KPP_BASE_ADDR + 0x00)
> +
> +/*
> + * Keypad Status Register Address
> + */
> +#define KPSR    (KPP_BASE_ADDR + 0x02)
> +
> +/*
> + * Keypad Data Direction Address
> + */
> +#define KDDR    (KPP_BASE_ADDR + 0x04)
> +
> +/*
> + * Keypad Data Register
> + */
> +#define KPDR    (KPP_BASE_ADDR + 0x06)

Please use a C struct to describe the register map.  We deprecate the
notation of base register + offset.


Also consider to providing the comments at the end of the line - this
would reduce your line count to 20% here and make the code much better
to read.


> +enum KeyEvent {
> +	KDepress,
> +	KRelease
> +};
> +
> +/*!
> + * This enum represents the keypad state machine to maintain debounce logic
> + * for key press/release.
> + */
> +enum KeyState {
> +
> +	/*!
> +	 * Key press state.
> +	 */
> +	KStateUp,
> +
> +	/*!
> +	 * Key press debounce state.
> +	 */
> +	KStateFirstDown,
> +
> +	/*!
> +	 * Key release state.
> +	 */
> +	KStateDown,
> +
> +	/*!
> +	 * Key release debounce state.
> +	 */
> +	KStateFirstUp
> +};

See before - please use all lower case variable names.  We do not
allow CamelCase in U-Boot.

Please fix globally!

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It's not an optical illusion, it just looks like one.   -- Phil White