diff mbox series

[3/5] Input: pinephone-keyboard - Build in the default keymap

Message ID 20220129230043.12422-4-samuel@sholland.org
State New
Headers show
Series Pine64 PinePhone keyboard support | expand

Commit Message

Samuel Holland Jan. 29, 2022, 11 p.m. UTC 
The PinePhone keyboard comes with removable keys, but there is a default
layout labeled from the factory. Use this keymap if none is provided in
the devicetree.

Suggested-by: Ondrej Jirman <x@xff.cz>
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/input/keyboard/pinephone-keyboard.c | 128 +++++++++++++++++++-
 1 file changed, 123 insertions(+), 5 deletions(-)

Comments

Dmitry Torokhov Jan. 31, 2022, 7:45 p.m. UTC | #1 
Hi Samuel,

On Sat, Jan 29, 2022 at 05:00:40PM -0600, Samuel Holland wrote:
> The PinePhone keyboard comes with removable keys, but there is a default
> layout labeled from the factory. Use this keymap if none is provided in
> the devicetree.

Why can't we require to have it in device tree?

Thanks.
Samuel Holland Feb. 2, 2022, 4:58 a.m. UTC | #2 
Hi,

On 1/31/22 1:45 PM, Dmitry Torokhov wrote:
> Hi Samuel,
> 
> On Sat, Jan 29, 2022 at 05:00:40PM -0600, Samuel Holland wrote:
>> The PinePhone keyboard comes with removable keys, but there is a default
>> layout labeled from the factory. Use this keymap if none is provided in
>> the devicetree.
> 
> Why can't we require to have it in device tree?

We can. I am okay with dropping this patch and making the properties required if
that is preferred.

The keyboard is supported on at least four device trees (three revisions of
PinePhone, plus the PinePhone Pro), so moving the default keymap to the driver
avoids duplicating that block of data in each device tree/overlay.

Regards,
Samuel
Jarrah April 12, 2022, 10:20 a.m. UTC | #3 
On 1/30/22 10:00, Samuel Holland wrote:
> +
> +static const uint32_t ppkb_default_fn_keymap[] = {
> +	KEY(0,  0, KEY_FN_ESC),
> +	KEY(0,  1, KEY_F1),
> +	KEY(0,  2, KEY_F2),
> +	KEY(0,  3, KEY_F3),
> +	KEY(0,  4, KEY_F4),
> +	KEY(0,  5, KEY_F5),
> +	KEY(0,  6, KEY_F6),
> +	KEY(0,  7, KEY_F7),
> +	KEY(0,  8, KEY_F8),
> +	KEY(0,  9, KEY_F9),
> +	KEY(0, 10, KEY_F10),
> +	KEY(0, 11, KEY_DELETE),
> +
> +	KEY(2,  0, KEY_SYSRQ),
> +	KEY(2, 10, KEY_INSERT),
> +


The driver currently being patched into most distros supporting the 
keyboard exposes the symbols printed on the keyboard rather than the F* 
keys on the function layer. While I agree than exposing function keys on 
the Fn layer is more logical, in practice running this patch for a day 
I've found it's far more useful to have quick access to the standard set 
of symbols (such as | and -) than to have the function keys.

Would it be possible to either set the default back to symbols or expose 
another layer (potentially under the "pine" key)? An alternative 
solution proposed on the Mobian issue for this was to add a module 
option, allowing these to be switched at runtime rather than compile time.
Pavel Machek May 30, 2022, 7:05 a.m. UTC | #4 
Hi!

> > On Sat, Jan 29, 2022 at 05:00:40PM -0600, Samuel Holland wrote:
> >> The PinePhone keyboard comes with removable keys, but there is a default
> >> layout labeled from the factory. Use this keymap if none is provided in
> >> the devicetree.
> > 
> > Why can't we require to have it in device tree?
> 
> We can. I am okay with dropping this patch and making the properties required if
> that is preferred.
> 
> The keyboard is supported on at least four device trees (three revisions of
> PinePhone, plus the PinePhone Pro), so moving the default keymap to the driver
> avoids duplicating that block of data in each device tree/overlay.

#include is supported on dts, so there's no need to duplicate the keymaps, etc.

Best regards,
									Pavel
diff mbox series

Patch

diff --git a/drivers/input/keyboard/pinephone-keyboard.c b/drivers/input/keyboard/pinephone-keyboard.c
index 9a071753fd91..8065bc3e101a 100644
--- a/drivers/input/keyboard/pinephone-keyboard.c
+++ b/drivers/input/keyboard/pinephone-keyboard.c
@@ -24,6 +24,113 @@ 
 #define PPKB_SYS_CONFIG			0x20
 #define PPKB_SYS_CONFIG_DISABLE_SCAN		BIT(0)
 
+#define PPKB_DEFAULT_KEYMAP_ROWS	6
+#define PPKB_DEFAULT_KEYMAP_COLS	12
+
+static const uint32_t ppkb_default_fn_keymap[] = {
+	KEY(0,  0, KEY_FN_ESC),
+	KEY(0,  1, KEY_F1),
+	KEY(0,  2, KEY_F2),
+	KEY(0,  3, KEY_F3),
+	KEY(0,  4, KEY_F4),
+	KEY(0,  5, KEY_F5),
+	KEY(0,  6, KEY_F6),
+	KEY(0,  7, KEY_F7),
+	KEY(0,  8, KEY_F8),
+	KEY(0,  9, KEY_F9),
+	KEY(0, 10, KEY_F10),
+	KEY(0, 11, KEY_DELETE),
+
+	KEY(2,  0, KEY_SYSRQ),
+	KEY(2, 10, KEY_INSERT),
+
+	KEY(3,  0, KEY_LEFTSHIFT),
+	KEY(3,  8, KEY_HOME),
+	KEY(3,  9, KEY_UP),
+	KEY(3, 10, KEY_END),
+
+	KEY(4, 1, KEY_LEFTCTRL),
+	KEY(4, 6, KEY_LEFT),
+	KEY(4, 8, KEY_RIGHT),
+	KEY(4, 9, KEY_DOWN),
+
+	KEY(5, 2, KEY_FN),
+	KEY(5, 3, KEY_LEFTALT),
+	KEY(5, 5, KEY_RIGHTALT),
+};
+
+static const struct matrix_keymap_data ppkb_default_fn_keymap_data = {
+	.keymap		= ppkb_default_fn_keymap,
+	.keymap_size	= ARRAY_SIZE(ppkb_default_fn_keymap),
+};
+
+static const uint32_t ppkb_default_keymap[] = {
+	KEY(0,  0, KEY_ESC),
+	KEY(0,  1, KEY_1),
+	KEY(0,  2, KEY_2),
+	KEY(0,  3, KEY_3),
+	KEY(0,  4, KEY_4),
+	KEY(0,  5, KEY_5),
+	KEY(0,  6, KEY_6),
+	KEY(0,  7, KEY_7),
+	KEY(0,  8, KEY_8),
+	KEY(0,  9, KEY_9),
+	KEY(0, 10, KEY_0),
+	KEY(0, 11, KEY_BACKSPACE),
+
+	KEY(1,  0, KEY_TAB),
+	KEY(1,  1, KEY_Q),
+	KEY(1,  2, KEY_W),
+	KEY(1,  3, KEY_E),
+	KEY(1,  4, KEY_R),
+	KEY(1,  5, KEY_T),
+	KEY(1,  6, KEY_Y),
+	KEY(1,  7, KEY_U),
+	KEY(1,  8, KEY_I),
+	KEY(1,  9, KEY_O),
+	KEY(1, 10, KEY_P),
+	KEY(1, 11, KEY_ENTER),
+
+	KEY(2,  0, KEY_LEFTMETA),
+	KEY(2,  1, KEY_A),
+	KEY(2,  2, KEY_S),
+	KEY(2,  3, KEY_D),
+	KEY(2,  4, KEY_F),
+	KEY(2,  5, KEY_G),
+	KEY(2,  6, KEY_H),
+	KEY(2,  7, KEY_J),
+	KEY(2,  8, KEY_K),
+	KEY(2,  9, KEY_L),
+	KEY(2, 10, KEY_SEMICOLON),
+
+	KEY(3,  0, KEY_LEFTSHIFT),
+	KEY(3,  1, KEY_Z),
+	KEY(3,  2, KEY_X),
+	KEY(3,  3, KEY_C),
+	KEY(3,  4, KEY_V),
+	KEY(3,  5, KEY_B),
+	KEY(3,  6, KEY_N),
+	KEY(3,  7, KEY_M),
+	KEY(3,  8, KEY_COMMA),
+	KEY(3,  9, KEY_DOT),
+	KEY(3, 10, KEY_SLASH),
+
+	KEY(4,  1, KEY_LEFTCTRL),
+	KEY(4,  4, KEY_SPACE),
+	KEY(4,  6, KEY_APOSTROPHE),
+	KEY(4,  8, KEY_RIGHTBRACE),
+	KEY(4,  9, KEY_LEFTBRACE),
+
+	KEY(5,  2, KEY_FN),
+	KEY(5,  3, KEY_LEFTALT),
+	KEY(5,  5, KEY_RIGHTALT),
+};
+
+static const struct matrix_keymap_data ppkb_default_keymap_data = {
+	.keymap		= ppkb_default_keymap,
+	.keymap_size	= ARRAY_SIZE(ppkb_default_keymap),
+};
+
 struct pinephone_keyboard {
 	struct input_dev *input;
 	unsigned short *fn_keymap;
@@ -151,6 +258,8 @@  static irqreturn_t ppkb_irq_thread(int irq, void *data)
 
 static int ppkb_probe(struct i2c_client *client)
 {
+	const struct matrix_keymap_data *fn_keymap_data = &ppkb_default_fn_keymap_data;
+	const struct matrix_keymap_data *keymap_data = &ppkb_default_keymap_data;
 	struct device *dev = &client->dev;
 	unsigned int phys_rows, phys_cols;
 	unsigned int map_rows, map_cols;
@@ -176,9 +285,18 @@  static int ppkb_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	ret = matrix_keypad_parse_properties(dev, &map_rows, &map_cols);
-	if (ret)
-		return ret;
+	/* Allow the devicetree to override the default keymaps. */
+	if (of_property_read_bool(dev->of_node, "linux,fn-keymap") ||
+	    of_property_read_bool(dev->of_node, "linux,keymap")) {
+		ret = matrix_keypad_parse_properties(dev, &map_rows, &map_cols);
+		if (ret)
+			return ret;
+
+		fn_keymap_data = keymap_data = NULL;
+	} else {
+		map_rows = PPKB_DEFAULT_KEYMAP_ROWS;
+		map_cols = PPKB_DEFAULT_KEYMAP_COLS;
+	}
 
 	phys_rows = info[PPKB_MATRIX_SIZE] & 0xf;
 	phys_cols = info[PPKB_MATRIX_SIZE] >> 4;
@@ -214,14 +332,14 @@  static int ppkb_probe(struct i2c_client *client)
 	__set_bit(EV_MSC, ppkb->input->evbit);
 	__set_bit(EV_REP, ppkb->input->evbit);
 
-	ret = matrix_keypad_build_keymap(NULL, "linux,fn-keymap",
+	ret = matrix_keypad_build_keymap(fn_keymap_data, "linux,fn-keymap",
 					 map_rows, map_cols, NULL, ppkb->input);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to build FN keymap\n");
 
 	ppkb->fn_keymap = ppkb->input->keycode;
 
-	ret = matrix_keypad_build_keymap(NULL, "linux,keymap",
+	ret = matrix_keypad_build_keymap(keymap_data, "linux,keymap",
 					 map_rows, map_cols, NULL, ppkb->input);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to build keymap\n");