[RESEND PATCH 0/2] Input: mt6779-keypad - fix hw code logic and row/col selection

[RESEND PATCH 0/2] Input: mt6779-keypad - fix hw code logic and row/col selection

[Mattijs Korpershoek]

This serie is the first follow-up on the mt6779-keypad in
order to enable it on the MediaTek mt8183-pumpkin board.

To fully enable it on mt8183-pumpkin, we still need:
* double key support
* dts changes

To ease up reviewing, I preferred sending this first.

The first patch fixes a logic bug based on the (non-public) datasheet
I have.
The second patch configures the keypad correctly in order to not
report bogus values.

Thank you,
Mattijs

Mattijs Korpershoek (2):
  Input: mt6779-keypad - fix hardware code mapping
  Input: mt6779-keypad - implement row/column selection

 drivers/input/keyboard/mt6779-keypad.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)


base-commit: b243018eafeb69bf074ef013c54504632fd161ec
-- 
2.32.0

[RESEND PATCH 1/2] Input: mt6779-keypad - fix hardware code mapping

[Mattijs Korpershoek]

In mt6779_keypad_irq_handler(), we
1. Read a hardware code from KPD_MEM1 -> KPD_MEM5
2. Use that hardware code to compute columns/rows for the standard
   keyboard matrix.

According to the (non-public) datasheet, the
map between the hardware code and the cols/rows is:

        |(0)  |(1)  |(2)
    ----*-----*-----*-----
        |     |     |
        |(9)  |(10) |(11)
    ----*-----*-----*-----
        |     |     |
        |(18) |(19) |(20)
    ----*-----*-----*-----
        |     |     |

This brings us to another formula:
-> row = code / 9;
-> col = code % 3;

Implement this mapping in bitnr_to_col_row() to fetch the
correct input event from keypad->input_dev->keycode and report that
back to userspace.

Fixes: f28af984e771 ("Input: mt6779-keypad - add MediaTek keypad driver")
Co-developed-by: Fabien Parent <fparent@baylibre.com>
Signed-off-by: Fabien Parent <fparent@baylibre.com>
Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
 drivers/input/keyboard/mt6779-keypad.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/mt6779-keypad.c b/drivers/input/keyboard/mt6779-keypad.c
index 2e7c9187c10f..23360de20da5 100644
--- a/drivers/input/keyboard/mt6779-keypad.c
+++ b/drivers/input/keyboard/mt6779-keypad.c
@@ -36,6 +36,19 @@ static const struct regmap_config mt6779_keypad_regmap_cfg = {
 	.max_register = 36,
 };
 
+/*
+ * | hardware key code | col0 | col1 | col2|
+ * | ----------------- | -----| ---- | --- |
+ * | row0              | 0    | 1    | 2   |
+ * | row1              | 9    | 10   | 11  |
+ * | row2              | 18   | 19   | 20  |
+ */
+static void bitnr_to_col_row(int bit_nr, int *col, int *row)
+{
+	*row = bit_nr / 9;
+	*col = bit_nr % 3;
+}
+
 static irqreturn_t mt6779_keypad_irq_handler(int irq, void *dev_id)
 {
 	struct mt6779_keypad *keypad = dev_id;
@@ -61,8 +74,7 @@ static irqreturn_t mt6779_keypad_irq_handler(int irq, void *dev_id)
 		if (bit_nr % 32 >= 16)
 			continue;
 
-		row = bit_nr / 32;
-		col = bit_nr % 32;
+		bitnr_to_col_row(bit_nr, &col, &row);
 		scancode = MATRIX_SCAN_CODE(row, col, row_shift);
 		/* 1: not pressed, 0: pressed */
 		pressed = !test_bit(bit_nr, new_state);
-- 
2.32.0

Re: [RESEND PATCH 1/2] Input: mt6779-keypad - fix hardware code mapping

[Dmitry Torokhov]

On Fri, May 13, 2022 at 05:18:44PM +0200, Mattijs Korpershoek wrote:
> In mt6779_keypad_irq_handler(), we
> 1. Read a hardware code from KPD_MEM1 -> KPD_MEM5
> 2. Use that hardware code to compute columns/rows for the standard
>    keyboard matrix.
> 
> According to the (non-public) datasheet, the
> map between the hardware code and the cols/rows is:
> 
>         |(0)  |(1)  |(2)
>     ----*-----*-----*-----
>         |     |     |
>         |(9)  |(10) |(11)
>     ----*-----*-----*-----
>         |     |     |
>         |(18) |(19) |(20)
>     ----*-----*-----*-----
>         |     |     |
> 
> This brings us to another formula:
> -> row = code / 9;
> -> col = code % 3;

What if there are more than 3 columns?

> 
> Implement this mapping in bitnr_to_col_row() to fetch the
> correct input event from keypad->input_dev->keycode and report that
> back to userspace.
> 
> Fixes: f28af984e771 ("Input: mt6779-keypad - add MediaTek keypad driver")
> Co-developed-by: Fabien Parent <fparent@baylibre.com>
> Signed-off-by: Fabien Parent <fparent@baylibre.com>
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
>  drivers/input/keyboard/mt6779-keypad.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/keyboard/mt6779-keypad.c b/drivers/input/keyboard/mt6779-keypad.c
> index 2e7c9187c10f..23360de20da5 100644
> --- a/drivers/input/keyboard/mt6779-keypad.c
> +++ b/drivers/input/keyboard/mt6779-keypad.c
> @@ -36,6 +36,19 @@ static const struct regmap_config mt6779_keypad_regmap_cfg = {
>  	.max_register = 36,
>  };
>  
> +/*
> + * | hardware key code | col0 | col1 | col2|
> + * | ----------------- | -----| ---- | --- |
> + * | row0              | 0    | 1    | 2   |
> + * | row1              | 9    | 10   | 11  |
> + * | row2              | 18   | 19   | 20  |
> + */
> +static void bitnr_to_col_row(int bit_nr, int *col, int *row)
> +{
> +	*row = bit_nr / 9;
> +	*col = bit_nr % 3;
> +}
> +
>  static irqreturn_t mt6779_keypad_irq_handler(int irq, void *dev_id)
>  {
>  	struct mt6779_keypad *keypad = dev_id;
> @@ -61,8 +74,7 @@ static irqreturn_t mt6779_keypad_irq_handler(int irq, void *dev_id)
>  		if (bit_nr % 32 >= 16)
>  			continue;
>  
> -		row = bit_nr / 32;
> -		col = bit_nr % 32;
> +		bitnr_to_col_row(bit_nr, &col, &row);
>  		scancode = MATRIX_SCAN_CODE(row, col, row_shift);
>  		/* 1: not pressed, 0: pressed */
>  		pressed = !test_bit(bit_nr, new_state);
> -- 
> 2.32.0
> 

-- 
Dmitry

Re: [RESEND PATCH 1/2] Input: mt6779-keypad - fix hardware code mapping

[Mattijs Korpershoek]

Hi Dmitry,

Thank you for your review,

On dim., mai 15, 2022 at 22:23, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Fri, May 13, 2022 at 05:18:44PM +0200, Mattijs Korpershoek wrote:
>> In mt6779_keypad_irq_handler(), we
>> 1. Read a hardware code from KPD_MEM1 -> KPD_MEM5
>> 2. Use that hardware code to compute columns/rows for the standard
>>    keyboard matrix.
>> 
>> According to the (non-public) datasheet, the
>> map between the hardware code and the cols/rows is:
>> 
>>         |(0)  |(1)  |(2)
>>     ----*-----*-----*-----
>>         |     |     |
>>         |(9)  |(10) |(11)
>>     ----*-----*-----*-----
>>         |     |     |
>>         |(18) |(19) |(20)
>>     ----*-----*-----*-----
>>         |     |     |
>> 
>> This brings us to another formula:
>> -> row = code / 9;
>> -> col = code % 3;
>
> What if there are more than 3 columns?
That's not supported, in hardware, according to the datasheet.

The datasheet I have states that "The interface of MT6763 only supports
3*3 single or 2*2 double, but internal ASIC still detects keys in the
manner of 8*8 single, and 3*3 double. The registers and key codes still
follows the legacy naming".

Should I add another patch in this series to add that limitation in the
probe? There are no checks done in the probe() right now.

>
>> 
>> Implement this mapping in bitnr_to_col_row() to fetch the
>> correct input event from keypad->input_dev->keycode and report that
>> back to userspace.
>> 
>> Fixes: f28af984e771 ("Input: mt6779-keypad - add MediaTek keypad driver")
>> Co-developed-by: Fabien Parent <fparent@baylibre.com>
>> Signed-off-by: Fabien Parent <fparent@baylibre.com>
>> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> ---
>>  drivers/input/keyboard/mt6779-keypad.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/input/keyboard/mt6779-keypad.c b/drivers/input/keyboard/mt6779-keypad.c
>> index 2e7c9187c10f..23360de20da5 100644
>> --- a/drivers/input/keyboard/mt6779-keypad.c
>> +++ b/drivers/input/keyboard/mt6779-keypad.c
>> @@ -36,6 +36,19 @@ static const struct regmap_config mt6779_keypad_regmap_cfg = {
>>  	.max_register = 36,
>>  };
>>  
>> +/*
>> + * | hardware key code | col0 | col1 | col2|
>> + * | ----------------- | -----| ---- | --- |
>> + * | row0              | 0    | 1    | 2   |
>> + * | row1              | 9    | 10   | 11  |
>> + * | row2              | 18   | 19   | 20  |
>> + */
>> +static void bitnr_to_col_row(int bit_nr, int *col, int *row)
>> +{
>> +	*row = bit_nr / 9;
>> +	*col = bit_nr % 3;
>> +}
>> +
>>  static irqreturn_t mt6779_keypad_irq_handler(int irq, void *dev_id)
>>  {
>>  	struct mt6779_keypad *keypad = dev_id;
>> @@ -61,8 +74,7 @@ static irqreturn_t mt6779_keypad_irq_handler(int irq, void *dev_id)
>>  		if (bit_nr % 32 >= 16)
>>  			continue;
>>  
>> -		row = bit_nr / 32;
>> -		col = bit_nr % 32;
>> +		bitnr_to_col_row(bit_nr, &col, &row);
>>  		scancode = MATRIX_SCAN_CODE(row, col, row_shift);
>>  		/* 1: not pressed, 0: pressed */
>>  		pressed = !test_bit(bit_nr, new_state);
>> -- 
>> 2.32.0
>> 
>
> -- 
> Dmitry

Re: [RESEND PATCH 1/2] Input: mt6779-keypad - fix hardware code mapping

[AngeloGioacchino Del Regno]

Il 16/05/22 09:30, Mattijs Korpershoek ha scritto:
> Hi Dmitry,
> 
> Thank you for your review,
> 
> On dim., mai 15, 2022 at 22:23, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
>> On Fri, May 13, 2022 at 05:18:44PM +0200, Mattijs Korpershoek wrote:
>>> In mt6779_keypad_irq_handler(), we
>>> 1. Read a hardware code from KPD_MEM1 -> KPD_MEM5
>>> 2. Use that hardware code to compute columns/rows for the standard
>>>     keyboard matrix.
>>>
>>> According to the (non-public) datasheet, the
>>> map between the hardware code and the cols/rows is:
>>>
>>>          |(0)  |(1)  |(2)
>>>      ----*-----*-----*-----
>>>          |     |     |
>>>          |(9)  |(10) |(11)
>>>      ----*-----*-----*-----
>>>          |     |     |
>>>          |(18) |(19) |(20)
>>>      ----*-----*-----*-----
>>>          |     |     |
>>>
>>> This brings us to another formula:
>>> -> row = code / 9;
>>> -> col = code % 3;
>>
>> What if there are more than 3 columns?
> That's not supported, in hardware, according to the datasheet.
> 
> The datasheet I have states that "The interface of MT6763 only supports
> 3*3 single or 2*2 double, but internal ASIC still detects keys in the
> manner of 8*8 single, and 3*3 double. The registers and key codes still
> follows the legacy naming".
> 
> Should I add another patch in this series to add that limitation in the
> probe? There are no checks done in the probe() right now.
> 

I've just checked a downstream kernel for MT6795 and that one looks like
being fully compatible with this driver as well... and as far as downstream
is concerned, apparently, mt6735, 6739, 6755, 6757, 6758, 6763, 6771, 6775
all have the same register layout and the downstream driver for these is
always the very same one...

...so, I don't think that there's currently any SoC that supports more than
three columns. Besides, a fast check shows that MT8195 also has the same.
At this point, I'd say that assuming that there are 3 columns, nor less, not
more, is just fine.

To stay on the safe side, though, perhaps add a comment explaining that
this driver works on that assumption? ..but that's clear, anyway, if you
actually read the code.

 From my perspective, this commit is good to go.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [RESEND PATCH 1/2] Input: mt6779-keypad - fix hardware code mapping

[Mattijs Korpershoek]

On Mon, May 16, 2022 at 13:06, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

> Il 16/05/22 09:30, Mattijs Korpershoek ha scritto:
>> Hi Dmitry,
>> 
>> Thank you for your review,
>> 
>> On dim., mai 15, 2022 at 22:23, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>> 
>>> On Fri, May 13, 2022 at 05:18:44PM +0200, Mattijs Korpershoek wrote:
>>>> In mt6779_keypad_irq_handler(), we
>>>> 1. Read a hardware code from KPD_MEM1 -> KPD_MEM5
>>>> 2. Use that hardware code to compute columns/rows for the standard
>>>>     keyboard matrix.
>>>>
>>>> According to the (non-public) datasheet, the
>>>> map between the hardware code and the cols/rows is:
>>>>
>>>>          |(0)  |(1)  |(2)
>>>>      ----*-----*-----*-----
>>>>          |     |     |
>>>>          |(9)  |(10) |(11)
>>>>      ----*-----*-----*-----
>>>>          |     |     |
>>>>          |(18) |(19) |(20)
>>>>      ----*-----*-----*-----
>>>>          |     |     |
>>>>
>>>> This brings us to another formula:
>>>> -> row = code / 9;
>>>> -> col = code % 3;
>>>
>>> What if there are more than 3 columns?
>> That's not supported, in hardware, according to the datasheet.
>> 
>> The datasheet I have states that "The interface of MT6763 only supports
>> 3*3 single or 2*2 double, but internal ASIC still detects keys in the
>> manner of 8*8 single, and 3*3 double. The registers and key codes still
>> follows the legacy naming".
>> 
>> Should I add another patch in this series to add that limitation in the
>> probe? There are no checks done in the probe() right now.
>> 
>
> I've just checked a downstream kernel for MT6795 and that one looks like
> being fully compatible with this driver as well... and as far as downstream
> is concerned, apparently, mt6735, 6739, 6755, 6757, 6758, 6763, 6771, 6775
> all have the same register layout and the downstream driver for these is
> always the very same one...
Thank you for taking the time to check in your downstream kernels, I
really appreciate it.

>
> ...so, I don't think that there's currently any SoC that supports more than
> three columns. Besides, a fast check shows that MT8195 also has the same.
> At this point, I'd say that assuming that there are 3 columns, nor less, not
> more, is just fine.
>
> To stay on the safe side, though, perhaps add a comment explaining that
> this driver works on that assumption? ..but that's clear, anyway, if you
> actually read the code.
>
>  From my perspective, this commit is good to go.
I will keep as is for v2 series and apply your review tag. thanks again !

>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [RESEND PATCH 1/2] Input: mt6779-keypad - fix hardware code mapping

[Dmitry Torokhov]

On Mon, May 16, 2022 at 01:06:43PM +0200, AngeloGioacchino Del Regno wrote:
> Il 16/05/22 09:30, Mattijs Korpershoek ha scritto:
> > Hi Dmitry,
> > 
> > Thank you for your review,
> > 
> > On dim., mai 15, 2022 at 22:23, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > 
> > > On Fri, May 13, 2022 at 05:18:44PM +0200, Mattijs Korpershoek wrote:
> > > > In mt6779_keypad_irq_handler(), we
> > > > 1. Read a hardware code from KPD_MEM1 -> KPD_MEM5
> > > > 2. Use that hardware code to compute columns/rows for the standard
> > > >     keyboard matrix.
> > > > 
> > > > According to the (non-public) datasheet, the
> > > > map between the hardware code and the cols/rows is:
> > > > 
> > > >          |(0)  |(1)  |(2)
> > > >      ----*-----*-----*-----
> > > >          |     |     |
> > > >          |(9)  |(10) |(11)
> > > >      ----*-----*-----*-----
> > > >          |     |     |
> > > >          |(18) |(19) |(20)
> > > >      ----*-----*-----*-----
> > > >          |     |     |
> > > > 
> > > > This brings us to another formula:
> > > > -> row = code / 9;
> > > > -> col = code % 3;
> > > 
> > > What if there are more than 3 columns?
> > That's not supported, in hardware, according to the datasheet.
> > 
> > The datasheet I have states that "The interface of MT6763 only supports
> > 3*3 single or 2*2 double, but internal ASIC still detects keys in the
> > manner of 8*8 single, and 3*3 double. The registers and key codes still
> > follows the legacy naming".
> > 
> > Should I add another patch in this series to add that limitation in the
> > probe? There are no checks done in the probe() right now.
> > 
> 
> I've just checked a downstream kernel for MT6795 and that one looks like
> being fully compatible with this driver as well... and as far as downstream
> is concerned, apparently, mt6735, 6739, 6755, 6757, 6758, 6763, 6771, 6775
> all have the same register layout and the downstream driver for these is
> always the very same one...
> 
> ...so, I don't think that there's currently any SoC that supports more than
> three columns. Besides, a fast check shows that MT8195 also has the same.
> At this point, I'd say that assuming that there are 3 columns, nor less, not
> more, is just fine.

OK, now that I looked at the datasheet I remember how it came about. The
programming (register) interface does not really care about how actual
matrix is organized, and instead has a set of bits representing keys,
from KEY0 to KEY77, arranged in 5 chunks of 15 bits split into 5 32-bit
registers. So we simply decided to use register number as row and
offset in the register as column when encoding our "matrix".

This does not match the actual keypad matrix organization, so if we want
to change this, that's fine, but then we also need to recognize that we
are skipping bits 16-31, 48-63, and so on, so to get to the right key
number we need to do something like:

	key = bit_nr / 32 * 16 + bit_nr % 32;
	row = key / 9;
	col = key % 9;

I looked at the datasheets I have and they talk about 8x8 single keypad
matrix, and 3x3 double keypad (with actual matrices either 3x3 or 2x2)
but I do not actually see this map layout that Mattijs drew  documented
anywhere though...  I also wonder if there are already existing DTSes in
the wild that will be rendered invalid by these changes. I wonder if it
would not be be better to document the existing meaning of row and
column in the driver?

Thanks.

-- 
Dmitry

Re: [RESEND PATCH 1/2] Input: mt6779-keypad - fix hardware code mapping

[Mattijs Korpershoek]

On dim., mai 22, 2022 at 22:42, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Mon, May 16, 2022 at 01:06:43PM +0200, AngeloGioacchino Del Regno wrote:
>> Il 16/05/22 09:30, Mattijs Korpershoek ha scritto:
>> > Hi Dmitry,
>> > 
>> > Thank you for your review,
>> > 
>> > On dim., mai 15, 2022 at 22:23, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>> > 
>> > > On Fri, May 13, 2022 at 05:18:44PM +0200, Mattijs Korpershoek wrote:
>> > > > In mt6779_keypad_irq_handler(), we
>> > > > 1. Read a hardware code from KPD_MEM1 -> KPD_MEM5
>> > > > 2. Use that hardware code to compute columns/rows for the standard
>> > > >     keyboard matrix.
>> > > > 
>> > > > According to the (non-public) datasheet, the
>> > > > map between the hardware code and the cols/rows is:
>> > > > 
>> > > >          |(0)  |(1)  |(2)
>> > > >      ----*-----*-----*-----
>> > > >          |     |     |
>> > > >          |(9)  |(10) |(11)
>> > > >      ----*-----*-----*-----
>> > > >          |     |     |
>> > > >          |(18) |(19) |(20)
>> > > >      ----*-----*-----*-----
>> > > >          |     |     |
>> > > > 
>> > > > This brings us to another formula:
>> > > > -> row = code / 9;
>> > > > -> col = code % 3;
>> > > 
>> > > What if there are more than 3 columns?
>> > That's not supported, in hardware, according to the datasheet.
>> > 
>> > The datasheet I have states that "The interface of MT6763 only supports
>> > 3*3 single or 2*2 double, but internal ASIC still detects keys in the
>> > manner of 8*8 single, and 3*3 double. The registers and key codes still
>> > follows the legacy naming".
>> > 
>> > Should I add another patch in this series to add that limitation in the
>> > probe? There are no checks done in the probe() right now.
>> > 
>> 
>> I've just checked a downstream kernel for MT6795 and that one looks like
>> being fully compatible with this driver as well... and as far as downstream
>> is concerned, apparently, mt6735, 6739, 6755, 6757, 6758, 6763, 6771, 6775
>> all have the same register layout and the downstream driver for these is
>> always the very same one...
>> 
>> ...so, I don't think that there's currently any SoC that supports more than
>> three columns. Besides, a fast check shows that MT8195 also has the same.
>> At this point, I'd say that assuming that there are 3 columns, nor less, not
>> more, is just fine.
>
> OK, now that I looked at the datasheet I remember how it came about. The
> programming (register) interface does not really care about how actual
> matrix is organized, and instead has a set of bits representing keys,
> from KEY0 to KEY77, arranged in 5 chunks of 15 bits split into 5 32-bit
> registers. So we simply decided to use register number as row and
> offset in the register as column when encoding our "matrix".

That's correct and that's a good way to phrase it.
I will add that in the commit message.

>
> This does not match the actual keypad matrix organization, so if we want
> to change this, that's fine, but then we also need to recognize that we
> are skipping bits 16-31, 48-63, and so on, so to get to the right key
> number we need to do something like:
>
> 	key = bit_nr / 32 * 16 + bit_nr % 32;
> 	row = key / 9;
> 	col = key % 9;

I would prefer to have the driver's matrix_keypad (build in probe()) to
match the actual hardware. To me this seems easier to understand for
people familiar with the hardware.

I've also tested the above snippet and it matches my expectations.

>
> I looked at the datasheets I have and they talk about 8x8 single keypad
> matrix, and 3x3 double keypad (with actual matrices either 3x3 or 2x2)

Indeed. I plan to send out double keypad support for this driver since
that's actually needed for mt8183-pumpkin as well.
It's already in our mtk-v5.10[1] integration tree but I have not submitted
it yet.
I planned to send this a separate series to avoid burdening / have
smaller chunks to review. If that was a mistake, please let me know.

> but I do not actually see this map layout that Mattijs drew  documented

The map layout that I draw is not directly copied from the datasheet.
It's a "translation" of the following table:

| hardware key code | col0 | col1 | col2|
| ----------------- | -----| ---- | --- |
| row0              | 0    | 1    | 2   |
| row1              | 9    | 10   | 11  |
| row2              | 18   | 19   | 20  |

It seems that caused more confusion than actual useful information,
sorry about that.

> anywhere though...  I also wonder if there are already existing DTSes in
> the wild that will be rendered invalid by these changes. I wonder if it
> would not be be better to document the existing meaning of row and
> column in the driver?

The concern for "DTSes in the wild" that will break is a valid point.
I'm not aware of any of those. Most vendor trees i've seen don't use
this driver at all. I hope that will change at some point.

In the end. I'd prefer to have the driver's keypad matrix match
the actual hardware. Right now we can have a 5x32 matrix which seems
absurd. Having at most an 8x8 is more reasonable.

I'd like to send v3 with just fixing the row/column suggestion in
mt6779_keypad_irq_handler() that Dmitry suggested.

Would that work Dmitry?


[1] https://gitlab.com/mediatek/aiot/bsp/linux/-/tree/mtk-v5.10

> Thanks.
>
> -- 
> Dmitry

Re: [RESEND PATCH 1/2] Input: mt6779-keypad - fix hardware code mapping

[Dmitry Torokhov]

On Tue, May 24, 2022 at 09:21:28PM +0200, Mattijs Korpershoek wrote:
> On dim., mai 22, 2022 at 22:42, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > On Mon, May 16, 2022 at 01:06:43PM +0200, AngeloGioacchino Del Regno wrote:
> >> Il 16/05/22 09:30, Mattijs Korpershoek ha scritto:
> >> > Hi Dmitry,
> >> > 
> >> > Thank you for your review,
> >> > 
> >> > On dim., mai 15, 2022 at 22:23, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >> > 
> >> > > On Fri, May 13, 2022 at 05:18:44PM +0200, Mattijs Korpershoek wrote:
> >> > > > In mt6779_keypad_irq_handler(), we
> >> > > > 1. Read a hardware code from KPD_MEM1 -> KPD_MEM5
> >> > > > 2. Use that hardware code to compute columns/rows for the standard
> >> > > >     keyboard matrix.
> >> > > > 
> >> > > > According to the (non-public) datasheet, the
> >> > > > map between the hardware code and the cols/rows is:
> >> > > > 
> >> > > >          |(0)  |(1)  |(2)
> >> > > >      ----*-----*-----*-----
> >> > > >          |     |     |
> >> > > >          |(9)  |(10) |(11)
> >> > > >      ----*-----*-----*-----
> >> > > >          |     |     |
> >> > > >          |(18) |(19) |(20)
> >> > > >      ----*-----*-----*-----
> >> > > >          |     |     |
> >> > > > 
> >> > > > This brings us to another formula:
> >> > > > -> row = code / 9;
> >> > > > -> col = code % 3;
> >> > > 
> >> > > What if there are more than 3 columns?
> >> > That's not supported, in hardware, according to the datasheet.
> >> > 
> >> > The datasheet I have states that "The interface of MT6763 only supports
> >> > 3*3 single or 2*2 double, but internal ASIC still detects keys in the
> >> > manner of 8*8 single, and 3*3 double. The registers and key codes still
> >> > follows the legacy naming".
> >> > 
> >> > Should I add another patch in this series to add that limitation in the
> >> > probe? There are no checks done in the probe() right now.
> >> > 
> >> 
> >> I've just checked a downstream kernel for MT6795 and that one looks like
> >> being fully compatible with this driver as well... and as far as downstream
> >> is concerned, apparently, mt6735, 6739, 6755, 6757, 6758, 6763, 6771, 6775
> >> all have the same register layout and the downstream driver for these is
> >> always the very same one...
> >> 
> >> ...so, I don't think that there's currently any SoC that supports more than
> >> three columns. Besides, a fast check shows that MT8195 also has the same.
> >> At this point, I'd say that assuming that there are 3 columns, nor less, not
> >> more, is just fine.
> >
> > OK, now that I looked at the datasheet I remember how it came about. The
> > programming (register) interface does not really care about how actual
> > matrix is organized, and instead has a set of bits representing keys,
> > from KEY0 to KEY77, arranged in 5 chunks of 15 bits split into 5 32-bit
> > registers. So we simply decided to use register number as row and
> > offset in the register as column when encoding our "matrix".
> 
> That's correct and that's a good way to phrase it.
> I will add that in the commit message.
> 
> >
> > This does not match the actual keypad matrix organization, so if we want
> > to change this, that's fine, but then we also need to recognize that we
> > are skipping bits 16-31, 48-63, and so on, so to get to the right key
> > number we need to do something like:
> >
> > 	key = bit_nr / 32 * 16 + bit_nr % 32;
> > 	row = key / 9;
> > 	col = key % 9;
> 
> I would prefer to have the driver's matrix_keypad (build in probe()) to
> match the actual hardware. To me this seems easier to understand for
> people familiar with the hardware.
> 
> I've also tested the above snippet and it matches my expectations.
> 
> >
> > I looked at the datasheets I have and they talk about 8x8 single keypad
> > matrix, and 3x3 double keypad (with actual matrices either 3x3 or 2x2)
> 
> Indeed. I plan to send out double keypad support for this driver since
> that's actually needed for mt8183-pumpkin as well.
> It's already in our mtk-v5.10[1] integration tree but I have not submitted
> it yet.
> I planned to send this a separate series to avoid burdening / have
> smaller chunks to review. If that was a mistake, please let me know.
> 
> > but I do not actually see this map layout that Mattijs drew  documented
> 
> The map layout that I draw is not directly copied from the datasheet.
> It's a "translation" of the following table:
> 
> | hardware key code | col0 | col1 | col2|
> | ----------------- | -----| ---- | --- |
> | row0              | 0    | 1    | 2   |
> | row1              | 9    | 10   | 11  |
> | row2              | 18   | 19   | 20  |
> 
> It seems that caused more confusion than actual useful information,
> sorry about that.
> 
> > anywhere though...  I also wonder if there are already existing DTSes in
> > the wild that will be rendered invalid by these changes. I wonder if it
> > would not be be better to document the existing meaning of row and
> > column in the driver?
> 
> The concern for "DTSes in the wild" that will break is a valid point.
> I'm not aware of any of those. Most vendor trees i've seen don't use
> this driver at all. I hope that will change at some point.
> 
> In the end. I'd prefer to have the driver's keypad matrix match
> the actual hardware. Right now we can have a 5x32 matrix which seems
> absurd. Having at most an 8x8 is more reasonable.
> 
> I'd like to send v3 with just fixing the row/column suggestion in
> mt6779_keypad_irq_handler() that Dmitry suggested.
> 
> Would that work Dmitry?

OK, let's do that. Although I'd be curious to see the double keypad
patches as according to the datasheets I saw the translation is
different for those.

Thanks.

-- 
Dmitry

Re: [RESEND PATCH 1/2] Input: mt6779-keypad - fix hardware code mapping

[Mattijs Korpershoek]

On sam., mai 28, 2022 at 22:23, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Tue, May 24, 2022 at 09:21:28PM +0200, Mattijs Korpershoek wrote:
>> On dim., mai 22, 2022 at 22:42, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>> 
>> > On Mon, May 16, 2022 at 01:06:43PM +0200, AngeloGioacchino Del Regno wrote:
>> >> Il 16/05/22 09:30, Mattijs Korpershoek ha scritto:
>> >> > Hi Dmitry,
>> >> > 
>> >> > Thank you for your review,
>> >> > 
>> >> > On dim., mai 15, 2022 at 22:23, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>> >> > 
>> >> > > On Fri, May 13, 2022 at 05:18:44PM +0200, Mattijs Korpershoek wrote:
>> >> > > > In mt6779_keypad_irq_handler(), we
>> >> > > > 1. Read a hardware code from KPD_MEM1 -> KPD_MEM5
>> >> > > > 2. Use that hardware code to compute columns/rows for the standard
>> >> > > >     keyboard matrix.
>> >> > > > 
>> >> > > > According to the (non-public) datasheet, the
>> >> > > > map between the hardware code and the cols/rows is:
>> >> > > > 
>> >> > > >          |(0)  |(1)  |(2)
>> >> > > >      ----*-----*-----*-----
>> >> > > >          |     |     |
>> >> > > >          |(9)  |(10) |(11)
>> >> > > >      ----*-----*-----*-----
>> >> > > >          |     |     |
>> >> > > >          |(18) |(19) |(20)
>> >> > > >      ----*-----*-----*-----
>> >> > > >          |     |     |
>> >> > > > 
>> >> > > > This brings us to another formula:
>> >> > > > -> row = code / 9;
>> >> > > > -> col = code % 3;
>> >> > > 
>> >> > > What if there are more than 3 columns?
>> >> > That's not supported, in hardware, according to the datasheet.
>> >> > 
>> >> > The datasheet I have states that "The interface of MT6763 only supports
>> >> > 3*3 single or 2*2 double, but internal ASIC still detects keys in the
>> >> > manner of 8*8 single, and 3*3 double. The registers and key codes still
>> >> > follows the legacy naming".
>> >> > 
>> >> > Should I add another patch in this series to add that limitation in the
>> >> > probe? There are no checks done in the probe() right now.
>> >> > 
>> >> 
>> >> I've just checked a downstream kernel for MT6795 and that one looks like
>> >> being fully compatible with this driver as well... and as far as downstream
>> >> is concerned, apparently, mt6735, 6739, 6755, 6757, 6758, 6763, 6771, 6775
>> >> all have the same register layout and the downstream driver for these is
>> >> always the very same one...
>> >> 
>> >> ...so, I don't think that there's currently any SoC that supports more than
>> >> three columns. Besides, a fast check shows that MT8195 also has the same.
>> >> At this point, I'd say that assuming that there are 3 columns, nor less, not
>> >> more, is just fine.
>> >
>> > OK, now that I looked at the datasheet I remember how it came about. The
>> > programming (register) interface does not really care about how actual
>> > matrix is organized, and instead has a set of bits representing keys,
>> > from KEY0 to KEY77, arranged in 5 chunks of 15 bits split into 5 32-bit
>> > registers. So we simply decided to use register number as row and
>> > offset in the register as column when encoding our "matrix".
>> 
>> That's correct and that's a good way to phrase it.
>> I will add that in the commit message.
>> 
>> >
>> > This does not match the actual keypad matrix organization, so if we want
>> > to change this, that's fine, but then we also need to recognize that we
>> > are skipping bits 16-31, 48-63, and so on, so to get to the right key
>> > number we need to do something like:
>> >
>> > 	key = bit_nr / 32 * 16 + bit_nr % 32;
>> > 	row = key / 9;
>> > 	col = key % 9;
>> 
>> I would prefer to have the driver's matrix_keypad (build in probe()) to
>> match the actual hardware. To me this seems easier to understand for
>> people familiar with the hardware.
>> 
>> I've also tested the above snippet and it matches my expectations.
>> 
>> >
>> > I looked at the datasheets I have and they talk about 8x8 single keypad
>> > matrix, and 3x3 double keypad (with actual matrices either 3x3 or 2x2)
>> 
>> Indeed. I plan to send out double keypad support for this driver since
>> that's actually needed for mt8183-pumpkin as well.
>> It's already in our mtk-v5.10[1] integration tree but I have not submitted
>> it yet.
>> I planned to send this a separate series to avoid burdening / have
>> smaller chunks to review. If that was a mistake, please let me know.
>> 
>> > but I do not actually see this map layout that Mattijs drew  documented
>> 
>> The map layout that I draw is not directly copied from the datasheet.
>> It's a "translation" of the following table:
>> 
>> | hardware key code | col0 | col1 | col2|
>> | ----------------- | -----| ---- | --- |
>> | row0              | 0    | 1    | 2   |
>> | row1              | 9    | 10   | 11  |
>> | row2              | 18   | 19   | 20  |
>> 
>> It seems that caused more confusion than actual useful information,
>> sorry about that.
>> 
>> > anywhere though...  I also wonder if there are already existing DTSes in
>> > the wild that will be rendered invalid by these changes. I wonder if it
>> > would not be be better to document the existing meaning of row and
>> > column in the driver?
>> 
>> The concern for "DTSes in the wild" that will break is a valid point.
>> I'm not aware of any of those. Most vendor trees i've seen don't use
>> this driver at all. I hope that will change at some point.
>> 
>> In the end. I'd prefer to have the driver's keypad matrix match
>> the actual hardware. Right now we can have a 5x32 matrix which seems
>> absurd. Having at most an 8x8 is more reasonable.
>> 
>> I'd like to send v3 with just fixing the row/column suggestion in
>> mt6779_keypad_irq_handler() that Dmitry suggested.
>> 
>> Would that work Dmitry?
>
> OK, let's do that. Although I'd be curious to see the double keypad
> patches as according to the datasheets I saw the translation is
> different for those.

Sorry for the delay. I had some long needed away time from my computer.

Yes, it's a different translation for double keypad.

I will send a v3 to fix single keys and make sure to send the double
keypad support afterwards.


>
> Thanks.
>
> -- 
> Dmitry

[RESEND PATCH 2/2] Input: mt6779-keypad - implement row/column selection

[Mattijs Korpershoek]

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)))
+#define MTK_KPD_SEL_ROWMASK(r)	(MTK_KPD_SEL_ROW >> (6 - (r)))
 #define MTK_KPD_NUM_MEMS	5
 #define MTK_KPD_NUM_BITS	136	/* 4*32+8 MEM5 only use 8 BITS */
 
@@ -171,6 +176,11 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
 	regmap_write(keypad->regmap, MTK_KPD_DEBOUNCE,
 		     (debounce * (1 << 5)) & MTK_KPD_DEBOUNCE_MASK);
 
+	regmap_update_bits(keypad->regmap, MTK_KPD_SEL, MTK_KPD_SEL_ROW,
+			   MTK_KPD_SEL_ROWMASK(keypad->n_rows));
+	regmap_update_bits(keypad->regmap, MTK_KPD_SEL, MTK_KPD_SEL_COL,
+			   MTK_KPD_SEL_COLMASK(keypad->n_cols));
+
 	keypad->clk = devm_clk_get(&pdev->dev, "kpd");
 	if (IS_ERR(keypad->clk))
 		return PTR_ERR(keypad->clk);
-- 
2.32.0

Re: [RESEND PATCH 2/2] Input: mt6779-keypad - implement row/column selection

[Dmitry Torokhov]

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)

?

Thanks.

-- 
Dmitry

Re: [RESEND PATCH 2/2] Input: mt6779-keypad - implement row/column selection

[Mattijs Korpershoek]

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