mbox series

[v1,0/6] Input: mt6779-keypad - double keys support

Message ID 20220720-mt8183-keypad-v1-0-ef9fc29dbff4@baylibre.com
Headers show
Series Input: mt6779-keypad - double keys support | expand

Message

Mattijs Korpershoek July 20, 2022, 2:48 p.m. UTC 
The MediaTek keypad controller has multiple operating modes:
* single key detection (currently implemented)
* double key detection

With double key detection, each (row,column) is a group that can detect
two keys in the key matrix.
This minimizes the overall pin counts for cost reduction.
However, pressing multiple keys in the same group will not be
detected properly.

On some boards, like mt8183-pumpkin, double key detection is used.

Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

---
Fabien Parent (2):
      arm64: dts: mediatek: mt8183: add keyboard node
      arm64: dts: mediatek: mt8183-pumpkin: add keypad support

Mattijs Korpershoek (4):
      MAINTAINERS: input: add mattijs for mt6779-keypad
      dt-bindings: mediatek,mt6779-keypad: use unevaluatedProperties
      dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys
      Input: mt6779-keypad - support double keys matrix

 .../bindings/input/mediatek,mt6779-keypad.yaml      |  8 +++++++-
 MAINTAINERS                                         |  6 ++++++
 arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dts     | 21 +++++++++++++++++++++
 arch/arm64/boot/dts/mediatek/mt8183.dtsi            |  9 +++++++++
 drivers/input/keyboard/mt6779-keypad.c              | 17 +++++++++++++++--
 5 files changed, 58 insertions(+), 3 deletions(-)
---
base-commit: 3b87ed7ea4d598c81a03317a92dfbd59102224fd
change-id: 20220720-mt8183-keypad-20aa77106ff0

Best regards,

Comments

Matthias Brugger July 20, 2022, 2:53 p.m. UTC | #1 
On 20/07/2022 16:48, Mattijs Korpershoek wrote:
> MediaTek keypad has 2 modes of detecting key events:
> - single key: each (row, column) can detect one key
> - double key: each (row, column) is a group of 2 keys
> 
> Double key support exists to minimize cost, since it reduces the number
> of pins required for physical keys.
> 
> Double key is configured by setting BIT(0) of the KP_SEL register.
> 
> Enable double key matrix support based on the mediatek,double-keys
> device tree property.
> 
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> 
> diff --git a/drivers/input/keyboard/mt6779-keypad.c b/drivers/input/keyboard/mt6779-keypad.c
> index bf447bf598fb..9a5dbd415dac 100644
> --- a/drivers/input/keyboard/mt6779-keypad.c
> +++ b/drivers/input/keyboard/mt6779-keypad.c
> @@ -18,6 +18,7 @@
>   #define MTK_KPD_DEBOUNCE_MASK	GENMASK(13, 0)
>   #define MTK_KPD_DEBOUNCE_MAX_MS	256
>   #define MTK_KPD_SEL		0x0020
> +#define MTK_KPD_SEL_DOUBLE_KP_MODE	BIT(0)
>   #define MTK_KPD_SEL_COL	GENMASK(15, 10)
>   #define MTK_KPD_SEL_ROW	GENMASK(9, 4)
>   #define MTK_KPD_SEL_COLMASK(c)	GENMASK((c) + 9, 10)
> @@ -31,6 +32,7 @@ struct mt6779_keypad {
>   	struct clk *clk;
>   	u32 n_rows;
>   	u32 n_cols;
> +	bool double_keys;
>   	DECLARE_BITMAP(keymap_state, MTK_KPD_NUM_BITS);
>   };
>   
> @@ -67,8 +69,13 @@ static irqreturn_t mt6779_keypad_irq_handler(int irq, void *dev_id)
>   			continue;
>   
>   		key = bit_nr / 32 * 16 + bit_nr % 32;
> -		row = key / 9;
> -		col = key % 9;
> +		if (keypad->double_keys) {
> +			row = key / 13;
> +			col = (key % 13) / 2;
> +		} else {
> +			row = key / 9;
> +			col = key % 9;
> +		}
>   
>   		scancode = MATRIX_SCAN_CODE(row, col, row_shift);
>   		/* 1: not pressed, 0: pressed */
> @@ -150,6 +157,8 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
>   
>   	wakeup = device_property_read_bool(&pdev->dev, "wakeup-source");
>   
> +	keypad->double_keys = device_property_read_bool(&pdev->dev, "mediatek,double-keys");
> +
>   	dev_dbg(&pdev->dev, "n_row=%d n_col=%d debounce=%d\n",
>   		keypad->n_rows, keypad->n_cols, debounce);
>   
> @@ -166,6 +175,10 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
>   	regmap_write(keypad->regmap, MTK_KPD_DEBOUNCE,
>   		     (debounce * (1 << 5)) & MTK_KPD_DEBOUNCE_MASK);
>   
> +	if (keypad->double_keys)
> +		regmap_update_bits(keypad->regmap, MTK_KPD_SEL,
> +				   MTK_KPD_SEL_DOUBLE_KP_MODE, MTK_KPD_SEL_DOUBLE_KP_MODE);
> +
>   	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,
>
AngeloGioacchino Del Regno July 21, 2022, 8:34 a.m. UTC | #2 
Il 20/07/22 16:48, Mattijs Korpershoek ha scritto:
> MediaTek keypad has 2 modes of detecting key events:
> - single key: each (row, column) can detect one key
> - double key: each (row, column) is a group of 2 keys
> 
> Double key support exists to minimize cost, since it reduces the number
> of pins required for physical keys.
> 
> Double key is configured by setting BIT(0) of the KP_SEL register.
> 
> Enable double key matrix support based on the mediatek,double-keys
> device tree property.
> 
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> 
> diff --git a/drivers/input/keyboard/mt6779-keypad.c b/drivers/input/keyboard/mt6779-keypad.c
> index bf447bf598fb..9a5dbd415dac 100644
> --- a/drivers/input/keyboard/mt6779-keypad.c
> +++ b/drivers/input/keyboard/mt6779-keypad.c
> @@ -18,6 +18,7 @@
>   #define MTK_KPD_DEBOUNCE_MASK	GENMASK(13, 0)
>   #define MTK_KPD_DEBOUNCE_MAX_MS	256
>   #define MTK_KPD_SEL		0x0020
> +#define MTK_KPD_SEL_DOUBLE_KP_MODE	BIT(0)
>   #define MTK_KPD_SEL_COL	GENMASK(15, 10)
>   #define MTK_KPD_SEL_ROW	GENMASK(9, 4)
>   #define MTK_KPD_SEL_COLMASK(c)	GENMASK((c) + 9, 10)
> @@ -31,6 +32,7 @@ struct mt6779_keypad {
>   	struct clk *clk;
>   	u32 n_rows;
>   	u32 n_cols;
> +	bool double_keys;
>   	DECLARE_BITMAP(keymap_state, MTK_KPD_NUM_BITS);
>   };
>   
> @@ -67,8 +69,13 @@ static irqreturn_t mt6779_keypad_irq_handler(int irq, void *dev_id)
>   			continue;
>   
>   		key = bit_nr / 32 * 16 + bit_nr % 32;
> -		row = key / 9;
> -		col = key % 9;
> +		if (keypad->double_keys) {
> +			row = key / 13;
> +			col = (key % 13) / 2;
> +		} else {
> +			row = key / 9;
> +			col = key % 9;
> +		}

I don't fully like this if branch permanently evaluating true or false, as no
runtime can actually change this result...

In practice, it's fine, but I was wondering if anyone would disagree with the
following proposal...

struct mt6779_keypad {
	.......
	void (*calc_row_col)(unsigned int *row, unsigned int *col);
};

In mt6779_keypad_irq_handler:

	key = bit_nr / 32 * 16 + bit_nr % 32;
	keypad->calc_row_col(&row, &col);

and below...

>   
>   		scancode = MATRIX_SCAN_CODE(row, col, row_shift);
>   		/* 1: not pressed, 0: pressed */
> @@ -150,6 +157,8 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
>   
>   	wakeup = device_property_read_bool(&pdev->dev, "wakeup-source");
>   
> +	keypad->double_keys = device_property_read_bool(&pdev->dev, "mediatek,double-keys");
> +
>   	dev_dbg(&pdev->dev, "n_row=%d n_col=%d debounce=%d\n",
>   		keypad->n_rows, keypad->n_cols, debounce);
>   
> @@ -166,6 +175,10 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
>   	regmap_write(keypad->regmap, MTK_KPD_DEBOUNCE,
>   		     (debounce * (1 << 5)) & MTK_KPD_DEBOUNCE_MASK);
>   
> +	if (keypad->double_keys)

		keypad->calc_row_col = mt6779_keypad_calc_row_col_double_kp;

> +		regmap_update_bits(keypad->regmap, MTK_KPD_SEL,
> +				   MTK_KPD_SEL_DOUBLE_KP_MODE, MTK_KPD_SEL_DOUBLE_KP_MODE);
> +

	} else {
		keypad->calc_row_col = mt6779_keypad_calc_row_col_single_kp;
	}

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

what do you think?

Cheers,
Angelo
AngeloGioacchino Del Regno July 21, 2022, 8:41 a.m. UTC | #3 
Il 20/07/22 16:48, Mattijs Korpershoek ha scritto:
> From: Fabien Parent <fparent@baylibre.com>
> 
> MT8183 has an on-SoC keyboard controller commonly used for volume
> up/down buttons.
> 
> List it in the SoC dts so that boards can enable/use it.
> 
> Signed-off-by: Fabien Parent <fparent@baylibre.com>
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Mattijs Korpershoek July 21, 2022, 2:51 p.m. UTC | #4 
On Thu, Jul 21, 2022 at 10:34, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

> Il 20/07/22 16:48, Mattijs Korpershoek ha scritto:
>> MediaTek keypad has 2 modes of detecting key events:
>> - single key: each (row, column) can detect one key
>> - double key: each (row, column) is a group of 2 keys
>> 
>> Double key support exists to minimize cost, since it reduces the number
>> of pins required for physical keys.
>> 
>> Double key is configured by setting BIT(0) of the KP_SEL register.
>> 
>> Enable double key matrix support based on the mediatek,double-keys
>> device tree property.
>> 
>> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
>> 
>> diff --git a/drivers/input/keyboard/mt6779-keypad.c b/drivers/input/keyboard/mt6779-keypad.c
>> index bf447bf598fb..9a5dbd415dac 100644
>> --- a/drivers/input/keyboard/mt6779-keypad.c
>> +++ b/drivers/input/keyboard/mt6779-keypad.c
>> @@ -18,6 +18,7 @@
>>   #define MTK_KPD_DEBOUNCE_MASK	GENMASK(13, 0)
>>   #define MTK_KPD_DEBOUNCE_MAX_MS	256
>>   #define MTK_KPD_SEL		0x0020
>> +#define MTK_KPD_SEL_DOUBLE_KP_MODE	BIT(0)
>>   #define MTK_KPD_SEL_COL	GENMASK(15, 10)
>>   #define MTK_KPD_SEL_ROW	GENMASK(9, 4)
>>   #define MTK_KPD_SEL_COLMASK(c)	GENMASK((c) + 9, 10)
>> @@ -31,6 +32,7 @@ struct mt6779_keypad {
>>   	struct clk *clk;
>>   	u32 n_rows;
>>   	u32 n_cols;
>> +	bool double_keys;
>>   	DECLARE_BITMAP(keymap_state, MTK_KPD_NUM_BITS);
>>   };
>>   
>> @@ -67,8 +69,13 @@ static irqreturn_t mt6779_keypad_irq_handler(int irq, void *dev_id)
>>   			continue;
>>   
>>   		key = bit_nr / 32 * 16 + bit_nr % 32;
>> -		row = key / 9;
>> -		col = key % 9;
>> +		if (keypad->double_keys) {
>> +			row = key / 13;
>> +			col = (key % 13) / 2;
>> +		} else {
>> +			row = key / 9;
>> +			col = key % 9;
>> +		}
>
> I don't fully like this if branch permanently evaluating true or false, as no
> runtime can actually change this result...
>
> In practice, it's fine, but I was wondering if anyone would disagree with the
> following proposal...
>
> struct mt6779_keypad {
> 	.......
> 	void (*calc_row_col)(unsigned int *row, unsigned int *col);
> };
>
> In mt6779_keypad_irq_handler:
>
> 	key = bit_nr / 32 * 16 + bit_nr % 32;
> 	keypad->calc_row_col(&row, &col);
>
> and below...
>
>>   
>>   		scancode = MATRIX_SCAN_CODE(row, col, row_shift);
>>   		/* 1: not pressed, 0: pressed */
>> @@ -150,6 +157,8 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
>>   
>>   	wakeup = device_property_read_bool(&pdev->dev, "wakeup-source");
>>   
>> +	keypad->double_keys = device_property_read_bool(&pdev->dev, "mediatek,double-keys");
>> +
>>   	dev_dbg(&pdev->dev, "n_row=%d n_col=%d debounce=%d\n",
>>   		keypad->n_rows, keypad->n_cols, debounce);
>>   
>> @@ -166,6 +175,10 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
>>   	regmap_write(keypad->regmap, MTK_KPD_DEBOUNCE,
>>   		     (debounce * (1 << 5)) & MTK_KPD_DEBOUNCE_MASK);
>>   
>> +	if (keypad->double_keys)
>
> 		keypad->calc_row_col = mt6779_keypad_calc_row_col_double_kp;
>
>> +		regmap_update_bits(keypad->regmap, MTK_KPD_SEL,
>> +				   MTK_KPD_SEL_DOUBLE_KP_MODE, MTK_KPD_SEL_DOUBLE_KP_MODE);
>> +
>
> 	} else {
> 		keypad->calc_row_col = mt6779_keypad_calc_row_col_single_kp;
> 	}
>
>>   	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,
>
> what do you think?

Hi Angelo,

Thank you for your detailed suggestion. I like it and since I have to
resend a v2 anyways, I will consider implementing it.
On the other hand, I'm a little reluctant because it means that I'll
have to remove Matthias's reviewed-by :(

>
> Cheers,
> Angelo
AngeloGioacchino Del Regno July 21, 2022, 2:55 p.m. UTC | #5 
Il 21/07/22 16:51, Mattijs Korpershoek ha scritto:
> On Thu, Jul 21, 2022 at 10:34, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:
> 
>> Il 20/07/22 16:48, Mattijs Korpershoek ha scritto:
>>> MediaTek keypad has 2 modes of detecting key events:
>>> - single key: each (row, column) can detect one key
>>> - double key: each (row, column) is a group of 2 keys
>>>
>>> Double key support exists to minimize cost, since it reduces the number
>>> of pins required for physical keys.
>>>
>>> Double key is configured by setting BIT(0) of the KP_SEL register.
>>>
>>> Enable double key matrix support based on the mediatek,double-keys
>>> device tree property.
>>>
>>> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>>> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
>>>
>>> diff --git a/drivers/input/keyboard/mt6779-keypad.c b/drivers/input/keyboard/mt6779-keypad.c
>>> index bf447bf598fb..9a5dbd415dac 100644
>>> --- a/drivers/input/keyboard/mt6779-keypad.c
>>> +++ b/drivers/input/keyboard/mt6779-keypad.c
>>> @@ -18,6 +18,7 @@
>>>    #define MTK_KPD_DEBOUNCE_MASK	GENMASK(13, 0)
>>>    #define MTK_KPD_DEBOUNCE_MAX_MS	256
>>>    #define MTK_KPD_SEL		0x0020
>>> +#define MTK_KPD_SEL_DOUBLE_KP_MODE	BIT(0)
>>>    #define MTK_KPD_SEL_COL	GENMASK(15, 10)
>>>    #define MTK_KPD_SEL_ROW	GENMASK(9, 4)
>>>    #define MTK_KPD_SEL_COLMASK(c)	GENMASK((c) + 9, 10)
>>> @@ -31,6 +32,7 @@ struct mt6779_keypad {
>>>    	struct clk *clk;
>>>    	u32 n_rows;
>>>    	u32 n_cols;
>>> +	bool double_keys;
>>>    	DECLARE_BITMAP(keymap_state, MTK_KPD_NUM_BITS);
>>>    };
>>>    
>>> @@ -67,8 +69,13 @@ static irqreturn_t mt6779_keypad_irq_handler(int irq, void *dev_id)
>>>    			continue;
>>>    
>>>    		key = bit_nr / 32 * 16 + bit_nr % 32;
>>> -		row = key / 9;
>>> -		col = key % 9;
>>> +		if (keypad->double_keys) {
>>> +			row = key / 13;
>>> +			col = (key % 13) / 2;
>>> +		} else {
>>> +			row = key / 9;
>>> +			col = key % 9;
>>> +		}
>>
>> I don't fully like this if branch permanently evaluating true or false, as no
>> runtime can actually change this result...
>>
>> In practice, it's fine, but I was wondering if anyone would disagree with the
>> following proposal...
>>
>> struct mt6779_keypad {
>> 	.......
>> 	void (*calc_row_col)(unsigned int *row, unsigned int *col);
>> };
>>
>> In mt6779_keypad_irq_handler:
>>
>> 	key = bit_nr / 32 * 16 + bit_nr % 32;
>> 	keypad->calc_row_col(&row, &col);
>>
>> and below...
>>
>>>    
>>>    		scancode = MATRIX_SCAN_CODE(row, col, row_shift);
>>>    		/* 1: not pressed, 0: pressed */
>>> @@ -150,6 +157,8 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
>>>    
>>>    	wakeup = device_property_read_bool(&pdev->dev, "wakeup-source");
>>>    
>>> +	keypad->double_keys = device_property_read_bool(&pdev->dev, "mediatek,double-keys");
>>> +
>>>    	dev_dbg(&pdev->dev, "n_row=%d n_col=%d debounce=%d\n",
>>>    		keypad->n_rows, keypad->n_cols, debounce);
>>>    
>>> @@ -166,6 +175,10 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
>>>    	regmap_write(keypad->regmap, MTK_KPD_DEBOUNCE,
>>>    		     (debounce * (1 << 5)) & MTK_KPD_DEBOUNCE_MASK);
>>>    
>>> +	if (keypad->double_keys)
>>
>> 		keypad->calc_row_col = mt6779_keypad_calc_row_col_double_kp;
>>
>>> +		regmap_update_bits(keypad->regmap, MTK_KPD_SEL,
>>> +				   MTK_KPD_SEL_DOUBLE_KP_MODE, MTK_KPD_SEL_DOUBLE_KP_MODE);
>>> +
>>
>> 	} else {
>> 		keypad->calc_row_col = mt6779_keypad_calc_row_col_single_kp;
>> 	}
>>
>>>    	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,
>>
>> what do you think?
> 
> Hi Angelo,
> 
> Thank you for your detailed suggestion. I like it and since I have to
> resend a v2 anyways, I will consider implementing it.
> On the other hand, I'm a little reluctant because it means that I'll
> have to remove Matthias's reviewed-by :(
> 

Yes, you will have to. In that case:

Matthias, any considerations about this idea? :)))

>>
>> Cheers,
>> Angelo
Mattijs Korpershoek July 26, 2022, 9:52 a.m. UTC | #6 
On Thu, Jul 21, 2022 at 16:55, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:
[...]

> Il 21/07/22 16:51, Mattijs Korpershoek ha scritto:
>> 
>> Hi Angelo,
>> 
>> Thank you for your detailed suggestion. I like it and since I have to
>> resend a v2 anyways, I will consider implementing it.
>> On the other hand, I'm a little reluctant because it means that I'll
>> have to remove Matthias's reviewed-by :(
>> 
>
> Yes, you will have to. In that case:
>
> Matthias, any considerations about this idea? :)))

Since the binding document changed, I have to rework this patch anyways.
So I will drop Matthias's reviewed-by in v2.

>
>>>
>>> Cheers,
>>> Angelo