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
