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

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

[Olof Johansson]

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

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

Signed-off-by: Olof Johansson <olof@lixom.net>
---

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

diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt b/Documentation/devicetree/bindings/input/matrix-keymap.txt
new file mode 100644
index 0000000..894f786
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/matrix-keymap.txt
@@ -0,0 +1,35 @@
+For matrix keyboards there are two kinds of layout bindings using
+linux key codes.
+
+Required properties:
+- compatible: "matrix-keyboard-controller"
+
+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 >;
diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
index fe7c4b9..ff13cd3 100644
--- a/include/linux/input/matrix_keypad.h
+++ b/include/linux/input/matrix_keypad.h
@@ -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
@@ -106,4 +107,58 @@ matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 	__clear_bit(KEY_RESERVED, keybit);
 }
 
+#ifdef CONFIG_OF
+static inline 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;
+
+	for_each_child_of_node(np, knp)
+		kd->keymap_size++;
+
+	keymap = kzalloc(kd->keymap_size * sizeof(u32), GFP_KERNEL);
+	if (!keymap) {
+		kfree(kd);
+		return NULL;
+	}
+
+	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 */
-- 
1.7.8.GIT

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

[Rob Herring]

Olof,

On 12/28/2011 04:52 PM, Olof Johansson wrote:
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> This adds a basic device tree binding for simple key matrix data.
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>
> ---
> 
> 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
> 
> diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt b/Documentation/devicetree/bindings/input/matrix-keymap.txt
> new file mode 100644
> index 0000000..894f786
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/matrix-keymap.txt
> @@ -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.

> +
> +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 >;
> diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
> index fe7c4b9..ff13cd3 100644
> --- a/include/linux/input/matrix_keypad.h
> +++ b/include/linux/input/matrix_keypad.h
> @@ -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
> @@ -106,4 +107,58 @@ matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
>  	__clear_bit(KEY_RESERVED, keybit);
>  }
>  
> +#ifdef CONFIG_OF
> +static inline struct matrix_keymap_data *
> +matrix_keyboard_of_fill_keymap(struct device_node *np)
> +{

Seems a bit large to inline.

> +	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) {
> +		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.

Rob

> +
> +	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 */

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

[Olof Johansson]

On Wed, Dec 28, 2011 at 3:30 PM, Rob Herring <robherring2@gmail.com> wrote:
> Olof,
>
> On 12/28/2011 04:52 PM, Olof Johansson wrote:
>> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>
>> This adds a basic device tree binding for simple key matrix data.
>>
>> Signed-off-by: Olof Johansson <olof@lixom.net>
>> ---
>>
>> 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
>>
>> diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt b/Documentation/devicetree/bindings/input/matrix-keymap.txt
>> new file mode 100644
>> index 0000000..894f786
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/matrix-keymap.txt
>> @@ -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.

The view I had when adding a compatible field was that the controllers
that use this binding essentially inherits and extends it, so having
this as a common compatible seems like the right thing to do. It also
gives an easy reference ('see binding for
"matrix-keyboard-controller"' from the other binding, etc).

>> +
>> +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 >;
>> diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
>> index fe7c4b9..ff13cd3 100644
>> --- a/include/linux/input/matrix_keypad.h
>> +++ b/include/linux/input/matrix_keypad.h
>> @@ -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
>> @@ -106,4 +107,58 @@ matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
>> ? ? ? __clear_bit(KEY_RESERVED, keybit);
>> ?}
>>
>> +#ifdef CONFIG_OF
>> +static inline struct matrix_keymap_data *
>> +matrix_keyboard_of_fill_keymap(struct device_node *np)
>> +{
>
> Seems a bit large to inline.

It's pushing the limits for it, yes. Dmitry, should I start a shared C
file for this? If so, where? drivers/input/keyboard/keymap.c?

>> + ? ? 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) {
>> + ? ? ? ? ? ? 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.

Yes, caller is expected to free. Dmitry doesn't like devm_alloc so I
guess having a free function could be a decent idea (I had it
open-coded in the driver that uses this, will switch that over too).


-Olof

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

[Stephen Warren]

Olof Johansson wrote at Wednesday, December 28, 2011 3:53 PM:
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> This adds a basic device tree binding for simple key matrix data.
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>
> ---
> 
> 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.
...
> diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt
...
> +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.

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.

-- 
nvpublic

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

[Olof Johansson]

On Wed, Dec 28, 2011 at 10:16 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Olof Johansson wrote at Wednesday, December 28, 2011 3:53 PM:
>> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>
>> This adds a basic device tree binding for simple key matrix data.
>>
>> Signed-off-by: Olof Johansson <olof@lixom.net>
>> ---
>>
>> 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.
> ...
>> diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt
> ...
>> +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.
>
> 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.

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

The row/column format is closer to how the hardware is actually
behaving, and it's trivial to remap that to the kind of
two-dimensional array like Simon provides (it's already done in the
input layer). It also has the benefit of not using up space for
undefined keys.

Since there is no concept of modifier keys in hardware (they are just
keypress events), the translation table should be in the u-boot
implementation instead of in the binding.

There is already precedence to using the linux key code format instead
of character encodings in keyboard maps, and the reasoning behind it
is quite sane: Since it's a userspace-exported interface it is a
stable one. If we went with the array that Simon proposed, there will
need to be a transition layer in the kernel device tree parser to
remap everything back, with very little to no benefit when it comes to
the usability of the binding, as far as I can tell.

It is also a little unclear to me whether that binding should be
localized or not, which would add another level of headaches when it
comes to reverse-mapping the character array back to linux key
bindings, since then you would also need to provide one remapping
table per locale supported.


-Olof

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

[Stephen Warren]

Olof Johansson wrote at Wednesday, December 28, 2011 11:34 PM:
> On Wed, Dec 28, 2011 at 10:16 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > Olof Johansson wrote at Wednesday, December 28, 2011 3:53 PM:
> >> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >>
> >> This adds a basic device tree binding for simple key matrix data.
> >>
> >> Signed-off-by: Olof Johansson <olof@lixom.net>
> >> ---
> >>
> >> 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.
> > ...
> >> diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt
> > ...
> >> +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.
> >
> > 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.

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.

-- 
nvpublic

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

[Olof Johansson]

On Wed, Dec 28, 2011 at 11:01 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Olof Johansson wrote at Wednesday, December 28, 2011 11:34 PM:
>> On Wed, Dec 28, 2011 at 10:16 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> > Olof Johansson wrote at Wednesday, December 28, 2011 3:53 PM:
>> >> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> >>
>> >> This adds a basic device tree binding for simple key matrix data.
>> >>
>> >> Signed-off-by: Olof Johansson <olof@lixom.net>
>> >> ---
>> >>
>> >> 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.
>> > ...
>> >> diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt
>> > ...
>> >> +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.
>> >
>> > 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.
>
> 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.

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

The bloat is pretty minimal, since the parsing is simple. But yeah, I
see your point.


-Olof

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

[Rob Herring]

On 12/29/2011 01:06 AM, Olof Johansson wrote:
> On Wed, Dec 28, 2011 at 11:01 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> Olof Johansson wrote at Wednesday, December 28, 2011 11:34 PM:
>>> On Wed, Dec 28, 2011 at 10:16 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>>> Olof Johansson wrote at Wednesday, December 28, 2011 3:53 PM:
>>>>> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>>>
>>>>> This adds a basic device tree binding for simple key matrix data.
>>>>>
>>>>> Signed-off-by: Olof Johansson <olof@lixom.net>
>>>>> ---
>>>>>
>>>>> 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.
>>>> ...
>>>>> diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt
>>>> ...
>>>>> +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.
>>>>
>>>> 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.
>>
>> 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.
> 

I think both can coexist because the Samsung version is really a
derivative of gpio-keys binding. I think we want to encourage your
keymap version, so only supporting it in the generic code should do that.

Rob

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

[Thomas Abraham]

Hi Olof,

On 29 December 2011 12:36, Olof Johansson <olof@lixom.net> wrote:
[...]

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

I am okay with switching to the new keyboard binding. I will modify
the Samsung keyboard device tree support and prepare a patch.

Thanks,
Thomas.

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

[Grant Likely]

On Wed, Dec 28, 2011 at 11:06:02PM -0800, Olof Johansson wrote:
> On Wed, Dec 28, 2011 at 11:01 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > 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.

g.

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

[Olof Johansson]

On Sun, Jan 1, 2012 at 11:21 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Dec 28, 2011 at 11:06:02PM -0800, Olof Johansson wrote:
>> On Wed, Dec 28, 2011 at 11:01 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> > 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.

The main reason I had it there today was to make it easier to do a
versioned binding in case of future revisions, but I guess it's not
worth the hassle.

So, in that case I will move out the fn management out of the common
code as well, and pass in the property name to
matrix_keyboard_of_fill_keymap. That way, the drivers that need to do
a fn-keymap can do so outside of the shared code (and it gets rid of
the FIXME in that case as well). Revised patch in a bit.


-Olof

[PATCH v2] Input: keyboard - add device tree bindings for simple key matrixes

[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 <olof@lixom.net>
---
 .../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

diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt b/Documentation/devicetree/bindings/input/matrix-keymap.txt
new file mode 100644
index 0000000..480ca46
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/matrix-keymap.txt
@@ -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 >;
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index df7061f..83e6eed 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -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
diff --git a/drivers/input/keyboard/keymap.c b/drivers/input/keyboard/keymap.c
new file mode 100644
index 0000000..80d1074
--- /dev/null
+++ b/drivers/input/keyboard/keymap.c
@@ -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
diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
index fe7c4b9..ac999c6 100644
--- a/include/linux/input/matrix_keypad.h
+++ b/include/linux/input/matrix_keypad.h
@@ -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 */
-- 
1.7.8.GIT

[PATCH v2] Input: keyboard - add device tree bindings for simple key matrixes

[Rob Herring]

On 12/29/2011 12:42 PM, Olof Johansson wrote:
> 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 <olof@lixom.net>
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt b/Documentation/devicetree/bindings/input/matrix-keymap.txt
> new file mode 100644
> index 0000000..480ca46
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/matrix-keymap.txt
> @@ -0,0 +1,25 @@
> +For matrix keyboards there are two kinds of layout bindings using
> +linux key codes.

Need to update this.

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

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.

Rob

> +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 >;
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index df7061f..83e6eed 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -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
> diff --git a/drivers/input/keyboard/keymap.c b/drivers/input/keyboard/keymap.c
> new file mode 100644
> index 0000000..80d1074
> --- /dev/null
> +++ b/drivers/input/keyboard/keymap.c
> @@ -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
> diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
> index fe7c4b9..ac999c6 100644
> --- a/include/linux/input/matrix_keypad.h
> +++ b/include/linux/input/matrix_keypad.h
> @@ -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 */

[PATCH v2] Input: keyboard - add device tree bindings for simple key matrixes

[Olof Johansson]

On Thu, Dec 29, 2011 at 1:14 PM, Rob Herring <robherring2@gmail.com> wrote:
> On 12/29/2011 12:42 PM, Olof Johansson wrote:
>> 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 <olof@lixom.net>
>> ---
>> ?.../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
>>
>> diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt b/Documentation/devicetree/bindings/input/matrix-keymap.txt
>> new file mode 100644
>> index 0000000..480ca46
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/matrix-keymap.txt
>> @@ -0,0 +1,25 @@
>> +For matrix keyboards there are two kinds of layout bindings using
>> +linux key codes.
>
> Need to update this.

Oops, yes.

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

Actually, in the case of the default layout for tegra-kbc, there are
109 keys defined in a 8*31 (248) matrix, and linux,fn-keymap is going
to be much sparser than that.

Instead, how about packing the row, column and index into one cell as
8, 8, 16 bits? That would be a win for both sparse and dense maps.

Number of rows and columns for the keyboard could be provided as
properties (num-rows, num-columns), it's something I've gone back and
forth in whether it should be in the generic binding or something that
is specific per keyboard vendor -- some of them are hardcoded so
providing properties might not add value.


-Olof

[PATCH v2] Input: keyboard - add device tree bindings for simple key matrixes

[Dmitry Torokhov]

Hi Olof,

On Thu, Dec 29, 2011 at 10:42:26AM -0800, Olof Johansson wrote:
> 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.

> Signed-off-by: Olof Johansson <olof@lixom.net>
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt b/Documentation/devicetree/bindings/input/matrix-keymap.txt
> new file mode 100644
> index 0000000..480ca46
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/matrix-keymap.txt
> @@ -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 >;
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index df7061f..83e6eed 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -4,6 +4,7 @@
>  
>  # Each configuration option enables a list of files.
>  
> +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.

>  obj-$(CONFIG_KEYBOARD_ADP5520)		+= adp5520-keys.o
>  obj-$(CONFIG_KEYBOARD_ADP5588)		+= adp5588-keys.o
>  obj-$(CONFIG_KEYBOARD_ADP5589)		+= adp5589-keys.o
> diff --git a/drivers/input/keyboard/keymap.c b/drivers/input/keyboard/keymap.c
> new file mode 100644
> index 0000000..80d1074
> --- /dev/null
> +++ b/drivers/input/keyboard/keymap.c
> @@ -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 *

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

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

> +	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?

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

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

> +	kfree(kd);
> +}
> +EXPORT_SYMBOL_GPL(matrix_keyboard_of_free_keymap);
> +#endif
> diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
> index fe7c4b9..ac999c6 100644
> --- a/include/linux/input/matrix_keypad.h
> +++ b/include/linux/input/matrix_keypad.h
> @@ -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 */
> -- 
> 1.7.8.GIT
> 

-- 
Dmitry

[PATCH v2] Input: keyboard - add device tree bindings for simple key matrixes

[Olof Johansson]

Hi,

On Thu, Dec 29, 2011 at 2:05 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Olof,
>
> On Thu, Dec 29, 2011 at 10:42:26AM -0800, Olof Johansson wrote:
>> 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.
>
>> Signed-off-by: Olof Johansson <olof@lixom.net>
>> ---
>> ?.../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
>>
>> diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt b/Documentation/devicetree/bindings/input/matrix-keymap.txt
>> new file mode 100644
>> index 0000000..480ca46
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/matrix-keymap.txt
>> @@ -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 >;
>> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
>> index df7061f..83e6eed 100644
>> --- a/drivers/input/keyboard/Makefile
>> +++ b/drivers/input/keyboard/Makefile
>> @@ -4,6 +4,7 @@
>>
>> ?# Each configuration option enables a list of files.
>>
>> +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.

Hmm. It could be argued that any driver that provides a keyboard
complex enough to need this binding should be under
drivers/input/keyboard (possibly provided MFD-style by the misc
device). But sure, I'll move it and add a slient Kconfig for it. :)

>> ?obj-$(CONFIG_KEYBOARD_ADP5520) ? ? ? ? ? ? ? += adp5520-keys.o
>> ?obj-$(CONFIG_KEYBOARD_ADP5588) ? ? ? ? ? ? ? += adp5588-keys.o
>> ?obj-$(CONFIG_KEYBOARD_ADP5589) ? ? ? ? ? ? ? += adp5589-keys.o
>> diff --git a/drivers/input/keyboard/keymap.c b/drivers/input/keyboard/keymap.c
>> new file mode 100644
>> index 0000000..80d1074
>> --- /dev/null
>> +++ b/drivers/input/keyboard/keymap.c
>> @@ -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 *
>
> Instead of allocating keymap data why don't we populate keymap and
> keybits directly, like matrix_keypad_build_keymap() does?

The reason I did it this way is the way the drivers are written for
device tree: They use a helper that fills in the platform data, and
then use the same probe flow as for the platform device. If I was to
fill in the keymap/keybits here, then we would have separate code
flows for the two ways to enter the driver probe.

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

Sure.

>> + ? ? 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?

See discussion on other subthread: I think we want to switch to a
compacter format anyway and just use one cell per key. But if not,
yeah this check is useful for that case.

>> +
>> + ? ? 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);
>
> If we decide to keep allocating kd then checking kd->keymap before
> freeing is not necessary.

Good point, fixing.


-Olof

[PATCH v3] Input: keyboard - add device tree bindings for simple key matrixes

[Olof Johansson]

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 <olof@lixom.net>
---
 .../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

diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt b/Documentation/devicetree/bindings/input/matrix-keymap.txt
new file mode 100644
index 0000000..1db8e12
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/matrix-keymap.txt
@@ -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
+
+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.
+
+- 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 = < 0x00030012
+			 0x0102003a >;
+	linux,fn-key = < 2 1 >;
+	linux,fn-keymap = < 0x0002004a >;
diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig
index 001b147..3325979 100644
--- a/drivers/input/Kconfig
+++ b/drivers/input/Kconfig
@@ -25,6 +25,10 @@ config INPUT
 
 if INPUT
 
+config INPUT_OF_MATRIX_KEYMAP
+	depends on USE_OF
+	bool
+
 config INPUT_FF_MEMLESS
 	tristate "Support for memoryless force-feedback devices"
 	help
diff --git a/drivers/input/Makefile b/drivers/input/Makefile
index 0c78949..b173a13a 100644
--- a/drivers/input/Makefile
+++ b/drivers/input/Makefile
@@ -24,3 +24,4 @@ obj-$(CONFIG_INPUT_TOUCHSCREEN)	+= touchscreen/
 obj-$(CONFIG_INPUT_MISC)	+= misc/
 
 obj-$(CONFIG_INPUT_APMPOWER)	+= apm-power.o
+obj-$(CONFIG_INPUT_OF_MATRIX_KEYMAP) += of_keymap.o
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index cdc385b..3371954c 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -394,6 +394,7 @@ config KEYBOARD_NOMADIK
 config KEYBOARD_TEGRA
 	tristate "NVIDIA Tegra internal matrix keyboard controller support"
 	depends on ARCH_TEGRA
+	select INPUT_OF_MATRIX_KEYMAP if USE_OF
 	help
 	  Say Y here if you want to use a matrix keyboard connected directly
 	  to the internal keyboard controller on Tegra SoCs.
diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c
new file mode 100644
index 0000000..b33627d
--- /dev/null
+++ b/drivers/input/of_keymap.c
@@ -0,0 +1,87 @@
+/*
+ * Helpers for open firmware matrix keyboard bindings
+ *
+ * Copyright (C) 2012 Google, Inc
+ *
+ * Author:
+ * 	Olof Johansson <olof@lixom.net>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#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>
+
+struct matrix_keymap_data *
+matrix_keyboard_of_fill_keymap(struct device_node *np)
+{
+	struct matrix_keymap_data *kd;
+	int proplen = 0, i;
+	u32 *keymap;
+	const __be32 *prop;
+
+	if (!np)
+		return NULL;
+
+	prop = of_get_property(np, "linux,keymap", &proplen);
+
+	if (!of_device_is_compatible(np, "matrix-keyboard-controller"))
+		return NULL;
+
+	if (proplen % 4) {
+		pr_warn("Malformed linux,keymap property in %s\n",
+			np->full_name);
+		return NULL;
+	}
+
+	kd = kmalloc(sizeof(*kd), GFP_KERNEL);
+	if (!kd)
+		return NULL;
+	kd->keymap_size = proplen / sizeof(u32);
+
+	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++) {
+		u32 tmp = be32_to_cpup(prop+i);
+		int key_code, row, col;
+
+		row = (tmp >> 24) & 0xff;
+		col = (tmp >> 16) & 0xff;
+		key_code = tmp & 0xffff;
+		*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;
+	kfree(kd->keymap);
+	kfree(kd);
+}
+EXPORT_SYMBOL_GPL(matrix_keyboard_of_free_keymap);
diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
index fe7c4b9..fcf45ec 100644
--- a/include/linux/input/matrix_keypad.h
+++ b/include/linux/input/matrix_keypad.h
@@ -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
@@ -106,4 +107,22 @@ matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 	__clear_bit(KEY_RESERVED, keybit);
 }
 
+#ifdef CONFIG_INPUT_OF_MATRIX_KEYMAP
+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_keyboard_of_free_keymap(const struct matrix_keymap_data *kd)
+{
+}
+#endif
+
 #endif /* _MATRIX_KEYPAD_H */
-- 
1.7.8.GIT

[PATCH v3] Input: keyboard - add device tree bindings for simple key matrixes

[Simon Glass]

Hi Olof,

On Sun, Jan 1, 2012 at 10:09 PM, Olof Johansson <olof@lixom.net> wrote:
> 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 <olof@lixom.net>
> ---
> ?.../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
>
> diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt b/Documentation/devicetree/bindings/input/matrix-keymap.txt
> new file mode 100644
> index 0000000..1db8e12
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/matrix-keymap.txt
> @@ -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.

Regards,
Simon

> +
> +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.
> +
> +- 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 = < 0x00030012
> + ? ? ? ? ? ? ? ? ? ? ? ?0x0102003a >;
> + ? ? ? linux,fn-key = < 2 1 >;
> + ? ? ? linux,fn-keymap = < 0x0002004a >;

[snip]

Regards,
Simon

[PATCH v3] Input: keyboard - add device tree bindings for simple key matrixes

[Olof Johansson]

Hi,

On Mon, Jan 02, 2012 at 10:39:04AM -0800, Simon Glass wrote:
> > diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt b/Documentation/devicetree/bindings/input/matrix-keymap.txt
> > new file mode 100644
> > index 0000000..1db8e12
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/matrix-keymap.txt
> > @@ -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).

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.

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

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.

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

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.

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

> 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

[PATCH v3] Input: keyboard - add device tree bindings for simple key matrixes

[Simon Glass]

Hi Olof,

On Tue, Jan 3, 2012 at 7:43 AM, Olof Johansson <olof@lixom.net> wrote:
> Hi,
>
> On Mon, Jan 02, 2012 at 10:39:04AM -0800, Simon Glass wrote:
>> > diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt b/Documentation/devicetree/bindings/input/matrix-keymap.txt
>> > new file mode 100644
>> > index 0000000..1db8e12
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/input/matrix-keymap.txt
>> > @@ -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).
>
> 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.

Yes - my point is that the number of entries in the 1-byte binding
(with no row, column embedded) is unlikely to be more than the next
power of 2. So by avoiding including 2 or 3 unnecessary bytes we can
actually get a size reduction in most cases. It is also easier to
enter IMO. Can the Linux key codes fit in 8 bits?

>
>> 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):
>
> 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.

Yes it would be better to use one binding, just so long as it is
efficient and doesn't introduce a lot of new complexity in U-Boot
which is useful only with fdt.

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

OK - just need some signal as to what key codes are modifiers.

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

Yes I hope so.

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

Thanks for looking at this. We still have the need of the intermediate
table but since that is in BSS I don't think it is a problem.

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

I will have a look at this approach and see what the impact is, and
reply on this thread.

Regards,
Simon

>
>
> -Olof

[PATCH v3] Input: keyboard - add device tree bindings for simple key matrixes

[Russell King - ARM Linux]

On Tue, Jan 03, 2012 at 08:22:21AM -0800, Simon Glass wrote:
> 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.

[PATCH v3] Input: keyboard - add device tree bindings for simple key matrixes

[Dmitry Torokhov]

On Tue, Jan 03, 2012 at 04:29:30PM +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 03, 2012 at 08:22:21AM -0800, Simon Glass wrote:
> > 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.

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

-- 
Dmitry

[PATCH v3] Input: keyboard - add device tree bindings for simple key matrixes

[Olof Johansson]

On Tue, Jan 3, 2012 at 8:44 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Tue, Jan 03, 2012 at 04:29:30PM +0000, Russell King - ARM Linux wrote:
>> On Tue, Jan 03, 2012 at 08:22:21AM -0800, Simon Glass wrote:
>> > 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.
>
> For bootloader environment 0-255 range is probably sufficient though,
> the upper keys are somewhat recent additions to the maps...

To keep things in common it would be convenient to not cap the key
code at 8 bits for everybody though, since we're looking at reaching
agreement on a common solution between firmware and linux. And no
matter what the size of the word is there will be need for a
translation table.


-Olof

[PATCH v3] Input: keyboard - add device tree bindings for simple key matrixes

[Russell King - ARM Linux]

On Tue, Jan 03, 2012 at 08:44:32AM -0800, Dmitry Torokhov wrote:
> On Tue, Jan 03, 2012 at 04:29:30PM +0000, Russell King - ARM Linux wrote:
> > On Tue, Jan 03, 2012 at 08:22:21AM -0800, Simon Glass wrote:
> > > 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.
> 
> 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.

[PATCH v3] Input: keyboard - add device tree bindings for simple key matrixes

[Dmitry Torokhov]

On Tue, Jan 03, 2012 at 05:06:15PM +0000, Russell King - ARM Linux wrote:
> On Tue, Jan 03, 2012 at 08:44:32AM -0800, Dmitry Torokhov wrote:
> > On Tue, Jan 03, 2012 at 04:29:30PM +0000, Russell King - ARM Linux wrote:
> > > On Tue, Jan 03, 2012 at 08:22:21AM -0800, Simon Glass wrote:
> > > > 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.
> > 
> > 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.

Well, what's 8 years ;) On a more serious note keys above 255 are really
the extended set - keys of remote controls, camera control, touchpad,
various aplication launchers, etc that are not very interesting for
bootloader. But, as Olof mentioned, the encoding must be sufficient for
Linux as well as bootloader, so limiting it is not a good idea.

-- 
Dmitry

[PATCH v3] Input: keyboard - add device tree bindings for simple key matrixes

[Stephen Warren]

Olof Johansson wrote at Sunday, January 01, 2012 11:09 PM:
> 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 <olof@lixom.net>

Acked-by: Stephen Warren <swarren@nvidia.com>

The new binding looks good to me. I assume the FIXME in the code was
deliberate.

-- 
nvpublic

[PATCH v4] Input: keyboard - add device tree bindings for simple key matrixes

[Olof Johansson]

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)

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 <olof@lixom.net>
---
 .../devicetree/bindings/input/matrix-keymap.txt    |   19 +++++
 drivers/input/Kconfig                              |    4 +
 drivers/input/Makefile                             |    1 +
 drivers/input/keyboard/Kconfig                     |    1 +
 drivers/input/of_keymap.c                          |   85 ++++++++++++++++++++
 include/linux/input/matrix_keypad.h                |   19 +++++
 6 files changed, 129 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/matrix-keymap.txt
 create mode 100644 drivers/input/of_keymap.c

diff --git a/Documentation/devicetree/bindings/input/matrix-keymap.txt b/Documentation/devicetree/bindings/input/matrix-keymap.txt
new file mode 100644
index 0000000..3cd8b98
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/matrix-keymap.txt
@@ -0,0 +1,19 @@
+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:
+- 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
+
+Optional properties:
+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".
+
+Example:
+	linux,keymap = < 0x00030012
+			 0x0102003a >;
diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig
index 001b147..3325979 100644
--- a/drivers/input/Kconfig
+++ b/drivers/input/Kconfig
@@ -25,6 +25,10 @@ config INPUT
 
 if INPUT
 
+config INPUT_OF_MATRIX_KEYMAP
+	depends on USE_OF
+	bool
+
 config INPUT_FF_MEMLESS
 	tristate "Support for memoryless force-feedback devices"
 	help
diff --git a/drivers/input/Makefile b/drivers/input/Makefile
index 0c78949..b173a13a 100644
--- a/drivers/input/Makefile
+++ b/drivers/input/Makefile
@@ -24,3 +24,4 @@ obj-$(CONFIG_INPUT_TOUCHSCREEN)	+= touchscreen/
 obj-$(CONFIG_INPUT_MISC)	+= misc/
 
 obj-$(CONFIG_INPUT_APMPOWER)	+= apm-power.o
+obj-$(CONFIG_INPUT_OF_MATRIX_KEYMAP) += of_keymap.o
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index cdc385b..3371954c 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -394,6 +394,7 @@ config KEYBOARD_NOMADIK
 config KEYBOARD_TEGRA
 	tristate "NVIDIA Tegra internal matrix keyboard controller support"
 	depends on ARCH_TEGRA
+	select INPUT_OF_MATRIX_KEYMAP if USE_OF
 	help
 	  Say Y here if you want to use a matrix keyboard connected directly
 	  to the internal keyboard controller on Tegra SoCs.
diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c
new file mode 100644
index 0000000..7f11f6d
--- /dev/null
+++ b/drivers/input/of_keymap.c
@@ -0,0 +1,85 @@
+/*
+ * Helpers for open firmware matrix keyboard bindings
+ *
+ * Copyright (C) 2012 Google, Inc
+ *
+ * Author:
+ * 	Olof Johansson <olof@lixom.net>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#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>
+
+struct matrix_keymap_data *
+matrix_keyboard_of_fill_keymap(struct device_node *np, char *propname)
+{
+	struct matrix_keymap_data *kd;
+	int proplen = 0, i;
+	u32 *keymap;
+	const __be32 *prop;
+
+	if (!np)
+		return NULL;
+
+	if (!propname)
+		propname = "linux,keymap";
+
+	prop = of_get_property(np, propname, &proplen);
+
+	if (proplen % 4) {
+		pr_warn("Malformed keymap property %s in %s\n",
+			propname, np->full_name);
+		return NULL;
+	}
+
+	kd = kmalloc(sizeof(*kd), GFP_KERNEL);
+	if (!kd)
+		return NULL;
+	kd->keymap_size = proplen / sizeof(u32);
+
+	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++) {
+		u32 tmp = be32_to_cpup(prop+i);
+		int key_code, row, col;
+
+		row = (tmp >> 24) & 0xff;
+		col = (tmp >> 16) & 0xff;
+		key_code = tmp & 0xffff;
+		*keymap++ = KEY(row, col, key_code);
+	}
+
+	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;
+	kfree(kd->keymap);
+	kfree(kd);
+}
+EXPORT_SYMBOL_GPL(matrix_keyboard_of_free_keymap);
diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h
index fe7c4b9..b54fd87 100644
--- a/include/linux/input/matrix_keypad.h
+++ b/include/linux/input/matrix_keypad.h
@@ -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
@@ -106,4 +107,22 @@ matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 	__clear_bit(KEY_RESERVED, keybit);
 }
 
+#ifdef CONFIG_INPUT_OF_MATRIX_KEYMAP
+struct matrix_keymap_data *
+matrix_keyboard_of_fill_keymap(struct device_node *np, char *propname);
+
+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, char *propname)
+{
+	return NULL;
+}
+
+static inline void
+matrix_keyboard_of_free_keymap(const struct matrix_keymap_data *kd)
+{
+}
+#endif
+
 #endif /* _MATRIX_KEYPAD_H */
-- 
1.7.8.GIT

[PATCH v4] Input: keyboard - add device tree bindings for simple key matrixes

[Stephen Warren]

Olof Johansson wrote at Tuesday, January 03, 2012 2:37 PM:
> 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)

> diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c

> +struct matrix_keymap_data *
> +matrix_keyboard_of_fill_keymap(struct device_node *np, char *propname)
> +{
...
> +	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 <swarren@nvidia.com>

-- 
nvpublic

[PATCH v4] Input: keyboard - add device tree bindings for simple key matrixes

[Simon Glass]

Hi Olof,

On Tue, Jan 3, 2012 at 2:37 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Olof Johansson wrote at Tuesday, January 03, 2012 2:37 PM:
>> 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)
>
>> diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c
>
>> +struct matrix_keymap_data *
>> +matrix_keyboard_of_fill_keymap(struct device_node *np, char *propname)
>> +{
> ...
>> + ? ? 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 <swarren@nvidia.com>

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

Regards,
Simon

>
> --
> nvpublic
>

[PATCH v4] Input: keyboard - add device tree bindings for simple key matrixes

[Dmitry Torokhov]

On Tue, Jan 03, 2012 at 02:37:44PM -0800, Stephen Warren wrote:
> Olof Johansson wrote at Tuesday, January 03, 2012 2:37 PM:
> > 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)
> 
> > diff --git a/drivers/input/of_keymap.c b/drivers/input/of_keymap.c
> 
> > +struct matrix_keymap_data *
> > +matrix_keyboard_of_fill_keymap(struct device_node *np, char *propname)
> > +{
> ...
> > +	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 <swarren@nvidia.com>
> 

Applied, thanks everyone.

-- 
Dmitry

[PATCH] Input: tegra-kbc - add device tree support

[Olof Johansson]

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.

Signed-off-by: Olof Johansson <olof@lixom.net>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
---

This patch depends on "Input: keyboard - add device tree bindings for
simple key matrixes".


-Olof
 .../devicetree/bindings/input/tegra-kbc.txt        |   23 +++++
 drivers/input/keyboard/tegra-kbc.c                 |  103 ++++++++++++++++++--
 2 files changed, 118 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/tegra-kbc.txt

diff --git a/Documentation/devicetree/bindings/input/tegra-kbc.txt b/Documentation/devicetree/bindings/input/tegra-kbc.txt
new file mode 100644
index 0000000..b630eeb4
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/tegra-kbc.txt
@@ -0,0 +1,23 @@
+* Tegra keyboard controller
+
+Required properties:
+- compatible: "nvidia,tegra20-kbc", "matrix-keyboard-controller"
+
+Optional properties, in addition to those specified by
+"matrix-keyboard-controller" bindings:
+
+- linux,fn-keymap: a second keymap, same specification as the
+matrix-keyboard-controller spec but to be used when the KEY_FN modifier
+key is pressed.
+- nvidia,debounce-delay-ms: delay in milliseconds per row scan for debouncing
+- nvidia,repeat-delay-ms: delay in milliseconds before repeat starts
+- nvidia,ghost-filter: enable ghost filtering for this device
+- nvidia,wakeup-source: configure keyboard as a wakeup source for suspend/resume
+
+Example:
+
+keyboard: keyboard {
+	compatible = "nvidia,tegra20-kbc";
+	reg = <0x7000e200 0x100>;
+	nvidia,ghost-filter;
+};
diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
index 08d02f2..0794879 100644
--- a/drivers/input/keyboard/tegra-kbc.c
+++ b/drivers/input/keyboard/tegra-kbc.c
@@ -26,6 +26,7 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/interrupt.h>
+#include <linux/of.h>
 #include <linux/clk.h>
 #include <linux/slab.h>
 #include <mach/clk.h>
@@ -82,7 +83,7 @@ struct tegra_kbc {
 	struct clk *clk;
 };
 
-static const u32 tegra_kbc_default_keymap[] = {
+static const u32 tegra_kbc_default_keymap[] __devinitdata = {
 	KEY(0, 2, KEY_W),
 	KEY(0, 3, KEY_S),
 	KEY(0, 4, KEY_A),
@@ -217,7 +218,8 @@ static const u32 tegra_kbc_default_keymap[] = {
 	KEY(31, 4, KEY_HELP),
 };
 
-static const struct matrix_keymap_data tegra_kbc_default_keymap_data = {
+static const
+struct matrix_keymap_data tegra_kbc_default_keymap_data __devinitdata = {
 	.keymap		= tegra_kbc_default_keymap,
 	.keymap_size	= ARRAY_SIZE(tegra_kbc_default_keymap),
 };
@@ -576,6 +578,62 @@ tegra_kbc_check_pin_cfg(const struct tegra_kbc_platform_data *pdata,
 	return true;
 }
 
+#ifdef CONFIG_OF
+static struct tegra_kbc_platform_data * __devinit
+tegra_kbc_dt_parse_pdata(struct platform_device *pdev)
+{
+	struct tegra_kbc_platform_data *pdata;
+	struct device_node *np = pdev->dev.of_node;
+	u32 prop;
+	int i;
+
+	if (!np)
+		return NULL;
+
+	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	if (!of_property_read_u32(np, "nvidia,debounce-delay-ms", &prop))
+		pdata->debounce_cnt = prop;
+
+	if (!of_property_read_u32(np, "nvidia,repeat-delay-ms", &prop))
+		pdata->repeat_cnt = prop;
+
+	if (of_find_property(np, "nvidia,needs-ghost-filter", NULL))
+		pdata->use_ghost_filter = true;
+
+	if (of_find_property(np, "nvidia,wakeup-source", NULL))
+		pdata->wakeup = true;
+
+	/*
+	 * All currently known keymaps with device tree support use the same
+	 * pin_cfg, so set it up here.
+	 */
+	for (i = 0; i < KBC_MAX_ROW; i++) {
+		pdata->pin_cfg[i].num = i;
+		pdata->pin_cfg[i].is_row = true;
+	}
+
+	for (i = 0; i < KBC_MAX_COL; i++) {
+		pdata->pin_cfg[KBC_MAX_ROW + i].num = i;
+		pdata->pin_cfg[KBC_MAX_ROW + i].is_row = false;
+	}
+
+	pdata->keymap_data = matrix_keyboard_of_fill_keymap(np, "linux,keymap");
+
+	/* FIXME: Add handling of linux,fn-keymap here */
+
+	return pdata;
+}
+#else
+static inline struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata(
+	struct platform_device *pdev)
+{
+	return NULL;
+}
+#endif
+
 static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 {
 	const struct tegra_kbc_platform_data *pdata = pdev->dev.platform_data;
@@ -590,21 +648,28 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 	unsigned int scan_time_rows;
 
 	if (!pdata)
-		return -EINVAL;
+		pdata = tegra_kbc_dt_parse_pdata(pdev);
 
-	if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows))
+	if (!pdata)
 		return -EINVAL;
 
+	if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows)) {
+		err = -EINVAL;
+		goto err_free_pdata;
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
 		dev_err(&pdev->dev, "failed to get I/O memory\n");
-		return -ENXIO;
+		err = -ENXIO;
+		goto err_free_pdata;
 	}
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
 		dev_err(&pdev->dev, "failed to get keyboard IRQ\n");
-		return -ENXIO;
+		err = -ENXIO;
+		goto err_free_pdata;
 	}
 
 	kbc = kzalloc(sizeof(*kbc), GFP_KERNEL);
@@ -693,6 +758,9 @@ static int __devinit tegra_kbc_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, kbc);
 	device_init_wakeup(&pdev->dev, pdata->wakeup);
 
+	if (!pdev->dev.platform_data)
+		matrix_keyboard_of_free_keymap(pdata->keymap_data);
+
 	return 0;
 
 err_free_irq:
@@ -706,6 +774,11 @@ err_free_mem_region:
 err_free_mem:
 	input_free_device(input_dev);
 	kfree(kbc);
+err_free_pdata:
+	if (!pdev->dev.platform_data) {
+		matrix_keyboard_of_free_keymap(pdata->keymap_data);
+		kfree(pdata);
+	}
 
 	return err;
 }
@@ -715,6 +788,8 @@ static int __devexit tegra_kbc_remove(struct platform_device *pdev)
 	struct tegra_kbc *kbc = platform_get_drvdata(pdev);
 	struct resource *res;
 
+	platform_set_drvdata(pdev, NULL);
+
 	free_irq(kbc->irq, pdev);
 	clk_put(kbc->clk);
 
@@ -723,9 +798,14 @@ static int __devexit tegra_kbc_remove(struct platform_device *pdev)
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	release_mem_region(res->start, resource_size(res));
 
-	kfree(kbc);
+	/*
+	 * If we do not have platform data attached to the device we
+	 * allocated it ourselves and thus need to free it.
+	 */
+	if (!pdev->dev.platform_data)
+		kfree(kbc->pdata);
 
-	platform_set_drvdata(pdev, NULL);
+	kfree(kbc);
 
 	return 0;
 }
@@ -793,6 +873,12 @@ static int tegra_kbc_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(tegra_kbc_pm_ops, tegra_kbc_suspend, tegra_kbc_resume);
 
+static const struct of_device_id tegra_kbc_of_match[] = {
+	{ .compatible = "nvidia,tegra20-kbc", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tegra_kbc_of_match);
+
 static struct platform_driver tegra_kbc_driver = {
 	.probe		= tegra_kbc_probe,
 	.remove		= __devexit_p(tegra_kbc_remove),
@@ -800,6 +886,7 @@ static struct platform_driver tegra_kbc_driver = {
 		.name	= "tegra-kbc",
 		.owner  = THIS_MODULE,
 		.pm	= &tegra_kbc_pm_ops,
+		.of_match_table = tegra_kbc_of_match,
 	},
 };
 module_platform_driver(tegra_kbc_driver);
-- 
1.7.8.GIT

[PATCH] Input: tegra-kbc - add device tree support

[Stephen Warren]

Olof Johansson wrote at Tuesday, January 03, 2012 3:28 PM:
> 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.

> diff --git a/Documentation/devicetree/bindings/input/tegra-kbc.txt
...
> +Required properties:
> +- compatible: "nvidia,tegra20-kbc", "matrix-keyboard-controller"

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

Otherwise,

Acked-by: Stephen Warren <swarren@nvidia.com>

-- 
nvpublic

[PATCH] Input: tegra-kbc - add device tree support

[Olof Johansson]

On Tue, Jan 3, 2012 at 2:42 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Olof Johansson wrote at Tuesday, January 03, 2012 3:28 PM:
>> 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.
>
>> diff --git a/Documentation/devicetree/bindings/input/tegra-kbc.txt
> ...
>> +Required properties:
>> +- compatible: "nvidia,tegra20-kbc", "matrix-keyboard-controller"
>
> 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?

> Otherwise,
>
> Acked-by: Stephen Warren <swarren@nvidia.com>

Thanks!

-Olof

[PATCH] Input: tegra-kbc - add device tree support

[Dmitry Torokhov]

On Tuesday, January 03, 2012 02:58:57 PM Olof Johansson wrote:
> On Tue, Jan 3, 2012 at 2:42 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > Olof Johansson wrote at Tuesday, January 03, 2012 3:28 PM:
> >> 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.
> >> 
> >> diff --git a/Documentation/devicetree/bindings/input/tegra-kbc.txt
> > 
> > ...
> > 
> >> +Required properties:
> >> +- compatible: "nvidia,tegra20-kbc", "matrix-keyboard-controller"
> > 
> > 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?

Olof,

Any chance I could get a patch against my 'next' branch? I already applied
patch that did DT bindings without keymap support.

Thanks.

-- 
Dmitry