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

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

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 */
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html