Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Mattijs Korpershoek <mkorpershoek@baylibre.com>
To:     Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc:     Matthias Brugger <matthias.bgg@gmail.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Kevin Hilman <khilman@baylibre.com>,
        Fabien Parent <fparent@baylibre.com>,
        linux-input@vger.kernel.org, linux-mediatek@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [RESEND PATCH 2/2] Input: mt6779-keypad - implement row/column
 selection
In-Reply-To: <YoHfUYmq49V5Bygd@google.com>
References: <20220513151845.2802795-1-mkorpershoek@baylibre.com>
 <20220513151845.2802795-3-mkorpershoek@baylibre.com>
 <YoHfUYmq49V5Bygd@google.com>
Date:   Mon, 16 May 2022 09:30:49 +0200
Message-ID: <875ym6kk7q.fsf@baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Hi Dmitry,

Thank you for your review,

On dim., mai 15, 2022 at 22:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> 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@baylibre.com>
>> ---
>>  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