| Message ID | 20220129230043.12422-4-samuel@sholland.org |
|---|---|
| State | New |
| Headers | show |
| Series | Pine64 PinePhone keyboard support | expand |
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.
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
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.
On Tue, Apr 12, 2022 at 08:20:24PM +1000, Jarrah wrote: > > 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. You will not get access to all the symbols anyway, this way. You should solve this via xkb and kernel keymaps (man keymaps(5)). Normal users should not be modifying basic keymap in DT or the driver anyway. kind regards, o.
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 --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");
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(-)