Input: keyboard - add device tree bindings for simple key matrixes

From: Dmitry Torokhov <[email protected]>

This adds a basic device tree binding for simple key matrix data.

Signed-off-by Olof Johansson <[email protected]>

---

Based on email exchange this morning, this is a first cut at a shared definition and helper function to parse and fill in the keymap data.

Instead of doing the direct parsing into the final keymap format, I chose to fill in the pdata-equivalent since that is how the OF pdata fillers work right now if code is to be kept common with the legacy platform_device probe interface.

This is a prerequisite for a revised version of the tegra-kbc device tree support that I will repost separately once this interface is stable.

(quoted diff) +0 -1

-Olof
 
 .../devicetree/bindings/input/matrix-keymap.txt    |   35 ++++++++++++
 include/linux/input/matrix_keypad.h                |   55 ++++++++++++++++++++
 2 files changed, 90 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/matrix-keymap.txt

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +0 -0

new file mode 100644
index 0000000..894f786

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +30 -0

+
+For simple keyboards with just a few buttons, you can specify each key
+as a subnode of the keyboard controller, with the following
+properties:
+
+- keypad,row: the row number to which the key is connected.
+- keypad,column: the column number to which the key is connected.
+- linux,code: the key-code to be reported when the key is pressed
+  and released.
+
+Example:
+
+	key_1 {
+		keypad,row = <0>;
+		keypad,column = <3>;
+		linux,code = <2>;
+	};
+
+
+For a more complex keyboard, such as a full laptop, a more compact
+binding can be used instead, with the following property directly in
+the keyboard controller node:
+
+- linux,keymap: an array of 3-cell entries containing the equivalent
+  of the three separate properties above: row, column and linux
+  key-code.
+
+Example:
+	linux,keymap = < 0 3 2
+			 0 4 5 >;

include/linux/input/matrix_keypad.h +1 -0

index fe7c4b9..ff13cd3 100644
@@ -3,6 +3,7 @@
 
 #include <linux/types.h>
 #include <linux/input.h>
+#include <linux/of.h>
 
 #define MATRIX_MAX_ROWS		32
 #define MATRIX_MAX_COLS		32

include/linux/input/matrix_keypad.h +12 -0

+	struct matrix_keymap_data *kd;
+	struct device_node *knp;
+	int proplen = 0, i;
+	u32 *keymap, row, col, key_code;
+	const __be32 *prop = of_get_property(np, "linux,keymap", &proplen);
+
+	if (!of_device_is_compatible(np, "matrix-keyboard-controller"))
+		return NULL;
+
+	kd = kmalloc(sizeof(*kd), GFP_KERNEL);
+	if (!kd)
+		return NULL;

include/linux/input/matrix_keypad.h +28 -0

+
+	kd->keymap = keymap;
+
+	for (i = 0; i < proplen/3; i++){
+		/* property format is < row column keycode > */
+		row = be32_to_cpup(prop++);
+		col = be32_to_cpup(prop++);
+		key_code = be32_to_cpup(prop++);
+		*keymap++ = KEY(row, col, key_code);
+	}
+
+	for_each_child_of_node(np, knp) {
+               of_property_read_u32(knp, "keypad,row", &row);
+               of_property_read_u32(knp, "keypad,column", &col);
+               of_property_read_u32(knp, "linux,code", &key_code);
+               *keymap++ = KEY(row, col, key_code);
+	}
+
+	return kd;
+}
+#else
+static inline struct matrix_keymap_data *
+matrix_keyboard_of_fill_keymap(struct device_node *np)
+{
+	return NULL;
+}
+#endif
+
 #endif /* _MATRIX_KEYPAD_H */

Olof,

Quoted 26 lines · Olof Johansson

From: Dmitry Torokhov <[email protected]>

This adds a basic device tree binding for simple key matrix data.

Signed-off-by Olof Johansson <[email protected]>

---

Based on email exchange this morning, this is a first cut at a shared definition and helper function to parse and fill in the keymap data.

Instead of doing the direct parsing into the final keymap format, I chose to fill in the pdata-equivalent since that is how the OF pdata fillers work right now if code is to be kept common with the legacy platform_device probe interface.

This is a prerequisite for a revised version of the tegra-kbc device tree support that I will repost separately once this interface is stable.

-Olof

?.../devicetree/bindings/input/matrix-keymap.txt ? ?| ? 35 ++++++++++++ ?include/linux/input/matrix_keypad.h ? ? ? ? ? ? ? ?| ? 55 ++++++++++++++++++++ ?2 files changed, 90 insertions(+), 0 deletions(-)
?create mode 100644 Documentation/devicetree/bindings/input/matrix-keymap.txt

Documentation/devicetree/bindings/input/matrix-keymap.txt line 5

Quoted 11 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +5 -0

new file mode 100644
index 0000000..894f786
@@ -0,0 +1,35 @@
+For matrix keyboards there are two kinds of layout bindings using
+linux key codes.
+
+Required properties:
+- compatible: "matrix-keyboard-controller"

Seems like this is not really needed. You've already matched the node before you call matrix_keyboard_of_fill_keymap.

Quoted 46 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +30 -0

+
+For simple keyboards with just a few buttons, you can specify each key
+as a subnode of the keyboard controller, with the following
+properties:
+
+- keypad,row: the row number to which the key is connected.
+- keypad,column: the column number to which the key is connected.
+- linux,code: the key-code to be reported when the key is pressed
+ ?and released.
+
+Example:
+
+ ? ? key_1 {
+ ? ? ? ? ? ? keypad,row = <0>;
+ ? ? ? ? ? ? keypad,column = <3>;
+ ? ? ? ? ? ? linux,code = <2>;
+ ? ? };
+
+
+For a more complex keyboard, such as a full laptop, a more compact
+binding can be used instead, with the following property directly in
+the keyboard controller node:
+
+- linux,keymap: an array of 3-cell entries containing the equivalent
+ ?of the three separate properties above: row, column and linux
+ ?key-code.
+
+Example:
+ ? ? linux,keymap = < 0 3 2
+ ? ? ? ? ? ? ? ? ? ? ?0 4 5 >;

include/linux/input/matrix_keypad.h +0 -0

index fe7c4b9..ff13cd3 100644
@@ -3,6 +3,7 @@
 

?#include <linux/types.h>
?#include <linux/input.h>
+#include <linux/of.h>

?#define MATRIX_MAX_ROWS ? ? ? ? ? ? ?32
?#define MATRIX_MAX_COLS ? ? ? ? ? ? ?32

include/linux/input/matrix_keypad.h +0 -0

@@ -106,4 +107,58 @@ matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 ? ? ? __clear_bit(KEY_RESERVED, keybit);
 ?}
 

include/linux/input/matrix_keypad.h matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data, · line 113

Quoted 4 lines

include/linux/input/matrix_keypad.h +4 -0

+#ifdef CONFIG_OF
+static inline struct matrix_keymap_data *
+matrix_keyboard_of_fill_keymap(struct device_node *np)
+{

Seems a bit large to inline.

Quoted 19 lines

include/linux/input/matrix_keypad.h +19 -0

+ ? ? struct matrix_keymap_data *kd;
+ ? ? struct device_node *knp;
+ ? ? int proplen = 0, i;
+ ? ? u32 *keymap, row, col, key_code;
+ ? ? const __be32 *prop = of_get_property(np, "linux,keymap", &proplen);
+
+ ? ? if (!of_device_is_compatible(np, "matrix-keyboard-controller"))
+ ? ? ? ? ? ? return NULL;
+
+ ? ? kd = kmalloc(sizeof(*kd), GFP_KERNEL);
+ ? ? if (!kd)
+ ? ? ? ? ? ? return NULL;
+ ? ? kd->keymap_size = proplen / 3;
+
+ ? ? for_each_child_of_node(np, knp)
+ ? ? ? ? ? ? kd->keymap_size++;
+
+ ? ? keymap = kzalloc(kd->keymap_size * sizeof(u32), GFP_KERNEL);
+ ? ? if (!keymap) {

include/linux/input/matrix_keypad.h matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data, · line 135

Quoted 3 lines

include/linux/input/matrix_keypad.h +3 -0

+ ? ? ? ? ? ? kfree(kd);
+ ? ? ? ? ? ? return NULL;
+ ? ? }

Looks like memory leaks would be very likely. Is the caller expected to free these? If so, a matching free function should be provided. Perhaps trying to keep platform_data compatibility is not the best approach if it would simplify this.

From: Dmitry Torokhov <[email protected]>

This adds a basic device tree binding for simple key matrix data.

Signed-off-by Olof Johansson <[email protected]>

---

Based on email exchange this morning, this is a first cut at a shared definition and helper function to parse and fill in the keymap data.

Instead of doing the direct parsing into the final keymap format, I chose to fill in the pdata-equivalent since that is how the OF pdata fillers work right now if code is to be kept common with the legacy platform_device probe interface.

This is a prerequisite for a revised version of the tegra-kbc device tree support that I will repost separately once this interface is stable.

(quoted diff) +0 -0

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +15 -0

+For simple keyboards with just a few buttons, you can specify each key
+as a subnode of the keyboard controller, with the following
+properties:
+
+- keypad,row: the row number to which the key is connected.
+- keypad,column: the column number to which the key is connected.
+- linux,code: the key-code to be reported when the key is pressed
+  and released.
+
+Example:
+
+	key_1 {
+		keypad,row = <0>;
+		keypad,column = <3>;
+		linux,code = <2>;

Olof Johansson wrote at Wednesday, December 28, 2011 3:53 PM:

Quoted 17 lines

From: Dmitry Torokhov <[email protected]>

This adds a basic device tree binding for simple key matrix data.

Signed-off-by Olof Johansson <[email protected]>

---

Based on email exchange this morning, this is a first cut at a shared definition and helper function to parse and fill in the keymap data.

Instead of doing the direct parsing into the final keymap format, I chose to fill in the pdata-equivalent since that is how the OF pdata fillers work right now if code is to be kept common with the legacy platform_device probe interface.

This is a prerequisite for a revised version of the tegra-kbc device tree support that I will repost separately once this interface is stable.

...

Quoted 1 line

(quoted diff) +0 -0

...

Quoted 20 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +20 -0

+For simple keyboards with just a few buttons, you can specify each key
+as a subnode of the keyboard controller, with the following
+properties:
+
+- keypad,row: the row number to which the key is connected.
+- keypad,column: the column number to which the key is connected.
+- linux,code: the key-code to be reported when the key is pressed
+ ?and released.
+
+Example:
+
+ ? ? key_1 {
+ ? ? ? ? ? ? keypad,row = <0>;
+ ? ? ? ? ? ? keypad,column = <3>;
+ ? ? ? ? ? ? linux,code = <2>;
+ ? ? };
+
+
+For a more complex keyboard, such as a full laptop, a more compact
+binding can be used instead, with the following property directly in

Documentation/devicetree/bindings/input/matrix-keymap.txt

Quoted 5 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +5 -0

+the keyboard controller node:
+
+- linux,keymap: an array of 3-cell entries containing the equivalent
+ ?of the three separate properties above: row, column and linux
+ ?key-code.

Why allow two completely different bindings? Is there some deficiency to the compact binding that means it isn't suitable for all cases?

The binding proposed by Simon Glass for U-Boot was just a matrix of keycodes, with any unused locations containing zero, e.g. see:

http://patchwork.ozlabs.org/patch/129825/

... which is yet another option. We obviously don't want U-Boot and the kernel to expect different DT content either.

Quoted 41 lines

Olof Johansson wrote at Wednesday, December 28, 2011 3:53 PM:

Quoted 17 lines

From: Dmitry Torokhov <[email protected]>

This adds a basic device tree binding for simple key matrix data.

Signed-off-by Olof Johansson <[email protected]>

---

Based on email exchange this morning, this is a first cut at a shared definition and helper function to parse and fill in the keymap data.

Instead of doing the direct parsing into the final keymap format, I chose to fill in the pdata-equivalent since that is how the OF pdata fillers work right now if code is to be kept common with the legacy platform_device probe interface.

This is a prerequisite for a revised version of the tegra-kbc device tree support that I will repost separately once this interface is stable.

...

Quoted 1 line

(quoted diff) +0 -0

...

Quoted 20 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +20 -0

+For simple keyboards with just a few buttons, you can specify each key
+as a subnode of the keyboard controller, with the following
+properties:
+
+- keypad,row: the row number to which the key is connected.
+- keypad,column: the column number to which the key is connected.
+- linux,code: the key-code to be reported when the key is pressed
+ ?and released.
+
+Example:
+
+ ? ? key_1 {
+ ? ? ? ? ? ? keypad,row = <0>;
+ ? ? ? ? ? ? keypad,column = <3>;
+ ? ? ? ? ? ? linux,code = <2>;
+ ? ? };
+
+
+For a more complex keyboard, such as a full laptop, a more compact
+binding can be used instead, with the following property directly in

Quoted 8 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt

Quoted 5 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +5 -0

+the keyboard controller node:
+
+- linux,keymap: an array of 3-cell entries containing the equivalent
+ ?of the three separate properties above: row, column and linux
+ ?key-code.

Why allow two completely different bindings? Is there some deficiency to the compact binding that means it isn't suitable for all cases?

The main reason is that the samsung keyboard driver already implements the more verbose one, and allowing that binding to coexist while also providing a more compact one seems like the right thing to do.

Olof Johansson wrote at Wednesday, December 28, 2011 11:34 PM:

Quoted 51 lines · Stephen Warren

Quoted 49 lines

Olof Johansson wrote at Wednesday, December 28, 2011 3:53 PM:

Quoted 17 lines

From: Dmitry Torokhov <[email protected]>

This adds a basic device tree binding for simple key matrix data.

Signed-off-by Olof Johansson <[email protected]>

---

Based on email exchange this morning, this is a first cut at a shared definition and helper function to parse and fill in the keymap data.

Instead of doing the direct parsing into the final keymap format, I chose to fill in the pdata-equivalent since that is how the OF pdata fillers work right now if code is to be kept common with the legacy platform_device probe interface.

This is a prerequisite for a revised version of the tegra-kbc device tree support that I will repost separately once this interface is stable.

...

Quoted 1 line

(quoted diff) +0 -0

...

Quoted 15 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +15 -0

+For simple keyboards with just a few buttons, you can specify each key
+as a subnode of the keyboard controller, with the following
+properties:
+
+- keypad,row: the row number to which the key is connected.
+- keypad,column: the column number to which the key is connected.
+- linux,code: the key-code to be reported when the key is pressed
+ ?and released.
+
+Example:
+
+ ? ? key_1 {
+ ? ? ? ? ? ? keypad,row = <0>;
+ ? ? ? ? ? ? keypad,column = <3>;
+ ? ? ? ? ? ? linux,code = <2>;

Documentation/devicetree/bindings/input/matrix-keymap.txt

Quoted 10 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +10 -0

+ ? ? };
+
+
+For a more complex keyboard, such as a full laptop, a more compact
+binding can be used instead, with the following property directly in
+the keyboard controller node:
+
+- linux,keymap: an array of 3-cell entries containing the equivalent
+ ?of the three separate properties above: row, column and linux
+ ?key-code.

Why allow two completely different bindings? Is there some deficiency to the compact binding that means it isn't suitable for all cases?

Quoted 3 lines

The main reason is that the samsung keyboard driver already implements the more verbose one, and allowing that binding to coexist while also providing a more compact one seems like the right thing to do.

Can we deprecate the Samsung format, and only allow it for that Samsung device (and allow both there), and require a single format for any other keyboard?

The issue here is that U-Boot wants to stay small, and having to allow two alternative methods to specify a keyboard is going to bloat the code; I expect some pushback adding DT support for just one binding by the time they see all the DT parsing code that's needed to do it all right.

Quoted 56 lines · Stephen Warren

Olof Johansson wrote at Wednesday, December 28, 2011 11:34 PM:

Quoted 54 lines · Stephen Warren

Quoted 41 lines

Olof Johansson wrote at Wednesday, December 28, 2011 3:53 PM:

Quoted 17 lines

From: Dmitry Torokhov <[email protected]>

This adds a basic device tree binding for simple key matrix data.

Signed-off-by Olof Johansson <[email protected]>

---

Based on email exchange this morning, this is a first cut at a shared definition and helper function to parse and fill in the keymap data.

Instead of doing the direct parsing into the final keymap format, I chose to fill in the pdata-equivalent since that is how the OF pdata fillers work right now if code is to be kept common with the legacy platform_device probe interface.

This is a prerequisite for a revised version of the tegra-kbc device tree support that I will repost separately once this interface is stable.

...

Quoted 1 line

(quoted diff) +0 -0

...

Quoted 20 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +20 -0

+For simple keyboards with just a few buttons, you can specify each key
+as a subnode of the keyboard controller, with the following
+properties:
+
+- keypad,row: the row number to which the key is connected.
+- keypad,column: the column number to which the key is connected.
+- linux,code: the key-code to be reported when the key is pressed
+  and released.
+
+Example:
+
+     key_1 {
+             keypad,row = <0>;
+             keypad,column = <3>;
+             linux,code = <2>;
+     };
+
+
+For a more complex keyboard, such as a full laptop, a more compact
+binding can be used instead, with the following property directly in

Quoted 8 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt line 31

Quoted 5 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +5 -0

+the keyboard controller node:
+
+- linux,keymap: an array of 3-cell entries containing the equivalent
+  of the three separate properties above: row, column and linux
+  key-code.

Why allow two completely different bindings? Is there some deficiency to the compact binding that means it isn't suitable for all cases?

The main reason is that the samsung keyboard driver already implements the more verbose one, and allowing that binding to coexist while also providing a more compact one seems like the right thing to do.

Quoted 3 lines

Can we deprecate the Samsung format, and only allow it for that Samsung device (and allow both there), and require a single format for any other keyboard?

I'm definitely ok with that. Thomas, Grant, Rob? The code in question is queued for 3.3, so it hasn't been out in a real release yet.

Adding Kukjin as well since it's getting merged through his tree.

Quoted 3 lines

Can we deprecate the Samsung format, and only allow it for that Samsung device (and allow both there), and require a single format for any other keyboard?

I'm definitely ok with that. Thomas, Grant, Rob? The code in question is queued for 3.3, so it hasn't been out in a real release yet.

Adding Kukjin as well since it's getting merged through his tree.

Quoted 3 lines · Stephen Warren

Can we deprecate the Samsung format, and only allow it for that Samsung device (and allow both there), and require a single format for any other keyboard?

I'm definitely ok with that. Thomas, Grant, Rob? The code in question is queued for 3.3, so it hasn't been out in a real release yet.

Adding Kukjin as well since it's getting merged through his tree.

Quoted 9 lines · Olof Johansson

Quoted 3 lines · Stephen Warren

Can we deprecate the Samsung format, and only allow it for that Samsung device (and allow both there), and require a single format for any other keyboard?

I'm definitely ok with that. Thomas, Grant, Rob? The code in question is queued for 3.3, so it hasn't been out in a real release yet.

Adding Kukjin as well since it's getting merged through his tree.

Yeah, I'm okay with that.

BTW, please drop the "compatible" value for this binding. ?This binding describes a common way of describing key mappings, but it isn't a complete device binding in and of itself. ?Rather, it is a binding used by other bindings; and as such no driver should ever bind against it directly. ?The samsung driver can continue to use the other format if it so desires; there is no harm in it doing so.

Instead, a driver that expects the binding can just call the matrix_keyboard_of_fill_keymap() library function without checking the compatible list.

This adds a simple device tree binding for simple key matrix data.

Changes since v1:
* Removed samsung-style binding support
* Added linux,fn-keymap and linux,fn-key optional properties

Signed-off-by Olof Johansson <[email protected]>

---
 .../devicetree/bindings/input/matrix-keymap.txt    |   25 ++++++
 drivers/input/keyboard/Makefile                    |    1 +
 drivers/input/keyboard/keymap.c                    |   78 ++++++++++++++++++++
 include/linux/input/matrix_keypad.h                |   34 +++++----
 4 files changed, 123 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/matrix-keymap.txt
 create mode 100644 drivers/input/keyboard/keymap.c

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +0 -0

new file mode 100644
index 0000000..480ca46

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +16 -0

+Optional properties:
+- linux,fn-keymap: A separate array of 3-cell entries similar to
+  linux,keymap but only to be used when the function key modifier is
+  active. This is used for keyboards that have a software-based modifier
+  key for 'Fn' but that need to describe the custom layout that should
+  be used when said modifier key is active.
+
+- linux,fn-key: a 2-cell entry containing the < row column > of the
+  function modifier key used to switch to the above linux,fn-keymap
+  layout.
+
+Example:
+	linux,keymap = < 0 3 2
+			 0 4 5 >;
+	linux,fn-key = < 2 1 >;
+	linux,fn-keymap = < 0 3 1 >;

drivers/input/keyboard/Makefile +1 -0

index df7061f..83e6eed 100644
@@ -4,6 +4,7 @@
 
 # Each configuration option enables a list of files.
 
+obj-$(CONFIG_INPUT_KEYBOARD)		+= keymap.o
 obj-$(CONFIG_KEYBOARD_ADP5520)		+= adp5520-keys.o
 obj-$(CONFIG_KEYBOARD_ADP5588)		+= adp5588-keys.o
 obj-$(CONFIG_KEYBOARD_ADP5589)		+= adp5589-keys.o

drivers/input/keyboard/keymap.c new file +78 -0

new file mode 100644
index 0000000..80d1074
@@ -0,0 +1,78 @@
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/input.h>
+#include <linux/of.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/export.h>
+#include <linux/gfp.h>
+#include <linux/slab.h>
+
+void matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
+				unsigned int row_shift,
+				unsigned short *keymap, unsigned long *keybit)
+{
+	int i;
+
+	for (i = 0; i < keymap_data->keymap_size; i++) {
+		unsigned int key = keymap_data->keymap[i];
+		unsigned int row = KEY_ROW(key);
+		unsigned int col = KEY_COL(key);
+		unsigned short code = KEY_VAL(key);
+
+		keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
+		__set_bit(code, keybit);
+	}
+	__clear_bit(KEY_RESERVED, keybit);
+}
+EXPORT_SYMBOL_GPL(matrix_keypad_build_keymap);
+
+#ifdef CONFIG_OF
+struct matrix_keymap_data *
+matrix_keyboard_of_fill_keymap(struct device_node *np)
+{
+	struct matrix_keymap_data *kd;
+	struct device_node *knp;
+	int proplen = 0, i;
+	u32 *keymap, row, col, key_code;
+	const __be32 *prop = of_get_property(np, "linux,keymap", &proplen);
+
+	if (!of_device_is_compatible(np, "matrix-keyboard-controller"))
+		return NULL;
+
+	kd = kmalloc(sizeof(*kd), GFP_KERNEL);
+	if (!kd)
+		return NULL;
+	kd->keymap_size = proplen / 3;
+
+	keymap = kzalloc(kd->keymap_size * sizeof(u32), GFP_KERNEL);
+	if (!keymap) {
+		kfree(kd);
+		return NULL;
+	}
+
+	kd->keymap = keymap;
+
+	for (i = 0; i < kd->keymap_size; i++){
+		/* property format is < row column keycode > */
+		row = be32_to_cpup(prop++);
+		col = be32_to_cpup(prop++);
+		key_code = be32_to_cpup(prop++);
+		*keymap++ = KEY(row, col, key_code);
+	}
+
+	/* FIXME: Need to add handling of the linux,fn-keymap property here */
+
+	return kd;
+}
+EXPORT_SYMBOL_GPL(matrix_keyboard_of_fill_keymap);
+
+void matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd)
+{
+	if (!kd)
+		return;
+	if (kd->keymap)
+		kfree(kd->keymap);
+	kfree(kd);
+}
+EXPORT_SYMBOL_GPL(matrix_keyboard_of_free_keymap);
+#endif

include/linux/input/matrix_keypad.h +19 -15

index fe7c4b9..ac999c6 100644
@@ -3,6 +3,7 @@
 
 #include <linux/types.h>
 #include <linux/input.h>
+#include <linux/of.h>
 
 #define MATRIX_MAX_ROWS		32
 #define MATRIX_MAX_COLS		32

@@ -87,23 +88,26 @@ struct matrix_keypad_platform_data {
  * an array of keycodes that is suitable for using in a standard matrix
  * keyboard driver that uses row and col as indices.
  */
+void matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
+				unsigned int row_shift,
+				unsigned short *keymap, unsigned long *keybit);
+
+#ifdef CONFIG_OF
+struct matrix_keymap_data *
+matrix_keyboard_of_fill_keymap(struct device_node *np);
+
+void matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd);
+#else
+static inline struct matrix_keymap_data *
+matrix_keyboard_of_fill_keymap(struct device_node *np)
+{
+	return NULL;
+}
+
 static inline void
-matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
-			   unsigned int row_shift,
-			   unsigned short *keymap, unsigned long *keybit)
+matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd)
 {
-	int i;
-
-	for (i = 0; i < keymap_data->keymap_size; i++) {
-		unsigned int key = keymap_data->keymap[i];
-		unsigned int row = KEY_ROW(key);
-		unsigned int col = KEY_COL(key);
-		unsigned short code = KEY_VAL(key);
-
-		keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
-		__set_bit(code, keybit);
-	}
-	__clear_bit(KEY_RESERVED, keybit);
 }
+#endif
 
 #endif /* _MATRIX_KEYPAD_H */

Quoted 16 lines · Olof Johansson

This adds a simple device tree binding for simple key matrix data.

Changes since v1:
?* Removed samsung-style binding support
?* Added linux,fn-keymap and linux,fn-key optional properties

Signed-off-by Olof Johansson <[email protected]>

---

?.../devicetree/bindings/input/matrix-keymap.txt ? ?| ? 25 ++++++
?drivers/input/keyboard/Makefile ? ? ? ? ? ? ? ? ? ?| ? ?1 +
?drivers/input/keyboard/keymap.c ? ? ? ? ? ? ? ? ? ?| ? 78 ++++++++++++++++++++
?include/linux/input/matrix_keypad.h ? ? ? ? ? ? ? ?| ? 34 +++++----

?4 files changed, 123 insertions(+), 15 deletions(-)
?create mode 100644 Documentation/devicetree/bindings/input/matrix-keymap.txt ?create mode 100644 drivers/input/keyboard/keymap.c

Documentation/devicetree/bindings/input/matrix-keymap.txt line 2

Quoted 8 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +2 -0

new file mode 100644
index 0000000..480ca46
@@ -0,0 +1,25 @@
+For matrix keyboards there are two kinds of layout bindings using
+linux key codes.

Need to update this.

Quoted 6 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +6 -0

+
+Required properties:
+- compatible: "matrix-keyboard-controller"
+- linux,keymap: an array of 3-cell entries containing the equivalent
+ ?of the three separate properties above: row, column and linux
+ ?key-code.

Documentation/devicetree/bindings/input/matrix-keymap.txt line 9

Quoted 1 line

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +1 -0

+

How about just embedding the row/column information as the array position in the keymap? Then you only need 1 cell per key. Sparsely populated matrices are probably the exception. If more than 1/3 of the keys are present, you come out ahead in terms of data usage. You would need to specify the number of rows and columns, but that is probably easier than determining the max from the keymap.

This adds a simple device tree binding for simple key matrix data.

Changes since v1:
* Removed samsung-style binding support
* Added linux,fn-keymap and linux,fn-key optional properties

Signed-off-by Olof Johansson <[email protected]>

---
 .../devicetree/bindings/input/matrix-keymap.txt    |   25 ++++++
 drivers/input/keyboard/Makefile                    |    1 +
 drivers/input/keyboard/keymap.c                    |   78 ++++++++++++++++++++
 include/linux/input/matrix_keypad.h                |   34 +++++----
 4 files changed, 123 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/matrix-keymap.txt
 create mode 100644 drivers/input/keyboard/keymap.c

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +25 -0

new file mode 100644
index 0000000..480ca46
@@ -0,0 +1,25 @@
+For matrix keyboards there are two kinds of layout bindings using
+linux key codes.
+
+Required properties:
+- compatible: "matrix-keyboard-controller"
+- linux,keymap: an array of 3-cell entries containing the equivalent
+  of the three separate properties above: row, column and linux
+  key-code.
+
+Optional properties:
+- linux,fn-keymap: A separate array of 3-cell entries similar to
+  linux,keymap but only to be used when the function key modifier is
+  active. This is used for keyboards that have a software-based modifier
+  key for 'Fn' but that need to describe the custom layout that should
+  be used when said modifier key is active.
+
+- linux,fn-key: a 2-cell entry containing the < row column > of the
+  function modifier key used to switch to the above linux,fn-keymap
+  layout.
+
+Example:
+	linux,keymap = < 0 3 2
+			 0 4 5 >;
+	linux,fn-key = < 2 1 >;
+	linux,fn-keymap = < 0 3 1 >;

drivers/input/keyboard/Makefile +0 -0

index df7061f..83e6eed 100644

drivers/input/keyboard/Makefile +0 -0

 obj-$(CONFIG_KEYBOARD_ADP5520)		+= adp5520-keys.o
 obj-$(CONFIG_KEYBOARD_ADP5588)		+= adp5588-keys.o
 obj-$(CONFIG_KEYBOARD_ADP5589)		+= adp5589-keys.o

drivers/input/keyboard/keymap.c new file +20 -0

new file mode 100644
index 0000000..80d1074
@@ -0,0 +1,78 @@
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/input.h>
+#include <linux/of.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/export.h>
+#include <linux/gfp.h>
+#include <linux/slab.h>
+
+void matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
+				unsigned int row_shift,
+				unsigned short *keymap, unsigned long *keybit)
+{
+	int i;
+
+	for (i = 0; i < keymap_data->keymap_size; i++) {
+		unsigned int key = keymap_data->keymap[i];
+		unsigned int row = KEY_ROW(key);
+		unsigned int col = KEY_COL(key);
+		unsigned short code = KEY_VAL(key);

drivers/input/keyboard/keymap.c new file +19 -0

+
+	keymap = kzalloc(kd->keymap_size * sizeof(u32), GFP_KERNEL);
+	if (!keymap) {
+		kfree(kd);
+		return NULL;
+	}
+
+	kd->keymap = keymap;
+
+	for (i = 0; i < kd->keymap_size; i++){
+		/* property format is < row column keycode > */
+		row = be32_to_cpup(prop++);
+		col = be32_to_cpup(prop++);
+		key_code = be32_to_cpup(prop++);
+		*keymap++ = KEY(row, col, key_code);
+	}
+
+	/* FIXME: Need to add handling of the linux,fn-keymap property here */
+

drivers/input/keyboard/keymap.c new file +4 -0

+	kfree(kd);
+}
+EXPORT_SYMBOL_GPL(matrix_keyboard_of_free_keymap);
+#endif

include/linux/input/matrix_keypad.h +19 -15

index fe7c4b9..ac999c6 100644
@@ -3,6 +3,7 @@
 
 #include <linux/types.h>
 #include <linux/input.h>
+#include <linux/of.h>
 
 #define MATRIX_MAX_ROWS		32
 #define MATRIX_MAX_COLS		32

@@ -87,23 +88,26 @@ struct matrix_keypad_platform_data {
  * an array of keycodes that is suitable for using in a standard matrix
  * keyboard driver that uses row and col as indices.
  */
+void matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
+				unsigned int row_shift,
+				unsigned short *keymap, unsigned long *keybit);
+
+#ifdef CONFIG_OF
+struct matrix_keymap_data *
+matrix_keyboard_of_fill_keymap(struct device_node *np);
+
+void matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd);
+#else
+static inline struct matrix_keymap_data *
+matrix_keyboard_of_fill_keymap(struct device_node *np)
+{
+	return NULL;
+}
+
 static inline void
-matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
-			   unsigned int row_shift,
-			   unsigned short *keymap, unsigned long *keybit)
+matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd)
 {
-	int i;
-
-	for (i = 0; i < keymap_data->keymap_size; i++) {
-		unsigned int key = keymap_data->keymap[i];
-		unsigned int row = KEY_ROW(key);
-		unsigned int col = KEY_COL(key);
-		unsigned short code = KEY_VAL(key);
-
-		keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
-		__set_bit(code, keybit);
-	}
-	__clear_bit(KEY_RESERVED, keybit);
 }
+#endif
 
 #endif /* _MATRIX_KEYPAD_H */

Signature · 1 line

1.7.8.GIT

Hi Olof,

Quoted 6 lines · Olof Johansson

This adds a simple device tree binding for simple key matrix data.

Changes since v1:
?* Removed samsung-style binding support
?* Added linux,fn-keymap and linux,fn-key optional properties

This looks very reasonable to me, a few comments though.

Quoted 49 lines

Signed-off-by Olof Johansson <[email protected]>

---

?.../devicetree/bindings/input/matrix-keymap.txt ? ?| ? 25 ++++++
?drivers/input/keyboard/Makefile ? ? ? ? ? ? ? ? ? ?| ? ?1 +
?drivers/input/keyboard/keymap.c ? ? ? ? ? ? ? ? ? ?| ? 78 ++++++++++++++++++++
?include/linux/input/matrix_keypad.h ? ? ? ? ? ? ? ?| ? 34 +++++----

?4 files changed, 123 insertions(+), 15 deletions(-)
?create mode 100644 Documentation/devicetree/bindings/input/matrix-keymap.txt ?create mode 100644 drivers/input/keyboard/keymap.c

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +25 -0

new file mode 100644
index 0000000..480ca46
@@ -0,0 +1,25 @@
+For matrix keyboards there are two kinds of layout bindings using
+linux key codes.
+
+Required properties:
+- compatible: "matrix-keyboard-controller"
+- linux,keymap: an array of 3-cell entries containing the equivalent
+ ?of the three separate properties above: row, column and linux
+ ?key-code.
+
+Optional properties:
+- linux,fn-keymap: A separate array of 3-cell entries similar to
+ ?linux,keymap but only to be used when the function key modifier is
+ ?active. This is used for keyboards that have a software-based modifier
+ ?key for 'Fn' but that need to describe the custom layout that should
+ ?be used when said modifier key is active.
+
+- linux,fn-key: a 2-cell entry containing the < row column > of the
+ ?function modifier key used to switch to the above linux,fn-keymap
+ ?layout.
+
+Example:
+ ? ? linux,keymap = < 0 3 2
+ ? ? ? ? ? ? ? ? ? ? ?0 4 5 >;
+ ? ? linux,fn-key = < 2 1 >;
+ ? ? linux,fn-keymap = < 0 3 1 >;

drivers/input/keyboard/Makefile +0 -0

index df7061f..83e6eed 100644
@@ -4,6 +4,7 @@
 

?# Each configuration option enables a list of files.

Quoted 1 line

+obj-$(CONFIG_INPUT_KEYBOARD) ? ? ? ? += keymap.o

I do not think we should restrict it keyboard devices only; there could be users in input/misc as well, so I'd rather have it as drivers/input/matrix-keymap.c

I'd start with just matrix_keyboard_of_build_keymap() there and moved matrix_keypad_build_keymap() as a follow-up patch as drivers using it should 'select' it.

Quoted 34 lines

?obj-$(CONFIG_KEYBOARD_ADP5520) ? ? ? ? ? ? ? += adp5520-keys.o ?obj-$(CONFIG_KEYBOARD_ADP5588) ? ? ? ? ? ? ? += adp5588-keys.o ?obj-$(CONFIG_KEYBOARD_ADP5589) ? ? ? ? ? ? ? += adp5589-keys.o

drivers/input/keyboard/keymap.c new file +25 -0

new file mode 100644
index 0000000..80d1074
@@ -0,0 +1,78 @@
+#include <linux/kernel.h>
+#include <linux/types.h>
+#include <linux/input.h>
+#include <linux/of.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/export.h>
+#include <linux/gfp.h>
+#include <linux/slab.h>
+
+void matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned int row_shift,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? unsigned short *keymap, unsigned long *keybit)
+{
+ ? ? int i;
+
+ ? ? for (i = 0; i < keymap_data->keymap_size; i++) {
+ ? ? ? ? ? ? unsigned int key = keymap_data->keymap[i];
+ ? ? ? ? ? ? unsigned int row = KEY_ROW(key);
+ ? ? ? ? ? ? unsigned int col = KEY_COL(key);
+ ? ? ? ? ? ? unsigned short code = KEY_VAL(key);
+
+ ? ? ? ? ? ? keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
+ ? ? ? ? ? ? __set_bit(code, keybit);
+ ? ? }
+ ? ? __clear_bit(KEY_RESERVED, keybit);

drivers/input/keyboard/keymap.c line 30

Quoted 5 lines

drivers/input/keyboard/keymap.c new file +5 -0

+}
+EXPORT_SYMBOL_GPL(matrix_keypad_build_keymap);
+
+#ifdef CONFIG_OF
+struct matrix_keymap_data *

Instead of allocating keymap data why don't we populate keymap and keybits directly, like matrix_keypad_build_keymap() does?

drivers/input/keyboard/keymap.c line 36

Quoted 6 lines

drivers/input/keyboard/keymap.c new file +6 -0

+matrix_keyboard_of_fill_keymap(struct device_node *np)
+{
+ ? ? struct matrix_keymap_data *kd;
+ ? ? struct device_node *knp;
+ ? ? int proplen = 0, i;
+ ? ? u32 *keymap, row, col, key_code;

Let's fail safely if np is NULL.

drivers/input/keyboard/keymap.c line 45

Quoted 9 lines

drivers/input/keyboard/keymap.c new file +9 -0

+ ? ? const __be32 *prop = of_get_property(np, "linux,keymap", &proplen);
+
+ ? ? if (!of_device_is_compatible(np, "matrix-keyboard-controller"))
+ ? ? ? ? ? ? return NULL;
+
+ ? ? kd = kmalloc(sizeof(*kd), GFP_KERNEL);
+ ? ? if (!kd)
+ ? ? ? ? ? ? return NULL;
+ ? ? kd->keymap_size = proplen / 3;

Validate that proplen % 3 == 0?

Quoted 24 lines

drivers/input/keyboard/keymap.c new file +24 -0

+
+ ? ? keymap = kzalloc(kd->keymap_size * sizeof(u32), GFP_KERNEL);
+ ? ? if (!keymap) {
+ ? ? ? ? ? ? kfree(kd);
+ ? ? ? ? ? ? return NULL;
+ ? ? }
+
+ ? ? kd->keymap = keymap;
+
+ ? ? for (i = 0; i < kd->keymap_size; i++){
+ ? ? ? ? ? ? /* property format is < row column keycode > */
+ ? ? ? ? ? ? row = be32_to_cpup(prop++);
+ ? ? ? ? ? ? col = be32_to_cpup(prop++);
+ ? ? ? ? ? ? key_code = be32_to_cpup(prop++);
+ ? ? ? ? ? ? *keymap++ = KEY(row, col, key_code);
+ ? ? }
+
+ ? ? /* FIXME: Need to add handling of the linux,fn-keymap property here */
+
+ ? ? return kd;
+}
+EXPORT_SYMBOL_GPL(matrix_keyboard_of_fill_keymap);
+
+void matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd)

drivers/input/keyboard/keymap.c line 74

Quoted 5 lines

drivers/input/keyboard/keymap.c new file +5 -0

+{
+ ? ? if (!kd)
+ ? ? ? ? ? ? return;
+ ? ? if (kd->keymap)
+ ? ? ? ? ? ? kfree(kd->keymap);

If we decide to keep allocating kd then checking kd->keymap before freeing is not necessary.

This adds a simple device tree binding for simple key matrix data and a helper to fill in the platform data.

The implementation is in a shared file outside if drivers/input/keyboard since some drivers in drivers/input/misc might be making use of it as well.

Changes since v2:
?* Removed reference to "legacy" format with a subnode per key ?* Changed to a packed format with one cell per key instead of 3. ?* Moved new OF helper to separate file
?* Misc fixups based on comments

Changes since v1:
?* Removed samsung-style binding support
?* Added linux,fn-keymap and linux,fn-key optional properties

Signed-off-by Olof Johansson <[email protected]>

---

?.../devicetree/bindings/input/matrix-keymap.txt ? ?| ? 27 ++++++
?drivers/input/Kconfig ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?4 +
?drivers/input/Makefile ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ? ?1 +
?drivers/input/keyboard/Kconfig ? ? ? ? ? ? ? ? ? ? | ? ?1 +
?drivers/input/of_keymap.c ? ? ? ? ? ? ? ? ? ? ? ? ?| ? 87 ++++++++++++++++++++
?include/linux/input/matrix_keypad.h ? ? ? ? ? ? ? ?| ? 19 ++++

?6 files changed, 139 insertions(+), 0 deletions(-)
?create mode 100644 Documentation/devicetree/bindings/input/matrix-keymap.txt ?create mode 100644 drivers/input/of_keymap.c

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +0 -0

new file mode 100644
index 0000000..1db8e12

From a point of view of efficiency, drivers can simply keep a pointer

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +7 -0

+
+Optional properties:
+- linux,fn-keymap: A separate array of packed 1-cell entries similar to
+ ?linux,keymap but only to be used when the function key modifier is
+ ?active. This is used for keyboards that have a software-based modifier
+ ?key for 'Fn' but that need to describe the custom layout that should
+ ?be used when said modifier key is active.

Quoted 6 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +0 -0

new file mode 100644
index 0000000..1db8e12

Documentation/devicetree/bindings/input/matrix-keymap.txt line 10

Quoted 10 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +10 -0

@@ -0,0 +1,27 @@
+A simple common binding for matrix-connected key boards. Currently targeted at
+defining the keys in the scope of linux key codes since that is a stable and
+standardized interface at this time.
+
+Required properties:
+- compatible: "matrix-keyboard-controller"
+- linux,keymap: an array of packed 1-cell entries containing the equivalent
+ ?of row, column and linux key-code. The 32-bit big endian cell is packed
+ ?as:
+ ? ? ? row << 24 | column << 16 | key-code

This looks much better than the Samsung binding and the original 3-cell one.

U-Boot Tegra (keyboard patch series) currently uses a 16x8 matrix, and uses a single byte (the ASCII character) for a total of 128 bytes per keymap. Since U-Boot does not have (or apparently want) the concept of key codes or the added code and intermediate data this requires, the binding presented here will not suit U-Boot so far.

These rows and columns embedded in the cell make it more of a pain to write the fdt description. If you just set up the cells in (column, row) order and set the matrix size (rows, columns) then you end up with 128 entries on Tegra. If you use uint16 then this could be 256 bytes instead of 512. The binding you present for Tegra would be 109 * 4 = 436 bytes. However I take your point that Fn maps are much more sparse, so overall this is likely to be similar (512 bytes for either method once Fn mappings are taken into account).

But going back to U-Boot, it does not have the intermediate table that you malloc and decant into, it does not understand key codes so doesn't know what to do when Shift is pressed, doesn't understand languages, etc. In order to use these Linux bindings in U-Boot we would need to add new input layer, extra decode code and intermediate tables. I can almost hear the NAKs already (bear in mind fdt only went into U-Boot in the December release and people are more sensitive to code size / performance there than in Linux). I did ask about this on this list a few weeks ago but no response yet.

We could perhaps add an alternative direct ASCII binding like this example (which would have to be in a separate node):

                /* Use a packed binding which doesn't include
row,column numbers in each cell */
                packed-binding;
                matrix-columns = <8>;
                matrix-rows = <16>;
                ascii-binding;  /* ASCII characters instead of keycodes */
		u-boot,keymap  = /size/ 8 <00  00  'w' 's' 'a' 'z' 00  DE
				    00  00  00  00  00  00  00  00
				    00  00  00  00  00  00  00  00
				    '5' '4' 'r' 'e' 'f' 'd' 'x' 00
				    '7' '6' 't' 'h' 'g' 'v' 'c' ' '
				    '9' '8' 'u' 'y' 'j' 'n' 'b' 5C
				    '-' '0' 'o' 'i' 'l' 'k' ',' 'm'
				    00  '=' ']' 0D  00  00  00  00
				    00  00  00  00  DF  DF  00  00
				    00  00  00  00  00  DC  00  DD
				    00  00  00  00  00  00  00  00
				    '[' 'p' 27  ';' '/' '.' 00  00
				    00  00  08  '3' '2' 1E  00  00
				    00  7F  00  00  00  1D  1F  1C
				    00  00  00  'q' 00  00  '1' 00
				    1B  '`' 00  09  00  00  00  00>;
		u-boot,keymap-DF = /size/ 8 <00  00  'W' 'S' 'A' 'Z' 00  00
				    00  00  00  00  00  00  00  00
				    00  00  00  00  00  00  00  00

...

The DC-DF codes codes denote Shift, Fn and Ctrl keys which would need a separate keymap - although we could probably calculate the Ctrl one.

From a point of view of efficiency, drivers can simply keep a pointer to the appropriate property and read out the codes based on (row,column) position.

If we have something like this, then in order for the keyboard to work in U-Boot, vendors would need to create a completely separate ASCII mapping. Yes I agree this is a bit of a pain, but the above map is fairly easy to type in, and quite compact.

Given the push-back on the U-Boot list from Linux people about my bindings, I would like to work out the U-Boot side in this thread if possible, since it seems to be dependent on what Linux does. But I hope what is created is efficient enough not to bloat U-Boot or require an new input layer to be added just to use fdt.

Hi,

Quoted 25 lines · Simon Glass

Quoted 6 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +0 -0

new file mode 100644
index 0000000..1db8e12

Documentation/devicetree/bindings/input/matrix-keymap.txt line 10

Quoted 10 lines

Documentation/devicetree/bindings/input/matrix-keymap.txt new file +10 -0

@@ -0,0 +1,27 @@
+A simple common binding for matrix-connected key boards. Currently targeted at
+defining the keys in the scope of linux key codes since that is a stable and
+standardized interface at this time.
+
+Required properties:
+- compatible: "matrix-keyboard-controller"
+- linux,keymap: an array of packed 1-cell entries containing the equivalent
+ ?of row, column and linux key-code. The 32-bit big endian cell is packed
+ ?as:
+ ? ? ? row << 24 | column << 16 | key-code

This looks much better than the Samsung binding and the original 3-cell one.

U-Boot Tegra (keyboard patch series) currently uses a 16x8 matrix, and uses a single byte (the ASCII character) for a total of 128 bytes per keymap. Since U-Boot does not have (or apparently want) the concept of key codes or the added code and intermediate data this requires, the binding presented here will not suit U-Boot so far.

Quoted 8 lines

These rows and columns embedded in the cell make it more of a pain to write the fdt description. If you just set up the cells in (column, row) order and set the matrix size (rows, columns) then you end up with 128 entries on Tegra. If you use uint16 then this could be 256 bytes instead of 512. The binding you present for Tegra would be 109 * 4 = 436 bytes. However I take your point that Fn maps are much more sparse, so overall this is likely to be similar (512 bytes for either method once Fn mappings are taken into account).

Looking at the keymap you posted, you define 62 keys and 16 function keys. So if that is all you care about on your particular keyboard, that means that the layout with the 1-cell format is 248 bytes for the base keymap, and 64 bytes for the fn-keymap. The benefit of using the row/column/keycode format is that the table is only as large as the number of keys you care about.

Quoted 11 lines

But going back to U-Boot, it does not have the intermediate table that you malloc and decant into, it does not understand key codes so doesn't know what to do when Shift is pressed, doesn't understand languages, etc. In order to use these Linux bindings in U-Boot we would need to add new input layer, extra decode code and intermediate tables. I can almost hear the NAKs already (bear in mind fdt only went into U-Boot in the December release and people are more sensitive to code size / performance there than in Linux). I did ask about this on this list a few weeks ago but no response yet.

Quoted 2 lines

We could perhaps add an alternative direct ASCII binding like this example (which would have to be in a separate node):

You keep saying "direct ASCII", but your map contains non-ASCII characters. ASCII provides no way at all to specify things such as shift, or arrow keys, or other common new keys on keyboards. Instead you have had to make up an ad-hoc custom map that contains your own special codes for these keys.

For example, for arrow keys you seem to overload the FS/GS/RS/US. I'm not saying we expect to need to use those ascii codes, but it does seem arbitrary to just grab them for arrow keys. So in essence you have come up with your own encoding of non-ASCII keys instead of reusing what is already available through the linux keycodes.

Also, for example with return, does it encode as CR, LF or CR+LF? Etc.

Doing a binding in pseudo-ASCII is just asking for extra complications, in my opinion. Over time that encoding is likely to swell to similar sizes anyway, but still be incompatible.

Quoted 28 lines

? ? ? ? ? ? ? ? /* Use a packed binding which doesn't include row,column numbers in each cell */
? ? ? ? ? ? ? ? packed-binding;
? ? ? ? ? ? ? ? matrix-columns = <8>;
? ? ? ? ? ? ? ? matrix-rows = <16>;
? ? ? ? ? ? ? ? ascii-binding; ?/* ASCII characters instead of keycodes */ ? ? ? ? ? ? ? u-boot,keymap ?= /size/ 8 <00 ?00 ?'w' 's' 'a' 'z' 00 ?DE ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 00 ?00 ?00 ?00 ?00 ?00 ?00 ?00 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 00 ?00 ?00 ?00 ?00 ?00 ?00 ?00 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? '5' '4' 'r' 'e' 'f' 'd' 'x' 00 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? '7' '6' 't' 'h' 'g' 'v' 'c' ' ' ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? '9' '8' 'u' 'y' 'j' 'n' 'b' 5C ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? '-' '0' 'o' 'i' 'l' 'k' ',' 'm' ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 00 ?'=' ']' 0D ?00 ?00 ?00 ?00 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 00 ?00 ?00 ?00 ?DF ?DF ?00 ?00 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 00 ?00 ?00 ?00 ?00 ?DC ?00 ?DD ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 00 ?00 ?00 ?00 ?00 ?00 ?00 ?00 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? '[' 'p' 27 ?';' '/' '.' 00 ?00 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 00 ?00 ?08 ?'3' '2' 1E ?00 ?00 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 00 ?7F ?00 ?00 ?00 ?1D ?1F ?1C ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 00 ?00 ?00 ?'q' 00 ?00 ?'1' 00 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 1B ?'`' 00 ?09 ?00 ?00 ?00 ?00>; ? ? ? ? ? ? ? u-boot,keymap-DF = /size/ 8 <00 ?00 ?'W' 'S' 'A' 'Z' 00 ?00 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 00 ?00 ?00 ?00 ?00 ?00 ?00 ?00 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 00 ?00 ?00 ?00 ?00 ?00 ?00 ?00
...

Quoted 2 lines

The DC-DF codes codes denote Shift, Fn and Ctrl keys which would need a separate keymap - although we could probably calculate the Ctrl one.

I had included the row/column property in my binding for KEY_FN, but in hindsight it's probably just enough to decode that through the regular key layout, since there will be an entry for it there.

Quoted 9 lines

From a point of view of efficiency, drivers can simply keep a pointer to the appropriate property and read out the codes based on (row,column) position.

If we have something like this, then in order for the keyboard to work in U-Boot, vendors would need to create a completely separate ASCII mapping. Yes I agree this is a bit of a pain, but the above map is fairly easy to type in, and quite compact.

It doesn't make sense to come up with a fresh, brand new binding that requires anyone adding a new device to it to do extra work to do two redunant descriptions of the same device. We should be able to find common ground.

The KEY_* codes that are interesting all seem to be in the range of 0-83. In the u-boot case, that means the base KEY*-to-ascii table would be 83 bytes.

That means that the memory consumption for your case above would be 4*62+4*16=312 bytes for the base+fn table, and then 3*83 = 249 bytes for the KEY*-to-ASCII tables for regular/shift/ctrl, for a total of 561 bytes. I'm sure there will be a few special cases in the code just like you have now for arrow keys, etc, but they don't have to be in the 83-byte table.

Sure, that is somewhat more than your 4*128 byte tables, but it is also a much more flexible binding that is less likely to hit limitations down the road.

Quoted 6 lines

Given the push-back on the U-Boot list from Linux people about my bindings, I would like to work out the U-Boot side in this thread if possible, since it seems to be dependent on what Linux does. But I hope what is created is efficient enough not to bloat U-Boot or require an new input layer to be added just to use fdt.

I think it's a great idea to use a common binding, since there will just be wasted effort for vendors to have to create two maps and keep them in sync.

-Olof

Can the Linux key codes fit in 8 bits?

Quoted 1 line · Simon Glass

Can the Linux key codes fit in 8 bits?

That depends on your point of view.

If you hack on X, then the answer is yes and you ignore the squeels of your users when certain key presses get misinterpreted. (The Psion LX platform, otherwise known as the Netbook Pro, suffered with this problem.)

If you are a kernel hacker, the answer is no, because key codes currently go all the way to 0x300.

Quoted 9 lines · Russell King - ARM Linux

Quoted 1 line · Simon Glass

Can the Linux key codes fit in 8 bits?

That depends on your point of view.

If you hack on X, then the answer is yes and you ignore the squeels of your users when certain key presses get misinterpreted. ?(The Psion LX platform, otherwise known as the Netbook Pro, suffered with this problem.)

Quoted 2 lines

If you are a kernel hacker, the answer is no, because key codes currently go all the way to 0x300.

For bootloader environment 0-255 range is probably sufficient though, the upper keys are somewhat recent additions to the maps...

Quoted 9 lines · Russell King - ARM Linux

Quoted 1 line · Simon Glass

Can the Linux key codes fit in 8 bits?

That depends on your point of view.

If you hack on X, then the answer is yes and you ignore the squeels of your users when certain key presses get misinterpreted. (The Psion LX platform, otherwise known as the Netbook Pro, suffered with this problem.)

Quoted 2 lines

If you are a kernel hacker, the answer is no, because key codes currently go all the way to 0x300.

For bootloader environment 0-255 range is probably sufficient though, the upper keys are somewhat recent additions to the maps...

Quoted 13 lines · Dmitry Torokhov

Quoted 11 lines · Russell King - ARM Linux

Quoted 1 line · Simon Glass

Can the Linux key codes fit in 8 bits?

That depends on your point of view.

If you hack on X, then the answer is yes and you ignore the squeels of your users when certain key presses get misinterpreted. (The Psion LX platform, otherwise known as the Netbook Pro, suffered with this problem.)

If you are a kernel hacker, the answer is no, because key codes currently go all the way to 0x300.

Quoted 2 lines

For bootloader environment 0-255 range is probably sufficient though, the upper keys are somewhat recent additions to the maps...

I assume you deem 'recent' to mean 8 years ago - they've been there since at least 2.6.9, which is where the problem I refer to above was first noticed.

This adds a simple device tree binding for simple key matrix data and a helper to fill in the platform data.

The implementation is in a shared file outside if drivers/input/keyboard since some drivers in drivers/input/misc might be making use of it as well.

Changes since v2:
* Removed reference to "legacy" format with a subnode per key
* Changed to a packed format with one cell per key instead of 3.
* Moved new OF helper to separate file
* Misc fixups based on comments

Changes since v1:
* Removed samsung-style binding support
* Added linux,fn-keymap and linux,fn-key optional properties

Signed-off-by Olof Johansson <[email protected]>

This adds a simple device tree binding for simple key matrix data and a helper to fill in the platform data.

The implementation is in a shared file outside if drivers/input/keyboard since some drivers in drivers/input/misc might be making use of it as well.

Changes since v3:
* Dropped compatible field in matrix-keymap.txt
* Dropped linux,fn-key
* Dropped linux,fn-keymap optional property but included guideline on naming convention
* Now passing property name in to the helper function (or will assume "linux,keymap" if passed NULL)

drivers/input/of_keymap.c +3 -0

 
+struct matrix_keymap_data *
+matrix_keyboard_of_fill_keymap(struct device_node *np, char *propname)
+{

Olof Johansson wrote at Tuesday, January 03, 2012 2:37 PM:

Quoted 20 lines

This adds a simple device tree binding for simple key matrix data and a helper to fill in the platform data.

The implementation is in a shared file outside if drivers/input/keyboard since some drivers in drivers/input/misc might be making use of it as well.

Changes since v3:
?* Dropped compatible field in matrix-keymap.txt
?* Dropped linux,fn-key
?* Dropped linux,fn-keymap optional property but included guideline on ? ?naming convention
?* Now passing property name in to the helper function (or will assume ? ?"linux,keymap" if passed NULL)

drivers/input/of_keymap.c +3 -0

 
+struct matrix_keymap_data *
+matrix_keyboard_of_fill_keymap(struct device_node *np, char *propname)
+{

...

Quoted 3 lines

+ ? ? kd = kmalloc(sizeof(*kd), GFP_KERNEL);
+ ? ? if (!kd)
+ ? ? ? ? ? ? return NULL;

Should that use kzalloc in case struct matrix_keymap_data grows some new fields that people will assume are set to zero since the struct would usually be in .data? Still, people should probably grep the code when making such changes...

Otherwise,

Acked-by Stephen Warren <[email protected]>

--
nvpublic

Do you have a Tegra .dts binding for the Seaboard keyboard that I can test with please?

[pruned cc list]

Hi,

Quoted 2 lines · Simon Glass

Do you have a Tegra .dts binding for the Seaboard keyboard that I can test with please?

Hmm, I had one for the previous binding. If you didn't get tired of waiting for this reply and make one yourself, I'll send one over in a bit. :)

-Olof

Olof Johansson wrote at Tuesday, January 03, 2012 2:37 PM:

Quoted 20 lines

This adds a simple device tree binding for simple key matrix data and a helper to fill in the platform data.

The implementation is in a shared file outside if drivers/input/keyboard since some drivers in drivers/input/misc might be making use of it as well.

Changes since v3:
* Dropped compatible field in matrix-keymap.txt
* Dropped linux,fn-key
* Dropped linux,fn-keymap optional property but included guideline on naming convention
* Now passing property name in to the helper function (or will assume "linux,keymap" if passed NULL)

drivers/input/of_keymap.c +3 -0

 
+struct matrix_keymap_data *
+matrix_keyboard_of_fill_keymap(struct device_node *np, char *propname)
+{

...

drivers/input/of_keymap.c line 53

Quoted 3 lines

drivers/input/of_keymap.c +3 -0

+	kd = kmalloc(sizeof(*kd), GFP_KERNEL);
+	if (!kd)
+		return NULL;

Should that use kzalloc in case struct matrix_keymap_data grows some new fields that people will assume are set to zero since the struct would usually be in .data? Still, people should probably grep the code when making such changes...

Otherwise,

Acked-by Stephen Warren <[email protected]>

This includes an optional "linux,fn-keymap" binding that is not yet implemented, that will be used to specify the Fn-key modifier layout if needed.

(quoted diff) +0 -0

+Required properties:
+- compatible: "nvidia,tegra20-kbc", "matrix-keyboard-controller"

Olof Johansson wrote at Tuesday, January 03, 2012 3:28 PM:

Quoted 4 lines

This includes an optional "linux,fn-keymap" binding that is not yet implemented, that will be used to specify the Fn-key modifier layout if needed.

(quoted diff) +0 -0

...

Quoted 2 lines

+Required properties:
+- compatible: "nvidia,tegra20-kbc", "matrix-keyboard-controller"

I think "matrix-keyboard-controller" should be dropped now?

Otherwise,

Acked-by Stephen Warren <[email protected]>

Quoted 12 lines · Stephen Warren

Olof Johansson wrote at Tuesday, January 03, 2012 3:28 PM:

Quoted 5 lines

This includes an optional "linux,fn-keymap" binding that is not yet implemented, that will be used to specify the Fn-key modifier layout if needed.

(quoted diff) +0 -0

...

Quoted 2 lines

+Required properties:
+- compatible: "nvidia,tegra20-kbc", "matrix-keyboard-controller"

Quoted 1 line

I think "matrix-keyboard-controller" should be dropped now?

Crap, yeah, I had dropped it but forgot to commit before I did send-email.

Dmitry, if this is the only comment, do you want me to resend or are you OK with editing yourself before applying?