Hi Dmitry,

Thank you for your review,

On dim., mai 15, 2022 at 22:21, Dmitry Torokhov [off-list ref] wrote:

On Fri, May 13, 2022 at 05:18:45PM +0200, Mattijs Korpershoek wrote:

quoted

The MediaTek keypad has a total of 6 input rows and 6 input columns.
By default, rows/columns 0-2 are enabled.

This is controlled by the KP_SEL register:
- bits[9:4]   control row selection
- bits[15:10] control column selection

Each bit enables the corresponding row/column number (e.g KP_SEL[4]
enables ROW0)

Depending on how the keypad is wired, this may result in wrong readings
of the keypad state.

Program the KP_SEL register to limit the key detection to n_rows,
n_cols we retrieve from the device tree.

Signed-off-by: Mattijs Korpershoek <redacted>
---
 drivers/input/keyboard/mt6779-keypad.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/input/keyboard/mt6779-keypad.c b/drivers/input/keyboard/mt6779-keypad.c
index 23360de20da5..653dfc619696 100644
--- a/drivers/input/keyboard/mt6779-keypad.c
+++ b/drivers/input/keyboard/mt6779-keypad.c

@@ -17,6 +17,11 @@
 #define MTK_KPD_DEBOUNCE	0x0018
 #define MTK_KPD_DEBOUNCE_MASK	GENMASK(13, 0)
 #define MTK_KPD_DEBOUNCE_MAX_MS	256
+#define MTK_KPD_SEL		0x0020
+#define MTK_KPD_SEL_COL	GENMASK(15, 10)
+#define MTK_KPD_SEL_ROW	GENMASK(9, 4)
+#define MTK_KPD_SEL_COLMASK(c)	(MTK_KPD_SEL_COL >> (6 - (c)))

Would it be clearer to say

#define MTK_KPD_SEL_COLMASK(c)	GENMASK((c) + 3, 4)

Per my understanding, GENMASK is mostly used for build-time constants
since it does compile-time checks with BUILD_BUG_ON_ZERO.

Since "c" is defined at run time (as it comes from keypad->n_cols, which
comes from the device tree), it felt more appropriate to not use GENMASK.

Looking again, i've found examples in sound/soc/codecs/cs35l41-lib.c
that use GENMASK with a run-time value.

Will send a v2 using GENMASK, thank you for the suggestion.

?

Thanks.

-- 
Dmitry