Re: [PATCH v17 2/3] drivers: input: keyboard: Add mtk keypad driver

[Andy Shevchenko]

On Thu, Aug 13, 2020 at 3:57 AM Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
> On Mon, Aug 10, 2020 at 02:51:28PM +0800, Fengping Yu wrote:
> > From: "fengping.yu" <fengping.yu@xxxxxxxxxxxx>
> >
> > This patch adds matrix keypad support for Mediatek SoCs.

...

> > +static irqreturn_t kpd_irq_handler(int irq, void *dev_id)
> > +{
> > +     struct mtk_keypad *keypad = dev_id;
> > +     unsigned short *keycode = keypad->input_dev->keycode;
> > +     DECLARE_BITMAP(new_state, MTK_KPD_NUM_BITS);
> > +     DECLARE_BITMAP(change, MTK_KPD_NUM_BITS);
> > +     int bit_nr;
> > +     int pressed;
> > +     unsigned short code;
> > +
> > +     regmap_bulk_read(keypad->regmap, MTK_KPD_MEM,
> > +                     new_state, MTK_KPD_NUM_MEMS);
> > +
> > +     bitmap_xor(change, new_state, keypad->keymap_state, MTK_KPD_NUM_BITS);
> > +
> > +     for_each_set_bit(bit_nr, change, MTK_KPD_NUM_BITS) {
>
> Should we be explicit:
>
>                 if (bit_nr % 32 >= 16) // or "if ((bit_nr / 16) % 2)"
>                         continue;
>
> so that we sure we do not trip over garbage (if any) in the unused
> bits?

Shouldn't we rather rely on the fact that bitmap API explicitly takes
a bit number as an argument. What garbage are you thinking of?
If you are talking about gaps, then probably existing
for_each_set_clump8() or free size analogue (not yet in upstream,
though) should be used instead?

> > +             /* 1: not pressed, 0: pressed */
> > +             pressed = !test_bit(bit_nr, new_state);
> > +             dev_dbg(&keypad->input_dev->dev, "%s",
> > +                     pressed ? "pressed" : "released");
> > +
> > +             /* 32bit register only use low 16bit as keypad mem register */
> > +             code = keycode[bit_nr - 16 * (BITS_TO_U32(bit_nr) - 1)];
>
> This will give index of 16 for (0,0).

I was also puzzled by this in one of the review rounds, but I don't
remember what was the explanation.

> Is this what we want? Hmm, this is
> all weird... I think we need:
>
>                 row = bit_nr / 32;
>                 col = bit_nr % 32;
>                 if (col > 15)
>                         continue;
>
>                 // set row_shift in probe() as:
>                 // keypad_data->row_shift =
>                 //              get_count_order(keypad_data->n_cols);
>                 code = keycode[MATRIX_SCAN_CODE(row, col,
>                                                 keypad_data->row_shift)];
>
> This will properly unpack the keymap built by
> matrix_keypad_build_keymap().
>
> > +
> > +             input_report_key(keypad->input_dev, code, pressed);
> > +             input_sync(keypad->input_dev);
> > +
> > +             dev_dbg(&keypad->input_dev->dev,
> > +                     "report Linux keycode = %d\n", code);
> > +     }
> > +
> > +     bitmap_copy(keypad->keymap_state, new_state, MTK_KPD_NUM_BITS);
> > +
> > +     return IRQ_HANDLED;
> > +}

-- 
With Best Regards,
Andy Shevchenko