Hi Dmitry, Thank you for your review, On dim., mai 15, 2022 at 22:21, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote: > On Fri, May 13, 2022 at 05:18:45PM +0200, Mattijs Korpershoek wrote: >> 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 <mkorpershoek@xxxxxxxxxxxx> >> --- >> 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
