diff mbox series

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

Message ID 20220129230043.12422-2-samuel@sholland.org (mailing list archive)
State Superseded
Headers show
Series Pine64 PinePhone keyboard support | expand

Commit Message

Samuel Holland Jan. 29, 2022, 11 p.m. UTC
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

Comments

Rob Herring Feb. 11, 2022, 1:23 p.m. UTC | #1
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
> 
>
Samuel Holland Feb. 14, 2022, 4:41 a.m. UTC | #2
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
>>
>>
diff mbox series

Patch

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>;
+          };
+        };
+      };
+    };