Message ID | 20220720-mt8183-keypad-v1-0-ef9fc29dbff4@baylibre.com |
---|---|
Headers | show |
Series | Input: mt6779-keypad - double keys support | expand |
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, >
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
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>
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
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
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
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,