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

On Mon, Jun 1, 2009 at 1:39 AM, Robert Jarzmik [off-list ref] wrote:

Eric Miao [off-list ref] writes:

Hi Eric, some minor cosmetic points ...

<snip>

quoted

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

This is AFAIK not necessary. This originally came from the
{corgi,spitz}kbd.c, where it is believed a small delay after
de-activating all columns, and a delay after activating one
specific column are needed (though the delay is suggested
in the {corgi,spitz}kbd.c as 10us). I'll check if it still works
without this delay (I believe it still works).

<snip>

quoted

+/*
+ * 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.

OK.

I've attached the updated one as attachment here.