Re: [PATCH] input: add support for generic GPIO-based matrix keypad

[Robert Jarzmik <robert.jarzmik@free.fr>]

Eric Miao <eric.y.miao@gmail.com> writes:

Hi Eric, some minor cosmetic points ...

<snip>

> +static void activate_all_cols(struct matrix_keypad_platform_data
> *pdata, int on)
That function declaration spacing looks suspicious, looks like my mailer (or
yours) ate a bit out of it, and patch doesn't accept to apply it.

<snip>

+static void activate_col(struct matrix_keypad_platform_data *pdata,
+			 int col, int on)
+{
+	__activate_col(pdata, col, on);
+
+	if (on && pdata->col_scan_delay_us)
+		udelay(pdata->col_scan_delay_us);
+}
That function is called is the worst case 16 times in a row (if I understood
correctly matrix_keypad_scan()). If delay is 100us, that makes 1.6ms for a
keystroke with all kernel blocked (udelay).
I didn't follow the discussion before, so maybe that's the only way. Still,
that's too bad ...

<snip>

> +/*
> + * This gets the keys from keyboard and reports it to input subsystem
> + */
> +static void matrix_keypad_scan(struct work_struct *work)
> +{
> +	struct matrix_keypad *keypad =
> +	       	container_of(work, struct matrix_keypad, work.work);
> +	struct matrix_keypad_platform_data *pdata = keypad->pdata;
> +	uint32_t new_state[MATRIX_MAX_COLS];
> +	int row, col;
> +
> +	/* de-activate all columns for scanning */
> +	activate_all_cols(pdata, 0);
> +
> +	memset(new_state, 0, sizeof(new_state));
> +
> +	/* assert each column and read the row status out */
> +	for (col = 0; col < pdata->num_col_gpios; col++) {
> +
> +		activate_col(pdata, col, 1);
> +
> +		for (row = 0; row < pdata->num_row_gpios; row++)
> +			new_state[col] |= row_asserted(pdata, row) ?
> +						(1 << row) : 0;
> +		activate_col(pdata, col, 0);
> +	}
> +
> +	for (col = 0; col < pdata->num_col_gpios; col++) {
> +		uint32_t bits_changed;
> +		
Nitpicking: extra space on that line.

<snip>

Cheers.

--
Robert