[PATCH 0/5] Pine64 PinePhone keyboard support

[PATCH 0/5] Pine64 PinePhone keyboard support

[Samuel Holland]

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/20220129222424.45707-1-samuel@sholland.org/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

[PATCH 1/5] dt-bindings: input: Add the PinePhone keyboard binding

[Samuel Holland]

Add devicetree support for the PinePhone keyboard case, which provides a
matrix keyboard interface and a proxied I2C bus.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 .../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 <samuel@sholland.org>
+
+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

Re: [PATCH 1/5] dt-bindings: input: Add the PinePhone keyboard binding

[Rob Herring]

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 <samuel@sholland.org>
> ---
> 
>  .../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 <samuel@sholland.org>
> +
> +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
> 
> 

Re: [PATCH 1/5] dt-bindings: input: Add the PinePhone keyboard binding

[Samuel Holland]

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 <samuel@sholland.org>
>> ---
>>
>>  .../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 <samuel@sholland.org>
>> +
>> +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
>>
>>

[PATCH 2/5] Input: pinephone-keyboard - Add PinePhone keyboard driver

[Samuel Holland]

The official Pine64 PinePhone keyboard case contains a matrix keypad and
a MCU which runs a libre firmware. Add support for its I2C interface.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 MAINTAINERS                                 |   6 +
 drivers/input/keyboard/Kconfig              |  10 +
 drivers/input/keyboard/Makefile             |   1 +
 drivers/input/keyboard/pinephone-keyboard.c | 258 ++++++++++++++++++++
 4 files changed, 275 insertions(+)
 create mode 100644 drivers/input/keyboard/pinephone-keyboard.c

diff --git a/MAINTAINERS b/MAINTAINERS
index f41088418aae..85428a707e67 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15320,6 +15320,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/chemical/plantower,pms7003.yaml
 F:	drivers/iio/chemical/pms7003.c
 
+PINE64 PINEPHONE KEYBOARD DRIVER
+M:	Samuel Holland <samuel@sholland.org>
+S:	Supported
+F:	Documentation/devicetree/bindings/input/pine64,pinephone-keyboard.yaml
+F:	drivers/input/keyboard/pinephone-keyboard.c
+
 PLDMFW LIBRARY
 M:	Jacob Keller <jacob.e.keller@intel.com>
 S:	Maintained
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 0c607da9ee10..f9978da1c916 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -523,6 +523,16 @@ config KEYBOARD_OPENCORES
 	  To compile this driver as a module, choose M here; the
 	  module will be called opencores-kbd.
 
+config KEYBOARD_PINEPHONE
+	tristate "Pine64 PinePhone Keyboard"
+	depends on I2C
+	select CRC8
+	select INPUT_MATRIXKMAP
+	help
+	  Say Y here to enable support for the keyboard in the Pine64 PinePhone
+	  keyboard case. This driver supports the FLOSS firmware available at
+	  https://megous.com/git/pinephone-keyboard/
+
 config KEYBOARD_PXA27x
 	tristate "PXA27x/PXA3xx keypad support"
 	depends on PXA27x || PXA3xx || ARCH_MMP
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index e3c8648f834e..84da37e3a2f5 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_KEYBOARD_NSPIRE)		+= nspire-keypad.o
 obj-$(CONFIG_KEYBOARD_OMAP)		+= omap-keypad.o
 obj-$(CONFIG_KEYBOARD_OMAP4)		+= omap4-keypad.o
 obj-$(CONFIG_KEYBOARD_OPENCORES)	+= opencores-kbd.o
+obj-$(CONFIG_KEYBOARD_PINEPHONE)	+= pinephone-keyboard.o
 obj-$(CONFIG_KEYBOARD_PMIC8XXX)		+= pmic8xxx-keypad.o
 obj-$(CONFIG_KEYBOARD_PXA27x)		+= pxa27x_keypad.o
 obj-$(CONFIG_KEYBOARD_PXA930_ROTARY)	+= pxa930_rotary.o
diff --git a/drivers/input/keyboard/pinephone-keyboard.c b/drivers/input/keyboard/pinephone-keyboard.c
new file mode 100644
index 000000000000..9a071753fd91
--- /dev/null
+++ b/drivers/input/keyboard/pinephone-keyboard.c
@@ -0,0 +1,258 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright (C) 2021-2022 Samuel Holland <samuel@sholland.org>
+
+#include <linux/crc8.h>
+#include <linux/i2c.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+
+#define DRV_NAME			"pinephone-keyboard"
+
+#define PPKB_CRC8_POLYNOMIAL		0x07
+
+#define PPKB_DEVICE_ID_HI		0x00
+#define PPKB_DEVICE_ID_HI_VALUE			0x4b
+#define PPKB_DEVICE_ID_LO		0x01
+#define PPKB_DEVICE_ID_LO_VALUE			0x42
+#define PPKB_FW_REVISION		0x02
+#define PPKB_FW_FEATURES		0x03
+#define PPKB_MATRIX_SIZE		0x06
+#define PPKB_SCAN_CRC			0x07
+#define PPKB_SCAN_DATA			0x08
+#define PPKB_SYS_CONFIG			0x20
+#define PPKB_SYS_CONFIG_DISABLE_SCAN		BIT(0)
+
+struct pinephone_keyboard {
+	struct input_dev *input;
+	unsigned short *fn_keymap;
+	u8 crc_table[CRC8_TABLE_SIZE];
+	u8 row_shift;
+	u8 rows;
+	u8 cols;
+	u8 fn_state;
+	u8 buf_swap;
+	u8 buf[];
+};
+
+static int ppkb_set_scan(struct i2c_client *client, bool enable)
+{
+	struct device *dev = &client->dev;
+	int ret, val;
+
+	ret = i2c_smbus_read_byte_data(client, PPKB_SYS_CONFIG);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read config: %d\n", ret);
+		return ret;
+	}
+
+	if (enable)
+		val = ret & ~PPKB_SYS_CONFIG_DISABLE_SCAN;
+	else
+		val = ret | PPKB_SYS_CONFIG_DISABLE_SCAN;
+	ret = i2c_smbus_write_byte_data(client, PPKB_SYS_CONFIG, val);
+	if (ret) {
+		dev_err(dev, "Failed to write config: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void ppkb_update(struct i2c_client *client)
+{
+	struct pinephone_keyboard *ppkb = i2c_get_clientdata(client);
+	struct device *dev = &client->dev;
+	size_t buf_len = ppkb->cols + 1;
+	u8 *old_buf = ppkb->buf;
+	u8 *new_buf = ppkb->buf;
+	unsigned short *keymap;
+	int col, crc, ret, row;
+
+	if (ppkb->buf_swap)
+		old_buf += buf_len;
+	else
+		new_buf += buf_len;
+
+	ret = i2c_smbus_read_i2c_block_data(client, PPKB_SCAN_CRC,
+					    buf_len, new_buf);
+	if (ret != buf_len) {
+		dev_err(dev, "Failed to read scan data: %d\n", ret);
+		return;
+	}
+
+	crc = crc8(ppkb->crc_table, new_buf + 1, ppkb->cols, CRC8_INIT_VALUE);
+	if (crc != new_buf[0]) {
+		dev_err(dev, "Bad scan data (%02x != %02x)\n", crc, new_buf[0]);
+		return;
+	}
+
+	ppkb->buf_swap = !ppkb->buf_swap;
+
+	keymap = ppkb->fn_state ? ppkb->fn_keymap : ppkb->input->keycode;
+	for (col = 0; col < ppkb->cols; ++col) {
+		u8 old = *(++old_buf);
+		u8 new = *(++new_buf);
+		u8 changed = old ^ new;
+
+		for (row = 0; row < ppkb->rows; ++row) {
+			int code = MATRIX_SCAN_CODE(row, col, ppkb->row_shift);
+			int value = new & BIT(row);
+
+			if (!(changed & BIT(row)))
+				continue;
+
+			dev_dbg(dev, "row %u col %u %sed\n",
+				row, col, value ? "press" : "releas");
+			if (keymap[code] == KEY_FN) {
+				dev_dbg(dev, "FN is now %sed\n",
+					value ? "press" : "releas");
+				keymap = value ? ppkb->fn_keymap
+					       : ppkb->input->keycode;
+				ppkb->fn_state = value;
+			}
+			input_event(ppkb->input, EV_MSC, MSC_SCAN, code);
+			input_report_key(ppkb->input, keymap[code], value);
+		}
+	}
+	input_sync(ppkb->input);
+}
+
+static int ppkb_open(struct input_dev *input)
+{
+	struct i2c_client *client = input_get_drvdata(input);
+	int ret;
+
+	ret = ppkb_set_scan(client, true);
+	if (ret)
+		return ret;
+
+	ppkb_update(client);
+
+	return 0;
+}
+
+static void ppkb_close(struct input_dev *input)
+{
+	struct i2c_client *client = input_get_drvdata(input);
+
+	ppkb_set_scan(client, false);
+}
+
+static irqreturn_t ppkb_irq_thread(int irq, void *data)
+{
+	struct i2c_client *client = data;
+
+	ppkb_update(client);
+
+	return IRQ_HANDLED;
+}
+
+static int ppkb_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	unsigned int phys_rows, phys_cols;
+	unsigned int map_rows, map_cols;
+	struct pinephone_keyboard *ppkb;
+	u8 info[PPKB_MATRIX_SIZE + 1];
+	int ret;
+
+	ret = i2c_smbus_read_i2c_block_data(client, 0, sizeof(info), info);
+	if (ret != sizeof(info))
+		return dev_err_probe(dev, ret, "Failed to read device ID\n");
+
+	if (info[PPKB_DEVICE_ID_HI] != PPKB_DEVICE_ID_HI_VALUE ||
+	    info[PPKB_DEVICE_ID_LO] != PPKB_DEVICE_ID_LO_VALUE)
+		return dev_err_probe(dev, -ENODEV, "Unexpected device ID\n");
+
+	dev_info(dev, "Found keyboard firmware version %d.%d features %#x\n",
+		 info[PPKB_FW_REVISION] >> 4,
+		 info[PPKB_FW_REVISION] & 0xf,
+		 info[PPKB_FW_FEATURES]);
+
+	/* Disable scan by default to save power. */
+	ret = ppkb_set_scan(client, false);
+	if (ret)
+		return ret;
+
+	ret = matrix_keypad_parse_properties(dev, &map_rows, &map_cols);
+	if (ret)
+		return ret;
+
+	phys_rows = info[PPKB_MATRIX_SIZE] & 0xf;
+	phys_cols = info[PPKB_MATRIX_SIZE] >> 4;
+	if (map_rows != phys_rows || map_cols != phys_cols)
+		return dev_err_probe(dev, -EINVAL,
+				     "Keyboard size is %ux%u, but keymap is %ux%u\n",
+				     phys_rows, phys_cols, map_rows, map_cols);
+
+	/* Allocate two buffers, and include space for reading the CRC. */
+	ppkb = devm_kzalloc(dev, struct_size(ppkb, buf, 2 * (phys_cols + 1)), GFP_KERNEL);
+	if (!ppkb)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, ppkb);
+
+	crc8_populate_msb(ppkb->crc_table, PPKB_CRC8_POLYNOMIAL);
+	ppkb->row_shift = get_count_order(map_cols);
+	ppkb->rows = map_rows;
+	ppkb->cols = map_cols;
+
+	ppkb->input = devm_input_allocate_device(dev);
+	if (!ppkb->input)
+		return -ENOMEM;
+
+	input_set_drvdata(ppkb->input, client);
+
+	ppkb->input->name = "PinePhone Keyboard";
+	ppkb->input->phys = DRV_NAME "/input0";
+	ppkb->input->id.bustype = BUS_I2C;
+	ppkb->input->open = ppkb_open;
+	ppkb->input->close = ppkb_close;
+
+	__set_bit(EV_MSC, ppkb->input->evbit);
+	__set_bit(EV_REP, ppkb->input->evbit);
+
+	ret = matrix_keypad_build_keymap(NULL, "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",
+					 map_rows, map_cols, NULL, ppkb->input);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to build keymap\n");
+
+	ret = input_register_device(ppkb->input);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register input\n");
+
+	ret = devm_request_threaded_irq(dev, client->irq, NULL, ppkb_irq_thread,
+					IRQF_ONESHOT, client->name, client);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to request IRQ\n");
+
+	return 0;
+}
+
+static const struct of_device_id ppkb_of_match[] = {
+	{ .compatible = "pine64,pinephone-keyboard" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ppkb_of_match);
+
+static struct i2c_driver ppkb_driver = {
+	.probe_new	= ppkb_probe,
+	.driver		= {
+		.name		= DRV_NAME,
+		.of_match_table = ppkb_of_match,
+	},
+};
+module_i2c_driver(ppkb_driver);
+
+MODULE_AUTHOR("Samuel Holland <samuel@sholland.org>");
+MODULE_DESCRIPTION("Pine64 PinePhone keyboard driver");
+MODULE_LICENSE("GPL");
-- 
2.33.1

Re: [PATCH 2/5] Input: pinephone-keyboard - Add PinePhone keyboard driver

[Ondřej Jirman]

Hello Samuel,

On Sat, Jan 29, 2022 at 05:00:39PM -0600, Samuel Holland wrote:
> The official Pine64 PinePhone keyboard case contains a matrix keypad and
> a MCU which runs a libre firmware. Add support for its I2C interface.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---

> [...]

> +
> +	ppkb->buf_swap = !ppkb->buf_swap;
> +
> +	keymap = ppkb->fn_state ? ppkb->fn_keymap : ppkb->input->keycode;
> +	for (col = 0; col < ppkb->cols; ++col) {
> +		u8 old = *(++old_buf);
> +		u8 new = *(++new_buf);
> +		u8 changed = old ^ new;
> +
> +		for (row = 0; row < ppkb->rows; ++row) {
> +			int code = MATRIX_SCAN_CODE(row, col, ppkb->row_shift);
> +			int value = new & BIT(row);
> +
> +			if (!(changed & BIT(row)))
> +				continue;
> +
> +			dev_dbg(dev, "row %u col %u %sed\n",
> +				row, col, value ? "press" : "releas");
> +			if (keymap[code] == KEY_FN) {
> +				dev_dbg(dev, "FN is now %sed\n",
> +					value ? "press" : "releas");
> +				keymap = value ? ppkb->fn_keymap
> +					       : ppkb->input->keycode;
> +				ppkb->fn_state = value;
> +			}
> +			input_event(ppkb->input, EV_MSC, MSC_SCAN, code);
> +			input_report_key(ppkb->input, keymap[code], value);

I think there's a logic issue here with the Fn layer. Consider what happens
when you press Fn press F1 and then release Fn and release F1. In that case
input_report_key will report press of F1 (in fn layer) but release of '1'
which is not in Fn layer, because Fn layer was de-activated before releasing
the modified key.

From the PoV of the user, this will probably lead to auto-repeat of F1 and
spurious '1' release without preceding press event. So the userspace sees
F1 as stuck and auto-repeats it.

kind regards,
	o.

> +		}
> +	}
> +	input_sync(ppkb->input);
> +}
> +

Re: [PATCH 2/5] Input: pinephone-keyboard - Add PinePhone keyboard driver

[Ondřej Jirman]

Hello again Samuel,

On Sat, Jan 29, 2022 at 05:00:39PM -0600, Samuel Holland wrote:
> The official Pine64 PinePhone keyboard case contains a matrix keypad and
> a MCU which runs a libre firmware. Add support for its I2C interface.

I had one bug report from a user that reminded me of something that's
missing in the driver.

Please add a regulator support for power supply for the keyboard. On Pinephone
the keyboard needs USB-5V regulator to be enabled, because final version of the
keyboard is supplied from the VBAT POGO pin and not internally from kb battery,
as on the prototypes.

If this regulator is not enabled the keyboard will be supplied from battery
- voltage drop on a boost regulator diode, which causes very frequent brownouts
of the keyboard MCU.

Users don't notice, becuse MCU recovers quickly and only a key or two is missed
every once in a while, but it should be fixed. And this problem probably only
shows up when the phone battery is either quite discharged, or random load
spikes push its voltage bellow what's necessary for the MCU to run, temporarily.

kind regards,
	o.

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

[Samuel Holland]

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(-)

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

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

[Dmitry Torokhov]

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

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

[Samuel Holland]

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

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

[Pavel Machek]

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

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

[Jarrah]

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.

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

[Ondřej Jirman]

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.

[PATCH 4/5] Input: pinephone-keyboard - Support the proxied I2C bus

[Samuel Holland]

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 <samuel@sholland.org>
---

 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 <samuel@sholland.org>
 
 #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

Re: [PATCH 4/5] Input: pinephone-keyboard - Support the proxied I2C bus

[Ondřej Jirman]

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 <samuel@sholland.org>
> ---
> 
>  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 <samuel@sholland.org>
>  
>  #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
> 

Re: [PATCH 4/5] Input: pinephone-keyboard - Support the proxied I2C bus

[Samuel Holland]

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 <samuel@sholland.org>
>> ---
>>
>>  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 <samuel@sholland.org>
>>  
>>  #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
>>

Re: [PATCH 4/5] Input: pinephone-keyboard - Support the proxied I2C bus

[Ondřej Jirman]

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

[PATCH 5/5] [DO NOT MERGE] arm64: dts: allwinner: pinephone: Add keyboard

[Samuel Holland]

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 <samuel@sholland.org>
---

 .../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