On Mon, Jun 1, 2009 at 1:39 AM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> 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 ...
>

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