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

On Thu, Aug 13, 2020 at 3:57 AM Dmitry Torokhov
[off-list ref] wrote:

On Mon, Aug 10, 2020 at 02:51:28PM +0800, Fengping Yu wrote:

quoted

From: "fengping.yu" <redacted>

This patch adds matrix keypad support for Mediatek SoCs.

...

quoted

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

quoted

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

quoted

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