This series adds support for the official keyboard case for the Pine64
PinePhone and PinePhone Pro. This accessory contains a keyboard MCU and
an IP5209 power bank IC. The keyboard MCU firmware[0] is free software.
It exposes the keyboard scan matrix over I2C, and also provides commands
for SMBus access to the IP5209. In order to keep the IP5209 driver[1]
generic, this is modeled as an I2C bus child of the keyboard.
[0]: https://megous.com/git/pinephone-keyboard/about/
[1]: https://lore.kernel.org/lkml/[email protected]/T/
Samuel Holland (5):
dt-bindings: input: Add the PinePhone keyboard binding
Input: pinephone-keyboard - Add PinePhone keyboard driver
Input: pinephone-keyboard - Build in the default keymap
Input: pinephone-keyboard - Support the proxied I2C bus
[DO NOT MERGE] arm64: dts: allwinner: pinephone: Add keyboard
.../input/pine64,pinephone-keyboard.yaml | 90 ++++
MAINTAINERS | 6 +
.../dts/allwinner/sun50i-a64-pinephone.dtsi | 18 +
drivers/input/keyboard/Kconfig | 10 +
drivers/input/keyboard/Makefile | 1 +
drivers/input/keyboard/pinephone-keyboard.c | 449 ++++++++++++++++++
6 files changed, 574 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/pine64,pinephone-keyboard.yaml
create mode 100644 drivers/input/keyboard/pinephone-keyboard.c
--
2.33.1
Add devicetree support for the PinePhone keyboard case, which provides a
matrix keyboard interface and a proxied I2C bus.
Signed-off-by: Samuel Holland <[email protected]>
---
.../input/pine64,pinephone-keyboard.yaml | 90 +++++++++++++++++++
1 file changed, 90 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/pine64,pinephone-keyboard.yaml
diff --git a/Documentation/devicetree/bindings/input/pine64,pinephone-keyboard.yaml b/Documentation/devicetree/bindings/input/pine64,pinephone-keyboard.yaml
new file mode 100644
index 000000000000..00f084b263f0
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/pine64,pinephone-keyboard.yaml
@@ -0,0 +1,90 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/pine64,pinephone-keyboard.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Pine64 PinePhone keyboard device tree bindings
+
+maintainers:
+ - Samuel Holland <[email protected]>
+
+description:
+ A keyboard accessory is available for the Pine64 PinePhone and PinePhone Pro.
+ It connects via I2C, providing a raw scan matrix, a flashing interface, and a
+ subordinate I2C bus for communication with a battery charger IC.
+
+allOf:
+ - $ref: /schemas/input/matrix-keymap.yaml#
+
+properties:
+ compatible:
+ const: pine64,pinephone-keyboard
+
+ reg:
+ const: 0x15
+
+ interrupts:
+ maxItems: 1
+
+ linux,fn-keymap:
+ $ref: /schemas/input/matrix-keymap.yaml#/properties/linux,keymap
+ description: keymap used when the Fn key is pressed
+
+ wakeup-source: true
+
+ i2c-bus:
+ $ref: /schemas/i2c/i2c-controller.yaml#
+
+dependencies:
+ linux,fn-keymap: [ 'keypad,num-columns', 'keypad,num-rows' ]
+ linux,keymap: [ 'keypad,num-columns', 'keypad,num-rows' ]
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/input/input.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ keyboard@15 {
+ compatible = "pine64,pinephone-keyboard";
+ reg = <0x15>;
+ interrupt-parent = <&r_pio>;
+ interrupts = <0 12 IRQ_TYPE_EDGE_FALLING>; /* PL12 */
+ keypad,num-rows = <6>;
+ keypad,num-columns = <12>;
+ linux,fn-keymap = <MATRIX_KEY(0, 0, KEY_FN_ESC)
+ MATRIX_KEY(0, 1, KEY_F1)
+ MATRIX_KEY(0, 2, KEY_F2)
+ /* ... */
+ MATRIX_KEY(5, 2, KEY_FN)
+ MATRIX_KEY(5, 3, KEY_LEFTALT)
+ MATRIX_KEY(5, 5, KEY_RIGHTALT)>;
+ linux,keymap = <MATRIX_KEY(0, 0, KEY_ESC)
+ MATRIX_KEY(0, 1, KEY_1)
+ MATRIX_KEY(0, 2, KEY_2)
+ /* ... */
+ MATRIX_KEY(5, 2, KEY_FN)
+ MATRIX_KEY(5, 3, KEY_LEFTALT)
+ MATRIX_KEY(5, 5, KEY_RIGHTALT)>;
+
+ i2c-bus {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ charger@75 {
+ reg = <0x75>;
+ };
+ };
+ };
+ };
--
2.33.1
The PinePhone keyboard case contains a battery managed by an integrated
power bank IC. The power bank IC communicates over I2C, and the keyboard
MCU firmware provides an interface to read and write its registers.
Let's use this interface to implement a SMBus adapter, so we can reuse
the driver for the power bank IC.
Signed-off-by: Samuel Holland <[email protected]>
---
drivers/input/keyboard/pinephone-keyboard.c | 73 +++++++++++++++++++++
1 file changed, 73 insertions(+)
diff --git a/drivers/input/keyboard/pinephone-keyboard.c b/drivers/input/keyboard/pinephone-keyboard.c
index 8065bc3e101a..7d2e16e588a0 100644
--- a/drivers/input/keyboard/pinephone-keyboard.c
+++ b/drivers/input/keyboard/pinephone-keyboard.c
@@ -3,6 +3,7 @@
// Copyright (C) 2021-2022 Samuel Holland <[email protected]>
#include <linux/crc8.h>
+#include <linux/delay.h>
#include <linux/i2c.h>
#include <linux/input/matrix_keypad.h>
#include <linux/interrupt.h>
@@ -23,6 +24,11 @@
#define PPKB_SCAN_DATA 0x08
#define PPKB_SYS_CONFIG 0x20
#define PPKB_SYS_CONFIG_DISABLE_SCAN BIT(0)
+#define PPKB_SYS_SMBUS_COMMAND 0x21
+#define PPKB_SYS_SMBUS_DATA 0x22
+#define PPKB_SYS_COMMAND 0x23
+#define PPKB_SYS_COMMAND_SMBUS_READ 0x91
+#define PPKB_SYS_COMMAND_SMBUS_WRITE 0xa1
#define PPKB_DEFAULT_KEYMAP_ROWS 6
#define PPKB_DEFAULT_KEYMAP_COLS 12
@@ -132,6 +138,7 @@ static const struct matrix_keymap_data ppkb_default_keymap_data = {
};
struct pinephone_keyboard {
+ struct i2c_adapter adapter;
struct input_dev *input;
unsigned short *fn_keymap;
u8 crc_table[CRC8_TABLE_SIZE];
@@ -143,6 +150,57 @@ struct pinephone_keyboard {
u8 buf[];
};
+static int ppkb_adap_smbus_xfer(struct i2c_adapter *adap, u16 addr,
+ unsigned short flags, char read_write,
+ u8 command, int size,
+ union i2c_smbus_data *data)
+{
+ struct i2c_client *client = adap->algo_data;
+ u8 buf[3];
+ int ret;
+
+ buf[0] = command;
+ buf[1] = data->byte;
+ buf[2] = read_write == I2C_SMBUS_READ ? PPKB_SYS_COMMAND_SMBUS_READ
+ : PPKB_SYS_COMMAND_SMBUS_WRITE;
+
+ ret = i2c_smbus_write_i2c_block_data(client, PPKB_SYS_SMBUS_COMMAND,
+ sizeof(buf), buf);
+ if (ret)
+ return ret;
+
+ /* Read back the command status until it passes or fails. */
+ do {
+ usleep_range(300, 500);
+ ret = i2c_smbus_read_byte_data(client, PPKB_SYS_COMMAND);
+ } while (ret == buf[2]);
+ if (ret < 0)
+ return ret;
+ /* Commands return 0x00 on success and 0xff on failure. */
+ if (ret)
+ return -EIO;
+
+ if (read_write == I2C_SMBUS_READ) {
+ ret = i2c_smbus_read_byte_data(client, PPKB_SYS_SMBUS_DATA);
+ if (ret < 0)
+ return ret;
+
+ data->byte = ret;
+ }
+
+ return 0;
+}
+
+static u32 ppkg_adap_functionality(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_SMBUS_BYTE_DATA;
+}
+
+static const struct i2c_algorithm ppkb_adap_algo = {
+ .smbus_xfer = ppkb_adap_smbus_xfer,
+ .functionality = ppkg_adap_functionality,
+};
+
static int ppkb_set_scan(struct i2c_client *client, bool enable)
{
struct device *dev = &client->dev;
@@ -265,6 +323,7 @@ static int ppkb_probe(struct i2c_client *client)
unsigned int map_rows, map_cols;
struct pinephone_keyboard *ppkb;
u8 info[PPKB_MATRIX_SIZE + 1];
+ struct device_node *i2c_bus;
int ret;
ret = i2c_smbus_read_i2c_block_data(client, 0, sizeof(info), info);
@@ -312,6 +371,20 @@ static int ppkb_probe(struct i2c_client *client)
i2c_set_clientdata(client, ppkb);
+ i2c_bus = of_get_child_by_name(dev->of_node, "i2c-bus");
+ if (i2c_bus) {
+ ppkb->adapter.owner = THIS_MODULE;
+ ppkb->adapter.algo = &ppkb_adap_algo;
+ ppkb->adapter.algo_data = client;
+ ppkb->adapter.dev.parent = dev;
+ ppkb->adapter.dev.of_node = i2c_bus;
+ strscpy(ppkb->adapter.name, DRV_NAME, sizeof(ppkb->adapter.name));
+
+ ret = devm_i2c_add_adapter(dev, &ppkb->adapter);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to add I2C adapter\n");
+ }
+
crc8_populate_msb(ppkb->crc_table, PPKB_CRC8_POLYNOMIAL);
ppkb->row_shift = get_count_order(map_cols);
ppkb->rows = map_rows;
--
2.33.1
The official PinePhone keyboard accessory connects to the phone's POGO
pins for I2C and interrupts. It has an Injoinic IP5209 power bank IC
connected to the keyboard's internal I2C bus.
Signed-off-by: Samuel Holland <[email protected]>
---
.../dts/allwinner/sun50i-a64-pinephone.dtsi | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
index 87847116ab6d..2fa1bdf8aa63 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi
@@ -208,6 +208,24 @@ accelerometer@68 {
/* Connected to pogo pins (external spring based pinheader for user addons) */
&i2c2 {
status = "okay";
+
+ keyboard@15 {
+ compatible = "pine64,pinephone-keyboard";
+ reg = <0x15>;
+ interrupt-parent = <&r_pio>;
+ interrupts = <0 12 IRQ_TYPE_EDGE_FALLING>; /* PL12 */
+ wakeup-source;
+
+ i2c-bus {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ charger@75 {
+ compatible = "injoinic,ip5209";
+ reg = <0x75>;
+ };
+ };
+ };
};
&lradc {
--
2.33.1
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 <[email protected]>
Signed-off-by: Samuel Holland <[email protected]>
---
drivers/input/keyboard/pinephone-keyboard.c | 128 +++++++++++++++++++-
1 file changed, 123 insertions(+), 5 deletions(-)
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");
--
2.33.1
Hello Samuel,
On Sat, Jan 29, 2022 at 05:00:41PM -0600, Samuel Holland wrote:
> The PinePhone keyboard case contains a battery managed by an integrated
> power bank IC. The power bank IC communicates over I2C, and the keyboard
> MCU firmware provides an interface to read and write its registers.
> Let's use this interface to implement a SMBus adapter, so we can reuse
> the driver for the power bank IC.
>
> Signed-off-by: Samuel Holland <[email protected]>
> ---
>
> drivers/input/keyboard/pinephone-keyboard.c | 73 +++++++++++++++++++++
> 1 file changed, 73 insertions(+)
>
> diff --git a/drivers/input/keyboard/pinephone-keyboard.c b/drivers/input/keyboard/pinephone-keyboard.c
> index 8065bc3e101a..7d2e16e588a0 100644
> --- a/drivers/input/keyboard/pinephone-keyboard.c
> +++ b/drivers/input/keyboard/pinephone-keyboard.c
> @@ -3,6 +3,7 @@
> // Copyright (C) 2021-2022 Samuel Holland <[email protected]>
>
> #include <linux/crc8.h>
> +#include <linux/delay.h>
> #include <linux/i2c.h>
> #include <linux/input/matrix_keypad.h>
> #include <linux/interrupt.h>
> @@ -23,6 +24,11 @@
> #define PPKB_SCAN_DATA 0x08
> #define PPKB_SYS_CONFIG 0x20
> #define PPKB_SYS_CONFIG_DISABLE_SCAN BIT(0)
> +#define PPKB_SYS_SMBUS_COMMAND 0x21
> +#define PPKB_SYS_SMBUS_DATA 0x22
> +#define PPKB_SYS_COMMAND 0x23
> +#define PPKB_SYS_COMMAND_SMBUS_READ 0x91
> +#define PPKB_SYS_COMMAND_SMBUS_WRITE 0xa1
>
> #define PPKB_DEFAULT_KEYMAP_ROWS 6
> #define PPKB_DEFAULT_KEYMAP_COLS 12
> @@ -132,6 +138,7 @@ static const struct matrix_keymap_data ppkb_default_keymap_data = {
> };
>
> struct pinephone_keyboard {
> + struct i2c_adapter adapter;
> struct input_dev *input;
> unsigned short *fn_keymap;
> u8 crc_table[CRC8_TABLE_SIZE];
> @@ -143,6 +150,57 @@ struct pinephone_keyboard {
> u8 buf[];
> };
>
> +static int ppkb_adap_smbus_xfer(struct i2c_adapter *adap, u16 addr,
> + unsigned short flags, char read_write,
> + u8 command, int size,
> + union i2c_smbus_data *data)
> +{
> + struct i2c_client *client = adap->algo_data;
> + u8 buf[3];
> + int ret;
> +
> + buf[0] = command;
> + buf[1] = data->byte;
> + buf[2] = read_write == I2C_SMBUS_READ ? PPKB_SYS_COMMAND_SMBUS_READ
> + : PPKB_SYS_COMMAND_SMBUS_WRITE;
> +
> + ret = i2c_smbus_write_i2c_block_data(client, PPKB_SYS_SMBUS_COMMAND,
> + sizeof(buf), buf);
> + if (ret)
> + return ret;
[1]
> + /* Read back the command status until it passes or fails. */
> + do {
> + usleep_range(300, 500);
> + ret = i2c_smbus_read_byte_data(client, PPKB_SYS_COMMAND);
> + } while (ret == buf[2]);
> + if (ret < 0)
> + return ret;
> + /* Commands return 0x00 on success and 0xff on failure. */
> + if (ret)
> + return -EIO;
> +
> + if (read_write == I2C_SMBUS_READ) {
> + ret = i2c_smbus_read_byte_data(client, PPKB_SYS_SMBUS_DATA);
> + if (ret < 0)
> + return ret;
Please use a single read transfer to get both command result and data.
There will be less risk that some userspace app will issue another command
in between command status being read as 0 and data byte being read.
Otherwise if you use this in some read/modify/write operation, you
may write unexpected value to PMIC. I2C register layout is designed
to make this as optimal as possible in a single I2C transaction, so
you only need 3 bytes to start command and 2 bytes to read the result
and data, both in a single xfer. There's very high likelihood the command
will complete in those 300 - 500 us anyway, because the timing is
predictable. If this delay is set right, it's almost guaranteed,
only two xfers will be necessary to run the command and get the result+
status.
And if possible, it would be best if the bus was somehow made busy for
other users, until the whole comand/result sequence completes, to eliminate
the possibility of another command being issued by other bus users
around [1].
Thank you and kind regards,
o.
> + data->byte = ret;
> + }
> +
> + return 0;
> +}
> +
> +static u32 ppkg_adap_functionality(struct i2c_adapter *adap)
> +{
> + return I2C_FUNC_SMBUS_BYTE_DATA;
> +}
> +
> +static const struct i2c_algorithm ppkb_adap_algo = {
> + .smbus_xfer = ppkb_adap_smbus_xfer,
> + .functionality = ppkg_adap_functionality,
> +};
> +
> static int ppkb_set_scan(struct i2c_client *client, bool enable)
> {
> struct device *dev = &client->dev;
> @@ -265,6 +323,7 @@ static int ppkb_probe(struct i2c_client *client)
> unsigned int map_rows, map_cols;
> struct pinephone_keyboard *ppkb;
> u8 info[PPKB_MATRIX_SIZE + 1];
> + struct device_node *i2c_bus;
> int ret;
>
> ret = i2c_smbus_read_i2c_block_data(client, 0, sizeof(info), info);
> @@ -312,6 +371,20 @@ static int ppkb_probe(struct i2c_client *client)
>
> i2c_set_clientdata(client, ppkb);
>
> + i2c_bus = of_get_child_by_name(dev->of_node, "i2c-bus");
> + if (i2c_bus) {
> + ppkb->adapter.owner = THIS_MODULE;
> + ppkb->adapter.algo = &ppkb_adap_algo;
> + ppkb->adapter.algo_data = client;
> + ppkb->adapter.dev.parent = dev;
> + ppkb->adapter.dev.of_node = i2c_bus;
> + strscpy(ppkb->adapter.name, DRV_NAME, sizeof(ppkb->adapter.name));
> +
> + ret = devm_i2c_add_adapter(dev, &ppkb->adapter);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to add I2C adapter\n");
> + }
> +
> crc8_populate_msb(ppkb->crc_table, PPKB_CRC8_POLYNOMIAL);
> ppkb->row_shift = get_count_order(map_cols);
> ppkb->rows = map_rows;
> --
> 2.33.1
>
On 1/29/22 8:05 PM, Ondřej Jirman wrote:
> Hello Samuel,
>
> On Sat, Jan 29, 2022 at 05:00:41PM -0600, Samuel Holland wrote:
>> The PinePhone keyboard case contains a battery managed by an integrated
>> power bank IC. The power bank IC communicates over I2C, and the keyboard
>> MCU firmware provides an interface to read and write its registers.
>> Let's use this interface to implement a SMBus adapter, so we can reuse
>> the driver for the power bank IC.
>>
>> Signed-off-by: Samuel Holland <[email protected]>
>> ---
>>
>> drivers/input/keyboard/pinephone-keyboard.c | 73 +++++++++++++++++++++
>> 1 file changed, 73 insertions(+)
>>
>> diff --git a/drivers/input/keyboard/pinephone-keyboard.c b/drivers/input/keyboard/pinephone-keyboard.c
>> index 8065bc3e101a..7d2e16e588a0 100644
>> --- a/drivers/input/keyboard/pinephone-keyboard.c
>> +++ b/drivers/input/keyboard/pinephone-keyboard.c
>> @@ -3,6 +3,7 @@
>> // Copyright (C) 2021-2022 Samuel Holland <[email protected]>
>>
>> #include <linux/crc8.h>
>> +#include <linux/delay.h>
>> #include <linux/i2c.h>
>> #include <linux/input/matrix_keypad.h>
>> #include <linux/interrupt.h>
>> @@ -23,6 +24,11 @@
>> #define PPKB_SCAN_DATA 0x08
>> #define PPKB_SYS_CONFIG 0x20
>> #define PPKB_SYS_CONFIG_DISABLE_SCAN BIT(0)
>> +#define PPKB_SYS_SMBUS_COMMAND 0x21
>> +#define PPKB_SYS_SMBUS_DATA 0x22
>> +#define PPKB_SYS_COMMAND 0x23
>> +#define PPKB_SYS_COMMAND_SMBUS_READ 0x91
>> +#define PPKB_SYS_COMMAND_SMBUS_WRITE 0xa1
>>
>> #define PPKB_DEFAULT_KEYMAP_ROWS 6
>> #define PPKB_DEFAULT_KEYMAP_COLS 12
>> @@ -132,6 +138,7 @@ static const struct matrix_keymap_data ppkb_default_keymap_data = {
>> };
>>
>> struct pinephone_keyboard {
>> + struct i2c_adapter adapter;
>> struct input_dev *input;
>> unsigned short *fn_keymap;
>> u8 crc_table[CRC8_TABLE_SIZE];
>> @@ -143,6 +150,57 @@ struct pinephone_keyboard {
>> u8 buf[];
>> };
>>
>> +static int ppkb_adap_smbus_xfer(struct i2c_adapter *adap, u16 addr,
>> + unsigned short flags, char read_write,
>> + u8 command, int size,
>> + union i2c_smbus_data *data)
>> +{
>> + struct i2c_client *client = adap->algo_data;
>> + u8 buf[3];
>> + int ret;
>> +
>> + buf[0] = command;
>> + buf[1] = data->byte;
>> + buf[2] = read_write == I2C_SMBUS_READ ? PPKB_SYS_COMMAND_SMBUS_READ
>> + : PPKB_SYS_COMMAND_SMBUS_WRITE;
>> +
>> + ret = i2c_smbus_write_i2c_block_data(client, PPKB_SYS_SMBUS_COMMAND,
>> + sizeof(buf), buf);
>> + if (ret)
>> + return ret;
>
> [1]
>
>> + /* Read back the command status until it passes or fails. */
>> + do {
>> + usleep_range(300, 500);
>> + ret = i2c_smbus_read_byte_data(client, PPKB_SYS_COMMAND);
>> + } while (ret == buf[2]);
>> + if (ret < 0)
>> + return ret;
>> + /* Commands return 0x00 on success and 0xff on failure. */
>> + if (ret)
>> + return -EIO;
>> +
>> + if (read_write == I2C_SMBUS_READ) {
>> + ret = i2c_smbus_read_byte_data(client, PPKB_SYS_SMBUS_DATA);
>> + if (ret < 0)
>> + return ret;
>
> Please use a single read transfer to get both command result and data.
> There will be less risk that some userspace app will issue another command
> in between command status being read as 0 and data byte being read.
>
> Otherwise if you use this in some read/modify/write operation, you
> may write unexpected value to PMIC. I2C register layout is designed
> to make this as optimal as possible in a single I2C transaction, so
> you only need 3 bytes to start command and 2 bytes to read the result
> and data, both in a single xfer. There's very high likelihood the command
> will complete in those 300 - 500 us anyway, because the timing is
> predictable. If this delay is set right, it's almost guaranteed,
> only two xfers will be necessary to run the command and get the result+
> status.
I did this originally, but it causes a different race condition: since the data
is read first, the command can complete between when the data is read and when
the result is read. If this happens, the command will be seen as complete, but
the data will be garbage.
This caused occasional read errors for the charger's power supply properties,
because I2C reads sometimes returned nonsensical values for those bytes.
> And if possible, it would be best if the bus was somehow made busy for
> other users, until the whole comand/result sequence completes, to eliminate
> the possibility of another command being issued by other bus users
> around [1].
Yes, I can add a call to i2c_lock_bus() here.
Regards,
Samuel
> Thank you and kind regards,
> o.
>
>> + data->byte = ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static u32 ppkg_adap_functionality(struct i2c_adapter *adap)
>> +{
>> + return I2C_FUNC_SMBUS_BYTE_DATA;
>> +}
>> +
>> +static const struct i2c_algorithm ppkb_adap_algo = {
>> + .smbus_xfer = ppkb_adap_smbus_xfer,
>> + .functionality = ppkg_adap_functionality,
>> +};
>> +
>> static int ppkb_set_scan(struct i2c_client *client, bool enable)
>> {
>> struct device *dev = &client->dev;
>> @@ -265,6 +323,7 @@ static int ppkb_probe(struct i2c_client *client)
>> unsigned int map_rows, map_cols;
>> struct pinephone_keyboard *ppkb;
>> u8 info[PPKB_MATRIX_SIZE + 1];
>> + struct device_node *i2c_bus;
>> int ret;
>>
>> ret = i2c_smbus_read_i2c_block_data(client, 0, sizeof(info), info);
>> @@ -312,6 +371,20 @@ static int ppkb_probe(struct i2c_client *client)
>>
>> i2c_set_clientdata(client, ppkb);
>>
>> + i2c_bus = of_get_child_by_name(dev->of_node, "i2c-bus");
>> + if (i2c_bus) {
>> + ppkb->adapter.owner = THIS_MODULE;
>> + ppkb->adapter.algo = &ppkb_adap_algo;
>> + ppkb->adapter.algo_data = client;
>> + ppkb->adapter.dev.parent = dev;
>> + ppkb->adapter.dev.of_node = i2c_bus;
>> + strscpy(ppkb->adapter.name, DRV_NAME, sizeof(ppkb->adapter.name));
>> +
>> + ret = devm_i2c_add_adapter(dev, &ppkb->adapter);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to add I2C adapter\n");
>> + }
>> +
>> crc8_populate_msb(ppkb->crc_table, PPKB_CRC8_POLYNOMIAL);
>> ppkb->row_shift = get_count_order(map_cols);
>> ppkb->rows = map_rows;
>> --
>> 2.33.1
>>
On Sat, Jan 29, 2022 at 08:43:30PM -0600, Samuel Holland wrote:
> On 1/29/22 8:05 PM, Ondřej Jirman wrote:
> >
> > Please use a single read transfer to get both command result and data.
> > There will be less risk that some userspace app will issue another command
> > in between command status being read as 0 and data byte being read.
> >
> > Otherwise if you use this in some read/modify/write operation, you
> > may write unexpected value to PMIC. I2C register layout is designed
> > to make this as optimal as possible in a single I2C transaction, so
> > you only need 3 bytes to start command and 2 bytes to read the result
> > and data, both in a single xfer. There's very high likelihood the command
> > will complete in those 300 - 500 us anyway, because the timing is
> > predictable. If this delay is set right, it's almost guaranteed,
> > only two xfers will be necessary to run the command and get the result+
> > status.
>
> I did this originally, but it causes a different race condition: since the data
> is read first, the command can complete between when the data is read and when
> the result is read. If this happens, the command will be seen as complete, but
> the data will be garbage.
>
> This caused occasional read errors for the charger's power supply properties,
> because I2C reads sometimes returned nonsensical values for those bytes.
Oh, well. :) I guess the firmware would need to wait for any ongoing I2C
tranfer to finish before setting the command status to 0, for this to work.
Another lesson learned. :(
> > And if possible, it would be best if the bus was somehow made busy for
> > other users, until the whole comand/result sequence completes, to eliminate
> > the possibility of another command being issued by other bus users
> > around [1].
>
> Yes, I can add a call to i2c_lock_bus() here.
Perfect.
thank you,
o.
> Regards,
> Samuel
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.
--
Dmitry
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 Sat, Jan 29, 2022 at 05:00:38PM -0600, Samuel Holland wrote:
> Add devicetree support for the PinePhone keyboard case, which provides a
> matrix keyboard interface and a proxied I2C bus.
>
> Signed-off-by: Samuel Holland <[email protected]>
> ---
>
> .../input/pine64,pinephone-keyboard.yaml | 90 +++++++++++++++++++
> 1 file changed, 90 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/pine64,pinephone-keyboard.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/pine64,pinephone-keyboard.yaml b/Documentation/devicetree/bindings/input/pine64,pinephone-keyboard.yaml
> new file mode 100644
> index 000000000000..00f084b263f0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/pine64,pinephone-keyboard.yaml
> @@ -0,0 +1,90 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/pine64,pinephone-keyboard.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Pine64 PinePhone keyboard device tree bindings
> +
> +maintainers:
> + - Samuel Holland <[email protected]>
> +
> +description:
> + A keyboard accessory is available for the Pine64 PinePhone and PinePhone Pro.
> + It connects via I2C, providing a raw scan matrix, a flashing interface, and a
> + subordinate I2C bus for communication with a battery charger IC.
> +
> +allOf:
> + - $ref: /schemas/input/matrix-keymap.yaml#
> +
> +properties:
> + compatible:
> + const: pine64,pinephone-keyboard
> +
> + reg:
> + const: 0x15
> +
> + interrupts:
> + maxItems: 1
> +
> + linux,fn-keymap:
This should be handled in a common way. Not sure if there's anything
existing for alternate key maps. Child nodes of alternate maps would
scale better than new property name for every alternate map. Or you
could make linux,keymap contain multiple maps (e.g. 2x XxY entries)
Or if the map doesn't change, just put it in the driver.
> + $ref: /schemas/input/matrix-keymap.yaml#/properties/linux,keymap
Referencing individual properties should be avoided.
> + description: keymap used when the Fn key is pressed
> +
> + wakeup-source: true
> +
> + i2c-bus:
> + $ref: /schemas/i2c/i2c-controller.yaml#
> +
> +dependencies:
> + linux,fn-keymap: [ 'keypad,num-columns', 'keypad,num-rows' ]
> + linux,keymap: [ 'keypad,num-columns', 'keypad,num-rows' ]
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/input/input.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + keyboard@15 {
> + compatible = "pine64,pinephone-keyboard";
> + reg = <0x15>;
> + interrupt-parent = <&r_pio>;
> + interrupts = <0 12 IRQ_TYPE_EDGE_FALLING>; /* PL12 */
> + keypad,num-rows = <6>;
> + keypad,num-columns = <12>;
> + linux,fn-keymap = <MATRIX_KEY(0, 0, KEY_FN_ESC)
> + MATRIX_KEY(0, 1, KEY_F1)
> + MATRIX_KEY(0, 2, KEY_F2)
> + /* ... */
> + MATRIX_KEY(5, 2, KEY_FN)
> + MATRIX_KEY(5, 3, KEY_LEFTALT)
> + MATRIX_KEY(5, 5, KEY_RIGHTALT)>;
> + linux,keymap = <MATRIX_KEY(0, 0, KEY_ESC)
> + MATRIX_KEY(0, 1, KEY_1)
> + MATRIX_KEY(0, 2, KEY_2)
> + /* ... */
> + MATRIX_KEY(5, 2, KEY_FN)
> + MATRIX_KEY(5, 3, KEY_LEFTALT)
> + MATRIX_KEY(5, 5, KEY_RIGHTALT)>;
> +
> + i2c-bus {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + charger@75 {
> + reg = <0x75>;
> + };
> + };
> + };
> + };
> --
> 2.33.1
>
>
On 2/11/22 7:23 AM, Rob Herring wrote:
> On Sat, Jan 29, 2022 at 05:00:38PM -0600, Samuel Holland wrote:
>> Add devicetree support for the PinePhone keyboard case, which provides a
>> matrix keyboard interface and a proxied I2C bus.
>>
>> Signed-off-by: Samuel Holland <[email protected]>
>> ---
>>
>> .../input/pine64,pinephone-keyboard.yaml | 90 +++++++++++++++++++
>> 1 file changed, 90 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/input/pine64,pinephone-keyboard.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/input/pine64,pinephone-keyboard.yaml b/Documentation/devicetree/bindings/input/pine64,pinephone-keyboard.yaml
>> new file mode 100644
>> index 000000000000..00f084b263f0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/pine64,pinephone-keyboard.yaml
>> @@ -0,0 +1,90 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/input/pine64,pinephone-keyboard.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Pine64 PinePhone keyboard device tree bindings
>> +
>> +maintainers:
>> + - Samuel Holland <[email protected]>
>> +
>> +description:
>> + A keyboard accessory is available for the Pine64 PinePhone and PinePhone Pro.
>> + It connects via I2C, providing a raw scan matrix, a flashing interface, and a
>> + subordinate I2C bus for communication with a battery charger IC.
>> +
>> +allOf:
>> + - $ref: /schemas/input/matrix-keymap.yaml#
>> +
>> +properties:
>> + compatible:
>> + const: pine64,pinephone-keyboard
>> +
>> + reg:
>> + const: 0x15
>> +
>> + interrupts:
>> + maxItems: 1
>> +
>> + linux,fn-keymap:
>
> This should be handled in a common way. Not sure if there's anything
> existing for alternate key maps. Child nodes of alternate maps would
> scale better than new property name for every alternate map. Or you
> could make linux,keymap contain multiple maps (e.g. 2x XxY entries)
matrix-keymap.yaml proposes doing exactly what I do here:
Some users of this binding might choose to specify secondary keymaps for
cases where there is a modifier key such as a Fn key. Proposed names
for said properties are "linux,fn-keymap" or with another descriptive
word for the modifier other from "Fn".
The only other reference to linux,fn-keymap is in the nvidia,tegra20-kbc
binding, even though that driver doesn't use this property. Instead, what the
tegra-kbc.c code does is double the number of rows to include the Fn layer:
if (kbc->keymap_data && kbc->use_fn_map)
keymap_rows *= 2;
(except that use_fn_map is set nowhere, so this is dead code). Using extra rows
for the Fn layer seems a bit magic to me, but if it is documented, I suppose it
is fine.
So there is not much in the way of prior art.
> Or if the map doesn't change, just put it in the driver.
The keys are removable, so technically it can change, but maybe that's better
handled by userspace than in the DTB? On the other hand, from Dmitry's comment,
it sounds like he would prefer leaving the map out of the driver.
Regards,
Samuel
>> + $ref: /schemas/input/matrix-keymap.yaml#/properties/linux,keymap
>
> Referencing individual properties should be avoided.
>
>> + description: keymap used when the Fn key is pressed
>> +
>> + wakeup-source: true
>> +
>> + i2c-bus:
>> + $ref: /schemas/i2c/i2c-controller.yaml#
>> +
>> +dependencies:
>> + linux,fn-keymap: [ 'keypad,num-columns', 'keypad,num-rows' ]
>> + linux,keymap: [ 'keypad,num-columns', 'keypad,num-rows' ]
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - interrupts
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/input/input.h>
>> + #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> + i2c {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + keyboard@15 {
>> + compatible = "pine64,pinephone-keyboard";
>> + reg = <0x15>;
>> + interrupt-parent = <&r_pio>;
>> + interrupts = <0 12 IRQ_TYPE_EDGE_FALLING>; /* PL12 */
>> + keypad,num-rows = <6>;
>> + keypad,num-columns = <12>;
>> + linux,fn-keymap = <MATRIX_KEY(0, 0, KEY_FN_ESC)
>> + MATRIX_KEY(0, 1, KEY_F1)
>> + MATRIX_KEY(0, 2, KEY_F2)
>> + /* ... */
>> + MATRIX_KEY(5, 2, KEY_FN)
>> + MATRIX_KEY(5, 3, KEY_LEFTALT)
>> + MATRIX_KEY(5, 5, KEY_RIGHTALT)>;
>> + linux,keymap = <MATRIX_KEY(0, 0, KEY_ESC)
>> + MATRIX_KEY(0, 1, KEY_1)
>> + MATRIX_KEY(0, 2, KEY_2)
>> + /* ... */
>> + MATRIX_KEY(5, 2, KEY_FN)
>> + MATRIX_KEY(5, 3, KEY_LEFTALT)
>> + MATRIX_KEY(5, 5, KEY_RIGHTALT)>;
>> +
>> + i2c-bus {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + charger@75 {
>> + reg = <0x75>;
>> + };
>> + };
>> + };
>> + };
>> --
>> 2.33.1
>>
>>
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
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html